controller: Make ensure conditions consistent #408

sgreene570 · 2020-06-10T16:15:32Z

Switches all ensure functions in pkg/operator/controller/ingress
to use a have boolean rather than manually checking if
!= nil for consistency.

Also makes sure that failed update branches in each ensure<thing> function return the current object, if available, as mentioned in openshift/cluster-dns-operator#176.

Miciah · 2020-06-10T19:11:12Z

pkg/operator/controller/ingress/controller.go

@@ -607,7 +607,7 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
 		errs = append(errs, err)
 	}

-	deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig)
+	_, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig)


This change highlights some questionable logic—it should be verifying that ensureRouterDeployment did indeed return a deployment. Granted, it is unlikely that the deployment would not exist at this point if err is nil (looking at ensureRouterDeployment, the only way both deployment and err could be nil would be if the API let us get or create the deployment in one API call, and then it reported not-found in the following get request), but we shouldn't and needn't assume that it does. Does that make sense?

So, if I'm understanding correctly, the way to rectify this problem would be to check that ensureRouterDeployment returns true for it's first return value (rather than ditch the value with _)? Would changing line 611 to if (err != nil || !haveDeployment) { be sufficient?

Ah, well line 612 formats an error message with err, which could be null if haveDeployment were false. So it would probably be best to just add another if statement, but I digress.

Yeah, I'd add an else if !haveDeployment clause that appended an appropriate error message to errs and returned (to avoid dereferencing a nil deployment further down).

There was a second instance of this in this PR, which I have also corrected, so thanks!

Miciah · 2020-06-10T21:31:31Z

pkg/operator/controller/ingress/controller.go

 			errs = append(errs, fmt.Errorf("failed to ensure wildcard dnsrecord for %s: %v", ci.Name, err))
+		} else if !haveWC {
+			errs = append(errs, fmt.Errorf("failed to get wildcard dnsrecord for %s", ci.Name))


This case is trickier because the ingresscontroller should always have a deployment, but sometimes it shouldn't have a load-balancer service. In particular, if the ingresscontroller's endpoint publishing strategy type is not "LoadBalancerService", then lbService should be nil here, and given a nil lbService, ensureWildcardDNSRecord should return false, nil, nil. (Further down, we assign record to wildcardRecord and pass the latter to syncIngressControllerStatus, which knows what to do with a nil value.)

I'd suggest that ensureLoadBalancerService should return a Boolean value, and as long as ensureLoadBalancerService returns a nil error value, we should call ensureWildcardDNSRecord and pass it the Boolean value from ensureLoadBalancerService.

Ah, my mistake! Your suggestion definitely seems reasonable. I believe I was able to amend this PR properly, but let me know if I missed something again. Thanks! 😃

Switches all ensure<thing> functions in pkg/operator/controller/ingress to use a have<thing> boolean rather than manually checking if <thing> != nil for consistency.

Miciah · 2020-06-10T23:10:29Z

/lgtm
Thanks!

openshift-ci-robot · 2020-06-10T23:11:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, sgreene570

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah]

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

Miciah · 2020-06-11T00:21:40Z

e2e-aws-operator failed on TestRouteAdmissionPolicy; the failure is probably not caused by the changes in this PR (see #400).

openshift-bot · 2020-06-11T04:08:47Z

/retest