Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Validate deletion timestamp doesn't change on update #24839

Merged
merged 1 commit into from
Apr 28, 2016
Merged

Validate deletion timestamp doesn't change on update #24839

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion pkg/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
101 changes: 99 additions & 2 deletions pkg/api/validation/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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"}},
Expand All @@ -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{
Expand Down
67 changes: 61 additions & 6 deletions pkg/registry/namespace/etcd/etcd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down Expand Up @@ -99,21 +101,74 @@ func (r *REST) Delete(ctx api.Context, name string, options *api.DeleteOptions)

namespace := nsObj.(*api.Namespace)

// Ensure we have a UID precondition
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem good but unrelated-- do they really need to be cherrypicked?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UID preconditions didn't exist in 1.2, I'll adjust appropriately when I cherry pick

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
if len(namespace.Spec.Finalizers) != 0 {
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."))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it get deleted automatically when the last finalizer is removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, the namespace controller sends an additional delete call.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the namespace controller handles that:
https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/namespace/namespace_controller_utils.go#L369-L376

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For garbage collector, we were planning on making the update path auto delete things with a deletion timestamp when the last finalizer is removed. Do you have any objections to changing that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work much better if multiple components were removing finalizers, assuming you can craft a reasonable PATCH request that removes a single finalizer.

return nil, err
}
return r.Store.Delete(ctx, name, nil)
return r.Store.Delete(ctx, name, options)
}

func (r *StatusREST) New() runtime.Object {
Expand Down
2 changes: 1 addition & 1 deletion pkg/registry/namespace/strategy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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},
}
Expand Down