Prevent MaxScaleLimit being exceeded on update (#11010)

knative · Mar 24, 2021 · 1a0d326 · 1a0d326
1 parent 4682e3e
commit 1a0d326
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 36 deletions.
diff --git a/pkg/apis/autoscaling/annotation_validation.go b/pkg/apis/autoscaling/annotation_validation.go
@@ -52,7 +52,7 @@ func getIntGE0(m map[string]string, k string) (int32, *apis.FieldError) {
 // ValidateAnnotations verifies the autoscaling annotations.
 func ValidateAnnotations(ctx context.Context, config *autoscalerconfig.Config, anns map[string]string) *apis.FieldError {
 	return validateClass(anns).
-		Also(validateMinMaxScale(ctx, config, anns)).
+		Also(validateMinMaxScale(config, anns)).
 		Also(validateFloats(anns)).
 		Also(validateWindow(anns)).
 		Also(validateLastPodRetention(anns)).
@@ -173,7 +173,7 @@ func validateWindow(annotations map[string]string) *apis.FieldError {
 	return nil
 }
 
-func validateMinMaxScale(ctx context.Context, config *autoscalerconfig.Config, annotations map[string]string) *apis.FieldError {
+func validateMinMaxScale(config *autoscalerconfig.Config, annotations map[string]string) *apis.FieldError {
 	min, errs := getIntGE0(annotations, MinScaleAnnotationKey)
 	max, err := getIntGE0(annotations, MaxScaleAnnotationKey)
 	errs = errs.Also(err)
@@ -184,21 +184,30 @@ func validateMinMaxScale(ctx context.Context, config *autoscalerconfig.Config, a
 			Paths:   []string{MaxScaleAnnotationKey, MinScaleAnnotationKey},
 		})
 	}
-	_, exists := annotations[MaxScaleAnnotationKey]
-	// If max scale annotation is not set but default MaxScale is set, then max scale will not be unlimited.
-	if !apis.IsInCreate(ctx) || config.MaxScaleLimit == 0 || (!exists && config.MaxScale > 0) {
-		return errs
+
+	if _, hasMaxScaleAnnotation := annotations[MaxScaleAnnotationKey]; hasMaxScaleAnnotation {
+		errs = errs.Also(validateMaxScaleWithinLimit(max, config.MaxScaleLimit))
+	}
+
+	return errs
+}
+
+func validateMaxScaleWithinLimit(maxScale, maxScaleLimit int32) (errs *apis.FieldError) {
+	if maxScaleLimit == 0 {
+		return nil
 	}
-	if max > config.MaxScaleLimit {
-		errs = errs.Also(apis.ErrOutOfBoundsValue(
-			max, 1, config.MaxScaleLimit, MaxScaleAnnotationKey))
+
+	if maxScale > maxScaleLimit {
+		errs = errs.Also(apis.ErrOutOfBoundsValue(maxScale, 1, maxScaleLimit, MaxScaleAnnotationKey))
 	}
-	if max == 0 {
+
+	if maxScale == 0 {
 		errs = errs.Also(&apis.FieldError{
-			Message: fmt.Sprint("maxScale=0 (unlimited), must be less than ", config.MaxScaleLimit),
+			Message: fmt.Sprint("maxScale=0 (unlimited), must be less than ", maxScaleLimit),
 			Paths:   []string{MaxScaleAnnotationKey},
 		})
 	}
+
 	return errs
 }
 

diff --git a/pkg/apis/autoscaling/annotation_validation_test.go b/pkg/apis/autoscaling/annotation_validation_test.go
@@ -24,7 +24,6 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 
-	"knative.dev/pkg/apis"
 	"knative.dev/serving/pkg/autoscaler/config/autoscalerconfig"
 )
 
@@ -34,7 +33,6 @@ func TestValidateAnnotations(t *testing.T) {
 		annotations   map[string]string
 		expectErr     string
 		configMutator func(*autoscalerconfig.Config)
-		isInCreate    bool
 	}{{
 		name:        "nil annotations",
 		annotations: nil,
@@ -102,7 +100,6 @@ func TestValidateAnnotations(t *testing.T) {
 		configMutator: func(config *autoscalerconfig.Config) {
 			config.MaxScaleLimit = 10
 		},
-		isInCreate:  true,
 		annotations: map[string]string{MaxScaleAnnotationKey: "11"},
 		expectErr:   "expected 1 <= 11 <= 10: " + MaxScaleAnnotationKey,
 	}, {
@@ -111,23 +108,14 @@ func TestValidateAnnotations(t *testing.T) {
 			config.MaxScaleLimit = 10
 			config.MaxScale = 11
 		},
-		isInCreate:  true,
 		annotations: map[string]string{MaxScaleAnnotationKey: "20"},
 		expectErr:   "expected 1 <= 20 <= 10: " + MaxScaleAnnotationKey,
-	}, {
-		name: "maxScale is greater than MaxScaleLimit when not in create",
-		configMutator: func(config *autoscalerconfig.Config) {
-			config.MaxScaleLimit = 10
-		},
-		isInCreate:  false,
-		annotations: map[string]string{MaxScaleAnnotationKey: "11"},
 	}, {
 		name: "maxScale is explicitly set to 0 when MaxScaleLimit and default MaxScale are set",
 		configMutator: func(config *autoscalerconfig.Config) {
 			config.MaxScaleLimit = 10
-			config.MaxScale = 11
+			config.MaxScale = 1
 		},
-		isInCreate:  true,
 		annotations: map[string]string{MaxScaleAnnotationKey: "0"},
 		expectErr:   "maxScale=0 (unlimited), must be less than 10: " + MaxScaleAnnotationKey,
 	}, {
@@ -136,20 +124,11 @@ func TestValidateAnnotations(t *testing.T) {
 			config.MaxScaleLimit = 10
 			config.MaxScale = 11
 		},
-		isInCreate: true,
-	}, {
-		name: "neither maxScale nor default MaxScale is set when MaxScaleLimit is set",
-		configMutator: func(config *autoscalerconfig.Config) {
-			config.MaxScaleLimit = 10
-		},
-		isInCreate: true,
-		expectErr:  "maxScale=0 (unlimited), must be less than 10: " + MaxScaleAnnotationKey,
 	}, {
 		name: "maxScale is less than MaxScaleLimit",
 		configMutator: func(config *autoscalerconfig.Config) {
 			config.MaxScaleLimit = 10
 		},
-		isInCreate:  true,
 		annotations: map[string]string{MaxScaleAnnotationKey: "9"},
 	}, {
 		name: "valid algorithm on KPA",
@@ -386,9 +365,6 @@ func TestValidateAnnotations(t *testing.T) {
 				c.configMutator(cfg)
 			}
 			ctx := context.Background()
-			if c.isInCreate {
-				ctx = apis.WithinCreate(ctx)
-			}
 			if got, want := ValidateAnnotations(ctx, cfg, c.annotations).Error(), c.expectErr; !reflect.DeepEqual(got, want) {
 				t.Errorf("\nErr = %q,\nwant: %q, diff(-want,+got):\n%s", got, want, cmp.Diff(want, got))
 			}