Add clusterroles for approving CSRs easily#49284
Add clusterroles for approving CSRs easily#49284k8s-github-robot merged 1 commit intokubernetes:masterfrom
Conversation
|
/retest |
1 similar comment
|
/retest |
| }, | ||
| { | ||
| // 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"}, |
There was a problem hiding this comment.
This functionality is alpha. Can we name this experimental-*?
There was a problem hiding this comment.
@ericchiang My understanding was that this would be beta in v1.8?
cc @mikedanese @jcbsmpsn
There was a problem hiding this comment.
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
| }, | ||
| { | ||
| // 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"}, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@mikedanese would you be ok with system:csr-approver:node-{client,server}-{,rotation}-csr?
|
@mikedanese @liggitt updated |
e6397ef to
3331dc2
Compare
| }, | ||
| { | ||
| // a role making the csrapprover controller approve a node client CSR | ||
| ObjectMeta: metav1.ObjectMeta{Name: "system:csr-approver:node-client-csr"}, |
There was a problem hiding this comment.
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
|
@mikedanese updated, please LGTM |
|
/lgtm |
|
(Mike should definitely have approval access for this part of the code, not sure why he doesn't) |
| }, | ||
| { | ||
| // a role making the csrapprover controller approve a node client CSR | ||
| ObjectMeta: metav1.ObjectMeta{Name: "system:csr-approver:nodeclient"}, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
|
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 @mikedanese PTAL
@liggitt I'll do that |
|
/lgtm |
|
[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. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest Review the full test history for this PR. |
2 similar comments
|
/retest Review the full test history for this PR. |
|
/retest Review the full test history for this PR. |
|
Automatic merge from submit-queue (batch tested with PRs 49284, 49555, 47639, 49526, 49724) |
|
@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 |
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:
cc @kubernetes/sig-auth-pr-reviews @kubernetes/sig-cluster-lifecycle-pr-reviews