diff --git a/pkg/bundle/bundle.go b/pkg/bundle/bundle.go index aa75e2b77..4dff172bd 100644 --- a/pkg/bundle/bundle.go +++ b/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,79 +201,22 @@ 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, @@ -282,7 +224,7 @@ func (b *bundle) reconcileBundle(ctx context.Context, req ctrl.Request) (result 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) -} diff --git a/pkg/bundle/controller.go b/pkg/bundle/controller.go index 7d5085355..8ea366ce2 100644 --- a/pkg/bundle/controller.go +++ b/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 diff --git a/pkg/bundle/internal/target/cleanup_controller.go b/pkg/bundle/internal/target/cleanup_controller.go new file mode 100644 index 000000000..713366eb4 --- /dev/null +++ b/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 +} diff --git a/pkg/bundle/internal/target/target.go b/pkg/bundle/internal/target/target.go index 0b5d1ccc1..d2d5ddbfb 100644 --- a/pkg/bundle/internal/target/target.go +++ b/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) +}