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 c88fff6527db..a68c4afea442 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,32 @@ func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value i return store } +// 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 yet. + for name := range configConstructors { + dw.WatchWithDefault(corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + }, + }, 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()) 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) + } + } +}