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(rbac): do not create clusterrole for namespace deployment on Traefik v3 #1012

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

ChandonPierre
Copy link
Contributor

A namespace scoped deployment should not create cluster scoped rbac.

@mloiseleur
Copy link
Contributor

@ChandonPierre Thanks. You found an interesting way for rbac namespaced.

Would you please add test and some documentation in values.yaml ?

@ChandonPierre
Copy link
Contributor Author

@ChandonPierre Thanks. You found an interesting way for rbac namespaced.

Would you please add test and some documentation in values.yaml ?

Updated!

Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

This PR only works with traefik v3.0.0.
Please update your if statements and your tests accordingly by adding something like:
{{- if semverCompare "<3.0.0-0" (include "imageVersion" $) }}

traefik/templates/_podtemplate.tpl Outdated Show resolved Hide resolved
@ChandonPierre
Copy link
Contributor Author

This PR only works with traefik v3.0.0. Please update your if statements and your tests accordingly by adding something like: {{- if semverCompare "<3.0.0-0" (include "imageVersion" $) }}

Yes, you are correct. Updated to use the previous behavior on Traefik V2, and disable ingressclass lookup/ClusterRole when Traefik V3 and namespaced

@mloiseleur mloiseleur changed the title fix(rbac): do not create clusterrole for namespace deployment fix(rbac): do not create clusterrole for namespace deployment on Traefik v3 Mar 8, 2024
@mloiseleur
Copy link
Contributor

@ChandonPierre In its current form, this PR will be breaking for v3 users, using spec.ingressClassname with rbac.namespaced enabled. They will be forced to add annotations.

Wdyt about:

  1. Introducing this new provider.kubernetesIngress.disableIngressClassLookup in values for v3 users
  2. When both provider.kubernetesIngress.disableIngressClassLookup and rbac.namespaced are set, then it won't generate ClusterRole

@ChandonPierre
Copy link
Contributor Author

ChandonPierre commented Mar 9, 2024

@ChandonPierre In its current form, this PR will be breaking for v3 users, using spec.ingressClassname with rbac.namespaced enabled. They will be forced to add annotations.

Wdyt about:

  1. Introducing this new provider.kubernetesIngress.disableIngressClassLookup in values for v3 users
  2. When both provider.kubernetesIngress.disableIngressClassLookup and rbac.namespaced are set, then it won't generate ClusterRole

I would argue the previous behvaior was a bug, or at the very least, a misnomer.

But I agree not regressing existing functionality. Updated PR with the suggested behavior.

Copy link
Contributor

@mloiseleur mloiseleur left a comment

Choose a reason for hiding this comment

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

Thanks 👍 LGTM

Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

LGTM.
I tested it, it works well, thanks for your contribution :-)

@traefiker traefiker merged commit d0f3442 into traefik:master Mar 15, 2024
1 check passed
@ChandonPierre ChandonPierre deleted the cpierre/namespace branch March 21, 2024 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants