Merge pull request #6246 from derekwaynecarr/finalize_fix

pkg/api/validation/validation.go

@@ -1149,21 +1149,13 @@ func validateFinalizerName(stringValue string) errs.ValidationErrorList {
 	return errs.ValidationErrorList{}
 }
 
-// ValidateNamespaceUpdate tests to make sure a namespace update can be applied.  Modifies oldNamespace.
-func ValidateNamespaceUpdate(oldNamespace *api.Namespace, namespace *api.Namespace) errs.ValidationErrorList {
+// ValidateNamespaceUpdate tests to make sure a namespace update can be applied.
+// newNamespace is updated with fields that cannot be changed
+func ValidateNamespaceUpdate(newNamespace *api.Namespace, oldNamespace *api.Namespace) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
-	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldNamespace.ObjectMeta, &namespace.ObjectMeta).Prefix("metadata")...)
-
-	// TODO: move reset function to its own location
-	// Ignore metadata changes now that they have been tested
-	oldNamespace.ObjectMeta = namespace.ObjectMeta
-	oldNamespace.Spec.Finalizers = namespace.Spec.Finalizers
-
-	// TODO: Add a 'real' ValidationError type for this error and provide print actual diffs.
-	if !api.Semantic.DeepEqual(oldNamespace, namespace) {
-		glog.V(4).Infof("Update failed validation %#v vs %#v", oldNamespace, namespace)
-		allErrs = append(allErrs, fmt.Errorf("update contains more than labels or annotation changes"))
-	}
+	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldNamespace.ObjectMeta, &newNamespace.ObjectMeta).Prefix("metadata")...)
+	newNamespace.Spec.Finalizers = oldNamespace.Spec.Finalizers
+	newNamespace.Status = oldNamespace.Status
 	return allErrs
 }
 
@@ -1176,15 +1168,14 @@ func ValidateNamespaceStatusUpdate(newNamespace, oldNamespace *api.Namespace) er
 	return allErrs
 }
 
-// ValidateNamespaceFinalizeUpdate tests to see if the update is legal for an end user to make. newNamespace is updated with fields
-// that cannot be changed.
+// ValidateNamespaceFinalizeUpdate tests to see if the update is legal for an end user to make.
+// newNamespace is updated with fields that cannot be changed.
 func ValidateNamespaceFinalizeUpdate(newNamespace, oldNamespace *api.Namespace) errs.ValidationErrorList {
 	allErrs := errs.ValidationErrorList{}
 	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldNamespace.ObjectMeta, &newNamespace.ObjectMeta).Prefix("metadata")...)
 	for i := range newNamespace.Spec.Finalizers {
 		allErrs = append(allErrs, validateFinalizerName(string(newNamespace.Spec.Finalizers[i]))...)
 	}
-	newNamespace.ObjectMeta = oldNamespace.ObjectMeta
 	newNamespace.Status = oldNamespace.Status
 	return allErrs
 }

pkg/api/validation/validation_test.go

@@ -2450,6 +2450,9 @@ func TestValidateNamespace(t *testing.T) {
 		},
 		{
 			ObjectMeta: api.ObjectMeta{Name: "abc-123"},
+			Spec: api.NamespaceSpec{
+				Finalizers: []api.FinalizerName{"example.com/something", "example.com/other"},
+			},
 		},
 	}
 	for _, successCase := range successCases {
@@ -2513,6 +2516,20 @@ func TestValidateNamespaceFinalizeUpdate(t *testing.T) {
 					Finalizers: []api.FinalizerName{"foo.com/bar", "what.com/bar"},
 				},
 			}, true},
+		{api.Namespace{
+			ObjectMeta: api.ObjectMeta{
+				Name: "fooemptyfinalizer"},
+			Spec: api.NamespaceSpec{
+				Finalizers: []api.FinalizerName{"foo.com/bar"},
+			},
+		},
+			api.Namespace{
+				ObjectMeta: api.ObjectMeta{
+					Name: "fooemptyfinalizer"},
+				Spec: api.NamespaceSpec{
+					Finalizers: []api.FinalizerName{"", "foo.com/bar", "what.com/bar"},
+				},
+			}, false},
 	}
 	for i, test := range tests {
 		test.namespace.ObjectMeta.ResourceVersion = "1"
@@ -2579,59 +2596,76 @@ func TestValidateNamespaceUpdate(t *testing.T) {
 		{api.Namespace{}, api.Namespace{}, true},
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name: "foo"}},
+				Name: "foo1"}},
 			api.Namespace{
 				ObjectMeta: api.ObjectMeta{
-					Name: "bar"},
+					Name: "bar1"},
 			}, false},
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name:   "foo",
+				Name:   "foo2",
 				Labels: map[string]string{"foo": "bar"},
 			},
 		}, api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name:   "foo",
