refactoring

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")

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)
 }

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")
+	}
+}

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{}

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)
+		}
+	}
+}