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

Support Etcd v3, enhance KV support #2407

Merged
merged 3 commits into from
Nov 17, 2017

Conversation

nmengin
Copy link
Contributor

@nmengin nmengin commented Nov 14, 2017

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.

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

@@ -36,13 +36,19 @@ import:
- tlsconfig
- package: github.com/docker/go-units
version: 9e638d38cf6977a37a8ea0078f3ee75a7cdb2dd1
- package: github.com/coreos/etcd
version: v3.2.9
Copy link
Member

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

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.

@@ -72,14 +84,12 @@ func (s *EtcdSuite) TestSimpleConfiguration(c *check.C) {

// TODO validate : run on 80
// Expected a 404 as we did not configure anything
err = try.GetRequest("http://127.0.0.1:8000/", 1000*time.Millisecond, try.StatusCodeIs(http.StatusNotFound))
err = try.GetRequest(traefikEtcdURL, 1000*time.Millisecond, try.StatusCodeIs(http.StatusNotFound))
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 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
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
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 👍

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 :)


!!! Note
the option `useAPIV3` allows using Etcd API V3 only if it's set to true.
This option will be **deprecated** and API V3 will be the only version compatible with Træfik.
Copy link
Member

Choose a reason for hiding this comment

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

s/will be/is

esac
}

main $@
Copy link
Member

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

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

etcdv3.Register()
p.SetStoreType(store.ETCDV3)
} else {
log.Warn("The ETCD API V2 is deprecated. Please use API V3 instead")
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 add a comment // TODO: Deprecated before ?

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

letsEncrypt not working with etcd
6 participants