Add finalizer for CSV cleanup

Remove old CSV cleanup code
Cleanup CRB/CR/MWC/VWC via finalizer.

Signed-off-by: Todd Short <[email protected]>

Author: tmshort

Diff for: pkg/controller/operators/olm/operator.go

@@ -11,7 +11,6 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/labeller"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/controller/operators/olm/plugins"
 	"github.com/sirupsen/logrus"
-	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -50,7 +49,6 @@ import (
 	csvutility "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/csv"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/event"
 	index "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/index"
-	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubestate"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/labeler"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"
@@ -86,7 +84,6 @@ type Operator struct {
 	olmConfigQueue               workqueue.RateLimitingInterface
 	csvCopyQueueSet              *queueinformer.ResourceQueueSet
 	copiedCSVGCQueueSet          *queueinformer.ResourceQueueSet
-	objGCQueueSet                *queueinformer.ResourceQueueSet
 	nsQueueSet                   workqueue.RateLimitingInterface
 	apiServiceQueue              workqueue.RateLimitingInterface
 	csvIndexers                  map[string]cache.Indexer
@@ -202,7 +199,6 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 		olmConfigQueue:               workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "olmConfig"),
 		csvCopyQueueSet:              queueinformer.NewEmptyResourceQueueSet(),
 		copiedCSVGCQueueSet:          queueinformer.NewEmptyResourceQueueSet(),
-		objGCQueueSet:                queueinformer.NewEmptyResourceQueueSet(),
 		apiServiceQueue:              workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "apiservice"),
 		resolver:                     config.strategyResolver,
 		apiReconciler:                config.apiReconciler,
@@ -480,21 +476,6 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 		if err := op.RegisterQueueInformer(serviceAccountQueueInformer); err != nil {
 			return nil, err
 		}
-
-		objGCQueue := workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), fmt.Sprintf("%s/obj-gc", namespace))
-		op.objGCQueueSet.Set(namespace, objGCQueue)
-		objGCQueueInformer, err := queueinformer.NewQueue(
-			ctx,
-			queueinformer.WithLogger(op.logger),
-			queueinformer.WithQueue(objGCQueue),
-			queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncGCObject).ToSyncer()),
-		)
-		if err != nil {
-			return nil, err
-		}
-		if err := op.RegisterQueueInformer(objGCQueueInformer); err != nil {
-			return nil, err
-		}
 	}
 
 	complete := map[schema.GroupVersionResource][]bool{}
@@ -568,22 +549,6 @@ func newOperatorWithConfig(ctx context.Context, config *operatorConfig) (*Operat
 		return nil, err
 	}
 
