From f707a175f901adfda26cdfe38d692e6411ba859e Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Thu, 14 Nov 2024 12:10:54 +0200 Subject: [PATCH 1/7] Fix configuration defaulting --- pkg/apis/serving/v1/configuration_defaults.go | 13 +++++ .../serving/v1/configuration_defaults_test.go | 56 ++++++++++++++++++- pkg/apis/serving/v1/revision_defaults_test.go | 31 ++++++++++ 3 files changed, 99 insertions(+), 1 deletion(-) diff --git a/pkg/apis/serving/v1/configuration_defaults.go b/pkg/apis/serving/v1/configuration_defaults.go index 008c22358581..f695520049fd 100644 --- a/pkg/apis/serving/v1/configuration_defaults.go +++ b/pkg/apis/serving/v1/configuration_defaults.go @@ -20,7 +20,9 @@ import ( "context" "knative.dev/pkg/apis" + "knative.dev/serving/pkg/apis/config" "knative.dev/serving/pkg/apis/serving" + cconfig "knative.dev/serving/pkg/reconciler/configuration/config" ) type configSpecKey struct{} @@ -67,5 +69,16 @@ func (cs *ConfigurationSpec) SetDefaults(ctx context.Context) { return } } + + configurationConfig := cconfig.FromContext(ctx) + apisConfig := config.Config{} + if configurationConfig != nil && configurationConfig.Defaults != nil { + apisConfig.Defaults = configurationConfig.Defaults.DeepCopy() + } + if configurationConfig != nil && configurationConfig.Features != nil { + apisConfig.Features = configurationConfig.Features.DeepCopy() + } + ctx = config.ToContext(ctx, &apisConfig) + cs.Template.SetDefaults(ctx) } diff --git a/pkg/apis/serving/v1/configuration_defaults_test.go b/pkg/apis/serving/v1/configuration_defaults_test.go index ae6e8d9058e3..109453d1ada7 100644 --- a/pkg/apis/serving/v1/configuration_defaults_test.go +++ b/pkg/apis/serving/v1/configuration_defaults_test.go @@ -26,10 +26,11 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "knative.dev/pkg/apis" + logtesting "knative.dev/pkg/logging/testing" "knative.dev/pkg/ptr" - "knative.dev/serving/pkg/apis/config" "knative.dev/serving/pkg/apis/serving" + cconfig "knative.dev/serving/pkg/reconciler/configuration/config" ) func TestConfigurationDefaulting(t *testing.T) { @@ -156,6 +157,59 @@ func TestConfigurationDefaulting(t *testing.T) { }, }, }, + }, { + name: "run latest with identical timeout defaults", + in: &Configuration{ + Spec: ConfigurationSpec{ + Template: RevisionTemplateSpec{ + Spec: RevisionSpec{ + PodSpec: corev1.PodSpec{ + EnableServiceLinks: ptr.Bool(true), + Containers: []corev1.Container{{ + Image: "busybox", + }}, + }, + ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), + }, + }, + }, + }, + want: &Configuration{ + Spec: ConfigurationSpec{ + Template: RevisionTemplateSpec{ + Spec: RevisionSpec{ + PodSpec: corev1.PodSpec{ + EnableServiceLinks: ptr.Bool(true), + Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, + Image: "busybox", + Resources: defaultResources, + ReadinessProbe: defaultProbe, + }}, + }, + TimeoutSeconds: ptr.Int64(423), + ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), + }, + }, + }, + }, + ctx: func() context.Context { + logger := logtesting.TestLogger(t) + s := cconfig.NewStore(logger) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "revision-timeout-seconds": "423", + "revision-response-start-timeout-seconds": "423", + "revision-idle-timeout-seconds": "423", + }, + }) + + return s.ToContext(context.Background()) + }(), }} for _, test := range tests { diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index 4d08fd3f8ccf..2d6a3434888f 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -129,6 +129,37 @@ func TestRevisionDefaulting(t *testing.T) { }, }, }, + }, { + name: "Some revision timeouts set with identical values", + in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, + wc: func(ctx context.Context) context.Context { + s := config.NewStore(logger) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "revision-timeout-seconds": "423", + "revision-response-start-timeout-seconds": "423", + }, + }) + return s.ToContext(ctx) + }, + want: &Revision{ + Spec: RevisionSpec{ + ContainerConcurrency: ptr.Int64(0), + TimeoutSeconds: ptr.Int64(423), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, + Resources: defaultResources, + ReadinessProbe: defaultProbe, + }}, + }, + }, + }, }, { name: "with context, in create, expect ESL set", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, From 2bbcb45b4a9cd20538895ab7a85241bf497f4f35 Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Tue, 14 Jan 2025 12:02:08 +0200 Subject: [PATCH 2/7] address comments, refactoring --- .../serving/v1/configuration_defaults_test.go | 35 +-- pkg/apis/serving/v1/revision_defaults_test.go | 207 +++++++----------- 2 files changed, 97 insertions(+), 145 deletions(-) diff --git a/pkg/apis/serving/v1/configuration_defaults_test.go b/pkg/apis/serving/v1/configuration_defaults_test.go index 109453d1ada7..332210216867 100644 --- a/pkg/apis/serving/v1/configuration_defaults_test.go +++ b/pkg/apis/serving/v1/configuration_defaults_test.go @@ -18,9 +18,11 @@ package v1 import ( "context" + "fmt" "testing" "github.com/google/go-cmp/cmp" + "go.uber.org/zap" authv1 "k8s.io/api/authentication/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -33,6 +35,8 @@ import ( cconfig "knative.dev/serving/pkg/reconciler/configuration/config" ) +const defaultTimeoutSeconds = 400 + func TestConfigurationDefaulting(t *testing.T) { tests := []struct { name string @@ -187,29 +191,22 @@ func TestConfigurationDefaulting(t *testing.T) { ReadinessProbe: defaultProbe, }}, }, - TimeoutSeconds: ptr.Int64(423), + TimeoutSeconds: ptr.Int64(defaultTimeoutSeconds), ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), }, }, }, }, - ctx: func() context.Context { - logger := logtesting.TestLogger(t) - s := cconfig.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + ctx: defaultConfigurationContextWithStore(logtesting.TestLogger(t), corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": "423", - "revision-response-start-timeout-seconds": "423", - "revision-idle-timeout-seconds": "423", - }, - }) - - return s.ToContext(context.Background()) - }(), + "revision-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), + "revision-response-start-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), + "revision-idle-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), + }})(context.Background()), }} for _, test := range tests { @@ -382,3 +379,13 @@ func TestConfigurationUserInfo(t *testing.T) { }) } } + +func defaultConfigurationContextWithStore(logger *zap.SugaredLogger, cms ...corev1.ConfigMap) func(ctx context.Context) context.Context { + return func(ctx context.Context) context.Context { + s := cconfig.NewStore(logger) + for _, cm := range cms { + s.OnConfigChanged(&cm) + } + return s.ToContext(ctx) + } +} diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index 2d6a3434888f..34460fde1b7d 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -18,10 +18,12 @@ package v1 import ( "context" + "fmt" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + "go.uber.org/zap" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" @@ -67,25 +69,19 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "with context", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": "423", - }, - }) - - return s.ToContext(ctx) - }, + "revision-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), - TimeoutSeconds: ptr.Int64(423), + TimeoutSeconds: ptr.Int64(defaultTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: config.DefaultUserContainerName, @@ -98,26 +94,21 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "all revision timeouts set", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": "423", + "revision-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), "revision-idle-timeout-seconds": "100", "revision-response-start-timeout-seconds": "50", - }, - }) - return s.ToContext(ctx) - }, + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), - TimeoutSeconds: ptr.Int64(423), + TimeoutSeconds: ptr.Int64(defaultTimeoutSeconds), ResponseStartTimeoutSeconds: ptr.Int64(50), IdleTimeoutSeconds: ptr.Int64(100), PodSpec: corev1.PodSpec{ @@ -132,25 +123,18 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "Some revision timeouts set with identical values", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "revision-timeout-seconds": "423", - "revision-response-start-timeout-seconds": "423", - }, - }) - return s.ToContext(ctx) - }, + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "revision-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), + "revision-response-start-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), - TimeoutSeconds: ptr.Int64(423), + TimeoutSeconds: ptr.Int64(defaultTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: config.DefaultUserContainerName, @@ -163,21 +147,15 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "with context, in create, expect ESL set", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, apis.WithinCreate, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ "revision-timeout-seconds": "323", - }, - }) - - return apis.WithinCreate(s.ToContext(ctx)) - }, + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -200,20 +178,15 @@ func TestRevisionDefaulting(t *testing.T) { Containers: []corev1.Container{{}}, }, }}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ "enable-service-links": "true", - }, - }) - return s.ToContext(ctx) - }, + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -231,20 +204,15 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "with service links CM `true`", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, apis.WithinCreate, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ "enable-service-links": "true", - }, - }) - return apis.WithinCreate(s.ToContext(ctx)) - }, + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -262,20 +230,15 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "with service links `false`", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, apis.WithinCreate, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ "enable-service-links": "false", - }, - }) - return apis.WithinCreate(s.ToContext(ctx)) - }, + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -296,20 +259,15 @@ func TestRevisionDefaulting(t *testing.T) { EnableServiceLinks: ptr.Bool(false), Containers: []corev1.Container{{}}, }}}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ "enable-service-links": "true", // this should be ignored. - }, - }) - return s.ToContext(ctx) - }, + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -363,21 +321,15 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "timeout sets to default when 0 is specified", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}, TimeoutSeconds: ptr.Int64(0)}}, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, Data: map[string]string{ "revision-timeout-seconds": "456", - }, - }) - - return s.ToContext(ctx) - }, + }}), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -528,11 +480,9 @@ func TestRevisionDefaulting(t *testing.T) { PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}, }, }, - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, + corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, @@ -543,11 +493,7 @@ func TestRevisionDefaulting(t *testing.T) { "revision-cpu-limit": "400M", "revision-memory-limit": "500m", "revision-ephemeral-storage-limit": "600M", - }, - }) - - return s.ToContext(ctx) - }, + }}), want: &Revision{ Spec: RevisionSpec{ TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), @@ -902,19 +848,12 @@ func TestRevisionDefaulting(t *testing.T) { }, }, { name: "Default security context with feature enabled", - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.DefaultsConfigName}}) - s.OnConfigChanged( - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}, - Data: map[string]string{"secure-pod-defaults": "Enabled"}, - }, - ) - - return s.ToContext(ctx) - }, + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.DefaultsConfigName}}, + corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}, + Data: map[string]string{"secure-pod-defaults": "Enabled"}, + }), in: &Revision{ Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ @@ -1022,19 +961,12 @@ func TestRevisionDefaulting(t *testing.T) { }, }, { name: "uses pod defaults in security context", - wc: func(ctx context.Context) context.Context { - s := config.NewStore(logger) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) - s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.DefaultsConfigName}}) - s.OnConfigChanged( - &corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}, - Data: map[string]string{"secure-pod-defaults": "Enabled"}, - }, - ) - - return s.ToContext(ctx) - }, + wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, + corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.DefaultsConfigName}}, + corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}, + Data: map[string]string{"secure-pod-defaults": "Enabled"}, + }), in: &Revision{ Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ @@ -1374,3 +1306,16 @@ func TestRevisionDefaultingContainerName(t *testing.T) { t.Errorf("Failed to set default values for init container name") } } + +func defaultRevisionContextWithStore(logger *zap.SugaredLogger, ctxFunc func(ctx context.Context) context.Context, cms ...corev1.ConfigMap) func(ctx context.Context) context.Context { + return func(ctx context.Context) context.Context { + s := config.NewStore(logger) + for _, cm := range cms { + s.OnConfigChanged(&cm) + } + if ctxFunc != nil { + ctx = ctxFunc(ctx) + } + return s.ToContext(ctx) + } +} From c11b5c901747dd055579622c24b16613f695c2a6 Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Mon, 20 Jan 2025 14:15:52 +0200 Subject: [PATCH 3/7] refactor --- pkg/apis/serving/v1/configuration_defaults.go | 11 +---------- pkg/apis/serving/v1/configuration_defaults_test.go | 8 ++++---- pkg/reconciler/configuration/config/store.go | 10 +++++++++- 3 files changed, 14 insertions(+), 15 deletions(-) diff --git a/pkg/apis/serving/v1/configuration_defaults.go b/pkg/apis/serving/v1/configuration_defaults.go index f695520049fd..278e54a460cd 100644 --- a/pkg/apis/serving/v1/configuration_defaults.go +++ b/pkg/apis/serving/v1/configuration_defaults.go @@ -20,7 +20,6 @@ import ( "context" "knative.dev/pkg/apis" - "knative.dev/serving/pkg/apis/config" "knative.dev/serving/pkg/apis/serving" cconfig "knative.dev/serving/pkg/reconciler/configuration/config" ) @@ -71,14 +70,6 @@ func (cs *ConfigurationSpec) SetDefaults(ctx context.Context) { } configurationConfig := cconfig.FromContext(ctx) - apisConfig := config.Config{} - if configurationConfig != nil && configurationConfig.Defaults != nil { - apisConfig.Defaults = configurationConfig.Defaults.DeepCopy() - } - if configurationConfig != nil && configurationConfig.Features != nil { - apisConfig.Features = configurationConfig.Features.DeepCopy() - } - ctx = config.ToContext(ctx, &apisConfig) - + ctx = cconfig.ToContext(ctx, configurationConfig) cs.Template.SetDefaults(ctx) } diff --git a/pkg/apis/serving/v1/configuration_defaults_test.go b/pkg/apis/serving/v1/configuration_defaults_test.go index 332210216867..4092492b14f6 100644 --- a/pkg/apis/serving/v1/configuration_defaults_test.go +++ b/pkg/apis/serving/v1/configuration_defaults_test.go @@ -18,7 +18,7 @@ package v1 import ( "context" - "fmt" + "strconv" "testing" "github.com/google/go-cmp/cmp" @@ -203,9 +203,9 @@ func TestConfigurationDefaulting(t *testing.T) { Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), - "revision-response-start-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), - "revision-idle-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), + "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + "revision-response-start-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + "revision-idle-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), }})(context.Background()), }} diff --git a/pkg/reconciler/configuration/config/store.go b/pkg/reconciler/configuration/config/store.go index 5b602992050a..e81095b4edd6 100644 --- a/pkg/reconciler/configuration/config/store.go +++ b/pkg/reconciler/configuration/config/store.go @@ -20,6 +20,7 @@ import ( "context" "knative.dev/pkg/configmap" + apisconfig "knative.dev/serving/pkg/apis/config" cfgmap "knative.dev/serving/pkg/apis/config" ) @@ -63,7 +64,14 @@ func FromContextOrDefaults(ctx context.Context) *Config { // ToContext attaches the provided Config to the provided context, returning the // new context with the Config attached. func ToContext(ctx context.Context, c *Config) context.Context { - return context.WithValue(ctx, cfgKey{}, c) + ctx = context.WithValue(ctx, cfgKey{}, c) + if c != nil { + ctx = apisconfig.ToContext(ctx, &apisconfig.Config{ + Defaults: c.Defaults, + Features: c.Features, + }) + } + return ctx } // Store is a typed wrapper around configmap.Untyped store to handle our configmaps. From 5df6756ed36c608dd839951e3e1a4dccb00321a0 Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Mon, 20 Jan 2025 14:27:14 +0200 Subject: [PATCH 4/7] lint --- .../serving/v1/configuration_defaults_test.go | 3 +- pkg/apis/serving/v1/revision_defaults_test.go | 40 ++++++++++++------- pkg/reconciler/configuration/config/store.go | 17 ++++---- 3 files changed, 35 insertions(+), 25 deletions(-) diff --git a/pkg/apis/serving/v1/configuration_defaults_test.go b/pkg/apis/serving/v1/configuration_defaults_test.go index 4092492b14f6..6de83d955371 100644 --- a/pkg/apis/serving/v1/configuration_defaults_test.go +++ b/pkg/apis/serving/v1/configuration_defaults_test.go @@ -206,7 +206,8 @@ func TestConfigurationDefaulting(t *testing.T) { "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), "revision-response-start-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), "revision-idle-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), - }})(context.Background()), + }, + })(context.Background()), }} for _, test := range tests { diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index 34460fde1b7d..17b87af1f088 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -18,7 +18,7 @@ package v1 import ( "context" - "fmt" + "strconv" "testing" "github.com/google/go-cmp/cmp" @@ -76,8 +76,9 @@ func TestRevisionDefaulting(t *testing.T) { Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), - }}), + "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -101,10 +102,11 @@ func TestRevisionDefaulting(t *testing.T) { Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), + "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), "revision-idle-timeout-seconds": "100", "revision-response-start-timeout-seconds": "50", - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -128,9 +130,10 @@ func TestRevisionDefaulting(t *testing.T) { Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), - "revision-response-start-timeout-seconds": fmt.Sprintf("%d", defaultTimeoutSeconds), - }}), + "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + "revision-response-start-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -155,7 +158,8 @@ func TestRevisionDefaulting(t *testing.T) { }, Data: map[string]string{ "revision-timeout-seconds": "323", - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -186,7 +190,8 @@ func TestRevisionDefaulting(t *testing.T) { }, Data: map[string]string{ "enable-service-links": "true", - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -212,7 +217,8 @@ func TestRevisionDefaulting(t *testing.T) { }, Data: map[string]string{ "enable-service-links": "true", - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -238,7 +244,8 @@ func TestRevisionDefaulting(t *testing.T) { }, Data: map[string]string{ "enable-service-links": "false", - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -267,7 +274,8 @@ func TestRevisionDefaulting(t *testing.T) { }, Data: map[string]string{ "enable-service-links": "true", // this should be ignored. - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -329,7 +337,8 @@ func TestRevisionDefaulting(t *testing.T) { }, Data: map[string]string{ "revision-timeout-seconds": "456", - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -493,7 +502,8 @@ func TestRevisionDefaulting(t *testing.T) { "revision-cpu-limit": "400M", "revision-memory-limit": "500m", "revision-ephemeral-storage-limit": "600M", - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), diff --git a/pkg/reconciler/configuration/config/store.go b/pkg/reconciler/configuration/config/store.go index e81095b4edd6..197fbd4fd6be 100644 --- a/pkg/reconciler/configuration/config/store.go +++ b/pkg/reconciler/configuration/config/store.go @@ -21,15 +21,14 @@ import ( "knative.dev/pkg/configmap" apisconfig "knative.dev/serving/pkg/apis/config" - cfgmap "knative.dev/serving/pkg/apis/config" ) type cfgKey struct{} // Config holds the collection of configurations that we attach to contexts. type Config struct { - Defaults *cfgmap.Defaults - Features *cfgmap.Features + Defaults *apisconfig.Defaults + Features *apisconfig.Features } // FromContext extracts a Config from the provided context. @@ -51,11 +50,11 @@ func FromContextOrDefaults(ctx context.Context) *Config { } if cfg.Defaults == nil { - cfg.Defaults, _ = cfgmap.NewDefaultsConfigFromMap(nil) + cfg.Defaults, _ = apisconfig.NewDefaultsConfigFromMap(nil) } if cfg.Features == nil { - cfg.Features, _ = cfgmap.NewFeaturesConfigFromMap(nil) + cfg.Features, _ = apisconfig.NewFeaturesConfigFromMap(nil) } return cfg @@ -86,8 +85,8 @@ func NewStore(logger configmap.Logger, onAfterStore ...func(name string, value i "apis", logger, configmap.Constructors{ - cfgmap.DefaultsConfigName: cfgmap.NewDefaultsConfigFromConfigMap, - cfgmap.FeaturesConfigName: cfgmap.NewFeaturesConfigFromConfigMap, + apisconfig.DefaultsConfigName: apisconfig.NewDefaultsConfigFromConfigMap, + apisconfig.FeaturesConfigName: apisconfig.NewFeaturesConfigFromConfigMap, }, onAfterStore..., ), @@ -104,10 +103,10 @@ func (s *Store) ToContext(ctx context.Context) context.Context { // Load creates a Config from the current config state of the Store. func (s *Store) Load() *Config { cfg := &Config{} - if def, ok := s.UntypedLoad(cfgmap.DefaultsConfigName).(*cfgmap.Defaults); ok { + if def, ok := s.UntypedLoad(apisconfig.DefaultsConfigName).(*apisconfig.Defaults); ok { cfg.Defaults = def.DeepCopy() } - if feat, ok := s.UntypedLoad(cfgmap.FeaturesConfigName).(*cfgmap.Features); ok { + if feat, ok := s.UntypedLoad(apisconfig.FeaturesConfigName).(*apisconfig.Features); ok { cfg.Features = feat.DeepCopy() } From ed1188b283be75d3faaf1e100be23183cdab03e0 Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Mon, 20 Jan 2025 14:40:26 +0200 Subject: [PATCH 5/7] fixes --- pkg/apis/serving/v1/configuration_defaults.go | 4 ---- .../serving/v1/configuration_defaults_test.go | 10 +++++----- pkg/apis/serving/v1/revision_defaults_test.go | 17 ++++++++--------- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/pkg/apis/serving/v1/configuration_defaults.go b/pkg/apis/serving/v1/configuration_defaults.go index 278e54a460cd..008c22358581 100644 --- a/pkg/apis/serving/v1/configuration_defaults.go +++ b/pkg/apis/serving/v1/configuration_defaults.go @@ -21,7 +21,6 @@ import ( "knative.dev/pkg/apis" "knative.dev/serving/pkg/apis/serving" - cconfig "knative.dev/serving/pkg/reconciler/configuration/config" ) type configSpecKey struct{} @@ -68,8 +67,5 @@ func (cs *ConfigurationSpec) SetDefaults(ctx context.Context) { return } } - - configurationConfig := cconfig.FromContext(ctx) - ctx = cconfig.ToContext(ctx, configurationConfig) cs.Template.SetDefaults(ctx) } diff --git a/pkg/apis/serving/v1/configuration_defaults_test.go b/pkg/apis/serving/v1/configuration_defaults_test.go index 6de83d955371..b12c88aca88c 100644 --- a/pkg/apis/serving/v1/configuration_defaults_test.go +++ b/pkg/apis/serving/v1/configuration_defaults_test.go @@ -35,7 +35,7 @@ import ( cconfig "knative.dev/serving/pkg/reconciler/configuration/config" ) -const defaultTimeoutSeconds = 400 +const someTimeoutSeconds = 400 func TestConfigurationDefaulting(t *testing.T) { tests := []struct { @@ -191,7 +191,7 @@ func TestConfigurationDefaulting(t *testing.T) { ReadinessProbe: defaultProbe, }}, }, - TimeoutSeconds: ptr.Int64(defaultTimeoutSeconds), + TimeoutSeconds: ptr.Int64(someTimeoutSeconds), ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), }, }, @@ -203,9 +203,9 @@ func TestConfigurationDefaulting(t *testing.T) { Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), - "revision-response-start-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), - "revision-idle-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + "revision-timeout-seconds": strconv.Itoa(someTimeoutSeconds), + "revision-response-start-timeout-seconds": strconv.Itoa(someTimeoutSeconds), + "revision-idle-timeout-seconds": strconv.Itoa(someTimeoutSeconds), }, })(context.Background()), }} diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index 17b87af1f088..8cf0eec22f37 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -76,13 +76,13 @@ func TestRevisionDefaulting(t *testing.T) { Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + "revision-timeout-seconds": strconv.Itoa(someTimeoutSeconds), }, }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), - TimeoutSeconds: ptr.Int64(defaultTimeoutSeconds), + TimeoutSeconds: ptr.Int64(someTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: config.DefaultUserContainerName, @@ -102,7 +102,7 @@ func TestRevisionDefaulting(t *testing.T) { Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + "revision-timeout-seconds": strconv.Itoa(someTimeoutSeconds), "revision-idle-timeout-seconds": "100", "revision-response-start-timeout-seconds": "50", }, @@ -110,7 +110,7 @@ func TestRevisionDefaulting(t *testing.T) { want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), - TimeoutSeconds: ptr.Int64(defaultTimeoutSeconds), + TimeoutSeconds: ptr.Int64(someTimeoutSeconds), ResponseStartTimeoutSeconds: ptr.Int64(50), IdleTimeoutSeconds: ptr.Int64(100), PodSpec: corev1.PodSpec{ @@ -130,14 +130,14 @@ func TestRevisionDefaulting(t *testing.T) { Name: config.DefaultsConfigName, }, Data: map[string]string{ - "revision-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), - "revision-response-start-timeout-seconds": strconv.Itoa(defaultTimeoutSeconds), + "revision-timeout-seconds": strconv.Itoa(someTimeoutSeconds), + "revision-response-start-timeout-seconds": strconv.Itoa(someTimeoutSeconds), }, }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), - TimeoutSeconds: ptr.Int64(defaultTimeoutSeconds), + TimeoutSeconds: ptr.Int64(someTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: config.DefaultUserContainerName, @@ -502,8 +502,7 @@ func TestRevisionDefaulting(t *testing.T) { "revision-cpu-limit": "400M", "revision-memory-limit": "500m", "revision-ephemeral-storage-limit": "600M", - }, - }), + }}), want: &Revision{ Spec: RevisionSpec{ TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), From be94fa39facdd793a46524497273918669d70bd3 Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Mon, 20 Jan 2025 14:44:44 +0200 Subject: [PATCH 6/7] lint --- pkg/apis/serving/v1/revision_defaults_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index 8cf0eec22f37..d2159ea3686a 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -502,7 +502,8 @@ func TestRevisionDefaulting(t *testing.T) { "revision-cpu-limit": "400M", "revision-memory-limit": "500m", "revision-ephemeral-storage-limit": "600M", - }}), + }, + }), want: &Revision{ Spec: RevisionSpec{ TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), From e644df4706fa9cd22cb43b57ba37d0f1c7d0143f Mon Sep 17 00:00:00 2001 From: Stavros Kontopoulos Date: Tue, 21 Jan 2025 17:25:25 +0200 Subject: [PATCH 7/7] address comments --- pkg/apis/serving/v1/revision_defaults_test.go | 203 ++++++++---------- 1 file changed, 92 insertions(+), 111 deletions(-) diff --git a/pkg/apis/serving/v1/revision_defaults_test.go b/pkg/apis/serving/v1/revision_defaults_test.go index d2159ea3686a..0ecc975f51a2 100644 --- a/pkg/apis/serving/v1/revision_defaults_test.go +++ b/pkg/apis/serving/v1/revision_defaults_test.go @@ -69,16 +69,14 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "with context", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "revision-timeout-seconds": strconv.Itoa(someTimeoutSeconds), - }, - }), + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "revision-timeout-seconds": strconv.Itoa(someTimeoutSeconds), + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -95,18 +93,16 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "all revision timeouts set", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "revision-timeout-seconds": strconv.Itoa(someTimeoutSeconds), - "revision-idle-timeout-seconds": "100", - "revision-response-start-timeout-seconds": "50", - }, - }), + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "revision-timeout-seconds": strconv.Itoa(someTimeoutSeconds), + "revision-idle-timeout-seconds": "100", + "revision-response-start-timeout-seconds": "50", + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -125,7 +121,7 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "Some revision timeouts set with identical values", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, corev1.ConfigMap{ + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: config.DefaultsConfigName, }, @@ -150,16 +146,14 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "with context, in create, expect ESL set", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: defaultRevisionContextWithStore(logger, apis.WithinCreate, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "revision-timeout-seconds": "323", - }, - }), + wc: configMapsToContext(logger, apis.WithinCreate, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "revision-timeout-seconds": "323", + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -182,16 +176,14 @@ func TestRevisionDefaulting(t *testing.T) { Containers: []corev1.Container{{}}, }, }}, - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-service-links": "true", - }, - }), + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-service-links": "true", + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -209,16 +201,14 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "with service links CM `true`", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: defaultRevisionContextWithStore(logger, apis.WithinCreate, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-service-links": "true", - }, - }), + wc: configMapsToContext(logger, apis.WithinCreate, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-service-links": "true", + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -236,16 +226,14 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "with service links `false`", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, - wc: defaultRevisionContextWithStore(logger, apis.WithinCreate, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-service-links": "false", - }, - }), + wc: configMapsToContext(logger, apis.WithinCreate, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-service-links": "false", + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -266,16 +254,14 @@ func TestRevisionDefaulting(t *testing.T) { EnableServiceLinks: ptr.Bool(false), Containers: []corev1.Container{{}}, }}}, - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-service-links": "true", // this should be ignored. - }, - }), + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "enable-service-links": "true", // this should be ignored. + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -329,16 +315,14 @@ func TestRevisionDefaulting(t *testing.T) { }, { name: "timeout sets to default when 0 is specified", in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}, TimeoutSeconds: ptr.Int64(0)}}, - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "revision-timeout-seconds": "456", - }, - }), + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "revision-timeout-seconds": "456", + }, + }), want: &Revision{ Spec: RevisionSpec{ ContainerConcurrency: ptr.Int64(0), @@ -489,21 +473,19 @@ func TestRevisionDefaulting(t *testing.T) { PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}, }, }, - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "revision-cpu-request": "100m", - "revision-memory-request": "200M", - "revision-ephemeral-storage-request": "300m", - "revision-cpu-limit": "400M", - "revision-memory-limit": "500m", - "revision-ephemeral-storage-limit": "600M", - }, - }), + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "revision-cpu-request": "100m", + "revision-memory-request": "200M", + "revision-ephemeral-storage-request": "300m", + "revision-cpu-limit": "400M", + "revision-memory-limit": "500m", + "revision-ephemeral-storage-limit": "600M", + }, + }), want: &Revision{ Spec: RevisionSpec{ TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds), @@ -858,12 +840,10 @@ func TestRevisionDefaulting(t *testing.T) { }, }, { name: "Default security context with feature enabled", - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.DefaultsConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}, - Data: map[string]string{"secure-pod-defaults": "Enabled"}, - }), + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}, + Data: map[string]string{"secure-pod-defaults": "Enabled"}, + }), in: &Revision{ Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ @@ -971,12 +951,10 @@ func TestRevisionDefaulting(t *testing.T) { }, }, { name: "uses pod defaults in security context", - wc: defaultRevisionContextWithStore(logger, nil, corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}, - corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.DefaultsConfigName}}, - corev1.ConfigMap{ - ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}, - Data: map[string]string{"secure-pod-defaults": "Enabled"}, - }), + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}, + Data: map[string]string{"secure-pod-defaults": "Enabled"}, + }), in: &Revision{ Spec: RevisionSpec{ PodSpec: corev1.PodSpec{ @@ -1317,9 +1295,12 @@ func TestRevisionDefaultingContainerName(t *testing.T) { } } -func defaultRevisionContextWithStore(logger *zap.SugaredLogger, ctxFunc func(ctx context.Context) context.Context, cms ...corev1.ConfigMap) func(ctx context.Context) context.Context { +func configMapsToContext(logger *zap.SugaredLogger, ctxFunc func(ctx context.Context) context.Context, cms ...corev1.ConfigMap) func(ctx context.Context) context.Context { return func(ctx context.Context) context.Context { s := config.NewStore(logger) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: autoscalerconfig.ConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.FeaturesConfigName}}) + s.OnConfigChanged(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: config.DefaultsConfigName}}) for _, cm := range cms { s.OnConfigChanged(&cm) }