diff --git a/internal/pkg/controller/filter.go b/internal/pkg/controller/filter.go index a66279ff..e1ba2f51 100644 --- a/internal/pkg/controller/filter.go +++ b/internal/pkg/controller/filter.go @@ -1,6 +1,8 @@ package controller import ( + "time" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -9,23 +11,37 @@ import ( ) // BuildEventFilter combines a resource-specific predicate with common filters. -func BuildEventFilter(resourcePredicate predicate.Predicate, cfg *config.Config, initialized *bool) predicate.Predicate { +// +// startTime is the moment the controller began watching; it is used to tell +// genuine post-startup creates apart from the initial-sync replay of +// pre-existing resources (which the informer delivers as create events). +func BuildEventFilter(resourcePredicate predicate.Predicate, cfg *config.Config, startTime time.Time) predicate.Predicate { return predicate.And( resourcePredicate, reload.NamespaceFilterPredicate(cfg), reload.LabelSelectorPredicate(cfg), reload.IgnoreAnnotationPredicate(cfg), - createEventPredicate(cfg, initialized), + createEventPredicate(cfg, startTime), ) } -func createEventPredicate(cfg *config.Config, initialized *bool) predicate.Predicate { +func createEventPredicate(cfg *config.Config, startTime time.Time) predicate.Predicate { return predicate.Funcs{ CreateFunc: func(e event.CreateEvent) bool { - if !*initialized && !cfg.SyncAfterRestart { + if !cfg.ReloadOnCreate { return false } - return cfg.ReloadOnCreate + // SyncAfterRestart processes every create, including the + // initial-sync replay of resources that already existed. + if cfg.SyncAfterRestart { + return true + } + // Otherwise only honor resources created after the controller + // started. Resources replayed during the initial cache sync carry + // an older creation timestamp and must not trigger reloads on + // startup, but a genuine create that arrives afterwards must be + // honored even if it is the very first event this controller sees. + return e.Object.GetCreationTimestamp().Time.After(startTime) }, UpdateFunc: func(e event.UpdateEvent) bool { return true diff --git a/internal/pkg/controller/filter_test.go b/internal/pkg/controller/filter_test.go index be6eec36..16b2ae8e 100644 --- a/internal/pkg/controller/filter_test.go +++ b/internal/pkg/controller/filter_test.go @@ -2,6 +2,7 @@ package controller import ( "testing" + "time" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -11,47 +12,59 @@ import ( ) func TestCreateEventPredicate_CreateEvent(t *testing.T) { + startTime := time.Date(2024, 1, 1, 12, 0, 0, 0, time.UTC) + tests := []struct { name string reloadOnCreate bool syncAfterRestart bool - initialized bool - expectedResult bool + // createdAfterStart controls the resource's creation timestamp relative + // to the controller start time: true => created after start (a genuine + // post-startup create), false => created before start (initial-sync replay). + createdAfterStart bool + expectedResult bool }{ { - name: "reload on create enabled, initialized", - reloadOnCreate: true, - syncAfterRestart: false, - initialized: true, - expectedResult: true, + // Regression: a genuine create after startup must be honored even + // when it is the very first event the controller sees (no prior + // reconcile). This is the reloadOnCreate e2e scenario. + name: "reload on create enabled, created after start", + reloadOnCreate: true, + syncAfterRestart: false, + createdAfterStart: true, + expectedResult: true, }, { - name: "reload on create disabled, initialized", - reloadOnCreate: false, - syncAfterRestart: false, - initialized: true, - expectedResult: false, + // Pre-existing resources replayed during initial sync must not + // trigger reloads on startup. + name: "reload on create enabled, created before start (initial sync replay)", + reloadOnCreate: true, + syncAfterRestart: false, + createdAfterStart: false, + expectedResult: false, }, { - name: "not initialized, sync after restart enabled", - reloadOnCreate: true, - syncAfterRestart: true, - initialized: false, - expectedResult: true, + name: "reload on create disabled", + reloadOnCreate: false, + syncAfterRestart: false, + createdAfterStart: true, + expectedResult: false, }, { - name: "not initialized, sync after restart disabled", - reloadOnCreate: true, - syncAfterRestart: false, - initialized: false, - expectedResult: false, + // SyncAfterRestart processes every create, including initial-sync + // replays of pre-existing resources. + name: "sync after restart honors pre-existing create", + reloadOnCreate: true, + syncAfterRestart: true, + createdAfterStart: false, + expectedResult: true, }, { - name: "not initialized, sync after restart disabled, reload on create disabled", - reloadOnCreate: false, - syncAfterRestart: false, - initialized: false, - expectedResult: false, + name: "sync after restart but reload on create disabled", + reloadOnCreate: false, + syncAfterRestart: true, + createdAfterStart: true, + expectedResult: false, }, } @@ -62,12 +75,20 @@ func TestCreateEventPredicate_CreateEvent(t *testing.T) { ReloadOnCreate: tt.reloadOnCreate, SyncAfterRestart: tt.syncAfterRestart, } - initialized := tt.initialized - pred := createEventPredicate(cfg, &initialized) + pred := createEventPredicate(cfg, startTime) + + creationTime := startTime.Add(-time.Hour) + if tt.createdAfterStart { + creationTime = startTime.Add(time.Hour) + } cm := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "default", + CreationTimestamp: metav1.NewTime(creationTime), + }, } e := event.CreateEvent{Object: cm} @@ -83,9 +104,8 @@ func TestCreateEventPredicate_CreateEvent(t *testing.T) { func TestCreateEventPredicate_UpdateEvent(t *testing.T) { cfg := &config.Config{} - initialized := true - pred := createEventPredicate(cfg, &initialized) + pred := createEventPredicate(cfg, time.Now()) cm := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, @@ -123,9 +143,8 @@ func TestCreateEventPredicate_DeleteEvent(t *testing.T) { cfg := &config.Config{ ReloadOnDelete: tt.reloadOnDelete, } - initialized := true - pred := createEventPredicate(cfg, &initialized) + pred := createEventPredicate(cfg, time.Now()) cm := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, @@ -144,9 +163,8 @@ func TestCreateEventPredicate_DeleteEvent(t *testing.T) { func TestCreateEventPredicate_GenericEvent(t *testing.T) { cfg := &config.Config{} - initialized := true - pred := createEventPredicate(cfg, &initialized) + pred := createEventPredicate(cfg, time.Now()) cm := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, @@ -165,11 +183,10 @@ func TestBuildEventFilter(t *testing.T) { ReloadOnCreate: true, ReloadOnDelete: true, } - initialized := true resourcePred := &alwaysTruePredicate{} - filter := BuildEventFilter(resourcePred, cfg, &initialized) + filter := BuildEventFilter(resourcePred, cfg, time.Now()) if filter == nil { t.Fatal("BuildEventFilter() should return a non-nil predicate") diff --git a/internal/pkg/controller/resource_reconciler.go b/internal/pkg/controller/resource_reconciler.go index 7bfdd805..0e511d74 100644 --- a/internal/pkg/controller/resource_reconciler.go +++ b/internal/pkg/controller/resource_reconciler.go @@ -2,7 +2,6 @@ package controller import ( "context" - "sync" "time" "github.com/go-logr/logr" @@ -55,9 +54,7 @@ type ResourceReconciler[T client.Object] struct { ResourceReconcilerDeps ResourceConfig[T] - handler *ReloadHandler - initialized bool - initOnce sync.Once + handler *ReloadHandler } // NewResourceReconciler creates a new generic resource reconciler. @@ -77,13 +74,6 @@ func (r *ResourceReconciler[T]) Reconcile(ctx context.Context, req ctrl.Request) resourceType := string(r.ResourceType) log := r.Log.WithValues(resourceType, req.NamespacedName) - r.initOnce.Do( - func() { - r.initialized = true - log.Info(resourceType + " controller initialized") - }, - ) - r.Collectors.RecordEventReceived("reconcile", resourceType) resource := r.NewResource() @@ -184,19 +174,20 @@ func (r *ResourceReconciler[T]) reloadHandler() *ReloadHandler { return r.handler } -// Initialized returns whether the reconciler has been initialized. -func (r *ResourceReconciler[T]) Initialized() *bool { - return &r.initialized -} - // 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() return ctrl.NewControllerManagedBy(mgr). For(forObject). WithEventFilter( BuildEventFilter( r.CreatePredicates(r.Config, r.ReloadService.Hasher()), - r.Config, r.Initialized(), + r.Config, startTime, ), ). Complete(r)