From 6fc300eaa9085bc09a6e169566152d0981c6c802 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Tue, 11 Sep 2018 12:14:04 -0400 Subject: [PATCH 1/7] Create a store which manages the revision reconciler's configs --- .../v1alpha1/revision/config/store.go | 120 ++++++++++++ .../v1alpha1/revision/config/store_test.go | 185 ++++++++++++++++++ .../config/testdata/config-autoscaler.yaml | 1 + .../config/testdata/config-logging.yaml | 1 + pkg/reconciler/v1alpha1/testing/aliases.go | 1 + 5 files changed, 308 insertions(+) create mode 100644 pkg/reconciler/v1alpha1/revision/config/store.go create mode 100644 pkg/reconciler/v1alpha1/revision/config/store_test.go create mode 120000 pkg/reconciler/v1alpha1/revision/config/testdata/config-autoscaler.yaml create mode 120000 pkg/reconciler/v1alpha1/revision/config/testdata/config-logging.yaml diff --git a/pkg/reconciler/v1alpha1/revision/config/store.go b/pkg/reconciler/v1alpha1/revision/config/store.go new file mode 100644 index 000000000000..14a96075794e --- /dev/null +++ b/pkg/reconciler/v1alpha1/revision/config/store.go @@ -0,0 +1,120 @@ +/* +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" + "sync/atomic" + + "github.com/knative/pkg/configmap" + pkglogging "github.com/knative/pkg/logging" + "github.com/knative/serving/pkg/autoscaler" + "github.com/knative/serving/pkg/logging" + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" +) + +type configsKey struct{} + +// +k8s:deepcopy-gen=false +type Config struct { + Controller *Controller + Network *Network + Observability *Observability + Logging *pkglogging.Config + Autoscaler *autoscaler.Config +} + +// +k8s:deepcopy-gen=false +type Store struct { + Logger *zap.SugaredLogger + + controller atomic.Value + network atomic.Value + observability atomic.Value + logging atomic.Value + autoscaler atomic.Value +} + +func FromContext(ctx context.Context) *Config { + return ctx.Value(configsKey{}).(*Config) +} + +func WithConfig(ctx context.Context, c *Config) context.Context { + return context.WithValue(ctx, configsKey{}, c) +} + +func (s *Store) ToContext(ctx context.Context) context.Context { + return WithConfig(ctx, s.Load()) +} + +func (s *Store) WatchConfigs(w configmap.Watcher) { + w.Watch(NetworkConfigName, s.setNetwork) + w.Watch(ObservabilityConfigName, s.setObservability) + w.Watch(ControllerConfigName, s.setController) + + w.Watch(autoscaler.ConfigName, s.setAutoscaler) + w.Watch(logging.ConfigName, s.setLogging) +} + +func (s *Store) Load() *Config { + return &Config{ + Controller: s.controller.Load().(*Controller).DeepCopy(), + Network: s.network.Load().(*Network).DeepCopy(), + Observability: s.observability.Load().(*Observability).DeepCopy(), + Logging: s.logging.Load().(*pkglogging.Config).DeepCopy(), + Autoscaler: s.autoscaler.Load().(*autoscaler.Config).DeepCopy(), + } +} + +func (s *Store) setController(c *corev1.ConfigMap) { + val, err := NewControllerConfigFromConfigMap(c) + s.save("controller", &s.controller, val, err) +} + +func (s *Store) setNetwork(c *corev1.ConfigMap) { + val, err := NewNetworkFromConfigMap(c) + s.save("network", &s.network, val, err) +} + +func (s *Store) setObservability(c *corev1.ConfigMap) { + val, err := NewObservabilityFromConfigMap(c) + s.save("observability", &s.observability, val, err) +} + +func (s *Store) setLogging(c *corev1.ConfigMap) { + val, err := logging.NewConfigFromConfigMap(c) + s.save("logging", &s.logging, val, err) +} + +func (s *Store) setAutoscaler(c *corev1.ConfigMap) { + val, err := autoscaler.NewConfigFromConfigMap(c) + s.save("autoscaler", &s.autoscaler, val, err) +} + +func (s *Store) save(desc string, v *atomic.Value, value interface{}, err error) { + if err != nil { + if v.Load() != nil { + s.Logger.Errorf("Error updating revision %s config: %v", desc, err) + } else { + s.Logger.Fatalf("Error initializing revision %s config: %v", desc, err) + } + return + } + s.Logger.Infof("Revision %s config was added or updated: %v", desc, value) + v.Store(value) +} 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..bf753fd10941 --- /dev/null +++ b/pkg/reconciler/v1alpha1/revision/config/store_test.go @@ -0,0 +1,185 @@ +/* +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/google/go-cmp/cmp/cmpopts" + "github.com/knative/pkg/configmap" + "github.com/knative/serving/pkg/autoscaler" + "github.com/knative/serving/pkg/logging" + "go.uber.org/zap/zapcore" + + . "github.com/knative/serving/pkg/reconciler/v1alpha1/testing" +) + +func TestStoreWatchConfigs(t *testing.T) { + watcher := &mockWatcher{} + + store := Store{Logger: TestLogger(t)} + store.WatchConfigs(watcher) + + want := []string{ + ControllerConfigName, + NetworkConfigName, + ObservabilityConfigName, + logging.ConfigName, + autoscaler.ConfigName, + } + + got := watcher.watches + + if diff := cmp.Diff(want, got, sortStrings); diff != "" { + t.Errorf("Unexpected configmap watches (-want, +got): %v", diff) + } +} + +func TestStoreLoadWithContext(t *testing.T) { + store := Store{Logger: 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.setController(controllerConfig) + store.setNetwork(networkConfig) + store.setObservability(observabilityConfig) + store.setLogging(loggingConfig) + store.setAutoscaler(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 := Store{Logger: TestLogger(t)} + + store.setController(ConfigMapFromTestFile(t, ControllerConfigName)) + store.setNetwork(ConfigMapFromTestFile(t, NetworkConfigName)) + store.setObservability(ConfigMapFromTestFile(t, ObservabilityConfigName)) + store.setLogging(ConfigMapFromTestFile(t, logging.ConfigName)) + store.setAutoscaler(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 immutabled") + } + if newConfig.Autoscaler.MaxScaleUpRate == config.Autoscaler.MaxScaleUpRate { + t.Error("Autoscaler config is not immutabled") + } +} + +func TestStoreFailedUpdate(t *testing.T) { + store := Store{Logger: TestLogger(t)} + + controllerConfig := ConfigMapFromTestFile(t, ControllerConfigName) + networkConfig := ConfigMapFromTestFile(t, NetworkConfigName) + observabilityConfig := ConfigMapFromTestFile(t, ObservabilityConfigName) + loggingConfig := ConfigMapFromTestFile(t, logging.ConfigName) + autoscalerConfig := ConfigMapFromTestFile(t, autoscaler.ConfigName) + + loggingConfig.Data["loglevel.controller"] = "debug" + + store.setController(controllerConfig) + store.setNetwork(networkConfig) + store.setObservability(observabilityConfig) + store.setLogging(loggingConfig) + store.setAutoscaler(autoscalerConfig) + + // Set a bad level which causes the update to fail + loggingConfig.Data["loglevel.controller"] = "unknown" + store.setLogging(loggingConfig) + + config := store.Load() + if got, want := config.Logging.LoggingLevel["controller"], zapcore.DebugLevel; got != want { + t.Errorf("Expected the update to fail - logging level want: %v, got: %v", want, got) + } +} + +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/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/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 ) From 97b7efd59cc62e515450a507df36b0d64f5d8ec2 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Tue, 11 Sep 2018 16:36:37 -0400 Subject: [PATCH 2/7] Have the revision reconciliation use a stable config --- pkg/reconciler/v1alpha1/revision/cruds.go | 62 ++---- .../v1alpha1/revision/queueing_test.go | 88 ++++---- .../v1alpha1/revision/reconcile_resources.go | 23 +- pkg/reconciler/v1alpha1/revision/resolve.go | 12 +- .../v1alpha1/revision/resolve_test.go | 23 +- pkg/reconciler/v1alpha1/revision/revision.go | 196 ++++------------ .../v1alpha1/revision/revision_test.go | 181 +++++++-------- .../v1alpha1/revision/table_test.go | 209 +++++++----------- 8 files changed, 307 insertions(+), 487 deletions(-) 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..b3a26cca9103 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,12 @@ 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) } // Reconciler implements controller.Reconciler for Revision resources. @@ -96,34 +98,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 @@ -148,6 +124,8 @@ func NewController( vpaInformer vpav1alpha1informers.VerticalPodAutoscalerInformer, ) *controller.Impl { + configStore := &config.Store{} + c := &Reconciler{ Base: reconciler.NewBase(opt, controllerAgentName), vpaClient: vpaClient, @@ -160,7 +138,15 @@ func NewController( endpointsLister: endpointsInformer.Lister(), configMapLister: configMapInformer.Lister(), buildtracker: &buildTracker{builds: map[key]set{}}, + configStore: configStore, + resolver: &digestResolver{ + client: opt.KubeClientSet, + transport: http.DefaultTransport, + }, } + + configStore.Logger = c.Logger.Named("config-store") + impl := controller.NewImpl(c, c.Logger, "Revisions") // Set up an event handler for when the resource types of interest change @@ -201,11 +187,10 @@ 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.WatchConfigs(opt.ConfigMapWatcher) return impl } @@ -223,6 +208,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 +244,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 +310,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 +368,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..963991151556 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.(*config.Store).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..c4ac908600aa 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,27 @@ 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.WithConfig(ctx, 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{}, + } +} From 47c9a2e218b9decbb7f50a5ce551a67783ec0813 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Wed, 12 Sep 2018 15:27:34 -0400 Subject: [PATCH 3/7] Access route configs through a context --- hack/update-codegen.sh | 1 + pkg/reconciler/v1alpha1/route/config/doc.go | 1 + pkg/reconciler/v1alpha1/route/config/store.go | 79 ++++++++++ .../v1alpha1/route/config/store_test.go | 141 ++++++++++++++++++ .../route/config/zz_generated.deepcopy.go | 72 +++++++++ pkg/reconciler/v1alpha1/route/route.go | 46 +++--- pkg/reconciler/v1alpha1/route/route_test.go | 20 ++- pkg/reconciler/v1alpha1/route/table_test.go | 36 ++++- 8 files changed, 353 insertions(+), 43 deletions(-) create mode 100644 pkg/reconciler/v1alpha1/route/config/store.go create mode 100644 pkg/reconciler/v1alpha1/route/config/store_test.go create mode 100644 pkg/reconciler/v1alpha1/route/config/zz_generated.deepcopy.go 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/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..4990c844d101 --- /dev/null +++ b/pkg/reconciler/v1alpha1/route/config/store.go @@ -0,0 +1,79 @@ +/* +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" + "sync/atomic" + + "github.com/knative/pkg/configmap" + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" +) + +type configsKey struct{} + +// +k8s:deepcopy-gen=false +type Config struct { + Domain *Domain +} + +// +k8s:deepcopy-gen=false +type Store struct { + Logger *zap.SugaredLogger + domain atomic.Value +} + +func FromContext(ctx context.Context) *Config { + return ctx.Value(configsKey{}).(*Config) +} + +func WithConfig(ctx context.Context, c *Config) context.Context { + return context.WithValue(ctx, configsKey{}, c) +} + +func (s *Store) ToContext(ctx context.Context) context.Context { + return WithConfig(ctx, s.Load()) +} + +func (s *Store) WatchConfigs(w configmap.Watcher) { + w.Watch(DomainConfigName, s.setDomain) +} + +func (s *Store) Load() *Config { + return &Config{ + Domain: s.domain.Load().(*Domain).DeepCopy(), + } +} + +func (s *Store) setDomain(c *corev1.ConfigMap) { + val, err := NewDomainFromConfigMap(c) + s.save("domain", &s.domain, val, err) +} + +func (s *Store) save(desc string, v *atomic.Value, value interface{}, err error) { + if err != nil { + if v.Load() != nil { + s.Logger.Errorf("Error updating route %s config: %v", desc, err) + } else { + s.Logger.Fatalf("Error initializing route %s config: %v", desc, err) + } + return + } + s.Logger.Infof("Route %s config was added or updated: %v", desc, value) + v.Store(value) +} 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..d7632fd6aae3 --- /dev/null +++ b/pkg/reconciler/v1alpha1/route/config/store_test.go @@ -0,0 +1,141 @@ +/* +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" + "fmt" + "io/ioutil" + "testing" + + "github.com/ghodss/yaml" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + "github.com/knative/pkg/configmap" + "github.com/knative/serving/pkg/system" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + . "github.com/knative/pkg/logging/testing" +) + +func TestStoreWatchConfigs(t *testing.T) { + watcher := &mockWatcher{} + + store := Store{Logger: TestLogger(t)} + store.WatchConfigs(watcher) + + want := []string{ + DomainConfigName, + } + + got := watcher.watches + + if diff := cmp.Diff(want, got, sortStrings); diff != "" { + t.Errorf("Unexpected configmap watches (-want, +got): %v", diff) + } +} + +func TestStoreLoadWithContext(t *testing.T) { + store := Store{Logger: TestLogger(t)} + + domainConfig := configMapFromFile(t, DomainConfigName) + + store.setDomain(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 := Store{Logger: TestLogger(t)} + + store.setDomain(configMapFromFile(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") + } +} + +func TestStoreFailedUpdate(t *testing.T) { + store := Store{Logger: TestLogger(t)} + + domainConfig := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: DomainConfigName, + Namespace: system.Namespace, + }, + Data: map[string]string{ + "example.com": "", + }, + } + + store.setDomain(domainConfig) + + domainConfig.Data = map[string]string{} // default is required + store.setDomain(domainConfig) + + config := store.Load() + if _, ok := config.Domain.Domains["example.com"]; !ok { + t.Errorf("Expected the update to fail") + } +} + +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 +}) + +func configMapFromFile(t *testing.T, name string) *corev1.ConfigMap { + t.Helper() + + b, err := ioutil.ReadFile(fmt.Sprintf("testdata/%s.yaml", name)) + if err != nil { + t.Errorf("ReadFile() = %v", err) + } + var cm corev1.ConfigMap + if err := yaml.Unmarshal(b, &cm); err != nil { + t.Errorf("yaml.Unmarshal() = %v", err) + } + + return &cm +} 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..2a6fd688f0cf 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 @@ -81,6 +82,8 @@ func NewController( virtualServiceInformer istioinformers.VirtualServiceInformer, ) *controller.Impl { + configStore := &config.Store{} + // No need to lock domainConfigMutex yet since the informers that can modify // domainConfig haven't started yet. c := &Reconciler{ @@ -90,8 +93,10 @@ func NewController( revisionLister: revisionInformer.Lister(), serviceLister: serviceInformer.Lister(), virtualServiceLister: virtualServiceInformer.Lister(), + configStore: configStore, } impl := controller.NewImpl(c, c.Logger, "Routes") + configStore.Logger = c.Logger.Named("config-store") c.Logger.Info("Setting up event handlers") routeInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ -122,7 +127,7 @@ func NewController( }) c.Logger.Info("Setting up ConfigMap receivers") - opt.ConfigMapWatcher.Watch(config.DomainConfigName, c.receiveDomainConfig) + configStore.WatchConfigs(opt.ConfigMapWatcher) return impl } @@ -142,6 +147,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 +207,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 +264,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..d357fdba64d5 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.WithConfig(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"}, + }, + }, + }, + } +} From a0710a2c8224398364cb2d66e75c9b55872bb151 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Thu, 13 Sep 2018 12:59:12 -0400 Subject: [PATCH 4/7] Refactor each reconciler's store to reconciler/config --- pkg/reconciler/config/store.go | 107 ++++++++++ pkg/reconciler/config/store_test.go | 182 ++++++++++++++++++ .../v1alpha1/revision/config/store.go | 98 +++------- .../v1alpha1/revision/config/store_test.go | 95 ++------- pkg/reconciler/v1alpha1/revision/revision.go | 7 +- .../v1alpha1/revision/revision_test.go | 2 +- .../v1alpha1/revision/table_test.go | 6 +- pkg/reconciler/v1alpha1/route/config/store.go | 58 +++--- .../v1alpha1/route/config/store_test.go | 92 +-------- pkg/reconciler/v1alpha1/route/route.go | 7 +- pkg/reconciler/v1alpha1/route/table_test.go | 2 +- 11 files changed, 371 insertions(+), 285 deletions(-) create mode 100644 pkg/reconciler/config/store.go create mode 100644 pkg/reconciler/config/store_test.go diff --git a/pkg/reconciler/config/store.go b/pkg/reconciler/config/store.go new file mode 100644 index 000000000000..b9295106ebd6 --- /dev/null +++ b/pkg/reconciler/config/store.go @@ -0,0 +1,107 @@ +/* +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" +) + +type Logger interface { + Infof(string, ...interface{}) + Fatalf(string, ...interface{}) + Errorf(string, ...interface{}) +} + +type UntypedStore struct { + name string + logger Logger + + storages map[string]*atomic.Value + constructors map[string]reflect.Value +} + +func NewUntypedStore( + name string, + logger Logger, + constructors ...interface{}) *UntypedStore { + + store := &UntypedStore{ + name: name, + logger: logger, + storages: make(map[string]*atomic.Value), + constructors: make(map[string]reflect.Value), + } + + // TODO(dprotaso) Check argument validity + for i := 0; i < len(constructors); i = i + 2 { + configName := constructors[i].(string) + constructor := constructors[i+1] + store.registerConfig(configName, constructor) + } + + return store +} + +func (s *UntypedStore) registerConfig(name string, constructor interface{}) { + // TODO(dprotaso) assert constructor type + s.storages[name] = &atomic.Value{} + s.constructors[name] = reflect.ValueOf(constructor) +} + +func (s *UntypedStore) WatchConfigs(w configmap.Watcher) { + for configMapName, _ := range s.constructors { + w.Watch(configMapName, s.OnConfigChanged) + } +} + +func (s *UntypedStore) UntypedLoad(name string) interface{} { + storage := s.storages[name] + return storage.Load() +} + +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), + } + + // Safety here will be addressed by the TODO in registerConfig + 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..83855903c58a --- /dev/null +++ b/pkg/reconciler/config/store_test.go @@ -0,0 +1,182 @@ +/* +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 TestStoreWatchConfigs(t *testing.T) { + constructor := func(c *corev1.ConfigMap) (interface{}, error) { + return c.Name, nil + } + + store := NewUntypedStore( + "name", + TestLogger(t), + "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), + "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), + "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), + "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 index 14a96075794e..27265df9a8d9 100644 --- a/pkg/reconciler/v1alpha1/revision/config/store.go +++ b/pkg/reconciler/v1alpha1/revision/config/store.go @@ -18,17 +18,14 @@ package config import ( "context" - "sync/atomic" - "github.com/knative/pkg/configmap" pkglogging "github.com/knative/pkg/logging" "github.com/knative/serving/pkg/autoscaler" "github.com/knative/serving/pkg/logging" - "go.uber.org/zap" - corev1 "k8s.io/api/core/v1" + "github.com/knative/serving/pkg/reconciler/config" ) -type configsKey struct{} +type cfgKey struct{} // +k8s:deepcopy-gen=false type Config struct { @@ -39,82 +36,45 @@ type Config struct { Autoscaler *autoscaler.Config } -// +k8s:deepcopy-gen=false -type Store struct { - Logger *zap.SugaredLogger - - controller atomic.Value - network atomic.Value - observability atomic.Value - logging atomic.Value - autoscaler atomic.Value -} - func FromContext(ctx context.Context) *Config { - return ctx.Value(configsKey{}).(*Config) -} - -func WithConfig(ctx context.Context, c *Config) context.Context { - return context.WithValue(ctx, configsKey{}, c) + return ctx.Value(cfgKey{}).(*Config) } -func (s *Store) ToContext(ctx context.Context) context.Context { - return WithConfig(ctx, s.Load()) +func ToContext(ctx context.Context, c *Config) context.Context { + return context.WithValue(ctx, cfgKey{}, c) } -func (s *Store) WatchConfigs(w configmap.Watcher) { - w.Watch(NetworkConfigName, s.setNetwork) - w.Watch(ObservabilityConfigName, s.setObservability) - w.Watch(ControllerConfigName, s.setController) - - w.Watch(autoscaler.ConfigName, s.setAutoscaler) - w.Watch(logging.ConfigName, s.setLogging) +// +k8s:deepcopy-gen=false +type Store struct { + *config.UntypedStore } -func (s *Store) Load() *Config { - return &Config{ - Controller: s.controller.Load().(*Controller).DeepCopy(), - Network: s.network.Load().(*Network).DeepCopy(), - Observability: s.observability.Load().(*Observability).DeepCopy(), - Logging: s.logging.Load().(*pkglogging.Config).DeepCopy(), - Autoscaler: s.autoscaler.Load().(*autoscaler.Config).DeepCopy(), +func NewStore(logger config.Logger) *Store { + store := &Store{ + UntypedStore: config.NewUntypedStore( + "revision", + logger, + ControllerConfigName, NewControllerConfigFromConfigMap, + NetworkConfigName, NewNetworkFromConfigMap, + ObservabilityConfigName, NewObservabilityFromConfigMap, + autoscaler.ConfigName, autoscaler.NewConfigFromConfigMap, + logging.ConfigName, logging.NewConfigFromConfigMap, + ), } -} - -func (s *Store) setController(c *corev1.ConfigMap) { - val, err := NewControllerConfigFromConfigMap(c) - s.save("controller", &s.controller, val, err) -} - -func (s *Store) setNetwork(c *corev1.ConfigMap) { - val, err := NewNetworkFromConfigMap(c) - s.save("network", &s.network, val, err) -} -func (s *Store) setObservability(c *corev1.ConfigMap) { - val, err := NewObservabilityFromConfigMap(c) - s.save("observability", &s.observability, val, err) + return store } -func (s *Store) setLogging(c *corev1.ConfigMap) { - val, err := logging.NewConfigFromConfigMap(c) - s.save("logging", &s.logging, val, err) -} - -func (s *Store) setAutoscaler(c *corev1.ConfigMap) { - val, err := autoscaler.NewConfigFromConfigMap(c) - s.save("autoscaler", &s.autoscaler, val, err) +func (s *Store) ToContext(ctx context.Context) context.Context { + return ToContext(ctx, s.Load()) } -func (s *Store) save(desc string, v *atomic.Value, value interface{}, err error) { - if err != nil { - if v.Load() != nil { - s.Logger.Errorf("Error updating revision %s config: %v", desc, err) - } else { - s.Logger.Fatalf("Error initializing revision %s config: %v", desc, err) - } - return +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(), } - s.Logger.Infof("Revision %s config was added or updated: %v", desc, value) - v.Store(value) } diff --git a/pkg/reconciler/v1alpha1/revision/config/store_test.go b/pkg/reconciler/v1alpha1/revision/config/store_test.go index bf753fd10941..09d33a5927aa 100644 --- a/pkg/reconciler/v1alpha1/revision/config/store_test.go +++ b/pkg/reconciler/v1alpha1/revision/config/store_test.go @@ -22,38 +22,14 @@ import ( "testing" "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/knative/pkg/configmap" "github.com/knative/serving/pkg/autoscaler" "github.com/knative/serving/pkg/logging" - "go.uber.org/zap/zapcore" . "github.com/knative/serving/pkg/reconciler/v1alpha1/testing" ) -func TestStoreWatchConfigs(t *testing.T) { - watcher := &mockWatcher{} - - store := Store{Logger: TestLogger(t)} - store.WatchConfigs(watcher) - - want := []string{ - ControllerConfigName, - NetworkConfigName, - ObservabilityConfigName, - logging.ConfigName, - autoscaler.ConfigName, - } - - got := watcher.watches - - if diff := cmp.Diff(want, got, sortStrings); diff != "" { - t.Errorf("Unexpected configmap watches (-want, +got): %v", diff) - } -} - func TestStoreLoadWithContext(t *testing.T) { - store := Store{Logger: TestLogger(t)} + store := NewStore(TestLogger(t)) controllerConfig := ConfigMapFromTestFile(t, ControllerConfigName) networkConfig := ConfigMapFromTestFile(t, NetworkConfigName) @@ -61,11 +37,11 @@ func TestStoreLoadWithContext(t *testing.T) { loggingConfig := ConfigMapFromTestFile(t, logging.ConfigName) autoscalerConfig := ConfigMapFromTestFile(t, autoscaler.ConfigName) - store.setController(controllerConfig) - store.setNetwork(networkConfig) - store.setObservability(observabilityConfig) - store.setLogging(loggingConfig) - store.setAutoscaler(autoscalerConfig) + store.OnConfigChanged(controllerConfig) + store.OnConfigChanged(networkConfig) + store.OnConfigChanged(observabilityConfig) + store.OnConfigChanged(loggingConfig) + store.OnConfigChanged(autoscalerConfig) config := FromContext(store.ToContext(context.Background())) @@ -106,13 +82,13 @@ func TestStoreLoadWithContext(t *testing.T) { } func TestStoreImmutableConfig(t *testing.T) { - store := Store{Logger: TestLogger(t)} + store := NewStore(TestLogger(t)) - store.setController(ConfigMapFromTestFile(t, ControllerConfigName)) - store.setNetwork(ConfigMapFromTestFile(t, NetworkConfigName)) - store.setObservability(ConfigMapFromTestFile(t, ObservabilityConfigName)) - store.setLogging(ConfigMapFromTestFile(t, logging.ConfigName)) - store.setAutoscaler(ConfigMapFromTestFile(t, autoscaler.ConfigName)) + 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() @@ -134,52 +110,9 @@ func TestStoreImmutableConfig(t *testing.T) { t.Error("Observability config is not immutable") } if newConfig.Logging.LoggingConfig == "mutated" { - t.Error("Logging config is not immutabled") + t.Error("Logging config is not immutable") } if newConfig.Autoscaler.MaxScaleUpRate == config.Autoscaler.MaxScaleUpRate { - t.Error("Autoscaler config is not immutabled") + t.Error("Autoscaler config is not immutable") } } - -func TestStoreFailedUpdate(t *testing.T) { - store := Store{Logger: TestLogger(t)} - - controllerConfig := ConfigMapFromTestFile(t, ControllerConfigName) - networkConfig := ConfigMapFromTestFile(t, NetworkConfigName) - observabilityConfig := ConfigMapFromTestFile(t, ObservabilityConfigName) - loggingConfig := ConfigMapFromTestFile(t, logging.ConfigName) - autoscalerConfig := ConfigMapFromTestFile(t, autoscaler.ConfigName) - - loggingConfig.Data["loglevel.controller"] = "debug" - - store.setController(controllerConfig) - store.setNetwork(networkConfig) - store.setObservability(observabilityConfig) - store.setLogging(loggingConfig) - store.setAutoscaler(autoscalerConfig) - - // Set a bad level which causes the update to fail - loggingConfig.Data["loglevel.controller"] = "unknown" - store.setLogging(loggingConfig) - - config := store.Load() - if got, want := config.Logging.LoggingLevel["controller"], zapcore.DebugLevel; got != want { - t.Errorf("Expected the update to fail - logging level want: %v, got: %v", want, got) - } -} - -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/revision.go b/pkg/reconciler/v1alpha1/revision/revision.go index b3a26cca9103..ba49dfa8de43 100644 --- a/pkg/reconciler/v1alpha1/revision/revision.go +++ b/pkg/reconciler/v1alpha1/revision/revision.go @@ -78,6 +78,7 @@ type resolver interface { type configStore interface { ToContext(ctx context.Context) context.Context WatchConfigs(w configmap.Watcher) + Load() *config.Config } // Reconciler implements controller.Reconciler for Revision resources. @@ -124,8 +125,6 @@ func NewController( vpaInformer vpav1alpha1informers.VerticalPodAutoscalerInformer, ) *controller.Impl { - configStore := &config.Store{} - c := &Reconciler{ Base: reconciler.NewBase(opt, controllerAgentName), vpaClient: vpaClient, @@ -138,15 +137,12 @@ func NewController( endpointsLister: endpointsInformer.Lister(), configMapLister: configMapInformer.Lister(), buildtracker: &buildTracker{builds: map[key]set{}}, - configStore: configStore, resolver: &digestResolver{ client: opt.KubeClientSet, transport: http.DefaultTransport, }, } - configStore.Logger = c.Logger.Named("config-store") - impl := controller.NewImpl(c, c.Logger, "Revisions") // Set up an event handler for when the resource types of interest change @@ -190,6 +186,7 @@ func NewController( // 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 diff --git a/pkg/reconciler/v1alpha1/revision/revision_test.go b/pkg/reconciler/v1alpha1/revision/revision_test.go index 963991151556..5821a975bf94 100644 --- a/pkg/reconciler/v1alpha1/revision/revision_test.go +++ b/pkg/reconciler/v1alpha1/revision/revision_test.go @@ -429,7 +429,7 @@ func TestCreateRevWithVPA(t *testing.T) { revClient := servingClient.ServingV1alpha1().Revisions(testNamespace) rev := getTestRevision() - if !controller.Reconciler.(*Reconciler).configStore.(*config.Store).Load().Autoscaler.EnableVPA { + if !controller.Reconciler.(*Reconciler).configStore.Load().Autoscaler.EnableVPA { t.Fatal("EnableVPA = false, want true") } diff --git a/pkg/reconciler/v1alpha1/revision/table_test.go b/pkg/reconciler/v1alpha1/revision/table_test.go index c4ac908600aa..7b7b022eecd6 100644 --- a/pkg/reconciler/v1alpha1/revision/table_test.go +++ b/pkg/reconciler/v1alpha1/revision/table_test.go @@ -1740,7 +1740,11 @@ type testConfigStore struct { } func (t *testConfigStore) ToContext(ctx context.Context) context.Context { - return config.WithConfig(ctx, t.config) + return config.ToContext(ctx, t.config) +} + +func (t *testConfigStore) Load() *config.Config { + return t.config } func (t *testConfigStore) WatchConfigs(w configmap.Watcher) {} diff --git a/pkg/reconciler/v1alpha1/route/config/store.go b/pkg/reconciler/v1alpha1/route/config/store.go index 4990c844d101..d7fc77ddb56f 100644 --- a/pkg/reconciler/v1alpha1/route/config/store.go +++ b/pkg/reconciler/v1alpha1/route/config/store.go @@ -18,62 +18,48 @@ package config import ( "context" - "sync/atomic" - "github.com/knative/pkg/configmap" - "go.uber.org/zap" - corev1 "k8s.io/api/core/v1" + "github.com/knative/serving/pkg/reconciler/config" ) -type configsKey struct{} +type cfgKey struct{} // +k8s:deepcopy-gen=false type Config struct { Domain *Domain } -// +k8s:deepcopy-gen=false -type Store struct { - Logger *zap.SugaredLogger - domain atomic.Value -} - func FromContext(ctx context.Context) *Config { - return ctx.Value(configsKey{}).(*Config) + return ctx.Value(cfgKey{}).(*Config) } -func WithConfig(ctx context.Context, c *Config) context.Context { - return context.WithValue(ctx, configsKey{}, c) +func ToContext(ctx context.Context, c *Config) context.Context { + return context.WithValue(ctx, cfgKey{}, c) } -func (s *Store) ToContext(ctx context.Context) context.Context { - return WithConfig(ctx, s.Load()) -} - -func (s *Store) WatchConfigs(w configmap.Watcher) { - w.Watch(DomainConfigName, s.setDomain) +// +k8s:deepcopy-gen=false +type Store struct { + *config.UntypedStore } -func (s *Store) Load() *Config { - return &Config{ - Domain: s.domain.Load().(*Domain).DeepCopy(), +func NewStore(logger config.Logger) *Store { + store := &Store{ + UntypedStore: config.NewUntypedStore( + "route", + logger, + DomainConfigName, NewDomainFromConfigMap, + ), } + + return store } -func (s *Store) setDomain(c *corev1.ConfigMap) { - val, err := NewDomainFromConfigMap(c) - s.save("domain", &s.domain, val, err) +func (s *Store) ToContext(ctx context.Context) context.Context { + return ToContext(ctx, s.Load()) } -func (s *Store) save(desc string, v *atomic.Value, value interface{}, err error) { - if err != nil { - if v.Load() != nil { - s.Logger.Errorf("Error updating route %s config: %v", desc, err) - } else { - s.Logger.Fatalf("Error initializing route %s config: %v", desc, err) - } - return +func (s *Store) Load() *Config { + return &Config{ + Domain: s.UntypedLoad(DomainConfigName).(*Domain).DeepCopy(), } - s.Logger.Infof("Route %s config was added or updated: %v", desc, value) - v.Store(value) } diff --git a/pkg/reconciler/v1alpha1/route/config/store_test.go b/pkg/reconciler/v1alpha1/route/config/store_test.go index d7632fd6aae3..1e9e792567de 100644 --- a/pkg/reconciler/v1alpha1/route/config/store_test.go +++ b/pkg/reconciler/v1alpha1/route/config/store_test.go @@ -18,44 +18,20 @@ package config import ( "context" - "fmt" - "io/ioutil" "testing" - "github.com/ghodss/yaml" "github.com/google/go-cmp/cmp" - "github.com/google/go-cmp/cmp/cmpopts" - "github.com/knative/pkg/configmap" - "github.com/knative/serving/pkg/system" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" . "github.com/knative/pkg/logging/testing" + . "github.com/knative/serving/pkg/reconciler/testing" ) -func TestStoreWatchConfigs(t *testing.T) { - watcher := &mockWatcher{} - - store := Store{Logger: TestLogger(t)} - store.WatchConfigs(watcher) - - want := []string{ - DomainConfigName, - } - - got := watcher.watches - - if diff := cmp.Diff(want, got, sortStrings); diff != "" { - t.Errorf("Unexpected configmap watches (-want, +got): %v", diff) - } -} - func TestStoreLoadWithContext(t *testing.T) { - store := Store{Logger: TestLogger(t)} + store := NewStore(TestLogger(t)) - domainConfig := configMapFromFile(t, DomainConfigName) + domainConfig := ConfigMapFromTestFile(t, DomainConfigName) - store.setDomain(domainConfig) + store.OnConfigChanged(domainConfig) config := FromContext(store.ToContext(context.Background())) @@ -68,9 +44,8 @@ func TestStoreLoadWithContext(t *testing.T) { } func TestStoreImmutableConfig(t *testing.T) { - store := Store{Logger: TestLogger(t)} - - store.setDomain(configMapFromFile(t, DomainConfigName)) + store := NewStore(TestLogger(t)) + store.OnConfigChanged(ConfigMapFromTestFile(t, DomainConfigName)) config := store.Load() @@ -84,58 +59,3 @@ func TestStoreImmutableConfig(t *testing.T) { t.Error("Domain config is not immutable") } } - -func TestStoreFailedUpdate(t *testing.T) { - store := Store{Logger: TestLogger(t)} - - domainConfig := &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: DomainConfigName, - Namespace: system.Namespace, - }, - Data: map[string]string{ - "example.com": "", - }, - } - - store.setDomain(domainConfig) - - domainConfig.Data = map[string]string{} // default is required - store.setDomain(domainConfig) - - config := store.Load() - if _, ok := config.Domain.Domains["example.com"]; !ok { - t.Errorf("Expected the update to fail") - } -} - -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 -}) - -func configMapFromFile(t *testing.T, name string) *corev1.ConfigMap { - t.Helper() - - b, err := ioutil.ReadFile(fmt.Sprintf("testdata/%s.yaml", name)) - if err != nil { - t.Errorf("ReadFile() = %v", err) - } - var cm corev1.ConfigMap - if err := yaml.Unmarshal(b, &cm); err != nil { - t.Errorf("yaml.Unmarshal() = %v", err) - } - - return &cm -} diff --git a/pkg/reconciler/v1alpha1/route/route.go b/pkg/reconciler/v1alpha1/route/route.go index 2a6fd688f0cf..f28683f609b2 100644 --- a/pkg/reconciler/v1alpha1/route/route.go +++ b/pkg/reconciler/v1alpha1/route/route.go @@ -82,8 +82,6 @@ func NewController( virtualServiceInformer istioinformers.VirtualServiceInformer, ) *controller.Impl { - configStore := &config.Store{} - // No need to lock domainConfigMutex yet since the informers that can modify // domainConfig haven't started yet. c := &Reconciler{ @@ -93,10 +91,8 @@ func NewController( revisionLister: revisionInformer.Lister(), serviceLister: serviceInformer.Lister(), virtualServiceLister: virtualServiceInformer.Lister(), - configStore: configStore, } impl := controller.NewImpl(c, c.Logger, "Routes") - configStore.Logger = c.Logger.Named("config-store") c.Logger.Info("Setting up event handlers") routeInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{ @@ -127,7 +123,8 @@ func NewController( }) c.Logger.Info("Setting up ConfigMap receivers") - configStore.WatchConfigs(opt.ConfigMapWatcher) + c.configStore = config.NewStore(c.Logger.Named("config-store")) + c.configStore.WatchConfigs(opt.ConfigMapWatcher) return impl } diff --git a/pkg/reconciler/v1alpha1/route/table_test.go b/pkg/reconciler/v1alpha1/route/table_test.go index d357fdba64d5..d9a663489001 100644 --- a/pkg/reconciler/v1alpha1/route/table_test.go +++ b/pkg/reconciler/v1alpha1/route/table_test.go @@ -1666,7 +1666,7 @@ type testConfigStore struct { } func (t *testConfigStore) ToContext(ctx context.Context) context.Context { - return config.WithConfig(ctx, t.config) + return config.ToContext(ctx, t.config) } func (t *testConfigStore) WatchConfigs(w configmap.Watcher) {} From 5152e4adcc02ccb3c3f60649ebe2ef7677ba4d23 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Tue, 18 Sep 2018 14:08:30 -0400 Subject: [PATCH 5/7] Use a typedef'd map to help with argument validity --- pkg/reconciler/config/store.go | 9 ++++----- pkg/reconciler/config/store_test.go | 20 ++++++++++++------- .../v1alpha1/revision/config/store.go | 12 ++++++----- pkg/reconciler/v1alpha1/route/config/store.go | 4 +++- 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/pkg/reconciler/config/store.go b/pkg/reconciler/config/store.go index b9295106ebd6..0b1a8178268c 100644 --- a/pkg/reconciler/config/store.go +++ b/pkg/reconciler/config/store.go @@ -30,6 +30,8 @@ type Logger interface { Errorf(string, ...interface{}) } +type Constructors map[string]interface{} + type UntypedStore struct { name string logger Logger @@ -41,7 +43,7 @@ type UntypedStore struct { func NewUntypedStore( name string, logger Logger, - constructors ...interface{}) *UntypedStore { + constructors Constructors) *UntypedStore { store := &UntypedStore{ name: name, @@ -50,10 +52,7 @@ func NewUntypedStore( constructors: make(map[string]reflect.Value), } - // TODO(dprotaso) Check argument validity - for i := 0; i < len(constructors); i = i + 2 { - configName := constructors[i].(string) - constructor := constructors[i+1] + for configName, constructor := range constructors { store.registerConfig(configName, constructor) } diff --git a/pkg/reconciler/config/store_test.go b/pkg/reconciler/config/store_test.go index 83855903c58a..63f87bb893f7 100644 --- a/pkg/reconciler/config/store_test.go +++ b/pkg/reconciler/config/store_test.go @@ -41,8 +41,10 @@ func TestStoreWatchConfigs(t *testing.T) { store := NewUntypedStore( "name", TestLogger(t), - "config-name-1", constructor, - "config-name-2", constructor, + Constructors{ + "config-name-1": constructor, + "config-name-2": constructor, + }, ) watcher := &mockWatcher{} @@ -65,9 +67,13 @@ func TestStoreConfigChange(t *testing.T) { return c.Name, nil } - store := NewUntypedStore("name", TestLogger(t), - "config-name-1", constructor, - "config-name-2", constructor, + store := NewUntypedStore( + "name", + TestLogger(t), + Constructors{ + "config-name-1": constructor, + "config-name-2": constructor, + }, ) store.OnConfigChanged(&corev1.ConfigMap{ @@ -108,7 +114,7 @@ func TestStoreFailedFirstConversionCrashes(t *testing.T) { } store := NewUntypedStore("name", TestLogger(t), - "config-name-1", constructor, + Constructors{"config-name-1": constructor}, ) store.OnConfigChanged(&corev1.ConfigMap{ @@ -140,7 +146,7 @@ func TestStoreFailedUpdate(t *testing.T) { } store := NewUntypedStore("name", TestLogger(t), - "config-name-1", constructor, + Constructors{"config-name-1": constructor}, ) store.OnConfigChanged(&corev1.ConfigMap{ diff --git a/pkg/reconciler/v1alpha1/revision/config/store.go b/pkg/reconciler/v1alpha1/revision/config/store.go index 27265df9a8d9..32d98933a455 100644 --- a/pkg/reconciler/v1alpha1/revision/config/store.go +++ b/pkg/reconciler/v1alpha1/revision/config/store.go @@ -54,11 +54,13 @@ func NewStore(logger config.Logger) *Store { UntypedStore: config.NewUntypedStore( "revision", logger, - ControllerConfigName, NewControllerConfigFromConfigMap, - NetworkConfigName, NewNetworkFromConfigMap, - ObservabilityConfigName, NewObservabilityFromConfigMap, - autoscaler.ConfigName, autoscaler.NewConfigFromConfigMap, - logging.ConfigName, logging.NewConfigFromConfigMap, + config.Constructors{ + ControllerConfigName: NewControllerConfigFromConfigMap, + NetworkConfigName: NewNetworkFromConfigMap, + ObservabilityConfigName: NewObservabilityFromConfigMap, + autoscaler.ConfigName: autoscaler.NewConfigFromConfigMap, + logging.ConfigName: logging.NewConfigFromConfigMap, + }, ), } diff --git a/pkg/reconciler/v1alpha1/route/config/store.go b/pkg/reconciler/v1alpha1/route/config/store.go index d7fc77ddb56f..1b34db4f9403 100644 --- a/pkg/reconciler/v1alpha1/route/config/store.go +++ b/pkg/reconciler/v1alpha1/route/config/store.go @@ -47,7 +47,9 @@ func NewStore(logger config.Logger) *Store { UntypedStore: config.NewUntypedStore( "route", logger, - DomainConfigName, NewDomainFromConfigMap, + config.Constructors{ + DomainConfigName: NewDomainFromConfigMap, + }, ), } From d9e172111e8f3eebe7177e6b581a515ada1526e7 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Tue, 18 Sep 2018 14:19:20 -0400 Subject: [PATCH 6/7] assert constructor type --- pkg/reconciler/config/store.go | 18 +++++++++++++-- pkg/reconciler/config/store_test.go | 36 +++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/config/store.go b/pkg/reconciler/config/store.go index 0b1a8178268c..f7e41162a651 100644 --- a/pkg/reconciler/config/store.go +++ b/pkg/reconciler/config/store.go @@ -60,7 +60,22 @@ func NewUntypedStore( } func (s *UntypedStore) registerConfig(name string, constructor interface{}) { - // TODO(dprotaso) assert constructor type + 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) } @@ -86,7 +101,6 @@ func (s *UntypedStore) OnConfigChanged(c *corev1.ConfigMap) { reflect.ValueOf(c), } - // Safety here will be addressed by the TODO in registerConfig outputs := constructor.Call(inputs) result := outputs[0].Interface() errVal := outputs[1] diff --git a/pkg/reconciler/config/store_test.go b/pkg/reconciler/config/store_test.go index 63f87bb893f7..fefb584c6c97 100644 --- a/pkg/reconciler/config/store_test.go +++ b/pkg/reconciler/config/store_test.go @@ -33,6 +33,42 @@ import ( . "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 From e4b8deee0855418da0199217430bf6fbf490afe6 Mon Sep 17 00:00:00 2001 From: Dave Protasowski Date: Tue, 18 Sep 2018 20:40:18 -0400 Subject: [PATCH 7/7] update godoc comments --- pkg/reconciler/config/store.go | 40 +++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/config/store.go b/pkg/reconciler/config/store.go index f7e41162a651..da86aef59730 100644 --- a/pkg/reconciler/config/store.go +++ b/pkg/reconciler/config/store.go @@ -24,14 +24,29 @@ import ( 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 @@ -40,6 +55,19 @@ type UntypedStore struct { 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, @@ -80,17 +108,27 @@ func (s *UntypedStore) registerConfig(name string, constructor interface{}) { 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 { + 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