-	// add queue for all namespaces as well
-	objGCQueue := workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), fmt.Sprintf("%s/obj-gc", ""))
-	op.objGCQueueSet.Set("", objGCQueue)
-	objGCQueueInformer, err := queueinformer.NewQueue(
-		ctx,
-		queueinformer.WithLogger(op.logger),
-		queueinformer.WithQueue(objGCQueue),
-		queueinformer.WithSyncer(queueinformer.LegacySyncHandler(op.syncGCObject).ToSyncer()),
-	)
-	if err != nil {
-		return nil, err
-	}
-	if err := op.RegisterQueueInformer(objGCQueueInformer); err != nil {
-		return nil, err
-	}
-
 	// Register QueueInformer for olmConfig
 	olmConfigInformer := externalversions.NewSharedInformerFactoryWithOptions(
 		op.client,
@@ -948,98 +913,6 @@ func (a *Operator) EnsureCSVMetric() error {
 	return nil
 }
 
-func (a *Operator) syncGCObject(obj interface{}) (syncError error) {
-	metaObj, ok := obj.(metav1.Object)
-	if !ok {
-		a.logger.Warn("object sync: casting to metav1.Object failed")
-		return
-	}
-	logger := a.logger.WithFields(logrus.Fields{
-		"name":      metaObj.GetName(),
-		"namespace": metaObj.GetNamespace(),
-		"self":      metaObj.GetSelfLink(),
-	})
-
-	switch metaObj.(type) {
-	case *rbacv1.ClusterRole:
-		if name, ns, ok := ownerutil.GetOwnerByKindLabel(metaObj, v1alpha1.ClusterServiceVersionKind); ok {
-			_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ns).Get(name)
-			if err == nil {
-				logger.Debugf("CSV still present, must wait until it is deleted (owners=%v/%v)", ns, name)
-				syncError = fmt.Errorf("cleanup must wait")
-				return
-			} else if !apierrors.IsNotFound(err) {
-				syncError = err
-				return
-			}
-		}
-
-		if err := a.opClient.DeleteClusterRole(metaObj.GetName(), &metav1.DeleteOptions{}); err != nil {
-			logger.WithError(err).Warn("cannot delete cluster role")
-			break
-		}
-		logger.Debugf("Deleted cluster role %v due to no owning CSV", metaObj.GetName())
-	case *rbacv1.ClusterRoleBinding:
-		if name, ns, ok := ownerutil.GetOwnerByKindLabel(metaObj, v1alpha1.ClusterServiceVersionKind); ok {
-			_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ns).Get(name)
-			if err == nil {
-				logger.Debugf("CSV still present, must wait until it is deleted (owners=%v)", name)
-				syncError = fmt.Errorf("cleanup must wait")
-				return
-			} else if !apierrors.IsNotFound(err) {
-				syncError = err
-				return
-			}
-		}
-
-		if err := a.opClient.DeleteClusterRoleBinding(metaObj.GetName(), &metav1.DeleteOptions{}); err != nil {
-			logger.WithError(err).Warn("cannot delete cluster role binding")
-			break
-		}
-		logger.Debugf("Deleted cluster role binding %v due to no owning CSV", metaObj.GetName())
-	case *admissionregistrationv1.MutatingWebhookConfiguration:
-		if name, ns, ok := ownerutil.GetOwnerByKindLabel(metaObj, v1alpha1.ClusterServiceVersionKind); ok {
-			_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ns).Get(name)
-			if err == nil {
-				logger.Debugf("CSV still present, must wait until it is deleted (owners=%v)", name)
-				syncError = fmt.Errorf("cleanup must wait")
-				return
-			} else if !apierrors.IsNotFound(err) {
-				logger.Infof("error CSV retrieval error")
-				syncError = err
-				return
-			}
-		}
-
-		if err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().Delete(context.TODO(), metaObj.GetName(), metav1.DeleteOptions{}); err != nil {
-			logger.WithError(err).Warn("cannot delete MutatingWebhookConfiguration")
-			break
-		}
-		logger.Debugf("Deleted MutatingWebhookConfiguration %v due to no owning CSV", metaObj.GetName())
-	case *admissionregistrationv1.ValidatingWebhookConfiguration:
-		if name, ns, ok := ownerutil.GetOwnerByKindLabel(metaObj, v1alpha1.ClusterServiceVersionKind); ok {
-			_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ns).Get(name)
-			if err == nil {
-				logger.Debugf("CSV still present, must wait until it is deleted (owners=%v)", name)
-				syncError = fmt.Errorf("cleanup must wait")
-				return
-			} else if !apierrors.IsNotFound(err) {
-				logger.Infof("Error CSV retrieval error")
-				syncError = err
-				return
-			}
-		}
-
-		if err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().Delete(context.TODO(), metaObj.GetName(), metav1.DeleteOptions{}); err != nil {
-			logger.WithError(err).Warn("cannot delete ValidatingWebhookConfiguration")
-			break
-		}
-		logger.Debugf("Deleted ValidatingWebhookConfiguration %v due to no owning CSV", metaObj.GetName())
-	}
-
-	return
-}
-
 func (a *Operator) syncObject(obj interface{}) (syncError error) {
 	// Assert as metav1.Object
 	metaObj, ok := obj.(metav1.Object)
@@ -1054,30 +927,8 @@ func (a *Operator) syncObject(obj interface{}) (syncError error) {
 		"self":      metaObj.GetSelfLink(),
 	})
 
-	// Requeues objects that can't have ownerrefs (cluster -> namespace, cross-namespace)
-	if ownerutil.IsOwnedByKindLabel(metaObj, v1alpha1.ClusterServiceVersionKind) {
-		name, ns, ok := ownerutil.GetOwnerByKindLabel(metaObj, v1alpha1.ClusterServiceVersionKind)
-		if !ok {
-			logger.Error("unexpected owner label retrieval failure")
-		}
-		_, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(ns).Get(name)
-		if !apierrors.IsNotFound(err) {
-			logger.Debug("requeueing owner csvs from owner label")
-			a.requeueOwnerCSVs(metaObj)
-		} else {
-			switch metaObj.(type) {
-			case *rbacv1.ClusterRole, *rbacv1.ClusterRoleBinding, *admissionregistrationv1.MutatingWebhookConfiguration, *admissionregistrationv1.ValidatingWebhookConfiguration:
-				resourceEvent := kubestate.NewResourceEvent(
-					kubestate.ResourceUpdated,
-					metaObj,
-				)
-				if syncError = a.objGCQueueSet.RequeueEvent("", resourceEvent); syncError != nil {
-					logger.WithError(syncError).Warnf("failed to requeue gc event: %v", resourceEvent)
-				}
-				return
-			}
-		}
-	}
+	// Objects that can't have ownerrefs (cluster -> namespace, cross-namespace)
+	// are handled by finalizer
 
 	// Requeue all owner CSVs
 	if ownerutil.IsOwnedByKind(metaObj, v1alpha1.ClusterServiceVersionKind) {
@@ -1332,50 +1183,6 @@ func (a *Operator) handleClusterServiceVersionDeletion(obj interface{}) {
 		}
 	}
 
