controller: Make ensure conditions consistent

pkg/operator/controller/ingress/controller.go

@@ -547,18 +547,18 @@ 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 {
+	if haveRec, _, err := r.currentWildcardDNSRecord(ingress); 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 {
+	} else if 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 {
+	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 deploy != nil {
+	} else if haveDepl {
 		errs = append(errs, fmt.Errorf("deployment still exists for ingress %s/%s", ingress.Namespace, ingress.Name))
 	}
 
@@ -607,10 +607,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 +627,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)
@@ -846,15 +846,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,35 +28,35 @@ 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)
+	haveWC, current, err := r.currentWildcardDNSRecord(ic)
 	if err != nil {
-		return nil, err
+		return false, nil, err
 	}
 
 	switch {
-	case desired != nil && current == nil:
+	case !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 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 true, current, nil
 }
 
 // ensureWildcardDNSRecord will return any necessary wildcard DNS records for the
@@ -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/load_balancer_service.go

@@ -65,24 +65,26 @@ 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) {
+func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, deploymentRef metav1.OwnerReference, infraConfig *configv1.Infrastructure) (bool, *corev1.Service, error) {
 	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 desiredLBService != nil && !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
@@ -142,15 +144,15 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef
 
 // 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/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)
 		}

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