externalize ipfailover command by juanvallejo · Pull Request #20962 · openshift/origin

juanvallejo · 2018-09-12T20:37:09Z

Updates the ipfailover command, as well as the keepalived plugin to use external versions.

soltysh · 2018-09-13T20:42:18Z

pkg/oc/cli/admin/ipfailover/ipfailover.go

-		NetworkInterface: ipfailover.DefaultInterface,
-		VRRPIDOffset:     0,
-		Replicas:         1,
+type IPFailoverConfigCmdOptions struct {


IPFailoverConfigOptions - we don't use Cmd anywhere, we should not introduce it here. Honestly, since the commands name is ipfailover the struct name should be just IPFailoverOptions.

+1 on IPFailoverOptions

soltysh · 2018-09-13T20:42:42Z

pkg/oc/cli/admin/ipfailover/ipfailover.go

-	options.Action.BindForOutput(cmd.Flags())
-	cmd.Flags().String("output-version", "", "The preferred API versions of the output objects")
-
+	cmd.Flags().StringVar(&o.ConfigOptions.Type, "type", ipfailover.DefaultType, "The type of IP failover configurator to use.")


This default should be set in New... method, not here.

soltysh · 2018-09-13T20:43:08Z

pkg/oc/cli/admin/ipfailover/ipfailover.go

+
+	cmd.Flags().BoolVar(&o.ConfigOptions.Create, "create", o.ConfigOptions.Create, "If true, create the configuration if it does not exist.")
+
+	cmd.Flags().StringVar(&o.ConfigOptions.VirtualIPs, "virtual-ips", "", "A set of virtual IP ranges and/or addresses that the routers bind and serve on and provide IP failover capability for.")


Ditto here and in the next few lines wrt to defaults.

soltysh · 2018-09-13T20:43:18Z

pkg/oc/cli/admin/ipfailover/ipfailover.go

+	cmd.Flags().StringVar(&o.ConfigOptions.IptablesChain, "iptables-chain", ipfailover.DefaultIptablesChain, "Add a rule to this iptables chain to accept 224.0.0.28 multicast packets if no rule exists. When iptables-chain is empty do not change iptables.")
+	cmd.Flags().StringVarP(&o.ConfigOptions.NetworkInterface, "interface", "i", "", "Network interface bound by VRRP to use for the set of virtual IP ranges/addresses specified.")
+
+	cmd.Flags().IntVarP(&o.ConfigOptions.WatchPort, "watch-port", "w", ipfailover.DefaultWatchPort, "Port to monitor or watch for resource availability.")


soltysh · 2018-09-13T20:46:00Z

pkg/oc/cli/admin/ipfailover/ipfailover.go


-	configList := []runtime.Object{
-		&kapi.ServiceAccount{ObjectMeta: metav1.ObjectMeta{Name: options.ServiceAccount}},
+	configList := []runtime.RawExtension{


Instead of dealing with corev1.List, wouldn't it be simpler to work with []runtime.Object?

soltysh · 2018-09-13T20:50:25Z

pkg/oc/cli/admin/ipfailover/ipfailover.go

+			continue
+		}
+
+		_, err = o.DynamicClient.Resource(mapping.Resource).Namespace(namespace).Create(unstructuredObj)


Please use short if, where possible, this is a perfect candidate. Btw. if you don't know that yet @pweil- calls me short-if 👮‍♂️

soltysh · 2018-09-13T20:51:33Z

pkg/oc/cli/admin/ipfailover/ipfailover/options.go

+
+func NewIPFailoverConfigOptions() *IPFailoverConfigOptions {
+	return &IPFailoverConfigOptions{
+


Unnecessary empty line 😉

soltysh · 2018-09-13T20:52:11Z

pkg/oc/cli/admin/ipfailover/keepalived/plugin.go

 	}

-	configList := &kapi.List{Items: []runtime.Object{dc}}
+	configList := &corev1.List{


Yeah, working with []runtime.Object might simplify these.

soltysh · 2018-09-13T20:53:07Z

pkg/oc/cli/admin/ipfailover/validation.go

+func validateServiceAccount(client securityv1typedclient.SecurityV1Interface, serviceAccount string) error {
+	sccList, err := client.SecurityContextConstraints().List(metav1.ListOptions{})
+	if err != nil {
+		if !errors.IsUnauthorized(err) {


if err != nil && errors.IsUnathorized(err) { ...

???

Heh, will fix this up. I pretty much just copy + pasted it exactly as it was before this PR (it used to live in the ipfailover.go file)

soltysh · 2018-09-13T20:55:11Z

test/cmd/router.sh


 # test ipfailover
-os::cmd::expect_failure_and_text 'oc adm ipfailover --dry-run' 'you must specify at least one virtual IP address'
+os::cmd::expect_failure_and_text 'oc adm ipfailover --dry-run' 'service account "ipfailover" does not have sufficient privileges'


That's a concerning change

Ah, the reason for this change was because before, we called the Generate method on the ipfailover plugin before validating the service account.

After this update, we validate the service account before we call the Generate method on the ipfailover plugin. The validation for the service account, now happens in the command's Validate method, which is called before the Run method. The Generate method is called inside of the Run method now.

No behavior changes here, as service account validation used to fail before this update with the same error, until a security context was created for it.

Thanks for detailed explanation.

soltysh

/lgtm
/approve

openshift-ci-robot · 2018-09-17T08:28:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanvallejo, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~contrib/completions/OWNERS~~ [juanvallejo,soltysh]
~~pkg/oc/OWNERS~~ [soltysh]
~~test/cmd/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Sep 12, 2018

openshift-ci-robot requested review from enj and stevekuznetsov September 12, 2018 20:37

juanvallejo force-pushed the jvallejo/externalize-ipfailover branch 2 times, most recently from c472321 to 915c3bb Compare September 12, 2018 21:54

soltysh suggested changes Sep 13, 2018

View reviewed changes

externalize ipfailover command

d59f50a

juanvallejo force-pushed the jvallejo/externalize-ipfailover branch from 915c3bb to d59f50a Compare September 17, 2018 01:49

openshift-ci-robot assigned soltysh Sep 17, 2018

soltysh approved these changes Sep 17, 2018

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 17, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2018

openshift-merge-robot merged commit d3fbfc8 into openshift:master Sep 17, 2018

juanvallejo deleted the jvallejo/externalize-ipfailover branch September 17, 2018 13:58


		cmd.Flags().BoolVar(&o.ConfigOptions.Create, "create", o.ConfigOptions.Create, "If true, create the configuration if it does not exist.")

		cmd.Flags().StringVar(&o.ConfigOptions.VirtualIPs, "virtual-ips", "", "A set of virtual IP ranges and/or addresses that the routers bind and serve on and provide IP failover capability for.")


		func NewIPFailoverConfigOptions() *IPFailoverConfigOptions {
		return &IPFailoverConfigOptions{

Conversation

juanvallejo commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Sep 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants