WIP: Dedicated controller for cleaning up bundle targets

pkg/bundle/bundle.go

@@ -25,7 +25,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/tools/record"
@@ -169,9 +168,9 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result
 		return ctrl.Result{}, statusPatch, nil
 	}
 
-	targetResources := map[target.Resource]struct{}{}
+	var targetResources []target.Resource
 
-	namespaceSelector, err := b.bundleTargetNamespaceSelector(&bundle)
+	namespaceSelector, err := target.NamespaceSelector(&bundle)
 	if err != nil {
 		b.recorder.Eventf(&bundle, corev1.EventTypeWarning, "NamespaceSelectorError", "Failed to build namespace match labels selector: %s", err)
 		return ctrl.Result{}, nil, fmt.Errorf("failed to build NamespaceSelector: %w", err)
@@ -202,87 +201,30 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result
 			}
 
 			if bundle.Spec.Target.Secret != nil {
-				targetResources[target.Resource{Kind: target.KindSecret, NamespacedName: namespacedName}] = struct{}{}
+				targetResources = append(targetResources, target.Resource{Kind: target.KindSecret, NamespacedName: namespacedName})
 			}
 			if bundle.Spec.Target.ConfigMap != nil {
-				targetResources[target.Resource{Kind: target.KindConfigMap, NamespacedName: namespacedName}] = struct{}{}
-			}
-		}
-	}
-
-	// Find all old existing target resources.
-	targetKinds := []target.Kind{target.KindConfigMap}
-	if b.Options.SecretTargetsEnabled {
-		targetKinds = append(targetKinds, target.KindSecret)
-	}
-	for _, kind := range targetKinds {
-		targetList := &metav1.PartialObjectMetadataList{
-			TypeMeta: metav1.TypeMeta{
-				APIVersion: "v1",
-				Kind:       string(kind),
-			},
-		}
-		err := b.targetReconciler.Cache.List(ctx, targetList, &client.ListOptions{
-			LabelSelector: labels.SelectorFromSet(map[string]string{
-				trustapi.BundleLabelKey: bundle.Name,
-			}),
-		})
-		if err != nil {
-			log.Error(err, "failed to list targets", "kind", kind)
-			b.recorder.Eventf(&bundle, corev1.EventTypeWarning, fmt.Sprintf("%sListError", kind), "Failed to list %ss: %s", strings.ToLower(string(kind)), err)
-			return ctrl.Result{}, nil, fmt.Errorf("failed to list %ss: %w", kind, err)
-		}
-
-		for _, t := range targetList.Items {
-			key := target.Resource{
-				Kind: kind,
-				NamespacedName: types.NamespacedName{
-					Name:      t.Name,
-					Namespace: t.Namespace,
-				},
-			}
-
-			targetLog := log.WithValues("target", key)
-
-			if _, ok := targetResources[key]; ok {
-				// This target is still a target, so we don't need to remove it.
-				continue
-			}
-
-			// Don't reconcile target for targets that are being deleted.
-			if t.GetDeletionTimestamp() != nil {
-				targetLog.V(2).WithValues("deletionTimestamp", t.GetDeletionTimestamp()).Info("skipping sync for target as it is being deleted")
-				continue
-			}
-
-			if !metav1.IsControlledBy(&t, &bundle) /* #nosec G601 -- False positive. See https://github.com/golang/go/discussions/56010 */ {
-				targetLog.V(2).Info("skipping sync for target as it is not controlled by bundle")
-				continue
-			}
-
-			if _, err := b.targetReconciler.CleanupTarget(ctx, key, &bundle); err != nil {
-				// Failing target cleanup is not considered critical, log error and continue.
-				targetLog.Error(err, "failed to cleanup bundle target")
+				targetResources = append(targetResources, target.Resource{Kind: target.KindConfigMap, NamespacedName: namespacedName})
 			}
 		}
 	}
 
 	var needsUpdate bool
 
