Skip to content

fix(charts): Skip cluster-scope RBAC on namespaced#5843

Merged
k8s-ci-robot merged 5 commits intokubernetes-sigs:masterfrom
TobyTheHutt:5832-fix-clusterrole-creation-on-namespaced-scopes
Feb 27, 2026
Merged

fix(charts): Skip cluster-scope RBAC on namespaced#5843
k8s-ci-robot merged 5 commits intokubernetes-sigs:masterfrom
TobyTheHutt:5832-fix-clusterrole-creation-on-namespaced-scopes

Conversation

@TobyTheHutt
Copy link
Copy Markdown
Contributor

@TobyTheHutt TobyTheHutt commented Sep 16, 2025

What does it do ?

  • Restore tenant-friendly installs by keeping RBAC namespaced if .Values.gatewayNamespace is set
  • namespaced=true and gatewayNamespace set results in a namespaced Role and Rolebinding for listing namespaces
  • namespaced=true AND gatewayNamespace unset will retain the original ClusterRole and ClusterRoleBinding along with a Role and RoleBinding
  • namespace=false will retain the original ClusterRole and ClusterRoleBinding
  • No breaking changes introduced

The Helm Linter and Tests were run and the templates get rendered as expected:

  • helm template charts/external-dns --set namespaced=true --set sources[0]=gateway-httproute --set gatewayNamespace=gateway-ns
  • helm template charts/external-dns --set namespaced=true --set sources[0]=gateway-httproute
  • helm template charts/external-dns --set namespaced=false --set sources[0]=gateway-httproute

Motivation

When installing with namespaced=true and gateway sources, a270a32 added a ClusterRole/ClusterRoleBinding which breaks tenant setups that disallow cluster-scoped permissions.

This change keeps RBAC namespaced when .Values.gatewayNamespace is set.

Fixes: #5832

More

  • Yes, this PR title follows Conventional Commits
  • Yes, I added unit tests
  • Yes, I updated end user documentation accordingly

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. chart labels Sep 16, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 16, 2025
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Sep 16, 2025

Hi @TobyTheHutt. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 16, 2025
@TobyTheHutt
fix(charts): Skip cluster-scope RBAC on namespaced
b9476b6 
* Restore tenant-friendly installs by keeping RBAC namespaced if
  `.Values.gatewayNamespace` is set
* `namespaced=true` and `gatewayNamespace` set results in a namespaced
  Role and Rolebinding for listing namespaces
* `namespaced=true` AND `gatewayNamespace` unset will retain the
  original `ClusterRole` and `ClusterRoleBinding`
* `namespace=false` will retain the original `ClusterRole` and
  `ClusterRoleBinding`
* No breaking changes introduced

Ticket: kubernetes-sigs#5832

Signed-off-by: Tobias Harnickell <tobias.harnickell@bedag.ch>
@TobyTheHutt TobyTheHutt force-pushed the 5832-fix-clusterrole-creation-on-namespaced-scopes branch from 3e5dabb to b9476b6 Compare September 16, 2025 15:18
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 16, 2025
@AndrewCharlesHay
Copy link
Copy Markdown
Contributor

AndrewCharlesHay commented Sep 19, 2025

@TobyTheHutt

Thank you for addressing the RBAC scoping issue! The changes look well thought-out and restore tenant-friendly installs by correctly handling namespaced RBAC when .Values.gatewayNamespace is set. I appreciate the clear documentation and the test cases you provided.

A few points:

The logic for retaining ClusterRole/ClusterRoleBinding vs. Role/RoleBinding is clearly explained and matches expected behaviors for tenant scenarios.
Good job updating both unit tests and user documentation.
I verified the Helm template commands you listed and the rendered resources look correct.
Questions/Comments:

Are there any edge cases where setting both namespaced=true and leaving gatewayNamespace unset could lead to unexpected permission escalation?
Could you clarify if this change impacts upgrades for existing users who move from cluster-scoped to namespaced installs?
Overall, LGTM—thanks for the fix! Just want to make sure we’re covering all upgrade and edge scenarios.

@TobyTheHutt
Copy link
Copy Markdown
Contributor Author

TobyTheHutt commented Sep 19, 2025

@AndrewCharlesHay

Thanks for the review. To your questions:

Permission escalation risk:
I see no new risks introduced regarding this.
The only cluster-scoped permission granted is ["get","watch","list"] on namespaces. This did not change and behaves exactly as before.

Upgrade path
Users which are already running namespaced with gatewayNamespace set will now lose the previous cluster RBAC during upgrade. Helm will prune the *-nemaspaces ClusterRole and ClusterRoleBinding, as they are no longer rendered.
This is documented in:

https://github.com/kubernetes-sigs/external-dns/pull/5843/files#diff-c1cb383aca0c29f8ccafa3ef682fdee07fc3b068d461a4e64617b66d480d842fR61-R65

...and...

https://github.com/kubernetes-sigs/external-dns/pull/5843/files#diff-c1cb383aca0c29f8ccafa3ef682fdee07fc3b068d461a4e64617b66d480d842fR115

If a user moves from cluster-scoped to namespaced, it still flips the main RBAC objects from ClusterRole and ClusterRoleBinding to Role and RoleBinding, as this is treated as a usual kind swap in Helm. It will delete the cluster-scoped pair and replace them with namespaced equivalents.

BUT: Users need to remember to set gatewayNamespace during the same upgrade, if they want to avoid retaining the namespace-listing ClusterRole.

Next steps

I think the changes implemented are safe and any issues that may arise from them can be remedied with simple and obvious measures.
However, if desired, I could:

  • Add a chart warning regarding the upgrade limitations
  • Add a note in the docs, hinting at the described upgrade limitations
  • Add a test asserting the kind change

Let me know how we want to proceed.

@mloiseleur
Copy link
Copy Markdown
Collaborator

mloiseleur commented Sep 21, 2025

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 21, 2025
stevehipwell
stevehipwell suggested changes Sep 25, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @TobyTheHutt. I've commented on a minor nit to fix and could you also add this change to the chart CHANGELOG?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2025
@stevehipwell
Copy link
Copy Markdown
Contributor

stevehipwell commented Dec 10, 2025

@TobyTheHutt are you planning on continuing to work on this?

@stevehipwell stevehipwell mentioned this pull request Dec 11, 2025
Closed
@TobyTheHutt
Copy link
Copy Markdown
Contributor Author

TobyTheHutt commented Dec 11, 2025

@stevehipwell thanks for the ping. Life happened. Yes, I'll take up work again in the coming days.

TobyTheHutt and others added 2 commits January 16, 2026 06:46
@TobyTheHutt
Merge remote-tracking branch 'origin/master' into 5832-fix-clusterrol…
f716a57 
…e-creation-on-namespaced-scopes

# Conflicts:
#	charts/external-dns/tests/json-schema_test.yaml
@TobyTheHutt
fix: Add review feedback
2fa752f 
Signed-off-by: Tobias Harnickell <tobias@harnickell.ch>
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 20, 2026
@coveralls
Copy link
Copy Markdown

coveralls commented Jan 20, 2026 •
edited
Loading

Pull Request Test Coverage Report for Build 21328086724

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 79.002%
Totals Coverage Status
Change from base Build 21207630900: 0.0%
Covered Lines: 16031
Relevant Lines: 20292
💛 - Coveralls

@TobyTheHutt
fix: Ran helm-docs
395443e 
Signed-off-by: Tobias Harnickell <tobias@harnickell.ch>
@TobyTheHutt TobyTheHutt force-pushed the 5832-fix-clusterrole-creation-on-namespaced-scopes branch from 5006b41 to 395443e Compare January 25, 2026 05:56
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 25, 2026
@TobyTheHutt
doc: Update chart changelog
7b29dc5 
Signed-off-by: Tobias Harnickell <tobias@harnickell.ch>
@TobyTheHutt
Copy link
Copy Markdown
Contributor Author

TobyTheHutt commented Jan 25, 2026

@stevehipwell I implemented the proposed changes. Again, I'm really sorry for the long wait from my side.

@ivankatliarchuk
Copy link
Copy Markdown
Member

ivankatliarchuk commented Feb 20, 2026

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 20, 2026
stevehipwell
stevehipwell approved these changes Feb 27, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Feb 27, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stevehipwell

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2026
@k8s-ci-robot k8s-ci-robot merged commit ead9653 into kubernetes-sigs:master Feb 27, 2026
19 checks passed
ivankatliarchuk added a commit to gofogo/k8s-sigs-external-dns-fork that referenced this pull request Mar 10, 2026
@ivankatliarchuk
Merge branch 'master' into feat-metric-skipped_records_owner_mismatch…
c125f07 
…_total

* master: (21 commits)
  refactor(testutils): extract log test helpers into subpackage to fix (kubernetes-sigs#6236)
  chore(deps): bump mkdocs-material (kubernetes-sigs#6237)
  feat(endpoint): reject alias property on unsupported record types (kubernetes-sigs#6188)
  fix(charts): Skip cluster-scope RBAC on namespaced (kubernetes-sigs#5843)
  chore(deps): bump the dev-dependencies group across 1 directory with 3 updates (kubernetes-sigs#6226)
  feat(pdns): add --[no-]prefer-alias flag and alias annotation support (kubernetes-sigs#6129)
  fix(ci): failed to download the coveralls binary from GitHub releases (kubernetes-sigs#6228)
  docs: add external-dns-pscloud-webhook to New providers list (kubernetes-sigs#6214)
  fix(crd): allow trailing dot in CNAME targets (kubernetes-sigs#6218)
  docs: added deep wiki badge (kubernetes-sigs#6215)
  feat(crd): Support MX record with trailing dot (kubernetes-sigs#6163)
  chore(source): standardize sources with merge endpionts and deduplicate targets (kubernetes-sigs#6174)
  chore(store): Added RESTConfig() to ClientGenerator (kubernetes-sigs#6177)
  chore(ingress): clarify that both IP and Hostname are collected from LoadBalancer status (kubernetes-sigs#6138)
  chore(endpoint): added empty checks (kubernetes-sigs#6157)
  chore(linter): enable unparam (kubernetes-sigs#6160)
  fix(tlsutils): fix nil error wrapping and wrong env var in TLS config (kubernetes-sigs#6198)
  chore(endpoint): harden crypto (kubernetes-sigs#6197)
  feat(fqdn): Deduplicate and sort ExecTemplate output. Add functions (kubernetes-sigs#6173)
  benchmark(endpoint): endpoint benchmarks (kubernetes-sigs#6156)
  ...
ivankatliarchuk added a commit to gofogo/k8s-sigs-external-dns-fork that referenced this pull request Mar 10, 2026
@ivankatliarchuk
Merge branch 'master' into chore-standartize-source-constructors
81ec0af 
* master: (23 commits)
  refactor(testutils): extract log test helpers into subpackage to fix (kubernetes-sigs#6236)
  chore(deps): bump mkdocs-material (kubernetes-sigs#6237)
  feat(endpoint): reject alias property on unsupported record types (kubernetes-sigs#6188)
  fix(charts): Skip cluster-scope RBAC on namespaced (kubernetes-sigs#5843)
  chore(deps): bump the dev-dependencies group across 1 directory with 3 updates (kubernetes-sigs#6226)
  feat(pdns): add --[no-]prefer-alias flag and alias annotation support (kubernetes-sigs#6129)
  fix(ci): failed to download the coveralls binary from GitHub releases (kubernetes-sigs#6228)
  docs: add external-dns-pscloud-webhook to New providers list (kubernetes-sigs#6214)
  fix(crd): allow trailing dot in CNAME targets (kubernetes-sigs#6218)
  docs: added deep wiki badge (kubernetes-sigs#6215)
  feat(crd): Support MX record with trailing dot (kubernetes-sigs#6163)
  chore(source): standardize sources with merge endpionts and deduplicate targets (kubernetes-sigs#6174)
  chore(store): Added RESTConfig() to ClientGenerator (kubernetes-sigs#6177)
  chore(ingress): clarify that both IP and Hostname are collected from LoadBalancer status (kubernetes-sigs#6138)
  chore(endpoint): added empty checks (kubernetes-sigs#6157)
  chore(linter): enable unparam (kubernetes-sigs#6160)
  fix(tlsutils): fix nil error wrapping and wrong env var in TLS config (kubernetes-sigs#6198)
  chore(endpoint): harden crypto (kubernetes-sigs#6197)
  feat(fqdn): Deduplicate and sort ExecTemplate output. Add functions (kubernetes-sigs#6173)
  benchmark(endpoint): endpoint benchmarks (kubernetes-sigs#6156)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ivankatliarchuk ivankatliarchuk Awaiting requested review from ivankatliarchuk

1 more reviewer

@stevehipwell stevehipwell stevehipwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@ivankatliarchuk ivankatliarchuk

@stevehipwell stevehipwell

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. chart cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Helm Chart fails to install namespaced, into namespace, without cluster-level permissions.

7 participants

@TobyTheHutt @k8s-ci-robot @AndrewCharlesHay @mloiseleur @stevehipwell @coveralls @ivankatliarchuk