From 67c9ce8585cf6207fc39aa160713e83f3868281d Mon Sep 17 00:00:00 2001 From: Knative Prow Robot Date: Wed, 22 Jan 2025 03:27:10 +0000 Subject: [PATCH] [release-1.16] Fix configuration timeout defaulting (#15721) * Fix configuration defaulting * address comments, refactoring * refactor * lint * fixes * lint * address comments --------- Co-authored-by: Stavros Kontopoulos --- .../serving/v1/configuration_defaults_test.go | 64 +++- pkg/apis/serving/v1/revision_defaults_test.go | 295 ++++++++---------- pkg/reconciler/configuration/config/store.go | 27 +- 3 files changed, 211 insertions(+), 175 deletions(-) diff --git a/pkg/apis/serving/v1/configuration_defaults_test.go b/pkg/apis/serving/v1/configuration_defaults_test.go index ae6e8d9058e3..b12c88aca88c 100644 --- a/pkg/apis/serving/v1/configuration_defaults_test.go +++ b/pkg/apis/serving/v1/configuration_defaults_test.go @@ -18,20 +18,25 @@ package v1 import ( "context" + "strconv" "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" "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" ) +const someTimeoutSeconds = 400 + func TestConfigurationDefaulting(t *testing.T) { tests := []struct { name string @@ -156,6 +161,53 @@ 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(someTimeoutSeconds), + ContainerConcurrency: ptr.Int64(config.DefaultContainerConcurrency), + }, + }, + }, + }, + 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": strconv.Itoa(someTimeoutSeconds), + "revision-response-start-timeout-seconds": strconv.Itoa(someTimeoutSeconds), + "revision-idle-timeout-seconds": strconv.Itoa(someTimeoutSeconds), + }, + })(context.Background()), }} for _, test := range tests { @@ -328,3 +380,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 fd06263a0451..a905b5e3e129 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" + "strconv" "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,18 @@ 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{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "revision-timeout-seconds": "423", - }, - }) - - return s.ToContext(ctx) - }, + 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), - TimeoutSeconds: ptr.Int64(423), + TimeoutSeconds: ptr.Int64(someTimeoutSeconds), PodSpec: corev1.PodSpec{ Containers: []corev1.Container{{ Name: config.DefaultUserContainerName, @@ -98,26 +93,20 @@ 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{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "revision-timeout-seconds": "423", - "revision-idle-timeout-seconds": "100", - "revision-response-start-timeout-seconds": "50", - }, - }) - return s.ToContext(ctx) - }, + 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), - TimeoutSeconds: ptr.Int64(423), + TimeoutSeconds: ptr.Int64(someTimeoutSeconds), ResponseStartTimeoutSeconds: ptr.Int64(50), IdleTimeoutSeconds: ptr.Int64(100), PodSpec: corev1.PodSpec{ @@ -130,23 +119,41 @@ func TestRevisionDefaulting(t *testing.T) { }, }, }, { - name: "with context, in create, expect ESL set", + 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": "323", + wc: configMapsToContext(logger, nil, corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.DefaultsConfigName, + }, + Data: map[string]string{ + "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(someTimeoutSeconds), + PodSpec: corev1.PodSpec{ + Containers: []corev1.Container{{ + Name: config.DefaultUserContainerName, + Resources: defaultResources, + ReadinessProbe: defaultProbe, + }}, }, - }) - - return apis.WithinCreate(s.ToContext(ctx)) + }, }, + }, { + name: "with context, in create, expect ESL set", + in: &Revision{Spec: RevisionSpec{PodSpec: corev1.PodSpec{Containers: []corev1.Container{{}}}}}, + 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), @@ -169,20 +176,14 @@ 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{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-service-links": "true", - }, - }) - return s.ToContext(ctx) - }, + 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), @@ -200,20 +201,14 @@ 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{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-service-links": "true", - }, - }) - return apis.WithinCreate(s.ToContext(ctx)) - }, + 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), @@ -231,20 +226,14 @@ 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{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-service-links": "false", - }, - }) - return apis.WithinCreate(s.ToContext(ctx)) - }, + 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), @@ -265,20 +254,14 @@ 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{ - ObjectMeta: metav1.ObjectMeta{ - Name: config.DefaultsConfigName, - }, - Data: map[string]string{ - "enable-service-links": "true", // this should be ignored. - }, - }) - return s.ToContext(ctx) - }, + 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), @@ -332,21 +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: 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": "456", - }, - }) - - return s.ToContext(ctx) - }, + 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), @@ -497,26 +473,19 @@ 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{ - 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", - }, - }) - - return s.ToContext(ctx) - }, + 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), @@ -871,19 +840,10 @@ 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: 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{ @@ -982,19 +942,10 @@ 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: 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{ @@ -1334,3 +1285,19 @@ func TestRevisionDefaultingContainerName(t *testing.T) { t.Errorf("Failed to set default values for init container name") } } + +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) + } + if ctxFunc != nil { + ctx = ctxFunc(ctx) + } + return s.ToContext(ctx) + } +} diff --git a/pkg/reconciler/configuration/config/store.go b/pkg/reconciler/configuration/config/store.go index 5b602992050a..197fbd4fd6be 100644 --- a/pkg/reconciler/configuration/config/store.go +++ b/pkg/reconciler/configuration/config/store.go @@ -20,15 +20,15 @@ import ( "context" "knative.dev/pkg/configmap" - cfgmap "knative.dev/serving/pkg/apis/config" + apisconfig "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. @@ -50,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 @@ -63,7 +63,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. @@ -78,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..., ), @@ -96,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() }