deploy: add RBAC definitions#69
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
This rbac.yaml file is identical to the one in the E2E PR kubernetes/kubernetes#69868 A similar PR was filed against external-provisioner (kubernetes-csi/external-provisioner#156). |
|
/assign @msau |
|
@pohly: GitHub didn't allow me to assign the following users: msau. Note that only kubernetes-csi members and repo collaborators can be assigned. DetailsIn response to this:
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. |
|
/assign |
| verbs: ["get", "list", "watch", "create", "update", "patch"] | ||
| - apiGroups: [""] | ||
| resources: ["nodes"] | ||
| verbs: ["get", "update", "patch"] |
There was a problem hiding this comment.
does driver-registrar still need to patch nodes? With kubelet plugin watcher, I believe the driver registrar doesn't update node objects anymore.
There was a problem hiding this comment.
I haven't tried it, but you are probably right.
Do we need two rbac.yaml files then (one for registrar with --kubelet-plugin-path, one without), or do we only document the mode with --kubelet-plugin-path?
There was a problem hiding this comment.
hmmm I guess we technically still need to support the old way until we deprecate and remove it, so 2 manifests may be needed for now.
There was a problem hiding this comment.
do we have an issue open to make --kubelet-plugin-path a required argument? We wanted to remove the Node permissions ASAP for security reasons so I think it should be fairly high priority to deprecate the "old way."
There was a problem hiding this comment.
I think it's mainly that we need to deprecate the old registration mechanism in Kubernetes. And then we can remove it after two releases. I suppose we could remove it immediately from the sidecars if we say some X version of the sidecar does not support an older Kubernetes version
There was a problem hiding this comment.
How about this?
- a single .rbac file
- the "nodes" commented out with a comment explaining that this is only needed when running without
--kubelet-plugin-path
This is similar to (but not quite the same) how the optional leadership feature is handled in external-attacher and external-provisioner: https://github.com/kubernetes-csi/external-attacher/blob/5b2ae23a80b5e727f7d4bd8aadef2be3f2c4817e/deploy/kubernetes/rbac.yaml#L54-L65
The difference is that the external-attacher/provisioner have an optional Role, which was needed because the permissions are restricted to the namespace. We could split out the "nodes" access into a separate ClusterRole, but as this is going to be phased out, I consider that overkill.
In the review of kubernetes-retired/driver-registrar#69 it was pointed out that the "nodes" permissions are not longer needed.
As discussed in the Kubernetes CSI working group, each sidecar container should document the RBAC definitions that are needed to run the sidecar app.
|
/hold Let's wait for clean test results in kubernetes/kubernetes#70582 before merging, just to be absolutely sure that it works. I wasn't able to test locally, hack/local-up-cluster.sh failed for me in Kubernetes master. |
k8s-ci-robot
added
the
do-not-merge/hold
|
/hold cancel @msau42: kubernetes/kubernetes#70582 is about to be merged, let's do the same here. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pohly, saad-ali The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
5 participants
As discussed in the Kubernetes CSI working group, each sidecar
container should document the RBAC definitions that are needed to
run the sidecar app.