-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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.
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
add safe way to prevent status from touching objectmeta #45552
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@k8s-bot kops aws e2e test this |
/release-note-none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure that now is a great time to push for status & spec separation in the storage layer. Is it blocking something?
|
||
ResetObjectMetaForStatus(meta, existingMeta) | ||
|
||
// not all fields are stomped during the reset. These fields should not have been set.false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set.false?
// for a pre-existing object. Status updates are for updating status, not annotations | ||
func ResetObjectMetaForStatus(meta, existingMeta Object) { | ||
meta.SetGeneration(existingMeta.GetGeneration()) | ||
meta.SetSelfLink(existingMeta.GetSelfLink()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self link should always be cleared before it goes to storage, as should resource version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(so this shouldn't matter)
// you're enforcing immutability (those are fine) and whether /status should be able to update | ||
// these values (these are usually not fine). | ||
|
||
// generateName doesn't do anything after create |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but should a status update ever change it? I think not.
existingMeta.SetClusterName("") | ||
existingMeta.SetCreationTimestamp(Time{}) | ||
existingMeta.SetDeletionTimestamp(nil) | ||
existingMeta.SetDeletionGracePeriodSeconds(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was under the impression that this could be shortened but not extended?
Does it ever make sense for a status update to shorten it? Actually I think the answer there might be yes?
meta.SetSelfLink(existingMeta.GetSelfLink()) | ||
meta.SetLabels(existingMeta.GetLabels()) | ||
meta.SetAnnotations(existingMeta.GetAnnotations()) | ||
meta.SetFinalizers(existingMeta.GetFinalizers()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it ever make sense to remove a finalizer and change status in the same request? I think the answer is probably yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it ever make sense to remove a finalizer and change status in the same request? I think the answer is probably yes?
Removing finalizers was done using the /finalize
subresource for namespaces. I can also see a reason for doing some in a normal update/patch, but I don't think we've had a case for doing it in status before.
meta.SetGeneration(existingMeta.GetGeneration()) | ||
meta.SetSelfLink(existingMeta.GetSelfLink()) | ||
meta.SetLabels(existingMeta.GetLabels()) | ||
meta.SetAnnotations(existingMeta.GetAnnotations()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it ever make sense to change an annotation and something in status at the same time? I think yes, unfortunately.
Otherwise controllers that want to do this need to send two updates and wouldn't be able to make an atomic change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise controllers that want to do this need to send two updates and wouldn't be able to make an atomic change.
This doesn't stop it from ever being possible (you can do something different the strategy as opposed to https://github.com/kubernetes/kubernetes/pull/45552/files#diff-fefac67143bac6db5964a9cb79e6c988R97), but it would change it to be non-default. I think its more the exception than the rule.
func ResetObjectMetaForStatus(meta, existingMeta Object) { | ||
meta.SetGeneration(existingMeta.GetGeneration()) | ||
meta.SetSelfLink(existingMeta.GetSelfLink()) | ||
meta.SetLabels(existingMeta.GetLabels()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for annotations, but less emphatically. Until we have a decent, working general field mechanism I think it probably does make sense to allow label and status updates at the same time. (use case: allow pod's node selector or affinity / taint selectors to match status-y things about nodes)
I don't know of anyone doing this currently, though?
meta.SetLabels(existingMeta.GetLabels()) | ||
meta.SetAnnotations(existingMeta.GetAnnotations()) | ||
meta.SetFinalizers(existingMeta.GetFinalizers()) | ||
meta.SetOwnerReferences(existingMeta.GetOwnerReferences()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to change ownership at the same time as a status update? I think that might make sense for some controllers.
It's coming up as we add more resources which have status on them and would come up for the apiserver generator too since it has subresource support. |
@lavalamp Many of your comments are about whether something is ever reasonable as opposed to whether we thing it's normally reasonable. This doesn't make it impossible to do something different (allow mutation on annotations for instance), but it does allow people who want "normal" behavior to do it in a single line that stays current with objectmeta like this: https://github.com/kubernetes/kubernetes/pull/45552/files#diff-fefac67143bac6db5964a9cb79e6c988R97 . In the places where you talk about "ever reasonable", do you think its actually a common case? Few if any controllers mutate these things via status now. |
newAPIService.Finalizers = oldAPIService.Finalizers | ||
newAPIService.OwnerReferences = oldAPIService.OwnerReferences | ||
|
||
metav1.ResetObjectMetaForStatus(newAPIService, oldAPIService) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we actually test for modification in ValidateUpdate
instread of overriding/resetting invalid field updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we actually test for modification in ValidateUpdate instread of overriding/resetting invalid field updates?
That would do weird things with unconditional updates where we want to stomp status to something. Cases where you don't want an unconditional update are caught by resourceVersion.
Automatic merge from submit-queue (batch tested with PRs 45826, 45747, 45548, 45606, 41766) prevent pods/status from touching ownerreferences pods/status updates touching ownerreferences causes new fields to be dropped. I think we really want to protect our metatdata by default with something like #45552 . This fixes the immediate problem. ```release-note prevent pods/status from touching ownerreferences ``` @derekwaynecarr @eparis
So we would only ever use this on new v2 APIs and new resource groups, because otherwise this is a breaking API change? |
Since /status is controller driven by code that we own, I think we could also close down the majority of other resources as well. I'm not proposing that we willfully break existing endpoints, but I'd like to close as many as possible. |
I don't see how we can change the set of things status can update in a safe way. That's a way bigger API change than we normally allow. |
I'm hard pressed to think of a reason why we couldn't stop rc/status from updating finalizers as a for instance, but I'd accept "new resources only" as a starting point. |
@deads2k PR needs rebase |
Please see #20978. |
This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review. cc @deads2k @lavalamp @smarterclayton You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days |
I think that new resources will want it and CRs definitely want it. |
Take two: #59038 |
Fixes #45539
This provides a way for
/status
endpoints to prevent objectmeta mutation moving forward.@kubernetes/sig-api-machinery-pr-reviews
@sttts as promised