Merge pull request kubernetes#24933 from liggitt/automated-cherry-pic…

…k-of-#24839-upstream-release-1.2 Automatic merge from submit-queue Automated cherry pick of kubernetes#24839 Cherry pick of kubernetes#24839 on release-1.2.
shyamjvs · May 2, 2016 · abefa57 · abefa57
2 parents 85ed5c2 + 7dfb543
commit abefa57
Show file tree

Hide file tree

Showing 4 changed files with 151 additions and 9 deletions.
diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go
@@ -360,9 +360,12 @@ func ValidateObjectMetaUpdate(newMeta, oldMeta *api.ObjectMeta, fldPath *field.P
 	}
 
 	// TODO: needs to check if newMeta==nil && oldMeta !=nil after the repair logic is removed.
-	if newMeta.DeletionGracePeriodSeconds != nil && oldMeta.DeletionGracePeriodSeconds != nil && *newMeta.DeletionGracePeriodSeconds != *oldMeta.DeletionGracePeriodSeconds {
+	if newMeta.DeletionGracePeriodSeconds != nil && (oldMeta.DeletionGracePeriodSeconds == nil || *newMeta.DeletionGracePeriodSeconds != *oldMeta.DeletionGracePeriodSeconds) {
 		allErrs = append(allErrs, field.Invalid(fldPath.Child("deletionGracePeriodSeconds"), newMeta.DeletionGracePeriodSeconds, "field is immutable; may only be changed via deletion"))
 	}
+	if newMeta.DeletionTimestamp != nil && (oldMeta.DeletionTimestamp == nil || !newMeta.DeletionTimestamp.Equal(*oldMeta.DeletionTimestamp)) {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("deletionTimestamp"), newMeta.DeletionTimestamp, "field is immutable; may only be changed via deletion"))
+	}
 
 	// Reject updates that don't specify a resource version
 	if len(newMeta.ResourceVersion) == 0 {

diff --git a/pkg/api/validation/validation_test.go b/pkg/api/validation/validation_test.go
@@ -122,6 +122,89 @@ func TestValidateObjectMetaUpdateIgnoresCreationTimestamp(t *testing.T) {
 	}
 }
 
+func TestValidateObjectMetaUpdatePreventsDeletionFieldMutation(t *testing.T) {
+	now := unversioned.NewTime(time.Unix(1000, 0).UTC())
+	later := unversioned.NewTime(time.Unix(2000, 0).UTC())
+	gracePeriodShort := int64(30)
+	gracePeriodLong := int64(40)
+
+	testcases := map[string]struct {
+		Old          api.ObjectMeta
+		New          api.ObjectMeta
+		ExpectedNew  api.ObjectMeta
+		ExpectedErrs []string
+	}{
+		"valid without deletion fields": {
+			Old:          api.ObjectMeta{Name: "test", ResourceVersion: "1"},
+			New:          api.ObjectMeta{Name: "test", ResourceVersion: "1"},
+			ExpectedNew:  api.ObjectMeta{Name: "test", ResourceVersion: "1"},
+			ExpectedErrs: []string{},
+		},
+		"valid with deletion fields": {
+			Old:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now, DeletionGracePeriodSeconds: &gracePeriodShort},
+			New:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now, DeletionGracePeriodSeconds: &gracePeriodShort},
+			ExpectedNew:  api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now, DeletionGracePeriodSeconds: &gracePeriodShort},
+			ExpectedErrs: []string{},
+		},
+
+		"invalid set deletionTimestamp": {
+			Old:          api.ObjectMeta{Name: "test", ResourceVersion: "1"},
+			New:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
+			ExpectedNew:  api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
+			ExpectedErrs: []string{"field.deletionTimestamp: Invalid value: \"1970-01-01T00:16:40Z\": field is immutable; may only be changed via deletion"},
+		},
+		"invalid clear deletionTimestamp": {
+			Old:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
+			New:          api.ObjectMeta{Name: "test", ResourceVersion: "1"},
+			ExpectedNew:  api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
+			ExpectedErrs: []string{}, // no errors, validation copies the old value
+		},
+		"invalid change deletionTimestamp": {
+			Old:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
+			New:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &later},
+			ExpectedNew:  api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionTimestamp: &now},
+			ExpectedErrs: []string{}, // no errors, validation copies the old value
+		},
+
+		"invalid set deletionGracePeriodSeconds": {
+			Old:          api.ObjectMeta{Name: "test", ResourceVersion: "1"},
+			New:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort},
+			ExpectedNew:  api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort},
+			ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: 30: field is immutable; may only be changed via deletion"},
+		},
+		"invalid clear deletionGracePeriodSeconds": {
+			Old:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort},
+			New:          api.ObjectMeta{Name: "test", ResourceVersion: "1"},
+			ExpectedNew:  api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort},
+			ExpectedErrs: []string{}, // no errors, validation copies the old value
+		},
+		"invalid change deletionGracePeriodSeconds": {
+			Old:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodShort},
+			New:          api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodLong},
+			ExpectedNew:  api.ObjectMeta{Name: "test", ResourceVersion: "1", DeletionGracePeriodSeconds: &gracePeriodLong},
+			ExpectedErrs: []string{"field.deletionGracePeriodSeconds: Invalid value: 40: field is immutable; may only be changed via deletion"},
+		},
+	}
+
+	for k, tc := range testcases {
+		errs := ValidateObjectMetaUpdate(&tc.New, &tc.Old, field.NewPath("field"))
+		if len(errs) != len(tc.ExpectedErrs) {
+			t.Logf("%s: Expected: %#v", k, tc.ExpectedErrs)
+			t.Logf("%s: Got: %#v", k, errs)
+			t.Errorf("%s: expected %d errors, got %d", k, len(tc.ExpectedErrs), len(errs))
+			continue
+		}
+		for i := range errs {
+			if errs[i].Error() != tc.ExpectedErrs[i] {
+				t.Errorf("%s: error #%d: expected %q, got %q", k, i, tc.ExpectedErrs[i], errs[i].Error())
+			}
+		}
+		if !reflect.DeepEqual(tc.New, tc.ExpectedNew) {
+			t.Errorf("%s: Expected after validation:\n%#v\ngot\n%#v", k, tc.ExpectedNew, tc.New)
+		}
+	}
+}
+
 // Ensure trailing slash is allowed in generate name
 func TestValidateObjectMetaTrimsTrailingSlash(t *testing.T) {
 	errs := ValidateObjectMeta(
@@ -2123,11 +2206,11 @@ func TestValidatePodUpdate(t *testing.T) {
 		},
 		{
 			api.Pod{
-				ObjectMeta: api.ObjectMeta{Name: "foo", DeletionTimestamp: &now},
+				ObjectMeta: api.ObjectMeta{Name: "foo"},
 				Spec:       api.PodSpec{Containers: []api.Container{{Image: "foo:V1"}}},
 			},
 			api.Pod{
-				ObjectMeta: api.ObjectMeta{Name: "foo"},
+				ObjectMeta: api.ObjectMeta{Name: "foo", DeletionTimestamp: &now},
 				Spec:       api.PodSpec{Containers: []api.Container{{Image: "foo:V1"}}},
 			},
 			true,
@@ -4451,6 +4534,7 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) {
 				Phase: api.NamespaceActive,
 			},
 		}, true},
+		// Cannot set deletionTimestamp via status update
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{
 				Name: "foo"}},
