Add Consul & BoltDB datasource support #178

stuart-c · 2017-07-31T22:02:50Z

Currently tested with Consul, but will be possible to enable etcd, zookeeper & boltdb

stuart-c · 2017-07-31T22:20:17Z

Please note integration tests are currently failing due to the Docker image not containing Consul

hairyhenderson · 2017-08-01T01:21:47Z

Please note integration tests are currently failing due to the Docker image not containing Consul

Ok, cool - I'm adding consul in the image - will see if that works.

hairyhenderson · 2017-08-01T01:30:22Z

Ok, I've added consul in hairyhenderson/dockerfiles#12 - I'll see if I can get CircleCI to rebuild.

hairyhenderson · 2017-08-01T04:15:42Z

libkv.go

+	sourceType = ""
+	client = ""
+
+	if strings.HasPrefix(url, "consul:") {


If you change the function's signature to accept a *url.URL, you can switch on url.Scheme instead - would probably be a little less verbose, and cleaner

Yep. Had thought the same as I nodded off last night. Much cleaner and needed anyway for the BoltDB support

hairyhenderson · 2017-08-01T11:10:12Z

Looks like the Appveyor test is failing because it's testing a merged build. You'll need to rebase and adjust your use of Getenv - I've moved it to a different package (github.com/hairyhenderson/gomplate/env).

Were you planning on adding integration tests for etcd, zk and boltdb?

I would like to see unit tests too, if possible 🙂

Also, from looking at how NewLibKV is used, it seems like it won't be possible to mix datasources. I'm not sure it'll be a common use-case, but we do need to be able to do something like gomplate -d c=consul:// -d b=boltdb:///foo/bar.db ...

stuart-c · 2017-08-01T11:27:24Z

Yes I'm hoping to add additional integration tests (once I figure out how to do so in a sensible way).

The intention is to be able to specify multiple datasources & mix the types. What are you seeing that might not make that possible?

hairyhenderson · 2017-08-01T18:16:55Z

The intention is to be able to specify multiple datasources & mix the types. What are you seeing that might not make that possible?

Clearly it was too early in the morning for me when I read that code 😂 - I think I was thinking that source.KV is cached, and so further calls to readLibKV for that source would be set to a specific type... Obviously that's not a problem because the source would be different! Next time I'll drink more coffee first... ☕️ 🤦‍♂️

stuart-c · 2017-08-01T18:39:27Z

Good. At least I've not messed something up

hairyhenderson · 2017-08-02T13:45:48Z

libkv.go

+			}
+			options.ConnectionTimeout = time.Duration(num) * time.Second
+		}
+		if ssl := env.Getenv("CONSUL_HTTP_SSL", ""); ssl != "" {


Why is this necessary? Wouldn't the CONSUL_HTTP_ADDR's scheme reflect whether it's HTTPS or not?

I do see that it doesn't have a scheme in the default above, and I think that should probably be changed.

Additionally, what might make sense (maybe in a followup PR though?) is to also support a consul:// URL that has the host in it, something like:

$ gomplate -d consul=consul+http://consul.server:8500/ -i '{{ ds "consul" "mykey" }}' some value from consul...

Or with HTTPS:

$ gomplate -d consul=consul+https://tls-protected-consul.server:8533/ -i '{{ ds "consul" "mykey" }}' some value from consul...

Unfortunately CONSUL_HTTP_ADDR doesn't specify if it is TLS or not. Instead there is a separate CONSUL_HTTP_SSL option which needs setting to true if using TLS.

I'll look at adding consul+http / consul+https as additional options.

@stuart-c hmm... on the consul docs, CONSUL_HTTP_ADDR doesn't refer to a remote Consul agent, but the local Consul agent that you're starting. So maybe we're overloading it in a bit of a confusing way.

Also, from the docs:

This can also be specified via the CONSUL_HTTP_ADDR environment variable. The default value is http://127.0.0.1:8500. The scheme can also be set to HTTPS by setting the environment variable CONSUL_HTTP_SSL=true.

In that example (in https://www.consul.io/docs/commands/index.html) a scheme is used, but in a further example for CONSUL_HTTP_ADDR directly, the value is scheme-less.

I'd recommend changing to CONSUL_URL to reduce the confusion (and potential conflicts), and using the scheme to reflect TLS or not.

CONSUL_HTTP_ADDR is the expected environment variable:

http://techblog.zeomega.com/devops/golang/2015/06/09/consul-kv-api-in-golang.html

The reason it talks about "local agent" is that the expected usage is you run an agent on every machine (and therefore always refer to it as localhost), but the full servers (1, 3 or 5) are the ones which actually store the data and are talked to via the agents.

The go API will automatically pick up that env variable, it is just libkv which overwrites it.

I think it would be more confusing for users of Consul to have all the normal env variables (e.g. CONSUL_HTTP_TOKEN - as per https://www.consul.io/docs/commands/index.html#environment-variables) but then something totally different for the server address (a bit like we are using the same env variable for Vault as you would with the Vault CLI).

Added support for consul+https, etc.

The go API will automatically pick up that env variable, it is just libkv which overwrites it.

Ok, I understand.

Added support for consul+https, etc.

👍

hairyhenderson · 2017-08-02T13:49:50Z

libkv.go

 	}
 	if url.Scheme == "boltdb" {
 		boltdb.Register()
 		sourceType = store.BOLTDB
 		client = url.Path
 		if client == "" {
-			client = env.Getenv("BOLTDB_PATH", "")
+			client = env.Getenv("BOLTDB_DATABASE", "")


Is there a reason you wouldn't just use url.Path all the time? Or is there a use-case for setting the path with an environment variable?

I don't mind either way

Ok, let's keep it simple and start by only supporting url.Path - it's easy to add it later if necessary :)

hairyhenderson · 2017-08-02T13:54:14Z

libkv.go

+			logFatal("boltdb missing bucket")
+		}
+		if timeout := env.Getenv("BOLTDB_TIMEOUT", ""); timeout != "" {
+			num, err := strconv.ParseInt(timeout, 10, 16)


Just a curiosity, why 16 here?

No real reason

…passing BoltDB filename

…nvironment variables

…+http & etcd+https schemes

hairyhenderson · 2017-08-02T18:04:04Z

libkv/libkv.go

+		if err != nil {
+			logFatal("consul setup error", err)
+		}
+		s = setup


Seems like an unnecessary assignment - could just name the variable setup on ln39 and remove this (and the rest of the occurrences below

hairyhenderson · 2017-08-02T18:10:46Z

test/integration/datasources_consul.bats

+  gomplate -d consul=consul:// -i '{{(datasource "consul" "foo")}}'
+  [ "$status" -eq 0 ]
+  [[ "${output}" == "$BATS_TEST_DESCRIPTION" ]]
+}


Can you add the following tests:

@test "Consul datasource works with hostname in URL" { consul kv put foo "$BATS_TEST_DESCRIPTION" unset CONSUL_HTTP_ADDR gomplate -d consul=consul+http://localhost:8500/ -i '{{(datasource "consul" "foo")}}' [ "$status" -eq 0 ] [[ "${output}" == "$BATS_TEST_DESCRIPTION" ]] } @test "Consul datasource works with consul+http scheme" { consul kv put foo "$BATS_TEST_DESCRIPTION" unset CONSUL_HTTP_ADDR gomplate -d consul=consul+http:// -i '{{(datasource "consul" "foo")}}' [ "$status" -eq 0 ] [[ "${output}" == "$BATS_TEST_DESCRIPTION" ]] }

I guess it'd be pretty onerous to test consul+https because we'd have to set up a self-signed cert... So no need for that now.

hairyhenderson · 2017-08-02T18:14:15Z

@stuart-c As useful as it is to have etcd and ZooKeeper support "for free" from libkv, I'm a little uncertain about including those from the get-go in this PR. Mostly because it'd add some extra complication in the tests which I'm really not sure is necessary right now. BoltDB seems simple so maybe useful to keep in for now.

How do you feel about removing the etcd and zk support, in the interests of focusing on getting solid consul and boltdb support in sooner?

stuart-c · 2017-08-02T19:42:53Z

I've removed Etcd & Zookeeper from this PR (will re-add via separate PRs).

I've added integration tests for BoltDB

hairyhenderson · 2017-08-02T20:19:08Z

test/integration/datasources_consul.bats

+
+@test "Consul datasource works with consul+http scheme" {
+  consul kv put foo "$BATS_TEST_DESCRIPTION"
+  unset CONSUL_HTTP_ADDR


oops - this actually won't work (since the addr isn't getting set in the URL on the next line)

It'd probably be best to set CONSUL_HTTP_ADDR explicitly in setup

hairyhenderson · 2017-08-02T20:20:00Z

I've removed Etcd & Zookeeper from this PR (will re-add via separate PRs).

Looks like there are still dependencies in vendor/ from these - can you remove those?

I've added integration tests for BoltDB

Great!

hairyhenderson · 2017-08-02T20:21:21Z

@stuart-c is there anything else you want to do with this PR, or do you think it's ready to go? I'm not seeing any unit tests, but I can add those later.

There's also the question of docs - I can add those later too.

stuart-c · 2017-08-02T21:25:39Z

I've just added some documentation. If you are happy looking after unit tests then I don't think I have anything further to add...

hairyhenderson · 2017-08-03T00:21:26Z

Awesome! Thanks for the docs, they look good.

Thanks so much for persisting with this, @stuart-c, I really appreciate it!

hairyhenderson

LGTM!

stuart-c force-pushed the add-libkv branch 2 times, most recently from dffe7a4 to bf8e501 Compare July 31, 2017 22:11

hairyhenderson mentioned this pull request Aug 1, 2017

Adding consul into the build image, for testing hairyhenderson/dockerfiles#12

Merged

hairyhenderson reviewed Aug 1, 2017

View reviewed changes

stuart-c force-pushed the add-libkv branch 2 times, most recently from 1285da6 to b58787e Compare August 1, 2017 16:00

hairyhenderson reviewed Aug 2, 2017

View reviewed changes

stuart-c added 9 commits August 2, 2017 15:46

Add libkv support

3990306

Add vendoring

96236d4

Fix linting errors

42f2e65

Code review changes: Switch to using URL instead of string. Also fix …

9ab30e4

…passing BoltDB filename

Fix due to rebase since env was moved to its own package

926250a

Add a load of configuration options and fix boltdb

b807bd4

Split into multiple funcs

6d76d1f

Add TLS configuration

730dfd4

Move to package

bc70f24

stuart-c force-pushed the add-libkv branch from 8dedf5d to bc70f24 Compare August 2, 2017 14:46

stuart-c added 2 commits August 2, 2017 17:42

Code review comments: Remove support for setting file & bucket from e…

e5c3f31

…nvironment variables

Code review comments: Add support for consul+http, consul+https, etcd…

2121177

…+http & etcd+https schemes

hairyhenderson reviewed Aug 2, 2017

View reviewed changes

stuart-c added 3 commits August 2, 2017 20:01

Code review comment: Remove Etcd & Zookeeper support for now

d203f5e

Code review comment: Simplify variable usage

2c305da

Code review comment: Add extra Consul integration tests

4c7f422

Add integration tests for BoldDB & fix implementation

27f7202

stuart-c force-pushed the add-libkv branch from 2a10416 to 27f7202 Compare August 2, 2017 19:45

hairyhenderson reviewed Aug 2, 2017

View reviewed changes

hairyhenderson added enhancement api/minor labels Aug 2, 2017

stuart-c added 2 commits August 2, 2017 21:42

Code review comment: Remove old vendor dependencies

e813997

Add some documentation

ed74c9e

hairyhenderson approved these changes Aug 3, 2017

View reviewed changes

hairyhenderson merged commit ab59ea0 into hairyhenderson:master Aug 3, 2017

hairyhenderson changed the title ~~Add libkv support~~ Add Consul & BoltDB datasource support Aug 3, 2017

stuart-c deleted the add-libkv branch August 3, 2017 02:06

hairyhenderson mentioned this pull request Aug 3, 2017

Consul support #173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Consul & BoltDB datasource support #178

Add Consul & BoltDB datasource support #178

stuart-c commented Jul 31, 2017

stuart-c commented Jul 31, 2017

hairyhenderson commented Aug 1, 2017

hairyhenderson commented Aug 1, 2017

hairyhenderson Aug 1, 2017

stuart-c Aug 1, 2017

hairyhenderson commented Aug 1, 2017

stuart-c commented Aug 1, 2017

hairyhenderson commented Aug 1, 2017

stuart-c commented Aug 1, 2017

hairyhenderson Aug 2, 2017

stuart-c Aug 2, 2017

hairyhenderson Aug 2, 2017

stuart-c Aug 2, 2017

stuart-c Aug 2, 2017

hairyhenderson Aug 2, 2017

hairyhenderson Aug 2, 2017

stuart-c Aug 2, 2017

hairyhenderson Aug 2, 2017

stuart-c Aug 2, 2017

hairyhenderson Aug 2, 2017

stuart-c Aug 2, 2017

hairyhenderson Aug 2, 2017

hairyhenderson Aug 2, 2017

hairyhenderson Aug 2, 2017

hairyhenderson commented Aug 2, 2017

stuart-c commented Aug 2, 2017

hairyhenderson Aug 2, 2017

hairyhenderson commented Aug 2, 2017

hairyhenderson commented Aug 2, 2017

stuart-c commented Aug 2, 2017

hairyhenderson commented Aug 3, 2017

hairyhenderson left a comment

Add Consul & BoltDB datasource support #178

Add Consul & BoltDB datasource support #178

Conversation

stuart-c commented Jul 31, 2017

stuart-c commented Jul 31, 2017

hairyhenderson commented Aug 1, 2017

hairyhenderson commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hairyhenderson commented Aug 1, 2017

stuart-c commented Aug 1, 2017

hairyhenderson commented Aug 1, 2017

stuart-c commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hairyhenderson commented Aug 2, 2017

stuart-c commented Aug 2, 2017

Choose a reason for hiding this comment

hairyhenderson commented Aug 2, 2017

hairyhenderson commented Aug 2, 2017

stuart-c commented Aug 2, 2017

hairyhenderson commented Aug 3, 2017

hairyhenderson left a comment

Choose a reason for hiding this comment