Cleanup cluster roles and bindings (#2938)

* Fix

Signed-off-by: Pavol Loffay <[email protected]>

* Fix

Signed-off-by: Pavol Loffay <[email protected]>

* Fix

Signed-off-by: Pavol Loffay <[email protected]>

* Fix

Signed-off-by: Pavol Loffay <[email protected]>

* Add test

Signed-off-by: Pavol Loffay <[email protected]>

---------

Signed-off-by: Pavol Loffay <[email protected]>

Author: pavolloffay

.chloggen/cleanup-roles.yaml

@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: bug_fix
+
+# The name of the component, or a single word describing the area of concern, (e.g. collector, target allocator, auto-instrumentation, opamp, github action)
+component: collector
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Cleanup ClusterRoles and ClusterRoleBindings created by the operator
+
+# One or more tracking issues related to the change
+issues: [2938]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext: The operator uses finalizer on the collector to run the cleanup

controllers/common.go

@@ -122,9 +122,18 @@ func reconcileDesiredObjects(ctx context.Context, kubeClient client.Client, logg
 	if len(errs) > 0 {
 		return fmt.Errorf("failed to create objects for %s: %w", owner.GetName(), errors.Join(errs...))
 	}
+	// Pruning owned objects in the cluster which are not should not be present after the reconciliation.
+	err := deleteObjects(ctx, kubeClient, logger, ownedObjects)
+	if err != nil {
+		return fmt.Errorf("failed to prune objects for %s: %w", owner.GetName(), err)
+	}
+	return nil
+}
+
+func deleteObjects(ctx context.Context, kubeClient client.Client, logger logr.Logger, objects map[types.UID]client.Object) error {
 	// Pruning owned objects in the cluster which are not should not be present after the reconciliation.
 	pruneErrs := []error{}
-	for _, obj := range ownedObjects {
+	for _, obj := range objects {
 		l := logger.WithValues(
 			"object_name", obj.GetName(),
 			"object_kind", obj.GetObjectKind().GroupVersionKind(),
@@ -137,8 +146,5 @@ func reconcileDesiredObjects(ctx context.Context, kubeClient client.Client, logg
 			pruneErrs = append(pruneErrs, err)
 		}
 	}
-	if len(pruneErrs) > 0 {
-		return fmt.Errorf("failed to prune objects for %s: %w", owner.GetName(), errors.Join(pruneErrs...))
-	}
-	return nil
+	return errors.Join(pruneErrs...)
 }

controllers/opentelemetrycollector_controller.go

@@ -35,6 +35,7 @@ import (
 	"k8s.io/client-go/tools/record"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/open-telemetry/opentelemetry-operator/apis/v1beta1"
 	"github.com/open-telemetry/opentelemetry-operator/internal/autodetect/openshift"
@@ -127,7 +128,42 @@ func (r *OpenTelemetryCollectorReconciler) findOtelOwnedObjects(ctx context.Cont
 	for i := range pdbList.Items {
 		ownedObjects[pdbList.Items[i].GetUID()] = &pdbList.Items[i]
 	}
+	if params.Config.CreateRBACPermissions() == rbac.Available {
+		clusterObjects, err := r.findClusterRoleObjects(ctx, params)
+		if err != nil {
+			return nil, err
+		}
+		for k, v := range clusterObjects {
+			ownedObjects[k] = v
+		}
+	}
+	return ownedObjects, nil
+}
 
+// The cluster scope objects do not have owner reference.
+func (r *OpenTelemetryCollectorReconciler) findClusterRoleObjects(ctx context.Context, params manifests.Params) (map[types.UID]client.Object, error) {
+	ownedObjects := map[types.UID]client.Object{}
+	// Remove cluster roles and bindings.
+	// Users might switch off the RBAC creation feature on the operator which should remove existing RBAC.
+	listOpsCluster := &client.ListOptions{
+		LabelSelector: labels.SelectorFromSet(manifestutils.SelectorLabels(params.OtelCol.ObjectMeta, collector.ComponentOpenTelemetryCollector)),
+	}
+	clusterroleList := &rbacv1.ClusterRoleList{}
+	err := r.List(ctx, clusterroleList, listOpsCluster)
+	if err != nil {
+		return nil, fmt.Errorf("error listing ClusterRoles: %w", err)
+	}
+	for i := range clusterroleList.Items {
+		ownedObjects[clusterroleList.Items[i].GetUID()] = &clusterroleList.Items[i]
+	}
+	clusterrolebindingList := &rbacv1.ClusterRoleBindingList{}
+	err = r.List(ctx, clusterrolebindingList, listOpsCluster)
+	if err != nil {
+		return nil, fmt.Errorf("error listing ClusterRoleBIndings: %w", err)
+	}
+	for i := range clusterrolebindingList.Items {
+		ownedObjects[clusterrolebindingList.Items[i].GetUID()] = &clusterrolebindingList.Items[i]
+	}
 	return ownedObjects, nil
 }
 
@@ -193,8 +229,32 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 		// on deleted requests.
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
+
+	params, err := r.getParams(instance)
+	if err != nil {
+		log.Error(err, "Failed to create manifest.Params")
+		return ctrl.Result{}, err
+	}
+
 	// We have a deletion, short circuit and let the deletion happen
 	if deletionTimestamp := instance.GetDeletionTimestamp(); deletionTimestamp != nil {
+		if controllerutil.ContainsFinalizer(&instance, collectorFinalizer) {
+			// If the finalization logic fails, don't remove the finalizer so
+			// that we can retry during the next reconciliation.
+			if err = r.finalizeCollector(ctx, params); err != nil {
+				return ctrl.Result{}, err
+			}
+
+			// Once all finalizers have been
+			// removed, the object will be deleted.
+			if controllerutil.RemoveFinalizer(&instance, collectorFinalizer) {
+				err = r.Update(ctx, &instance)
+				if err != nil {
+					return ctrl.Result{}, err
+				}
+			}
+		}
+
 		return ctrl.Result{}, nil
 	}
 
@@ -204,10 +264,14 @@ func (r *OpenTelemetryCollectorReconciler) Reconcile(ctx context.Context, req ct
 		return ctrl.Result{}, nil
 	}
 
-	params, err := r.getParams(instance)
-	if err != nil {
-		log.Error(err, "Failed to create manifest.Params")
-		return ctrl.Result{}, err
+	// Add finalizer for this CR
+	if !controllerutil.ContainsFinalizer(&instance, collectorFinalizer) {
+		if controllerutil.AddFinalizer(&instance, collectorFinalizer) {
+			err = r.Update(ctx, &instance)
+			if err != nil {
+				return ctrl.Result{}, err
+			}
+		}
 	}
 
 	desiredObjects, buildErr := BuildCollector(params)
@@ -255,3 +319,17 @@ func (r *OpenTelemetryCollectorReconciler) SetupWithManager(mgr ctrl.Manager) er
 
 	return builder.Complete(r)
 }
+
+const collectorFinalizer = "opentelemetrycollector.opentelemetry.io/finalizer"
+
+func (r *OpenTelemetryCollectorReconciler) finalizeCollector(ctx context.Context, params manifests.Params) error {
+	// The cluster scope objects do not have owner reference. They need to be deleted explicitly
+	if params.Config.CreateRBACPermissions() == rbac.Available {
+		objects, err := r.findClusterRoleObjects(ctx, params)
+		if err != nil {
+			return err
+		}
+		return deleteObjects(ctx, r.Client, r.log, objects)
+	}
+	return nil
+}