From 050d2656f251639fab604060a4114cf6669ebe84 Mon Sep 17 00:00:00 2001 From: Rishabh Singh Date: Wed, 2 Sep 2026 15:29:32 +0530 Subject: [PATCH 1/2] Tolerate missing config ConfigMaps at webhook startup The config Store registers its ConfigMaps with the InformedWatcher using plain Watch(), so the watcher fatals at startup if config-defaults, config-features, or config-autoscaler is absent. On a cold start this deadlocks against the config validating webhook (failurePolicy: Fail), which cannot admit the ConfigMaps while the webhook itself is down. Override Store.WatchConfigs to use WatchWithDefault when the watcher is a DefaultingWatcher, registering an empty default per ConfigMap so startup no longer fails. Real ConfigMap values are observed as soon as they appear. Fixes #16658 Signed-off-by: Rishabh Singh --- pkg/apis/config/store.go | 32 +++++++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/pkg/apis/config/store.go b/pkg/apis/config/store.go index c88fff6527db..130e1abfbdeb 100644 --- a/pkg/apis/config/store.go +++ b/pkg/apis/config/store.go @@ -19,6 +19,8 @@ package config import ( "context" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/configmap" asconfig "knative.dev/serving/pkg/autoscaler/config" "knative.dev/serving/pkg/autoscaler/config/autoscalerconfig" @@ -76,17 +78,19 @@ type Store struct { *configmap.UntypedStore } +var configConstructors = configmap.Constructors{ + DefaultsConfigName: NewDefaultsConfigFromConfigMap, + FeaturesConfigName: NewFeaturesConfigFromConfigMap, + asconfig.ConfigName: asconfig.NewConfigFromConfigMap, +} + // NewStore creates a new store of Configs and optionally calls functions when ConfigMaps are updated. func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value interface{})) *Store { store := &Store{ UntypedStore: configmap.NewUntypedStore( "apis", logger, - configmap.Constructors{ - DefaultsConfigName: NewDefaultsConfigFromConfigMap, - FeaturesConfigName: NewFeaturesConfigFromConfigMap, - asconfig.ConfigName: asconfig.NewConfigFromConfigMap, - }, + configConstructors, onAfterStore..., ), } @@ -94,6 +98,24 @@ func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value i return store } +// WatchConfigs registers watches for all ConfigMaps in the Store using WatchWithDefault +// to ensure the watcher doesn't fail if ConfigMaps are missing during startup. +func (s *Store) WatchConfigs(w configmap.Watcher) { + // Check if the watcher supports defaults (DefaultingWatcher interface) + if dw, ok := w.(configmap.DefaultingWatcher); ok { + // Use WatchWithDefault to register defaults with the watcher + // This prevents Start() from failing if ConfigMaps don't exist + for name := range configConstructors { + dw.WatchWithDefault(corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + }, s.UntypedStore.OnConfigChanged) + } + } else { + // Fallback to regular Watch if DefaultingWatcher not supported + s.UntypedStore.WatchConfigs(w) + } +} + // ToContext attaches the current Config state to the provided context. func (s *Store) ToContext(ctx context.Context) context.Context { return ToContext(ctx, s.Load()) From ec7bd4766bce3a83f340209dab065060a2f95d5a Mon Sep 17 00:00:00 2001 From: Rishabh Singh Date: Sat, 5 Sep 2026 22:50:34 +0530 Subject: [PATCH 2/2] Address review: webhook scoped, add namespace param and tests **Webhook-specific scoping:** - Renamed WatchConfigs -> WatchConfigsWithDefaults - Revision controller continues using WatchConfigs (fail-fast behavior) **Fixes:** - Nit 1: Add namespace parameter, pass system.Namespace() - Nit 2: Add unit tests for the new default configuration watcher behavior. The tests verify that all required ConfigMaps are registered with the watcher, the correct namespace is used, default configuration updates the Store through the registered callbacks, real ConfigMaps replace the defaults, and watchers that do not support defaulting continue to use the existing Watch behavior. Signed-off-by: Rishabh Singh --- cmd/webhook/main.go | 5 +- pkg/apis/config/store.go | 22 ++-- pkg/apis/config/store_test.go | 217 ++++++++++++++++++++++++++++++++++ 3 files changed, 235 insertions(+), 9 deletions(-) diff --git a/cmd/webhook/main.go b/cmd/webhook/main.go index 46f270952e00..e2d11fbc8587 100644 --- a/cmd/webhook/main.go +++ b/cmd/webhook/main.go @@ -26,6 +26,7 @@ import ( "knative.dev/pkg/leaderelection" "knative.dev/pkg/logging" "knative.dev/pkg/signals" + "knative.dev/pkg/system" "knative.dev/pkg/webhook" "knative.dev/pkg/webhook/certificates" "knative.dev/pkg/webhook/configmaps" @@ -81,7 +82,7 @@ var callbacks = map[schema.GroupVersionKind]validation.Callback{ func newDefaultingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl { // Decorate contexts with the current state of the config. store := apisconfig.NewStore(logging.FromContext(ctx).Named("config-store")) - store.WatchConfigs(cmw) + store.WatchConfigsWithDefaults(cmw, system.Namespace()) return defaulting.NewAdmissionController(ctx, @@ -106,7 +107,7 @@ func newDefaultingAdmissionController(ctx context.Context, cmw configmap.Watcher func newValidationAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl { // Decorate contexts with the current state of the config. store := apisconfig.NewStore(logging.FromContext(ctx).Named("config-store")) - store.WatchConfigs(cmw) + store.WatchConfigsWithDefaults(cmw, system.Namespace()) return validation.NewAdmissionController(ctx, diff --git a/pkg/apis/config/store.go b/pkg/apis/config/store.go index 130e1abfbdeb..a68c4afea442 100644 --- a/pkg/apis/config/store.go +++ b/pkg/apis/config/store.go @@ -98,16 +98,24 @@ func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value i return store } -// WatchConfigs registers watches for all ConfigMaps in the Store using WatchWithDefault -// to ensure the watcher doesn't fail if ConfigMaps are missing during startup. -func (s *Store) WatchConfigs(w configmap.Watcher) { - // Check if the watcher supports defaults (DefaultingWatcher interface) +// WatchConfigsWithDefaults is like WatchConfigs but uses WatchWithDefault to register +// default ConfigMaps when the watcher supports it. This allows the watcher to tolerate +// missing ConfigMaps at startup time, preventing the circular dependency where: +// - the webhook cannot start without ConfigMaps present, but +// - ConfigMaps cannot be created/validated while the webhook is down. +// +// This method should only be used by the webhook. Other controllers (like revision +// controller) may intentionally want to fail-fast if required ConfigMaps are missing. +func (s *Store) WatchConfigsWithDefaults(w configmap.Watcher, namespace string) { if dw, ok := w.(configmap.DefaultingWatcher); ok { - // Use WatchWithDefault to register defaults with the watcher - // This prevents Start() from failing if ConfigMaps don't exist + // Use WatchWithDefault to register defaults with the watcher. + // This prevents Start() from failing if ConfigMaps don't exist yet. for name := range configConstructors { dw.WatchWithDefault(corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: name}, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, }, s.UntypedStore.OnConfigChanged) } } else { diff --git a/pkg/apis/config/store_test.go b/pkg/apis/config/store_test.go index 60f010734575..da6fb1b0e292 100644 --- a/pkg/apis/config/store_test.go +++ b/pkg/apis/config/store_test.go @@ -18,12 +18,16 @@ package config import ( "context" + "sync" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + "knative.dev/pkg/configmap" logtesting "knative.dev/pkg/logging/testing" + "knative.dev/pkg/system" . "knative.dev/pkg/configmap/testing" autoscalerconfig "knative.dev/serving/pkg/autoscaler/config" @@ -123,3 +127,216 @@ func TestStoreImmutableConfig(t *testing.T) { t.Error("Autoscaler config is not immutable") } } + +// mockDefaultingWatcher implements configmap.DefaultingWatcher for testing +type mockDefaultingWatcher struct { + mu sync.Mutex + watchedDefaults map[string]corev1.ConfigMap + watchedCallbacks map[string][]configmap.Observer +} + +func newMockDefaultingWatcher() *mockDefaultingWatcher { + return &mockDefaultingWatcher{ + watchedDefaults: make(map[string]corev1.ConfigMap), + watchedCallbacks: make(map[string][]configmap.Observer), + } +} + +func (m *mockDefaultingWatcher) WatchWithDefault(cm corev1.ConfigMap, observers ...configmap.Observer) { + m.mu.Lock() + defer m.mu.Unlock() + m.watchedDefaults[cm.Name] = cm + m.watchedCallbacks[cm.Name] = observers +} + +func (m *mockDefaultingWatcher) Watch(name string, observers ...configmap.Observer) { + m.mu.Lock() + defer m.mu.Unlock() + m.watchedCallbacks[name] = observers +} + +func (m *mockDefaultingWatcher) Start(<-chan struct{}) error { + return nil +} + +func (m *mockDefaultingWatcher) getWatchedDefault(name string) (corev1.ConfigMap, bool) { + m.mu.Lock() + defer m.mu.Unlock() + cm, ok := m.watchedDefaults[name] + return cm, ok +} + +func (m *mockDefaultingWatcher) triggerCallback(cm *corev1.ConfigMap) { + m.mu.Lock() + callbacks := m.watchedCallbacks[cm.Name] + m.mu.Unlock() + + for _, cb := range callbacks { + cb(cm) + } +} + +// mockWatcher implements configmap.Watcher (without DefaultingWatcher) for testing +type mockWatcher struct { + mu sync.Mutex + watchedNames []string + watchedCallbacks map[string][]configmap.Observer +} + +func newMockWatcher() *mockWatcher { + return &mockWatcher{ + watchedCallbacks: make(map[string][]configmap.Observer), + } +} + +func (m *mockWatcher) Watch(name string, observers ...configmap.Observer) { + m.mu.Lock() + defer m.mu.Unlock() + m.watchedNames = append(m.watchedNames, name) + m.watchedCallbacks[name] = observers +} + +func (m *mockWatcher) Start(<-chan struct{}) error { + return nil +} + +func (m *mockWatcher) getWatchedNames() []string { + m.mu.Lock() + defer m.mu.Unlock() + names := make([]string, len(m.watchedNames)) + copy(names, m.watchedNames) + return names +} + +func TestWatchConfigsWithDefaults_RegistersAllConfigMaps(t *testing.T) { + store := NewStore(logtesting.TestLogger(t)) + watcher := newMockDefaultingWatcher() + + store.WatchConfigsWithDefaults(watcher, system.Namespace()) + + expectedConfigMaps := []string{ + DefaultsConfigName, + FeaturesConfigName, + autoscalerconfig.ConfigName, + } + + for _, expectedName := range expectedConfigMaps { + cm, ok := watcher.getWatchedDefault(expectedName) + if !ok { + t.Fatalf("Expected ConfigMap %q to be registered with WatchWithDefault, but it was not", expectedName) + } else if cm.Name != expectedName { + t.Errorf("Expected ConfigMap name %q, got %q", expectedName, cm.Name) + } + } +} + +func TestWatchConfigsWithDefaults_PassesNamespace(t *testing.T) { + store := NewStore(logtesting.TestLogger(t)) + watcher := newMockDefaultingWatcher() + namespace := "test-namespace" + + store.WatchConfigsWithDefaults(watcher, namespace) + + for name := range configConstructors { + cm, ok := watcher.getWatchedDefault(name) + if !ok { + t.Fatalf("Expected ConfigMap %q to be registered", name) + continue + } + if cm.Namespace != namespace { + t.Errorf("Expected ConfigMap %q to have namespace %q, got %q", name, namespace, cm.Namespace) + } + } +} + +func TestWatchConfigsWithDefaults_DefaultConfigsUpdateStore(t *testing.T) { + store := NewStore(logtesting.TestLogger(t)) + watcher := newMockDefaultingWatcher() + + store.WatchConfigsWithDefaults(watcher, system.Namespace()) + + // Simulate the defaulting watcher notifying observers with the default ConfigMaps. + for name := range configConstructors { + defaultCM, ok := watcher.getWatchedDefault(name) + if !ok { + t.Fatalf("ConfigMap %q not registered", name) + } + watcher.triggerCallback(&defaultCM) + } + + // Verify the Store was updated via the callbacks + config := store.Load() + + if config.Defaults == nil { + t.Error("Expected Defaults to be set from callback") + } + if config.Features == nil { + t.Error("Expected Features to be set from callback") + } + if config.Autoscaler == nil { + t.Error("Expected Autoscaler to be set from callback") + } +} + +func TestWatchConfigsWithDefaults_RealConfigMapReplacesDefault(t *testing.T) { + store := NewStore(logtesting.TestLogger(t)) + watcher := newMockDefaultingWatcher() + + store.WatchConfigsWithDefaults(watcher, system.Namespace()) + + // Simulate the defaulting watcher first providing the default, + // then observing the real ConfigMap. + defaultCM, ok := watcher.getWatchedDefault(DefaultsConfigName) + if !ok { + t.Fatal("Expected default ConfigMap to be registered") + } + watcher.triggerCallback(&defaultCM) + + config1 := store.Load() + if config1.Defaults == nil { + t.Fatal("Expected Defaults to be set from default ConfigMap") + } + + // Now trigger a real ConfigMap with actual values + realConfigMap := ConfigMapFromTestFile(t, DefaultsConfigName) + watcher.triggerCallback(realConfigMap) + + // Verify the real ConfigMap replaced the default + config2 := store.Load() + expectedDefaults, _ := NewDefaultsConfigFromConfigMap(realConfigMap) + if diff := cmp.Diff(expectedDefaults, config2.Defaults, ignoreStuff...); diff != "" { + t.Errorf("Real ConfigMap did not replace default (-want, +got):\n%v", diff) + } +} + +func TestWatchConfigsWithDefaults_FallbackToRegularWatch(t *testing.T) { + store := NewStore(logtesting.TestLogger(t)) + watcher := newMockWatcher() + + // A watcher that does not implement DefaultingWatcher should use the existing Watch path. + store.WatchConfigsWithDefaults(watcher, system.Namespace()) + + // Verify that regular Watch was called for all ConfigMaps + watchedNames := watcher.getWatchedNames() + expectedConfigMaps := []string{ + DefaultsConfigName, + FeaturesConfigName, + autoscalerconfig.ConfigName, + } + + if len(watchedNames) != len(expectedConfigMaps) { + t.Errorf("Expected %d ConfigMaps to be watched, got %d", len(expectedConfigMaps), len(watchedNames)) + } + + // Verify all expected ConfigMaps were watched + watchedMap := make(map[string]bool) + for _, name := range watchedNames { + watchedMap[name] = true + } + + for _, expectedName := range expectedConfigMaps { + if !watchedMap[expectedName] { + t.Errorf("Expected ConfigMap %q to be watched with regular Watch, but it was not", expectedName) + } + } +}