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

feat: add file provider #937

Merged
merged 8 commits into from
Nov 7, 2023
Merged

feat: add file provider #937

merged 8 commits into from
Nov 7, 2023

Conversation

ebuildy
Copy link
Contributor

@ebuildy ebuildy commented Oct 14, 2023

What does this PR do?

As discussed at #936 , this PR add the option to configure a traefik file provider, as an extra configuration.

Motivation

  • Make migration to kubernetes easier: just copy/paste your traefik config
  • Allow to cover the case of multiple traefik instances, fix the multiple TLSStore issue

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

@mloiseleur mloiseleur added the kind/enhancement New feature or request label Oct 16, 2023
@mloiseleur
Copy link
Contributor

@ebuildy Can you detail the issue you are referring to about multiple TLSStore ?

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 but you might wanna add a test for values.yaml defaults. The test should check that:

  • no configMap is created
  • no extra volume is mounted
  • no CLI parameters nor annotations are added

@mloiseleur
Copy link
Contributor

@ebuildy Thanks for this. I just need more details on

Allow to cover the case of multiple traefik instances, fix the multiple TLSStore issue

And we should be good to go.

Since the TLSStore is configurable either with values or an extra resources, what is the problem when configuring multiple traefik instances ?

@ebuildy
Copy link
Contributor Author

ebuildy commented Nov 6, 2023

Well I didnt manage to "scope" TLSStore object (or any other CRD like Middleware) to a specific Traefik.

To me this is not possible to run multiple Traefik instances, since there is no "selector". To give you an example, I know prometheus ServiceMonitor have NamespaceSelector field: "Selector to select which namespaces the Endpoints objects are discovered from."

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.

Ok. I get it, thanks.
Thanks for this PR, too, it looks good and tested.

Would you please follow the FHS and use a path like /etc/traefik/dynamic instead of /opt/traefik/extra-conf ?

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

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

traefik/values.yaml Outdated Show resolved Hide resolved
@traefiker traefiker merged commit ddd6cee into traefik:master Nov 7, 2023
1 check passed
@mloiseleur mloiseleur linked an issue Nov 7, 2023 that may be closed by this pull request
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add file provider from a ConfigMap
5 participants