From 330666336f28ebfada3c124016944947615a00cf Mon Sep 17 00:00:00 2001 From: Lukasz Raczylo Date: Sun, 21 Jun 2026 13:26:33 +0100 Subject: [PATCH] feat(controller): add exclude and paused source annotations exclude="true" opts a resource out of mirroring and tears down its existing mirrors; paused="true" freezes mirrors in place (no updates, no cleanup). Both respected by the source and namespace reconcilers. Also dedup target-namespace resolution via delegation, remove 8 unused annotation constants and their misleading godoc, and silence gosec G101 false positives on base64 test fixtures. --- pkg/constants/constants.go | 47 ++------ pkg/controller/mirror_test.go | 13 ++- pkg/controller/namespace_reconciler.go | 132 +++++------------------ pkg/controller/source_reconciler.go | 98 +++++++---------- pkg/controller/source_reconciler_test.go | 122 ++++++++++++++++++++- 5 files changed, 196 insertions(+), 216 deletions(-) diff --git a/pkg/constants/constants.go b/pkg/constants/constants.go index e790ebe..1caae7d 100644 --- a/pkg/constants/constants.go +++ b/pkg/constants/constants.go @@ -71,31 +71,18 @@ const ( // Annotation because: values can be complex patterns exceeding label limits. AnnotationTargetNamespaces = Domain + "/target-namespaces" - // AnnotationExclude explicitly excludes a resource from mirroring when "true". + // AnnotationExclude opts a resource out of mirroring when "true", overriding + // the enabled label and sync annotation. Any mirrors it previously created are + // removed. Use AnnotationPaused instead to freeze mirrors without deleting them. // Annotation because: used for configuration, not filtering. AnnotationExclude = Domain + "/exclude" - // AnnotationMaxTargets overrides the default maximum target limit per resource. - // Annotation because: numeric configuration value. - AnnotationMaxTargets = Domain + "/max-targets" - - // AnnotationRecreateOnImmutableChange controls delete/recreate behavior. - // When "true", kubemirror will delete and recreate mirrors on immutable field changes. - // Annotation because: configuration flag, not used for filtering. - AnnotationRecreateOnImmutableChange = Domain + "/recreate-on-immutable-change" - - // AnnotationPaused on controller deployment pauses all reconciliation when "true". + // AnnotationPaused freezes a source's mirrors when "true": existing mirrors + // are left untouched (no updates, no orphan cleanup) until the annotation is + // removed. Unlike AnnotationExclude, pausing does not delete existing mirrors. // Annotation because: operational control, not used for filtering. AnnotationPaused = Domain + "/paused" - // --- Source Tracking Annotations --- - // These are set by kubemirror on source resources for change detection. - - // AnnotationContentHash stores the SHA256 hash of the source resource content. - // Used for efficient change detection without deep comparison. - // Annotation because: computed value (64 chars), may exceed label limits. - AnnotationContentHash = Domain + "/content-hash" - // --- Mirror Ownership Annotations --- // These are set by kubemirror on mirror resources to track their source. // All are annotations because they store tracking data, not used for filtering. @@ -129,19 +116,6 @@ const ( // AnnotationSyncStatus stores human-readable sync status ("3/5 synced", etc.). AnnotationSyncStatus = Domain + "/sync-status" - // AnnotationFailedTargets stores comma-separated list of failed target namespaces. - AnnotationFailedTargets = Domain + "/failed-targets" - - // AnnotationWebhookError stores webhook rejection error message for debugging. - AnnotationWebhookError = Domain + "/webhook-error" - - // AnnotationTargetNamespaceUID tracks the UID of the target namespace. - // Used for detecting namespace recreation. - AnnotationTargetNamespaceUID = Domain + "/target-namespace-uid" - - // AnnotationDeletionAttempts tracks number of failed deletion attempts. - AnnotationDeletionAttempts = Domain + "/deletion-attempts" - // --- Transformation Annotations --- // These configure resource transformation during mirroring. @@ -189,13 +163,4 @@ var ( "bootstrap.kubernetes.io/token", "helm.sh/release.v1", } - - // Default Denied Resource Types - DefaultDeniedResourceTypes = []string{ - "events", - "pods", - "replicasets", - "endpoints", - "endpointslices", - } ) diff --git a/pkg/controller/mirror_test.go b/pkg/controller/mirror_test.go index 54b0ee4..eb70d12 100644 --- a/pkg/controller/mirror_test.go +++ b/pkg/controller/mirror_test.go @@ -182,7 +182,7 @@ func TestCreateMirror_Unstructured_StripsOwnerReferences(t *testing.T) { "externalsecrets.external-secrets.io/externalsecret-cleanup", }, }, - "data": map[string]interface{}{ + "data": map[string]interface{}{ //nolint:gosec // base64 test fixture, not a real credential "password": "c2VjcmV0", }, }, @@ -387,7 +387,7 @@ func TestUpdateMirror_UnstructuredSecret(t *testing.T) { }, }, "type": "Opaque", - "data": map[string]interface{}{ + "data": map[string]interface{}{ //nolint:gosec // base64 test fixture, not a real credential "password": "b2xkLXZhbHVl", // base64 encoded "old-value" }, }, @@ -403,7 +403,7 @@ func TestUpdateMirror_UnstructuredSecret(t *testing.T) { "generation": int64(10), }, "type": "kubernetes.io/tls", - "data": map[string]interface{}{ + "data": map[string]interface{}{ //nolint:gosec // base64 test fixture, not a real credential "password": "bmV3LXZhbHVl", // base64 encoded "new-value" "username": "YWRtaW4=", // base64 encoded "admin" }, @@ -670,10 +670,9 @@ func TestCreateMirror_NoSyncAnnotations(t *testing.T) { constants.LabelEnabled: "true", }, Annotations: map[string]string{ - constants.AnnotationSync: "true", - constants.AnnotationTargetNamespaces: "app1,app2", - constants.AnnotationExclude: "false", - constants.AnnotationRecreateOnImmutableChange: "true", + constants.AnnotationSync: "true", + constants.AnnotationTargetNamespaces: "app1,app2", + constants.AnnotationExclude: "false", }, }, Data: map[string][]byte{"key": []byte("value")}, diff --git a/pkg/controller/namespace_reconciler.go b/pkg/controller/namespace_reconciler.go index 48f2c76..a6eb55e 100644 --- a/pkg/controller/namespace_reconciler.go +++ b/pkg/controller/namespace_reconciler.go @@ -7,7 +7,6 @@ import ( "slices" corev1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -130,6 +129,13 @@ func (r *NamespaceReconciler) reconcileResourceType(ctx context.Context, rt conf continue } + // Skip excluded or paused sources. Excluded resources are torn down by the + // source reconciler; paused resources must stay frozen. In both cases the + // namespace reconciler must not create, update, or delete their mirrors. + if annotations[constants.AnnotationExclude] == "true" || isPaused(source) { + continue + } + // Resolve target namespaces for this source targetNamespaces, err := r.resolveTargetNamespaces(ctx, source) if err != nil { @@ -160,38 +166,10 @@ func (r *NamespaceReconciler) reconcileResourceType(ctx context.Context, rt conf "targetNamespace", namespaceName, "resourceType", rt.String()) } else { - // Namespace is no longer a target - check if mirror exists and delete it - mirror := &unstructured.Unstructured{} - mirror.SetGroupVersionKind(source.GroupVersionKind()) - mirror.SetNamespace(namespaceName) - mirror.SetName(source.GetName()) - - err := r.Get(ctx, client.ObjectKey{Namespace: namespaceName, Name: source.GetName()}, mirror) - if errors.IsNotFound(err) { - // No mirror exists, nothing to clean up - continue - } + // Namespace is no longer a target - delete the mirror if we own it. + outcome, err := deleteOwnedMirror(ctx, r.Client, source.GroupVersionKind(), + namespaceName, source.GetName(), source.GetNamespace(), source.GetName()) if err != nil { - logger.Error(err, "failed to check for mirror", - "source", source.GetName(), - "namespace", namespaceName) - errorCount++ - continue - } - - // Verify this is actually our mirror (not someone else's resource with the same name) - if !IsManagedByUs(mirror) { - continue - } - - // Verify this mirror points to our source - srcNs, srcName, _, found := GetSourceReference(mirror) - if !found || srcNs != source.GetNamespace() || srcName != source.GetName() { - continue - } - - // This mirror should be deleted (namespace no longer a valid target) - if err := r.Delete(ctx, mirror); err != nil { logger.Error(err, "failed to delete orphaned mirror", "source", source.GetName(), "sourceNamespace", source.GetNamespace(), @@ -199,89 +177,27 @@ func (r *NamespaceReconciler) reconcileResourceType(ctx context.Context, rt conf errorCount++ continue } - - reconciledCount++ - logger.V(1).Info("deleted orphaned mirror due to namespace label change", - "source", source.GetName(), - "sourceNamespace", source.GetNamespace(), - "targetNamespace", namespaceName, - "resourceType", rt.String()) + if outcome == mirrorDeleted { + reconciledCount++ + logger.V(1).Info("deleted orphaned mirror due to namespace label change", + "source", source.GetName(), + "sourceNamespace", source.GetNamespace(), + "targetNamespace", namespaceName, + "resourceType", rt.String()) + } } } return reconciledCount, errorCount, nil } -// resolveTargetNamespaces determines which namespaces should receive mirrors for a source. -// Uses the same logic as SourceReconciler.resolveTargetNamespaces. +// resolveTargetNamespaces determines which namespaces should receive mirrors for +// a source. It delegates to SourceReconciler so target-resolution logic +// (pattern parsing/validation, namespace listing, max-targets clamping) lives in +// exactly one place, mirroring the reconcileMirror delegation below. func (r *NamespaceReconciler) resolveTargetNamespaces(ctx context.Context, source *unstructured.Unstructured) ([]string, error) { - annotations := source.GetAnnotations() - if annotations == nil { - return nil, nil - } - - targetNsAnnotation := annotations[constants.AnnotationTargetNamespaces] - if targetNsAnnotation == "" { - return nil, nil - } - - // Parse patterns - patterns := filter.ParseTargetNamespaces(targetNsAnnotation) - if len(patterns) == 0 { - return nil, nil - } - - // Validate patterns and log warnings for invalid ones - validationResults, allValid := filter.ValidatePatterns(patterns) - if !allValid { - logger := log.FromContext(ctx) - invalidPatterns := filter.InvalidPatterns(validationResults) - for _, invalid := range invalidPatterns { - logger.Info("invalid glob pattern in target-namespaces annotation, pattern will be skipped", - "pattern", invalid.Pattern, - "error", invalid.Error.Error(), - "source", source.GetName(), - "namespace", source.GetNamespace(), - ) - } - - // Filter to only valid patterns - var validPatterns []string - for _, result := range validationResults { - if result.Valid { - validPatterns = append(validPatterns, result.Pattern) - } - } - patterns = validPatterns - - // If no valid patterns remain, return empty - if len(patterns) == 0 { - return nil, nil - } - } - - // Get all namespace info in a single API call (more efficient than 3 separate calls) - nsInfo, err := r.NamespaceLister.ListNamespacesWithLabels(ctx) - if err != nil { - return nil, fmt.Errorf("failed to list namespaces: %w", err) - } - - // Resolve target namespaces using the pre-categorized namespace info - targetNamespaces := filter.ResolveTargetNamespaces( - patterns, - nsInfo.All, - nsInfo.AllowMirrors, - nsInfo.OptOut, - source.GetNamespace(), - r.Filter, - ) - - // Enforce max targets limit - if r.Config != nil && r.Config.MaxTargetsPerResource > 0 && len(targetNamespaces) > r.Config.MaxTargetsPerResource { - targetNamespaces = targetNamespaces[:r.Config.MaxTargetsPerResource] - } - - return targetNamespaces, nil + return r.newSourceReconciler(source.GroupVersionKind()). + resolveTargetNamespaces(ctx, source) } // reconcileMirror creates or updates a mirror in the target namespace by diff --git a/pkg/controller/source_reconciler.go b/pkg/controller/source_reconciler.go index 7c31850..b9252f1 100644 --- a/pkg/controller/source_reconciler.go +++ b/pkg/controller/source_reconciler.go @@ -214,6 +214,15 @@ func (r *SourceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr return ctrl.Result{}, nil } + // Respect a per-source pause: freeze existing mirrors in place. We skip all + // mirror create/update and orphan cleanup, leaving any prior mirrors and the + // finalizer untouched until the annotation is removed. Deletion and disabling + // are handled above and intentionally take precedence over pause. + if isPaused(sourceObj) { + logger.V(1).Info("source is paused, leaving existing mirrors unchanged") + return ctrl.Result{}, nil + } + // Add finalizer if not present if !slices.Contains(sourceObj.GetFinalizers(), constants.FinalizerName) { logger.Info("adding finalizer to source resource") @@ -445,32 +454,16 @@ func (r *SourceReconciler) deleteAllMirrors(ctx context.Context, sourceObj metav continue } - existing := &unstructured.Unstructured{} - existing.SetGroupVersionKind(sourceUnstructured.GroupVersionKind()) - getErr := r.Get(ctx, client.ObjectKey{Namespace: ns, Name: sourceObj.GetName()}, existing) - if errors.IsNotFound(getErr) { - continue - } - if getErr != nil { - logger.Error(getErr, "failed to fetch potential mirror", "namespace", ns) - deleteErrs = append(deleteErrs, fmt.Errorf("get mirror %s/%s: %w", ns, sourceObj.GetName(), getErr)) - continue - } - - if !IsManagedByUs(existing) { - continue - } - srcNs, srcName, _, found := GetSourceReference(existing) - if !found || srcNs != sourceObj.GetNamespace() || srcName != sourceObj.GetName() { - continue - } - - if delErr := r.Delete(ctx, existing); delErr != nil && !errors.IsNotFound(delErr) { + outcome, delErr := deleteOwnedMirror(ctx, r.Client, sourceUnstructured.GroupVersionKind(), + ns, sourceObj.GetName(), sourceObj.GetNamespace(), sourceObj.GetName()) + if delErr != nil { logger.Error(delErr, "failed to delete mirror", "namespace", ns) deleteErrs = append(deleteErrs, fmt.Errorf("delete mirror %s/%s: %w", ns, sourceObj.GetName(), delErr)) continue } - deleteCount++ + if outcome == mirrorDeleted { + deleteCount++ + } } logger.Info("deleted mirrors", "count", deleteCount, "errors", len(deleteErrs)) @@ -516,41 +509,16 @@ func (r *SourceReconciler) cleanupOrphanedMirrors(ctx context.Context, sourceObj continue } - // Check if a mirror exists in this namespace - mirror := &unstructured.Unstructured{} - mirror.SetGroupVersionKind(sourceUnstructured.GroupVersionKind()) - mirror.SetNamespace(ns) - mirror.SetName(sourceObj.GetName()) - - err := r.Get(ctx, client.ObjectKey{Namespace: ns, Name: sourceObj.GetName()}, mirror) - if errors.IsNotFound(err) { - // No mirror exists, nothing to clean up - continue - } + outcome, err := deleteOwnedMirror(ctx, r.Client, sourceUnstructured.GroupVersionKind(), + ns, sourceObj.GetName(), sourceObj.GetNamespace(), sourceObj.GetName()) if err != nil { - logger.Error(err, "failed to check for mirror", "namespace", ns) - continue - } - - // Verify this is actually our mirror (not someone else's resource with the same name) - if !IsManagedByUs(mirror) { - continue - } - - // Verify this mirror points to our source - srcNs, srcName, _, found := GetSourceReference(mirror) - if !found || srcNs != sourceObj.GetNamespace() || srcName != sourceObj.GetName() { - continue - } - - // This is an orphaned mirror - delete it - if err := r.Delete(ctx, mirror); err != nil { logger.Error(err, "failed to delete orphaned mirror", "namespace", ns) continue } - - deletedCount++ - logger.V(1).Info("deleted orphaned mirror", "namespace", ns) + if outcome == mirrorDeleted { + deletedCount++ + logger.V(1).Info("deleted orphaned mirror", "namespace", ns) + } } return deletedCount, nil @@ -662,7 +630,10 @@ func isBlacklistedSecret(obj *unstructured.Unstructured) bool { return slices.Contains(constants.BlacklistedSecretTypes, secretType) } -// isEnabledForMirroring checks if a resource has both the label and annotation for mirroring. +// isEnabledForMirroring reports whether a resource should be mirrored. A resource +// must carry both the enabled label and the sync annotation, and must not be +// explicitly excluded. An excluded resource (exclude="true") is treated as +// disabled, so the reconcile loop tears down any mirrors it previously created. func isEnabledForMirroring(obj metav1.Object) bool { // Check label labels := obj.GetLabels() @@ -676,9 +647,23 @@ func isEnabledForMirroring(obj metav1.Object) bool { return false } + // Explicit opt-out always wins over the enable label/annotation. + if annotations[constants.AnnotationExclude] == "true" { + return false + } + return true } +// isPaused reports whether a source has the pause annotation set. A paused +// source is frozen: its existing mirrors are left exactly as they are (no +// updates, no orphan cleanup) until the annotation is removed. Unlike +// isEnabledForMirroring returning false, pausing does not delete mirrors. +func isPaused(obj metav1.Object) bool { + annotations := obj.GetAnnotations() + return annotations != nil && annotations[constants.AnnotationPaused] == "true" +} + // SetupWithManagerForResourceType sets up a controller for a specific resource type. // This allows dynamic controller registration for any discovered resource type. func (r *SourceReconciler) SetupWithManagerForResourceType( @@ -689,10 +674,7 @@ func (r *SourceReconciler) SetupWithManagerForResourceType( obj := &unstructured.Unstructured{} obj.SetGroupVersionKind(gvk) - // Create unique controller name including version and group to avoid collisions - // e.g., "HorizontalPodAutoscaler.v1.autoscaling" or "Secret.v1." (empty group for core resources) - // This matches the naming convention used by mirror reconcilers - controllerName := gvk.Kind + "." + gvk.Version + "." + gvk.Group + controllerName := gvkControllerName(gvk, false) // Create mirror object for watching mirrorObj := &unstructured.Unstructured{} diff --git a/pkg/controller/source_reconciler_test.go b/pkg/controller/source_reconciler_test.go index 71783a7..a970672 100644 --- a/pkg/controller/source_reconciler_test.go +++ b/pkg/controller/source_reconciler_test.go @@ -205,6 +205,36 @@ func TestIsEnabledForMirroring(t *testing.T) { }, want: false, }, + { + name: "excluded overrides enabled label and sync annotation", + obj: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constants.LabelEnabled: "true", + }, + Annotations: map[string]string{ + constants.AnnotationSync: "true", + constants.AnnotationExclude: "true", + }, + }, + }, + want: false, + }, + { + name: "exclude set to false does not disable mirroring", + obj: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + constants.LabelEnabled: "true", + }, + Annotations: map[string]string{ + constants.AnnotationSync: "true", + constants.AnnotationExclude: "false", + }, + }, + }, + want: true, + }, } for _, tt := range tests { @@ -215,6 +245,47 @@ func TestIsEnabledForMirroring(t *testing.T) { } } +func TestIsPaused(t *testing.T) { + tests := []struct { + obj metav1.Object + name string + want bool + }{ + { + name: "paused true", + obj: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{constants.AnnotationPaused: "true"}, + }}, + want: true, + }, + { + name: "paused false", + obj: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{constants.AnnotationPaused: "false"}, + }}, + want: false, + }, + { + name: "annotation absent", + obj: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{constants.AnnotationSync: "true"}, + }}, + want: false, + }, + { + name: "no annotations", + obj: &corev1.Secret{ObjectMeta: metav1.ObjectMeta{}}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, isPaused(tt.obj)) + }) + } +} + func TestSourceReconciler_resolveTargetNamespaces(t *testing.T) { tests := []struct { name string @@ -603,7 +674,7 @@ func TestSourceReconciler_Reconcile_AnnotationChange_AllToAllLabeled(t *testing. constants.FinalizerName, }, }, - "data": map[string]interface{}{ + "data": map[string]interface{}{ //nolint:gosec // base64 test fixture, not a real credential "password": "c2VjcmV0", }, }, @@ -632,7 +703,7 @@ func TestSourceReconciler_Reconcile_AnnotationChange_AllToAllLabeled(t *testing. constants.AnnotationSourceUID: "source-uid-123", }, }, - "data": map[string]interface{}{ + "data": map[string]interface{}{ //nolint:gosec // base64 test fixture, not a real credential "password": "c2VjcmV0", }, }, @@ -735,6 +806,53 @@ func TestSourceReconciler_Reconcile_AnnotationChange_AllToAllLabeled(t *testing. mockLister.AssertExpectations(t) } +func TestSourceReconciler_Reconcile_Paused_LeavesMirrorsUntouched(t *testing.T) { + // A paused source must early-return: no List, Create, Delete, or Update. + // The MockClient panics on any un-mocked call, so wiring only the source Get + // proves Reconcile touches nothing else. + source := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "Secret", + "metadata": map[string]any{ + "name": "test-secret", + "namespace": "default", + "uid": "source-uid-123", + "labels": map[string]any{ + constants.LabelEnabled: "true", + }, + "annotations": map[string]any{ + constants.AnnotationSync: "true", + constants.AnnotationTargetNamespaces: "all", + constants.AnnotationPaused: "true", + }, + "finalizers": []any{constants.FinalizerName}, + }, + "data": map[string]any{"password": "c2VjcmV0"}, //nolint:gosec // base64 test fixture, not a real credential + }, + } + + mockClient := new(MockClient) + // Only the source read is expected; nothing else. + mockClient.On("Get", mock.Anything, types.NamespacedName{Namespace: "default", Name: "test-secret"}, mock.Anything). + Return(nil, source).Once() + + r := &SourceReconciler{ + Client: mockClient, + Scheme: runtime.NewScheme(), + Config: &config.Config{}, + Filter: filter.NewNamespaceFilter(nil, nil), + GVK: schema.GroupVersionKind{Group: "", Version: "v1", Kind: "Secret"}, + } + + result, err := r.Reconcile(context.Background(), + ctrl.Request{NamespacedName: types.NamespacedName{Namespace: "default", Name: "test-secret"}}) + + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + mockClient.AssertExpectations(t) +} + func TestSourceReconciler_Reconcile_AnnotationChange_PatternChange(t *testing.T) { // Scenario: annotation changes from "app-*" → "prod-*" // Before: mirrors in app-1, app-2, app-3