-	ownerSelector := ownerutil.CSVOwnerSelector(clusterServiceVersion)
-	crbs, err := a.lister.RbacV1().ClusterRoleBindingLister().List(ownerSelector)
-	if err != nil {
-		logger.WithError(err).Warn("cannot list cluster role bindings")
-	}
-	for _, crb := range crbs {
-		if err := a.objGCQueueSet.RequeueEvent("", kubestate.NewResourceEvent(kubestate.ResourceUpdated, crb)); err != nil {
-			logger.WithError(err).Warnf("failed to requeue gc event: %v", crb)
-		}
-	}
-
-	crs, err := a.lister.RbacV1().ClusterRoleLister().List(ownerSelector)
-	if err != nil {
-		logger.WithError(err).Warn("cannot list cluster roles")
-	}
-	for _, cr := range crs {
-		if err := a.objGCQueueSet.RequeueEvent("", kubestate.NewResourceEvent(kubestate.ResourceUpdated, cr)); err != nil {
-			logger.WithError(err).Warnf("failed to requeue gc event: %v", cr)
-		}
-	}
-
-	webhookSelector := labels.SelectorFromSet(ownerutil.OwnerLabel(clusterServiceVersion, v1alpha1.ClusterServiceVersionKind)).String()
-	mWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().MutatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
-	if err != nil {
-		logger.WithError(err).Warn("cannot list MutatingWebhookConfigurations")
-	}
-	for _, webhook := range mWebhooks.Items {
-		w := webhook
-		if err := a.objGCQueueSet.RequeueEvent("", kubestate.NewResourceEvent(kubestate.ResourceUpdated, &w)); err != nil {
-			logger.WithError(err).Warnf("failed to requeue gc event: %v", webhook)
-		}
-	}
-
-	vWebhooks, err := a.opClient.KubernetesInterface().AdmissionregistrationV1().ValidatingWebhookConfigurations().List(context.TODO(), metav1.ListOptions{LabelSelector: webhookSelector})
-	if err != nil {
-		logger.WithError(err).Warn("cannot list ValidatingWebhookConfigurations")
-	}
-	for _, webhook := range vWebhooks.Items {
-		w := webhook
-		if err := a.objGCQueueSet.RequeueEvent("", kubestate.NewResourceEvent(kubestate.ResourceUpdated, &w)); err != nil {
-			logger.WithError(err).Warnf("failed to requeue gc event: %v", webhook)
-		}
-	}
-
 	// Conversion webhooks are defined within a CRD.
 	// In an effort to prevent customer dataloss, OLM does not delete CRDs associated with a CSV when it is deleted.
 	// Deleting a CSV that introduced a conversion webhook removes the deployment that serviced the conversion webhook calls.

Diff for: pkg/controller/operators/operatorconditiongenerator_controller.go

