diff --git a/hack/update-codegen.sh b/hack/update-codegen.sh index 9e0c02025d5b..22a8ff500b1e 100755 --- a/hack/update-codegen.sh +++ b/hack/update-codegen.sh @@ -41,6 +41,7 @@ ${GOPATH}/bin/deepcopy-gen \ -O zz_generated.deepcopy \ --go-header-file ${REPO_ROOT_DIR}/hack/boilerplate/boilerplate.go.txt \ -i github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config \ + -i github.com/knative/serving/pkg/reconciler/v1alpha1/route/config \ -i github.com/knative/serving/pkg/autoscaler \ -i github.com/knative/serving/pkg/logging diff --git a/pkg/reconciler/config/store.go b/pkg/reconciler/config/store.go new file mode 100644 index 000000000000..da86aef59730 --- /dev/null +++ b/pkg/reconciler/config/store.go @@ -0,0 +1,158 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "reflect" + "sync/atomic" + + "github.com/knative/pkg/configmap" + corev1 "k8s.io/api/core/v1" +) + +// The UntypedStore expects a logger that conforms to this interface +// The store will log when updates succeed or fail +type Logger interface { + Infof(string, ...interface{}) + Fatalf(string, ...interface{}) + Errorf(string, ...interface{}) +} + +// Constructors is a map for specifying config names to +// their function constructors +// +// The values of this map must be functions with the definition +// +// func(*k8s.io/api/core/v1.ConfigMap) (... , error) +// +// These functions can return any type along with an error +type Constructors map[string]interface{} + +// An UntypedStore is a responsible for storing and +// constructing configs from Kubernetes ConfigMaps +// +// WatchConfigs should be used with a configmap,Watcher +// in order for this store to remain up to date +type UntypedStore struct { + name string + logger Logger + + storages map[string]*atomic.Value + constructors map[string]reflect.Value +} + +// NewUntypedStore creates an UntypedStore with given name, +// Logger and Constructors +// +// The Logger must not be nil +// +// The values in the Constructors map must be functions with +// the definition +// +// func(*k8s.io/api/core/v1.ConfigMap) (... , error) +// +// These functions can return any type along with an error. +// If the function definition differs then NewUntypedStore +// will panic. +func NewUntypedStore( + name string, + logger Logger, + constructors Constructors) *UntypedStore { + + store := &UntypedStore{ + name: name, + logger: logger, + storages: make(map[string]*atomic.Value), + constructors: make(map[string]reflect.Value), + } + + for configName, constructor := range constructors { + store.registerConfig(configName, constructor) + } + + return store +} + +func (s *UntypedStore) registerConfig(name string, constructor interface{}) { + cType := reflect.TypeOf(constructor) + + if cType.Kind() != reflect.Func { + panic("config constructor must be a function") + } + + if cType.NumIn() != 1 || cType.In(0) != reflect.TypeOf(&corev1.ConfigMap{}) { + panic("config constructor must be of the type func(*k8s.io/api/core/v1/ConfigMap) (..., error)") + } + + errorType := reflect.TypeOf((*error)(nil)).Elem() + + if cType.NumOut() != 2 || !cType.Out(1).Implements(errorType) { + panic("config constructor must be of the type func(*k8s.io/api/core/v1/ConfigMap) (..., error)") + } + + s.storages[name] = &atomic.Value{} + s.constructors[name] = reflect.ValueOf(constructor) +} + +// WatchConfigs uses the provided configmap.Watcher +// to setup watches for the config names provided in the +// Constructors map +func (s *UntypedStore) WatchConfigs(w configmap.Watcher) { + for configMapName := range s.constructors { + w.Watch(configMapName, s.OnConfigChanged) + } +} + +// UntypedLoad will return the constructed value for a given +// ConfigMap name +func (s *UntypedStore) UntypedLoad(name string) interface{} { + storage := s.storages[name] + return storage.Load() +} + +// OnConfigChanged will invoke the mapped constructor against +// a Kubernetes ConfigMap. If successful it will be stored. +// If construction fails during the first appearance the store +// will log a fatal error. If construction fails while updating +// the store will log an error message. +func (s *UntypedStore) OnConfigChanged(c *corev1.ConfigMap) { + name := c.ObjectMeta.Name + + storage := s.storages[name] + constructor := s.constructors[name] + + inputs := []reflect.Value{ + reflect.ValueOf(c), + } + + outputs := constructor.Call(inputs) + result := outputs[0].Interface() + errVal := outputs[1] + + if !errVal.IsNil() { + err := errVal.Interface() + if storage.Load() != nil { + s.logger.Errorf("Error updating %s config %q: %q", s.name, name, err) + } else { + s.logger.Fatalf("Error initializing %s config %q: %q", s.name, name, err) + } + return + } + + s.logger.Infof("%s config %q config was added or updated: %v", s.name, name, result) + storage.Store(result) +} diff --git a/pkg/reconciler/config/store_test.go b/pkg/reconciler/config/store_test.go new file mode 100644 index 000000000000..fefb584c6c97 --- /dev/null +++ b/pkg/reconciler/config/store_test.go @@ -0,0 +1,224 @@ +/* +Copyright 2018 The Knative Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "errors" + "fmt" + "os" + "os/exec" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/knative/pkg/configmap" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + . "github.com/knative/pkg/logging/testing" +) + +func TestStoreBadConstructors(t *testing.T) { + tests := []struct { + name string + constructor interface{} + }{{ + name: "not a function", + constructor: "i'm pretending to be a function", + }, { + name: "no function arguments", + constructor: func() (bool, error) { return true, nil }, + }, { + name: "single argument is not a configmap", + constructor: func(bool) (bool, error) { return true, nil }, + }, { + name: "single return", + constructor: func(*corev1.ConfigMap) error { return nil }, + }, { + name: "wrong second return", + constructor: func(*corev1.ConfigMap) (bool, bool) { return true, true }, + }} + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + defer func() { + if r := recover(); r == nil { + t.Error("expected NewUntypedStore to panic") + } + }() + + NewUntypedStore("store", nil, Constructors{ + "test": test.constructor, + }) + }) + } +} + +func TestStoreWatchConfigs(t *testing.T) { + constructor := func(c *corev1.ConfigMap) (interface{}, error) { + return c.Name, nil + } + + store := NewUntypedStore( + "name", + TestLogger(t), + Constructors{ + "config-name-1": constructor, + "config-name-2": constructor, + }, + ) + + watcher := &mockWatcher{} + store.WatchConfigs(watcher) + + want := []string{ + "config-name-1", + "config-name-2", + } + + got := watcher.watches + + if diff := cmp.Diff(want, got, sortStrings); diff != "" { + t.Errorf("Unexpected configmap watches (-want, +got): %v", diff) + } +} + +func TestStoreConfigChange(t *testing.T) { + constructor := func(c *corev1.ConfigMap) (interface{}, error) { + return c.Name, nil + } + + store := NewUntypedStore( + "name", + TestLogger(t), + Constructors{ + "config-name-1": constructor, + "config-name-2": constructor, + }, + ) + + store.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-name-1", + }, + }) + + result := store.UntypedLoad("config-name-1") + + if diff := cmp.Diff(result, "config-name-1"); diff != "" { + t.Errorf("Expected loaded value diff: %s", diff) + } + + result = store.UntypedLoad("config-name-2") + + if diff := cmp.Diff(result, nil); diff != "" { + t.Errorf("Unexpected loaded value diff: %s", diff) + } + + store.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-name-2", + }, + }) + + result = store.UntypedLoad("config-name-2") + + if diff := cmp.Diff(result, "config-name-2"); diff != "" { + t.Errorf("Expected loaded value diff: %s", diff) + } +} + +func TestStoreFailedFirstConversionCrashes(t *testing.T) { + if os.Getenv("CRASH") == "1" { + constructor := func(c *corev1.ConfigMap) (interface{}, error) { + return nil, errors.New("failure") + } + + store := NewUntypedStore("name", TestLogger(t), + Constructors{"config-name-1": constructor}, + ) + + store.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-name-1", + }, + }) + return + } + + cmd := exec.Command(os.Args[0], fmt.Sprintf("-test.run=%s", t.Name())) + cmd.Env = append(os.Environ(), "CRASH=1") + err := cmd.Run() + if e, ok := err.(*exec.ExitError); ok && !e.Success() { + return + } + t.Fatalf("process should have exited with status 1 - err %v", err) +} + +func TestStoreFailedUpdate(t *testing.T) { + induceConstructorFailure := false + + constructor := func(c *corev1.ConfigMap) (interface{}, error) { + if induceConstructorFailure { + return nil, errors.New("failure") + } + + return time.Now().String(), nil + } + + store := NewUntypedStore("name", TestLogger(t), + Constructors{"config-name-1": constructor}, + ) + + store.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-name-1", + }, + }) + + firstLoad := store.UntypedLoad("config-name-1") + + induceConstructorFailure = true + store.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-name-1", + }, + }) + + secondLoad := store.UntypedLoad("config-name-1") + + if diff := cmp.Diff(firstLoad, secondLoad); diff != "" { + t.Errorf("Expected loaded value to remain the same dff: %s", diff) + } +} + +type mockWatcher struct { + watches []string +} + +func (w *mockWatcher) Watch(config string, o configmap.Observer) { + w.watches = append(w.watches, config) +} + +func (*mockWatcher) Start(<-chan struct{}) error { return nil } + +var _ configmap.Watcher = (*mockWatcher)(nil) + +var sortStrings = cmpopts.SortSlices(func(x, y string) bool { + return x < y +}) diff --git a/pkg/reconciler/v1alpha1/revision/config/store.go b/pkg/reconciler/v1alpha1/revision/config/store.go new file mode 100644 index 000000000000..32d98933a455 --- /dev/null +++ b/pkg/reconciler/v1alpha1/revision/config/store.go @@ -0,0 +1,82 @@ +/* +Copyright 2018 The Knative Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "context" + + pkglogging "github.com/knative/pkg/logging" + "github.com/knative/serving/pkg/autoscaler" + "github.com/knative/serving/pkg/logging" + "github.com/knative/serving/pkg/reconciler/config" +) + +type cfgKey struct{} + +// +k8s:deepcopy-gen=false +type Config struct { + Controller *Controller + Network *Network + Observability *Observability + Logging *pkglogging.Config + Autoscaler *autoscaler.Config +} + +func FromContext(ctx context.Context) *Config { + return ctx.Value(cfgKey{}).(*Config) +} + +func ToContext(ctx context.Context, c *Config) context.Context { + return context.WithValue(ctx, cfgKey{}, c) +} + +// +k8s:deepcopy-gen=false +type Store struct { + *config.UntypedStore +} + +func NewStore(logger config.Logger) *Store { + store := &Store{ + UntypedStore: config.NewUntypedStore( + "revision", + logger, + config.Constructors{ + ControllerConfigName: NewControllerConfigFromConfigMap, + NetworkConfigName: NewNetworkFromConfigMap, + ObservabilityConfigName: NewObservabilityFromConfigMap, + autoscaler.ConfigName: autoscaler.NewConfigFromConfigMap, + logging.ConfigName: logging.NewConfigFromConfigMap, + }, + ), + } + + return store +} + +func (s *Store) ToContext(ctx context.Context) context.Context { + return ToContext(ctx, s.Load()) +} + +func (s *Store) Load() *Config { + return &Config{ + Controller: s.UntypedLoad(ControllerConfigName).(*Controller).DeepCopy(), + Network: s.UntypedLoad(NetworkConfigName).(*Network).DeepCopy(), + Observability: s.UntypedLoad(ObservabilityConfigName).(*Observability).DeepCopy(), + Logging: s.UntypedLoad(logging.ConfigName).(*pkglogging.Config).DeepCopy(), + Autoscaler: s.UntypedLoad(autoscaler.ConfigName).(*autoscaler.Config).DeepCopy(), + } +} diff --git a/pkg/reconciler/v1alpha1/revision/config/store_test.go b/pkg/reconciler/v1alpha1/revision/config/store_test.go new file mode 100644 index 000000000000..09d33a5927aa --- /dev/null +++ b/pkg/reconciler/v1alpha1/revision/config/store_test.go @@ -0,0 +1,118 @@ +/* +Copyright 2018 The Knative Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "context" + "math/rand" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/knative/serving/pkg/autoscaler" + "github.com/knative/serving/pkg/logging" + + . "github.com/knative/serving/pkg/reconciler/v1alpha1/testing" +) + +func TestStoreLoadWithContext(t *testing.T) { + store := NewStore(TestLogger(t)) + + controllerConfig := ConfigMapFromTestFile(t, ControllerConfigName) + networkConfig := ConfigMapFromTestFile(t, NetworkConfigName) + observabilityConfig := ConfigMapFromTestFile(t, ObservabilityConfigName) + loggingConfig := ConfigMapFromTestFile(t, logging.ConfigName) + autoscalerConfig := ConfigMapFromTestFile(t, autoscaler.ConfigName) + + store.OnConfigChanged(controllerConfig) + store.OnConfigChanged(networkConfig) + store.OnConfigChanged(observabilityConfig) + store.OnConfigChanged(loggingConfig) + store.OnConfigChanged(autoscalerConfig) + + config := FromContext(store.ToContext(context.Background())) + + t.Run("controller", func(t *testing.T) { + expected, _ := NewControllerConfigFromConfigMap(controllerConfig) + if diff := cmp.Diff(expected, config.Controller); diff != "" { + t.Errorf("Unexpected controller config (-want, +got): %v", diff) + } + }) + + t.Run("network", func(t *testing.T) { + expected, _ := NewNetworkFromConfigMap(networkConfig) + if diff := cmp.Diff(expected, config.Network); diff != "" { + t.Errorf("Unexpected controller config (-want, +got): %v", diff) + } + }) + + t.Run("observability", func(t *testing.T) { + expected, _ := NewObservabilityFromConfigMap(observabilityConfig) + if diff := cmp.Diff(expected, config.Observability); diff != "" { + t.Errorf("Unexpected observability config (-want, +got): %v", diff) + } + }) + + t.Run("logging", func(t *testing.T) { + expected, _ := logging.NewConfigFromConfigMap(loggingConfig) + if diff := cmp.Diff(expected, config.Logging); diff != "" { + t.Errorf("Unexpected logging config (-want, +got): %v", diff) + } + }) + + t.Run("autoscaler", func(t *testing.T) { + expected, _ := autoscaler.NewConfigFromConfigMap(autoscalerConfig) + if diff := cmp.Diff(expected, config.Autoscaler); diff != "" { + t.Errorf("Unexpected autoscaler config (-want, +got): %v", diff) + } + }) +} + +func TestStoreImmutableConfig(t *testing.T) { + store := NewStore(TestLogger(t)) + + store.OnConfigChanged(ConfigMapFromTestFile(t, ControllerConfigName)) + store.OnConfigChanged(ConfigMapFromTestFile(t, NetworkConfigName)) + store.OnConfigChanged(ConfigMapFromTestFile(t, ObservabilityConfigName)) + store.OnConfigChanged(ConfigMapFromTestFile(t, logging.ConfigName)) + store.OnConfigChanged(ConfigMapFromTestFile(t, autoscaler.ConfigName)) + + config := store.Load() + + config.Controller.QueueSidecarImage = "mutated" + config.Network.IstioOutboundIPRanges = "mutated" + config.Observability.FluentdSidecarImage = "mutated" + config.Logging.LoggingConfig = "mutated" + config.Autoscaler.MaxScaleUpRate = rand.Float64() + + newConfig := store.Load() + + if newConfig.Controller.QueueSidecarImage == "mutated" { + t.Error("Controller config is not immutable") + } + if newConfig.Network.IstioOutboundIPRanges == "mutated" { + t.Error("Network config is not immutable") + } + if newConfig.Observability.FluentdSidecarImage == "mutated" { + t.Error("Observability config is not immutable") + } + if newConfig.Logging.LoggingConfig == "mutated" { + t.Error("Logging config is not immutable") + } + if newConfig.Autoscaler.MaxScaleUpRate == config.Autoscaler.MaxScaleUpRate { + t.Error("Autoscaler config is not immutable") + } +} diff --git a/pkg/reconciler/v1alpha1/revision/config/testdata/config-autoscaler.yaml b/pkg/reconciler/v1alpha1/revision/config/testdata/config-autoscaler.yaml new file mode 120000 index 000000000000..17e4b72c26e4 --- /dev/null +++ b/pkg/reconciler/v1alpha1/revision/config/testdata/config-autoscaler.yaml @@ -0,0 +1 @@ +../../../../../../config/config-autoscaler.yaml \ No newline at end of file diff --git a/pkg/reconciler/v1alpha1/revision/config/testdata/config-logging.yaml b/pkg/reconciler/v1alpha1/revision/config/testdata/config-logging.yaml new file mode 120000 index 000000000000..cd048f0179d2 --- /dev/null +++ b/pkg/reconciler/v1alpha1/revision/config/testdata/config-logging.yaml @@ -0,0 +1 @@ +../../../../../../config/config-logging.yaml \ No newline at end of file diff --git a/pkg/reconciler/v1alpha1/revision/cruds.go b/pkg/reconciler/v1alpha1/revision/cruds.go index a75585f40f23..638b5a00525a 100644 --- a/pkg/reconciler/v1alpha1/revision/cruds.go +++ b/pkg/reconciler/v1alpha1/revision/cruds.go @@ -22,30 +22,34 @@ import ( "github.com/google/go-cmp/cmp" + caching "github.com/knative/caching/pkg/apis/caching/v1alpha1" "github.com/knative/pkg/logging" - commonlogging "github.com/knative/pkg/logging" + kpa "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config" + "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources" "go.uber.org/zap" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" vpav1alpha1 "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1" - - caching "github.com/knative/caching/pkg/apis/caching/v1alpha1" - kpa "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" - "github.com/knative/serving/pkg/apis/serving/v1alpha1" - "github.com/knative/serving/pkg/autoscaler" - "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config" - "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources" ) func (c *Reconciler) createDeployment(ctx context.Context, rev *v1alpha1.Revision) (*appsv1.Deployment, error) { logger := logging.FromContext(ctx) + cfgs := config.FromContext(ctx) - deployment := resources.MakeDeployment(rev, c.getLoggingConfig(), c.getNetworkConfig(), - c.getObservabilityConfig(), c.getAutoscalerConfig(), c.getControllerConfig()) + deployment := resources.MakeDeployment( + rev, + cfgs.Logging, + cfgs.Network, + cfgs.Observability, + cfgs.Autoscaler, + cfgs.Controller, + ) // Resolve tag image references to digests. - if err := c.getResolver().Resolve(deployment); err != nil { + if err := c.resolver.Resolve(deployment, cfgs.Controller.RegistriesSkippingTagResolving); err != nil { logger.Error("Error resolving deployment", zap.Error(err)) rev.Status.MarkContainerMissing(err.Error()) return nil, fmt.Errorf("Error resolving container to digest: %v", err) @@ -119,39 +123,3 @@ func (c *Reconciler) createVPA(ctx context.Context, rev *v1alpha1.Revision) (*vp return c.vpaClient.PocV1alpha1().VerticalPodAutoscalers(vpa.Namespace).Create(vpa) } - -func (c *Reconciler) getNetworkConfig() *config.Network { - c.networkConfigMutex.Lock() - defer c.networkConfigMutex.Unlock() - return c.networkConfig.DeepCopy() -} - -func (c *Reconciler) getResolver() resolver { - c.resolverMutex.Lock() - defer c.resolverMutex.Unlock() - return c.resolver -} - -func (c *Reconciler) getControllerConfig() *config.Controller { - c.controllerConfigMutex.Lock() - defer c.controllerConfigMutex.Unlock() - return c.controllerConfig.DeepCopy() -} - -func (c *Reconciler) getLoggingConfig() *commonlogging.Config { - c.loggingConfigMutex.Lock() - defer c.loggingConfigMutex.Unlock() - return c.loggingConfig.DeepCopy() -} - -func (c *Reconciler) getObservabilityConfig() *config.Observability { - c.observabilityConfigMutex.Lock() - defer c.observabilityConfigMutex.Unlock() - return c.observabilityConfig.DeepCopy() -} - -func (c *Reconciler) getAutoscalerConfig() *autoscaler.Config { - c.autoscalerConfigMutex.Lock() - defer c.autoscalerConfigMutex.Unlock() - return c.autoscalerConfig.DeepCopy() -} diff --git a/pkg/reconciler/v1alpha1/revision/queueing_test.go b/pkg/reconciler/v1alpha1/revision/queueing_test.go index 5e695980d0fd..8d0e427a03a3 100644 --- a/pkg/reconciler/v1alpha1/revision/queueing_test.go +++ b/pkg/reconciler/v1alpha1/revision/queueing_test.go @@ -49,7 +49,7 @@ import ( type nopResolver struct{} -func (r *nopResolver) Resolve(_ *appsv1.Deployment) error { +func (r *nopResolver) Resolve(*appsv1.Deployment, map[string]struct{}) error { return nil } @@ -143,7 +143,7 @@ func newTestController(t *testing.T, servingObjects ...runtime.Object) ( buildInformer buildinformers.SharedInformerFactory, servingInformer informers.SharedInformerFactory, cachingInformer cachinginformers.SharedInformerFactory, - configMapWatcher configmap.Watcher, + configMapWatcher *configmap.ManualWatcher, vpaInformer vpainformers.SharedInformerFactory) { // Create fake clients @@ -153,46 +153,7 @@ func newTestController(t *testing.T, servingObjects ...runtime.Object) ( cachingClient = fakecachingclientset.NewSimpleClientset() vpaClient = fakevpaclientset.NewSimpleClientset() - configMapWatcher = configmap.NewStaticWatcher(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace, - Name: config.NetworkConfigName, - }, - }, &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace, - Name: logging.ConfigName, - }, - Data: map[string]string{ - "zap-logger-config": "{\"level\": \"error\",\n\"outputPaths\": [\"stdout\"],\n\"errorOutputPaths\": [\"stderr\"],\n\"encoding\": \"json\"}", - "loglevel.queueproxy": "info", - }, - }, &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace, - Name: config.ObservabilityConfigName, - }, - Data: map[string]string{ - "logging.enable-var-log-collection": "true", - "logging.fluentd-sidecar-image": testFluentdImage, - "logging.fluentd-sidecar-output-config": testFluentdSidecarOutputConfig, - }, - }, &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace, - Name: autoscaler.ConfigName, - }, - Data: map[string]string{ - "max-scale-up-rate": "1.0", - "container-concurrency-target-percentage": "0.5", - "container-concurrency-target-default": "10.0", - "stable-window": "5m", - "panic-window": "10s", - "scale-to-zero-threshold": "10m", - "concurrency-quantum-of-time": "100ms", - "tick-interval": "2s", - }, - }, getTestControllerConfigMap()) + configMapWatcher = &configmap.ManualWatcher{Namespace: system.Namespace} // Create informer factories with fake clients. The second parameter sets the // resync period to zero, disabling it. @@ -223,6 +184,49 @@ func newTestController(t *testing.T, servingObjects ...runtime.Object) ( ) controller.Reconciler.(*Reconciler).resolver = &nopResolver{} + configs := []*corev1.ConfigMap{ + getTestControllerConfigMap(), + { + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace, + Name: config.NetworkConfigName, + }}, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace, + Name: logging.ConfigName, + }, + Data: map[string]string{ + "zap-logger-config": "{\"level\": \"error\",\n\"outputPaths\": [\"stdout\"],\n\"errorOutputPaths\": [\"stderr\"],\n\"encoding\": \"json\"}", + "loglevel.queueproxy": "info", + }}, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace, + Name: config.ObservabilityConfigName, + }, + Data: map[string]string{ + "logging.enable-var-log-collection": "true", + "logging.fluentd-sidecar-image": testFluentdImage, + "logging.fluentd-sidecar-output-config": testFluentdSidecarOutputConfig, + }}, { + ObjectMeta: metav1.ObjectMeta{ + Namespace: system.Namespace, + Name: autoscaler.ConfigName, + }, + Data: map[string]string{ + "max-scale-up-rate": "1.0", + "container-concurrency-target-percentage": "0.5", + "container-concurrency-target-default": "10.0", + "stable-window": "5m", + "panic-window": "10s", + "scale-to-zero-threshold": "10m", + "concurrency-quantum-of-time": "100ms", + "tick-interval": "2s", + }}, + } + + for _, configMap := range configs { + configMapWatcher.OnChange(configMap) + } return } diff --git a/pkg/reconciler/v1alpha1/revision/reconcile_resources.go b/pkg/reconciler/v1alpha1/revision/reconcile_resources.go index 662a3f4d31fa..15ea75398d3b 100644 --- a/pkg/reconciler/v1alpha1/revision/reconcile_resources.go +++ b/pkg/reconciler/v1alpha1/revision/reconcile_resources.go @@ -24,16 +24,16 @@ import ( "github.com/google/go-cmp/cmp" "github.com/knative/pkg/logging" "github.com/knative/pkg/logging/logkey" + kpav1alpha1 "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config" + "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources" + resourcenames "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources/names" "go.uber.org/zap" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrs "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - kpav1alpha1 "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" - "github.com/knative/serving/pkg/apis/serving/v1alpha1" - "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources" - resourcenames "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources/names" ) const ( @@ -213,16 +213,19 @@ func (c *Reconciler) reconcileService(ctx context.Context, rev *v1alpha1.Revisio func (c *Reconciler) reconcileFluentdConfigMap(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) - if !c.getObservabilityConfig().EnableVarLogCollection { + cfgs := config.FromContext(ctx) + + if !cfgs.Observability.EnableVarLogCollection { return nil } + ns := rev.Namespace name := resourcenames.FluentdConfigMap(rev) configMap, err := c.configMapLister.ConfigMaps(ns).Get(name) if apierrs.IsNotFound(err) { // ConfigMap doesn't exist, going to create it - desiredConfigMap := resources.MakeFluentdConfigMap(rev, c.getObservabilityConfig()) + desiredConfigMap := resources.MakeFluentdConfigMap(rev, cfgs.Observability) configMap, err = c.KubeClientSet.CoreV1().ConfigMaps(ns).Create(desiredConfigMap) if err != nil { logger.Error("Error creating fluentd configmap", zap.Error(err)) @@ -233,7 +236,7 @@ func (c *Reconciler) reconcileFluentdConfigMap(ctx context.Context, rev *v1alpha logger.Errorf("configmaps.Get for %q failed: %s", name, err) return err } else { - desiredConfigMap := resources.MakeFluentdConfigMap(rev, c.getObservabilityConfig()) + desiredConfigMap := resources.MakeFluentdConfigMap(rev, cfgs.Observability) if !equality.Semantic.DeepEqual(configMap.Data, desiredConfigMap.Data) { logger.Infof("Reconciling fluentd configmap diff (-desired, +observed): %v", cmp.Diff(desiredConfigMap.Data, configMap.Data)) @@ -251,7 +254,9 @@ func (c *Reconciler) reconcileFluentdConfigMap(ctx context.Context, rev *v1alpha // TODO(#1876): Move this into the KPA's scope. func (c *Reconciler) reconcileVPA(ctx context.Context, rev *v1alpha1.Revision) error { logger := logging.FromContext(ctx) - if !c.getAutoscalerConfig().EnableVPA { + cfgs := config.FromContext(ctx) + + if !cfgs.Autoscaler.EnableVPA { return nil } diff --git a/pkg/reconciler/v1alpha1/revision/resolve.go b/pkg/reconciler/v1alpha1/revision/resolve.go index 1e8025b6a4aa..61d3379460fd 100644 --- a/pkg/reconciler/v1alpha1/revision/resolve.go +++ b/pkg/reconciler/v1alpha1/revision/resolve.go @@ -29,13 +29,15 @@ import ( ) type digestResolver struct { - client kubernetes.Interface - transport http.RoundTripper - registriesToSkip map[string]struct{} + client kubernetes.Interface + transport http.RoundTripper } // Resolve resolves the image references that use tags to digests. -func (r *digestResolver) Resolve(deploy *appsv1.Deployment) error { +func (r *digestResolver) Resolve( + deploy *appsv1.Deployment, + registriesToSkip map[string]struct{}, +) error { pod := deploy.Spec.Template.Spec opt := k8schain.Options{ Namespace: deploy.Namespace, @@ -58,7 +60,7 @@ func (r *digestResolver) Resolve(deploy *appsv1.Deployment) error { return err } - if _, ok := r.registriesToSkip[tag.Registry.RegistryStr()]; ok { + if _, ok := registriesToSkip[tag.Registry.RegistryStr()]; ok { continue } diff --git a/pkg/reconciler/v1alpha1/revision/resolve_test.go b/pkg/reconciler/v1alpha1/revision/resolve_test.go index 1ccb4d4fba2b..e8e1276d279b 100644 --- a/pkg/reconciler/v1alpha1/revision/resolve_test.go +++ b/pkg/reconciler/v1alpha1/revision/resolve_test.go @@ -35,6 +35,8 @@ import ( fakeclient "k8s.io/client-go/kubernetes/fake" ) +var emptyRegistrySet = map[string]struct{}{} + func mustDigest(t *testing.T, img v1.Image) v1.Hash { h, err := img.Digest() if err != nil { @@ -170,7 +172,7 @@ func TestResolve(t *testing.T) { }, }, } - if err := dr.Resolve(deploy); err != nil { + if err := dr.Resolve(deploy, emptyRegistrySet); err != nil { t.Fatalf("Resolve() = %v", err) } @@ -209,7 +211,7 @@ func TestResolveWithDigest(t *testing.T) { }, } deploy := original.DeepCopy() - if err := dr.Resolve(deploy); err != nil { + if err := dr.Resolve(deploy, emptyRegistrySet); err != nil { t.Fatalf("Resolve() = %v", err) } @@ -243,7 +245,7 @@ func TestResolveWithBadTag(t *testing.T) { }, }, } - if err := dr.Resolve(deploy); err == nil { + if err := dr.Resolve(deploy, emptyRegistrySet); err == nil { t.Fatalf("Resolve() = %v, want error", deploy) } } @@ -292,7 +294,7 @@ func TestResolveWithPingFailure(t *testing.T) { }, }, } - if err := dr.Resolve(deploy); err == nil { + if err := dr.Resolve(deploy, emptyRegistrySet); err == nil { t.Fatalf("Resolve() = %v, want error", deploy) } } @@ -341,7 +343,7 @@ func TestResolveWithManifestFailure(t *testing.T) { }, }, } - if err := dr.Resolve(deploy); err == nil { + if err := dr.Resolve(deploy, emptyRegistrySet); err == nil { t.Fatalf("Resolve() = %v, want error", deploy) } } @@ -366,7 +368,7 @@ func TestResolveNoAccess(t *testing.T) { }, } // If there is a failure accessing the ServiceAccount for this Pod, then we should see an error. - if err := dr.Resolve(deploy); err == nil { + if err := dr.Resolve(deploy, emptyRegistrySet); err == nil { t.Fatalf("Resolve() = %v, want error", deploy) } } @@ -400,9 +402,6 @@ func TestResolveSkippingRegistry(t *testing.T) { dr := &digestResolver{ client: client, transport: http.DefaultTransport, - registriesToSkip: map[string]struct{}{ - "localhost:5000": {}, - }, } deploy := &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -421,7 +420,11 @@ func TestResolveSkippingRegistry(t *testing.T) { }, } - if err := dr.Resolve(deploy); err != nil { + registriesToSkip := map[string]struct{}{ + "localhost:5000": {}, + } + + if err := dr.Resolve(deploy, registriesToSkip); err != nil { t.Fatalf("Resolve() = %v", err) } diff --git a/pkg/reconciler/v1alpha1/revision/revision.go b/pkg/reconciler/v1alpha1/revision/revision.go index ff475cfd26aa..ba49dfa8de43 100644 --- a/pkg/reconciler/v1alpha1/revision/revision.go +++ b/pkg/reconciler/v1alpha1/revision/revision.go @@ -21,15 +21,23 @@ import ( "net/http" "reflect" "strings" - "sync" buildv1alpha1 "github.com/knative/build/pkg/apis/build/v1alpha1" buildinformers "github.com/knative/build/pkg/client/informers/externalversions/build/v1alpha1" buildlisters "github.com/knative/build/pkg/client/listers/build/v1alpha1" cachinginformers "github.com/knative/caching/pkg/client/informers/externalversions/caching/v1alpha1" cachinglisters "github.com/knative/caching/pkg/client/listers/caching/v1alpha1" + "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" commonlogging "github.com/knative/pkg/logging" + "github.com/knative/serving/pkg/apis/serving" + "github.com/knative/serving/pkg/apis/serving/v1alpha1" + kpainformers "github.com/knative/serving/pkg/client/informers/externalversions/autoscaling/v1alpha1" + servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1" + kpalisters "github.com/knative/serving/pkg/client/listers/autoscaling/v1alpha1" + listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" + "github.com/knative/serving/pkg/reconciler" + "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config" "go.uber.org/zap" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -43,17 +51,6 @@ import ( appsv1listers "k8s.io/client-go/listers/apps/v1" corev1listers "k8s.io/client-go/listers/core/v1" "k8s.io/client-go/tools/cache" - - "github.com/knative/serving/pkg/apis/serving" - "github.com/knative/serving/pkg/apis/serving/v1alpha1" - "github.com/knative/serving/pkg/autoscaler" - kpainformers "github.com/knative/serving/pkg/client/informers/externalversions/autoscaling/v1alpha1" - servinginformers "github.com/knative/serving/pkg/client/informers/externalversions/serving/v1alpha1" - kpalisters "github.com/knative/serving/pkg/client/listers/autoscaling/v1alpha1" - listers "github.com/knative/serving/pkg/client/listers/serving/v1alpha1" - "github.com/knative/serving/pkg/logging" - "github.com/knative/serving/pkg/reconciler" - "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/config" ) const ( @@ -75,7 +72,13 @@ const ( ) type resolver interface { - Resolve(*appsv1.Deployment) error + Resolve(*appsv1.Deployment, map[string]struct{}) error +} + +type configStore interface { + ToContext(ctx context.Context) context.Context + WatchConfigs(w configmap.Watcher) + Load() *config.Config } // Reconciler implements controller.Reconciler for Revision resources. @@ -96,34 +99,8 @@ type Reconciler struct { configMapLister corev1listers.ConfigMapLister buildtracker *buildTracker - - resolver resolver - resolverMutex sync.Mutex - - // controllerConfig could change over time and access to it - // must go through controllerConfigMutex - controllerConfig *config.Controller - controllerConfigMutex sync.Mutex - - // networkConfig could change over time and access to it - // must go through networkConfigMutex - networkConfig *config.Network - networkConfigMutex sync.Mutex - - // loggingConfig could change over time and access to it - // must go through loggingConfigMutex - loggingConfig *commonlogging.Config - loggingConfigMutex sync.Mutex - - // observabilityConfig could change over time and access to it - // must go through observabilityConfigMutex - observabilityConfig *config.Observability - observabilityConfigMutex sync.Mutex - - // autoscalerConfig could change over time and access to it - // must go through autoscalerConfigMutex - autoscalerConfig *autoscaler.Config - autoscalerConfigMutex sync.Mutex + resolver resolver + configStore configStore } // Check that our Reconciler implements controller.Reconciler @@ -160,7 +137,12 @@ func NewController( endpointsLister: endpointsInformer.Lister(), configMapLister: configMapInformer.Lister(), buildtracker: &buildTracker{builds: map[key]set{}}, + resolver: &digestResolver{ + client: opt.KubeClientSet, + transport: http.DefaultTransport, + }, } + impl := controller.NewImpl(c, c.Logger, "Revisions") // Set up an event handler for when the resource types of interest change @@ -201,11 +183,11 @@ func NewController( }, }) - opt.ConfigMapWatcher.Watch(config.NetworkConfigName, c.receiveNetworkConfig) - opt.ConfigMapWatcher.Watch(logging.ConfigName, c.receiveLoggingConfig) - opt.ConfigMapWatcher.Watch(config.ObservabilityConfigName, c.receiveObservabilityConfig) - opt.ConfigMapWatcher.Watch(autoscaler.ConfigName, c.receiveAutoscalerConfig) - opt.ConfigMapWatcher.Watch(config.ControllerConfigName, c.receiveControllerConfig) + // TODO(mattmoor): When we support reconciling Deployment differences, + // we should consider triggering a global reconciliation here to the + // logging configuration changes are rolled out to active revisions. + c.configStore = config.NewStore(c.Logger.Named("config-store")) + c.configStore.WatchConfigs(opt.ConfigMapWatcher) return impl } @@ -223,6 +205,8 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { logger := commonlogging.FromContext(ctx) logger.Info("Running reconcile Revision") + ctx = c.configStore.ToContext(ctx) + // Get the Revision resource with this namespace/name original, err := c.revisionLister.Revisions(namespace).Get(name) // The resource may no longer exist, in which case we stop processing. @@ -257,7 +241,7 @@ func (c *Reconciler) reconcile(ctx context.Context, rev *v1alpha1.Revision) erro logger := commonlogging.FromContext(ctx) rev.Status.InitializeConditions() - c.updateRevisionLoggingURL(rev) + c.updateRevisionLoggingURL(ctx, rev) if rev.Spec.BuildName != "" { rev.Status.InitializeBuildCondition() @@ -323,11 +307,21 @@ func (c *Reconciler) reconcile(ctx context.Context, rev *v1alpha1.Revision) erro return nil } -func (c *Reconciler) updateRevisionLoggingURL(rev *v1alpha1.Revision) { - logURLTmpl := c.getObservabilityConfig().LoggingURLTemplate - if logURLTmpl != "" { - rev.Status.LogURL = strings.Replace(logURLTmpl, "${REVISION_UID}", string(rev.UID), -1) +func (c *Reconciler) updateRevisionLoggingURL( + ctx context.Context, + rev *v1alpha1.Revision, +) { + + config := config.FromContext(ctx) + if config.Observability.LoggingURLTemplate == "" { + return } + + uid := string(rev.UID) + + rev.Status.LogURL = strings.Replace( + config.Observability.LoggingURLTemplate, + "${REVISION_UID}", uid, -1) } func (c *Reconciler) EnqueueBuildTrackers(impl *controller.Impl) func(obj interface{}) { @@ -371,100 +365,3 @@ func (c *Reconciler) updateStatus(rev *v1alpha1.Revision) (*v1alpha1.Revision, e } return rev, nil } - -func (c *Reconciler) receiveNetworkConfig(configMap *corev1.ConfigMap) { - newNetworkConfig, err := config.NewNetworkFromConfigMap(configMap) - c.networkConfigMutex.Lock() - defer c.networkConfigMutex.Unlock() - if err != nil { - if c.networkConfig != nil { - c.Logger.Errorf("Error updating Network ConfigMap: %v", err) - } else { - c.Logger.Fatalf("Error initializing Network ConfigMap: %v", err) - } - return - } - c.Logger.Infof("Network config map is added or updated: %v", configMap) - c.networkConfig = newNetworkConfig -} - -func (c *Reconciler) receiveLoggingConfig(configMap *corev1.ConfigMap) { - newLoggingConfig, err := logging.NewConfigFromConfigMap(configMap) - c.loggingConfigMutex.Lock() - defer c.loggingConfigMutex.Unlock() - if err != nil { - if c.loggingConfig != nil { - c.Logger.Error("Error updating Logging ConfigMap.", zap.Error(err)) - } else { - c.Logger.Fatalf("Error initializing Logging ConfigMap: %v", err) - } - return - } - - // TODO(mattmoor): When we support reconciling Deployment differences, - // we should consider triggering a global reconciliation here to the - // logging configuration changes are rolled out to active revisions. - c.Logger.Infof("Logging config map is added or updated: %v", configMap) - c.loggingConfig = newLoggingConfig -} - -func (c *Reconciler) receiveControllerConfig(configMap *corev1.ConfigMap) { - controllerConfig, err := config.NewControllerConfigFromConfigMap(configMap) - - c.controllerConfigMutex.Lock() - defer c.controllerConfigMutex.Unlock() - - c.resolverMutex.Lock() - defer c.resolverMutex.Unlock() - - if err != nil { - if c.controllerConfig != nil { - c.Logger.Errorf("Error updating Controller ConfigMap: %v", err) - } else { - c.Logger.Fatalf("Error initializing Controller ConfigMap: %v", err) - } - return - } - - c.Logger.Infof("Controller config map is added or updated: %v", configMap) - - c.controllerConfig = controllerConfig - c.resolver = &digestResolver{ - client: c.KubeClientSet, - transport: http.DefaultTransport, - registriesToSkip: controllerConfig.RegistriesSkippingTagResolving, - } - -} - -func (c *Reconciler) receiveObservabilityConfig(configMap *corev1.ConfigMap) { - newObservabilityConfig, err := config.NewObservabilityFromConfigMap(configMap) - c.observabilityConfigMutex.Lock() - defer c.observabilityConfigMutex.Unlock() - if err != nil { - if c.observabilityConfig != nil { - c.Logger.Errorf("Error updating Observability ConfigMap: %v", err) - } else { - c.Logger.Fatalf("Error initializing Observability ConfigMap: %v", err) - } - return - } - c.Logger.Infof("Observability config map is added or updated: %v", configMap) - c.observabilityConfig = newObservabilityConfig -} - -func (c *Reconciler) receiveAutoscalerConfig(configMap *corev1.ConfigMap) { - newAutoscalerConfig, err := autoscaler.NewConfigFromConfigMap(configMap) - c.autoscalerConfigMutex.Lock() - defer c.autoscalerConfigMutex.Unlock() - if err != nil { - if c.autoscalerConfig != nil { - c.Logger.Errorf("Error updating Autoscaler ConfigMap: %v", err) - } else { - c.Logger.Fatalf("Error initializing Autoscaler ConfigMap: %v", err) - } - return - } - c.Logger.Infof("Autoscaler config map is added or updated: %v", configMap) - c.autoscalerConfig = newAutoscalerConfig -} diff --git a/pkg/reconciler/v1alpha1/revision/revision_test.go b/pkg/reconciler/v1alpha1/revision/revision_test.go index 4f29ef841b39..5821a975bf94 100644 --- a/pkg/reconciler/v1alpha1/revision/revision_test.go +++ b/pkg/reconciler/v1alpha1/revision/revision_test.go @@ -51,7 +51,6 @@ import ( "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources" resourcenames "github.com/knative/serving/pkg/reconciler/v1alpha1/revision/resources/names" "github.com/knative/serving/pkg/system" - "go.uber.org/zap/zapcore" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" @@ -136,7 +135,7 @@ func newTestControllerWithConfig(t *testing.T, controllerConfig *config.Controll buildInformer buildinformers.SharedInformerFactory, servingInformer informers.SharedInformerFactory, cachingInformer cachinginformers.SharedInformerFactory, - configMapWatcher configmap.Watcher, + configMapWatcher *configmap.ManualWatcher, vpaInformer vpainformers.SharedInformerFactory) { // Create fake clients @@ -146,6 +145,38 @@ func newTestControllerWithConfig(t *testing.T, controllerConfig *config.Controll cachingClient = fakecachingclientset.NewSimpleClientset() vpaClient = fakevpaclientset.NewSimpleClientset() + configMapWatcher = &configmap.ManualWatcher{Namespace: system.Namespace} + + // Create informer factories with fake clients. The second parameter sets the + // resync period to zero, disabling it. + kubeInformer = kubeinformers.NewSharedInformerFactory(kubeClient, 0) + buildInformer = buildinformers.NewSharedInformerFactory(buildClient, 0) + servingInformer = informers.NewSharedInformerFactory(servingClient, 0) + cachingInformer = cachinginformers.NewSharedInformerFactory(cachingClient, 0) + vpaInformer = vpainformers.NewSharedInformerFactory(vpaClient, 0) + + controller = NewController( + rclr.Options{ + KubeClientSet: kubeClient, + ServingClientSet: servingClient, + ConfigMapWatcher: configMapWatcher, + CachingClientSet: cachingClient, + Logger: TestLogger(t), + }, + vpaClient, + servingInformer.Serving().V1alpha1().Revisions(), + servingInformer.Autoscaling().V1alpha1().PodAutoscalers(), + buildInformer.Build().V1alpha1().Builds(), + cachingInformer.Caching().V1alpha1().Images(), + kubeInformer.Apps().V1().Deployments(), + kubeInformer.Core().V1().Services(), + kubeInformer.Core().V1().Endpoints(), + kubeInformer.Core().V1().ConfigMaps(), + vpaInformer.Poc().V1alpha1().VerticalPodAutoscalers(), + ) + + controller.Reconciler.(*Reconciler).resolver = &nopResolver{} + var cms []*corev1.ConfigMap cms = append(cms, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ @@ -189,50 +220,27 @@ func newTestControllerWithConfig(t *testing.T, controllerConfig *config.Controll }, getTestControllerConfigMap(), ) - for _, cm := range configs { - cms = append(cms, cm) - } - - configMapWatcher = configmap.NewStaticWatcher(cms...) - - // Create informer factories with fake clients. The second parameter sets the - // resync period to zero, disabling it. - kubeInformer = kubeinformers.NewSharedInformerFactory(kubeClient, 0) - buildInformer = buildinformers.NewSharedInformerFactory(buildClient, 0) - servingInformer = informers.NewSharedInformerFactory(servingClient, 0) - cachingInformer = cachinginformers.NewSharedInformerFactory(cachingClient, 0) - vpaInformer = vpainformers.NewSharedInformerFactory(vpaClient, 0) - controller = NewController( - rclr.Options{ - KubeClientSet: kubeClient, - ServingClientSet: servingClient, - ConfigMapWatcher: configMapWatcher, - CachingClientSet: cachingClient, - Logger: TestLogger(t), - }, - vpaClient, - servingInformer.Serving().V1alpha1().Revisions(), - servingInformer.Autoscaling().V1alpha1().PodAutoscalers(), - buildInformer.Build().V1alpha1().Builds(), - cachingInformer.Caching().V1alpha1().Images(), - kubeInformer.Apps().V1().Deployments(), - kubeInformer.Core().V1().Services(), - kubeInformer.Core().V1().Endpoints(), - kubeInformer.Core().V1().ConfigMaps(), - vpaInformer.Poc().V1alpha1().VerticalPodAutoscalers(), - ) + cms = append(cms, configs...) - controller.Reconciler.(*Reconciler).resolver = &nopResolver{} + for _, configMap := range cms { + configMapWatcher.OnChange(configMap) + } return } -func createRevision(t *testing.T, - kubeClient *fakekubeclientset.Clientset, kubeInformer kubeinformers.SharedInformerFactory, - servingClient *fakeclientset.Clientset, servingInformer informers.SharedInformerFactory, - cachingClient *fakecachingclientset.Clientset, cachingInformer cachinginformers.SharedInformerFactory, - controller *ctrl.Impl, rev *v1alpha1.Revision) *v1alpha1.Revision { +func createRevision( + t *testing.T, + kubeClient *fakekubeclientset.Clientset, + kubeInformer kubeinformers.SharedInformerFactory, + servingClient *fakeclientset.Clientset, + servingInformer informers.SharedInformerFactory, + cachingClient *fakecachingclientset.Clientset, + cachingInformer cachinginformers.SharedInformerFactory, + controller *ctrl.Impl, + rev *v1alpha1.Revision, +) *v1alpha1.Revision { t.Helper() servingClient.ServingV1alpha1().Revisions(rev.Namespace).Create(rev) // Since Reconcile looks in the lister, we need to add it to the informer @@ -244,11 +252,18 @@ func createRevision(t *testing.T, return rev } -func updateRevision(t *testing.T, - kubeClient *fakekubeclientset.Clientset, kubeInformer kubeinformers.SharedInformerFactory, - servingClient *fakeclientset.Clientset, servingInformer informers.SharedInformerFactory, - cachingClient *fakecachingclientset.Clientset, cachingInformer cachinginformers.SharedInformerFactory, - controller *ctrl.Impl, rev *v1alpha1.Revision) { +func updateRevision( + t *testing.T, + kubeClient *fakekubeclientset.Clientset, + kubeInformer kubeinformers.SharedInformerFactory, + servingClient *fakeclientset.Clientset, + servingInformer informers.SharedInformerFactory, + cachingClient *fakecachingclientset.Clientset, + cachingInformer cachinginformers.SharedInformerFactory, + controller *ctrl.Impl, + rev *v1alpha1.Revision, +) { + t.Helper() servingClient.ServingV1alpha1().Revisions(rev.Namespace).Update(rev) servingInformer.Serving().V1alpha1().Revisions().Informer().GetIndexer().Update(rev) @@ -339,7 +354,7 @@ type fixedResolver struct { digest string } -func (r *fixedResolver) Resolve(deploy *appsv1.Deployment) error { +func (r *fixedResolver) Resolve(deploy *appsv1.Deployment, _ map[string]struct{}) error { pod := deploy.Spec.Template.Spec for i := range pod.Containers { pod.Containers[i].Image = r.digest @@ -351,7 +366,7 @@ type errorResolver struct { error string } -func (r *errorResolver) Resolve(deploy *appsv1.Deployment) error { +func (r *errorResolver) Resolve(*appsv1.Deployment, map[string]struct{}) error { return errors.New(r.error) } @@ -392,6 +407,7 @@ func TestResolutionFailed(t *testing.T) { // TODO(mattmoor): Add VPA table testing func TestCreateRevWithVPA(t *testing.T) { controllerConfig := getTestControllerConfig() + kubeClient, _, servingClient, cachingClient, vpaClient, controller, kubeInformer, _, servingInformer, cachingInformer, _, _ := newTestControllerWithConfig(t, controllerConfig, &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace, @@ -413,7 +429,7 @@ func TestCreateRevWithVPA(t *testing.T) { revClient := servingClient.ServingV1alpha1().Revisions(testNamespace) rev := getTestRevision() - if !controller.Reconciler.(*Reconciler).getAutoscalerConfig().EnableVPA { + if !controller.Reconciler.(*Reconciler).configStore.Load().Autoscaler.EnableVPA { t.Fatal("EnableVPA = false, want true") } @@ -437,7 +453,8 @@ func TestCreateRevWithVPA(t *testing.T) { // TODO(mattmoor): add coverage of a Reconcile fixing a stale logging URL func TestUpdateRevWithWithUpdatedLoggingURL(t *testing.T) { controllerConfig := getTestControllerConfig() - kubeClient, _, servingClient, cachingClient, _, controller, kubeInformer, _, servingInformer, cachingInformer, _, _ := newTestControllerWithConfig(t, controllerConfig, &corev1.ConfigMap{ + kubeClient, _, servingClient, cachingClient, _, controller, kubeInformer, _, servingInformer, cachingInformer, watcher, _ := newTestControllerWithConfig(t, controllerConfig, &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace, Name: config.ObservabilityConfigName, @@ -456,7 +473,7 @@ func TestUpdateRevWithWithUpdatedLoggingURL(t *testing.T) { createRevision(t, kubeClient, kubeInformer, servingClient, servingInformer, cachingClient, cachingInformer, controller, rev) // Update controllers logging URL - controller.Reconciler.(*Reconciler).receiveObservabilityConfig(&corev1.ConfigMap{ + watcher.OnChange(&corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Namespace: system.Namespace, Name: config.ObservabilityConfigName, @@ -621,7 +638,8 @@ func TestMarkRevReadyUponEndpointBecomesReady(t *testing.T) { } func TestNoQueueSidecarImageUpdateFail(t *testing.T) { - kubeClient, _, servingClient, cachingClient, _, controller, kubeInformer, _, servingInformer, cachingInformer, _, _ := newTestController(t) + kubeClient, _, servingClient, cachingClient, _, controller, kubeInformer, _, servingInformer, cachingInformer, watcher, _ := newTestController(t) + rev := getTestRevision() config := getTestConfiguration() rev.OwnerReferences = append( @@ -629,14 +647,13 @@ func TestNoQueueSidecarImageUpdateFail(t *testing.T) { *kmeta.NewControllerRef(config), ) // Update controller config with no side car image - controller.Reconciler.(*Reconciler).receiveControllerConfig( - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: "config-controller", - Namespace: system.Namespace, - }, - Data: map[string]string{}, - }) + watcher.OnChange(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-controller", + Namespace: system.Namespace, + }, + Data: map[string]string{}, + }) createRevision(t, kubeClient, kubeInformer, servingClient, servingInformer, cachingClient, cachingInformer, controller, rev) // Look for the revision deployment. @@ -685,55 +702,15 @@ func TestIstioOutboundIPRangesInjection(t *testing.T) { } } -func TestReceiveLoggingConfig(t *testing.T) { - _, _, _, _, _, controller, _, _, _, _, _, _ := newTestController(t) - cm := corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: system.Namespace, - Name: logging.ConfigName, - }, - Data: map[string]string{ - "zap-logger-config": "{\"level\": \"error\",\n\"outputPaths\": [\"stdout\"],\n\"errorOutputPaths\": [\"stderr\"],\n\"encoding\": \"json\"}", - "loglevel.controller": "info", - }, - } - - r := controller.Reconciler.(*Reconciler) - r.receiveLoggingConfig(&cm) - if r.getLoggingConfig().LoggingConfig != cm.Data["zap-logger-config"] { - t.Errorf("Invalid logging config. want: %v, got: %v", cm.Data["zap-logger-config"], r.getLoggingConfig().LoggingConfig) - } - if r.getLoggingConfig().LoggingLevel["controller"] != zapcore.InfoLevel { - t.Errorf("Invalid logging level. want: %v, got: %v", zapcore.InfoLevel, r.getLoggingConfig().LoggingLevel["controller"]) - } - - cm.Data["loglevel.controller"] = "debug" - controller.Reconciler.(*Reconciler).receiveLoggingConfig(&cm) - if r.getLoggingConfig().LoggingConfig != cm.Data["zap-logger-config"] { - t.Errorf("Invalid logging config. want: %v, got: %v", cm.Data["zap-logger-config"], r.getLoggingConfig().LoggingConfig) - } - if r.getLoggingConfig().LoggingLevel["controller"] != zapcore.DebugLevel { - t.Errorf("Invalid logging level. want: %v, got: %v", zapcore.DebugLevel, r.getLoggingConfig().LoggingLevel["controller"]) - } - - cm.Data["loglevel.controller"] = "invalid" - controller.Reconciler.(*Reconciler).receiveLoggingConfig(&cm) - if r.getLoggingConfig().LoggingConfig != cm.Data["zap-logger-config"] { - t.Errorf("Invalid logging config. want: %v, got: %v", cm.Data["zap-logger-config"], r.getLoggingConfig().LoggingConfig) - } - if r.getLoggingConfig().LoggingLevel["controller"] != zapcore.DebugLevel { - t.Errorf("Invalid logging level. want: %v, got: %v", zapcore.DebugLevel, r.getLoggingConfig().LoggingLevel["controller"]) - } -} - func getPodAnnotationsForConfig(t *testing.T, configMapValue string, configAnnotationOverride string) map[string]string { controllerConfig := getTestControllerConfig() - kubeClient, _, servingClient, cachingClient, _, controller, kubeInformer, _, servingInformer, cachingInformer, _, _ := newTestControllerWithConfig(t, controllerConfig) + kubeClient, _, servingClient, cachingClient, _, controller, kubeInformer, _, servingInformer, cachingInformer, watcher, _ := newTestControllerWithConfig(t, controllerConfig) // Resolve image references to this "digest" digest := "foo@sha256:deadbeef" controller.Reconciler.(*Reconciler).resolver = &fixedResolver{digest} - controller.Reconciler.(*Reconciler).receiveNetworkConfig(&corev1.ConfigMap{ + + watcher.OnChange(&corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.NetworkConfigName, Namespace: system.Namespace, diff --git a/pkg/reconciler/v1alpha1/revision/table_test.go b/pkg/reconciler/v1alpha1/revision/table_test.go index 898e5c655bc7..7b7b022eecd6 100644 --- a/pkg/reconciler/v1alpha1/revision/table_test.go +++ b/pkg/reconciler/v1alpha1/revision/table_test.go @@ -17,6 +17,7 @@ limitations under the License. package revision import ( + "context" "testing" "time" @@ -24,6 +25,7 @@ import ( caching "github.com/knative/caching/pkg/apis/caching/v1alpha1" "github.com/knative/pkg/apis" duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" + "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" "github.com/knative/pkg/logging" kpav1alpha1 "github.com/knative/serving/pkg/apis/autoscaling/v1alpha1" @@ -43,48 +45,28 @@ import ( // This is heavily based on the way the OpenShift Ingress controller tests its reconciliation method. func TestReconcile(t *testing.T) { - networkConfig := &config.Network{IstioOutboundIPRanges: "*"} - loggingConfig := &logging.Config{} - observabilityConfig := &config.Observability{ - LoggingURLTemplate: "http://logger.io/${REVISION_UID}", - } - autoscalerConfig := &autoscaler.Config{} - controllerConfig := getTestControllerConfig() - // Create short-hand aliases that pass through the above config and Active to getRev and friends. rev := func(namespace, name, servingState, image string) *v1alpha1.Revision { - return getRev(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getRev(namespace, name, v1alpha1.RevisionServingStateType(servingState), image) } deploy := func(namespace, name, servingState, image string) *appsv1.Deployment { - return getDeploy(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getDeploy(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, ReconcilerTestConfig()) } image := func(namespace, name, servingState, image string) *caching.Image { - i, err := getImage(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + i, err := getImage(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, ReconcilerTestConfig()) if err != nil { t.Fatalf("Error building image: %v", err) } return i } kpa := func(namespace, name, servingState, image string) *kpav1alpha1.PodAutoscaler { - return getKPA(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getKPA(namespace, name, v1alpha1.RevisionServingStateType(servingState), image) } svc := func(namespace, name, servingState, image string) *corev1.Service { - return getService(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getService(namespace, name, v1alpha1.RevisionServingStateType(servingState), image) } endpoints := func(namespace, name, servingState, image string) *corev1.Endpoints { - return getEndpoints(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getEndpoints(namespace, name, v1alpha1.RevisionServingStateType(servingState), image) } table := TableTest{{ @@ -1369,65 +1351,45 @@ func TestReconcile(t *testing.T) { table.Test(t, MakeFactory(func(listers *Listers, opt reconciler.Options) controller.Reconciler { return &Reconciler{ - Base: reconciler.NewBase(opt, controllerAgentName), - revisionLister: listers.GetRevisionLister(), - kpaLister: listers.GetKPALister(), - buildLister: listers.GetBuildLister(), - imageLister: listers.GetImageLister(), - deploymentLister: listers.GetDeploymentLister(), - serviceLister: listers.GetK8sServiceLister(), - endpointsLister: listers.GetEndpointsLister(), - configMapLister: listers.GetConfigMapLister(), - controllerConfig: controllerConfig, - networkConfig: networkConfig, - loggingConfig: loggingConfig, - observabilityConfig: observabilityConfig, - autoscalerConfig: autoscalerConfig, - resolver: &nopResolver{}, - buildtracker: &buildTracker{builds: map[key]set{}}, + Base: reconciler.NewBase(opt, controllerAgentName), + revisionLister: listers.GetRevisionLister(), + kpaLister: listers.GetKPALister(), + buildLister: listers.GetBuildLister(), + imageLister: listers.GetImageLister(), + deploymentLister: listers.GetDeploymentLister(), + serviceLister: listers.GetK8sServiceLister(), + endpointsLister: listers.GetEndpointsLister(), + configMapLister: listers.GetConfigMapLister(), + resolver: &nopResolver{}, + buildtracker: &buildTracker{builds: map[key]set{}}, + configStore: &testConfigStore{config: ReconcilerTestConfig()}, } })) } func TestReconcileWithVarLogEnabled(t *testing.T) { - networkConfig := &config.Network{IstioOutboundIPRanges: "*"} - loggingConfig := &logging.Config{} - observabilityConfig := &config.Observability{ - LoggingURLTemplate: "http://logger.io/${REVISION_UID}", - EnableVarLogCollection: true, - } - autoscalerConfig := &autoscaler.Config{} - controllerConfig := getTestControllerConfig() + config := ReconcilerTestConfig() + config.Observability.EnableVarLogCollection = true // Create short-hand aliases that pass through the above config and Active to getRev and friends. rev := func(namespace, name, servingState, image string) *v1alpha1.Revision { - return getRev(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getRev(namespace, name, v1alpha1.RevisionServingStateType(servingState), image) } deploy := func(namespace, name, servingState, image string) *appsv1.Deployment { - return getDeploy(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getDeploy(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, config) } image := func(namespace, name, servingState, image string) *caching.Image { - i, err := getImage(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + i, err := getImage(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, config) if err != nil { t.Fatalf("Error building image: %v", err) } return i } kpa := func(namespace, name, servingState, image string) *kpav1alpha1.PodAutoscaler { - return getKPA(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getKPA(namespace, name, v1alpha1.RevisionServingStateType(servingState), image) } svc := func(namespace, name, servingState, image string) *corev1.Service { - return getService(namespace, name, v1alpha1.RevisionServingStateType(servingState), image, - loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + return getService(namespace, name, v1alpha1.RevisionServingStateType(servingState), image) } table := TableTest{{ @@ -1444,7 +1406,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { kpa("foo", "first-reconcile-var-log", "Active", "busybox"), deploy("foo", "first-reconcile-var-log", "Active", "busybox"), svc("foo", "first-reconcile-var-log", "Active", "busybox"), - resources.MakeFluentdConfigMap(rev("foo", "first-reconcile-var-log", "Active", "busybox"), observabilityConfig), + resources.MakeFluentdConfigMap(rev("foo", "first-reconcile-var-log", "Active", "busybox"), config.Observability), image("foo", "first-reconcile-var-log", "Active", "busybox"), }, WantUpdates: []clientgotesting.UpdateActionImpl{{ @@ -1488,7 +1450,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { // The first reconciliation of a Revision creates the following resources. deploy("foo", "create-configmap-failure", "Active", "busybox"), svc("foo", "create-configmap-failure", "Active", "busybox"), - resources.MakeFluentdConfigMap(rev("foo", "create-configmap-failure", "Active", "busybox"), observabilityConfig), + resources.MakeFluentdConfigMap(rev("foo", "create-configmap-failure", "Active", "busybox"), config.Observability), image("foo", "create-configmap-failure", "Active", "busybox"), // We don't create the autoscaler resources if we fail to create the fluentd configmap }, @@ -1548,7 +1510,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { kpa("foo", "steady-state", "Active", "busybox"), deploy("foo", "steady-state", "Active", "busybox"), svc("foo", "steady-state", "Active", "busybox"), - resources.MakeFluentdConfigMap(rev("foo", "steady-state", "Active", "busybox"), observabilityConfig), + resources.MakeFluentdConfigMap(rev("foo", "steady-state", "Active", "busybox"), config.Observability), image("foo", "steady-state", "Active", "busybox"), }, Key: "foo/steady-state", @@ -1586,7 +1548,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { // Use the ObjectMeta, but discard the rest. ObjectMeta: resources.MakeFluentdConfigMap( rev("foo", "update-fluentd-config", "Active", "busybox"), - observabilityConfig, + config.Observability, ).ObjectMeta, Data: map[string]string{ "bad key": "bad value", @@ -1598,7 +1560,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { // We should see a single update to the configmap we expect. Object: resources.MakeFluentdConfigMap( rev("foo", "update-fluentd-config", "Active", "busybox"), - observabilityConfig, + config.Observability, ), }}, Key: "foo/update-fluentd-config", @@ -1637,7 +1599,7 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { svc("foo", "update-configmap-failure", "Active", "busybox"), &corev1.ConfigMap{ // Use the ObjectMeta, but discard the rest. - ObjectMeta: resources.MakeFluentdConfigMap(rev("foo", "update-configmap-failure", "Active", "busybox"), observabilityConfig).ObjectMeta, + ObjectMeta: resources.MakeFluentdConfigMap(rev("foo", "update-configmap-failure", "Active", "busybox"), config.Observability).ObjectMeta, Data: map[string]string{ "bad key": "bad value", }, @@ -1646,29 +1608,25 @@ func TestReconcileWithVarLogEnabled(t *testing.T) { }, WantUpdates: []clientgotesting.UpdateActionImpl{{ // We should see a single update to the configmap we expect. - Object: resources.MakeFluentdConfigMap(rev("foo", "update-configmap-failure", "Active", "busybox"), observabilityConfig), + Object: resources.MakeFluentdConfigMap(rev("foo", "update-configmap-failure", "Active", "busybox"), config.Observability), }}, Key: "foo/update-configmap-failure", }} table.Test(t, MakeFactory(func(listers *Listers, opt reconciler.Options) controller.Reconciler { return &Reconciler{ - Base: reconciler.NewBase(opt, controllerAgentName), - revisionLister: listers.GetRevisionLister(), - kpaLister: listers.GetKPALister(), - buildLister: listers.GetBuildLister(), - imageLister: listers.GetImageLister(), - deploymentLister: listers.GetDeploymentLister(), - serviceLister: listers.GetK8sServiceLister(), - endpointsLister: listers.GetEndpointsLister(), - configMapLister: listers.GetConfigMapLister(), - controllerConfig: controllerConfig, - networkConfig: networkConfig, - loggingConfig: loggingConfig, - observabilityConfig: observabilityConfig, - autoscalerConfig: autoscalerConfig, - resolver: &nopResolver{}, - buildtracker: &buildTracker{builds: map[key]set{}}, + Base: reconciler.NewBase(opt, controllerAgentName), + revisionLister: listers.GetRevisionLister(), + kpaLister: listers.GetKPALister(), + buildLister: listers.GetBuildLister(), + imageLister: listers.GetImageLister(), + deploymentLister: listers.GetDeploymentLister(), + serviceLister: listers.GetK8sServiceLister(), + endpointsLister: listers.GetEndpointsLister(), + configMapLister: listers.GetConfigMapLister(), + resolver: &nopResolver{}, + buildtracker: &buildTracker{builds: map[key]set{}}, + configStore: &testConfigStore{config: config}, } })) } @@ -1733,10 +1691,7 @@ func build(namespace, name string, conds ...buildv1alpha1.BuildCondition) *build } } -// The input signatures of these functions should be kept in sync for readability. -func getRev(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string, - loggingConfig *logging.Config, networkConfig *config.Network, observabilityConfig *config.Observability, - autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) *v1alpha1.Revision { +func getRev(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string) *v1alpha1.Revision { return &v1alpha1.Revision{ ObjectMeta: om(namespace, name), Spec: v1alpha1.RevisionSpec{ @@ -1746,50 +1701,32 @@ func getRev(namespace, name string, servingState v1alpha1.RevisionServingStateTy } } -func getDeploy(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string, - loggingConfig *logging.Config, networkConfig *config.Network, observabilityConfig *config.Observability, - autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) *appsv1.Deployment { +func getDeploy(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string, config *config.Config) *appsv1.Deployment { - rev := getRev(namespace, name, servingState, image, loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) - return resources.MakeDeployment(rev, loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) + rev := getRev(namespace, name, servingState, image) + return resources.MakeDeployment(rev, config.Logging, config.Network, config.Observability, + config.Autoscaler, config.Controller) } -func getImage(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string, - loggingConfig *logging.Config, networkConfig *config.Network, observabilityConfig *config.Observability, - autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) (*caching.Image, error) { - - rev := getRev(namespace, name, servingState, image, loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) - deploy := resources.MakeDeployment(rev, loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) +func getImage(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string, config *config.Config) (*caching.Image, error) { + rev := getRev(namespace, name, servingState, image) + deploy := resources.MakeDeployment(rev, config.Logging, config.Network, config.Observability, + config.Autoscaler, config.Controller) return resources.MakeImageCache(rev, deploy) } -func getKPA(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string, - loggingConfig *logging.Config, networkConfig *config.Network, observabilityConfig *config.Observability, - autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) *kpav1alpha1.PodAutoscaler { - rev := getRev(namespace, name, servingState, image, loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) +func getKPA(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string) *kpav1alpha1.PodAutoscaler { + rev := getRev(namespace, name, servingState, image) return resources.MakeKPA(rev) } -func getService(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string, - loggingConfig *logging.Config, networkConfig *config.Network, observabilityConfig *config.Observability, - autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) *corev1.Service { - - rev := getRev(namespace, name, servingState, image, loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) +func getService(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string) *corev1.Service { + rev := getRev(namespace, name, servingState, image) return resources.MakeK8sService(rev) } -func getEndpoints(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string, - loggingConfig *logging.Config, networkConfig *config.Network, observabilityConfig *config.Observability, - autoscalerConfig *autoscaler.Config, controllerConfig *config.Controller) *corev1.Endpoints { - - service := getService(namespace, name, servingState, image, loggingConfig, networkConfig, observabilityConfig, - autoscalerConfig, controllerConfig) +func getEndpoints(namespace, name string, servingState v1alpha1.RevisionServingStateType, image string) *corev1.Endpoints { + service := getService(namespace, name, servingState, image) return &corev1.Endpoints{ ObjectMeta: metav1.ObjectMeta{ Namespace: service.Namespace, @@ -1797,3 +1734,31 @@ func getEndpoints(namespace, name string, servingState v1alpha1.RevisionServingS }, } } + +type testConfigStore struct { + config *config.Config +} + +func (t *testConfigStore) ToContext(ctx context.Context) context.Context { + return config.ToContext(ctx, t.config) +} + +func (t *testConfigStore) Load() *config.Config { + return t.config +} + +func (t *testConfigStore) WatchConfigs(w configmap.Watcher) {} + +var _ configStore = (*testConfigStore)(nil) + +func ReconcilerTestConfig() *config.Config { + return &config.Config{ + Controller: getTestControllerConfig(), + Network: &config.Network{IstioOutboundIPRanges: "*"}, + Observability: &config.Observability{ + LoggingURLTemplate: "http://logger.io/${REVISION_UID}", + }, + Logging: &logging.Config{}, + Autoscaler: &autoscaler.Config{}, + } +} diff --git a/pkg/reconciler/v1alpha1/route/config/doc.go b/pkg/reconciler/v1alpha1/route/config/doc.go index b35a1e8aaaeb..18fe4f381fe0 100644 --- a/pkg/reconciler/v1alpha1/route/config/doc.go +++ b/pkg/reconciler/v1alpha1/route/config/doc.go @@ -14,6 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ +// +k8s:deepcopy-gen=package // Package config holds the typed objects that define the schemas for // assorted ConfigMap objects on which the Route controller depends. package config diff --git a/pkg/reconciler/v1alpha1/route/config/store.go b/pkg/reconciler/v1alpha1/route/config/store.go new file mode 100644 index 000000000000..1b34db4f9403 --- /dev/null +++ b/pkg/reconciler/v1alpha1/route/config/store.go @@ -0,0 +1,67 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "context" + + "github.com/knative/serving/pkg/reconciler/config" +) + +type cfgKey struct{} + +// +k8s:deepcopy-gen=false +type Config struct { + Domain *Domain +} + +func FromContext(ctx context.Context) *Config { + return ctx.Value(cfgKey{}).(*Config) +} + +func ToContext(ctx context.Context, c *Config) context.Context { + return context.WithValue(ctx, cfgKey{}, c) +} + +// +k8s:deepcopy-gen=false +type Store struct { + *config.UntypedStore +} + +func NewStore(logger config.Logger) *Store { + store := &Store{ + UntypedStore: config.NewUntypedStore( + "route", + logger, + config.Constructors{ + DomainConfigName: NewDomainFromConfigMap, + }, + ), + } + + return store +} + +func (s *Store) ToContext(ctx context.Context) context.Context { + return ToContext(ctx, s.Load()) +} + +func (s *Store) Load() *Config { + return &Config{ + Domain: s.UntypedLoad(DomainConfigName).(*Domain).DeepCopy(), + } +} diff --git a/pkg/reconciler/v1alpha1/route/config/store_test.go b/pkg/reconciler/v1alpha1/route/config/store_test.go new file mode 100644 index 000000000000..1e9e792567de --- /dev/null +++ b/pkg/reconciler/v1alpha1/route/config/store_test.go @@ -0,0 +1,61 @@ +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package config + +import ( + "context" + "testing" + + "github.com/google/go-cmp/cmp" + + . "github.com/knative/pkg/logging/testing" + . "github.com/knative/serving/pkg/reconciler/testing" +) + +func TestStoreLoadWithContext(t *testing.T) { + store := NewStore(TestLogger(t)) + + domainConfig := ConfigMapFromTestFile(t, DomainConfigName) + + store.OnConfigChanged(domainConfig) + + config := FromContext(store.ToContext(context.Background())) + + t.Run("domain", func(t *testing.T) { + expected, _ := NewDomainFromConfigMap(domainConfig) + if diff := cmp.Diff(expected, config.Domain); diff != "" { + t.Errorf("Unexpected controller config (-want, +got): %v", diff) + } + }) +} + +func TestStoreImmutableConfig(t *testing.T) { + store := NewStore(TestLogger(t)) + store.OnConfigChanged(ConfigMapFromTestFile(t, DomainConfigName)) + + config := store.Load() + + config.Domain.Domains = map[string]*LabelSelector{ + "mutated": nil, + } + + newConfig := store.Load() + + if _, ok := newConfig.Domain.Domains["mutated"]; ok { + t.Error("Domain config is not immutable") + } +} diff --git a/pkg/reconciler/v1alpha1/route/config/zz_generated.deepcopy.go b/pkg/reconciler/v1alpha1/route/config/zz_generated.deepcopy.go new file mode 100644 index 000000000000..647fd701d707 --- /dev/null +++ b/pkg/reconciler/v1alpha1/route/config/zz_generated.deepcopy.go @@ -0,0 +1,72 @@ +// +build !ignore_autogenerated + +/* +Copyright 2018 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// This file was autogenerated by deepcopy-gen. Do not edit it manually! + +package config + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *Domain) DeepCopyInto(out *Domain) { + *out = *in + if in.Domains != nil { + in, out := &in.Domains, &out.Domains + *out = make(map[string]*LabelSelector, len(*in)) + for key, val := range *in { + if val == nil { + (*out)[key] = nil + } else { + (*out)[key] = new(LabelSelector) + val.DeepCopyInto((*out)[key]) + } + } + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new Domain. +func (in *Domain) DeepCopy() *Domain { + if in == nil { + return nil + } + out := new(Domain) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *LabelSelector) DeepCopyInto(out *LabelSelector) { + *out = *in + if in.Selector != nil { + in, out := &in.Selector, &out.Selector + *out = make(map[string]string, len(*in)) + for key, val := range *in { + (*out)[key] = val + } + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new LabelSelector. +func (in *LabelSelector) DeepCopy() *LabelSelector { + if in == nil { + return nil + } + out := new(LabelSelector) + in.DeepCopyInto(out) + return out +} diff --git a/pkg/reconciler/v1alpha1/route/route.go b/pkg/reconciler/v1alpha1/route/route.go index b8182f5817de..f28683f609b2 100644 --- a/pkg/reconciler/v1alpha1/route/route.go +++ b/pkg/reconciler/v1alpha1/route/route.go @@ -19,10 +19,10 @@ package route import ( "context" "fmt" - "sync" istioinformers "github.com/knative/pkg/client/informers/externalversions/istio/v1alpha3" istiolisters "github.com/knative/pkg/client/listers/istio/v1alpha3" + "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" "github.com/knative/pkg/logging" "go.uber.org/zap" @@ -47,6 +47,11 @@ const ( controllerAgentName = "route-controller" ) +type configStore interface { + ToContext(ctx context.Context) context.Context + WatchConfigs(w configmap.Watcher) +} + // Reconciler implements controller.Reconciler for Route resources. type Reconciler struct { *reconciler.Base @@ -57,11 +62,7 @@ type Reconciler struct { revisionLister listers.RevisionLister serviceLister corev1listers.ServiceLister virtualServiceLister istiolisters.VirtualServiceLister - - // Domain configuration could change over time and access to domainConfig - // must go through domainConfigMutex - domainConfig *config.Domain - domainConfigMutex sync.Mutex + configStore configStore } // Check that our Reconciler implements controller.Reconciler @@ -122,7 +123,8 @@ func NewController( }) c.Logger.Info("Setting up ConfigMap receivers") - opt.ConfigMapWatcher.Watch(config.DomainConfigName, c.receiveDomainConfig) + c.configStore = config.NewStore(c.Logger.Named("config-store")) + c.configStore.WatchConfigs(opt.ConfigMapWatcher) return impl } @@ -142,6 +144,8 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { } logger := logging.FromContext(ctx) + ctx = c.configStore.ToContext(ctx) + // Get the Route resource with this namespace/name original, err := c.routeLister.Routes(namespace).Get(name) if apierrs.IsNotFound(err) { @@ -200,7 +204,7 @@ func (c *Reconciler) reconcile(ctx context.Context, route *v1alpha1.Route) error // In all cases we will add annotations to the referred targets. This is so that when they become // routable we can know (through a listener) and attempt traffic configuration again. func (c *Reconciler) configureTraffic(ctx context.Context, r *v1alpha1.Route) (*v1alpha1.Route, error) { - r.Status.Domain = c.routeDomain(r) + r.Status.Domain = c.routeDomain(ctx, r) logger := logging.FromContext(ctx) t, err := traffic.BuildTrafficConfiguration(c.configurationLister, c.revisionLister, r) badTarget, isTargetError := err.(traffic.TargetError) @@ -257,25 +261,8 @@ func (c *Reconciler) EnqueueReferringRoute(impl *controller.Impl) func(obj inter // Misc helpers. ///////////////////////////////////////// -func (c *Reconciler) getDomainConfig() *config.Domain { - c.domainConfigMutex.Lock() - defer c.domainConfigMutex.Unlock() - return c.domainConfig -} - -func (c *Reconciler) routeDomain(route *v1alpha1.Route) string { - domain := c.getDomainConfig().LookupDomainForLabels(route.ObjectMeta.Labels) +func (c *Reconciler) routeDomain(ctx context.Context, route *v1alpha1.Route) string { + domainConfig := config.FromContext(ctx).Domain + domain := domainConfig.LookupDomainForLabels(route.ObjectMeta.Labels) return fmt.Sprintf("%s.%s.%s", route.Name, route.Namespace, domain) } - -func (c *Reconciler) receiveDomainConfig(configMap *corev1.ConfigMap) { - newDomainConfig, err := config.NewDomainFromConfigMap(configMap) - if err != nil { - c.Logger.Error("Failed to parse the new config map. Previous config map will be used.", - zap.Error(err)) - return - } - c.domainConfigMutex.Lock() - defer c.domainConfigMutex.Unlock() - c.domainConfig = newDomainConfig -} diff --git a/pkg/reconciler/v1alpha1/route/route_test.go b/pkg/reconciler/v1alpha1/route/route_test.go index f56e7b1d5c42..6b8ae3e7a020 100644 --- a/pkg/reconciler/v1alpha1/route/route_test.go +++ b/pkg/reconciler/v1alpha1/route/route_test.go @@ -160,7 +160,7 @@ func newTestReconciler(t *testing.T, configs ...*corev1.ConfigMap) ( kubeInformer kubeinformers.SharedInformerFactory, sharedInformer sharedinformers.SharedInformerFactory, servingInformer informers.SharedInformerFactory, - configMapWatcher configmap.Watcher) { + configMapWatcher *configmap.ManualWatcher) { kubeClient, sharedClient, servingClient, _, reconciler, kubeInformer, sharedInformer, servingInformer, configMapWatcher = newTestSetup(t) return } @@ -173,7 +173,7 @@ func newTestController(t *testing.T, configs ...*corev1.ConfigMap) ( kubeInformer kubeinformers.SharedInformerFactory, sharedInformer sharedinformers.SharedInformerFactory, servingInformer informers.SharedInformerFactory, - configMapWatcher configmap.Watcher) { + configMapWatcher *configmap.ManualWatcher) { kubeClient, sharedClient, servingClient, controller, _, kubeInformer, sharedInformer, servingInformer, configMapWatcher = newTestSetup(t) return } @@ -187,7 +187,7 @@ func newTestSetup(t *testing.T, configs ...*corev1.ConfigMap) ( kubeInformer kubeinformers.SharedInformerFactory, sharedInformer sharedinformers.SharedInformerFactory, servingInformer informers.SharedInformerFactory, - configMapWatcher configmap.Watcher) { + configMapWatcher *configmap.ManualWatcher) { // Create fake clients kubeClient = fakekubeclientset.NewSimpleClientset() @@ -206,7 +206,7 @@ func newTestSetup(t *testing.T, configs ...*corev1.ConfigMap) ( cms = append(cms, cm) } - configMapWatcher = configmap.NewStaticWatcher(cms...) + configMapWatcher = &configmap.ManualWatcher{Namespace: system.Namespace} sharedClient = fakesharedclientset.NewSimpleClientset() servingClient = fakeclientset.NewSimpleClientset() @@ -233,6 +233,10 @@ func newTestSetup(t *testing.T, configs ...*corev1.ConfigMap) ( reconciler = controller.Reconciler.(*Reconciler) + for _, cfg := range cms { + configMapWatcher.OnChange(cfg) + } + return } @@ -908,7 +912,7 @@ func TestEnqueueReferringRouteNotEnqueueIfNotGivenAConfig(t *testing.T) { } func TestUpdateDomainConfigMap(t *testing.T) { - kubeClient, sharedClient, servingClient, controller, kubeInformer, sharedInformer, servingInformer, _ := newTestReconciler(t) + kubeClient, sharedClient, servingClient, controller, kubeInformer, sharedInformer, servingInformer, watcher := newTestReconciler(t) route := getTestRouteWithTrafficTargets([]v1alpha1.TrafficTarget{}) routeClient := servingClient.ServingV1alpha1().Routes(route.Namespace) @@ -940,7 +944,7 @@ func TestUpdateDomainConfigMap(t *testing.T) { "mytestdomain.com": "selector:\n app: prod", }, } - controller.receiveDomainConfig(&domainConfig) + watcher.OnChange(&domainConfig) }, }, { expectedDomainSuffix: "newdefault.net", @@ -955,7 +959,7 @@ func TestUpdateDomainConfigMap(t *testing.T) { "mytestdomain.com": "selector:\n app: prod", }, } - controller.receiveDomainConfig(&domainConfig) + watcher.OnChange(&domainConfig) route.Labels = make(map[string]string) }, }, { @@ -971,7 +975,7 @@ func TestUpdateDomainConfigMap(t *testing.T) { "mytestdomain.com": "selector:\n app: prod", }, } - controller.receiveDomainConfig(&domainConfig) + watcher.OnChange(&domainConfig) route.Labels = make(map[string]string) }, }} diff --git a/pkg/reconciler/v1alpha1/route/table_test.go b/pkg/reconciler/v1alpha1/route/table_test.go index 23d8d2da7891..d9a663489001 100644 --- a/pkg/reconciler/v1alpha1/route/table_test.go +++ b/pkg/reconciler/v1alpha1/route/table_test.go @@ -17,11 +17,13 @@ limitations under the License. package route import ( + "context" "fmt" "testing" duckv1alpha1 "github.com/knative/pkg/apis/duck/v1alpha1" istiov1alpha3 "github.com/knative/pkg/apis/istio/v1alpha3" + "github.com/knative/pkg/configmap" "github.com/knative/pkg/controller" "github.com/knative/serving/pkg/apis/serving/v1alpha1" "github.com/knative/serving/pkg/reconciler" @@ -1440,13 +1442,8 @@ func TestReconcile(t *testing.T) { revisionLister: listers.GetRevisionLister(), serviceLister: listers.GetK8sServiceLister(), virtualServiceLister: listers.GetVirtualServiceLister(), - domainConfig: &config.Domain{ - Domains: map[string]*config.LabelSelector{ - "example.com": {}, - "another-example.com": { - Selector: map[string]string{"app": "prod"}, - }, - }, + configStore: &testConfigStore{ + config: ReconcilerTestConfig(), }, } })) @@ -1663,3 +1660,28 @@ func or(kind, name string) []metav1.OwnerReference { BlockOwnerDeletion: &boolTrue, }} } + +type testConfigStore struct { + config *config.Config +} + +func (t *testConfigStore) ToContext(ctx context.Context) context.Context { + return config.ToContext(ctx, t.config) +} + +func (t *testConfigStore) WatchConfigs(w configmap.Watcher) {} + +var _ configStore = (*testConfigStore)(nil) + +func ReconcilerTestConfig() *config.Config { + return &config.Config{ + Domain: &config.Domain{ + Domains: map[string]*config.LabelSelector{ + "example.com": {}, + "another-example.com": { + Selector: map[string]string{"app": "prod"}, + }, + }, + }, + } +} diff --git a/pkg/reconciler/v1alpha1/testing/aliases.go b/pkg/reconciler/v1alpha1/testing/aliases.go index bef76bf9e03f..39738067c992 100644 --- a/pkg/reconciler/v1alpha1/testing/aliases.go +++ b/pkg/reconciler/v1alpha1/testing/aliases.go @@ -37,6 +37,7 @@ var ( ExpectNormalEventDelivery = testing.ExpectNormalEventDelivery ValidateCreates = testing.ValidateCreates ValidateUpdates = testing.ValidateUpdates + ConfigMapFromTestFile = testing.ConfigMapFromTestFile TestLogger = logtesting.TestLogger )