Guard PodSpec dry run with a feature flag (#8276)

* add feature flag to dryrun

* Add new value to unit test

* Update default value to allowed

* update comment

* include example in the .yaml

* remove whitespace

* update checksum

* scope flag to kubernetes namespace

* typo

* fix nit

* fix nit

config/core/configmaps/features.yaml

@@ -20,7 +20,7 @@ metadata:
   labels:
     serving.knative.dev/release: devel
   annotations:
-    knative.dev/example-checksum: f9125b41
+    knative.dev/example-checksum: 3711c469
 data:
   _example: |
     ################################
@@ -41,5 +41,15 @@ data:
     # Indicates whether multi container support is enabled
     multi-container: "disabled"
 
+
     # Indicates whether Kubernetes FieldRef support is enabled
     kubernetes/field-ref: "disabled"
+
+
+    # This feature validates PodSpecs from the validating webhook
+    # against the K8s API Server.
+    #
+    # When "allowed", the client may enable the behavior with the
+    # 'features.knative.dev/podspec-dryrun':'enabled' annotation.
+    # When "enabled"  the server will always run the extra validation.
+    kubernetes/podspec-dryrun: "allowed"

pkg/apis/config/features.go

@@ -29,15 +29,20 @@ const (
 	// FeaturesConfigName is the name of the ConfigMap for the features.
 	FeaturesConfigName = "config-features"
 
-	Enabled  Flag = "Enabled"
-	Allowed  Flag = "Allowed"
+	// Enabled turns on an optional behavior.
+	Enabled Flag = "Enabled"
+	// Disabled turns off an optional behavior.
 	Disabled Flag = "Disabled"
+	// Allowed neither explicitly disables or enables a behavior.
+	// eg. allow a client to control behavior with an annotation or allow a new value through validation.
+	Allowed Flag = "Allowed"
 )
 
 func defaultFeaturesConfig() *Features {
 	return &Features{
 		MultiContainer:     Disabled,
 		KubernetesFieldRef: Disabled,
+		PodSpecDryRun:      Allowed,
 	}
 }
 
@@ -50,6 +55,9 @@ func NewFeaturesConfigFromMap(data map[string]string) (*Features, error) {
 		asFlag("kubernetes/field-ref", &nc.KubernetesFieldRef)); err != nil {
 		return nil, err
 	}
+	if err := cm.Parse(data, asFlag("kubernetes/podspec-dryrun", &nc.PodSpecDryRun)); err != nil {
+		return nil, err
+	}
 	return nc, nil
 }
 
@@ -62,6 +70,7 @@ func NewFeaturesConfigFromConfigMap(config *corev1.ConfigMap) (*Features, error)
 type Features struct {
 	MultiContainer     Flag
 	KubernetesFieldRef Flag
+	PodSpecDryRun      Flag
 }
 
 // asFlag parses the value at key as a Flag into the target, if it exists.

pkg/apis/config/features_test.go

