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

Check for dynamic tls updates on configuration preload #4022

Merged
merged 2 commits into from Jan 29, 2019

Conversation

ffilippopoulos
Copy link
Contributor

@ffilippopoulos ffilippopoulos commented Oct 11, 2018

What does this PR do?

Changes to look for tls certificate changes during preLoadConfiguration function. The aim is to solve issue: #3272 and make reload configuration because of cert changes when touch toml files.

Motivation

We are hitting this issue because we run an HA traefik setup in a kubernetes cluster and we use cert-manager to manage/update certificates. Each time a certificate is updated we have to go and restart traefik instances atm.

More

  • Added/updated tests
    added one test TestListenProvidersPublishWhenTLSCertChange under server/server_tests.go that loads a conf that includes TLS, then provides the same conf 2 times and expect no changes and finally rewrite new certs and provide the same conf that should trigger a reload.

@ffilippopoulos

This comment has been minimized.

@dbachrach
Copy link

We are running into a similar issue where cert-manager updates the secrets but the Traefik pods need to be explicitly killed. I had a question on this part though:

The aim is to solve issue: #3272 and make reload configuration because of cert changes when touch toml files.

Should you have to touch the toml file at all? Should a modification to the TLS files be all that's needed? If not, what aspect in the cert-manager update process will touch the toml file to trigger a reload in traefik?

@ffilippopoulos
Copy link
Contributor Author

@dbachrach that is proposed/discussed here: #3083 and is more of an enhancement. If you read through the issue there is a small explanation on why this mechanism (touching the toml file) was picked. This PR is just trying to solve the bug that is linked in the description.

Arguably this is still not going to be the ideal approach in our (and your) case as you still have to run a sidecar to watch the certs (i am using inotify for that) and touch the toml file in case of change.

@ffilippopoulos
Copy link
Contributor Author

Is there anything we can do to help this one progress? We are happy to review code if there are any concerns or anything similar. It is just holding us to migrate from tls termination on elbs as we would have to go and manually restart traefiks every couple of months which can be disturbing.

@ffilippopoulos ffilippopoulos requested review from a team as code owners January 29, 2019 14:37
@juliens juliens changed the base branch from master to v1.7 January 29, 2019 14:50
@juliens juliens added kind/bug/fix a bug fix and removed kind/enhancement a new or improved feature. labels Jan 29, 2019
@ldez ldez removed request for a team and juliens January 29, 2019 15:30
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

@juliens
Copy link
Member

juliens commented Jan 29, 2019

Thank you for your PR.

I think that this is better to replace file path with the content directly in the provider file. With this, it will fix the bug because the loadConfiguration will see the content modification.

I did the modification in order to merge it in the next bug fix release.

Copy link
Member

@jbdoumenjou jbdoumenjou 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
Member

@juliens juliens left a comment

Choose a reason for hiding this comment

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

LGTM

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.

None yet

8 participants