[release-4.5] Bug 1878148: Backport: Delete Router Deployment Only Upon DNSRecord Deletion

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

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.

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,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,
@@ -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 {

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

@@ -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

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)
 		}
 	}

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())
 		}

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)
 		}

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)
 		}

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)
 		}