-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Make the TLS certificates management dynamic. #2233
Make the TLS certificates management dynamic. #2233
Conversation
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.
👍
First quick review of this 🐻 PR.
@@ -0,0 +1,46 @@ | |||
# HTTPS file provider | |||
|
|||
Træfik HTTPS certificates can be managed dynamically with a configuration file. This provider allows users to add/renew certificates which are not managed by [Let's Encrypt](https://letsencrypt.org). |
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.
one sentence, one line
``` | ||
|
||
- `EntryPoints` : Array which contains all the entrypoints which have to use the certificates. | ||
- `Certificate` : Pair of certificate and key files. Can be the path of files or content. |
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.
Could add an empty line at end of the file?
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.
Done.
provider/file/file.go
Outdated
@@ -6,6 +6,8 @@ import ( | |||
"path" | |||
"strings" | |||
|
|||
"os" |
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.
Could you sort imports?
provider/file/file.go
Outdated
} | ||
|
||
if _, err := os.Stat(watchItem); err != nil { | ||
log.Debugf("Impossible to watch %s : %s", watchItem, err.Error()) |
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.
log.Debugf("Impossible to watch %s : %s", watchItem, err.Error())
become
log.Debugf("Impossible to watch %s : %v", watchItem, err)
provider/https/file/file.go
Outdated
@@ -0,0 +1,116 @@ | |||
package httpsFile |
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.
you must use the folder name
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 don't understand provider/https/file/file.go
.
Why not provider/https/https.go
like other provider?
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.
Because, the subpackage provider/https
will contain other HTTPS providers (entrypoints, acme...) in the future.
provider/provider.go
Outdated
@@ -22,12 +22,17 @@ type Provider interface { | |||
Provide(configurationChan chan<- types.ConfigMessage, pool *safe.Pool, constraints types.Constraints) error | |||
} | |||
|
|||
// BaseProvider should be inherited by providers | |||
// RootProvider should be inherited (directly or indirectly) by all providers | |||
type RootProvider struct { |
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.
Seems not needed
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.
@ldez Can you precise what do you mean?
server/server.go
Outdated
@@ -18,6 +18,8 @@ import ( | |||
"sync" | |||
"time" | |||
|
|||
"strings" |
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.
Could you sort imports?
server/server.go
Outdated
server.startLeadership() | ||
server.routinesPool.Go(func(stop chan bool) { | ||
server.listenProviders(stop) | ||
// Start starts the s. |
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.
Start starts the s.
-> Start starts the server.
docs/configuration/entrypoints.md
Outdated
|
||
!!! note | ||
|
||
If an empty TLS configuration is done, default self-signed certificates are generated. |
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.
!!! note
If an empty TLS configuration is done, default self-signed certificates are generated.
become:
!!! note
If an empty TLS configuration is done, default self-signed certificates are generated.
docs/index.md
Outdated
@@ -50,6 +50,7 @@ Run it and forget it! | |||
- Access Logs (JSON, CLF) | |||
- [Let's Encrypt](https://letsencrypt.org) support (Automatic HTTPS with renewal) | |||
- High Availability with cluster mode | |||
- [Dynamic TLS configuration](/https-providers/file.md) thanks to watched file |
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.
Need to be also added to the readme.md
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.
Done
c1be558
to
e87fade
Compare
4d62005
to
5700733
Compare
This PR makes my spidey senses tingle |
|
5700733
to
90a2191
Compare
Modifications to merge HTTPS file provider and backend/frontend fiole provider are done. |
6b45c43
to
4a4d29b
Compare
Super excited for this to get merged and be available. My organization is a huge fail with the LetsEncrypt certs and I need this to move traefik to production. |
4a4d29b
to
cda69d0
Compare
server/server.go
Outdated
server.defaultConfigurationValues(configMsg.Configuration) | ||
currentConfigurations := server.currentConfigurations.Get().(types.Configurations) | ||
jsonConf, _ := json.Marshal(configMsg.Configuration) | ||
log.Debugf("Configuration received from provider %server: %server", configMsg.ProviderName, string(jsonConf)) |
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.
Could you please change %server
by %s
server/server.go
Outdated
jsonConf, _ := json.Marshal(configMsg.Configuration) | ||
log.Debugf("Configuration received from provider %server: %server", configMsg.ProviderName, string(jsonConf)) | ||
if configMsg.Configuration == nil || configMsg.Configuration.Backends == nil && configMsg.Configuration.Frontends == nil && configMsg.Configuration.TLSConfiguration == nil { | ||
log.Infof("Skipping empty Configuration for provider %server", configMsg.ProviderName) |
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.
Could you please change %server
by %s
server/server.go
Outdated
if configMsg.Configuration == nil || configMsg.Configuration.Backends == nil && configMsg.Configuration.Frontends == nil && configMsg.Configuration.TLSConfiguration == nil { | ||
log.Infof("Skipping empty Configuration for provider %server", configMsg.ProviderName) | ||
} else if reflect.DeepEqual(currentConfigurations[configMsg.ProviderName], configMsg.Configuration) { | ||
log.Infof("Skipping same configuration for provider %server", configMsg.ProviderName) |
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.
Could you please change %server
by %s
server/server.go
Outdated
lastReceivedConfigurationValue := server.lastReceivedConfiguration.Get().(time.Time) | ||
providersThrottleDuration := time.Duration(server.globalConfiguration.ProvidersThrottleDuration) | ||
if time.Now().After(lastReceivedConfigurationValue.Add(providersThrottleDuration)) { | ||
log.Debugf("Last %server configuration received more than %server, OK", configMsg.ProviderName, server.globalConfiguration.ProvidersThrottleDuration.String()) |
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.
Could you please change %server
by %s
server/server.go
Outdated
// last config received more than n server ago | ||
server.configurationValidatedChan <- configMsg | ||
} else { | ||
log.Debugf("Last %server configuration received less than %server, waiting...", configMsg.ProviderName, server.globalConfiguration.ProvidersThrottleDuration.String()) |
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.
Could you please change %server
by %s
server/server.go
Outdated
<-time.After(providersThrottleDuration) | ||
lastReceivedConfigurationValue := server.lastReceivedConfiguration.Get().(time.Time) | ||
if time.Now().After(lastReceivedConfigurationValue.Add(time.Duration(providersThrottleDuration))) { | ||
log.Debugf("Waited for %server configuration, OK", configMsg.ProviderName) |
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.
Could you please change %server
by %s
server/server.go
Outdated
} | ||
newConfigurations[configMsg.ProviderName] = configMsg.Configuration | ||
log.Infof("Server configuration reloaded on %server", server.serverEntryPoints[newServerEntryPointName].httpServer.Addr) |
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.
Could you please change %server
by %s
tls/certificate.go
Outdated
return content, nil | ||
} | ||
|
||
//CreateTLSConfig creates a TLS config from Certificate structures |
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.
Could you please add space after //
ed2ba69
to
679faba
Compare
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.
LGTM 👏
Nice work @nmengin
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.
👏 👏
docs/configuration/backends/file.md
Outdated
``` | ||
|
||
## Multiple `.toml` Files | ||
|
||
You could have multiple `.toml` files in a directory: | ||
You could have multiple `.toml` files in a directory (and recursively and its sub-directories): |
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.
and recursively in its sub-directories ?
integration/https_test.go
Outdated
defer func() { | ||
fileutils.CopyFile(confFileName+"_tmp", confFileName) | ||
os.Remove(confFileName + "_tmp") | ||
}() |
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.
WDYT about create a temp file from the original file (fixtures/https/dynamic_https.toml) and to dynamically change the file in your traefik configuration template file (with adaptFile
) instead of this saving mechanism ?
server/server.go
Outdated
} | ||
// Update the last (HTTPS) configuration loading time |
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.
It's not only the https loading time ?
679faba
to
1ed0ba5
Compare
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.
LGTM 👏 👏 👏 👍
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.
LGTM
1ed0ba5
to
089610a
Compare
Does this PR address the issue described here? #1597 In summary, can we now add new acme (letsencrypt) domains without restarting the service? The update to the documentation here seems to indicate that automatic letsencrypt registration behavior is not affected by this patch. If restarting the service is still required after adding a new acme letsencrypt domain, can someone reopen #1597 or is a new issue required? |
@mattcollier The issue #1597 is not addressed by this PR. In fact the internal mechanism has been developed but the file provider does not allow managing LE certificates, even if I'm going to re-open the issue #1597 which has not to be linked to the issue #1623 (this one is really solved by the PR 😉 ). |
Description
Today, the certificates are managed statically in the
EntryPoints
section.The goal of this PR is to allow dynamic TLS certificates management by using the existing provider mechanism.
A HTTPS file provider is created too in the way to allow managing a list of certificates thanks to a watched file.
Others providers will be able to be created to add others ways to manage provided certificates.
Fixes #595 #1557 #1623 #2057
This PR fix totally or partially needs described in #1160 #2005.