diff --git a/pkg/operator/controller/ingress/controller.go b/pkg/operator/controller/ingress/controller.go index e3eb98bde4..48997acc29 100644 --- a/pkg/operator/controller/ingress/controller.go +++ b/pkg/operator/controller/ingress/controller.go @@ -547,19 +547,24 @@ func (r *reconciler) ensureIngressDeleted(ingress *operatorv1.IngressController) if err := r.deleteWildcardDNSRecord(ingress); err != nil { errs = append(errs, fmt.Errorf("failed to delete wildcard dnsrecord for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) } - if record, err := r.currentWildcardDNSRecord(ingress); err != nil { + haveRec, _, err := r.currentWildcardDNSRecord(ingress) + switch { + case err != nil: errs = append(errs, fmt.Errorf("failed to get current wildcard dnsrecord for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) - } else if record != nil { + case haveRec: errs = append(errs, fmt.Errorf("wildcard dnsrecord exists for ingress %s/%s", ingress.Namespace, ingress.Name)) - } - - if err := r.ensureRouterDeleted(ingress); err != nil { - errs = append(errs, fmt.Errorf("failed to delete deployment for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) - } - if deploy, err := r.currentRouterDeployment(ingress); err != nil { - errs = append(errs, fmt.Errorf("failed to get deployment for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) - } else if deploy != nil { - errs = append(errs, fmt.Errorf("deployment still exists for ingress %s/%s", ingress.Namespace, ingress.Name)) + default: + // The router deployment manages the load-balancer service + // which is used to find the hosted zone id. Delete the deployment + // only when the dnsrecord does not exist. + if err := r.ensureRouterDeleted(ingress); err != nil { + errs = append(errs, fmt.Errorf("failed to delete deployment for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) + } + if haveDepl, _, err := r.currentRouterDeployment(ingress); err != nil { + errs = append(errs, fmt.Errorf("failed to get deployment for ingress %s/%s: %v", ingress.Namespace, ingress.Name, err)) + } else if haveDepl { + errs = append(errs, fmt.Errorf("deployment still exists for ingress %s/%s", ingress.Namespace, ingress.Name)) + } } if len(errs) == 0 { @@ -607,10 +612,13 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d errs = append(errs, err) } - deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig) + haveDepl, deployment, err := r.ensureRouterDeployment(ci, infraConfig, ingressConfig, apiConfig, networkConfig) if err != nil { errs = append(errs, fmt.Errorf("failed to ensure deployment: %v", err)) return utilerrors.NewAggregate(errs) + } else if !haveDepl { + errs = append(errs, fmt.Errorf("failed to get router deployment %s/%s", ci.Namespace, ci.Name)) + return utilerrors.NewAggregate(errs) } trueVar := true @@ -624,11 +632,11 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d var lbService *corev1.Service var wildcardRecord *iov1.DNSRecord - if lb, err := r.ensureLoadBalancerService(ci, deploymentRef, infraConfig); err != nil { + if haveLB, lb, err := r.ensureLoadBalancerService(ci, deploymentRef, infraConfig); err != nil { errs = append(errs, fmt.Errorf("failed to ensure load balancer service for %s: %v", ci.Name, err)) } else { lbService = lb - if record, err := r.ensureWildcardDNSRecord(ci, lbService); err != nil { + if _, record, err := r.ensureWildcardDNSRecord(ci, lbService, haveLB); err != nil { errs = append(errs, fmt.Errorf("failed to ensure wildcard dnsrecord for %s: %v", ci.Name, err)) } else { wildcardRecord = record diff --git a/pkg/operator/controller/ingress/deployment.go b/pkg/operator/controller/ingress/deployment.go index 50a8b36814..74de1102ef 100644 --- a/pkg/operator/controller/ingress/deployment.go +++ b/pkg/operator/controller/ingress/deployment.go @@ -46,23 +46,23 @@ const ( // ensureRouterDeployment ensures the router deployment exists for a given // ingresscontroller. -func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network) (*appsv1.Deployment, error) { - desired, err := desiredRouterDeployment(ci, r.Config.IngressControllerImage, infraConfig, ingressConfig, apiConfig, networkConfig) +func (r *reconciler) ensureRouterDeployment(ci *operatorv1.IngressController, infraConfig *configv1.Infrastructure, ingressConfig *configv1.Ingress, apiConfig *configv1.APIServer, networkConfig *configv1.Network) (bool, *appsv1.Deployment, error) { + haveDepl, current, err := r.currentRouterDeployment(ci) if err != nil { - return nil, fmt.Errorf("failed to build router deployment: %v", err) + return false, nil, err } - current, err := r.currentRouterDeployment(ci) + desired, err := desiredRouterDeployment(ci, r.Config.IngressControllerImage, infraConfig, ingressConfig, apiConfig, networkConfig) if err != nil { - return nil, err + return haveDepl, current, fmt.Errorf("failed to build router deployment: %v", err) } switch { - case desired != nil && current == nil: + case !haveDepl: if err := r.createRouterDeployment(desired); err != nil { - return nil, err + return false, nil, err } - case desired != nil && current != nil: + case haveDepl: if err := r.updateRouterDeployment(current, desired); err != nil { - return nil, err + return true, current, err } } return r.currentRouterDeployment(ci) @@ -847,15 +847,15 @@ func hashableProbe(probe *corev1.Probe) *corev1.Probe { } // currentRouterDeployment returns the current router deployment. -func (r *reconciler) currentRouterDeployment(ci *operatorv1.IngressController) (*appsv1.Deployment, error) { +func (r *reconciler) currentRouterDeployment(ci *operatorv1.IngressController) (bool, *appsv1.Deployment, error) { deployment := &appsv1.Deployment{} if err := r.client.Get(context.TODO(), controller.RouterDeploymentName(ci), deployment); err != nil { if errors.IsNotFound(err) { - return nil, nil + return false, nil, nil } - return nil, err + return false, nil, err } - return deployment, nil + return true, deployment, nil } // createRouterDeployment creates a router deployment. diff --git a/pkg/operator/controller/ingress/dns.go b/pkg/operator/controller/ingress/dns.go index 3390e92f45..62a0ce3b43 100644 --- a/pkg/operator/controller/ingress/dns.go +++ b/pkg/operator/controller/ingress/dns.go @@ -28,38 +28,38 @@ import ( const defaultRecordTTL int64 = 30 // ensureWildcardDNSRecord will create DNS records for the given LB service. -// If service is nil, nothing is done. -func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, service *corev1.Service) (*iov1.DNSRecord, error) { - if service == nil { - return nil, nil +// If service is nil (haveLBS is false), nothing is done. +func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, service *corev1.Service, haveLBS bool) (bool, *iov1.DNSRecord, error) { + if !haveLBS { + return false, nil, nil } - desired := desiredWildcardRecord(ic, service) - current, err := r.currentWildcardDNSRecord(ic) + wantWC, desired := desiredWildcardDNSRecord(ic, service) + haveWC, current, err := r.currentWildcardDNSRecord(ic) if err != nil { - return nil, err + return false, nil, err } switch { - case desired != nil && current == nil: + case wantWC && !haveWC: if err := r.client.Create(context.TODO(), desired); err != nil { - return nil, fmt.Errorf("failed to create dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err) + 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 desired != nil && current != nil: + case wantWC && haveWC: if updated, err := r.updateDNSRecord(current, desired); err != nil { - return nil, fmt.Errorf("failed to update dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err) + return true, current, fmt.Errorf("failed to update dnsrecord %s/%s: %v", desired.Namespace, desired.Name, err) } else if updated { log.Info("updated dnsrecord", "dnsrecord", desired) return r.currentWildcardDNSRecord(ic) } } - return 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, @@ -136,16 +136,16 @@ func desiredWildcardRecord(ic *operatorv1.IngressController, service *corev1.Ser } } -func (r *reconciler) currentWildcardDNSRecord(ic *operatorv1.IngressController) (*iov1.DNSRecord, error) { +func (r *reconciler) currentWildcardDNSRecord(ic *operatorv1.IngressController) (bool, *iov1.DNSRecord, error) { current := &iov1.DNSRecord{} err := r.client.Get(context.TODO(), controller.WildcardDNSRecordName(ic), current) if err != nil { if errors.IsNotFound(err) { - return nil, nil + return false, nil, nil } - return nil, err + return false, nil, err } - return current, nil + return true, current, nil } func (r *reconciler) deleteWildcardDNSRecord(ic *operatorv1.IngressController) error { 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 666253c211..d63e061a74 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -65,33 +65,35 @@ var ( // ensureLoadBalancerService creates an LB service if one is desired but absent. // 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) (*corev1.Service, error) { - desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, infraConfig) +func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, infraConfig *configv1.Infrastructure) (bool, *corev1.Service, error) { + wantLBS, desiredLBService, err := desiredLoadBalancerService(ci, deploymentRef, infraConfig) if err != nil { - return nil, err + return false, nil, err } - currentLBService, err := r.currentLoadBalancerService(ci) + haveLBS, currentLBService, err := r.currentLoadBalancerService(ci) if err != nil { - return nil, err + return false, nil, err } - if desiredLBService != nil && currentLBService == nil { + if wantLBS && !haveLBS { if err := r.client.Create(context.TODO(), desiredLBService); err != nil { - return nil, fmt.Errorf("failed to create load balancer service %s/%s: %v", desiredLBService.Namespace, desiredLBService.Name, err) + return false, nil, fmt.Errorf("failed to create load balancer service %s/%s: %v", desiredLBService.Namespace, desiredLBService.Name, err) } log.Info("created load balancer service", "namespace", desiredLBService.Namespace, "name", desiredLBService.Name) - return desiredLBService, nil + return true, desiredLBService, nil } - return currentLBService, nil + // return haveLBS instead of forcing true here since + // there is no guarantee that currentLBService != nil + return haveLBS, currentLBService, nil } // desiredLoadBalancerService returns the desired LB service for a // 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() @@ -137,20 +139,20 @@ 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 // ingresscontroller. -func (r *reconciler) currentLoadBalancerService(ci *operatorv1.IngressController) (*corev1.Service, error) { +func (r *reconciler) currentLoadBalancerService(ci *operatorv1.IngressController) (bool, *corev1.Service, error) { service := &corev1.Service{} if err := r.client.Get(context.TODO(), controller.LoadBalancerServiceName(ci), service); err != nil { if errors.IsNotFound(err) { - return nil, nil + return false, nil, nil } - return nil, err + return false, nil, err } - return service, nil + return true, service, nil } // finalizeLoadBalancerService removes the "ingress.openshift.io/operator" finalizer @@ -159,11 +161,11 @@ func (r *reconciler) currentLoadBalancerService(ci *operatorv1.IngressController // on existing resources. // TODO: How can we delete this code? func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressController) (bool, error) { - service, err := r.currentLoadBalancerService(ci) + haveLBS, service, err := r.currentLoadBalancerService(ci) if err != nil { return false, err } - if service == nil { + if !haveLBS { return false, nil } // Mutate a copy to avoid assuming we know where the current one came from 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) } } diff --git a/pkg/operator/controller/ingress/monitoring.go b/pkg/operator/controller/ingress/monitoring.go index 9b4187ce49..91c0cf5658 100644 --- a/pkg/operator/controller/ingress/monitoring.go +++ b/pkg/operator/controller/ingress/monitoring.go @@ -37,7 +37,7 @@ func (r *reconciler) ensureServiceMonitor(ic *operatorv1.IngressController, svc } case haveSM: if updated, err := r.updateServiceMonitor(current, desired); err != nil { - return true, nil, fmt.Errorf("failed to update servicemonitor %s/%s: %v", desired.GetNamespace(), desired.GetName(), err) + return true, current, fmt.Errorf("failed to update servicemonitor %s/%s: %v", desired.GetNamespace(), desired.GetName(), err) } else if updated { log.Info("updated servicemonitor", "namespace", desired.GetNamespace(), "name", desired.GetName()) } diff --git a/pkg/operator/controller/ingress/nodeport_service.go b/pkg/operator/controller/ingress/nodeport_service.go index 1b6919e4c0..b25382ab1c 100644 --- a/pkg/operator/controller/ingress/nodeport_service.go +++ b/pkg/operator/controller/ingress/nodeport_service.go @@ -49,7 +49,7 @@ func (r *reconciler) ensureNodePortService(ic *operatorv1.IngressController, dep log.Info("created NodePort service", "service", desired) case wantService && haveService: if updated, err := r.updateNodePortService(current, desired); err != nil { - return true, nil, fmt.Errorf("failed to update NodePort service: %v", err) + return true, current, fmt.Errorf("failed to update NodePort service: %v", err) } else if updated { log.Info("updated NodePort service", "service", desired) } diff --git a/pkg/operator/controller/ingress/poddisruptionbudget.go b/pkg/operator/controller/ingress/poddisruptionbudget.go index 3f5ff346e1..b20877617d 100644 --- a/pkg/operator/controller/ingress/poddisruptionbudget.go +++ b/pkg/operator/controller/ingress/poddisruptionbudget.go @@ -49,7 +49,7 @@ func (r *reconciler) ensureRouterPodDisruptionBudget(ic *operatorv1.IngressContr log.Info("created pod disruption budget", "poddisruptionbudget", desired) case wantPDB && havePDB: if updated, err := r.updateRouterPodDisruptionBudget(current, desired); err != nil { - return true, nil, fmt.Errorf("failed to update pod disruption budget: %v", err) + return true, current, fmt.Errorf("failed to update pod disruption budget: %v", err) } else if updated { log.Info("updated pod disruption budget", "poddisruptionbudget", desired) } diff --git a/pkg/operator/controller/ingress/rsyslog_configmap.go b/pkg/operator/controller/ingress/rsyslog_configmap.go index 39c262936f..97472796ec 100644 --- a/pkg/operator/controller/ingress/rsyslog_configmap.go +++ b/pkg/operator/controller/ingress/rsyslog_configmap.go @@ -55,7 +55,7 @@ func (r *reconciler) ensureRsyslogConfigMap(ic *operatorv1.IngressController, de } case wantCM && haveCM: if updated, err := r.updateRsyslogConfigMap(current, desired); err != nil { - return true, nil, fmt.Errorf("failed to update configmap: %v", err) + return true, current, fmt.Errorf("failed to update configmap: %v", err) } else if updated { log.Info("updated configmap", "configmap", desired) } diff --git a/pkg/operator/controller/ingress/serviceca_configmap.go b/pkg/operator/controller/ingress/serviceca_configmap.go index ec231344af..e0aa42f6db 100644 --- a/pkg/operator/controller/ingress/serviceca_configmap.go +++ b/pkg/operator/controller/ingress/serviceca_configmap.go @@ -43,7 +43,7 @@ func (r *reconciler) ensureServiceCAConfigMap() (bool, *corev1.ConfigMap, error) log.Info("created configmap", "configmap", desired) case wantCM && haveCM: if updated, err := r.updateServiceCAConfigMap(current, desired); err != nil { - return true, nil, fmt.Errorf("failed to update configmap: %v", err) + return true, current, fmt.Errorf("failed to update configmap: %v", err) } else if updated { log.Info("updated configmap", "configmap", desired) }