diff --git a/pkg/api/validation/validation.go b/pkg/api/validation/validation.go index 2e3477b449..3e93ade711 100644 --- 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 index 84280be568..43f4447255 100644 --- 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( @@ -2124,11 +2207,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, @@ -4452,6 +4535,7 @@ func TestValidateNamespaceStatusUpdate(t *testing.T) { Phase: api.NamespaceActive, }, }, true}, + // Cannot set deletionTimestamp via status update {api.Namespace{ ObjectMeta: api.ObjectMeta{ Name: "foo"}}, @@ -4462,6 +4546,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 index f6eaffb1a9..9b4b5ce847 100644 --- 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/storage" "k8s.io/kubernetes/pkg/api/unversioned" "k8s.io/kubernetes/pkg/fields" "k8s.io/kubernetes/pkg/labels" @@ -29,6 +30,7 @@ import ( "k8s.io/kubernetes/pkg/registry/generic/registry" "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,13 +101,66 @@ func (r *REST) Delete(ctx api.Context, name string, options *api.DeleteOptions) namespace := nsObj.(*api.Namespace) + // Ensure we have a UID precondition + if options == nil { + options = api.NewDeleteOptions(0) + } + if options.Preconditions == nil { + options.Preconditions = &api.Preconditions{} + } + if options.Preconditions.UID == nil { + options.Preconditions.UID = &namespace.UID + } else if *options.Preconditions.UID != namespace.UID { + err = apierrors.NewConflict( + api.Resource("namespaces"), + name, + fmt.Errorf("Precondition failed: UID in precondition: %v, UID in object meta: %v", *options.Preconditions.UID, namespace.UID), + ) + return nil, err + } + // 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.Store.KeyFunc(ctx, name) + if err != nil { + return nil, err + } + + preconditions := storage.Preconditions{UID: options.Preconditions.UID} + + out := r.Store.NewFunc() + err = r.Store.Storage.GuaranteedUpdate( + ctx, key, out, false, &preconditions, + 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) + } + // 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 @@ -113,7 +168,7 @@ func (r *REST) Delete(ctx api.Context, name string, options *api.DeleteOptions) err = apierrors.NewConflict(api.Resource("namespaces"), namespace.Name, fmt.Errorf("The system is ensuring all content is removed from this namespace. Upon completion, this namespace will automatically be purged by the system.")) return nil, err } - return r.Store.Delete(ctx, name, nil) + return r.Store.Delete(ctx, name, options) } func (r *StatusREST) New() runtime.Object { diff --git a/pkg/registry/namespace/strategy_test.go b/pkg/registry/namespace/strategy_test.go index fb06a302f9..ec852541e2 100644 --- 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}, }