controller: Make ensure conditions consistent

Switches all ensure<thing> functions in pkg/operator/controller/ingress to use a have<thing> boolean rather than manually checking if <thing> != nil for consistency.
openshift · Jun 10, 2020 · b7786e1 · b7786e1
1 parent 93fc3dd
commit b7786e1
Show file tree

Hide file tree

Showing 8 changed files with 42 additions and 37 deletions.
diff --git a/pkg/operator/controller/ingress/controller.go b/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
@@ -628,8 +631,10 @@ func (r *reconciler) ensureIngressController(ci *operatorv1.IngressController, d
 		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 haveWC, record, err := r.ensureWildcardDNSRecord(ci, lbService); err != nil {
 			errs = append(errs, fmt.Errorf("failed to ensure wildcard dnsrecord for %s: %v", ci.Name, err))
+		} else if !haveWC {
+			errs = append(errs, fmt.Errorf("failed to get wildcard dnsrecord for %s", ci.Name))
 		} else {
 			wildcardRecord = record
 		}

diff --git 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)
@@ -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.

diff --git a/pkg/operator/controller/ingress/dns.go b/pkg/operator/controller/ingress/dns.go
@@ -29,34 +29,34 @@ 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) {
+func (r *reconciler) ensureWildcardDNSRecord(ic *operatorv1.IngressController, service *corev1.Service) (bool, *iov1.DNSRecord, error) {
 	if service == nil {
-		return nil, nil
+		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 {

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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)
 		}