Skip to content

Support Etcd v3, enhance KV support #2407

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
Merged
traefiker merged 3 commits into from
Nov 17, 2017
Merged

Support Etcd v3, enhance KV support #2407

traefiker merged 3 commits into from
Nov 17, 2017

Conversation

@nmengin
Copy link
Contributor

@nmengin nmengin commented Nov 14, 2017 •
edited
Loading

What does this PR do?

In the way to migrate fix some bugs with KV store, essentially ETCDV3, this PR migrates Træfik to the abronan libkv fork we have to migrate first stært in the way to fix dependencies.

Motivation

Fixes #926 #1034 #2069 #1974 #1975

More

  • Added example for manual tests

Additional information

The new libkv version allows connections to ETCD through both V2 and V3 API.
In the way to avoid breaking change, the parameter userAPIV3 was added to the backend ETCD.

Note : Both the Atcd API V2 and the option userAPIV3 are deprecated. More information about deprecated option in proposal #2212.

juliens
juliens approved these changes Nov 14, 2017
View reviewed changes
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

@nmengin nmengin force-pushed the feature/libkv-migration branch from 7f770cd to 2de94fa Compare November 15, 2017 09:48
@nmengin nmengin force-pushed the feature/libkv-migration branch from 308f991 to 46be176 Compare November 16, 2017 07:49
@nmengin nmengin added this to the 1.5 milestone Nov 16, 2017
mmatur
mmatur requested changes Nov 16, 2017
View reviewed changes
glide.yaml Outdated
Copy link
Member

@mmatur mmatur Nov 16, 2017

Choose a reason for hiding this comment

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

Just a question. Why it is not the same version that is present in docker compose file v3.2.9 <> v3.2.7

Copy link
Contributor Author

@nmengin nmengin Nov 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Modification done in the docker-compose fileS.

integration/etcd_test.go Outdated
Copy link
Member

@mmatur mmatur Nov 16, 2017

Choose a reason for hiding this comment

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

Could you please replace 1000*time.Millisecond with 1*time.Second

@nmengin nmengin force-pushed the feature/libkv-migration branch from 46be176 to 38df4a1 Compare November 16, 2017 08:05
@nmengin nmengin force-pushed the feature/libkv-migration branch 2 times, most recently from d0a92cb to d2d6830 Compare November 16, 2017 08:57
mmatur
mmatur approved these changes Nov 16, 2017
View reviewed changes
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 👏

@nmengin nmengin force-pushed the feature/libkv-migration branch from 3eb92e5 to 67fbb54 Compare November 17, 2017 09:28
@nmengin nmengin force-pushed the feature/libkv-migration branch from 67fbb54 to 680fa6c Compare November 17, 2017 09:51
ldez
ldez previously approved these changes Nov 17, 2017
View reviewed changes
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 👍

emilevauge
emilevauge requested changes Nov 17, 2017
View reviewed changes
Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Thanks a lot @nmengin 😍
Few minor comments but overall, looks very good to me :)

docs/configuration/backends/etcd.md Outdated
Copy link
Member

@emilevauge emilevauge Nov 17, 2017

Choose a reason for hiding this comment

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

s/will be/is

examples/cluster/manage_cluster_docker_environment.sh Outdated
Copy link
Member

@emilevauge emilevauge Nov 17, 2017

Choose a reason for hiding this comment

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

247 lines of fun ;)

vomit

Are we sure we want to maintain this in the repo ?

Copy link
Contributor Author

@nmengin nmengin Nov 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@emilevauge For the moment I prefer keep it as long as we do not have end-to-end test about the cluster mode.
It allows generating easily a Dockerized Træfik cluster...

Moreover, shell is life... shell is like an unicorn or a rainbow or both...

whererainbow

provider/etcd/etcd.go Outdated
Copy link
Member

@emilevauge emilevauge Nov 17, 2017

Choose a reason for hiding this comment

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

Could you add a comment // TODO: Deprecated before ?

@ldez ldez dismissed their stale review November 17, 2017 10:50

I have some remark

@nmengin nmengin force-pushed the feature/libkv-migration branch from 680fa6c to 8e81b7e Compare November 17, 2017 13:03
@nmengin nmengin force-pushed the feature/libkv-migration branch from 8e81b7e to 2c51cbf Compare November 17, 2017 13:24
@nmengin nmengin force-pushed the feature/libkv-migration branch from 2c51cbf to 8c4a392 Compare November 17, 2017 15:48
@nmengin nmengin force-pushed the feature/libkv-migration branch from 4e2a0e9 to f7f86da Compare November 17, 2017 16:06
@traefiker traefiker merged commit 66e489a into traefik:master Nov 17, 2017
@ldez ldez changed the title Update libkv dependency Support Etcd v3, enhance KV support Nov 17, 2017
@nmengin nmengin deleted the feature/libkv-migration branch August 3, 2018 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@juliens juliens juliens approved these changes

@mmatur mmatur mmatur approved these changes

@emilevauge emilevauge emilevauge approved these changes

+1 more reviewer

@ldez ldez ldez left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/cluster area/provider kind/enhancement a new or improved feature. size/S

Projects

None yet

Milestone

1.5

Development

Successfully merging this pull request may close these issues.

letsEncrypt not working with etcd

6 participants

@nmengin @juliens @mmatur @ldez @emilevauge @traefiker