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.

Sign up for GitHub

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

Jump to bottom

HA acme support #625

Merged
merged 6 commits into from
Sep 30, 2016
Merged

HA acme support #625

merged 6 commits into from
Sep 30, 2016

Conversation

emilevauge
Copy link
Member

@emilevauge emilevauge commented Aug 17, 2016 •
edited
Loading

  • add distributed datastore based on staert & libkv
  • add leadership election
  • add datastore challenge provider
  • add acme datastore storage
  • add migration tool
  • documentation

Fixes #348

Signed-off-by: Emile Vauge emile@vauge.com

@emilevauge emilevauge force-pushed the add-ha-acme-support branch 3 times, most recently from f4bc12d to 31f53bd Compare August 17, 2016 23:33
@ekozan
Copy link

ekozan commented Sep 26, 2016

any news on this one ?

@emilevauge emilevauge force-pushed the add-ha-acme-support branch 7 times, most recently from 0e30491 to 20d94c1 Compare September 27, 2016 21:17
@emilevauge
Copy link
Member Author

@containous/traefik I still need to write some documentation on this, but the core part can now be reviewed :)

@emilevauge emilevauge added this to the 1.1 milestone Sep 27, 2016
@Russell-IO
Copy link
Contributor

LGTM - can add some docs & merge. migration is less important, because it should just provision a new let's encrypt certificate right ? maybe an import tool would be easier.

@emilevauge emilevauge changed the title [WIP] ha acme support HA acme support Sep 29, 2016
vdemeester
vdemeester approved these changes Sep 29, 2016
View reviewed changes
Copy link
Contributor

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

Few comments but overall LGTM 🐸

acme/acme.go
tlsConfig.Certificates = append(tlsConfig.Certificates, *a.defaultCertificate)
tlsConfig.GetCertificate = a.getCertificate

datastore, err := cluster.NewDataStore(
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch, this is hard to read 🙀 The use of a function with argument here instead of a plain old struct makes it really hard to read/know what is what 😓

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

acme/challengeProvider.go
func newWrapperChallengeProvider() *wrapperChallengeProvider {
return &wrapperChallengeProvider{
challengeCerts: map[string]*tls.Certificate{},
func newMemoryChallengeProvider(store cluster.Store) *challengeProvider {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method adds no real value over just using &challengeProvider{…} in the code 👼

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

acme/localStore.go
func NewLocalStore(file string) *LocalStore {
return &LocalStore{
file: file,
storageLock: sync.RWMutex{},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed ? doesn't the empty value already correctly set the lock ? (/me is confused 😄)

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, I changed from *sync.RWMutex and forgot to remove it ;)

@emilevauge
Add KV datastore
5a0440d 
Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge
Add leadership election
bea5ad3 
Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge
Add ACME store
a428455 
Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge
Challenge certs PEM encoding
e72e658 
Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge emilevauge force-pushed the add-ha-acme-support branch 2 times, most recently from 0c23e18 to f045793 Compare September 30, 2016 10:15
@emilevauge
Add documentation
bb29d9c 
Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge
Add ACME account to storeconfig command
4ad4b8e 
Signed-off-by: Emile Vauge <emile@vauge.com>
@emilevauge emilevauge merged commit d4da14c into master Sep 30, 2016
@emilevauge emilevauge deleted the add-ha-acme-support branch September 30, 2016 11:49
sprohaska
sprohaska reviewed Oct 27, 2016
View reviewed changes
acme/account.go
// If there's an error, we assume the cert is broken, and needs update
return true
}
// <= 7 days left, renew certificate
Copy link

Choose a reason for hiding this comment

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

You should adjust the comment to 30 days left.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sprohaska sprohaska sprohaska left review comments

@vdemeester vdemeester vdemeester approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
6 participants
@emilevauge @ekozan @Russell-IO @vdemeester @sprohaska @ldez