+				Name:   "foo2",
 				Labels: map[string]string{"foo": "baz"},
 			},
 		}, true},
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name: "foo",
+				Name: "foo3",
 			},
 		}, api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name:   "foo",
+				Name:   "foo3",
 				Labels: map[string]string{"foo": "baz"},
 			},
 		}, true},
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name:   "foo",
+				Name:   "foo4",
 				Labels: map[string]string{"bar": "foo"},
 			},
 		}, api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name:   "foo",
+				Name:   "foo4",
 				Labels: map[string]string{"foo": "baz"},
 			},
 		}, true},
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name:   "foo",
+				Name:   "foo5",
 				Labels: map[string]string{"foo": "baz"},
 			},
 		}, api.Namespace{
 			ObjectMeta: api.ObjectMeta{
-				Name:   "foo",
+				Name:   "foo5",
 				Labels: map[string]string{"Foo": "baz"},
 			},
 		}, true},
+		{api.Namespace{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo6",
+				Labels: map[string]string{"foo": "baz"},
+			},
+		}, api.Namespace{
+			ObjectMeta: api.ObjectMeta{
+				Name:   "foo6",
+				Labels: map[string]string{"Foo": "baz"},
+			},
+			Spec: api.NamespaceSpec{
+				Finalizers: []api.FinalizerName{"kubernetes"},
+			},
+			Status: api.NamespaceStatus{
+				Phase: api.NamespaceTerminating,
+			},
+		}, true},
 	}
 	for i, test := range tests {
 		test.namespace.ObjectMeta.ResourceVersion = "1"
 		test.oldNamespace.ObjectMeta.ResourceVersion = "1"
-		errs := ValidateNamespaceUpdate(&test.oldNamespace, &test.namespace)
+		errs := ValidateNamespaceUpdate(&test.namespace, &test.oldNamespace)
 		if test.valid && len(errs) > 0 {
 			t.Errorf("%d: Unexpected error: %v", i, errs)
 			t.Logf("%#v vs %#v", test.oldNamespace.ObjectMeta, test.namespace.ObjectMeta)

pkg/namespace/namespace_controller.go

@@ -99,20 +99,16 @@ func finalized(namespace api.Namespace) bool {
 
 // finalize will finalize the namespace for kubernetes
 func finalize(kubeClient client.Interface, namespace api.Namespace) (*api.Namespace, error) {
-	namespaceFinalize := api.Namespace{
-		ObjectMeta: api.ObjectMeta{
-			Name:            namespace.Name,
-			ResourceVersion: namespace.ResourceVersion,
-		},
-		Spec: api.NamespaceSpec{},
-	}
+	namespaceFinalize := api.Namespace{}
+	namespaceFinalize.ObjectMeta = namespace.ObjectMeta
+	namespaceFinalize.Spec = namespace.Spec
 	finalizerSet := util.NewStringSet()
 	for i := range namespace.Spec.Finalizers {
 		if namespace.Spec.Finalizers[i] != api.FinalizerKubernetes {
 			finalizerSet.Insert(string(namespace.Spec.Finalizers[i]))
 		}
 	}
-	namespaceFinalize.Spec.Finalizers = make([]api.FinalizerName, len(finalizerSet), len(finalizerSet))
+	namespaceFinalize.Spec.Finalizers = make([]api.FinalizerName, 0, len(finalizerSet))
 	for _, value := range finalizerSet.List() {
 		namespaceFinalize.Spec.Finalizers = append(namespaceFinalize.Spec.Finalizers, api.FinalizerName(value))
 	}

pkg/registry/namespace/rest.go

@@ -118,6 +118,13 @@ func (namespaceFinalizeStrategy) ValidateUpdate(ctx api.Context, obj, old runtim
 	return validation.ValidateNamespaceFinalizeUpdate(obj.(*api.Namespace), old.(*api.Namespace))
 }
 
+// PrepareForUpdate clears fields that are not allowed to be set by end users on update.
+func (namespaceFinalizeStrategy) PrepareForUpdate(obj, old runtime.Object) {
+	newNamespace := obj.(*api.Namespace)
+	oldNamespace := old.(*api.Namespace)
+	newNamespace.Status = oldNamespace.Status
+}
+
 // MatchNamespace returns a generic matcher for a given label and field selector.
 func MatchNamespace(label labels.Selector, field fields.Selector) generic.Matcher {
 	return generic.MatcherFunc(func(obj runtime.Object) (bool, error) {

pkg/registry/namespace/rest_test.go

@@ -23,6 +23,7 @@ import (
 )
 
 func TestNamespaceStrategy(t *testing.T) {
+	ctx := api.NewDefaultContext()
 	if Strategy.NamespaceScoped() {
 		t.Errorf("Namespaces should not be namespace scoped")
 	}
@@ -30,11 +31,100 @@ func TestNamespaceStrategy(t *testing.T) {
 		t.Errorf("Namespaces should not allow create on update")
 	}
 	namespace := &api.Namespace{
-		ObjectMeta: api.ObjectMeta{Name: "foo"},
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"},
 		Status:     api.NamespaceStatus{Phase: api.NamespaceTerminating},
 	}
 	Strategy.PrepareForCreate(namespace)
 	if namespace.Status.Phase != api.NamespaceActive {
 		t.Errorf("Namespaces do not allow setting phase on create")
 	}
+	if len(namespace.Spec.Finalizers) != 1 || namespace.Spec.Finalizers[0] != api.FinalizerKubernetes {
+		t.Errorf("Prepare For Create should have added kubernetes finalizer")
+	}
+	errs := Strategy.Validate(ctx, namespace)
+	if len(errs) != 0 {
+		t.Errorf("Unexpected error validating %v", errs)
+	}
+	invalidNamespace := &api.Namespace{
+		ObjectMeta: api.ObjectMeta{Name: "bar", ResourceVersion: "4"},
+	}
+	// ensure we copy spec.finalizers from old to new
+	Strategy.PrepareForUpdate(invalidNamespace, namespace)
+	if len(invalidNamespace.Spec.Finalizers) != 1 || invalidNamespace.Spec.Finalizers[0] != api.FinalizerKubernetes {
+		t.Errorf("PrepareForUpdate should have preserved old.spec.finalizers")
+	}
+	errs = Strategy.ValidateUpdate(ctx, invalidNamespace, namespace)
+	if len(errs) == 0 {
+		t.Errorf("Expected a validation error")
+	}
+	if invalidNamespace.ResourceVersion != "4" {
+		t.Errorf("Incoming resource version on update should not be mutated")
+	}
+}
+
+func TestNamespaceStatusStrategy(t *testing.T) {
+	ctx := api.NewDefaultContext()
+	if StatusStrategy.NamespaceScoped() {
+		t.Errorf("Namespaces should not be namespace scoped")
+	}
+	if StatusStrategy.AllowCreateOnUpdate() {
+		t.Errorf("Namespaces should not allow create on update")
+	}
+	oldNamespace := &api.Namespace{
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"},
+		Spec:       api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes"}},
+		Status:     api.NamespaceStatus{Phase: api.NamespaceActive},
+	}
+	namespace := &api.Namespace{
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "9"},
+		Status:     api.NamespaceStatus{Phase: api.NamespaceTerminating},
+	}
+	StatusStrategy.PrepareForUpdate(namespace, oldNamespace)
+	if namespace.Status.Phase != api.NamespaceTerminating {
+		t.Errorf("Namespace status updates should allow change of phase: %v", namespace.Status.Phase)
+	}
+	if len(namespace.Spec.Finalizers) != 1 || namespace.Spec.Finalizers[0] != api.FinalizerKubernetes {
+		t.Errorf("PrepareForUpdate should have preserved old finalizers")
+	}
+	errs := StatusStrategy.ValidateUpdate(ctx, namespace, oldNamespace)
+	if len(errs) != 0 {
+		t.Errorf("Unexpected error %v", errs)
+	}
+	if namespace.ResourceVersion != "9" {
+		t.Errorf("Incoming resource version on update should not be mutated")
+	}
+}
+
+func TestNamespaceFinalizeStrategy(t *testing.T) {
+	ctx := api.NewDefaultContext()
+	if FinalizeStrategy.NamespaceScoped() {
+		t.Errorf("Namespaces should not be namespace scoped")
+	}
+	if FinalizeStrategy.AllowCreateOnUpdate() {
+		t.Errorf("Namespaces should not allow create on update")
+	}
+	oldNamespace := &api.Namespace{
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"},
+		Spec:       api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes", "example.com/org"}},
+		Status:     api.NamespaceStatus{Phase: api.NamespaceActive},
+	}
+	namespace := &api.Namespace{
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "9"},
+		Spec:       api.NamespaceSpec{Finalizers: []api.FinalizerName{"example.com/foo"}},
+		Status:     api.NamespaceStatus{Phase: api.NamespaceTerminating},
+	}
+	FinalizeStrategy.PrepareForUpdate(namespace, oldNamespace)
+	if namespace.Status.Phase != api.NamespaceActive {
+		t.Errorf("finalize updates should not allow change of phase: %v", namespace.Status.Phase)
+	}
+	if len(namespace.Spec.Finalizers) != 1 || string(namespace.Spec.Finalizers[0]) != "example.com/foo" {
+		t.Errorf("PrepareForUpdate should have modified finalizers")
+	}
+	errs := StatusStrategy.ValidateUpdate(ctx, namespace, oldNamespace)
+	if len(errs) != 0 {
+		t.Errorf("Unexpected error %v", errs)
+	}
+	if namespace.ResourceVersion != "9" {
+		t.Errorf("Incoming resource version on update should not be mutated")
+	}
 }