From 081081a3eb2aea672319c047eba0623b8db41c44 Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Wed, 10 Feb 2021 23:10:27 -0500 Subject: [PATCH] Delete the ingress.openshift.io/operator finalizer Add logic to remove the operator's finalizer on LoadBalancer services, which is no longer needed to clean up DNS records since the DNSRecord CRD was added. This commit fixes bug 1914127. https://bugzilla.redhat.com/show_bug.cgi?id=1914127 * pkg/operator/controller/ingress/load_balancer_service.go (ensureLoadBalancerService): Use the new deleteLoadBalancerService method to delete the service as needed. Use the new deleteLoadBalancerServiceFinalizer method to delete any finalizer on any existing service in the delete and update cases. (desiredLoadBalancerService): Delete logic to add a finalizer to the service. (deleteLoadBalancerServiceFinalizer): New method. Delete any finalizer from the service. (finalizeLoadBalancerService): Refactor to use the new deleteLoadBalancerServiceFinalizer method. (deleteLoadBalancerService): New method. --- .../ingress/load_balancer_service.go | 76 +++++++++++++++---- 1 file changed, 60 insertions(+), 16 deletions(-) diff --git a/pkg/operator/controller/ingress/load_balancer_service.go b/pkg/operator/controller/ingress/load_balancer_service.go index cc718b6930..bfe95a24af 100644 --- a/pkg/operator/controller/ingress/load_balancer_service.go +++ b/pkg/operator/controller/ingress/load_balancer_service.go @@ -16,6 +16,8 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + crclient "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -149,14 +151,32 @@ func (r *reconciler) ensureLoadBalancerService(ci *operatorv1.IngressController, case !wantLBS && !haveLBS: return false, nil, nil case !wantLBS && haveLBS: - // TODO: Delete the service. - return true, currentLBService, nil + if deletedFinalizer, err := r.deleteLoadBalancerServiceFinalizer(currentLBService); err != nil { + return true, currentLBService, fmt.Errorf("failed to remove finalizer from load balancer service: %v", err) + } else if deletedFinalizer { + haveLBS, currentLBService, err = r.currentLoadBalancerService(ci) + if err != nil { + return haveLBS, currentLBService, err + } + } + if err := r.deleteLoadBalancerService(currentLBService, &crclient.DeleteOptions{}); err != nil { + return true, currentLBService, err + } + return false, nil, nil case wantLBS && !haveLBS: if err := r.createLoadBalancerService(desiredLBService); err != nil { return false, nil, err } return r.currentLoadBalancerService(ci) case wantLBS && haveLBS: + if deletedFinalizer, err := r.deleteLoadBalancerServiceFinalizer(currentLBService); err != nil { + return true, currentLBService, fmt.Errorf("failed to remove finalizer from load balancer service: %v", err) + } else if deletedFinalizer { + haveLBS, currentLBService, err = r.currentLoadBalancerService(ci) + if err != nil { + return haveLBS, currentLBService, err + } + } if updated, err := r.updateLoadBalancerService(currentLBService, desiredLBService, platform); err != nil { return true, currentLBService, fmt.Errorf("failed to update load balancer service: %v", err) } else if updated { @@ -239,7 +259,6 @@ func desiredLoadBalancerService(ci *operatorv1.IngressController, deploymentRef } service.SetOwnerReferences([]metav1.OwnerReference{deploymentRef}) - service.Finalizers = []string{manifests.LoadBalancerServiceFinalizer} return true, service, nil } @@ -256,11 +275,34 @@ func (r *reconciler) currentLoadBalancerService(ci *operatorv1.IngressController return true, service, nil } +// deleteLoadBalancerServiceFinalizer removes the +// "ingress.openshift.io/operator" finalizer from the provided load balancer +// service. This finalizer used to be needed 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: Delete this method and all calls to it after 4.8. +func (r *reconciler) deleteLoadBalancerServiceFinalizer(service *corev1.Service) (bool, error) { + if !slice.ContainsString(service.Finalizers, manifests.LoadBalancerServiceFinalizer) { + return false, nil + } + + // Mutate a copy to avoid assuming we know where the current one came from + // (i.e. it could have been from a cache). + updated := service.DeepCopy() + updated.Finalizers = slice.RemoveString(updated.Finalizers, manifests.LoadBalancerServiceFinalizer) + if err := r.client.Update(context.TODO(), updated); err != nil { + return false, fmt.Errorf("failed to remove finalizer from service %s/%s: %v", service.Namespace, service.Name, err) + } + + log.Info("removed finalizer from load balancer service", "namespace", service.Namespace, "name", service.Name) + + return true, nil +} + // 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) (bool, error) { haveLBS, service, err := r.currentLoadBalancerService(ci) if err != nil { @@ -269,18 +311,8 @@ func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressControlle if !haveLBS { return false, nil } - // Mutate a copy to avoid assuming we know where the current one came from - // (i.e. it could have been from a cache). - updated := service.DeepCopy() - if slice.ContainsString(updated.Finalizers, manifests.LoadBalancerServiceFinalizer) { - updated.Finalizers = slice.RemoveString(updated.Finalizers, manifests.LoadBalancerServiceFinalizer) - if err := r.client.Update(context.TODO(), updated); err != nil { - return true, fmt.Errorf("failed to remove finalizer from service %s/%s for ingress %s/%s: %v", - service.Namespace, service.Name, ci.Namespace, ci.Name, err) - } - } - log.Info("finalized load balancer service for ingress", "namespace", ci.Namespace, "name", ci.Name) - return true, nil + _, err = r.deleteLoadBalancerServiceFinalizer(service) + return true, err } // createLoadBalancerService creates a load balancer service. @@ -292,6 +324,18 @@ func (r *reconciler) createLoadBalancerService(service *corev1.Service) error { return nil } +// deleteLoadBalancerService deletes a load balancer service. +func (r *reconciler) deleteLoadBalancerService(service *corev1.Service, options *crclient.DeleteOptions) error { + if err := r.client.Delete(context.TODO(), service, options); err != nil { + if errors.IsNotFound(err) { + return nil + } + return fmt.Errorf("failed to delete load balancer service %s/%s: %v", service.Namespace, service.Name, err) + } + log.Info("deleted load balancer service", "namespace", service.Namespace, "name", service.Name) + return nil +} + // updateLoadBalancerService updates a load balancer service. Returns a Boolean // indicating whether the service was updated, and an error value. func (r *reconciler) updateLoadBalancerService(current, desired *corev1.Service, platform *configv1.PlatformStatus) (bool, error) {