diff --git a/config/core/configmaps/features.yaml b/config/core/configmaps/features.yaml index a1f73f990ba4..f10bf5305095 100644 --- a/config/core/configmaps/features.yaml +++ b/config/core/configmaps/features.yaml @@ -20,7 +20,7 @@ metadata: labels: serving.knative.dev/release: devel annotations: - knative.dev/example-checksum: "52231144" + knative.dev/example-checksum: "38e90f86" data: _example: | ################################ @@ -41,6 +41,15 @@ data: # Indicates whether multi container support is enabled multi-container: "disabled" + # Indicates whether Kubernetes affinity support is enabled + kubernetes.podspec-affinity: "disabled" + + # Indicates whether Kubernetes nodeSelector support is enabled + kubernetes.podspec-nodeselector: "disabled" + + # Indicates whether Kubernetes tolerations support is enabled + kubernetes.podspec-tolerations: "disabled" + # Indicates whether Kubernetes FieldRef support is enabled kubernetes.podspec-fieldref: "disabled" diff --git a/pkg/apis/config/features.go b/pkg/apis/config/features.go index 6ee4c34ba716..c2f471272b33 100644 --- a/pkg/apis/config/features.go +++ b/pkg/apis/config/features.go @@ -41,8 +41,11 @@ const ( func defaultFeaturesConfig() *Features { return &Features{ MultiContainer: Disabled, + PodSpecAffinity: Disabled, PodSpecFieldRef: Disabled, PodSpecDryRun: Enabled, + PodSpecNodeSelector: Disabled, + PodSpecTolerations: Disabled, ResponsiveRevisionGC: Disabled, } } @@ -53,8 +56,11 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) { if err := cm.Parse(data, asFlag("multi-container", &nc.MultiContainer), + asFlag("kubernetes.podspec-affinity", &nc.PodSpecAffinity), asFlag("kubernetes.podspec-fieldref", &nc.PodSpecFieldRef), asFlag("kubernetes.podspec-dryrun", &nc.PodSpecDryRun), + asFlag("kubernetes.podspec-nodeselector", &nc.PodSpecNodeSelector), + asFlag("kubernetes.podspec-tolerations", &nc.PodSpecTolerations), asFlag("responsive-revision-gc", &nc.ResponsiveRevisionGC)); err != nil { return nil, err } @@ -69,8 +75,11 @@ func NewFeaturesConfigFromConfigMap(config *corev1.ConfigMap) (*Features, error) // Features specifies which features are allowed by the webhook. type Features struct { MultiContainer Flag + PodSpecAffinity Flag PodSpecFieldRef Flag PodSpecDryRun Flag + PodSpecNodeSelector Flag + PodSpecTolerations Flag ResponsiveRevisionGC Flag } diff --git a/pkg/apis/config/features_test.go b/pkg/apis/config/features_test.go index 23686fbc39b8..64e9bbfa3097 100644 --- a/pkg/apis/config/features_test.go +++ b/pkg/apis/config/features_test.go @@ -60,13 +60,19 @@ func TestFeaturesConfiguration(t *testing.T) { wantErr: false, wantFeatures: defaultWith(&Features{ MultiContainer: Enabled, + PodSpecAffinity: Enabled, PodSpecDryRun: Enabled, + PodSpecNodeSelector: Enabled, + PodSpecTolerations: Enabled, ResponsiveRevisionGC: Enabled, }), data: map[string]string{ - "multi-container": "Enabled", - "kubernetes.podspec-dryrun": "Enabled", - "responsive-revision-gc": "Enabled", + "multi-container": "Enabled", + "kubernetes.podspec-affinity": "Enabled", + "kubernetes.podspec-dryrun": "Enabled", + "kubernetes.podspec-nodeselector": "Enabled", + "kubernetes.podspec-tolerations": "Enabled", + "responsive-revision-gc": "Enabled", }, }, { name: "multi-container Allowed", @@ -86,6 +92,33 @@ func TestFeaturesConfiguration(t *testing.T) { data: map[string]string{ "multi-container": "Disabled", }, + }, { + name: "kubernetes.podspec-affinity Allowed", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecAffinity: Allowed, + }), + data: map[string]string{ + "kubernetes.podspec-affinity": "Allowed", + }, + }, { + name: "kubernetes.podspec-affinity Enabled", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecAffinity: Enabled, + }), + data: map[string]string{ + "kubernetes.podspec-affinity": "Enabled", + }, + }, { + name: "kubernetes.podspec-affinity Disabled", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecAffinity: Disabled, + }), + data: map[string]string{ + "kubernetes.podspec-affinity": "Disabled", + }, }, { name: "kubernetes.podspec-fieldref Allowed", wantErr: false, @@ -122,6 +155,60 @@ func TestFeaturesConfiguration(t *testing.T) { data: map[string]string{ "kubernetes.podspec-dryrun": "Disabled", }, + }, { + name: "kubernetes.podspec-nodeselector Allowed", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecNodeSelector: Allowed, + }), + data: map[string]string{ + "kubernetes.podspec-nodeselector": "Allowed", + }, + }, { + name: "kubernetes.podspec-nodeselector Enabled", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecNodeSelector: Enabled, + }), + data: map[string]string{ + "kubernetes.podspec-nodeselector": "Enabled", + }, + }, { + name: "kubernetes.podspec-nodeselector Disabled", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecNodeSelector: Disabled, + }), + data: map[string]string{ + "kubernetes.podspec-nodeselector": "Disabled", + }, + }, { + name: "kubernetes.podspec-tolerations Allowed", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecTolerations: Allowed, + }), + data: map[string]string{ + "kubernetes.podspec-tolerations": "Allowed", + }, + }, { + name: "kubernetes.podspec-tolerations Enabled", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecTolerations: Enabled, + }), + data: map[string]string{ + "kubernetes.podspec-tolerations": "Enabled", + }, + }, { + name: "kubernetes.podspec-tolerations Disabled", + wantErr: false, + wantFeatures: defaultWith(&Features{ + PodSpecTolerations: Disabled, + }), + data: map[string]string{ + "kubernetes.podspec-tolerations": "Disabled", + }, }, { name: "responsive-revision-gc Allowed", wantErr: false, diff --git a/pkg/apis/config/store.go b/pkg/apis/config/store.go index 5288d0b050af..29e2afab11b6 100644 --- a/pkg/apis/config/store.go +++ b/pkg/apis/config/store.go @@ -45,17 +45,23 @@ func FromContext(ctx context.Context) *Config { // FromContextOrDefaults is like FromContext, but when no Config is attached it // returns a Config populated with the defaults for each of the Config fields. func FromContextOrDefaults(ctx context.Context) *Config { - if cfg := FromContext(ctx); cfg != nil { - return cfg + cfg := FromContext(ctx) + if cfg == nil { + cfg = &Config{} } - defaults, _ := NewDefaultsConfigFromMap(map[string]string{}) - features, _ := NewFeaturesConfigFromMap(map[string]string{}) - autoscaler, _ := autoscalerconfig.NewConfigFromMap(map[string]string{}) - return &Config{ - Defaults: defaults, - Features: features, - Autoscaler: autoscaler, + + if cfg.Defaults == nil { + cfg.Defaults, _ = NewDefaultsConfigFromMap(map[string]string{}) + } + + if cfg.Features == nil { + cfg.Features, _ = NewFeaturesConfigFromMap(map[string]string{}) + } + + if cfg.Autoscaler == nil { + cfg.Autoscaler, _ = autoscalerconfig.NewConfigFromMap(map[string]string{}) } + return cfg } // ToContext attaches the provided Config to the provided context, returning the diff --git a/pkg/apis/serving/fieldmask.go b/pkg/apis/serving/fieldmask.go index b45f13bcaf27..ee7ba786d15d 100644 --- a/pkg/apis/serving/fieldmask.go +++ b/pkg/apis/serving/fieldmask.go @@ -17,7 +17,10 @@ limitations under the License. package serving import ( + "context" + corev1 "k8s.io/api/core/v1" + "knative.dev/serving/pkg/apis/config" ) // VolumeMask performs a _shallow_ copy of the Kubernetes Volume object to a new @@ -136,11 +139,12 @@ func KeyToPathMask(in *corev1.KeyToPath) *corev1.KeyToPath { // PodSpecMask performs a _shallow_ copy of the Kubernetes PodSpec object to a new // Kubernetes PodSpec object bringing over only the fields allowed in the Knative API. This // does not validate the contents or the bounds of the provided fields. -func PodSpecMask(in *corev1.PodSpec) *corev1.PodSpec { +func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec { if in == nil { return nil } + cfg := config.FromContextOrDefaults(ctx) out := new(corev1.PodSpec) // Allowed fields @@ -150,6 +154,17 @@ func PodSpecMask(in *corev1.PodSpec) *corev1.PodSpec { out.ImagePullSecrets = in.ImagePullSecrets out.EnableServiceLinks = in.EnableServiceLinks + // Feature fields + if cfg.Features.PodSpecAffinity != config.Disabled { + out.Affinity = in.Affinity + } + if cfg.Features.PodSpecNodeSelector != config.Disabled { + out.NodeSelector = in.NodeSelector + } + if cfg.Features.PodSpecTolerations != config.Disabled { + out.Tolerations = in.Tolerations + } + // Disallowed fields // This list is unnecessary, but added here for clarity out.InitContainers = nil @@ -157,7 +172,6 @@ func PodSpecMask(in *corev1.PodSpec) *corev1.PodSpec { out.TerminationGracePeriodSeconds = nil out.ActiveDeadlineSeconds = nil out.DNSPolicy = "" - out.NodeSelector = nil out.AutomountServiceAccountToken = nil out.NodeName = "" out.HostNetwork = false @@ -167,9 +181,7 @@ func PodSpecMask(in *corev1.PodSpec) *corev1.PodSpec { out.SecurityContext = nil out.Hostname = "" out.Subdomain = "" - out.Affinity = nil out.SchedulerName = "" - out.Tolerations = nil out.HostAliases = nil out.PriorityClassName = "" out.Priority = nil diff --git a/pkg/apis/serving/fieldmask_test.go b/pkg/apis/serving/fieldmask_test.go index b0345ef0c8f7..897f7e07b5ac 100644 --- a/pkg/apis/serving/fieldmask_test.go +++ b/pkg/apis/serving/fieldmask_test.go @@ -17,6 +17,7 @@ limitations under the License. package serving import ( + "context" "testing" corev1 "k8s.io/api/core/v1" @@ -117,7 +118,8 @@ func TestPodSpecMask(t *testing.T) { }}, } - got := PodSpecMask(in) + ctx := context.Background() + got := PodSpecMask(ctx, in) if &want == &got { t.Error("Input and output share addresses. Want different addresses") @@ -129,7 +131,7 @@ func TestPodSpecMask(t *testing.T) { t.Errorf("PodSpecMask (-want, +got): %s", diff) } - if got = PodSpecMask(nil); got != nil { + if got = PodSpecMask(ctx, nil); got != nil { t.Errorf("PodSpecMask(nil) = %v, want: nil", got) } } diff --git a/pkg/apis/serving/k8s_validation.go b/pkg/apis/serving/k8s_validation.go index 0791e33789f1..f257b3a347a7 100644 --- a/pkg/apis/serving/k8s_validation.go +++ b/pkg/apis/serving/k8s_validation.go @@ -263,7 +263,7 @@ func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError { // return apis.ErrMissingField(apis.CurrentField) // } - errs := apis.CheckDisallowedFields(ps, *PodSpecMask(&ps)) + errs := apis.CheckDisallowedFields(ps, *PodSpecMask(ctx, &ps)) volumes, err := ValidateVolumes(ps.Volumes, AllMountedVolumes(ps.Containers)) if err != nil { diff --git a/pkg/apis/serving/k8s_validation_test.go b/pkg/apis/serving/k8s_validation_test.go index f2fe9fff1a39..99b41885bee3 100644 --- a/pkg/apis/serving/k8s_validation_test.go +++ b/pkg/apis/serving/k8s_validation_test.go @@ -34,20 +34,41 @@ import ( type configOption func(*config.Config) *config.Config -func withMultiContainer() configOption { +func withMultiContainerEnabled() configOption { return func(cfg *config.Config) *config.Config { cfg.Features.MultiContainer = config.Enabled return cfg } } -func withFieldRef() configOption { +func withPodSpecFieldRefEnabled() configOption { return func(cfg *config.Config) *config.Config { cfg.Features.PodSpecFieldRef = config.Enabled return cfg } } +func withPodSpecAffinityEnabled() configOption { + return func(cfg *config.Config) *config.Config { + cfg.Features.PodSpecAffinity = config.Enabled + return cfg + } +} + +func withPodSpecNodeSelectorEnabled() configOption { + return func(cfg *config.Config) *config.Config { + cfg.Features.PodSpecNodeSelector = config.Enabled + return cfg + } +} + +func withPodSpecTolerationsEnabled() configOption { + return func(cfg *config.Config) *config.Config { + cfg.Features.PodSpecTolerations = config.Enabled + return cfg + } +} + func TestPodSpecValidation(t *testing.T) { tests := []struct { name string @@ -240,7 +261,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { Image: "helloworld", }}, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: nil, }, { name: "flag enabled: probes are not allowed for non serving containers", @@ -260,7 +281,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { }, }}, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: &apis.FieldError{ Message: "must not set the field(s)", Paths: []string{"containers[1].livenessProbe.timeoutSeconds", "containers[1].readinessProbe.timeoutSeconds"}, @@ -274,7 +295,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { Image: "helloworld", }}, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: apis.ErrMissingField("containers.ports"), }, { name: "flag enabled: multiple containers with multiple port", @@ -291,7 +312,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { }}, }}, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: apis.ErrMultipleOneOf("containers.ports"), }, { name: "flag enabled: multiple containers with multiple ports for each container", @@ -310,7 +331,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { }}, }}, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ Message: "More than one container port is set", Paths: []string{"containers[0].ports"}, @@ -330,7 +351,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { Image: "helloworld", }}, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: apis.ErrMultipleOneOf("containers.ports").Also(&apis.FieldError{ Message: "More than one container port is set", Paths: []string{"containers[0].ports"}, @@ -355,7 +376,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { }}, }}, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: &apis.FieldError{ Message: `"K_SERVICE" is a reserved environment variable`, Paths: []string{"containers[1].env[1].name"}, @@ -376,7 +397,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { }}, }}, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: nil, }, { name: "Volume mounts ok with single container", @@ -398,7 +419,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { }}, }}, }, - cfgOpts: []configOption{withFieldRef()}, + cfgOpts: []configOption{withPodSpecFieldRefEnabled()}, }, { name: "Volume not mounted when having a single container", ps: corev1.PodSpec{ @@ -414,7 +435,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { Image: "busybox", }}, }, - cfgOpts: []configOption{withFieldRef()}, + cfgOpts: []configOption{withPodSpecFieldRefEnabled()}, want: &apis.FieldError{ Message: "volume with name \"the-name\" not mounted", Paths: []string{"volumes[0].name"}}, @@ -456,7 +477,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { }, }, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, }, { name: "Volume not mounted when having multiple containers", ps: corev1.PodSpec{ @@ -488,7 +509,7 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { {Image: "busybox"}, }, }, - cfgOpts: []configOption{withMultiContainer()}, + cfgOpts: []configOption{withMultiContainerEnabled()}, want: &apis.FieldError{ Message: "volume with name \"the-name2\" not mounted", Paths: []string{"volumes[1].name"}, @@ -513,6 +534,124 @@ func TestPodSpecMultiContainerValidation(t *testing.T) { } } +func TestPodSpecFeatureValidation(t *testing.T) { + featureData := []struct { + name string + featureSpec corev1.PodSpec + cfgOpts []configOption + err *apis.FieldError + }{{ + name: "Affinity", + featureSpec: corev1.PodSpec{ + Affinity: &corev1.Affinity{ + NodeAffinity: &corev1.NodeAffinity{ + RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{ + NodeSelectorTerms: []corev1.NodeSelectorTerm{{ + MatchExpressions: []corev1.NodeSelectorRequirement{{ + Key: "failure-domain.beta.kubernetes.io/zone", + Operator: "In", + Values: []string{"us-east1-b"}, + }}, + }}, + }, + }, + }, + }, + err: &apis.FieldError{ + Message: "must not set the field(s)", + Paths: []string{"affinity"}, + }, + cfgOpts: []configOption{withPodSpecAffinityEnabled()}, + }, { + name: "NodeSelector", + featureSpec: corev1.PodSpec{ + NodeSelector: map[string]string{ + "kubernetes.io/arch": "amd64", + }, + }, + err: &apis.FieldError{ + Message: "must not set the field(s)", + Paths: []string{"nodeSelector"}, + }, + cfgOpts: []configOption{withPodSpecNodeSelectorEnabled()}, + }, { + name: "Tolerations", + featureSpec: corev1.PodSpec{ + Tolerations: []corev1.Toleration{{ + Key: "key", + Operator: "Equal", + Value: "value", + Effect: "NoSchedule", + }}, + }, + err: &apis.FieldError{ + Message: "must not set the field(s)", + Paths: []string{"tolerations"}, + }, + cfgOpts: []configOption{withPodSpecTolerationsEnabled()}, + }} + + featureTests := []struct { + nameTemplate string + enableFeature bool + includeFeatureSpec bool + wantError bool + }{{ + nameTemplate: "flag disabled: %s not present", + enableFeature: false, + includeFeatureSpec: false, + wantError: false, + }, { + nameTemplate: "flag disabled: %s present", + enableFeature: false, + includeFeatureSpec: true, + wantError: true, + }, { + nameTemplate: "flag enabled: %s not present", + enableFeature: true, + includeFeatureSpec: false, + wantError: false, + }, { + nameTemplate: "flag enabled: %s present", + enableFeature: true, + includeFeatureSpec: true, + wantError: false, + }} + + for _, featureData := range featureData { + for _, test := range featureTests { + t.Run(fmt.Sprintf(test.nameTemplate, featureData.name), func(t *testing.T) { + ctx := context.Background() + obj := corev1.PodSpec{ + Containers: []corev1.Container{{ + Image: "busybox", + }}, + } + want := &apis.FieldError{} + if test.wantError { + want = featureData.err + } + if test.enableFeature { + cfg := config.FromContextOrDefaults(ctx) + for _, opt := range featureData.cfgOpts { + cfg = opt(cfg) + } + ctx = config.ToContext(ctx, cfg) + } + if test.includeFeatureSpec { + obj.Affinity = featureData.featureSpec.Affinity + obj.NodeSelector = featureData.featureSpec.NodeSelector + obj.Tolerations = featureData.featureSpec.Tolerations + } + got := ValidatePodSpec(ctx, obj) + if diff := cmp.Diff(want.Error(), got.Error()); diff != "" { + t.Errorf("ValidatePodSpec (-want, +got): \n%s", diff) + } + }) + } + } +} + func TestPodSpecFieldRefValidation(t *testing.T) { tests := []struct { name string @@ -572,7 +711,7 @@ func TestPodSpecFieldRefValidation(t *testing.T) { Image: "busybox", }}, }, - cfgOpts: []configOption{withFieldRef()}, + cfgOpts: []configOption{withPodSpecFieldRefEnabled()}, }, { name: "flag enabled: fieldRef present", ps: corev1.PodSpec{ @@ -588,7 +727,7 @@ func TestPodSpecFieldRefValidation(t *testing.T) { }}, }}, }, - cfgOpts: []configOption{withFieldRef()}, + cfgOpts: []configOption{withPodSpecFieldRefEnabled()}, }, { name: "flag enabled: resourceFieldRef present", ps: corev1.PodSpec{ @@ -605,7 +744,7 @@ func TestPodSpecFieldRefValidation(t *testing.T) { }}, }}, }, - cfgOpts: []configOption{withFieldRef()}, + cfgOpts: []configOption{withPodSpecFieldRefEnabled()}, }} for _, test := range tests {