-
Notifications
You must be signed in to change notification settings - Fork 1.6k
🐛 (helm/v2-alpha): Fix cross-namespace RBAC file naming and templating #5360
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
base: master
Are you sure you want to change the base?
🐛 (helm/v2-alpha): Fix cross-namespace RBAC file naming and templating #5360
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Could you help us in the review? |
096e67d to
bb65586
Compare
|
/test pull-kubebuilder-e2e-k8s-1-34-0 |
| namespace := resource.GetNamespace() | ||
| if namespace != "" && (kind == "Role" || kind == "RoleBinding") { | ||
| fileName = fmt.Sprintf("%s-%s", fileName, namespace) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/hold
Requires changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filename collision fix is good.
The logic for cross-namespace RBAC filenames makes sense
bb65586 to
56dbecf
Compare
Fixes handling of namespace-scoped RBAC resources (Role, RoleBinding) in cross-namespace scenarios for leader election and cross-namespace permissions. Changes: - Extract manager namespace from Deployment resource - Append namespace suffix only for cross-namespace RBAC files - Template only manager namespace refs, preserve cross-namespace values Result: - manager-role.yaml (manager NS, templated) - manager-role-infrastructure.yaml (cross-NS, preserved) Assisted-by: Cursor
56dbecf to
015443b
Compare
|
/hold cancel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this fix! I really appreciate you addressing issue #5354.
The filename collision prevention logic in chart_writer.go (lines 165-174) looks great - appending the namespace suffix should properly distinguish cross-namespace roles.
I tested the new changes on my project to generate the helm chart and i found an interesting bug.
if the CRD name contains the namespace name, we will have this in the ClusterRole manifest:
....
resources:
- {{ .Release.Namespace }}s/finalizers
verbs:
- update
- apiGroups:
- identity.me.cloud
resources:
- {{ .Release.Namespace }}s/status
verbs:
- get
- patch
- update
you can see my inline comment for more details.
| // Replace only the manager namespace with template variable. | ||
| // Cross-namespace values (infrastructure, production, etc.) are naturally preserved | ||
| // because they don't match the manager namespace string. | ||
| yamlContent = strings.ReplaceAll(yamlContent, managerNamespace, namespaceTemplate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This string replacement is too aggressive and will break resource names that contain the namespace as a substring.
Example bug:
- CRD resource name:
users - Manager namespace:
user - Result:
{{ .Release.Namespace }}s❌ (should stayusers)
Suggested fix:
Replace only in the namespace field context:
yamlContent = strings.ReplaceAll(yamlContent, "namespace: "+managerNamespace, "namespace: "+namespaceTemplate)or even a better solution:
Parse YAML → Modify specific fields → Re-serialize
This way we know what we are changing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All that is under the manager namespace is the namespace we define when we install Helm: {{ .Release.Namespace }}.
If we follow your suggestion as-is, we’ll end up with a namespace bug: the namespace would be set to <project-name>-system, because that value is coming from kustomize in this case.
So, in the same way we replace all occurrences of <project-name>-system with {{ .Release.Namespace }}, this will continue to work correctly.
We just need a caveat around this, due to the ability to customize the namespace via options in the default config/kustomize configuration.
However, if the ns is not == the manager's one, then we keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, but i think you misunderstood me.
The replacement via strings.ReplaceAll works well on everything expect on the rbac yaml files for my example.
My CRD name is users and my manager deployment file which is config/manager/manager.yaml has the namespace field namespace: user. The namespace is a substring of the CRD name and when the helm chart is being created it replaces every user word with {{ .Release.Namespace }}.
This replacement will break the RBAC files including the CRD name inside it (resources field), it will create the following rbac template file:
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: {{ include "osiris.resourceName" (dict "suffix" "manager-role" "context" $) }}
rules:
- apiGroups:
- ""
resources:
- configmaps
verbs:
- get
- list
- patch
- update
- watch
- apiGroups:
- apps
resources:
- deployments
verbs:
- get
- list
- watch
- apiGroups:
- identity.me.cloud
resources:
- {{ .Release.Namespace }}s
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- identity.me.cloud
resources:
- {{ .Release.Namespace }}s/finalizers
verbs:
- update
- apiGroups:
- identity.me.cloud
resources:
- {{ .Release.Namespace }}s/status
verbs:
- get
- patch
- update
{{ .Release.Namespace }}s is the issue here.
the correct generated file would be:
apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: {{ include "osiris.resourceName" (dict "suffix" "manager-role" "context" $) }}
rules:
- apiGroups:
- ""
resources:
- configmaps
verbs:
- get
- list
- patch
- update
- watch
- apiGroups:
- apps
resources:
- deployments
verbs:
- get
- list
- watch
- apiGroups:
- identity.me.cloud
resources:
- users
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- identity.me.cloud
resources:
- users/finalizers
verbs:
- update
- apiGroups:
- identity.me.cloud
resources:
- users/status
verbs:
- get
- patch
- update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think treating the yaml files as a structured object would be our solution here, in that way we wouldn't blindly replace the substring inside the yaml files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Role that has ns == "<project-name>-system" in the install YAML must be applied in the same namespace where the manager is installed.
If you added a namespace via the RBAC markers, it won’t be replaced with "{{ .Release.Namespace }}", because that substitution is tied to "<project-name>-system". So, unless you literally set the marker namespace to "-system" (which wouldn’t make sense), Helm won’t swap it to "{{ .Release.Namespace }}".
Did you test this locally?
If so, can you please share the exact steps to reproduce the issue that you have seen?
Description
Fixes handling of namespace-scoped RBAC resources (Role, RoleBinding) in cross-namespace scenarios for the helm/v2-alpha plugin.
Problem: When multiple namespace-scoped Roles/RoleBindings exist with the same name in different namespaces (e.g., for leader election in
production, cross-namespace permissions ininfrastructure), they would overwrite each other's files, causing missing RBAC permissions at runtime.Solution: This PR ensures:
{{ .Release.Namespace }}rbac/directory (neverextras/)Behavior
== manager{{ .Release.Namespace }}!= managerExamples
Example 1: Role in Manager Namespace
Input (from kustomize):
Output (Helm template):
dist/chart/templates/rbac/leader-election-role.yamlExample 2: Role in Cross-Namespace (infrastructure)
Input (from kustomize):
Output (Helm template):
dist/chart/templates/rbac/manager-role-infrastructure.yamlExample 3: RoleBinding in Cross-Namespace with Manager Subject
Input (from kustomize):
Output (Helm template):
dist/chart/templates/rbac/manager-rolebinding-infrastructure.yamlWhy?
metadata.namespace: infrastructure→ Cross-namespace binding, keep explicitsubjects[].namespace: test-project-system→ References controller's namespace, template itExample 4: RoleBinding with External ServiceAccount
Input (from kustomize):
Output (Helm template):
Closes: #5354