search

open-policy-agent / kube-mgmt

Sidecar for managing OPA instances in Kubernetes.
Apache License 2.0
238 stars 106 forks source link

Do not use ClusterRole and ClusterRoleBinding when .Values.mgmt.namespaces list is empty #224

Closed AndreaVida closed 1 year ago

AndreaVida commented 1 year ago

Support watching all namespaces (*) or only one with limited privileges (no ClusterRole and ClusterRoleBinding).

Default values are backward compatible.

Closes https://github.com/open-policy-agent/kube-mgmt/issues/212.

AndreaVida commented 1 year ago

Hi @eshepelyuk,

about unit tests; my changes are only on the Helm Chart and I honestly have no idea on how unit tests can be done on it.

About implementation, I will try to not add a new value if possible and default behavior of watching current namespace is kept already. I am not 100% sure about the roles iteration stuff though, as the Go K8s library only supports one or all namespaces use cases and I am afraid the all case may require privileges to access all. Do you know more on this?

eshepelyuk commented 1 year ago

Hi @eshepelyuk,

about unit tests; my changes are only on the Helm Chart and I honestly have no idea on how unit tests can be done on it.

You should check test/unit folder and justfile recipe test-helm

About implementation, I will try to not add a new value if possible and default behavior of watching current namespace is kept already. I am not 100% sure about the roles iteration stuff though, as the Go K8s library only supports one or all namespaces use cases and I am afraid the all case may require privileges to access all. Do you know more on this?

I was referring to helm template not golang code.

AndreaVida commented 1 year ago

I was referring to helm template not golang code.

Indeed, but if the library requires cluster-wide access when watching on Namespaces.All it will not work without a ClusterRole. That's the thing I am not sure about. Are you?

eshepelyuk commented 1 year ago

I was referring to helm template not golang code.

Indeed, but if the library requires cluster-wide access when watching on Namespaces.All it will not work without a ClusterRole. That's the thing I am not sure about. Are you?

But if watched namespaces has value of * -the ClusterRole should be gemerated by helm, as now.

AndreaVida commented 1 year ago

I was referring to helm template not golang code.

Indeed, but if the library requires cluster-wide access when watching on Namespaces.All it will not work without a ClusterRole. That's the thing I am not sure about. Are you?

But if watched namespaces has value of * -the ClusterRole should be gemerated by helm, as now.

Exactly, I was referring to the case when namespaces list length is > 1 but I just double checked golang code and seems like in that case one Informer instance is spawned for each namespace to watch; if that's the case then your proposal should work. Do you confirm this is how it works?

eshepelyuk commented 1 year ago

yes I do.

AndreaVida commented 1 year ago

Thanks for the clarification! I will then work on discussed changes from now on.

eshepelyuk commented 1 year ago

Unfortunately there is a deeper issue. ClusterRole is mandatory for k8s resources replication, so it must be created as soon as resource replication is required. rbac.extraRules is intended for thus replication case only.

So much deeper changes required. The should be separate roles for policy \ data loading and for resource replication. My suggestion is to create two separate templates for them.