Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Sep 1, 2018
Merged

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

merged 1 commit into from
Sep 1, 2018
+48 −4

Conversation

grayluck
Copy link
Contributor

@grayluck grayluck commented Aug 10, 2018 •
edited
Loading

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.

@k8s-ci-robot 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 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 k8s-ci-robot requested review from mwielgus and vishh August 10, 2018 00:42
@k8s-ci-robot 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 grayluck mentioned this pull request Aug 10, 2018
Merged
@tallclair
Copy link
Member

This is unreleased code, right?

@grayluck
Copy link
Contributor Author

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

@k8s-ci-robot 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
@bowei
Copy link
Member

bowei commented Aug 10, 2018

/ok-to-test

@k8s-ci-robot 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
@bowei
Copy link
Member

bowei commented Aug 10, 2018

make sure you test with all of the versions

@k8s-ci-robot
Copy link
Contributor

@grayluck: cat image

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
Copy link
Contributor Author

/undo meow

@tallclair
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@tallclair
Copy link
Member

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

@grayluck grayluck force-pushed the namespace-cloudprovider-rbac branch from ab59f7e to f4a2e99 Compare August 21, 2018 23:00
@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Aug 21, 2018
@grayluck
Copy link
Contributor Author

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

@tallclair
Copy link
Member

/lgtm

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

/retest

@tallclair tallclair closed this Aug 22, 2018
@grayluck
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot 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
tallclair reviewed Aug 24, 2018
View reviewed changes
cluster/gce/addons/loadbalancing/cloud-provider-binding.yaml Outdated
# DEPRECATED: cloud-provider roles and bindings are deprecated. Use gce:cloud-provider instead.
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cluster/gce/addons/loadbalancing/cloud-provider-binding.yaml
@@ -18,7 +50,7 @@ apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

cluster/gce/addons/loadbalancing/cloud-provider-binding.yaml Outdated
kind: ClusterRoleBinding
metadata:
labels:
addonmanager.kubernetes.io/mode: Reconcile
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

bowei commented Aug 24, 2018

where is the change to remove the old role binding?

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

grayluck commented Aug 24, 2018 •
edited
Loading

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
Copy link
Contributor Author

/test pull-kubernetes-e2e-gke

tallclair
tallclair reviewed Aug 28, 2018
View reviewed changes
cluster/gce/addons/loadbalancing/cloud-provider-role.yaml Outdated
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.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@grayluck
Add namespace for (cluster)role(binding) cloud-provider.
bea625f 
Change the addonmanager mode to be from reconcile to EnsureExists.
@grayluck grayluck force-pushed the namespace-cloudprovider-rbac branch from a528459 to bea625f Compare August 28, 2018 03:47
@tallclair
Copy link
Member

/lgtm
thanks!

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

/test pull-kubernetes-e2e-gke

@bowei
Copy link
Member

bowei commented Aug 30, 2018

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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

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

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

1 similar comment
@fejta-bot
Copy link

/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
Copy link
Member

bowei commented Aug 31, 2018

/milestone v1.12

@k8s-ci-robot k8s-ci-robot added this to the v1.12 milestone Aug 31, 2018
@grayluck
Copy link
Contributor Author

Thanks Bowei!

@k8s-github-robot
Copy link

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.

@k8s-github-robot k8s-github-robot merged commit 6900a80 into kubernetes:master Sep 1, 2018
This was referenced Sep 5, 2018
Merged
Merged
k8s-ci-robot added a commit that referenced this pull request Sep 11, 2018
@k8s-ci-robot
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
@k8s-ci-robot
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
@k8s-ci-robot
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tallclair tallclair tallclair left review comments

@mwielgus mwielgus Awaiting requested review from mwielgus

@vishh vishh Awaiting requested review from vishh

Assignees

@bowei bowei

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.
6 participants
@grayluck @tallclair @bowei @k8s-ci-robot @fejta-bot @k8s-github-robot