Don't validate autoscaling annotations where they're not allowed (#10847)

Author: julz

Diff for: pkg/apis/autoscaling/v1alpha1/metric_validation.go

@@ -26,7 +26,7 @@ import (
 
 // Validate validates the entire Metric.
 func (m *Metric) Validate(ctx context.Context) *apis.FieldError {
-	return serving.ValidateObjectMetadata(ctx, m.GetObjectMeta()).ViaField("metadata").
+	return serving.ValidateObjectMetadata(ctx, m.GetObjectMeta(), true).ViaField("metadata").
 		Also(m.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
 }
 

Diff for: pkg/apis/autoscaling/v1alpha1/pa_validation.go

@@ -26,7 +26,7 @@ import (
 
 // Validate implements apis.Validatable interface.
 func (pa *PodAutoscaler) Validate(ctx context.Context) *apis.FieldError {
-	return serving.ValidateObjectMetadata(ctx, pa.GetObjectMeta()).ViaField("metadata").
+	return serving.ValidateObjectMetadata(ctx, pa.GetObjectMeta(), true).ViaField("metadata").
 		Also(pa.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
 }
 

Diff for: pkg/apis/serving/metadata_validation.go

@@ -30,12 +30,21 @@ import (
 	"knative.dev/serving/pkg/apis/config"
 )
 
-// ValidateObjectMetadata validates that `metadata` stanza of the
+// ValidateObjectMetadata validates that the `metadata` stanza of the
 // resources is correct.
-func ValidateObjectMetadata(ctx context.Context, meta metav1.Object) *apis.FieldError {
-	return apis.ValidateObjectMetadata(meta).
-		Also(autoscaling.ValidateAnnotations(ctx, config.FromContextOrDefaults(ctx).Autoscaler, meta.GetAnnotations()).
-			ViaField("annotations"))
+// If `allowAutoscalingAnnotations` is true autoscaling annotations, if
+// present, are validated. If `allowAutoscalingAnnotations` is false
+// autoscaling annotations are validated not to be present.
+func ValidateObjectMetadata(ctx context.Context, meta metav1.Object, allowAutoscalingAnnotations bool) *apis.FieldError {
+	errs := apis.ValidateObjectMetadata(meta)
+
+	if allowAutoscalingAnnotations {
+		errs = errs.Also(autoscaling.ValidateAnnotations(ctx, config.FromContextOrDefaults(ctx).Autoscaler, meta.GetAnnotations()).ViaField("annotations"))
+	} else {
+		errs = errs.Also(ValidateHasNoAutoscalingAnnotation(meta.GetAnnotations()).ViaField("annotations"))
+	}
+
+	return errs
 }
 
 // ValidateRolloutDurationAnnotation validates the rollout duration annotation.

Diff for: pkg/apis/serving/metadata_validation_test.go

@@ -35,10 +35,11 @@ import (
 
 func TestValidateObjectMetadata(t *testing.T) {
 	cases := []struct {
-		name       string
-		objectMeta metav1.Object
-		ctx        context.Context
-		expectErr  *apis.FieldError
+		name             string
+		objectMeta       metav1.Object
+		allowAutoscaling bool
+		ctx              context.Context
+		expectErr        *apis.FieldError
 	}{{
 		name: "invalid name - dots",
 		objectMeta: &metav1.ObjectMeta{
@@ -150,8 +151,9 @@ func TestValidateObjectMetadata(t *testing.T) {
 			},
 		},
 	}, {
-		name: "revision initial scale not parseable",
-		ctx:  config.ToContext(context.Background(), &config.Config{Autoscaler: &autoscalerconfig.Config{AllowZeroInitialScale: true}}),
+		name:             "revision initial scale not parseable",
+		ctx:              config.ToContext(context.Background(), &config.Config{Autoscaler: &autoscalerconfig.Config{AllowZeroInitialScale: true}}),
+		allowAutoscaling: true,
 		objectMeta: &metav1.ObjectMeta{
 			GenerateName: "some-name",
 			Annotations: map[string]string{
@@ -160,8 +162,9 @@ func TestValidateObjectMetadata(t *testing.T) {
 		},
 		expectErr: apis.ErrInvalidValue("invalid", "annotations."+autoscaling.InitialScaleAnnotationKey),
 	}, {
-		name: "negative revision initial scale",
-		ctx:  config.ToContext(context.Background(), &config.Config{Autoscaler: &autoscalerconfig.Config{AllowZeroInitialScale: true}}),
+		name:             "negative revision initial scale",
+		ctx:              config.ToContext(context.Background(), &config.Config{Autoscaler: &autoscalerconfig.Config{AllowZeroInitialScale: true}}),
+		allowAutoscaling: true,
 		objectMeta: &metav1.ObjectMeta{
 			GenerateName: "some-name",
 			Annotations: map[string]string{
@@ -170,31 +173,43 @@ func TestValidateObjectMetadata(t *testing.T) {
 		},
 		expectErr: apis.ErrInvalidValue("-2", "annotations."+autoscaling.InitialScaleAnnotationKey),
 	}, {
-		name: "cluster allows zero revision initial scale",
-		ctx:  config.ToContext(context.Background(), &config.Config{Autoscaler: &autoscalerconfig.Config{AllowZeroInitialScale: true}}),
+		name:             "cluster allows zero revision initial scale",
+		ctx:              config.ToContext(context.Background(), &config.Config{Autoscaler: &autoscalerconfig.Config{AllowZeroInitialScale: true}}),
+		allowAutoscaling: true,
 		objectMeta: &metav1.ObjectMeta{
 			GenerateName: "some-name",
 			Annotations: map[string]string{
 				autoscaling.InitialScaleAnnotationKey: "0",
 			},
 		},
 	}, {
-		name: "cluster does not allows zero revision initial scale",
+		name:             "cluster does not allow zero revision initial scale",
+		allowAutoscaling: true,
 		objectMeta: &metav1.ObjectMeta{
 			GenerateName: "some-name",
 			Annotations: map[string]string{
 				autoscaling.InitialScaleAnnotationKey: "0",
 			},
 		},
 		expectErr: apis.ErrInvalidValue("0", "annotations."+autoscaling.InitialScaleAnnotationKey),
+	}, {
+		name:             "autoscaling annotations on a resource that doesn't allow them",
+		allowAutoscaling: false,
+		objectMeta: &metav1.ObjectMeta{
+			GenerateName: "some-name",
+			Annotations: map[string]string{
+				autoscaling.InitialScaleAnnotationKey: "0",
+			},
+		},
+		expectErr: apis.ErrInvalidKeyName(autoscaling.InitialScaleAnnotationKey, "annotations", `autoscaling annotations must be put under "spec.template.metadata.annotations" to work`),
 	}}
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
 			if c.ctx == nil {
 				c.ctx = config.ToContext(context.Background(), &config.Config{Autoscaler: &autoscalerconfig.Config{AllowZeroInitialScale: false}})
 			}
-			err := ValidateObjectMetadata(c.ctx, c.objectMeta)
+			err := ValidateObjectMetadata(c.ctx, c.objectMeta, c.allowAutoscaling)
 			if got, want := err.Error(), c.expectErr.Error(); got != want {
 				t.Errorf("\nGot:  %q\nwant: %q", got, want)
 			}

Diff for: pkg/apis/serving/v1/configuration_validation.go

@@ -31,9 +31,8 @@ func (c *Configuration) Validate(ctx context.Context) (errs *apis.FieldError) {
 	// have changed (i.e. due to config-defaults changes), we elide the metadata and
 	// spec validation.
 	if !apis.IsInStatusUpdate(ctx) {
-		errs = errs.Also(serving.ValidateObjectMetadata(ctx, c.GetObjectMeta()))
+		errs = errs.Also(serving.ValidateObjectMetadata(ctx, c.GetObjectMeta(), false))
 		errs = errs.Also(c.validateLabels().ViaField("labels"))
-		errs = errs.Also(serving.ValidateHasNoAutoscalingAnnotation(c.GetAnnotations()).ViaField("annotations"))
 		errs = errs.ViaField("metadata")
 
 		ctx = apis.WithinParent(ctx, c.ObjectMeta)

Diff for: pkg/apis/serving/v1/revision_validation.go

@@ -32,7 +32,7 @@ import (
 
 // Validate ensures Revision is properly configured.
 func (r *Revision) Validate(ctx context.Context) *apis.FieldError {
-	errs := serving.ValidateObjectMetadata(ctx, r.GetObjectMeta()).Also(
+	errs := serving.ValidateObjectMetadata(ctx, r.GetObjectMeta(), true).Also(
 		r.ValidateLabels().ViaField("labels")).ViaField("metadata")
 	errs = errs.Also(r.Status.Validate(apis.WithinStatus(ctx)).ViaField("status"))
 

Diff for: pkg/apis/serving/v1/route_validation.go

@@ -28,7 +28,7 @@ import (
 
 // Validate makes sure that Route is properly configured.
 func (r *Route) Validate(ctx context.Context) *apis.FieldError {
-	errs := serving.ValidateObjectMetadata(ctx, r.GetObjectMeta()).Also(
+	errs := serving.ValidateObjectMetadata(ctx, r.GetObjectMeta(), false).Also(
 		r.validateLabels().ViaField("labels"))
 	errs = errs.Also(serving.ValidateRolloutDurationAnnotation(
 		r.GetAnnotations()).ViaField("annotations"))

Diff for: pkg/apis/serving/v1/service_validation.go

@@ -31,12 +31,10 @@ func (s *Service) Validate(ctx context.Context) (errs *apis.FieldError) {
 	// have changed (i.e. due to config-defaults changes), we elide the metadata and
 	// spec validation.
 	if !apis.IsInStatusUpdate(ctx) {
-		errs = errs.Also(serving.ValidateObjectMetadata(ctx, s.GetObjectMeta()))
+		errs = errs.Also(serving.ValidateObjectMetadata(ctx, s.GetObjectMeta(), false))
 		errs = errs.Also(s.validateLabels().ViaField("labels"))
 		errs = errs.Also(serving.ValidateRolloutDurationAnnotation(
 			s.GetAnnotations()).ViaField("annotations"))
-		errs = errs.Also(serving.ValidateHasNoAutoscalingAnnotation(
-			s.GetAnnotations()).ViaField("annotations"))
 		errs = errs.ViaField("metadata")
 
 		ctx = apis.WithinParent(ctx, s.ObjectMeta)