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

Make the TLS certificates management dynamic. #2233

Merged
merged 4 commits into from
Nov 9, 2017

Conversation

nmengin
Copy link
Contributor

@nmengin nmengin commented Oct 9, 2017

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.

Copy link
Contributor

@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.

👍

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).
Copy link
Contributor

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.
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -6,6 +6,8 @@ import (
"path"
"strings"

"os"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you sort imports?

}

if _, err := os.Stat(watchItem); err != nil {
log.Debugf("Impossible to watch %s : %s", watchItem, err.Error())
Copy link
Contributor

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)

@@ -0,0 +1,116 @@
package httpsFile
Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor Author

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.

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems not needed

Copy link
Contributor Author

@nmengin nmengin Oct 10, 2017

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"
Copy link
Contributor

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.
Copy link
Contributor

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.


!!! note

If an empty TLS configuration is done, default self-signed certificates are generated.
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@dtomcej
Copy link
Contributor

dtomcej commented Oct 13, 2017

This PR makes my spidey senses tingle
🕷

@nmengin
Copy link
Contributor Author

nmengin commented Oct 13, 2017

I am doing a new refacto in the way to merge file and httpsFile providers.

Do not review the code yet

@nmengin nmengin force-pushed the feature/create-ssl-file-provider branch from 5700733 to 90a2191 Compare October 20, 2017 15:15
@nmengin
Copy link
Contributor Author

nmengin commented Oct 20, 2017

Modifications to merge HTTPS file provider and backend/frontend fiole provider are done.
Documentation has to be updated but code can be reviewed yet!

@ornith-it-admin
Copy link

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.

@nmengin nmengin force-pushed the feature/create-ssl-file-provider branch from 4a4d29b to cda69d0 Compare November 2, 2017 13:41
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))
Copy link
Member

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)
Copy link
Member

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)
Copy link
Member

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())
Copy link
Member

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())
Copy link
Member

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)
Copy link
Member

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)
Copy link
Member

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

return content, nil
}

//CreateTLSConfig creates a TLS config from Certificate structures
Copy link
Member

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 //

Copy link
Member

@mmatur mmatur left a 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

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.

👏 👏

```

## 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):
Copy link
Member

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 ?

defer func() {
fileutils.CopyFile(confFileName+"_tmp", confFileName)
os.Remove(confFileName + "_tmp")
}()
Copy link
Member

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
Copy link
Member

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 ?

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 👏 👏 👏 👍

Copy link
Contributor

@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

@traefiker traefiker force-pushed the feature/create-ssl-file-provider branch from 1ed0ba5 to 089610a Compare November 9, 2017 11:04
@traefiker traefiker merged commit c469e66 into traefik:master Nov 9, 2017
@mattcollier
Copy link
Contributor

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?

@nmengin
Copy link
Contributor Author

nmengin commented Nov 10, 2017

@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 acme.domains are declared in the TOML file.
Another provider has to be developed in this way.

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 😉 ).

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.

File Watch Doesn't Work
9 participants