Add CSI External Components ClusterRole to bootstrapped roles #61866

davidz627 · 2018-03-29T00:02:38Z

Added CSI External Components ClusterRole to bootstrapped roles and removed creation from failing e2e test

/sig storage
/kind bug
/assign @liggitt @saad-ali

NONE

davidz627 · 2018-03-29T00:03:41Z

E2E test that was failing is tested and passing on GKE with this change

liggitt · 2018-03-29T00:33:29Z

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

+				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
+				rbac.NewRule("get", "list", "watch", "create", "update", "patch").Groups(legacyGroup).Resources("events").RuleOrDie(),
+				rbac.NewRule("get", "list").Groups(legacyGroup).Resources("secrets").RuleOrDie(),
+				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("nodes").RuleOrDie(),


How is a volume provisioner updating Node objects?

it is not, I am splitting up roles now :)

liggitt · 2018-03-29T00:34:44Z

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

+				rbac.NewRule("create", "delete", "get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),
+				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
+				rbac.NewRule("get", "list", "watch", "create", "update", "patch").Groups(legacyGroup).Resources("events").RuleOrDie(),
+				rbac.NewRule("get", "list").Groups(legacyGroup).Resources("secrets").RuleOrDie(),


I wouldn't expect listing of secrets, and I'd expect secret permissions to be granted specific to an installation in combination with the storage class setup, not globally to all secrets across all namespaces

liggitt · 2018-03-29T00:39:10Z

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

+				rbac.NewRule("get", "list", "watch", "update").Groups(storageGroup).Resources("volumeattachments").RuleOrDie(),
+				rbac.NewRule("get", "list", "watch").Groups(storageGroup).Resources("storageclasses").RuleOrDie(),
+			},
+		},


Should grant patch in addition to update and write external components to use patch to minimize chances of dropping fields on version skewed clusters

Right now none of the components use "Patch". It is an open issue and we are aware of it. Should we give "Patch" API access now, or wait until we update the components to support "Patch"ing

go ahead and add patch permission, since we know that is the desired operation long-term

liggitt · 2018-03-29T00:40:25Z

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

@@ -458,6 +458,19 @@ func ClusterRoles() []rbac.ClusterRole {
 				eventsRule(),
 			},
 		},
+		{
+			// a role for csi external components {csi-provisioner,csi-attacher,driver-registrar}
+			ObjectMeta: metav1.ObjectMeta{Name: "system:csi-external-components"},


Role names are typically singular

Where does the driver-registrar run?

If the provisioner, attacher, and driver are run in separate locations with different levels of access to the API, three roles might make more sense. I wouldn't want every kubelet to have the ability to delete any PV object

In the test we are attempting to fix they are all run in the same location, however we wish to leave the flexibility of CSI Driver implementers to split them up if they wish. I will create 3 roles with minimal permissions required for each component

davidz627 · 2018-03-29T22:30:45Z

@liggitt looking into splitting into 3 separate roles for the 3 external components.

Could you elaborate on "I'd expect secret permissions to be granted specific to an installation in combination with the storage class setup, not globally to all secrets across all namespaces" I'm not exactly sure what that means. You mean a role for secrets permissions would be created on installation of a specific CSI driver that is limited to the namespace it's sitting in?

saad-ali · 2018-03-29T22:46:50Z

I would suggest three different roles: csi-provisioner, csi-attacher and csi-mounter each with the minimal permissions that are absolutely required for the csi provisioner, the csi attacher, and the csi mounter.

/assign @msau42

davidz627 · 2018-03-30T00:41:49Z

Split into 3 roles. Minimal required permissions given for each role. @liggitt @saad-ali PTAL

liggitt · 2018-03-30T00:53:22Z

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

+			Rules: []rbac.PolicyRule{
+				rbac.NewRule("create", "delete", "list", "watch").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),
+				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),
+				rbac.NewRule("get").Groups(legacyGroup).Resources("secrets").RuleOrDie(),


provisioners don't have a general need to get every secret in the cluster. a provisioner might:

need no secret-related permissions

need permissions to view a particular secret for provisioning

