Fix ensureWildcardDNSRecord & ensureLoadBalancerService

pkg/operator/controller/ingress/dns.go

@@ -34,20 +34,20 @@ func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, s
 		return false, nil, nil
 	}
 
-	desired := desiredWildcardRecord(ic, service)
+	wantWC, desired := desiredWildcardDNSRecord(ic, service)
 	haveWC, current, err := r.currentWildcardDNSRecord(ic)
 	if err != nil {
 		return false, nil, err
 	}
 
 	switch {
-	case !haveWC:
+	case wantWC && !haveWC:
 		if err := r.client.Create(context.TODO(), desired); err != nil {
 			return false, nil, fmt.Errorf("failed to create dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err)
 		}
 		log.Info("created dnsrecord", "dnsrecord", desired)
 		return r.currentWildcardDNSRecord(ic)
-	case haveWC:
+	case wantWC && haveWC:
 		if updated, err := r.updateDNSRecord(current, desired); err != nil {
 			return true, current, fmt.Errorf("failed to update dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err)
 		} else if updated {
@@ -56,10 +56,10 @@ func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, s
 		}
 	}
 
-	return true, current, nil
+	return haveWC, current, nil
 }
 
-// ensureWildcardDNSRecord will return any necessary wildcard DNS records for the
+// desiredWildcardDNSRecord will return any necessary wildcard DNS records for the
 // ingresscontroller.
 //
 // For now, if the service has more than one .status.loadbalancer.ingress, only
@@ -68,29 +68,29 @@ func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, s
 // TODO: If .status.loadbalancer.ingress is processed once as non-empty and then
 // later becomes empty, what should we do? Currently we'll treat it as an intent
 // to not have a desired record.
-func desiredWildcardRecord(ic *operatorv1.IngressController, service *corev1.Service) *iov1.DNSRecord {
+func desiredWildcardDNSRecord(ic *operatorv1.IngressController, service *corev1.Service) (bool, *iov1.DNSRecord) {
 	// If the ingresscontroller has no ingress domain, we cannot configure any
 	// DNS records.
 	if len(ic.Status.Domain) == 0 {
-		return nil
+		return false, nil
 	}
 
 	// DNS is only managed for LB publishing.
 	if ic.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType {
-		return nil
+		return false, nil
 	}
 
 	// No LB target exists for the domain record to point at.
 	if len(service.Status.LoadBalancer.Ingress) == 0 {
-		return nil
+		return false, nil
 	}
 
 	ingress := service.Status.LoadBalancer.Ingress[0]
 
 	// Quick sanity check since we don't know how to handle both being set (is
 	// that even a valid state?)
 	if len(ingress.Hostname) > 0 && len(ingress.IP) > 0 {
-		return nil
+		return false, nil
 	}
 
 	name := controller.WildcardDNSRecordName(ic)
@@ -108,7 +108,7 @@ func desiredWildcardRecord(ic *operatorv1.IngressController, service *corev1.Ser
 	}
 
 	trueVar := true
-	return &iov1.DNSRecord{
+	return true, &iov1.DNSRecord{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: name.Namespace,
 			Name:      name.Name,

pkg/operator/controller/ingress/dns_test.go

@@ -95,15 +95,15 @@ func TestDesiredWildcardDNSRecord(t *testing.T) {
 			service.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, ingress)
 		}
 
-		actual := desiredWildcardRecord(controller, service)
+		haveWC, actual := desiredWildcardDNSRecord(controller, service)
 		switch {
-		case test.expect != nil && actual != nil:
+		case test.expect != nil && haveWC:
 			if !cmp.Equal(actual.Spec, *test.expect) {
 				t.Errorf("expected:\n%s\n\nactual:\n%s", toYaml(test.expect), toYaml(actual.Spec))
 			}
-		case test.expect == nil && actual != nil:
+		case test.expect == nil && haveWC:
 			t.Errorf("expected nil record, got:\n%s", toYaml(actual))
-		case test.expect != nil && actual == nil:
+		case test.expect != nil && !haveWC:
 			t.Errorf("expected record but got nil:\n%s", toYaml(test.expect))
 		}
 	}

pkg/operator/controller/ingress/load_balancer_service.go

@@ -66,7 +66,7 @@ var (
 // Always returns the current LB service if one exists (whether it already
 // existed or was created during the course of the function).
 func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, infraConfig *configv1.Infrastructure) (bool, *corev1.Service, error) {
-	desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, infraConfig)
+	wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, infraConfig)
 	if err != nil {
 		return false, nil, err
 	}
@@ -75,7 +75,7 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController,
 	if err != nil {
 		return false, nil, err
 	}
-	if desiredLBService != nil && !haveLBS {
+	if wantLBS && !haveLBS {
 		if err := r.client.Create(context.TODO(), desiredLBService); err != nil {
 			return false, nil, fmt.Errorf("failed to create load balancer service %s/%s: %v", desiredLBService.Namespace, desiredLBService.Name, err)
 		}
@@ -91,9 +91,9 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController,
 // ingresscontroller, or nil if an LB service isn't desired. An LB service is
 // desired if the high availability type is Cloud. An LB service will declare an
 // owner reference to the given deployment.
-func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, infraConfig *configv1.Infrastructure) (*corev1.Service, error) {
+func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, infraConfig *configv1.Infrastructure) (bool, *corev1.Service, error) {
 	if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType {
-		return nil, nil
+		return false, nil, nil
 	}
 	service := manifests.LoadBalancerService()
 
@@ -139,7 +139,7 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 
 	service.SetOwnerReferences([]metav1.OwnerReference{deploymentRef})
 	service.Finalizers = []string{manifests.LoadBalancerServiceFinalizer}
-	return service, nil
+	return true, service, nil
 }
 
 // currentLoadBalancerService returns any existing LB service for the

pkg/operator/controller/ingress/load_balancer_service_test.go

@@ -49,12 +49,12 @@ func TestDesiredLoadBalancerService(t *testing.T) {
 			},
 		}
 
-		svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig)
+		haveSvc, svc, err := desiredLoadBalancerService(ic, deploymentRef, infraConfig)
 		if err != nil {
 			t.Errorf("unexpected error from desiredLoadBalancerService for endpoint publishing strategy type %v: %v", tc.strategyType, err)
-		} else if tc.expect && svc == nil {
+		} else if tc.expect && !haveSvc {
 			t.Errorf("expected desiredLoadBalancerService to return a service for endpoint publishing strategy type %v, got nil", tc.strategyType)
-		} else if !tc.expect && svc != nil {
+		} else if !tc.expect && haveSvc {
 			t.Errorf("expected desiredLoadBalancerService to return nil service for endpoint publishing strategy type %v, got %#v", tc.strategyType, svc)
 		}
 	}