@@ -60,18 +60,21 @@ func TestFeaturesConfiguration(t *testing.T) {
 		wantErr: false,
 		wantFeatures: defaultWith(&Features{
 			MultiContainer: Allowed,
+			PodSpecDryRun:  Allowed,
 		}),
 		data: map[string]string{
 			"multi-container": "Allowed",
 		},
 	}, {
-		name:    "multi-container Enabled",
+		name:    "features Enabled",
 		wantErr: false,
 		wantFeatures: defaultWith(&Features{
 			MultiContainer: Enabled,
+			PodSpecDryRun:  Enabled,
 		}),
 		data: map[string]string{
-			"multi-container": "Enabled",
+			"multi-container":           "Enabled",
+			"kubernetes/podspec-dryrun": "Enabled",
 		},
 	}, {
 		name:    "multi-container Disabled",

pkg/webhook/validate_service.go

@@ -25,6 +25,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime"
 	"knative.dev/pkg/apis"
 	"knative.dev/pkg/logging"
+	"knative.dev/serving/pkg/apis/config"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
@@ -46,10 +47,20 @@ const (
 func ValidateRevisionTemplate(ctx context.Context, uns *unstructured.Unstructured) error {
 	content := uns.UnstructuredContent()
 
+	var mode DryRunMode
+	features := config.FromContextOrDefaults(ctx).Features
+	switch features.PodSpecDryRun {
+	case config.Enabled:
+		mode = DryRunEnabled
+	case config.Disabled:
+		return nil
+	default:
+		mode = DryRunMode(uns.GetAnnotations()[PodSpecDryRunAnnotation])
+	}
+
 	// TODO(https://github.com/knative/serving/issues/3425): remove this guard once variations
 	// of this are well-tested. Only run extra validation for the dry-run test.
 	// This will be in place to while the feature is tested for compatibility and later removed.
-	mode := DryRunMode(uns.GetAnnotations()[PodSpecDryRunAnnotation])
 	if mode != DryRunStrict && mode != DryRunEnabled {
 		return nil
 	}

pkg/webhook/validate_service_test.go

@@ -30,6 +30,7 @@ import (
 	fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake"
 	"knative.dev/pkg/logging"
 	logtesting "knative.dev/pkg/logging/testing"
+	"knative.dev/serving/pkg/apis/config"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
 )
 
@@ -74,7 +75,7 @@ func TestServiceValidation(t *testing.T) {
 		name: "invalid structure",
 		data: map[string]interface{}{
 			"metadata": validMetadata,
-			"spec":     true, // Invalid, spec is expcted to be a struct
+			"spec":     true, // Invalid, spec is expected to be a struct
 		},
 		want: "could not traverse nested spec.template field",
 	}, {
@@ -110,6 +111,52 @@ func TestServiceValidation(t *testing.T) {
 	}
 }
 
+func TestDryRunFeatureFlag(t *testing.T) {
+	data := map[string]interface{}{
+		"metadata": map[string]interface{}{
+			"name":        "valid",
+			"namespace":   "foo",
+			"annotations": map[string]interface{}{}, // Skip validation because no test annotation
+		},
+		"spec": true, // Invalid, spec is expected to be a struct
+	}
+
+	tests := []struct {
+		name       string
+		dryRunFlag config.Flag
+		want       string
+	}{{
+		name:       "enabled dry-run",
+		dryRunFlag: config.Enabled,
+		want:       "could not traverse nested spec.template field",
+	}, {
+		name:       "disabled dry-run",
+		dryRunFlag: config.Disabled,
+		want:       "", // expect no error despite invalid data.
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ctx, _ := fakekubeclient.With(context.Background())
+			logger := logtesting.TestLogger(t)
+			ctx = logging.WithLogger(ctx, logger)
+			ctx = enableDryRun(ctx, test.dryRunFlag)
+
+			unstruct := &unstructured.Unstructured{}
+			unstruct.SetUnstructuredContent(data)
+
+			got := ValidateRevisionTemplate(ctx, unstruct)
+			if got == nil {
+				if test.want != "" {
+					t.Errorf("Validate got=nil, want=%q", test.want)
+				}
+			} else if !strings.Contains(got.Error(), test.want) {
+				t.Errorf("Validate got=%q, want=%q", got.Error(), test.want)
+			}
+		})
+	}
+}
+
 func TestSkipUpdate(t *testing.T) {
 	validService := &v1.Service{
 		ObjectMeta: metav1.ObjectMeta{
@@ -177,3 +224,11 @@ func TestSkipUpdate(t *testing.T) {
 		})
 	}
 }
+
+func enableDryRun(ctx context.Context, flag config.Flag) context.Context {
+	return config.ToContext(ctx, &config.Config{
+		Features: &config.Features{
+			PodSpecDryRun: flag,
+		},
+	})
+}