Remove immutability in PA validation (#4269)

knative · Jun 6, 2019 · 6afdcf3 · 6afdcf3
1 parent 0800b94
commit 6afdcf3
Show file tree

Hide file tree

Showing 3 changed files with 2 additions and 221 deletions.
diff --git a/pkg/apis/autoscaling/v1alpha1/pa_types.go b/pkg/apis/autoscaling/v1alpha1/pa_types.go
@@ -50,7 +50,7 @@ type PodAutoscaler struct {
 
 // Verify that PodAutoscaler adheres to the appropriate interfaces.
 var (
-	// Check that PodAutoscaler can be validated, can be defaulted, and has immutable fields.
+	// Check that PodAutoscaler can be validated and can be defaulted.
 	_ apis.Validatable = (*PodAutoscaler)(nil)
 	_ apis.Defaultable = (*PodAutoscaler)(nil)
 

diff --git a/pkg/apis/autoscaling/v1alpha1/pa_validation.go b/pkg/apis/autoscaling/v1alpha1/pa_validation.go
@@ -31,37 +31,7 @@ import (
 func (pa *PodAutoscaler) Validate(ctx context.Context) *apis.FieldError {
 	errs := serving.ValidateObjectMetadata(pa.GetObjectMeta()).ViaField("metadata")
 	errs = errs.Also(pa.validateMetric())
-	errs = errs.Also(pa.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
-	if apis.IsInUpdate(ctx) {
-		original := apis.GetBaseline(ctx).(*PodAutoscaler)
-		errs = errs.Also(pa.checkImmutableFields(ctx, original))
-	}
-	return errs
-}
-
-func (current *PodAutoscaler) checkImmutableFields(ctx context.Context, original *PodAutoscaler) *apis.FieldError {
-	if diff, err := compareSpec(original, current); err != nil {
-		return &apis.FieldError{
-			Message: "Failed to diff PodAutoscaler",
-			Paths:   []string{"spec"},
-			Details: err.Error(),
-		}
-	} else if diff != "" {
-		return &apis.FieldError{
-			Message: "Immutable fields changed (-old +new)",
-			Paths:   []string{"spec"},
-			Details: diff,
-		}
-	}
-	// Verify the PA class does not change.
-	// For backward compatibility, we allow a new class where there was none before.
-	if oldClass, newClass, annotationChanged := classAnnotationChanged(original, current); annotationChanged {
-		return &apis.FieldError{
-			Message: fmt.Sprintf("Immutable class annotation changed (-%q +%q)", oldClass, newClass),
-			Paths:   []string{"annotations[autoscaling.knative.dev/class]"},
-		}
-	}
-	return nil
+	return errs.Also(pa.Spec.Validate(apis.WithinSpec(ctx)).ViaField("spec"))
 }
 
 // Validate validates PodAutoscaler Spec.

diff --git a/pkg/apis/autoscaling/v1alpha1/pa_validation_test.go b/pkg/apis/autoscaling/v1alpha1/pa_validation_test.go
@@ -238,192 +238,3 @@ func TestPodAutoscalerValidation(t *testing.T) {
 		})
 	}
 }
-
-func TestImmutableFields(t *testing.T) {
-	tests := []struct {
-		name string
-		new  *PodAutoscaler
-		old  *PodAutoscaler
-		want *apis.FieldError
-	}{{
-		name: "good (no change)",
-		new: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-			},
-		},
-		old: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-			},
-		},
-		want: nil,
-	}, {
-		name: "good (protocol added)",
-		new: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-				ProtocolType: net.ProtocolHTTP1,
-			},
-		},
-		old: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-			},
-		},
-		want: nil,
-	}, {
-		name: "bad (concurrency model change)",
-		new: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-			},
-		},
-		old: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ContainerConcurrency: 1,
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-			},
-		},
-		want: &apis.FieldError{
-			Message: "Immutable fields changed (-old +new)",
-			Paths:   []string{"spec"},
-			Details: `{v1alpha1.PodAutoscalerSpec}.ContainerConcurrency:
-	-: "1"
-	+: "0"
-`,
-		},
-	}, {
-		name: "bad (container concurrency change)",
-		new: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ContainerConcurrency: 0,
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-			},
-		},
-		old: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ContainerConcurrency: 1,
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-			},
-		},
-		want: &apis.FieldError{
-			Message: "Immutable fields changed (-old +new)",
-			Paths:   []string{"spec"},
-			Details: `{v1alpha1.PodAutoscalerSpec}.ContainerConcurrency:
-	-: "1"
-	+: "0"
-`,
-		},
-	}, {
-		name: "bad (multiple changes)",
-		new: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				DeprecatedServiceName: "foo",
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "bar",
-				},
-			},
-		},
-		old: &PodAutoscaler{
-			ObjectMeta: v1.ObjectMeta{
-				Name: "valid",
-			},
-			Spec: PodAutoscalerSpec{
-				ContainerConcurrency:  1,
-				DeprecatedServiceName: "food",
-				ScaleTargetRef: corev1.ObjectReference{
-					APIVersion: "apps/v1",
-					Kind:       "Deployment",
-					Name:       "baz",
-				},
-			},
-		},
-		want: &apis.FieldError{
-			Message: "Immutable fields changed (-old +new)",
-			Paths:   []string{"spec"},
-			Details: `{v1alpha1.PodAutoscalerSpec}.ContainerConcurrency:
-	-: "1"
-	+: "0"
-{v1alpha1.PodAutoscalerSpec}.ScaleTargetRef.Name:
-	-: "baz"
-	+: "bar"
-{v1alpha1.PodAutoscalerSpec}.DeprecatedServiceName:
-	-: "food"
-	+: "foo"
-`,
-		},
-	}}
-
-	for _, test := range tests {
-		t.Run(test.name, func(t *testing.T) {
-			ctx := context.Background()
-			ctx = apis.WithinUpdate(ctx, test.old)
-			got := test.new.Validate(ctx)
-			if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
-				t.Errorf("Validate (-want, +got) = %v", diff)
-			}
-		})
-	}
-}