@@ -4461,6 +4545,19 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) {
 				Status: api.NamespaceStatus{
 					Phase: api.NamespaceTerminating,
 				},
+			}, false},
+		// Can update phase via status update
+		{api.Namespace{
+			ObjectMeta: api.ObjectMeta{
+				Name:              "foo",
+				DeletionTimestamp: &now}},
+			api.Namespace{
+				ObjectMeta: api.ObjectMeta{
+					Name:              "foo",
+					DeletionTimestamp: &now},
+				Status: api.NamespaceStatus{
+					Phase: api.NamespaceTerminating,
+				},
 			}, true},
 		{api.Namespace{
 			ObjectMeta: api.ObjectMeta{

diff --git a/pkg/registry/namespace/etcd/etcd.go b/pkg/registry/namespace/etcd/etcd.go
@@ -21,6 +21,7 @@ import (
 
 	"k8s.io/kubernetes/pkg/api"
 	apierrors "k8s.io/kubernetes/pkg/api/errors"
+	storageerr "k8s.io/kubernetes/pkg/api/errors/etcd"
 	"k8s.io/kubernetes/pkg/api/unversioned"
 	"k8s.io/kubernetes/pkg/fields"
 	"k8s.io/kubernetes/pkg/labels"
@@ -29,6 +30,7 @@ import (
 	etcdgeneric "k8s.io/kubernetes/pkg/registry/generic/etcd"
 	"k8s.io/kubernetes/pkg/registry/namespace"
 	"k8s.io/kubernetes/pkg/runtime"
+	"k8s.io/kubernetes/pkg/storage"
 )
 
 // rest implements a RESTStorage for namespaces against etcd
@@ -99,12 +101,52 @@ func (r *REST) Delete(ctx api.Context, name string, options *api.DeleteOptions)
 	namespace := nsObj.(*api.Namespace)
 
 	// upon first request to delete, we switch the phase to start namespace termination
+	// TODO: enhance graceful deletion's calls to DeleteStrategy to allow phase change and finalizer patterns
 	if namespace.DeletionTimestamp.IsZero() {
-		now := unversioned.Now()
-		namespace.DeletionTimestamp = &now
-		namespace.Status.Phase = api.NamespaceTerminating
-		result, _, err := r.status.Update(ctx, namespace)
-		return result, err
+		key, err := r.Etcd.KeyFunc(ctx, name)
+		if err != nil {
+			return nil, err
+		}
+
+		out := r.Etcd.NewFunc()
+		err = r.Etcd.Storage.GuaranteedUpdate(
+			ctx, key, out, false,
+			storage.SimpleUpdate(func(existing runtime.Object) (runtime.Object, error) {
+				existingNamespace, ok := existing.(*api.Namespace)
+				if !ok {
+					// wrong type
+					return nil, fmt.Errorf("expected *api.Namespace, got %v", existing)
+				}
+				if existingNamespace.UID != namespace.UID {
+					return nil, apierrors.NewConflict(
+						api.Resource("namespaces"),
+						name,
+						fmt.Errorf("UID in precondition: %v, UID in object meta: %v", namespace.UID, existingNamespace.UID),
+					)
+				}
+				// Set the deletion timestamp if needed
+				if existingNamespace.DeletionTimestamp.IsZero() {
+					now := unversioned.Now()
+					existingNamespace.DeletionTimestamp = &now
+				}
+				// Set the namespace phase to terminating, if needed
+				if existingNamespace.Status.Phase != api.NamespaceTerminating {
+					existingNamespace.Status.Phase = api.NamespaceTerminating
+				}
+				return existingNamespace, nil
+			}),
+		)
+
+		if err != nil {
+			err = storageerr.InterpretGetError(err, api.Resource("namespaces"), name)
+			err = storageerr.InterpretUpdateError(err, api.Resource("namespaces"), name)
+			if _, ok := err.(*apierrors.StatusError); !ok {
+				err = apierrors.NewInternalError(err)
+			}
+			return nil, err
+		}
+
+		return out, nil
 	}
 
 	// prior to final deletion, we must ensure that finalizers is empty

diff --git a/pkg/registry/namespace/strategy_test.go b/pkg/registry/namespace/strategy_test.go
@@ -75,7 +75,7 @@ func TestNamespaceStatusStrategy(t *testing.T) {
 	}
 	now := unversioned.Now()
 	oldNamespace := &api.Namespace{
-		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10"},
+		ObjectMeta: api.ObjectMeta{Name: "foo", ResourceVersion: "10", DeletionTimestamp: &now},
 		Spec:       api.NamespaceSpec{Finalizers: []api.FinalizerName{"kubernetes"}},
 		Status:     api.NamespaceStatus{Phase: api.NamespaceActive},
 	}