Error on manipulation for cluster services / add-ons #17740

janetkuo · 2015-11-24T21:41:11Z

Stops the users from manipulating cluster services (add-ons, i.e. resources with the label kubernetes.io/cluster-service: "true") in kubectl.

For example, if we have an rc "addon" with such label:

$ kubectl delete rc addon
error: "addon" is a cluster service and should not be mutated. See http://releases.k8s.io/HEAD/cluster/addons for more info.
$ kubectl rolling-update rc addon -f addon2.yaml
error: replication controller "addon" is a cluster service and should not be mutated. See http://releases.k8s.io/HEAD/cluster/addons for more info.

We only do this check when delete, stop, rolling-update, and replace --force (which does delete) a resource.

For testing purpose, added a flag --write-protect[=true] to bypass the check if set to false.

cc @kubernetes/kubectl @kubernetes/goog-ux @nikhiljindal @erictune

This change is

janetkuo · 2015-11-24T21:45:42Z

Should we have a cmd flag that allows the user to bypass this check? Something like --overwrite-addon=true, or --addon-check=false? In this PR, if you create a resource with that label manually (or edit/label a resource with that label), you can never update/delete it.

j3ffml · 2015-11-25T21:37:13Z

I think we probably should have a way to bypass it, since we may want to for testing purposes. Case in point, #17614.

janetkuo · 2015-11-25T23:44:29Z

I added a flag --addon-check for bypassing it. Added tests in hack/test-cmd.sh too.

smarterclayton · 2015-11-26T00:41:16Z

Couple of design points:

Why are services special? Why isn't this kubernetes.io/cluster-resource:
true or kubernetes.io/delete-protection: true and it just can apply to all
resources? And then we use --confirm to override delete protection?

We have several types of auto resources - endpoints, services, secrets,
service accounts, namespaces which all need this.

I don't think this is specific to cluster scoped resources.

smarterclayton · 2015-11-26T01:47:01Z

@liggitt this would solve several deletion related problems.

liggitt · 2015-11-26T01:51:41Z

pkg/kubectl/cmd/util/helpers.go

