Add bootstrap cluster role for external pv provisioners #40308

wongma7 · 2017-01-23T17:49:40Z

The set of permissions an external provisioner #30285 running as a pod will need. Technically in order to dynamically provision PVs one doesn't need to "update" PVCs or "watch" events but the controller https://github.com/kubernetes-incubator/nfs-provisioner/tree/master/controller we are recommending people use does those things to: set lock annotations on PVCs and watch ProvisioningSucceeded/ProvisioningFailed events.

Some external provisioners may need additional permissions, for example nfs-provisioner requires "get" access to Services and Endpoints when run "statefully." I think in that case we would recommend creating a new ClusterRole specific to that provisioner, using this as a base?

(This was to be a part of my redo/fix of the external e2e test #39545 but I'm submitting it as a separate PR for now due to some issues I had with running nfs-provisioner on gce.)

@kubernetes/sig-auth-misc ?

k8s-ci-robot · 2017-01-23T17:49:40Z

Hi @wongma7. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

k8s-reviewable · 2017-01-23T17:49:47Z

This change is

jsafrane · 2017-01-24T08:17:02Z

@k8s-bot ok to test

liggitt · 2017-01-25T06:30:37Z

cc @kubernetes/sig-auth-pr-reviews

liggitt · 2017-01-25T06:33:03Z

since it seems likely that each external provisioner will want some different permissions, would it make more sense to include this in the external provisioner documentation as an example for them to use as a starting point for creating a role for their provisioner?

liggitt · 2017-01-25T06:33:32Z

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

@@ -284,6 +284,21 @@ func ClusterRoles() []rbac.ClusterRole {
 				rbac.NewRule("list", "watch").Groups(batchGroup).Resources("jobs", "cronjobs").RuleOrDie(),
 			},
 		},
+		{
+			// a role for an external/out-of-tree persistent volume provisioner
+			ObjectMeta: metav1.ObjectMeta{Name: "persistent-volume-provisioner"},


in general, the names of bootstrap roles should be prefixed with "system:", since users are free to define their own roles, and we want to minimize the number of potential name conflicts.

wongma7 · 2017-01-25T08:02:44Z

I think I overstated the likelihood of provisioners needing more permissions when I said "some": nfs-provisioner is probably alone because of the weird way it needs to know about its own service. I imagine most other provisioners will just call an API to fill in a flex/nfs/iscsi/etc. volume spec and have no need to touch additional kube objects so a bootstrap role would be useful for them

deads2k · 2017-01-25T13:46:11Z

If its just NFS, then this seems like a reasonable thing to do. It might even work out as a reasonable set of permissions to grant and let the provisioner create its own role for additional permissions to make inspection easier if its a low percentage.

liggitt · 2017-01-31T16:00:58Z

/lgtm /approve

liggitt · 2017-01-31T16:01:42Z

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml

+    - create
+    - patch
+    - update
+- apiVersion: rbac.authorization.k8s.io/v1alpha1


you'll need to update this... the testdata changed to v1beta1

ok, I updated it.

wongma7 · 2017-02-01T18:08:38Z

@liggitt PR needs a re-lgtm/approve, I amended rbac v1alpha1->v1beta1. thanks!

liggitt · 2017-02-01T18:10:22Z

/lgtm

wongma7 · 2017-02-01T23:05:14Z

@deads2k have an /lgtm, now seeking an /approve please. thank you! :)

liggitt · 2017-02-01T23:07:01Z

/approve

k8s-github-robot · 2017-02-01T23:07:05Z

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: liggitt, wongma7

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

k8s-github-robot · 2017-02-02T00:12:46Z

Automatic merge from submit-queue (batch tested with PRs 40574, 40806, 40308, 40771, 39440)

seh · 2017-02-02T16:05:14Z

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

+				rbac.NewRule("get", "list", "watch", "create", "delete").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),
+				// update is needed in addition to read access for setting lock annotations on PVCs
+				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
+				rbac.NewRule(Read...).Groups(storageGroup).Resources("storageclasses").RuleOrDie(),


I see here that we're allowing the "system:persistent-volume-provisioner" ClusterRole to read "storageclass" resources. The ClusterRole "edit" allows subjects to create PVCs, which mention these classes.

