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

Use KvStores as global config sources #481

Merged
merged 6 commits into from Jul 21, 2016
Merged

Use KvStores as global config sources #481

merged 6 commits into from Jul 21, 2016

Conversation

cocap10
Copy link

@cocap10 cocap10 commented Jun 24, 2016 •
edited

This PR :

TODO :

  • Load file instead of filename
  • Add a commun structure TLSClient
  • Improve tests :
    • Test with a full config
  • Add TLS support:
    • for consul
    • for ETCD
  • Update doc
  • Review :
    • Code
    • Doc

@vdemeester vdemeester added this to the 1.1 milestone Jun 24, 2016
emilevauge
emilevauge reviewed Jun 24, 2016
View reviewed changes
traefik.go
strings.Split(traefikConfiguration.Consul.Endpoint, ","),
nil,
strings.TrimPrefix(traefikConfiguration.Consul.Prefix, "/"), // TrimPrefix should be done in https://github.com/docker/libkv/blob/master/store/consul/consul.go#L113 : IDK why it doen't work
)
Copy link
Member

Choose a reason for hiding this comment

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

Consul TLS should be added.

@cocap10 cocap10 force-pushed the global-config-kv branch 3 times, most recently from 0d4bd55 to 6deab60 Compare June 27, 2016 14:09
@emilevauge emilevauge mentioned this pull request Jul 6, 2016
Closed
@cocap10 cocap10 force-pushed the global-config-kv branch 2 times, most recently from 30536eb to 0b6fda8 Compare July 12, 2016 11:17
@emilevauge emilevauge changed the title [WIP] Use KvStores as global config sources Use KvStores as global config sources Jul 12, 2016
emilevauge
emilevauge reviewed Jul 12, 2016
View reviewed changes
docs/basics.md
- arguments
- configuration file
- default

It means that arguments overrides configuration file.
It means that arguments overrides configuration file, and Key-value Store override arguments.
Copy link
Member

Choose a reason for hiding this comment

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

overrides

@emilevauge
Copy link
Member

Really go job @cocap10 :) I made few comments.
On the road to a fully distributed traefik ;)
/cc @containous/traefik

@cocap10 cocap10 force-pushed the global-config-kv branch 3 times, most recently from 3ee7fb8 to c51af7b Compare July 18, 2016 15:24
@cocap10
Copy link
Author

cocap10 commented Jul 18, 2016

rebased :)

cocap10
cocap10 reviewed Jul 19, 2016
View reviewed changes
provider/docker.go
Key string `description:"TLS key"`
InsecureSkipVerify bool `description:"TLS insecure skip verify"`
}

func (provider *Docker) createClient() (client.APIClient, error) {
var httpClient *http.Client
httpHeaders := map[string]string{
// FIXME(vdemeester) use version here O:)
Copy link
Author

Choose a reason for hiding this comment

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

What about this @vdemeester ? 👼

Copy link
Contributor

Choose a reason for hiding this comment

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

ah 😸.
Basic idea is to put the version of traefik in the http header (user-agent) so that we can debug easier looking at docker logs 😝.

This should be done, but not really related to this PR 👼

vdemeester
vdemeester reviewed Jul 19, 2016
View reviewed changes
docs/basics.md
Træfɪk's configuration has two parts:

- The [static Træfɪk configuration](/basics#static-trfk-configuration) which is loaded only at the begining.
- The [dynamic Træfɪk configuration](/basics#dynamic-trfk-configuration) which can be hot-reloaded (no need to restart the process).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should say the the dynamic configuration is where backend are defined (and potentially watched). wdyt ?

Copy link
Author

Choose a reason for hiding this comment

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

Mmm, we setup the connection to configuration backends in the static configuration, right ?
The dynamic configuration is about route rules provided by configuration backends

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm, that's true 😅

Copy link
Member

Choose a reason for hiding this comment

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

I think that you are both right :D

Maybe we should say the the dynamic configuration is where backend are defined (and potentially watched). wdyt ?

He meant frontends, backends and servers

Mmm, we setup the connection to configuration backends in the static configuration, right ?
The dynamic configuration is about route rules provided by configuration backends

He meant configuration backends = providers ;)

Copy link
Author

Choose a reason for hiding this comment

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

So, should I change anything ? ;)

@vdemeester
Copy link
Contributor

Few nits, but overall LGTM 🐯

@cocap10 cocap10 force-pushed the global-config-kv branch 2 times, most recently from e22227d to 5107596 Compare July 20, 2016 09:55
@cocap10
Copy link
Author

cocap10 commented Jul 20, 2016

re-rebased ;)

@vdemeester
Copy link
Contributor

@cocap10 needs another rebase 😓

@cocap10
Copy link
Author

cocap10 commented Jul 21, 2016

done ^^

@vdemeester
Copy link
Contributor

yay it's green 💚

@vdemeester vdemeester merged commit 686c23d into traefik:master Jul 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
area/provider/kv
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@cocap10 @emilevauge @vdemeester @ldez