Make selector immutable for v1beta2 deployment, replicaset and daemonset prior update #50719

crimsonfaith91 · 2017-08-15T23:41:49Z

What this PR does / why we need it:
This PR ensures controller selector is immutable for deployment and replicaset prior update by ignoring any change to Spec.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #50808

Special notes for your reviewer:
This will be a breaking change.

Release note:

For Deployment, ReplicaSet, and DaemonSet, selectors are now immutable when updating via the new `apps/v1beta2` API. For backward compatibility, selectors can still be changed when updating via `apps/v1beta1` or `extensions/v1beta1`.

crimsonfaith91 · 2017-08-15T23:44:27Z

pkg/registry/extensions/deployment/strategy.go

+	// We assume API version is not v1beta2 if no RequestInfo is found
+	requestInfo, found := genericapirequest.RequestInfoFrom(ctx)
+	if found && requestInfo.APIVersion == "v1beta2" {
+		newDeployment.Spec = oldDeployment.Spec


@kow3ns @enisoc Perhaps newDeployment.Spec.Selector = oldDeployment.Spec.Selector is better. Any thought?

crimsonfaith91 · 2017-08-15T23:46:08Z

pkg/registry/extensions/deployment/strategy.go

+	// update is not allowed to set spec for v1beta2
+	// We assume API version is not v1beta2 if no RequestInfo is found
+	requestInfo, found := genericapirequest.RequestInfoFrom(ctx)
+	if found && requestInfo.APIVersion == "v1beta2" {


For now, it affects only v1beta2 API version. In future, we may need to change it to v1 or have a range like greater than v1.

kow3ns

You can't clear the entire spec, this disallows all relevant changes to the API object. You might be able to clear the selector change. Is this the method that you discussed with @erictune?

crimsonfaith91 · 2017-08-16T17:15:19Z

@kow3ns Yes, it is one of the ways suggested by Eric. I am not sure we should ignore all changes to Spec (this is what StatusUpdate does), and wish to get feedback before writing unit tests. I also suggested newDeployment.Spec.Selector = oldDeployment.Spec.Selector in another comment.

crimsonfaith91 · 2017-08-16T20:17:54Z

pkg/registry/extensions/deployment/strategy.go

+	// update is not allowed to set Spec.Selector for v1beta2
+	// we assume it is not v1beta2 if no API group and/or version is found in RequestInfo
+	// TODO(#50791): after v1beta1 is retired, move selector immutability check to validation codes
+	requestInfo, found := genericapirequest.RequestInfoFrom(ctx)


@erictune @kow3ns As discussed by person, i added a TODO for moving the check to validation codes after v1beta1 retires, and included the API group in the condition.

crimsonfaith91 · 2017-08-17T02:37:00Z

pkg/registry/extensions/daemonset/strategy_test.go

+		if err != nil {
+			t.Errorf("Unexpected error for url: %s %v", test.url, err)
+		}
+		if !apiRequestInfo.IsResourceRequest {


@kow3ns
After writing the unit test, i think we can leave out the API group when determining whether to ignore changes to Spec.Selector. The main reason is due to how RequestInfo works.

IsResourceRequest returns true for requests to API resources, like /api/v1/..., and false for non-resource endpoints like /api, /healthz, and /swaggerapi. In other words, something like /api/v1beta2/... works, but not /api/apps/v1beta2/... (additional apps after api) or /apps/v1beta2/... (do not start with api).

Furthermore, we will have the silent ignore for v1 in future, which will be /api/v1/... that works perfectly with how RequestInfo works. Moreover, v1beta2 only exists for apps group, which does not justify checking the API group.

WDYT?

These controllers never existed in the groupless /api/ prefix, so your test shouldn't expect those inputs to work. Specifically, you want to make sure that /apis/extensions/v1beta1 allows mutation, and /apis/apps/v1beta2 does not. If we decide to plan ahead for /apis/apps/v1 you could test for that too, but I don't know if that's decided yet.

@enisoc Good point! Thanks for the clarification. I will test for /apis/extensions/v1beta1 and /apis/apps/v1beta2.

crimsonfaith91 · 2017-08-17T02:38:22Z

pkg/registry/extensions/daemonset/strategy.go

+	// we assume it is not v1beta2 if no API version is found in RequestInfo
+	// TODO(#50791): after v1beta1 is retired, move selector immutability check to validation codes
+	requestInfo, found := genericapirequest.RequestInfoFrom(ctx)
+	if found && requestInfo.APIVersion == "v1beta2" {


Refer this comment for why checking API group may be unnecessary.

…r update

liggitt · 2017-08-30T20:15:24Z

this is definitely worth a release note

liggitt · 2017-08-30T20:19:43Z

/lgtm
though I am not an approver on pkg/registry

smarterclayton · 2017-08-31T14:08:14Z

/approve

Based on discussion in linked issue and general agreement that we can potentially relax / add better changes in the future.

k8s-github-robot · 2017-08-31T14:08:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: crimsonfaith91, enisoc, liggitt, smarterclayton

Associated issue: 50808

The full list of commands accepted by this bot can be found here.

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

~~pkg/registry/OWNERS~~ [smarterclayton]
~~test/OWNERS~~ [enisoc,liggitt,smarterclayton]

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

k8s-github-robot · 2017-09-01T04:09:07Z

Automatic merge from submit-queue (batch tested with PRs 50719, 51216, 50212, 51408, 51381)

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

k8s-ci-robot assigned enisoc and kow3ns Aug 15, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 15, 2017

crimsonfaith91 changed the title ~~Make selector immutable for deployment and replicaset prior update~~ Make selector immutable for v1beta2 deployment and replicaset prior update Aug 15, 2017

crimsonfaith91 commented Aug 15, 2017

View reviewed changes

k8s-ci-robot unassigned enisoc and kow3ns Aug 16, 2017

k8s-ci-robot requested review from enisoc and kow3ns August 16, 2017 00:20

k8s-github-robot assigned mbohlool and deads2k Aug 16, 2017

k8s-ci-robot assigned enisoc and kow3ns and unassigned mbohlool and deads2k Aug 16, 2017

kow3ns suggested changes Aug 16, 2017

View reviewed changes

crimsonfaith91 force-pushed the immutability branch from 950355e to 7ac5c2f Compare August 16, 2017 20:16

crimsonfaith91 commented Aug 16, 2017

View reviewed changes

crimsonfaith91 force-pushed the immutability branch from 7ac5c2f to 387b01a Compare August 16, 2017 22:03

crimsonfaith91 changed the title ~~Make selector immutable for v1beta2 deployment and replicaset prior update~~ Make selector immutable for v1beta2 deployment, replicaset and daemonset prior update Aug 16, 2017

crimsonfaith91 force-pushed the immutability branch from 387b01a to f625dff Compare August 17, 2017 02:12

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 17, 2017

crimsonfaith91 commented Aug 17, 2017

View reviewed changes

crimsonfaith91 mentioned this pull request Aug 17, 2017

Controller selectors should be immutable after creation #50808

Closed

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

ignore selector changes for deployment, replicaset and daemonset prio…

9929e03

…r update

crimsonfaith91 force-pushed the immutability branch from fdc9a9b to 9929e03 Compare August 30, 2017 20:16

enisoc added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Aug 30, 2017

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

k8s-ci-robot assigned liggitt Aug 30, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017

k8s-ci-robot assigned smarterclayton Aug 30, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017

xilabao mentioned this pull request Sep 1, 2017

dispose of labels with the beta key #51756

Closed

k8s-github-robot merged commit 9a3dfbc into kubernetes:master Sep 1, 2017

liggitt mentioned this pull request Sep 18, 2017

kubectl apply deployment -f doesn't accept label/selector changes #26202

Closed

rphillips mentioned this pull request Sep 18, 2017

WIP: bump k8s to 1.8.0-beta.1 kubernetes-retired/bootkube#709

Closed

1 task

diegs mentioned this pull request Oct 10, 2017

modules/*: bump to k8s v1.8.2 coreos/tectonic-installer#1938

Merged

bacongobbler mentioned this pull request Oct 31, 2017

Upgrade fails when deployment.spect.template has a chart label helm/helm#2494

Closed

enisoc mentioned this pull request Nov 2, 2017

[apps/v1] Change DefaultGarbageCollectionPolicy for workload controllers #55027

Closed

2 tasks

This was referenced Sep 11, 2018

test desaintmartin/WhaTV#6

Closed

Putting chart version in spec.selector.matchLabels in apps/v1beta2 and later prevents ANY future upgrade of existing Release helm/charts#7680

Closed

klingerf mentioned this pull request Oct 15, 2018

Proxy injector must preserve the original pod template labels and annotations linkerd/linkerd2#1765

Merged

ayj mentioned this pull request Oct 30, 2018

fix 1.0.3 => 1.1.0 upgrade with default helm options istio/istio#9569

Merged

guangbochen mentioned this pull request May 29, 2019

Add ability to specify "upgrade from" in the catalog to specify which apps can be upgrade from (like in 1.6) rancher/rancher#20523

Closed

This was referenced Oct 3, 2019

error when deploying istio kubeflow/manifests#420

Closed

error when deploying istio #420 kubeflow/manifests#421

Closed

carbonin mentioned this pull request Jun 17, 2020

Deployment selector is immutable from the operator ManageIQ/manageiq-pods#550

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make selector immutable for v1beta2 deployment, replicaset and daemonset prior update #50719

Make selector immutable for v1beta2 deployment, replicaset and daemonset prior update #50719

crimsonfaith91 commented Aug 15, 2017 •

edited by enisoc

Loading

crimsonfaith91 Aug 15, 2017 •

edited

Loading

crimsonfaith91 Aug 15, 2017 •

edited

Loading

kow3ns left a comment

crimsonfaith91 commented Aug 16, 2017 •

edited

Loading

crimsonfaith91 Aug 16, 2017

crimsonfaith91 Aug 17, 2017 •

edited

Loading

enisoc Aug 17, 2017

crimsonfaith91 Aug 17, 2017

crimsonfaith91 Aug 17, 2017

liggitt commented Aug 30, 2017

liggitt commented Aug 30, 2017

smarterclayton commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

k8s-github-robot commented Sep 1, 2017

Make selector immutable for v1beta2 deployment, replicaset and daemonset prior update #50719

Make selector immutable for v1beta2 deployment, replicaset and daemonset prior update #50719

Conversation

crimsonfaith91 commented Aug 15, 2017 • edited by enisoc Loading

crimsonfaith91 Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

crimsonfaith91 Aug 15, 2017 • edited Loading

Choose a reason for hiding this comment

kow3ns left a comment

Choose a reason for hiding this comment

crimsonfaith91 commented Aug 16, 2017 • edited Loading

crimsonfaith91 Aug 16, 2017

Choose a reason for hiding this comment

crimsonfaith91 Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

enisoc Aug 17, 2017

Choose a reason for hiding this comment

crimsonfaith91 Aug 17, 2017

Choose a reason for hiding this comment

crimsonfaith91 Aug 17, 2017

Choose a reason for hiding this comment

liggitt commented Aug 30, 2017

liggitt commented Aug 30, 2017

smarterclayton commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

k8s-github-robot commented Sep 1, 2017

crimsonfaith91 commented Aug 15, 2017 •

edited by enisoc

Loading

crimsonfaith91 Aug 15, 2017 •

edited

Loading

crimsonfaith91 Aug 15, 2017 •

edited

Loading

crimsonfaith91 commented Aug 16, 2017 •

edited

Loading

crimsonfaith91 Aug 17, 2017 •

edited

Loading