+
+// AddMutateClusterServiceFlag adds a flag that prevents the manipulation on add-ons. Used by mutations only.
+func AddMutateClusterServiceFlag(cmd *cobra.Command) {
+	cmd.Flags().BoolP("addon-check", "", true, "If true, prevents the users from manipulating add-ons / cluster services. See http://releases.k8s.io/HEAD/cluster/addons for more info. Default true.")


deletion protection seems like something we'd want in general, and this seems like a too-specific application of it... can we make this flag more general?

liggitt · 2015-11-26T01:52:53Z

Why are services special? Why isn't this kubernetes.io/cluster-resource: true or kubernetes.io/delete-protection: true and it just can apply to all resources? And then we use --confirm to override delete protection?

That requires a GET before DELETE, which at the very least doubles requests. In other cases, someone could be allowed to delete something they cannot get... what would you do with that?

smarterclayton · 2015-11-26T15:34:40Z

Go ahead and do the delete. If the admin wants you to blindly delete, you
can blindly delete.

On Nov 25, 2015, at 9:07 PM, Jordan Liggitt notifications@github.com
wrote:

Why are services special? Why isn't this kubernetes.io/cluster-resource:
true or kubernetes.io/delete-protection: true and it just can apply to all
resources? And then we use --confirm to override delete protection?

That requires a GET before DELETE, which at the very least doubles
requests. In other cases, someone could be allowed to delete something they
cannot get... what would you do with that?

—
Reply to this email directly or view it on GitHub
#17740 (comment)
.

janetkuo · 2015-11-30T22:45:25Z

Changed the flag from --addon-check to --write-protect:

--write-protect[=true]: If true, prevents the users from manipulating resources that shouldn't be changed, such as add-ons and auto-generated resources. Note that changing this flag may cause problems to your cluster. Default true.

janetkuo · 2015-12-03T18:57:10Z

Rebased. @jlowdermilk @smarterclayton @liggitt PTAL.

liggitt · 2015-12-03T19:09:49Z

pkg/kubectl/cmd/delete_test.go

@@ -70,7 +70,7 @@ func TestDeleteNamedObject(t *testing.T) {
 		Codec: codec,
 		Client: fake.CreateHTTPClient(func(req *http.Request) (*http.Response, error) {
 			switch p, m := req.URL.Path, req.Method; {
-			case p == "/namespaces/test/replicationcontrollers/redis-master-controller" && m == "DELETE":
+			case p == "/namespaces/test/replicationcontrollers/redis-master-controller" && (m == "DELETE" || m == "GET"):
 				return &http.Response{StatusCode: 200, Body: objBody(codec, &rc.Items[0])}, nil
 			default:
 				// Ensures no GET is performed when deleting by name


these tests were explicitly here to make sure deletes succeed when GET fails (the case where a user is allowed to DELETE something they cannot GET)... we still need to test those scenarios.

so I think we need the following tests:

GET succeeds, and is followed by DELETE

GET returns 404... not sure if DELETE should be performed or not

GET returns 403, and is followed by DELETE, which succeeds

Comment still applies

Originally I added GET here because I needed to GET a resource to check if it's a cluster service (check its label) before deleting it.

I understand, but we need tests that ensure a DELETE can succeed even when the GET does not

janetkuo · 2015-12-18T06:52:55Z

Just rebased. Ping for another look.

liggitt · 2015-12-18T08:14:25Z

It doesn't look like my last set of comments were addressed. Please take another look

janetkuo · 2015-12-18T08:18:36Z

@liggitt I missed your comments. Will update accordingly.

nikhiljindal · 2016-01-07T22:10:04Z

Ping!
Another user on slack mentioned being tripped on this.

janetkuo · 2016-01-08T00:10:01Z

@liggitt I'm not sure if I understand you correctly. If we're making a new annotation, say kubernetes.io/write-protect=true, to mark resources as write-protected, how does that solve issue #17532? The users will need to add kubernetes.io/write-protect=true to their cluster services (i.e., resources with kubernetes.io/cluster-service=true label) to avoid mutating cluster services accidentally. But the users who ran into this problem didn't know that they shouldn't manipulate cluster services.

0xmichalis · 2016-01-08T10:32:07Z

pkg/kubectl/cmd/stop.go

 	cmdutil.AddOutputFlagsForMutation(cmd)
+	cmdutil.AddWriteProtectFlag(cmd)


stop is deprecated, should we update it?

0xmichalis · 2016-01-13T12:00:59Z

Needs a rebase

liggitt · 2016-01-29T01:20:11Z

@liggitt I'm not sure if I understand you correctly. If we're making a new annotation, say kubernetes.io/write-protect=true, to mark resources as write-protected, how does that solve issue #17532? The users will need to add kubernetes.io/write-protect=true to their cluster services (i.e., resources with kubernetes.io/cluster-service=true label) to avoid mutating cluster services accidentally.

I'm saying we should additionally set the kubernetes.io/write-protect=true annotation on cluster-services to gain this behavior. Hints to clients to write protect something is a generally useful thing, independent of whether something is a cluster-service.

k8s-bot · 2016-05-05T15:58:22Z

GCE e2e build/test failed for commit c5c557e.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

smarterclayton · 2016-05-06T14:47:11Z

I think we want the general annotation, because it aligns with a bunch of other author -> user communication we will want. We have use cases for a "do not reconcile automatically" annotation that I would like to make kubernetes.io/reconcile-protect that would potentially apply to kubectl apply. We may have a distinct delete protection in the future.

k8s-github-robot · 2016-08-04T14:59:28Z

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 @janetkuo @kargakis

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

janetkuo added kind/enhancement area/kubectl labels Nov 24, 2015

googlebot added the cla: yes label Nov 24, 2015

k8s-github-robot assigned j3ffml Nov 24, 2015

k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 24, 2015

janetkuo force-pushed the manipulate-cluster-services branch from 7946079 to 99591f3 Compare November 24, 2015 22:15

janetkuo force-pushed the manipulate-cluster-services branch from 99591f3 to 00640d0 Compare November 25, 2015 23:40

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 Nov 25, 2015

janetkuo force-pushed the manipulate-cluster-services branch from 00640d0 to 786d98e Compare November 25, 2015 23:50

liggitt reviewed Nov 26, 2015
View reviewed changes

janetkuo force-pushed the manipulate-cluster-services branch 2 times, most recently from ac47c28 to e6bbd32 Compare November 30, 2015 22:35

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 30, 2015

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

janetkuo force-pushed the manipulate-cluster-services branch from e6bbd32 to 6a1ecbf Compare December 3, 2015 18:56

janetkuo removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2015

liggitt reviewed Dec 3, 2015
View reviewed changes

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

janetkuo added 2 commits December 17, 2015 22:28

Error on manipulation for cluster services / add-ons

e7efd1b

Address comments

c5c557e

janetkuo force-pushed the manipulate-cluster-services branch from 6a1ecbf to c5c557e Compare December 18, 2015 06:47

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 18, 2015

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

0xmichalis reviewed Jan 8, 2016
View reviewed changes

j3ffml assigned 0xmichalis and unassigned j3ffml Jan 8, 2016

liggitt mentioned this pull request Jan 29, 2016

"kubectl delete namespace --all" always exits with failure #18282

Closed

k8s-github-robot added the kind/old-docs label Mar 1, 2016

vbehar mentioned this pull request Mar 16, 2016

confirmation before project deletion openshift/origin#8038

Closed

k8s-github-robot added the release-note-label-needed label Mar 31, 2016

liggitt mentioned this pull request Apr 10, 2016

Externalize immortal namespaces #23883

Merged

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 Jul 18, 2016

k8s-github-robot closed this Aug 4, 2016

soltysh mentioned this pull request Apr 10, 2018

Add validation when removing cluster-scope resources and providing namespace #62167

Closed

juanvallejo mentioned this pull request Sep 20, 2018

Add --cascade or --force flag to kubectl delete namespace, for non-empty namespaces kubernetes/kubectl#525

Closed

eddiezane mentioned this pull request Sep 16, 2020

--all should ask for confirmation kubernetes/kubectl#921

Closed

Error on manipulation for cluster services / add-ons #17740

Error on manipulation for cluster services / add-ons #17740

janetkuo commented Nov 24, 2015 •

edited by k8s-github-robot

janetkuo commented Nov 24, 2015

j3ffml commented Nov 25, 2015

janetkuo commented Nov 25, 2015

smarterclayton commented Nov 26, 2015

smarterclayton commented Nov 26, 2015

liggitt Nov 26, 2015

0xmichalis Nov 26, 2015

liggitt commented Nov 26, 2015

smarterclayton commented Nov 26, 2015

janetkuo commented Nov 30, 2015

janetkuo commented Dec 3, 2015

liggitt Dec 3, 2015

liggitt Dec 3, 2015

liggitt Dec 18, 2015

janetkuo Dec 18, 2015

liggitt Dec 18, 2015

janetkuo commented Dec 18, 2015

liggitt commented Dec 18, 2015

janetkuo commented Dec 18, 2015

nikhiljindal commented Jan 7, 2016

janetkuo commented Jan 8, 2016

0xmichalis Jan 8, 2016

0xmichalis commented Jan 13, 2016

liggitt commented Jan 29, 2016

k8s-bot commented May 5, 2016

smarterclayton commented May 6, 2016

k8s-github-robot commented Aug 4, 2016

		cmdutil.AddOutputFlagsForMutation(cmd)
		cmdutil.AddWriteProtectFlag(cmd)

Error on manipulation for cluster services / add-ons #17740

Error on manipulation for cluster services / add-ons #17740

Conversation

janetkuo commented Nov 24, 2015 • edited by k8s-github-robot

janetkuo commented Nov 24, 2015

j3ffml commented Nov 25, 2015

janetkuo commented Nov 25, 2015

smarterclayton commented Nov 26, 2015

smarterclayton commented Nov 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Nov 26, 2015

smarterclayton commented Nov 26, 2015

janetkuo commented Nov 30, 2015

janetkuo commented Dec 3, 2015

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

janetkuo commented Dec 18, 2015

liggitt commented Dec 18, 2015

janetkuo commented Dec 18, 2015

nikhiljindal commented Jan 7, 2016

janetkuo commented Jan 8, 2016

Choose a reason for hiding this comment

0xmichalis commented Jan 13, 2016

liggitt commented Jan 29, 2016

k8s-bot commented May 5, 2016

smarterclayton commented May 6, 2016

k8s-github-robot commented Aug 4, 2016

janetkuo commented Nov 24, 2015 •

edited by k8s-github-robot