Configure flux-specific RBAC for kubeapps-apis #3494 #3551
Conversation
|
CI is failing, very likely due to the latest CI patch we added not being in your branch. Just merging master into your branch should solve the issue. |
absoludity
left a comment
absoludity
left a comment
There was a problem hiding this comment.
I have also done some clean up to existing templates, where ClusterRole and ClusterRoleBinding were defined with a namespace, even though they are cluster-wide resources and are not associated with any particular namespace.
+1 to clean up where those resources specified a .metadata.namespace (as that's confusing and would be ignored), but we included the kubeapps installation namespace in the name of those cluster-level resources so that they don't clash when installing multiple copies of Kubeapps in different namespaces (ie. the name of each cluster role is unique per Kubeapps installation)
This used to be possible, but since Helm 3 updated the way CRDs are installed, it may be that its no longer possible (haven't checked) in which case, I don't mind either way, but have a slight preference for not changing the names as part of this PR.
| metadata: | ||
| name: "kubeapps:controller:kubeapps-apis-dev-{{ .Release.Namespace }}" | ||
| namespace: {{ .Release.Namespace | quote }} | ||
| name: "kubeapps:controller:kubeapps-apis-dev" |
There was a problem hiding this comment.
Although we should not have the .metadata.namespace specified above (thanks for deleting that one), we did want the kubeapps release namespace in the name. This was so that if multiple Kubeapps installations are added in different namespaces, they will each have their own cluster role that can be managed independently, rather than clashing on the clusterrole name. Since Helm 3 changed the way CRDs are installed, we may not even support multiple Kubeapps installations on the one cluster atm, in which case, not sure of the benefit.
Furthermore, I think this complete ClusterRole could be included in the {{- if .Values.kubeappsapis.unsafeUseDemoSA }} condition below? That is, this -dev clusterrole should only be added if the unsafeUseDemoSA afaict? (and as you mentioned, we'll be removing that option separately anyway).
There was a problem hiding this comment.
OK, will do that as long as I am changing the file anyway
| @@ -0,0 +1,47 @@ | |||
| {{- if .Values.featureFlags.kubeappsAPIsServer }} | |||
| {{- if has "fluxv2" .Values.kubeappsapis.enabledPlugins }} | |||
| @@ -0,0 +1,47 @@ | |||
| {{- if .Values.featureFlags.kubeappsAPIsServer }} | |||
There was a problem hiding this comment.
I'll do a separate PR to remove that option... it should no longer be an option since the UI is using it now.
| rules: | ||
| - apiGroups: ["source.toolkit.fluxcd.io", "helm.toolkit.fluxcd.io"] | ||
| resources: ["helmrepositories", "helmcharts", "helmreleases"] | ||
| verbs: ["get", "list", "watch", "create", "update", "delete"] |
There was a problem hiding this comment.
Just want to verify that the plugin really does need all 6 verbs? I'm surprised only because I thought the user is creating the HelmRelease CR.
There was a problem hiding this comment.
Nope, the plug-in creates HelmRelease CRD when the user calls CreateInstalledPackage(...)
There was a problem hiding this comment.
Right, but that's a synchronous action isn't it? So the plugin should be doing that with the users' token (as we do for the helm plugin), otherwise we'd be introducing a privilege escalation? (ie. a user who doesn't have RBAC permissions to create a flux HelmRelease can escalate their privs by using the kubeapps apis with the flux plugin, to get the flux plugiin to do it for them).
There was a problem hiding this comment.
yeah, that's true. So here we only need to list out what is needed by background task only, if so, only helmrepository will stay and most of the verbs will go away
| metadata: | ||
| name: "kubeapps:controller:kubeops-ns-discovery-{{ .Release.Namespace }}" | ||
| namespace: {{ .Release.Namespace | quote }} | ||
| name: "kubeapps:controller:kubeops-ns-discovery" |
There was a problem hiding this comment.
Again, great to remove the namespace metadata, but the intent of the name is to identify the ClusterRole created by the kubeapps installation in the .Release.Namespace namespace.
There was a problem hiding this comment.
gotcha. will do
No problem, will change the names back |
3 participants
In addition to addressing
Configure flux-specific RBAC for kubeapps-apis #3494
I have also done some clean up to existing templates, where ClusterRole and ClusterRoleBinding were defined with a namespace, even though they are cluster-wide resources and are not associated with any particular namespace. I mentioned this on slack a few days ago and decided to just clean it up