From c5d6aee8c3793b933f72e5d281db4c701cf83a8a Mon Sep 17 00:00:00 2001 From: Stephen Greene Date: Mon, 15 Jun 2020 09:50:51 -0400 Subject: [PATCH] Fix ensureWildcardDNSRecord & ensureLoadBalancerService Add a `wantFoo` boolean to `ensureWildcardDNSRecord` & `ensureLoadBalancerService` for consistency. Fixes a bug accidentally introduced in #408. --- pkg/operator/controller/ingress/dns.go | 22 +++++++++---------- pkg/operator/controller/ingress/dns_test.go | 8 +++---- .../ingress/load_balancer_service.go | 10 ++++----- .../ingress/load_balancer_service_test.go | 6 ++--- 4 files changed, 23 insertions(+), 23 deletions(-) diff --git a/pkg/operator/controller/ingress/dns.go b/pkg/operator/controller/ingress/dns.go index 3e3b2dc74a..62a0ce3b43 100644 --- a/pkg/operator/controller/ingress/dns.go +++ b/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,21 +68,21 @@ 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] @@ -90,7 +90,7 @@ func desiredWildcardRecord(ic *operatorv1.IngressController, service *corev1.Ser // 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, diff --git a/pkg/operator/controller/ingress/dns_test.go b/pkg/operator/controller/ingress/dns_test.go index 4a0634ce7a..2edd5edc4a 100644 --- a/pkg/operator/controller/ingress/dns_test.go +++ b/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)) } } diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index 745ff53a25..d63e061a74 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/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 diff --git a/pkg/operator/controller/ingress/load_balancer_service_test.go b/pkg/operator/controller/ingress/load_balancer_service_test.go index 531f613115..1ce291fd87 100644 --- a/pkg/operator/controller/ingress/load_balancer_service_test.go +++ b/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) } }