[release-4.2] Bug 1769882: Ensures LB service finalizer is removed

pkg/operator/controller/ingress/controller.go

@@ -337,18 +337,18 @@ func validateDomainUniqueness(desired *operatorv1.IngressController, existing []
 // ensureIngressDeleted tries to delete ingress, and if successful, will remove
 // the finalizer.
 func (r *reconciler) ensureIngressDeleted(ingress *operatorv1.IngressController) error {
+	errs := []error{}
 	if err := r.finalizeLoadBalancerService(ingress); err != nil {
-		return fmt.Errorf("failed to finalize load balancer service for %s: %v", ingress.Name, err)
+		errs = append(errs, fmt.Errorf("failed to finalize load balancer service for %s/%s: %v", ingress.Namespace, ingress.Name, err))
 	}
-	log.Info("finalized load balancer service for ingress", "namespace", ingress.Namespace, "name", ingress.Name)
 
 	// Delete the wildcard DNS record, and block ingresscontroller finalization
 	// until the dnsrecord has been finalized.
 	if err := r.deleteWildcardDNSRecord(ingress); err != nil {
-		return fmt.Errorf("failed to delete wildcard dnsrecord: %v", err)
+		errs = append(errs, fmt.Errorf("failed to delete wildcard dnsrecord: %v", err))
 	}
 	if record, err := r.currentWildcardDNSRecord(ingress); err != nil {
-		return fmt.Errorf("failed to get current wildcard dnsrecord: %v", err)
+		errs = append(errs, fmt.Errorf("failed to get current wildcard dnsrecord: %v", err))
 	} else {
 		if record != nil {
 			log.V(1).Info("waiting for wildcard dnsrecord to be deleted", "dnsrecord", record)
@@ -357,19 +357,21 @@ func (r *reconciler) ensureIngressDeleted(ingress *operatorv1.IngressController)
 	}
 
 	if err := r.ensureRouterDeleted(ingress); err != nil {
-		return fmt.Errorf("failed to delete deployment for ingress %s: %v", ingress.Name, err)
+		errs = append(errs, fmt.Errorf("failed to delete deployment for ingress %s: %v", ingress.Name, err))
 	}
-	log.Info("deleted deployment for ingress", "namespace", ingress.Namespace, "name", ingress.Name)
 
-	// Clean up the finalizer to allow the ingresscontroller to be deleted.
-	if slice.ContainsString(ingress.Finalizers, manifests.IngressControllerFinalizer) {
-		updated := ingress.DeepCopy()
-		updated.Finalizers = slice.RemoveString(updated.Finalizers, manifests.IngressControllerFinalizer)
-		if err := r.client.Update(context.TODO(), updated); err != nil {
-			return fmt.Errorf("failed to remove finalizer from ingresscontroller %s: %v", ingress.Name, err)
+	if len(errs) == 0 {
+		// Remove the "ingresscontroller.operator.openshift.io/finalizer-ingresscontroller" finalizer
+		// to allow the ingresscontroller to be deleted.
+		if slice.ContainsString(ingress.Finalizers, manifests.IngressControllerFinalizer) {
+			updated := ingress.DeepCopy()
+			updated.Finalizers = slice.RemoveString(updated.Finalizers, manifests.IngressControllerFinalizer)
+			if err := r.client.Update(context.TODO(), updated); err != nil {
+				errs = append(errs, fmt.Errorf("failed to remove finalizer from ingresscontroller %s: %v", ingress.Name, err))
+			}
 		}
 	}
-	return nil
+	return utilerrors.NewAggregate(errs)
 }
 
 // ensureIngressController ensures all necessary router resources exist for a given ingresscontroller.

pkg/operator/controller/ingress/deployment.go

@@ -60,6 +60,8 @@ func (r *reconciler) ensureRouterDeleted(ci *operatorv1.IngressController) error
 			return err
 		}
 	}
+	log.Info("deleted deployment", "namespace", deployment.Namespace, "name", deployment.Name)
+	r.recorder.Eventf(ci, "Normal", "DeletedDeployment", "Deleted deployment %s/%s", deployment.Namespace, deployment.Name)
 	return nil
 }
 

pkg/operator/controller/ingress/load_balancer_service.go

@@ -23,6 +23,10 @@ const (
 	//
 	// https://kubernetes.io/docs/concepts/services-networking/service/#proxy-protocol-support-on-aws
 	awsLBProxyProtocolAnnotation = "service.beta.kubernetes.io/aws-load-balancer-proxy-protocol"
+	// lbServiceCleanUpFinalizer is a finalizer attached to any service that has type=LoadBalancer.
+	// Upon deletion of the service, the actual deletion of the resource will be blocked until
+	// this finalizer is removed.
+	lbServiceCleanUpFinalizer = "service.kubernetes.io/load-balancer-cleanup"
 )
 
 var (
@@ -135,10 +139,10 @@ func (r *reconciler) currentLoadBalancerService(ci *operatorv1.IngressController
 	return service, nil
 }
 
-// finalizeLoadBalancerService removes finalizers from any LB service. This used
-// to be to help with DNS cleanup, but now that's no longer necessary, and so we
-// just need to clear the finalizer which might exist on existing resources.
-//
+// finalizeLoadBalancerService removes the "ingress.openshift.io/operator" finalizer
+// from the load balancer service of ci. This was for helping with DNS cleanup, but
+// that's no longer necessary. We just need to clear the finalizer which might exist
+// on existing resources.
 // TODO: How can we delete this code?
 func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressController) error {
 	service, err := r.currentLoadBalancerService(ci)
@@ -157,5 +161,11 @@ func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressControlle
 			return fmt.Errorf("failed to remove finalizer from service %s for ingress %s/%s: %v", service.Namespace, service.Name, ci.Name, err)
 		}
 	}
+	// The load balancer service is not deleted until the cloud infra is cleaned-up. For more details, see:
+	// https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/20190423-service-lb-finalizer.md
+	if slice.ContainsString(service.Finalizers, lbServiceCleanUpFinalizer) {
+		return fmt.Errorf("finalizer %s exists for service %s/%s", lbServiceCleanUpFinalizer, service.Namespace, service.Name)
+	}
+	log.Info("finalized load balancer service for ingress", "namespace", ci.Namespace, "name", ci.Name)
 	return nil
 }