@@ -2,15 +2,20 @@ package operators
 
 import (
 	"context"
+	"fmt"
 	"reflect"
 
 	"github.com/go-logr/logr"
+	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
@@ -95,6 +100,10 @@ func (r *OperatorConditionGeneratorReconciler) Reconcile(ctx context.Context, re
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
+	if err, ok := r.processFinalizer(ctx, in); !ok {
+		return ctrl.Result{}, err
+	}
+
 	operatorCondition := &operatorsv2.OperatorCondition{
 		ObjectMeta: metav1.ObjectMeta{
 			// For now, only generate an OperatorCondition with the same name as the csv.
@@ -170,3 +179,100 @@ func (r *OperatorConditionGeneratorReconciler) ensureOperatorCondition(operatorC
 	existingOperatorCondition.Spec.ServiceAccounts = operatorCondition.Spec.ServiceAccounts
 	return r.Client.Update(context.TODO(), existingOperatorCondition)
 }
+
+// Return values, err, ok; ok == true: continue Reconcile, ok == false: exit Reconcile
+func (r *OperatorConditionGeneratorReconciler) processFinalizer(ctx context.Context, csv *operatorsv1alpha1.ClusterServiceVersion) (error, bool) {
+	myFinalizerName := "operators.coreos.com/csv-cleanup"
+	log := r.log.WithValues("name", csv.GetName()).WithValues("namespace", csv.GetNamespace())
+
+	if csv.ObjectMeta.DeletionTimestamp.IsZero() {
+		// CSV is not being deleted, add finalizer if not present
+		if !controllerutil.ContainsFinalizer(csv, myFinalizerName) {
+			patch := client.MergeFrom(csv.DeepCopy())
+			controllerutil.AddFinalizer(csv, myFinalizerName)
+			if err := r.Client.Patch(ctx, csv, patch); err != nil {
+				log.Error(err, "Adding finalizer")
+				return err, false
+			}
+		}
+		return nil, true
+	}
+
+	if !controllerutil.ContainsFinalizer(csv, myFinalizerName) {
+		// Finalizer has been removed; stop reconciliation as the CSV is being deleted
+		return nil, false
+	}
+
+	// CSV is being deleted and the finalizer still present; do any clean up
+	ownerSelector := ownerutil.CSVOwnerSelector(csv)
+	listOptions := client.ListOptions{
+		LabelSelector: ownerSelector,
+	}
+	deleteOptions := client.DeleteAllOfOptions{
+		ListOptions: listOptions,
+	}
+	// Look for resources owned by this CSV, and delete them.
+	log.WithValues("selector", ownerSelector).Info("Cleaning up resources after CSV deletion")
+	var errs []error
+
+	err := r.Client.DeleteAllOf(ctx, &rbacv1.ClusterRoleBinding{}, &deleteOptions)
+	if client.IgnoreNotFound(err) != nil {
+		log.Error(err, "Deleting ClusterRoleBindings on CSV delete")
+		errs = append(errs, err)
+	}
+
+	err = r.Client.DeleteAllOf(ctx, &rbacv1.ClusterRole{}, &deleteOptions)
+	if client.IgnoreNotFound(err) != nil {
+		log.Error(err, "Deleting ClusterRoles on CSV delete")
+		errs = append(errs, err)
+	}
+
+	err = r.Client.DeleteAllOf(ctx, &admissionregistrationv1.MutatingWebhookConfiguration{}, &deleteOptions)
+	if client.IgnoreNotFound(err) != nil {
+		log.Error(err, "Deleting MutatingWebhookConfigurations on CSV delete")
+		errs = append(errs, err)
+	}
+
+	err = r.Client.DeleteAllOf(ctx, &admissionregistrationv1.ValidatingWebhookConfiguration{}, &deleteOptions)
+	if client.IgnoreNotFound(err) != nil {
+		log.Error(err, "Deleting ValidatingWebhookConfigurations on CSV delete")
+		errs = append(errs, err)
+	}
+
+	// Make sure things are deleted
+	crbList := &rbacv1.ClusterRoleBindingList{}
+	if _ = r.Client.List(ctx, crbList, &listOptions); len(crbList.Items) != 0 {
+		errs = append(errs, fmt.Errorf("waiting for ClusterRoleBindings to delete"))
+	}
+
+	crList := &rbacv1.ClusterRoleList{}
+	if _ = r.Client.List(ctx, crList, &listOptions); len(crList.Items) != 0 {
+		errs = append(errs, fmt.Errorf("waiting for ClusterRoles to delete"))
+	}
+
+	mwcList := &admissionregistrationv1.MutatingWebhookConfigurationList{}
+	if _ = r.Client.List(ctx, mwcList, &listOptions); len(mwcList.Items) != 0 {
+		errs = append(errs, fmt.Errorf("waiting for MutatingWebhookConfigurations to delete"))
+	}
+
+	vwcList := &admissionregistrationv1.ValidatingWebhookConfigurationList{}
+	if _ = r.Client.List(ctx, vwcList, &listOptions); len(vwcList.Items) != 0 {
+		errs = append(errs, fmt.Errorf("waiting for ValidatingWebhookConfigurations to delete"))
+	}
+
+	// Return any errors
+	if err := utilerrors.NewAggregate(errs); err != nil {
+		return err, false
+	}
+
+	// If no errors, remove our finalizer from the CSV and update
+	patch := client.MergeFrom(csv.DeepCopy())
+	controllerutil.RemoveFinalizer(csv, myFinalizerName)
+	if err := r.Client.Patch(ctx, csv, patch); err != nil {
+		log.Error(err, "Removing finalizer")
+		return err, false
+	}
+
+	// Stop reconciliation as the csv is being deleted
+	return nil, false
+}