need permissions to view secrets in a particular namespace for provisioning

need permissions to create attach/mount-related secrets in a particular namespace

rather than grant broad permissions here on the off chance the provisioner needs them, I'd expect specific permissions to be granted as a particular provisioner or storage class requires.

Ok, I will remove the secrets for now. We will have to implement some sort of workaround to allow for creating Roles in the test framework for GKE once we start adding tests for external CSI components that require secrets.

once we start adding tests for external CSI components that require secrets.

I'd actually expect those to come with recommended setups that put them in a namespace, set up a storage class for them, and granted them appropriate secret permissions within that namespace. Basically, the standard role (which you are defining here) takes care of the "every provisioner will have to do X" things, and they get to grant any additional permissions they want in a manifest specific to their component.

Right, when I remove the secrets it the roles I'm adding here will be consistent with "every provisioner will have to do X". The recommended setups given to users of CSI will have the additional roles to grant whatever additional permissions they need. The only issue is that the test framework on GKE does not allow creation of roles by default so there would just need to be a workaround in the test or a larger framework/GKE change would need to be done.

Live deployment of these plugins was never really the issue. The change was spurred mainly by test failure on GKE.

liggitt · 2018-03-30T00:55:05Z

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

+			// a role for csi external components {csi-provisioner,csi-attacher,driver-registrar}
+			ObjectMeta: metav1.ObjectMeta{Name: "system:csi-driver-registrar"},
+			Rules: []rbac.PolicyRule{
+				rbac.NewRule("get", "update").Groups(legacyGroup).Resources("nodes").RuleOrDie(),


I'm not familiar with the registrar component... where does this process run, and what on the Node object is it updating?
/cc @saad-ali

The driver registrar adds an annotation on the node that contains a JSON map of driver names to node names. These node names could be different from what Kubernetes views as the node name. The process will generally be deployed alongside the CSI Plugin in a Pod.

so this is giving every kubelet running the CSI plugin pod access to a credential (the service account credential for the CSI pod) granted a role that lets it modify any node in the cluster?

I believe so, @saad-ali to confirm

liggitt · 2018-03-30T01:00:30Z

test/e2e/storage/csi_volumes.go

-			clusterRole = csiClusterRole(cs, config, false)
-			serviceAccount = csiServiceAccount(cs, config, false)
-			csiClusterRoleBinding(cs, config, false, serviceAccount, clusterRole)
+			serviceAccount = csiCombinedServiceAccount(cs, config, false)


this is up to @kubernetes/sig-storage-pr-reviews, but I'd recommend exercising each pair of phases (provision/delete, attach/detach, mount/unmount) using a separate service account with just the specified role. That way, e2e tests make it really clear when a new operation is performed in a phase that isn't covered by the relevant role.

I think that would be a good test to have, however this test is for testing the HostPath CSI Plugin specifically, not the CSI external components. We should definitely add in tests for the external components soon @saad-ali.

@liggitt let me know if I misunderstood your comment

this test is for testing the HostPath CSI Plugin specifically, not the CSI external components

so does that mean it doesn't need the provisioner and attacher roles?

The csi-provisioner, driver-registrar, and csi-attacher are side-car containers necessary for the HostPath CSI Plugin container. The pod containing all 4 containers will need provisioner, attacher, and driver-registrar roles.

liggitt · 2018-03-30T01:01:17Z

thanks, looks a lot better. just a few comments

davidz627 · 2018-03-30T02:02:34Z

For future reference: for tests for plugins that require secrets, the test may have to implement a workaround described here: #43409 (comment)

Or we should get it so that the test framework in GKE is allowed to create ClusterRoles.

davidz627 · 2018-03-30T02:29:13Z

Removed "get" secrets from system:csi-external-provisioner cluster role.

msau42 · 2018-03-30T17:26:20Z

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

+			ObjectMeta: metav1.ObjectMeta{Name: "system:csi-external-provisioner"},
+			Rules: []rbac.PolicyRule{
+				rbac.NewRule("create", "delete", "list", "watch").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),
+				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumeclaims").RuleOrDie(),


Why does the external provisioner need to update PVCs?

It's required for leader election in order "to acquire lease to provision for pvc"

msau42 · 2018-03-30T17:27:48Z

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

+			// a role for the csi external attacher
+			ObjectMeta: metav1.ObjectMeta{Name: "system:csi-external-attacher"},
+			Rules: []rbac.PolicyRule{
+				rbac.NewRule("get", "list", "watch", "update").Groups(legacyGroup).Resources("persistentvolumes").RuleOrDie(),


What does the attacher update in PVs?

It adds a finalizer to the PV

davidz627 · 2018-04-02T18:31:26Z

/retest

saad-ali · 2018-04-02T21:05:51Z

Is self-reporting of CSI driver<->node name mappings expected in real deployments?

CSI allows for the node naming for storage system to be independent of node naming of the Kubernetes cluster. That said, I'm not aware of any cluster where this is actually the case.

That said, moving forward there are plans to extend this functionality to allow a storage plugin to report the "topology labels" that apply to a specific node (e.g. zone, rack, etc.), in addition to the node name. These labels would be applied to the node as node labels. So driver-registrar will require the ability to both add annotations and labels in the future.

liggitt · 2018-04-02T21:18:58Z

CSI allows for the node naming for storage system to be independent of node naming of the Kubernetes cluster.

That's fine, but it seems like it should be driven by a source of truth, not self-reporting.

moving forward there are plans to extend this functionality to allow a storage plugin to report the "topology labels" that apply to a specific node (e.g. zone, rack, etc.), in addition to the node name. These labels would be applied to the node as node labels.

letting nodes arbitrarily self-label themselves is problematic (see https://github.com/kubernetes/community/pull/911/files for multiple examples why) and work is in progress to reduce that in the future.

saad-ali · 2018-04-02T22:05:19Z

The csi-provisioner, driver-registrar, and csi-attacher are side-car containers necessary for the HostPath CSI Plugin container. The pod containing all 4 containers will need provisioner, attacher, and driver-registrar roles.

And a copy of this pod, with credentials granted all three of these roles, is anticipated to be running on every node?

This is not correct. Technically only 1 instance of provisioner and attacher need to running per cluster. So a CSI deployment would consist of two deployments:

StatefulSet or ReplicaSet with size=1
- 3 containers: provisioner, attacher, and plugin
DeamonSet
- 2 containers: driver-registrar and plugin.

The pod for 1, only needs provisioner and attacher permissions (not driver-registrar) and runs on a single node somewhere on the cluster.
The pods for 2, only need permission for driver-registrar (not for provisioner or attacher) and run on every single node.

That's fine, but it seems like it should be driven by a source of truth, not self-reporting.

Source of truth in this case is the storage system, and k8s interacts with the storage system via CSI. It queries the storage system and to discover the name of a node according to the storage system (via CSI), and it needs to store the mapping between the storage node name and k8s node name somewhere (the k8s node object was the most obvious place).

letting nodes arbitrarily self-label themselves is problematic (see https://github.com/kubernetes/community/pull/911/files for multiple examples why) and work is in progress to reduce that in the future.

Good to know. I'm not sure it can be avoided, but I'll set up some time to sync up on this and see if we can come up with a better solution.

verult · 2018-04-07T01:52:42Z

Instead of whitelisting specific label names or prefixes in NodeRestriction, what if we whitelist regexes?

Not sure exactly what the CSI topology labels would look like yet, but just wanted to see what are the options.

davidz627 · 2018-04-07T19:23:51Z

@saad-ali @msau42 @liggitt I have updated the test with the workaround that will get this test running on GKE, as well as linked a TODO to an issue to remove the workaround when the work surrounding removing the node update requirements from the CSI external components is resolved.

PTAL

liggitt · 2018-04-07T20:22:59Z

test/e2e/storage/csi_volumes.go

 	clusterRoleBindingClient := client.RbacV1().ClusterRoleBindings()
+	stdout, stderr, err := framework.RunCmd("gcloud", "config", "get-value", "core/account")


rather than writing a GCE-specific command, you could do this to get a clientset you could use to define roles (impersonation also used by https://github.com/kubernetes/kubernetes/blob/master/test/e2e/auth/node_authz.go#L65, and the e2e user is presumed to be sufficiently authorized to be allowed to impersonate):

By("Creating an impersonating superuser kubernetes clientset to define RBAC roles") config, err := framework.LoadConfig() Expect(err).NotTo(HaveOccurred()) config.Impersonate = restclient.ImpersonationConfig{ UserName: "superuser", Groups: []string{"system:masters"}, } superuserClientset, err := clientset.NewForConfig(config)

thats perfect, incorporating now.

saad-ali · 2018-04-09T06:36:34Z

@liggitt @davidz627 and I met and discussed the issues with the CSI driver-registrar permissions. The driver-registrar required permission to the Kubernetes node API object in order to persist the mapping of CSI storage node name and, in the future, to allow CSI volume plugins to specify node topology labels.

However this would require giving every instance of the driver-registrar on every node permission to edit all Kubernetes node API objects, meaning if one node was compromised, it could modify any other node object.

We decided that a better approach would be to remove the logic for applying node labels/annotation from the driver-registrar sidecar container, and move it to the kubelet. This will take advantage of the fact that kubelet's node access is already limited (via node authorizer) to its own node object.

The Kubernetes CSI team will track this as an item for the coming quarter. In the meantime, in order to fix the test on GKE, dyzz@ will give the E2E user admin privileges to allow it to create RBAC rules.

liggitt · 2018-04-09T18:58:37Z

test/e2e/storage/csi_volumes.go

-			clusterRole = csiClusterRole(cs, config, false)
-			serviceAccount = csiServiceAccount(cs, config, false)
-			csiClusterRoleBinding(cs, config, false, serviceAccount, clusterRole)
+			csiDriverRegistrarClusterRole(cs, config, false)


probably a non-issue for this PR because there's a single test in this section, but if there were multiple tests, teardown of this shared global role at the beginning of every test would be disruptive... as a follow-up, might want to change this to just create the clusterrole if needed and not worry about tearing it down afterward

similar comments about the service account binding... you'd probably want to make it unique per test (include namespace in the clusterrolebinding name)

liggitt · 2018-04-09T19:00:09Z

rbac changes lgtm
/approve

will let @kubernetes/sig-storage-pr-reviews LGTM the test

…emoved creation from failing e2e test

davidz627 · 2018-04-09T22:37:12Z

/retest

saad-ali · 2018-04-10T20:47:37Z

/lgtm

k8s-ci-robot · 2018-04-10T20:47:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, liggitt, saad-ali

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:

~~plugin/pkg/auth/OWNERS~~ [liggitt]
~~test/e2e/storage/OWNERS~~ [liggitt,saad-ali]

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

k8s-github-robot · 2018-04-11T06:52:16Z

Automatic merge from submit-queue (batch tested with PRs 62192, 61866, 62206, 62360). If you want to cherry-pick this change to another branch, please follow the instructions here.

@msau42

Automatic merge from submit-queue. Add RBAC policy rules for csi-external-provisioner and csi-external-attacher Adds RBAC Policy rules for `csi-external-provisioner` and `csi-external-attacher` so that CSI drivers can bind to these cluster roles on every version of k8s where CSI is Beta or above. These roles were added in 1.11 but never cherrypicked back to 1.10. The roles originally added as a part of a larger change here: #61866 I could not do a direct cherry-pick because some of the RBAC primitives changed and there was also a fix applied on top with this PR: #65070 The fix has been included in this commit. /kind enhancement /sig storage /cc @msau42 /assign @liggitt @MaciekPytel ```release-note NONE ```

k8s-ci-robot assigned liggitt and saad-ali Mar 29, 2018

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. kind/bug Categorizes issue or PR as related to a bug. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 29, 2018

k8s-ci-robot requested review from jingxu97 and liggitt March 29, 2018 00:02

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 29, 2018

davidz627 force-pushed the fix/CSIe2e branch from 06042cb to f0aad15 Compare March 29, 2018 00:18

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 29, 2018

liggitt reviewed Mar 29, 2018

View reviewed changes

k8s-ci-robot assigned msau42 Mar 29, 2018

davidz627 force-pushed the fix/CSIe2e branch from f0aad15 to 28fb18e Compare March 30, 2018 00:41

liggitt reviewed Mar 30, 2018

View reviewed changes

k8s-ci-robot requested a review from saad-ali March 30, 2018 00:55

liggitt reviewed Mar 30, 2018

View reviewed changes

davidz627 force-pushed the fix/CSIe2e branch from 28fb18e to 40917d6 Compare March 30, 2018 02:28

msau42 reviewed Mar 30, 2018

View reviewed changes

davidz627 force-pushed the fix/CSIe2e branch from 40917d6 to cd89294 Compare March 30, 2018 22:17

davidz627 force-pushed the fix/CSIe2e branch 3 times, most recently from ff2bcf9 to 60cdc7e Compare April 6, 2018 21:38

davidz627 mentioned this pull request Apr 6, 2018

Start running hostpath CSI driver test on GKE when node update permissions no longer required to run driver #62213

Closed

davidz627 force-pushed the fix/CSIe2e branch from 60cdc7e to 4748e59 Compare April 7, 2018 19:21

davidz627 force-pushed the fix/CSIe2e branch from 4748e59 to 096852e Compare April 7, 2018 20:20

liggitt reviewed Apr 7, 2018

View reviewed changes

davidz627 force-pushed the fix/CSIe2e branch from 096852e to 6b07ebd Compare April 7, 2018 21:52

saad-ali mentioned this pull request Apr 9, 2018

design: reduce scope of node on node object kubernetes/community#911

Merged

davidz627 force-pushed the fix/CSIe2e branch from 6b07ebd to fdaa8dc Compare April 9, 2018 18:50

liggitt reviewed Apr 9, 2018

View reviewed changes

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

Added CSI External Components ClusterRole to bootstrapped roles and r…

fbbccbf

…emoved creation from failing e2e test

davidz627 force-pushed the fix/CSIe2e branch from fdaa8dc to fbbccbf Compare April 9, 2018 21:17

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

k8s-github-robot merged commit 56d6f05 into kubernetes:master Apr 11, 2018

davidz627 deleted the fix/CSIe2e branch April 11, 2018 16:22

davidz627 mentioned this pull request Jun 13, 2018

Add RBAC policy rules for csi-external-provisioner and csi-external-attacher #65073

Merged

		clusterRoleBindingClient := client.RbacV1().ClusterRoleBindings()
		stdout, stderr, err := framework.RunCmd("gcloud", "config", "get-value", "core/account")

Add CSI External Components ClusterRole to bootstrapped roles #61866

Add CSI External Components ClusterRole to bootstrapped roles #61866

Conversation

davidz627 commented Mar 29, 2018

davidz627 commented Mar 29, 2018 • edited Loading

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

liggitt Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 commented Mar 29, 2018

saad-ali commented Mar 29, 2018

davidz627 commented Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 Mar 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Mar 30, 2018 • edited Loading

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

liggitt commented Mar 30, 2018

davidz627 commented Mar 30, 2018

davidz627 commented Mar 30, 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

davidz627 commented Apr 2, 2018

saad-ali commented Apr 2, 2018

liggitt commented Apr 2, 2018

saad-ali commented Apr 2, 2018 • edited Loading

verult commented Apr 7, 2018 • edited Loading

davidz627 commented Apr 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali commented Apr 9, 2018

Choose a reason for hiding this comment

liggitt commented Apr 9, 2018

davidz627 commented Apr 9, 2018

saad-ali commented Apr 10, 2018

k8s-ci-robot commented Apr 10, 2018

k8s-github-robot commented Apr 11, 2018

davidz627 commented Mar 29, 2018 •

edited

Loading

liggitt Mar 29, 2018 •

edited

Loading

davidz627 commented Mar 30, 2018 •

edited

Loading

davidz627 Mar 30, 2018 •

edited

Loading

liggitt Mar 30, 2018 •

edited

Loading

saad-ali commented Apr 2, 2018 •

edited

Loading

verult commented Apr 7, 2018 •

edited

Loading