Add namespace for (cluster)role(binding) cloud-provider. #67224

grayluck · 2018-08-10T00:42:39Z

What this PR does / why we need it:
Add namespace for (cluster)role(binding) cloud-provider.
Change the addonmanager mode to be from reconcile to EnsureExists.

Needs to be cherrypicked together with #59686.

Special notes for your reviewer:
/assign @bowei @tallclair
/sig auth

Release note:

Role, ClusterRole and their bindings for cloud-provider is put under system namespace. Their addonmanager mode switches to EnsureExists.

Manual tested. Cluster can be created succesfully using kube-up.sh with desired (cluster)role(binding)s.

tallclair · 2018-08-10T01:42:42Z

This is unreleased code, right?

grayluck · 2018-08-10T17:46:03Z

@tallclair PR#59686 is released in 1.10 and 1.11, confirmably. So it's already affecting some of the clusters by not namespacing the roles.
It's fine to have two sets of (cluster)roles and (cluster)rolebindings if those affected clusters get upgraded.

bowei · 2018-08-10T17:50:03Z

/ok-to-test

bowei · 2018-08-10T17:50:18Z

make sure you test with all of the versions

k8s-ci-robot · 2018-08-10T22:41:47Z

@grayluck:

In response to this:

/meow

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

grayluck · 2018-08-10T22:42:40Z

/undo meow

tallclair · 2018-08-13T18:38:08Z

What does the addon manager do with the old roles in this case? You can test it by SSHing to the master, and manually swapping out the manifest (/etc/kubernetes/manifests, I think).

grayluck · 2018-08-13T22:30:09Z

Here's what happened:

e2e-test-yankaiz-master loadbalancing # alias kk="kubectl -n kube-system"

e2e-test-yankaiz-master loadbalancing # kk get clusterrolebindings system:cloud-provider gce:cloud-provider
NAME                    KIND
system:cloud-provider   ClusterRoleBinding.v1.rbac.authorization.k8s.io
gce:cloud-provider      ClusterRoleBinding.v1.rbac.authorization.k8s.io

e2e-test-yankaiz-master loadbalancing # kk get clusterroles system:cloud-provider gce:cloud-provider
NAME                    KIND
system:cloud-provider   ClusterRole.v1.rbac.authorization.k8s.io
gce:cloud-provider      ClusterRole.v1.rbac.authorization.k8s.io

e2e-test-yankaiz-master loadbalancing # kk auth can-i create events --as=system:serviceaccount:kube-system:cloud-provider
yes

Everything looks working perfectly.

grayluck · 2018-08-21T18:16:21Z

Sure. I will test the rest of the versions in cherrypicks.

tallclair · 2018-08-21T22:57:34Z

I thought we discussed using the gce: prefix instead of system:, but can't find the reference...
@mikedanese

grayluck · 2018-08-21T23:01:38Z

I forgot to push my local change.. But the test still shows that gce:cloud-provider works with previous roles/bindings.

tallclair · 2018-08-22T00:39:11Z

/lgtm

tallclair · 2018-08-22T00:39:23Z

/retest

grayluck · 2018-08-24T00:46:02Z

/hold cancel

tallclair · 2018-08-24T00:49:36Z

cluster/gce/addons/loadbalancing/cloud-provider-binding.yaml

+# DEPRECATED: cloud-provider roles and bindings are deprecated. Use gce:cloud-provider instead.
+apiVersion: rbac.authorization.k8s.io/v1
+kind: RoleBinding
+metadata:


We discussed putting the deprecation notice in an annotation, and keeping Reconcile so the annotation is populated.

Sorry Tim I can't find an example annotation to denote deprecation. But I did find an related (closed without fixes) issue: #38269.

tallclair · 2018-08-24T00:50:53Z

cluster/gce/addons/loadbalancing/cloud-provider-binding.yaml

@@ -18,7 +50,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRoleBinding


Actually, I'm not sure there's any reason we need to keep the old ClusterRoleBinding. The new one grants a superset of the permissions.

I think we just need to keep the old cluster role incase someone was binding it to a different service account.

lol Good point. No, there's no reason to keep the bindings. Only the old roles need to be remained.

tallclair · 2018-08-24T00:51:33Z

cluster/gce/addons/loadbalancing/cloud-provider-binding.yaml

+kind: ClusterRoleBinding
+metadata:
+  labels:
+    addonmanager.kubernetes.io/mode: Reconcile


Techinically there's a slight risk to this, as it would clobber another role of the same name. I think it's probably ok given the gce: prefix.

It's not that likely that a cluster user has a role named gce:cloud-provider. Since we didn't get any reports reporting issue about roles named cloud-provider in Reconcile mode clobbering exist roles, I assume gce:cloud-provider is even safer than cloud-provider to have Reconcile.

bowei · 2018-08-24T19:24:10Z

where is the change to remove the old role binding?

grayluck · 2018-08-24T20:07:58Z

Re bowei@ The old bindings has been removed from cloud-provider-binding.yaml.
Talked with Tim for advice about deprecation annotations.
Added to the old (cluster)role.
PTAL Misters

grayluck · 2018-08-27T21:42:08Z

/test pull-kubernetes-e2e-gke

tallclair · 2018-08-28T03:33:22Z

cluster/gce/addons/loadbalancing/cloud-provider-role.yaml

+  annotations:
+    kubernetes.io/deprecation: 'cloud-provider role is DEPRECATED in the
+      concern of potential collisions. Do not refer to this role. Use
+      gce:cloud-provider instead.'


nit: We don't really want anyone to use the gce:cloud-provider role either... in generaly I would say that things in the kube-system namespace should not be used. I guess this is more of a concern with the cluster-scoped role, but the same holds true.

Also, consider declaring what release this will be removed in (probably 1.14)

Let's do 1.16 since the minor comes every quarter. This gives the deprecated role 1 year before removed.

Change the addonmanager mode to be from reconcile to EnsureExists.

tallclair · 2018-08-28T03:59:07Z

/lgtm
thanks!

grayluck · 2018-08-30T22:08:51Z

/test pull-kubernetes-e2e-gke

bowei · 2018-08-30T23:23:12Z

/lgtm
/approve

k8s-ci-robot · 2018-08-30T23:23:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, grayluck, tallclair

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~cluster/gce/OWNERS~~ [bowei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

fejta-bot · 2018-08-31T01:34:49Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

fejta-bot · 2018-08-31T05:04:48Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

bowei · 2018-08-31T23:43:53Z

/milestone v1.12

grayluck · 2018-08-31T23:56:55Z

Thanks Bowei!

k8s-github-robot · 2018-09-01T02:25:31Z

Automatic merge from submit-queue (batch tested with PRs 65251, 67255, 67224, 67297, 68105). If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

…24-upstream-release-1.10 Automated cherry pick of #67224: Add namespace for (cluster)role(binding) cloud-provider.

…86-upstream-release-1.9 Cherry pick of #59686 #67224: Add cloud-provider policies to be applied via addon mgr

…24-upstream-release-1.11 Automated cherry pick of #67224: Add namespace for (cluster)role(binding) cloud-provider.

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 10, 2018

k8s-ci-robot assigned bowei and tallclair Aug 10, 2018

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 10, 2018

k8s-ci-robot requested review from mwielgus and vishh August 10, 2018 00:42

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 10, 2018

grayluck mentioned this pull request Aug 10, 2018

Cherry pick of #59686 #67224: Add cloud-provider policies to be applied via addon mgr #66872

Merged

k8s-ci-robot 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 10, 2018

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 10, 2018

grayluck force-pushed the namespace-cloudprovider-rbac branch from ab59f7e to f4a2e99 Compare August 21, 2018 23:00

k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 21, 2018

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

tallclair closed this Aug 22, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2018

tallclair reviewed Aug 24, 2018

View reviewed changes

grayluck force-pushed the namespace-cloudprovider-rbac branch 2 times, most recently from 6edeeed to 008994b Compare August 24, 2018 18:50

grayluck force-pushed the namespace-cloudprovider-rbac branch from 008994b to a528459 Compare August 24, 2018 20:02

tallclair reviewed Aug 28, 2018

View reviewed changes

Add namespace for (cluster)role(binding) cloud-provider.

bea625f

Change the addonmanager mode to be from reconcile to EnsureExists.

grayluck force-pushed the namespace-cloudprovider-rbac branch from a528459 to bea625f Compare August 28, 2018 03:47

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2018

k8s-ci-robot added this to the v1.12 milestone Aug 31, 2018

k8s-github-robot merged commit 6900a80 into kubernetes:master Sep 1, 2018

This was referenced Sep 5, 2018

Automated cherry pick of #67224: Add namespace for (cluster)role(binding) cloud-provider. #68258

Merged

Automated cherry pick of #67224: Add namespace for (cluster)role(binding) cloud-provider. #68259

Merged

k8s-ci-robot added a commit that referenced this pull request Sep 11, 2018

Merge pull request #68258 from grayluck/automated-cherry-pick-of-#672…

6d34b42

…24-upstream-release-1.10 Automated cherry pick of #67224: Add namespace for (cluster)role(binding) cloud-provider.

k8s-ci-robot added a commit that referenced this pull request Sep 18, 2018

Merge pull request #66872 from grayluck/automated-cherry-pick-of-#596…

b65e797

…86-upstream-release-1.9 Cherry pick of #59686 #67224: Add cloud-provider policies to be applied via addon mgr

k8s-ci-robot added a commit that referenced this pull request Oct 4, 2018

Merge pull request #68259 from grayluck/automated-cherry-pick-of-#672…

0d7846c

…24-upstream-release-1.11 Automated cherry pick of #67224: Add namespace for (cluster)role(binding) cloud-provider.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add namespace for (cluster)role(binding) cloud-provider. #67224

Add namespace for (cluster)role(binding) cloud-provider. #67224

grayluck commented Aug 10, 2018 •

edited

tallclair commented Aug 10, 2018

grayluck commented Aug 10, 2018

bowei commented Aug 10, 2018

bowei commented Aug 10, 2018

k8s-ci-robot commented Aug 10, 2018

grayluck commented Aug 10, 2018

tallclair commented Aug 13, 2018

grayluck commented Aug 13, 2018

grayluck commented Aug 21, 2018

tallclair commented Aug 21, 2018

grayluck commented Aug 21, 2018

tallclair commented Aug 22, 2018

tallclair commented Aug 22, 2018

grayluck commented Aug 24, 2018

tallclair Aug 24, 2018

grayluck Aug 24, 2018

tallclair Aug 24, 2018

grayluck Aug 24, 2018

tallclair Aug 24, 2018

grayluck Aug 24, 2018

bowei commented Aug 24, 2018

grayluck commented Aug 24, 2018 •

edited

grayluck commented Aug 27, 2018

tallclair Aug 28, 2018

grayluck Aug 28, 2018

tallclair commented Aug 28, 2018

grayluck commented Aug 30, 2018

bowei commented Aug 30, 2018

k8s-ci-robot commented Aug 30, 2018

fejta-bot commented Aug 31, 2018

fejta-bot commented Aug 31, 2018

bowei commented Aug 31, 2018

grayluck commented Aug 31, 2018

k8s-github-robot commented Sep 1, 2018

		@@ -18,7 +50,7 @@ apiVersion: rbac.authorization.k8s.io/v1
		kind: ClusterRoleBinding

Add namespace for (cluster)role(binding) cloud-provider. #67224

Add namespace for (cluster)role(binding) cloud-provider. #67224

Conversation

grayluck commented Aug 10, 2018 • edited

tallclair commented Aug 10, 2018

grayluck commented Aug 10, 2018

bowei commented Aug 10, 2018

bowei commented Aug 10, 2018

k8s-ci-robot commented Aug 10, 2018

grayluck commented Aug 10, 2018

tallclair commented Aug 13, 2018

grayluck commented Aug 13, 2018

grayluck commented Aug 21, 2018

tallclair commented Aug 21, 2018

grayluck commented Aug 21, 2018

tallclair commented Aug 22, 2018

tallclair commented Aug 22, 2018

grayluck commented Aug 24, 2018

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

bowei commented Aug 24, 2018

grayluck commented Aug 24, 2018 • edited

grayluck commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tallclair commented Aug 28, 2018

grayluck commented Aug 30, 2018

bowei commented Aug 30, 2018

k8s-ci-robot commented Aug 30, 2018

fejta-bot commented Aug 31, 2018

fejta-bot commented Aug 31, 2018

bowei commented Aug 31, 2018

grayluck commented Aug 31, 2018

k8s-github-robot commented Sep 1, 2018

grayluck commented Aug 10, 2018 •

edited

grayluck commented Aug 24, 2018 •

edited