add safe way to prevent status from touching objectmeta #45552

deads2k · 2017-05-09T16:54:12Z

Fixes #45539

This provides a way for /status endpoints to prevent objectmeta mutation moving forward.

@kubernetes/sig-api-machinery-pr-reviews
@sttts as promised

k8s-reviewable · 2017-05-09T16:54:19Z

This change is

k8s-github-robot · 2017-05-09T16:54:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [deads2k]
~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

deads2k · 2017-05-09T17:51:50Z

@k8s-bot kops aws e2e test this

deads2k · 2017-05-09T17:52:00Z

/release-note-none

lavalamp

I'm not sure that now is a great time to push for status & spec separation in the storage layer. Is it blocking something?

lavalamp · 2017-05-09T17:48:11Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go

+
+	ResetObjectMetaForStatus(meta, existingMeta)
+
+	// not all fields are stomped during the reset.  These fields should not have been set.false


lavalamp · 2017-05-09T17:49:08Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go

+// 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())


self link should always be cleared before it goes to storage, as should resource version.

(so this shouldn't matter)

lavalamp · 2017-05-09T17:49:52Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go

+	// 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


...but should a status update ever change it? I think not.

lavalamp · 2017-05-09T17:52:15Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers_test.go

+	existingMeta.SetClusterName("")
+	existingMeta.SetCreationTimestamp(Time{})
+	existingMeta.SetDeletionTimestamp(nil)
+	existingMeta.SetDeletionGracePeriodSeconds(nil)


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?

lavalamp · 2017-05-09T17:53:00Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go

+	meta.SetSelfLink(existingMeta.GetSelfLink())
+	meta.SetLabels(existingMeta.GetLabels())
+	meta.SetAnnotations(existingMeta.GetAnnotations())
+	meta.SetFinalizers(existingMeta.GetFinalizers())


Does it ever make sense to remove a finalizer and change status in the same request? I think the answer is probably yes?

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.

lavalamp · 2017-05-09T17:54:50Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go

+	meta.SetGeneration(existingMeta.GetGeneration())
+	meta.SetSelfLink(existingMeta.GetSelfLink())
+	meta.SetLabels(existingMeta.GetLabels())
+	meta.SetAnnotations(existingMeta.GetAnnotations())


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.

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.

lavalamp · 2017-05-09T17:58:07Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go

+func ResetObjectMetaForStatus(meta, existingMeta Object) {
+	meta.SetGeneration(existingMeta.GetGeneration())
+	meta.SetSelfLink(existingMeta.GetSelfLink())
+	meta.SetLabels(existingMeta.GetLabels())


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?

lavalamp · 2017-05-09T17:59:38Z

staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/helpers.go

+	meta.SetLabels(existingMeta.GetLabels())
+	meta.SetAnnotations(existingMeta.GetAnnotations())
+	meta.SetFinalizers(existingMeta.GetFinalizers())
+	meta.SetOwnerReferences(existingMeta.GetOwnerReferences())


Does it make sense to change ownership at the same time as a status update? I think that might make sense for some controllers.

deads2k · 2017-05-09T18:23:44Z

I'm not sure that now is a great time to push for status & spec separation in the storage layer. Is it blocking something?

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.

deads2k · 2017-05-09T18:29:29Z

@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.

sttts · 2017-05-10T09:29:38Z

staging/src/k8s.io/kube-aggregator/pkg/registry/apiservice/strategy.go

-	newAPIService.Finalizers = oldAPIService.Finalizers
-	newAPIService.OwnerReferences = oldAPIService.OwnerReferences
+
+	metav1.ResetObjectMetaForStatus(newAPIService, oldAPIService)


Shouldn't we actually test for modification in ValidateUpdate instread of overriding/resetting invalid field updates?

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.

@derekwaynecarr

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

smarterclayton · 2017-06-15T15:25:28Z

So we would only ever use this on new v2 APIs and new resource groups, because otherwise this is a breaking API change?

deads2k · 2017-06-15T15:38:03Z

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.

smarterclayton · 2017-06-15T16:43:52Z

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.

deads2k · 2017-06-15T16:55:28Z

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.

k8s-github-robot · 2017-07-13T09:21:27Z

@deads2k PR needs rebase

bgrant0607 · 2017-08-23T04:50:48Z

Please see #20978.

k8s-github-robot · 2017-11-21T08:13:14Z

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

nikhita · 2018-01-24T14:36:43Z

@deads2k Looks like this PR was closed due to age. But from your comment here, I'm guessing we still want something like this?

deads2k · 2018-01-24T14:57:26Z

@deads2k Looks like this PR was closed due to age. But from your comment here, I'm guessing we still want something like this?

I think that new resources will want it and CRs definitely want it.

nikhita · 2018-01-30T11:57:00Z

I'd accept "new resources only" as a starting point.

Take two: #59038

add safe way to prevent status from touching objectmeta

1681bfd

deads2k assigned lavalamp and smarterclayton May 9, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 9, 2017

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels May 9, 2017

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 9, 2017

lavalamp reviewed May 9, 2017

View reviewed changes

sttts reviewed May 10, 2017

View reviewed changes

This was referenced May 15, 2017

OwnerReferencesPermissionEnforcement ignores pods/status #45747

Merged

prevent pods/status from touching ownerreferences #45826

Merged

deads2k mentioned this pull request May 26, 2017

Copy GenerateName for resource quota while updating its status. #46492

Closed

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 5, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2017

liggitt mentioned this pull request Aug 7, 2017

api-conventions: status subresource updates metadata kubernetes/community#882

Closed

nikhita mentioned this pull request Aug 17, 2017

apiextensions: update CRD strategy #50764

Merged

k8s-github-robot closed this Nov 21, 2017

nikhita mentioned this pull request Jan 30, 2018

Add safe way to prevent status from touching objectmeta #59038

Closed

nikhita mentioned this pull request Jan 30, 2018

sample-apiserver: add status subresource for Flunder #58305

Closed

deads2k deleted the api-10-status-reset branch July 3, 2018 18:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add safe way to prevent status from touching objectmeta #45552

add safe way to prevent status from touching objectmeta #45552

deads2k commented May 9, 2017 •

edited by k8s-github-robot

Loading

k8s-reviewable commented May 9, 2017

k8s-github-robot commented May 9, 2017

deads2k commented May 9, 2017

deads2k commented May 9, 2017

lavalamp left a comment

lavalamp May 9, 2017

lavalamp May 9, 2017

lavalamp May 9, 2017

lavalamp May 9, 2017

lavalamp May 9, 2017

lavalamp May 9, 2017

deads2k May 9, 2017

lavalamp May 9, 2017

deads2k May 9, 2017

lavalamp May 9, 2017

lavalamp May 9, 2017

deads2k commented May 9, 2017

deads2k commented May 9, 2017

sttts May 10, 2017

deads2k May 10, 2017

smarterclayton commented Jun 15, 2017

deads2k commented Jun 15, 2017

smarterclayton commented Jun 15, 2017

deads2k commented Jun 15, 2017

k8s-github-robot commented Jul 13, 2017

bgrant0607 commented Aug 23, 2017

k8s-github-robot commented Nov 21, 2017

nikhita commented Jan 24, 2018

deads2k commented Jan 24, 2018

nikhita commented Jan 30, 2018


		ResetObjectMetaForStatus(meta, existingMeta)

		// not all fields are stomped during the reset. These fields should not have been set.false

add safe way to prevent status from touching objectmeta #45552

add safe way to prevent status from touching objectmeta #45552

Conversation

deads2k commented May 9, 2017 • edited by k8s-github-robot Loading

k8s-reviewable commented May 9, 2017

k8s-github-robot commented May 9, 2017

deads2k commented May 9, 2017

deads2k commented May 9, 2017

lavalamp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 9, 2017

deads2k commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jun 15, 2017

deads2k commented Jun 15, 2017

smarterclayton commented Jun 15, 2017

deads2k commented Jun 15, 2017

k8s-github-robot commented Jul 13, 2017

bgrant0607 commented Aug 23, 2017

k8s-github-robot commented Nov 21, 2017

nikhita commented Jan 24, 2018

deads2k commented Jan 24, 2018

nikhita commented Jan 30, 2018

deads2k commented May 9, 2017 •

edited by k8s-github-robot

Loading