Should we allow the "edit" ClusterRole to also (get|list|watch) "storageclass" resources, so that they know which classes are available from which to choose? I asked the same question in the "sig-auth" Slack channel.

the edit cluster role is intended to be granted within a particular namespace, so allowing access to storageclasses in it would not be effective. Read access to storageclasses would need to be in a clusterrole bound to users with a clusterrolebinding, similar to the system:discovery role.

Oh, right; I missed that point about the intended scope for the "edit" role.

Is there a reason why we'd want to preclude users from reading these classes?
Is there an existing ClusterRole to which we could add this resource ("view" comes to mind), or does it warrant a new role?

"view" comes to mind

"view" is also intended to be granted within a namespace. I'm not sure whether I'd consider this fundamental enough to be part of discovery or not. @deads2k, what do you think?

Correct me if I'm wrong, I think the best place for read access to storage classes to go would be system:basicuser, that's how it is in openshift & I assume this basicuser and that one are meant to be roughly equivalent. But in openshift they deliberately only allow listing storage classes not, get nor watch, not sure if the same decision would be made here.

In my cluster, we've bound "view" via ClusterRoleBinding to a few namespace-related service accounts to allow them to see everything across the cluster. I didn't realize that was counter to its intention. We are perhaps a more promiscuous lot here, as we're still figuring out which namespaces are going to need to see which others.

The nascent documentation for "system:basic-user" reads as follows:

A role that allows a user read-only access to basic information about themselves.

Including "storagclass" resources would go beyond "themselves." Does that concern you?

a little. balance between granularity of roles and usability of many roles is tough.

Thank you for the discussion.
I'll prepare a proposal later this afternoon and reference it from here.

See #40881.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 23, 2017

k8s-github-robot assigned liggitt Jan 23, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jan 23, 2017

jsafrane added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Jan 24, 2017

wongma7 force-pushed the pv-rbac branch 2 times, most recently from ccf24a2 to e7fd51e Compare January 24, 2017 16:36

k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 24, 2017

liggitt reviewed Jan 25, 2017

View reviewed changes

wongma7 force-pushed the pv-rbac branch from e7fd51e to 4fee767 Compare January 25, 2017 15:39

k8s-github-robot assigned erictune and unassigned liggitt Jan 30, 2017

apelisse assigned liggitt Jan 31, 2017

liggitt added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jan 31, 2017

liggitt reviewed Jan 31, 2017

View reviewed changes

Add bootstrap cluster role for external pv provisioners

caefe1a

wongma7 force-pushed the pv-rbac branch from 4fee767 to caefe1a Compare January 31, 2017 16:16

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

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

k8s-github-robot merged commit b299c93 into kubernetes:master Feb 2, 2017

seh reviewed Feb 2, 2017

View reviewed changes

seh mentioned this pull request Feb 2, 2017

RBAC: Allow authenticated users to read "storageclasses" resources #40881

Closed

wongma7 mentioned this pull request Mar 9, 2017

Add bootstrap cluster role for external pv provisioners openshift/origin#13333

Merged

Add bootstrap cluster role for external pv provisioners #40308

Add bootstrap cluster role for external pv provisioners #40308

Uh oh!

Conversation

wongma7 commented Jan 23, 2017

Uh oh!

k8s-ci-robot commented Jan 23, 2017

Uh oh!

k8s-reviewable commented Jan 23, 2017

Uh oh!

jsafrane commented Jan 24, 2017

Uh oh!

liggitt commented Jan 25, 2017

Uh oh!

liggitt commented Jan 25, 2017

Uh oh!

liggitt Jan 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wongma7 commented Jan 25, 2017

Uh oh!

deads2k commented Jan 25, 2017

Uh oh!

liggitt commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wongma7 commented Feb 1, 2017

Uh oh!

liggitt commented Feb 1, 2017

Uh oh!

wongma7 commented Feb 1, 2017

Uh oh!

liggitt commented Feb 1, 2017

Uh oh!

k8s-github-robot commented Feb 1, 2017

Uh oh!

k8s-github-robot commented Feb 2, 2017

Uh oh!

seh Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seh Feb 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

liggitt Jan 25, 2017 •

edited

Loading

seh Feb 2, 2017 •

edited

Loading

seh Feb 2, 2017 •

edited

Loading