Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add feature flags for affinity/nodeSelector/tolerations #8645

Merged
merged 1 commit into from
Jul 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion config/core/configmaps/features.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ metadata:
labels:
serving.knative.dev/release: devel
annotations:
knative.dev/example-checksum: "52231144"
knative.dev/example-checksum: "38e90f86"
data:
_example: |
################################
Expand All @@ -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"
Expand Down
9 changes: 9 additions & 0 deletions pkg/apis/config/features.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,11 @@ const (
func defaultFeaturesConfig() *Features {
return &Features{
MultiContainer: Disabled,
PodSpecAffinity: Disabled,
PodSpecFieldRef: Disabled,
PodSpecDryRun: Enabled,
PodSpecNodeSelector: Disabled,
PodSpecTolerations: Disabled,
ResponsiveRevisionGC: Disabled,
}
}
Expand All @@ -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
}
Expand All @@ -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
}

Expand Down
93 changes: 90 additions & 3 deletions pkg/apis/config/features_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
24 changes: 15 additions & 9 deletions pkg/apis/config/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 16 additions & 4 deletions pkg/apis/serving/fieldmask.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -150,14 +154,24 @@ 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
out.RestartPolicy = ""
out.TerminationGracePeriodSeconds = nil
out.ActiveDeadlineSeconds = nil
out.DNSPolicy = ""
out.NodeSelector = nil
out.AutomountServiceAccountToken = nil
out.NodeName = ""
out.HostNetwork = false
Expand All @@ -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
Expand Down
6 changes: 4 additions & 2 deletions pkg/apis/serving/fieldmask_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package serving

import (
"context"
"testing"

corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -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")
Expand All @@ -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)
}
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/serving/k8s_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading