Add clusterroles for approving CSRs easily #49284

luxas · 2017-07-20T09:46:39Z

What this PR does / why we need it:

Adds ClusterRoles for CSR approving. Currently consumers like kubeadm and GKE have to create these rules by themselves, but are doing it slightly differently which leads to sprawl. Instead, the ClusterRoles are created by core, and the actual bindings created by respective deployment method.

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

fixes #48191

Special notes for your reviewer:

Release note:

The API Server now automatically creates RBAC ClusterRoles for CSR approving. 
Each deployment method should bind users/groups to the ClusterRoles if they are using this feature.

cc @kubernetes/sig-auth-pr-reviews @kubernetes/sig-cluster-lifecycle-pr-reviews

luxas · 2017-07-20T10:46:42Z

/retest

luxas · 2017-07-20T12:50:23Z

/retest

ericchiang · 2017-07-20T16:02:40Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

+		},
+		{
+			// a role making the csrapprover controller approve a node server CSR requested by the node itself
+			ObjectMeta: metav1.ObjectMeta{Name: "system:csr-approver:approve-node-server-renewal-csr"},


This functionality is alpha. Can we name this experimental-*?

@ericchiang My understanding was that this would be beta in v1.8?
cc @mikedanese @jcbsmpsn

I'd rather gate with the feature flag rather than change the name. when the flag goes to beta and is enabled, this will be enabled as well

mikedanese · 2017-07-20T17:53:50Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

+		},
+		{
+			// a role making the csrapprover controller approve a node client CSR requested by the node itself
+			ObjectMeta: metav1.ObjectMeta{Name: "system:csr-approver:approve-node-client-renewal-csr"},


The second approve and csr are redundant. "Renewal" is generally referred to as "rotation" in this context. I don't like how these don't match the subject access review verbs at all.

also, rotation/renewal is one reason a node might request these, but is not the only reason... I'd omit that from the name entirely

@mikedanese would you be ok with system:csr-approver:node-{client,server}-{,rotation}-csr?

luxas · 2017-07-20T18:14:40Z

@mikedanese @liggitt updated

mikedanese · 2017-07-24T20:49:25Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

@@ -361,7 +363,32 @@ func ClusterRoles() []rbac.ClusterRole {
 				eventsRule(),
 			},
 		},
+		{
+			// a role making the csrapprover controller approve a node client CSR
+			ObjectMeta: metav1.ObjectMeta{Name: "system:csr-approver:node-client-csr"},


I suggest just using the name of the sar verbs. The final csr is redundent. E.g.:

system:csr-approver:nodeclient system:csr-approver:selfnodeclient system:csr-approver:selfnodeserver

luxas · 2017-07-24T21:01:05Z

@mikedanese updated, please LGTM

mikedanese · 2017-07-24T21:09:30Z

/lgtm

luxas · 2017-07-24T21:24:47Z

(Mike should definitely have approval access for this part of the code, not sure why he doesn't)

liggitt · 2017-07-24T22:39:34Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/policy.go

@@ -361,7 +363,32 @@ func ClusterRoles() []rbac.ClusterRole {
 				eventsRule(),
 			},
 		},
+		{
+			// a role making the csrapprover controller approve a node client CSR
+			ObjectMeta: metav1.ObjectMeta{Name: "system:csr-approver:nodeclient"},


I like matching the name to the subresource, but the csr-approver namespacing is a little odd… this role doesn't let you approve things, it lets you submit things the autoapprover will approve. Is this the pattern we want to start for roles related to a particular component?

sharding by api group and possibly resource seems like a pattern that would work better in the future, e.g. system:certificates.k8s.io:certificatesigningrequests:nodeclient

@mikedanese Are you ok with that? SGTM

timothysc · 2017-07-31T19:49:12Z

@mattmoyer

liggitt · 2017-07-31T20:35:16Z

please open a corresponding PR against https://github.com/kubernetes/kubernetes.github.io/blob/release-1.8/docs/admin/authorization/rbac.md#other-component-roles (the 1.8 branch) when the names are finalized.

luxas · 2017-07-31T20:45:40Z

@liggitt @mikedanese PTAL

please open a corresponding PR against https://github.com/kubernetes/kubernetes.github.io/blob/release-1.8/docs/admin/authorization/rbac.md#other-component-roles (the 1.8 branch) when the names are finalized.

@liggitt I'll do that

liggitt · 2017-07-31T20:48:10Z

/lgtm

k8s-github-robot · 2017-07-31T20:49:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, luxas, mikedanese

Associated issue: 48191

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

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

~~plugin/pkg/auth/OWNERS~~ [liggitt]

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

fejta-bot · 2017-07-31T23:53:56Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-01T02:44:25Z

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

Review the full test history for this PR.

fejta-bot · 2017-08-01T05:04:57Z

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

Review the full test history for this PR.

k8s-github-robot · 2017-08-01T08:41:53Z

Automatic merge from submit-queue (batch tested with PRs 49284, 49555, 47639, 49526, 49724)

liggitt · 2017-08-20T21:41:20Z

@luxas also, when you open the doc PR, also update the TLS bootstrapping page to give specific examples of the roles you would need to bind to enable bootstrapping

k8s-ci-robot added 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 20, 2017

k8s-github-robot assigned deads2k and liggitt Jul 20, 2017

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 20, 2017

luxas mentioned this pull request Jul 20, 2017

Update the token discovery proposal kubernetes/community#628

Merged

ericchiang reviewed Jul 20, 2017

View reviewed changes

mikedanese reviewed Jul 20, 2017

View reviewed changes

luxas force-pushed the csr_cluster_roles branch from 02d534c to 19df066 Compare July 20, 2017 18:14

luxas force-pushed the csr_cluster_roles branch 2 times, most recently from e6397ef to 3331dc2 Compare July 24, 2017 20:26

mikedanese reviewed Jul 24, 2017

View reviewed changes

luxas force-pushed the csr_cluster_roles branch from 3331dc2 to 2fff965 Compare July 24, 2017 20:56

k8s-ci-robot assigned mikedanese Jul 24, 2017

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

luxas added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2017

liggitt removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 24, 2017

liggitt reviewed Jul 24, 2017

View reviewed changes

timothysc added this to the v1.8 milestone Jul 31, 2017

Add clusterroles for approving CSRs easily

e0ff623

luxas force-pushed the csr_cluster_roles branch from 2fff965 to e0ff623 Compare July 31, 2017 20:45

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

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

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

k8s-github-robot merged commit 2c521d7 into kubernetes:master Aug 1, 2017

This was referenced Aug 9, 2017

kubeadm: Move all node bootstrap token related code in one phase package #49982

Merged

Don't create the system:certificates.k8s.io:certificatesigningrequests:nodeclient ClusterRole in v1.9 kubernetes/kubeadm#384

Closed

luxas mentioned this pull request Aug 21, 2017

Add information about auto-bootstrapped ClusterRoles for CSR approving kubernetes/website#5132

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add clusterroles for approving CSRs easily #49284

Add clusterroles for approving CSRs easily #49284

luxas commented Jul 20, 2017

luxas commented Jul 20, 2017

luxas commented Jul 20, 2017

ericchiang Jul 20, 2017

luxas Jul 20, 2017

liggitt Jul 20, 2017

mikedanese Jul 20, 2017

liggitt Jul 20, 2017

luxas Jul 20, 2017

luxas commented Jul 20, 2017

mikedanese Jul 24, 2017

luxas commented Jul 24, 2017

mikedanese commented Jul 24, 2017

luxas commented Jul 24, 2017

liggitt Jul 24, 2017

liggitt Jul 31, 2017

luxas Jul 31, 2017

mikedanese Jul 31, 2017

timothysc commented Jul 31, 2017

liggitt commented Jul 31, 2017

luxas commented Jul 31, 2017

liggitt commented Jul 31, 2017

k8s-github-robot commented Jul 31, 2017

fejta-bot commented Jul 31, 2017

fejta-bot commented Aug 1, 2017

fejta-bot commented Aug 1, 2017

k8s-github-robot commented Aug 1, 2017

liggitt commented Aug 20, 2017

Add clusterroles for approving CSRs easily #49284

Add clusterroles for approving CSRs easily #49284

Conversation

luxas commented Jul 20, 2017

luxas commented Jul 20, 2017

luxas commented Jul 20, 2017

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

luxas commented Jul 20, 2017

Choose a reason for hiding this comment

luxas commented Jul 24, 2017

mikedanese commented Jul 24, 2017

luxas commented Jul 24, 2017

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

timothysc commented Jul 31, 2017

liggitt commented Jul 31, 2017

luxas commented Jul 31, 2017

liggitt commented Jul 31, 2017

k8s-github-robot commented Jul 31, 2017

fejta-bot commented Jul 31, 2017

fejta-bot commented Aug 1, 2017

fejta-bot commented Aug 1, 2017

k8s-github-robot commented Aug 1, 2017

liggitt commented Aug 20, 2017