Update RBAC roles for apprepositories to be clusterroles so they can be used in many namespaces. #1654
Conversation
… they can be used in many namespaces.
There was a problem hiding this comment.
Thanks @absoludity. Check my comments, we already have roles for reading and writing apprepositories for users.
| - watch | ||
| --- | ||
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: "kubeapps:controller:apprepository-reader-{{ .Release.Namespace }}" | ||
| name: "kubeapps:controller:appreposities-read" |
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: "kubeapps:apprepositories-write" |
There was a problem hiding this comment.
I would create just one ClusterRole (this is internal, see my comment below)
There was a problem hiding this comment.
I think there's some confusion here... it's not internal. The reader cluster-role was internal, but this is renaming it so it can be both used internally for the controller, but also available to be bound in any namespace to give users the ability to read (or write - hence the second) app repos in specific namespaces. More below.
docs/user/private-app-repository.md
Outdated
| You can give specific users the ability to create App Repositories in a specific namespace by granting them the necessary RBAC: | ||
|
|
||
| ```bash | ||
| kubectl -n custom-namespace create rolebinding username-apprepositories-read --user username --clusterrole kubeapps:apprepositories-write | ||
| ``` | ||
|
|
||
| or other users the ability to deploy charts from App Repositories in a specific namespace by granting them read access: | ||
|
|
||
| ```bash | ||
| kubectl -n custom-namespace create rolebinding username-apprepositories-read --user username --clusterrole kubeapps:apprepositories-read | ||
| ``` |
There was a problem hiding this comment.
we already have those roles per namespace:
It is also documented in docs/user/access-control.md . We should use the same roles in this guide.
There was a problem hiding this comment.
we already have those roles per namespace:
Yes, I was going to delete those, as unless I'm missing something, they are no longer useful. They are created in the release namespace only as they are intended to be bound in the release namespace only (ie. before per-namespace app repositories, we provided these unbound roles so people could easily add app repo readers / writers in the release namespace only.) Now we similarly want to provide unbound roles which can be applied in any namespace, hence the new cluster roles.
It is also documented in docs/user/access-control.md . We should use the same roles in this guide.
Depending whether you agree with the above, I'll update that doc to use the new cluster roles as well.
There was a problem hiding this comment.
ah, right, I missed the cluster/namespace context. To avoid a breaking change we can have both, the role and the clusterrole and modify the documentation to point to that clusterrole.
One thing we need to handle is several instances of Kubeapps per cluster. The easiest thing to do is to include the namespace in the name (even if it's a clusterrole): kubeapps:{ns}:apprepositories-write. WDYT?
There was a problem hiding this comment.
ah, right, I missed the cluster/namespace context. To avoid a breaking change we can have both, the role and the clusterrole and modify the documentation to point to that clusterrole.
Done. I initially was updating the access-control.md to per-namespace instructions, but then realised we shouldn't do that until we switch the default. For now I've just switched the reference to the cluster role.
One thing we need to handle is several instances of Kubeapps per cluster. The easiest thing to do is to include the namespace in the name (even if it's a clusterrole):
kubeapps:{ns}:apprepositories-write. WDYT?
I'd originally removed the release name, because it seemed redundant: helm doesn't resource-count anyway, so deleting a chart just deletes all its resources which I thought meant it would delete the (cluster-wide) CRD too, breaking other releases, but I checked and found that deleting a helm release (with helm 3) does not delete any CRD's it creates. So yep, I agree - I'll update them to be unique to the release.
There was a problem hiding this comment.
Note that this also required removing the condition (the feature flag), so that now this RBAC is always added if .Values.rbac.create is true.
|
Please take another look :) |
Renames the apprepositories-read cluster role so that it is no longer controller specific, and can be used to assign users to the role in specific namespaces. (Also removed the 'update' verb from this, not sure why it was there given the name, seems to work without it, but I assume the controller needed to update app repo resources at some point?).
Similarly, add a related apprepositories-write cluster role (unbound).
Updates the dev setup (which I use) so that the
kubeapps-operatoruser can create app repos in thekubeapps-user-namespace, while thekubeapps-useruser can only read them (so can deploy).Include usage of these clusterroles in the docs.
Ref: #1521