-	for t := range targetResources {
+	for _, t := range targetResources {
 		targetLog := log.WithValues("target", t)
 		synced, err := b.targetReconciler.ApplyTarget(logf.IntoContext(ctx, targetLog), t, &bundle, resolvedBundle)
 		if err != nil {
 			targetLog.Error(err, "failed sync bundle to target namespace")
-			b.recorder.Eventf(&bundle, corev1.EventTypeWarning, fmt.Sprintf("Sync%sTargetFailed", t.Kind), "Failed to sync target %s in Namespace %q: %s", t.Kind, t.Namespace, err)
+			b.recorder.Eventf(&bundle, corev1.EventTypeWarning, fmt.Sprintf("ApplyTarget%sTargetFailed", t.Kind), "Failed to sync target %s in Namespace %q: %s", t.Kind, t.Namespace, err)
 
 			b.setBundleCondition(
 				bundle.Status.Conditions,
 				&statusPatch.Conditions,
 				metav1.Condition{
 					Type:               trustapi.BundleConditionSynced,
 					Status:             metav1.ConditionFalse,
-					Reason:             fmt.Sprintf("Sync%sTargetFailed", t.Kind),
+					Reason:             fmt.Sprintf("ApplyTarget%sTargetFailed", t.Kind),
 					Message:            fmt.Sprintf("Failed to sync bundle %s to namespace %q: %s", t.Kind, t.Namespace, err),
 					ObservedGeneration: bundle.Generation,
 				},
@@ -330,16 +272,3 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result
 
 	return ctrl.Result{}, statusPatch, nil
 }
-
-func (b *bundle) bundleTargetNamespaceSelector(bundleObj *trustapi.Bundle) (labels.Selector, error) {
-	nsSelector := bundleObj.Spec.Target.NamespaceSelector
-
-	// LabelSelectorAsSelector returns a Selector selecting nothing if LabelSelector is nil,
-	// while our current default is to select everything. But this is subject to change.
-	// See https://github.com/cert-manager/trust-manager/issues/39
-	if nsSelector == nil {
-		return labels.Everything(), nil
-	}
-
-	return metav1.LabelSelectorAsSelector(nsSelector)
-}

pkg/bundle/controller.go

@@ -128,7 +128,7 @@ func AddBundleController(
 		// Reconcile all Bundles on a Namespace change.
 		Watches(&corev1.Namespace{}, b.enqueueRequestsFromBundleFunc(
 			func(obj client.Object, bundle trustapi.Bundle) bool {
-				namespaceSelector, err := b.bundleTargetNamespaceSelector(&bundle)
+				namespaceSelector, err := target.NamespaceSelector(&bundle)
 				if err != nil {
 					// We have an invalid selector, so we can skip this Bundle.
 					return false

pkg/bundle/internal/target/cleanup_controller.go

@@ -0,0 +1,125 @@
+package target
+
+import (
+	"context"
+	"fmt"
+
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
+	"github.com/cert-manager/trust-manager/pkg/bundle/controller"
+)
+
+// targetCleanupController is responsible for cleaning up obsolete bundle target resources.
+type targetCleanupController struct {
+	*Reconciler
+
+	// Options holds options for the Bundle controller.
+	controller.Options
+}
+
+// Reconcile is the top level function for the controller,
+// and will be called whenever a Bundle event happens.
+func (t *targetCleanupController) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	bundleObj := &trustapi.Bundle{}
+	err := t.Client.Get(ctx, req.NamespacedName, bundleObj)
+	if apierrors.IsNotFound(err) || bundleObj.GetDeletionTimestamp() != nil {
+		// Bundle doesn't exist or is about to be deleted.
+		// Kubernetes garbage collector will delete obsolete bundle targets.
+		return ctrl.Result{}, nil
+	}
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("failed to get %q: %s", req.NamespacedName, err)
+	}
+
+	return ctrl.Result{}, t.reconcile(ctx, bundleObj)
+}
+
+func (t *targetCleanupController) reconcile(ctx context.Context, bundle *trustapi.Bundle) error {
+	targetKinds := []Kind{KindConfigMap}
+	if t.Options.SecretTargetsEnabled {
+		targetKinds = append(targetKinds, KindSecret)
+	}
+
+	// Convert metav1.LabelSelector to labels.Selector
+	nsSelector, err := NamespaceSelector(bundle)
+	if err != nil {
+		return fmt.Errorf("failed to build NamespaceSelector: %w", err)
+	}
+
+	for _, kind := range targetKinds {
+		var targetTemplate *trustapi.TargetTemplate
+		switch kind {
+		case KindConfigMap:
+			targetTemplate = bundle.Spec.Target.ConfigMap
+		case KindSecret:
+			targetTemplate = bundle.Spec.Target.Secret
+		default:
+			return fmt.Errorf("unknown targetType: %s", kind)
+		}
+
+		targetList := &metav1.PartialObjectMetadataList{
+			TypeMeta: metav1.TypeMeta{
+				APIVersion: "v1",
+				Kind:       string(kind),
+			},
+		}
+		err := t.Cache.List(ctx, targetList, &client.ListOptions{
+			LabelSelector: labels.SelectorFromSet(map[string]string{
+				trustapi.BundleLabelKey: bundle.Name,
+			}),
+		})
+		if err != nil {
+			return fmt.Errorf("failed to list %ss: %w", kind, err)
+		}
+
+		processTarget := func(targetObj *metav1.PartialObjectMetadata) error {
+			targetResource := Resource{
+				Kind:           kind,
+				NamespacedName: client.ObjectKeyFromObject(targetObj),
+			}
+
+			if targetObj.GetDeletionTimestamp() != nil {
+				// Don't reconcile target for targets that are being deleted.
+				return nil
+			}
+			if !metav1.IsControlledBy(targetObj, bundle) /* #nosec G601 -- False positive. See https://github.com/golang/go/discussions/56010 */ {
+				// Skipping delete of target not controlled by bundle
+				return nil
+			}
+
+			if targetTemplate == nil {
+				// No targets of this kind should exist
+				_, err := t.CleanupTarget(ctx, targetResource, bundle)
+				return err
+			}
+			if !nsSelector.Empty() {
+				// Target namespace selector limits target namespaces. We have to check if target namespace matches selector.
+				ns := &corev1.Namespace{}
+				if err := t.Client.Get(ctx, client.ObjectKey{Name: targetObj.Namespace}, ns); err != nil {
+					return fmt.Errorf("failed to get %s namespace: %w", targetObj.Namespace, err)
+				}
+				if !nsSelector.Matches(labels.Set(ns.Labels)) {
+					// Target namespace does not match selector, and should be cleaned.
+					_, err := t.CleanupTarget(ctx, targetResource, bundle)
+					return err
+				}
+			}
+			return nil
+		}
+
+		for _, targetObj := range targetList.Items {
+			err := processTarget(&targetObj)
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}

pkg/bundle/internal/target/target.go

@@ -28,6 +28,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/json"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -423,3 +424,16 @@ func TrustBundleHash(data []byte, additionalFormats *trustapi.AdditionalFormats,
 
 	return hex.EncodeToString(hashValue[:])
 }
+
+func NamespaceSelector(bundleObj *trustapi.Bundle) (labels.Selector, error) {
+	nsSelector := bundleObj.Spec.Target.NamespaceSelector
+
+	// LabelSelectorAsSelector returns a Selector selecting nothing if LabelSelector is nil,
+	// while our current default is to select everything. But this is subject to change.
+	// See https://github.com/cert-manager/trust-manager/issues/39
+	if nsSelector == nil {
+		return labels.Everything(), nil
+	}
+
+	return metav1.LabelSelectorAsSelector(nsSelector)
+}