rbac: narrow permissions

[t0rr3sp3dr0]

Create new Roles and ServiceAccounts for the Interceptor and for the Scaler with granular permissions given to them. The Operator Role was changed to narrow its scope and remove write access to resources not owned by it.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO)
• Changelog has been updated and is aligned with our changelog requirements
• Any necessary documentation is added, such as:
  • README.md
  • The docs/ directory
  • The docs repo

Fixes #612

[t0rr3sp3dr0]

@JorTurFer @tomkerkhove, could you take a look at this PR and allow the workflows to run?

[tomkerkhove]

Can you elaborate on this please?

[t0rr3sp3dr0]

Sure! When I changed the permissions so the add-on components only had access to ConfigMaps on the keda namespace, it started to fail with the following error:

W0224 02:33:59.494889       1 reflector.go:424] pkg/mod/k8s.io/client-go@v0.26.1/tools/cache/reflector.go:169: failed to list *v1.ConfigMap: configmaps is forbidden: User "system:serviceaccount:keda:keda-http-add-on-interceptor" cannot list resource "configmaps" in API group "" at the cluster scope

After investigating, I found that the ConfigMap Informer was watching all ConfigMaps of the cluster, not just the ones on the keda namespace. Then I changed it to only look at ConfigMaps from the keda namespace (https://github.com/kedacore/http-add-on/pull/613/files#diff-a2bfc341efdd78af356e3724892bfd7ef8c5cfb3cc9cf14b209df74db39249b8L129-L132). But a user may want to install KEDA on another namespace and this is parameterised, so I need to tell the informer which namespace the add-on is installed for it to watch at ConfigMaps there. That's why I had to add this extra parameter.

[JorTurFer]

Nice improvement!
More fine permission management is always worth :)
How did you generate the RBAC files? We have this command in make file for generating current RBAC file based on the RBAC annotations. I like the idea of a better permission segmentation, but we should maintain a way to automatically generate manifests (including RBAC files).
I guess that we can adapt it for the new scenario

[t0rr3sp3dr0]

@JorTurFer, I changed the // +kubebuilder: comments so the RBAC manifests are automatically generated by controller-gen for the operator when we run make manifests. The YAMLs for the interceptor and the scaler are completely unmanaged in the current implementation, so I created the new manifests manually.

[JorTurFer]

I created the new manifests manually

I think that we should generate them automatically. It's more complicated but it's doable and we can rely on kubebuilder for generating the RBAC manifests, there are several good reasons for that like in case of RBAC API changes, we don't need to deal with them or the automatically update of every required RBAC just adding kubebuilder comments. I can help you with that if you want, I did it in KEDA (but finally it wasn't necessary so I removed it).

Let me know if you want to do that part or you prefer that I do it

[t0rr3sp3dr0]

@JorTurFer, done! I've configured manifest generation for interceptor and scaler with kubebuilder.

[JorTurFer]

LGTM from code pov, but I' m not totally sure about having 3 different service accounts, I see the improvement of role segmentation, but I'm not sure if having 1 sa per component is overkill.
Apart from that, should we update the helm chart now or maybe as the changes include the RBAC files we can do it for the release?
WDYT about these questions @tomkerkhove ?

[t0rr3sp3dr0]

I created three different ServiceAccounts to reduce the blast radius in case some component gets compromised. The interceptor receives arbitrary requests, parses their them, and then forwards to the appropriate services, so I don't like the idea of giving this component any kind of write access.

[JorTurFer]

I created three different ServiceAccounts to reduce the blast radius in case some component gets compromised. The interceptor receives arbitrary requests, parses their them, and then forwards to the appropriate services, so I don't like the idea of giving this component any kind of write access.

This makes sense totally. Thanks for the clarification ❤️ !
The only question left is about when we should update the helm chart @tomkerkhove

[tomkerkhove]

We can update it now already, but ship once we ship newer version

[JorTurFer]

@t0rr3sp3dr0 , do you know how to do it?
You need to open a PR reflecting this RBAC changes in this repo https://github.com/kedacore/charts/tree/main/http-add-on

I can help if you have any trouble

[t0rr3sp3dr0]

@JorTurFer, here is the PR with the changes to the Helm Chart: kedacore/charts#398