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

merged 1 commit into from
Apr 28, 2016
+165 −10

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Apr 27, 2016 •
edited
Loading

Fixes #24828

Updates validation to ensure that deletionTimestamp and deletionGracePeriodSeconds are not set during update operations to ensure those fields are only set during delete operations.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Apr 27, 2016
@liggitt liggitt force-pushed the immutable-deletion-timestamp branch 3 times, most recently from ff650e0 to 4fd03b3 Compare April 27, 2016 03:39
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 27, 2016
@liggitt liggitt force-pushed the immutable-deletion-timestamp branch 2 times, most recently from cbd4a84 to 751b2a0 Compare April 27, 2016 04:15
@liggitt
Copy link
Member Author

liggitt commented Apr 27, 2016 •
edited
Loading

@smarterclayton ptal regarding intended semantics on deletionTimestamp/deleteGracePeriodSeconds

@derekwaynecarr ptal at the namespace deletion change... this is the only registry doing unusual things with deletionTimestamp during delete. A rework of graceful deletion is needed to allow a DeleteStrategy to do things like phase changes or finalizer changes, but this was the minimal change I could come up with to avoid allowing mutating deletionTimestamp via anything other than a delete API call.

@liggitt liggitt force-pushed the immutable-deletion-timestamp branch from 751b2a0 to 347390d Compare April 27, 2016 05:00
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 27, 2016
@liggitt liggitt force-pushed the immutable-deletion-timestamp branch 4 times, most recently from 9bee4ec to 87a7fe7 Compare April 27, 2016 13:42
@derekwaynecarr
Copy link
Member

@lavalamp @liggitt - I am +100 in favor of this change. edit of a namespace should not have caused deletion timestamp to be set to trigger deletion of entire namespace!

@derekwaynecarr
Copy link
Member

I would also like this to be cherry-picked.

@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@derekwaynecarr derekwaynecarr added this to the v1.3 milestone Apr 27, 2016
@derekwaynecarr
Copy link
Member

This was an oversight in the implementation.

See the initial design that only discussed DELETE triggering a change to DeletionTimestamp: https://github.com/kubernetes/kubernetes/blob/master/docs/design/namespaces.md#phases

If a DELETE request occurs for a Namespace, the Namespace.ObjectMeta.DeletionTimestamp is set to the current server time. A namespace controller observes the change, and sets the Namespace.Status.Phase to Terminating.

It was NOT intended that update to a Namespace could trigger cascading deletion.

@zmerlynn
Copy link
Member

FYI, the milestone @k8s-cherrypick-bot wants is the furthest back you want it cherrypicked to. I'm assuming v1.2, since right now we're not shipping v1.1s.

@zmerlynn zmerlynn modified the milestones: v1.2, v1.3 Apr 27, 2016
@liggitt liggitt force-pushed the immutable-deletion-timestamp branch from 87a7fe7 to 2993a38 Compare April 27, 2016 19:16
@lavalamp lavalamp added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 27, 2016
lavalamp
lavalamp reviewed Apr 27, 2016
View reviewed changes
pkg/registry/namespace/etcd/etcd.go
}
if options.Preconditions.UID == nil {
options.Preconditions.UID = &namespace.UID
}
Copy link
Member

Choose a reason for hiding this comment

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

Else, check that namespace.UID == options.Preconditions.UID? (i.e., bail early)

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, do we return conflict errors in those cases?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that's what @caesarxuchao did in the place I'm thinking of.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@lavalamp
Copy link
Member

LGTM

@liggitt liggitt force-pushed the immutable-deletion-timestamp branch from 2993a38 to 1e58158 Compare April 28, 2016 15:50
@liggitt
Copy link
Member Author

liggitt commented Apr 28, 2016

comments addressed

@liggitt liggitt mentioned this pull request Apr 28, 2016
Merged
@liggitt
Copy link
Member Author

liggitt commented Apr 28, 2016

tagging per comment

@liggitt liggitt added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. labels Apr 28, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Apr 28, 2016

GCE e2e build/test passed for commit 1e58158.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit f44bb03 into kubernetes:master Apr 28, 2016
@liggitt liggitt deleted the immutable-deletion-timestamp branch April 28, 2016 20:01
@roberthbailey roberthbailey added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 29, 2016
k8s-github-robot pushed a commit that referenced this pull request May 2, 2016
Merge pull request #24933 from liggitt/automated-cherry-pick-of-#2483…
41455c0 
…9-upstream-release-1.2

Automatic merge from submit-queue

Automated cherry pick of #24839

Cherry pick of #24839 on release-1.2.
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#24933 from liggitt/automated-cherry-pic…
abefa57 
…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.
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
Merge pull request kubernetes#24933 from liggitt/automated-cherry-pic…
374618f 
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@lavalamp lavalamp

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
9 participants
@liggitt @derekwaynecarr @k8s-cherrypick-bot @zmerlynn @lavalamp @k8s-github-robot @k8s-bot @googlebot @roberthbailey