Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix scope of generated (Cluster)Role(Binding)s #1271

Merged
merged 5 commits into from
Oct 17, 2023

Conversation

tamcore
Copy link
Contributor

@tamcore tamcore commented Oct 13, 2023

This should address the issue raised by @ctml91 via comment in my previous PR: #1262 (comment)

Now it'll take both watchNamespaces and namespaceScope into consideration, for determining if (Cluster)Role(Binding)s are supposed to be deployed.

  • If namespaceScope is set, but watchNamespaces is not, it will default watchNamespaces to .Release.Namespace
  • If watchNamespaces is set, namespaceScope is automatically assumed, even if not explitly set
  • If watchNamespaces contains multiple, comma separated, namespaces, Role(Binding)s are generated for each namespace

Case 1:

  • Installation to grafana-operator namespace, but explitly watching grafana1,grafana2
$ helm template --namespace grafana-operator . --values values.yaml -s templates/rbac.yaml --set watchNamespaces="grafana1\,grafana2" | yq '.rules = "deleted"'
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  namespace: grafana1
  name: grafana-operator-permissions
rules: deleted
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  namespace: grafana2
  name: grafana-operator-permissions
rules: deleted
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: grafana-operator-permissions
  namespace: grafana1
subjects:
  - kind: ServiceAccount
    name: release-name-grafana-operator
    namespace: grafana-operator
roleRef:
  kind: Role
  name: grafana-operator-permissions
  apiGroup: rbac.authorization.k8s.io
rules: deleted
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: grafana-operator-permissions
  namespace: grafana2
subjects:
  - kind: ServiceAccount
    name: release-name-grafana-operator
    namespace: grafana-operator
roleRef:
  kind: Role
  name: grafana-operator-permissions
  apiGroup: rbac.authorization.k8s.io
rules: deleted

Case 2

  • Simply enabling namespaceScope
$ helm template --namespace grafana-operator . --values values.yaml -s templates/rbac.yaml --set namespaceScope=true | yq '.rules = "nothingtoseehere"'
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  namespace: grafana-operator
  name: grafana-operator-permissions
rules: nothingtoseehere
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: grafana-operator-permissions
  namespace: grafana-operator
subjects:
  - kind: ServiceAccount
    name: release-name-grafana-operator
    namespace: grafana-operator
roleRef:
  kind: Role
  name: grafana-operator-permissions
  apiGroup: rbac.authorization.k8s.io
rules: nothingtoseehere

Case 3

  • bare defaults (cluster-wide installation)
$ helm template --namespace grafana-operator . --values values.yaml -s templates/rbac.yaml | yq '.rules = "nothingtoseehere"'
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
  namespace: grafana-operator
  name: grafana-operator-permissions
rules: nothingtoseehere
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRoleBinding
metadata:
  name: grafana-operator-permissions
  namespace: grafana-operator
subjects:
  - kind: ServiceAccount
    name: release-name-grafana-operator
    namespace: grafana-operator
roleRef:
  kind: ClusterRole
  name: grafana-operator-permissions
  apiGroup: rbac.authorization.k8s.io
rules: nothingtoseehere

Case 4:

  • setting both namespaceScope AND watchNamespaces (as you would have to currently)
$ helm template --namespace grafana-operator . --values values.yaml -s templates/rbac.yaml --set namespaceScope=true,watchNamespaces="grafana1\,grafana2" | yq '.rules = "deleted"'
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  namespace: grafana1
  name: grafana-operator-permissions
rules: deleted
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  namespace: grafana2
  name: grafana-operator-permissions
rules: deleted
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: grafana-operator-permissions
  namespace: grafana1
subjects:
  - kind: ServiceAccount
    name: release-name-grafana-operator
    namespace: grafana-operator
roleRef:
  kind: Role
  name: grafana-operator-permissions
  apiGroup: rbac.authorization.k8s.io
rules: deleted
---
# Source: grafana-operator/templates/rbac.yaml
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: grafana-operator-permissions
  namespace: grafana2
subjects:
  - kind: ServiceAccount
    name: release-name-grafana-operator
    namespace: grafana-operator
roleRef:
  kind: Role
  name: grafana-operator-permissions
  apiGroup: rbac.authorization.k8s.io
rules: deleted

Now it'll take both `watchNamespaces` and `namespaceScope` into consideration, for determining if (Cluster)Role(Binding)s are supposed to be deployed.

- If `namespaceScope` is set, but `watchNamespaces` is not, it will default `watchNamespaces` to `.Release.Namespace`
- If `watchNamespaces` is set, `namespaceScope` is automatically assumed, even if not explitly set
- If `watchNamespaces` contains multiple, comma separated, namespaces, `Role(Binding)`s are generated for each namespace
@ctml91
Copy link

ctml91 commented Oct 13, 2023

Thanks 🙏 will this correct the OLM manifests or is this change only affecting helm based installs?

@NissesSenap
Copy link
Collaborator

This only affects the helm installation @ctml91 .
But if you have time to look into how OLM does this, we would a PR around it.

@NissesSenap
Copy link
Collaborator

My head hurts from reading all the helm chart templates, but the output looks good :).
Really nice to generate out the namespace rbac to the watch namespaces as well.

The only issue I see if a user already has created the rbac settings for the operator with the exact same name. If that is the case, they will get an error when they try to update the helm chart.
Could it be worth adding some logic to the helm chart naming to make sure that doesn't happen.

Maybe add the namespace name as a prefix or something like that.

What do you think @tamcore ?

@NissesSenap
Copy link
Collaborator

FYI @smuda , good for you to know about.

@tamcore
Copy link
Contributor Author

tamcore commented Oct 14, 2023

@NissesSenap not sure if i can follow 😅 but i guess it could make sense to prefix the generated resource names with the grafana-operator.fullname template, as it is in the other manifests as well.

@NissesSenap
Copy link
Collaborator

I changed my mind about the rbac names, sorry about that.
LGTM.

@NissesSenap NissesSenap enabled auto-merge (squash) October 17, 2023 12:06
@NissesSenap NissesSenap merged commit 6549cb2 into grafana:master Oct 17, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants