From 3ace65a9ade9bc63a679912972a3c29e433d148e Mon Sep 17 00:00:00 2001 From: Safwan Date: Mon, 22 Jun 2026 20:34:40 +0500 Subject: [PATCH] refactoring --- .../chart/reloader/templates/deployment.yaml | 20 ++ .../kubernetes/chart/reloader/values.yaml | 5 + internal/pkg/config/flags.go | 42 ++-- internal/pkg/config/flags_test.go | 131 ++++++++++++ internal/pkg/controller/manager.go | 2 +- .../pkg/controller/resource_reconciler.go | 62 ++++-- .../secretproviderclass_filter_test.go | 55 +++++ .../secretproviderclass_reconciler.go | 196 ++++++------------ .../secretproviderclass_reconciler_test.go | 28 +++ internal/pkg/reload/predicate_test.go | 15 ++ internal/pkg/reload/service.go | 5 + internal/pkg/reload/service_test.go | 86 ++++++++ internal/pkg/reload/strategy.go | 11 +- internal/pkg/reload/strategy_test.go | 44 ++++ 14 files changed, 535 insertions(+), 167 deletions(-) create mode 100644 internal/pkg/controller/secretproviderclass_filter_test.go diff --git a/deployments/kubernetes/chart/reloader/templates/deployment.yaml b/deployments/kubernetes/chart/reloader/templates/deployment.yaml index 4b0e9e57..fb6a8bb7 100644 --- a/deployments/kubernetes/chart/reloader/templates/deployment.yaml +++ b/deployments/kubernetes/chart/reloader/templates/deployment.yaml @@ -274,6 +274,26 @@ spec: {{- if .Values.reloader.custom_annotations.configmap_auto }} - "--configmap-auto-annotation" - "{{ .Values.reloader.custom_annotations.configmap_auto }}" + {{- end }} + {{- if .Values.reloader.custom_annotations.configmap_exclude }} + - "--configmap-exclude-annotation" + - "{{ .Values.reloader.custom_annotations.configmap_exclude }}" + {{- end }} + {{- if .Values.reloader.custom_annotations.secret_exclude }} + - "--secret-exclude-annotation" + - "{{ .Values.reloader.custom_annotations.secret_exclude }}" + {{- end }} + {{- if .Values.reloader.custom_annotations.secretproviderclass }} + - "--secretproviderclass-annotation" + - "{{ .Values.reloader.custom_annotations.secretproviderclass }}" + {{- end }} + {{- if .Values.reloader.custom_annotations.secretproviderclass_auto }} + - "--secretproviderclass-auto-annotation" + - "{{ .Values.reloader.custom_annotations.secretproviderclass_auto }}" + {{- end }} + {{- if .Values.reloader.custom_annotations.secretproviderclass_exclude }} + - "--secretproviderclass-exclude-annotation" + - "{{ .Values.reloader.custom_annotations.secretproviderclass_exclude }}" {{- end }} {{- if .Values.reloader.custom_annotations.search }} - "--auto-search-annotation" diff --git a/deployments/kubernetes/chart/reloader/values.yaml b/deployments/kubernetes/chart/reloader/values.yaml index bbd28800..1e867dc0 100644 --- a/deployments/kubernetes/chart/reloader/values.yaml +++ b/deployments/kubernetes/chart/reloader/values.yaml @@ -216,6 +216,11 @@ reloader: # custom_annotations: # configmap: "my.company.com/configmap" # secret: "my.company.com/secret" + # configmap_exclude: "my.company.com/configmap-exclude" + # secret_exclude: "my.company.com/secret-exclude" + # secretproviderclass: "my.company.com/secretproviderclass" + # secretproviderclass_auto: "my.company.com/secretproviderclass-auto" + # secretproviderclass_exclude: "my.company.com/secretproviderclass-exclude" # ignore: "my.company.com/reloader-ignore" custom_annotations: {} diff --git a/internal/pkg/config/flags.go b/internal/pkg/config/flags.go index 7f3008a6..784fef10 100644 --- a/internal/pkg/config/flags.go +++ b/internal/pkg/config/flags.go @@ -182,6 +182,26 @@ func BindFlags(fs *pflag.FlagSet, cfg *Config) { "secret-annotation", cfg.Annotations.SecretReload, "Annotation to detect changes in secrets, specified by name", ) + fs.String( + "configmap-exclude-annotation", cfg.Annotations.ConfigmapExclude, + "Annotation to exclude named configmaps from triggering reloads", + ) + fs.String( + "secret-exclude-annotation", cfg.Annotations.SecretExclude, + "Annotation to exclude named secrets from triggering reloads", + ) + fs.String( + "secretproviderclass-auto-annotation", cfg.Annotations.SecretProviderClassAuto, + "Annotation to detect changes in secret provider classes (CSI)", + ) + fs.String( + "secretproviderclass-annotation", cfg.Annotations.SecretProviderClassReload, + "Annotation to detect changes in secret provider classes (CSI), specified by name", + ) + fs.String( + "secretproviderclass-exclude-annotation", cfg.Annotations.SecretProviderClassExclude, + "Annotation to exclude named secret provider classes (CSI) from triggering reloads", + ) fs.String( "auto-search-annotation", cfg.Annotations.Search, "Annotation to detect changes in configmaps or secrets tagged with special match annotation", @@ -190,6 +210,10 @@ func BindFlags(fs *pflag.FlagSet, cfg *Config) { "search-match-annotation", cfg.Annotations.Match, "Annotation to mark secrets or configmaps to match the search", ) + fs.String( + "ignore-annotation", cfg.Annotations.Ignore, + "Annotation to ignore changes on watched resources", + ) fs.String( "pause-deployment-annotation", cfg.Annotations.PausePeriod, "Annotation to define the time period to pause a deployment after a configmap/secret change", @@ -289,23 +313,17 @@ func ApplyFlags(cfg *Config) error { cfg.Annotations.SecretAuto = v.GetString("secret-auto-annotation") cfg.Annotations.ConfigmapReload = v.GetString("configmap-annotation") cfg.Annotations.SecretReload = v.GetString("secret-annotation") + cfg.Annotations.ConfigmapExclude = v.GetString("configmap-exclude-annotation") + cfg.Annotations.SecretExclude = v.GetString("secret-exclude-annotation") + cfg.Annotations.SecretProviderClassAuto = v.GetString("secretproviderclass-auto-annotation") + cfg.Annotations.SecretProviderClassReload = v.GetString("secretproviderclass-annotation") + cfg.Annotations.SecretProviderClassExclude = v.GetString("secretproviderclass-exclude-annotation") cfg.Annotations.Search = v.GetString("auto-search-annotation") cfg.Annotations.Match = v.GetString("search-match-annotation") + cfg.Annotations.Ignore = v.GetString("ignore-annotation") cfg.Annotations.PausePeriod = v.GetString("pause-deployment-annotation") cfg.Annotations.PausedAt = v.GetString("pause-deployment-time-annotation") - // SecretProviderClass annotations have no dedicated CLI flag (parity with - // master); keep the configured defaults. - if cfg.Annotations.SecretProviderClassAuto == "" { - cfg.Annotations.SecretProviderClassAuto = DefaultAnnotations().SecretProviderClassAuto - } - if cfg.Annotations.SecretProviderClassReload == "" { - cfg.Annotations.SecretProviderClassReload = DefaultAnnotations().SecretProviderClassReload - } - if cfg.Annotations.SecretProviderClassExclude == "" { - cfg.Annotations.SecretProviderClassExclude = DefaultAnnotations().SecretProviderClassExclude - } - // Alerting cfg.Alerting.Enabled = v.GetBool("alert-on-reload") cfg.Alerting.WebhookURL = v.GetString("alert-webhook-url") diff --git a/internal/pkg/config/flags_test.go b/internal/pkg/config/flags_test.go index 2d18ab75..f9c6819a 100644 --- a/internal/pkg/config/flags_test.go +++ b/internal/pkg/config/flags_test.go @@ -55,8 +55,14 @@ func TestBindFlags(t *testing.T) { "secret-auto-annotation", "configmap-annotation", "secret-annotation", + "configmap-exclude-annotation", + "secret-exclude-annotation", + "secretproviderclass-auto-annotation", + "secretproviderclass-annotation", + "secretproviderclass-exclude-annotation", "auto-search-annotation", "search-match-annotation", + "ignore-annotation", "pause-deployment-annotation", "pause-deployment-time-annotation", "watch-namespace", @@ -153,6 +159,131 @@ func TestBindFlags_CustomValues(t *testing.T) { } } +func TestApplyFlags_SecretProviderClassAnnotations(t *testing.T) { + // Defaults are preserved when the flags are not provided. + resetViper() + cfg := NewDefault() + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + BindFlags(fs, cfg) + if err := fs.Parse(nil); err != nil { + t.Fatalf("Parse() error = %v", err) + } + if err := ApplyFlags(cfg); err != nil { + t.Fatalf("ApplyFlags() error = %v", err) + } + defaults := DefaultAnnotations() + if cfg.Annotations.SecretProviderClassAuto != defaults.SecretProviderClassAuto { + t.Errorf("SecretProviderClassAuto = %q, want default %q", cfg.Annotations.SecretProviderClassAuto, defaults.SecretProviderClassAuto) + } + if cfg.Annotations.SecretProviderClassReload != defaults.SecretProviderClassReload { + t.Errorf("SecretProviderClassReload = %q, want default %q", cfg.Annotations.SecretProviderClassReload, defaults.SecretProviderClassReload) + } + if cfg.Annotations.SecretProviderClassExclude != defaults.SecretProviderClassExclude { + t.Errorf("SecretProviderClassExclude = %q, want default %q", cfg.Annotations.SecretProviderClassExclude, defaults.SecretProviderClassExclude) + } + + // Custom values are applied from the flags. + resetViper() + cfg = NewDefault() + fs = pflag.NewFlagSet("test", pflag.ContinueOnError) + BindFlags(fs, cfg) + args := []string{ + "--secretproviderclass-auto-annotation=spc.example.com/auto", + "--secretproviderclass-annotation=spc.example.com/reload", + "--secretproviderclass-exclude-annotation=spc.example.com/exclude", + } + if err := fs.Parse(args); err != nil { + t.Fatalf("Parse() error = %v", err) + } + if err := ApplyFlags(cfg); err != nil { + t.Fatalf("ApplyFlags() error = %v", err) + } + if cfg.Annotations.SecretProviderClassAuto != "spc.example.com/auto" { + t.Errorf("SecretProviderClassAuto = %q, want %q", cfg.Annotations.SecretProviderClassAuto, "spc.example.com/auto") + } + if cfg.Annotations.SecretProviderClassReload != "spc.example.com/reload" { + t.Errorf("SecretProviderClassReload = %q, want %q", cfg.Annotations.SecretProviderClassReload, "spc.example.com/reload") + } + if cfg.Annotations.SecretProviderClassExclude != "spc.example.com/exclude" { + t.Errorf("SecretProviderClassExclude = %q, want %q", cfg.Annotations.SecretProviderClassExclude, "spc.example.com/exclude") + } +} + +func TestApplyFlags_ExcludeAnnotations(t *testing.T) { + // Defaults are preserved when the flags are not provided. + resetViper() + cfg := NewDefault() + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + BindFlags(fs, cfg) + if err := fs.Parse(nil); err != nil { + t.Fatalf("Parse() error = %v", err) + } + if err := ApplyFlags(cfg); err != nil { + t.Fatalf("ApplyFlags() error = %v", err) + } + defaults := DefaultAnnotations() + if cfg.Annotations.ConfigmapExclude != defaults.ConfigmapExclude { + t.Errorf("ConfigmapExclude = %q, want default %q", cfg.Annotations.ConfigmapExclude, defaults.ConfigmapExclude) + } + if cfg.Annotations.SecretExclude != defaults.SecretExclude { + t.Errorf("SecretExclude = %q, want default %q", cfg.Annotations.SecretExclude, defaults.SecretExclude) + } + + // Custom values are applied from the flags. + resetViper() + cfg = NewDefault() + fs = pflag.NewFlagSet("test", pflag.ContinueOnError) + BindFlags(fs, cfg) + args := []string{ + "--configmap-exclude-annotation=cm.example.com/exclude", + "--secret-exclude-annotation=sec.example.com/exclude", + } + if err := fs.Parse(args); err != nil { + t.Fatalf("Parse() error = %v", err) + } + if err := ApplyFlags(cfg); err != nil { + t.Fatalf("ApplyFlags() error = %v", err) + } + if cfg.Annotations.ConfigmapExclude != "cm.example.com/exclude" { + t.Errorf("ConfigmapExclude = %q, want %q", cfg.Annotations.ConfigmapExclude, "cm.example.com/exclude") + } + if cfg.Annotations.SecretExclude != "sec.example.com/exclude" { + t.Errorf("SecretExclude = %q, want %q", cfg.Annotations.SecretExclude, "sec.example.com/exclude") + } +} + +func TestApplyFlags_IgnoreAnnotation(t *testing.T) { + // Default is preserved when the flag is not provided. + resetViper() + cfg := NewDefault() + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + BindFlags(fs, cfg) + if err := fs.Parse(nil); err != nil { + t.Fatalf("Parse() error = %v", err) + } + if err := ApplyFlags(cfg); err != nil { + t.Fatalf("ApplyFlags() error = %v", err) + } + if cfg.Annotations.Ignore != DefaultAnnotations().Ignore { + t.Errorf("Ignore = %q, want default %q", cfg.Annotations.Ignore, DefaultAnnotations().Ignore) + } + + // Custom value is applied from the flag. + resetViper() + cfg = NewDefault() + fs = pflag.NewFlagSet("test", pflag.ContinueOnError) + BindFlags(fs, cfg) + if err := fs.Parse([]string{"--ignore-annotation=my.company.com/reloader-ignore"}); err != nil { + t.Fatalf("Parse() error = %v", err) + } + if err := ApplyFlags(cfg); err != nil { + t.Fatalf("ApplyFlags() error = %v", err) + } + if cfg.Annotations.Ignore != "my.company.com/reloader-ignore" { + t.Errorf("Ignore = %q, want %q", cfg.Annotations.Ignore, "my.company.com/reloader-ignore") + } +} + func TestApplyFlags_BooleanStrings(t *testing.T) { tests := []struct { name string diff --git a/internal/pkg/controller/manager.go b/internal/pkg/controller/manager.go index f6f81d97..869bdf00 100644 --- a/internal/pkg/controller/manager.go +++ b/internal/pkg/controller/manager.go @@ -246,7 +246,7 @@ func SetupReconcilers(mgr ctrl.Manager, cfg *config.Config, log logr.Logger, col }, mgr.GetAPIReader(), ) - if err := spcReconciler.SetupWithManager(mgr); err != nil { + if err := SetupSecretProviderClassReconciler(mgr, spcReconciler); err != nil { return fmt.Errorf("setting up secretproviderclass reconciler: %w", err) } log.Info("CSI SecretProviderClass reconciler enabled") diff --git a/internal/pkg/controller/resource_reconciler.go b/internal/pkg/controller/resource_reconciler.go index 0e511d74..1476d642 100644 --- a/internal/pkg/controller/resource_reconciler.go +++ b/internal/pkg/controller/resource_reconciler.go @@ -21,7 +21,9 @@ import ( // ResourceReconcilerDeps holds shared dependencies for resource reconcilers. type ResourceReconcilerDeps struct { - Client client.Client + Client client.Client + // APIReader is an optional non-cached reader for ResolveChange lookups. + APIReader client.Reader Log logr.Logger Config *config.Config ReloadService *reload.Service @@ -47,6 +49,16 @@ type ResourceConfig[T client.Object] struct { // CreatePredicates creates the predicates for this resource type. CreatePredicates func(cfg *config.Config, hasher *reload.Hasher) predicate.Predicate + + // ResolveChange derives the change from a second object instead of CreateChange; + // ok=false skips the event (CSI: change comes from the parent SecretProviderClass). + ResolveChange func(ctx context.Context, reader client.Reader, log logr.Logger, resource T) (reload.ResourceChange, bool) + + // SkipOnNotFound treats a missing object as a no-op (CSI deletes SPCPS as pods roll). + SkipOnNotFound bool + + // BuildFilter overrides the default BuildEventFilter for the watch. + BuildFilter func(cfg *config.Config, hasher *reload.Hasher) predicate.Predicate } // ResourceReconciler is a generic reconciler for ConfigMaps and Secrets. @@ -102,10 +114,17 @@ func (r *ResourceReconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{}, nil } + change, ok := r.buildChange(ctx, log, resource) + if !ok { + r.Collectors.RecordSkipped("resolve_skipped") + r.Collectors.RecordReconcile("success", time.Since(startTime)) + return ctrl.Result{}, nil + } + result, err := r.reloadHandler().Process( - ctx, req.Namespace, req.Name, r.ResourceType, + ctx, change.GetNamespace(), change.GetName(), r.ResourceType, func(workloads []workload.Workload) []reload.ReloadDecision { - return r.ReloadService.Process(r.CreateChange(resource, reload.EventTypeUpdate), workloads) + return r.ReloadService.Process(change, workloads) }, log, ) @@ -113,12 +132,25 @@ func (r *ResourceReconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) return result, err } +// buildChange returns the change via ResolveChange when set, else CreateChange. +func (r *ResourceReconciler[T]) buildChange(ctx context.Context, log logr.Logger, resource T) (reload.ResourceChange, bool) { + if r.ResolveChange != nil { + return r.ResolveChange(ctx, r.APIReader, log, resource) + } + return r.CreateChange(resource, reload.EventTypeUpdate), true +} + func (r *ResourceReconciler[T]) handleNotFound( ctx context.Context, req ctrl.Request, log logr.Logger, startTime time.Time, ) (ctrl.Result, error) { + if r.SkipOnNotFound { + r.Collectors.RecordSkipped("not_found") + r.Collectors.RecordReconcile("success", time.Since(startTime)) + return ctrl.Result{}, nil + } if r.Config.ReloadOnDelete { r.Collectors.RecordEventReceived("delete", string(r.ResourceType)) result, err := r.handleDelete(ctx, req, log) @@ -176,19 +208,19 @@ func (r *ResourceReconciler[T]) reloadHandler() *ReloadHandler { // SetupWithManager sets up the controller with the Manager. func (r *ResourceReconciler[T]) SetupWithManager(mgr ctrl.Manager, forObject T) error { - // Capture the moment the controller is wired up (before the manager starts - // watching). Resources that already exist are replayed during the initial - // cache sync with an older creation timestamp; the create predicate uses - // this to ignore those replays while still honoring genuine creates that - // arrive afterwards. - startTime := time.Now() + var filter predicate.Predicate + if r.BuildFilter != nil { + filter = r.BuildFilter(r.Config, r.ReloadService.Hasher()) + } else { + // time.Now() lets the create predicate ignore initial-sync replays of + // pre-existing resources (older creation timestamps) while honoring later creates. + filter = BuildEventFilter( + r.CreatePredicates(r.Config, r.ReloadService.Hasher()), + r.Config, time.Now(), + ) + } return ctrl.NewControllerManagedBy(mgr). For(forObject). - WithEventFilter( - BuildEventFilter( - r.CreatePredicates(r.Config, r.ReloadService.Hasher()), - r.Config, startTime, - ), - ). + WithEventFilter(filter). Complete(r) } diff --git a/internal/pkg/controller/secretproviderclass_filter_test.go b/internal/pkg/controller/secretproviderclass_filter_test.go new file mode 100644 index 00000000..4c49cc5d --- /dev/null +++ b/internal/pkg/controller/secretproviderclass_filter_test.go @@ -0,0 +1,55 @@ +package controller + +import ( + "testing" + + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/labels" + "sigs.k8s.io/controller-runtime/pkg/event" + csiv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" + + "github.com/stakater/Reloader/internal/pkg/config" + "github.com/stakater/Reloader/internal/pkg/reload" +) + +// TestSecretProviderClassReconciler_FilterIgnoresResourceLabelSelector pins the +// deliberate behavior that --resource-label-selector does NOT filter +// SecretProviderClassPodStatus events (they are CSI-driver-owned and cannot carry +// user labels). If the filter ever regressed to BuildEventFilter (which applies +// LabelSelectorPredicate), the changed-status event below would be dropped. +func TestSecretProviderClassReconciler_FilterIgnoresResourceLabelSelector(t *testing.T) { + cfg := config.NewDefault() + sel, err := labels.Parse("reloader=enabled") + if err != nil { + t.Fatal(err) + } + cfg.ResourceSelectors = []labels.Selector{sel} + + r := NewSecretProviderClassReconciler( + ResourceReconcilerDeps{ + Config: cfg, + Log: logr.Discard(), + ReloadService: reload.NewService(cfg, logr.Discard()), + }, + nil, + ) + + // SPCPS with a changed status and NO matching label. + oldObj := &csiv1.SecretProviderClassPodStatus{ + Status: csiv1.SecretProviderClassPodStatusStatus{ + SecretProviderClassName: "spc", + Objects: []csiv1.SecretProviderClassObject{{ID: "a", Version: "1"}}, + }, + } + newObj := &csiv1.SecretProviderClassPodStatus{ + Status: csiv1.SecretProviderClassPodStatusStatus{ + SecretProviderClassName: "spc", + Objects: []csiv1.SecretProviderClassObject{{ID: "a", Version: "2"}}, + }, + } + + filter := r.BuildFilter(r.Config, r.ReloadService.Hasher()) + if !filter.Update(event.UpdateEvent{ObjectOld: oldObj, ObjectNew: newObj}) { + t.Fatal("SPCPS status change must pass the filter even when --resource-label-selector is set") + } +} diff --git a/internal/pkg/controller/secretproviderclass_reconciler.go b/internal/pkg/controller/secretproviderclass_reconciler.go index 1c2691cf..a4c17511 100644 --- a/internal/pkg/controller/secretproviderclass_reconciler.go +++ b/internal/pkg/controller/secretproviderclass_reconciler.go @@ -2,168 +2,90 @@ package controller import ( "context" - "time" + "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" csiv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" + "github.com/stakater/Reloader/internal/pkg/config" "github.com/stakater/Reloader/internal/pkg/reload" - "github.com/stakater/Reloader/internal/pkg/workload" ) -// SecretProviderClassReconciler watches SecretProviderClassPodStatus objects and -// triggers workload reloads when the secret versions they track change. -// -// It watches SecretProviderClassPodStatus (the per-pod status written by the CSI -// driver) rather than SecretProviderClass directly, because only the pod status -// carries the current object IDs and versions that indicate a secret rotation. -type SecretProviderClassReconciler struct { - ResourceReconcilerDeps +// SecretProviderClassReconciler watches SecretProviderClassPodStatus (the per-pod +// status the CSI driver rewrites on rotation) and reloads matching workloads, +// reusing the generic reconciler via a ResolveChange hook. +type SecretProviderClassReconciler = ResourceReconciler[*csiv1.SecretProviderClassPodStatus] - // apiReader is a direct API client (not cached) used to look up the parent - // SecretProviderClass object. In tests this is set to the fake client. - apiReader client.Reader - - handler *ReloadHandler -} - -// NewSecretProviderClassReconciler creates a new SecretProviderClassReconciler. +// NewSecretProviderClassReconciler builds the reconciler. apiReader (non-cached) +// looks up the parent SecretProviderClass without starting a second informer. func NewSecretProviderClassReconciler(deps ResourceReconcilerDeps, apiReader client.Reader) *SecretProviderClassReconciler { - return &SecretProviderClassReconciler{ - ResourceReconcilerDeps: deps, - apiReader: apiReader, - } + deps.APIReader = apiReader + return NewResourceReconciler( + deps, + ResourceConfig[*csiv1.SecretProviderClassPodStatus]{ + ResourceType: reload.ResourceTypeSecretProviderClass, + NewResource: func() *csiv1.SecretProviderClassPodStatus { return &csiv1.SecretProviderClassPodStatus{} }, + ResolveChange: resolveSecretProviderClassChange, + SkipOnNotFound: true, + BuildFilter: secretProviderClassFilter, + }, + ) } -// Reconcile handles a SecretProviderClassPodStatus event. -func (r *SecretProviderClassReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - startTime := time.Now() - resourceType := string(reload.ResourceTypeSecretProviderClass) - log := r.Log.WithValues("secretproviderclasspodstatus", req.NamespacedName) - - r.Collectors.RecordEventReceived("reconcile", resourceType) - - spcps := &csiv1.SecretProviderClassPodStatus{} - if err := r.Client.Get(ctx, req.NamespacedName, spcps); err != nil { - if errors.IsNotFound(err) { - r.Collectors.RecordSkipped("not_found") - r.Collectors.RecordReconcile("success", time.Since(startTime)) - return ctrl.Result{}, nil - } - log.Error(err, "failed to get SecretProviderClassPodStatus") - r.Collectors.RecordError("get_secretproviderclasspodstatus") - r.Collectors.RecordReconcile("error", time.Since(startTime)) - return ctrl.Result{}, err - } - - namespace := spcps.GetNamespace() - if r.Config.IsNamespaceIgnored(namespace) { - log.V(1).Info("skipping SecretProviderClassPodStatus in ignored namespace") - r.Collectors.RecordSkipped("ignored_namespace") - r.Collectors.RecordReconcile("success", time.Since(startTime)) - return ctrl.Result{}, nil - } - - if r.NamespaceCache != nil && r.NamespaceCache.IsEnabled() && !r.NamespaceCache.Contains(namespace) { - log.V(1).Info("skipping SecretProviderClassPodStatus in namespace not matching selector", "namespace", namespace) - r.Collectors.RecordSkipped("namespace_selector") - r.Collectors.RecordReconcile("success", time.Since(startTime)) - return ctrl.Result{}, nil - } - - spcName, spcAnnotations := r.resolveSPCAnnotations(ctx, spcps) +// resolveSecretProviderClassChange builds the change from an SPCPS: it reads the +// SPC name from the status and looks up the SPC for its annotations. On any lookup +// error it proceeds with empty annotations so annotation-matched workloads still +// reload (master parity); an empty SPC name skips the event. +func resolveSecretProviderClassChange( + ctx context.Context, + reader client.Reader, + log logr.Logger, + spcps *csiv1.SecretProviderClassPodStatus, +) (reload.ResourceChange, bool) { + spcName := spcps.Status.SecretProviderClassName if spcName == "" { - r.Collectors.RecordSkipped("no_spc_name") - r.Collectors.RecordReconcile("success", time.Since(startTime)) - return ctrl.Result{}, nil + return nil, false } - change := reload.SecretProviderClassChange{ + annotations := map[string]string{} + spc := &csiv1.SecretProviderClass{} + if err := reader.Get(ctx, types.NamespacedName{Name: spcName, Namespace: spcps.GetNamespace()}, spc); err != nil { + if errors.IsNotFound(err) { + log.Info("SecretProviderClass not found; proceeding without its annotations", "spc", spcName) + } else { + log.V(1).Error(err, "failed to get SecretProviderClass; proceeding without its annotations", "spc", spcName) + } + } else if a := spc.GetAnnotations(); a != nil { + annotations = a + } + + return reload.SecretProviderClassChange{ Name: spcName, - Namespace: namespace, - Annotations: spcAnnotations, + Namespace: spcps.GetNamespace(), + Annotations: annotations, Status: spcps.Status, EventType: reload.EventTypeUpdate, - } + }, true +} - result, err := r.reloadHandler().Process( - ctx, namespace, spcName, reload.ResourceTypeSecretProviderClass, - func(workloads []workload.Workload) []reload.ReloadDecision { - return r.ReloadService.Process(change, workloads) - }, log, +// secretProviderClassFilter omits the label selector (driver-owned SPCPS can't +// carry user labels) and the namespace cache (checked in Reconcile to avoid a +// startup race). See docs/manual-testing-csi.md. +func secretProviderClassFilter(cfg *config.Config, hasher *reload.Hasher) predicate.Predicate { + return reload.CombinedPredicates( + reload.NamespaceFilterPredicateWithCache(cfg, nil), + reload.SecretProviderClassPodStatusPredicates(cfg, hasher), ) - - if err != nil { - r.Collectors.RecordReconcile("error", time.Since(startTime)) - } else { - r.Collectors.RecordReconcile("success", time.Since(startTime)) - } - return result, err } -// resolveSPCAnnotations looks up the SecretProviderClass referenced by the -// given pod status and returns its name and annotations. It never returns an -// error: on any Get failure it logs and returns the SPC name (from the pod -// status) with an empty annotations map, so callers can still process workloads -// that match via their own auto/named annotations. This matches master's -// behaviour in populateAnnotationsFromSecretProviderClass. -func (r *SecretProviderClassReconciler) resolveSPCAnnotations( - ctx context.Context, - spcps *csiv1.SecretProviderClassPodStatus, -) (string, map[string]string) { - spcName := spcps.Status.SecretProviderClassName - spc := &csiv1.SecretProviderClass{} - if err := r.apiReader.Get(ctx, types.NamespacedName{ - Name: spcName, - Namespace: spcps.GetNamespace(), - }, spc); err != nil { - if errors.IsNotFound(err) { - r.Log.WithValues("spc", spcName).Info("SecretProviderClass not found; proceeding without its annotations") - } else { - r.Log.V(1).Error(err, "failed to get SecretProviderClass; proceeding without its annotations", "spc", spcName) - } - return spcName, map[string]string{} - } - annotations := spc.GetAnnotations() - if annotations == nil { - annotations = map[string]string{} - } - return spc.Name, annotations -} - -func (r *SecretProviderClassReconciler) reloadHandler() *ReloadHandler { - if r.handler == nil { - r.handler = &ReloadHandler{ - Client: r.Client, - Lister: workload.NewLister(r.Client, r.Registry, r.Config), - ReloadService: r.ReloadService, - WebhookClient: r.WebhookClient, - Collectors: r.Collectors, - EventRecorder: r.EventRecorder, - Alerter: r.Alerter, - PauseHandler: r.PauseHandler, - } - } - return r.handler -} - -// SetupWithManager wires the reconciler to watch SecretProviderClassPodStatus. -func (r *SecretProviderClassReconciler) SetupWithManager(mgr ctrl.Manager) error { - var nsChecker reload.NamespaceChecker - if r.NamespaceCache != nil { - nsChecker = r.NamespaceCache - } - return ctrl.NewControllerManagedBy(mgr). - For(&csiv1.SecretProviderClassPodStatus{}). - WithEventFilter(reload.CombinedPredicates( - reload.NamespaceFilterPredicateWithCache(r.Config, nsChecker), - reload.SecretProviderClassPodStatusPredicates(r.Config, r.ReloadService.Hasher()), - )). - Complete(r) +// SetupSecretProviderClassReconciler sets up the reconciler with the manager. +func SetupSecretProviderClassReconciler(mgr ctrl.Manager, r *SecretProviderClassReconciler) error { + return r.SetupWithManager(mgr, &csiv1.SecretProviderClassPodStatus{}) } var _ reconcile.Reconciler = &SecretProviderClassReconciler{} diff --git a/internal/pkg/controller/secretproviderclass_reconciler_test.go b/internal/pkg/controller/secretproviderclass_reconciler_test.go index 8664996a..2ba6bae7 100644 --- a/internal/pkg/controller/secretproviderclass_reconciler_test.go +++ b/internal/pkg/controller/secretproviderclass_reconciler_test.go @@ -169,3 +169,31 @@ func TestSecretProviderClassReconciler_SPCNotFound(t *testing.T) { expectedEnvVar, updated.Spec.Template.Spec.Containers[0].Env) } } + +func TestSecretProviderClassReconciler_EmptySPCName(t *testing.T) { + cfg := config.NewDefault() + // SPCPS whose Status carries no SecretProviderClassName must be skipped cleanly. + spcps := &csiv1.SecretProviderClassPodStatus{ + ObjectMeta: metav1.ObjectMeta{Name: "orphan-spcps", Namespace: "default"}, + Status: csiv1.SecretProviderClassPodStatusStatus{}, + } + deployment := testutil.NewDeployment("test-deployment", "default", map[string]string{ + cfg.Annotations.SecretProviderClassAuto: "true", + }) + reconciler, cl := newSecretProviderClassReconcilerWithClient(t, cfg, deployment, spcps) + + if _, err := reconciler.Reconcile(context.Background(), reconcileRequest("orphan-spcps", "default")); err != nil { + t.Fatalf("Reconcile error: %v", err) + } + + // No reload should have happened (no SPC name to match against). + updated := &appsv1.Deployment{} + if err := cl.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "test-deployment"}, updated); err != nil { + t.Fatal(err) + } + for _, c := range updated.Spec.Template.Spec.Containers { + if len(c.Env) != 0 { + t.Errorf("expected no env vars injected for empty SPC name, got %+v", c.Env) + } + } +} diff --git a/internal/pkg/reload/predicate_test.go b/internal/pkg/reload/predicate_test.go index 85aacaa4..f62e292c 100644 --- a/internal/pkg/reload/predicate_test.go +++ b/internal/pkg/reload/predicate_test.go @@ -968,4 +968,19 @@ func TestSecretProviderClassPodStatusPredicates(t *testing.T) { if p.Update(event.UpdateEvent{ObjectOld: oldObj, ObjectNew: newObjSame}) { t.Fatal("UpdateFunc should return false on unchanged status") } + + // A metadata/label-only change (same Status) must NOT trigger a reload, + // since the predicate hashes only the status. (Master tested this via + // UpdateSecretProviderClassPodStatusLabels.) + labelOnly := oldObj.DeepCopy() + labelOnly.Labels = map[string]string{"unrelated": "changed"} + labelOnly.Annotations = map[string]string{"note": "touched"} + if p.Update(event.UpdateEvent{ObjectOld: oldObj, ObjectNew: labelOnly}) { + t.Fatal("UpdateFunc should return false on a label/metadata-only change") + } + + // Type-assertion failure (wrong object type) must be rejected, not panic. + if p.Update(event.UpdateEvent{ObjectOld: &corev1.ConfigMap{}, ObjectNew: &corev1.ConfigMap{}}) { + t.Fatal("UpdateFunc should return false when objects are not SPCPS") + } } diff --git a/internal/pkg/reload/service.go b/internal/pkg/reload/service.go index e712f90d..346539d6 100644 --- a/internal/pkg/reload/service.go +++ b/internal/pkg/reload/service.go @@ -268,6 +268,11 @@ func (s *Service) findVolumeUsingResource(volumes []corev1.Volume, resourceName } } } + case ResourceTypeSecretProviderClass: + // Match the CSI volume that references this SPC. + if vol.CSI != nil && vol.CSI.VolumeAttributes["secretProviderClass"] == resourceName { + return vol.Name + } } } return "" diff --git a/internal/pkg/reload/service_test.go b/internal/pkg/reload/service_test.go index 6141e2c5..9d12554d 100644 --- a/internal/pkg/reload/service_test.go +++ b/internal/pkg/reload/service_test.go @@ -5,6 +5,7 @@ import ( "testing" "github.com/go-logr/logr/testr" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" csiv1 "sigs.k8s.io/secrets-store-csi-driver/apis/v1" @@ -1385,3 +1386,88 @@ func TestService_ProcessCreateEventDisabled(t *testing.T) { t.Errorf("Expected nil decisions when create events disabled, got %v", decisions) } } + +func TestService_ApplyReload_SPC_TargetsMountingContainer(t *testing.T) { + cfg := config.NewDefault() + cfg.ReloadStrategy = config.ReloadStrategyEnvVars + svc := NewService(cfg, testr.New(t)) + + // Two containers; only the second mounts the CSI volume that references the SPC. + dep := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "multi", Namespace: "default"}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "c0"}, + {Name: "c1", VolumeMounts: []corev1.VolumeMount{{Name: "spc-vol", MountPath: "/mnt/secrets-store"}}}, + }, + Volumes: []corev1.Volume{ + { + Name: "spc-vol", + VolumeSource: corev1.VolumeSource{ + CSI: &corev1.CSIVolumeSource{ + Driver: "secrets-store.csi.k8s.io", + VolumeAttributes: map[string]string{"secretProviderClass": "my-spc"}, + }, + }, + }, + }, + }, + }, + }, + } + accessor := workload.NewDeploymentWorkload(dep) + + // autoReload=true exercises volume-based container targeting. + updated, err := svc.ApplyReload(context.Background(), accessor, "my-spc", ResourceTypeSecretProviderClass, "default", "spchash", true) + if err != nil { + t.Fatalf("ApplyReload failed: %v", err) + } + if !updated { + t.Fatal("expected updated=true") + } + + containers := accessor.GetContainers() + const envName = "STAKATER_MY_SPC_SECRETPROVIDERCLASS" + hasEnv := func(c corev1.Container) bool { + for _, e := range c.Env { + if e.Name == envName { + return true + } + } + return false + } + if hasEnv(containers[0]) { + t.Error("env var must NOT land on container[0] (it does not mount the SPC volume)") + } + if !hasEnv(containers[1]) { + t.Error("env var must land on container[1], which mounts the SPC CSI volume") + } +} + +func TestService_ProcessSecretProviderClass_GenericAuto(t *testing.T) { + cfg := config.NewDefault() + svc := NewService(cfg, testr.New(t)) + + // Generic auto annotation (not the typed SPC one) must also trigger an SPC reload. + deploy := testutil.NewDeployment("test-deploy", "default", map[string]string{ + "reloader.stakater.com/auto": "true", + }) + workloads := []workload.Workload{workload.NewDeploymentWorkload(deploy)} + + change := SecretProviderClassChange{ + Name: "my-spc", + Namespace: "default", + Status: csiv1.SecretProviderClassPodStatusStatus{ + SecretProviderClassName: "my-spc", + Objects: []csiv1.SecretProviderClassObject{{ID: "a", Version: "1"}}, + }, + EventType: EventTypeUpdate, + } + + decisions := svc.Process(change, workloads) + if len(decisions) != 1 || !decisions[0].ShouldReload { + t.Fatalf("expected generic-auto SPC reload, got %+v", decisions) + } +} diff --git a/internal/pkg/reload/strategy.go b/internal/pkg/reload/strategy.go index 3c3d9b7b..8881362e 100644 --- a/internal/pkg/reload/strategy.go +++ b/internal/pkg/reload/strategy.go @@ -179,8 +179,15 @@ func (s *AnnotationStrategy) Apply(input StrategyInput) (bool, error) { annotationKey := s.cfg.Annotations.LastReloadedFrom existingValue := input.PodAnnotations[annotationKey] - if existingValue == string(sourceJSON) { - return false, nil + // Idempotent on kind+name+hash, ignoring ReloadedAt: a timestamped compare + // would force a rollout every reconcile (one CSI rotation fans out to N + // SecretProviderClassPodStatus updates → N rollouts). + if existingValue != "" { + var prev ReloadSource + if err := json.Unmarshal([]byte(existingValue), &prev); err == nil && + prev.Kind == source.Kind && prev.Name == source.Name && prev.Hash == source.Hash { + return false, nil + } } input.PodAnnotations[annotationKey] = string(sourceJSON) diff --git a/internal/pkg/reload/strategy_test.go b/internal/pkg/reload/strategy_test.go index 57451597..30571135 100644 --- a/internal/pkg/reload/strategy_test.go +++ b/internal/pkg/reload/strategy_test.go @@ -255,6 +255,50 @@ func TestAnnotationStrategy_Apply(t *testing.T) { } }) + t.Run("idempotent for same resource and hash (ignores timestamp)", func(t *testing.T) { + annotations := make(map[string]string) + input := StrategyInput{ + ResourceName: "my-config", + ResourceType: ResourceTypeConfigMap, + Namespace: "default", + Hash: "abc123", + Container: &corev1.Container{Name: "c"}, + PodAnnotations: annotations, + } + + changed, err := strategy.Apply(input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !changed { + t.Fatal("expected changed=true on first apply") + } + firstValue := annotations[cfg.Annotations.LastReloadedFrom] + + // Re-applying the identical change must NOT report a change, even though + // a fresh ReloadedAt timestamp would make the serialized value differ. + changed, err = strategy.Apply(input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if changed { + t.Error("expected changed=false when re-applying the same resource+hash") + } + if annotations[cfg.Annotations.LastReloadedFrom] != firstValue { + t.Error("annotation value must not change on an idempotent re-apply") + } + + // A different hash (real content change) must trigger an update. + input.Hash = "def456" + changed, err = strategy.Apply(input) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !changed { + t.Error("expected changed=true when the hash changes") + } + }) + t.Run("error when annotations map is nil", func(t *testing.T) { input := StrategyInput{ ResourceName: "my-config",