Skip to content
This repository has been archived by the owner on Feb 1, 2021. It is now read-only.

Add TLS support for libkv #1254

Merged
merged 1 commit into from
Oct 12, 2015
Merged

Add TLS support for libkv #1254

merged 1 commit into from
Oct 12, 2015

Conversation

dhiltgen
Copy link
Contributor

@dhiltgen dhiltgen commented Oct 2, 2015

This adds TLS support into the KV store for swarm. The manage, join,
and list commands all have a new CLI argument, matching the docker engine
discovery backend. This required adding the tlsconfig utility
package from docker engine.

Here's an example showing re-use of the cluster certs for the KV store:

swarm manage --tlsverify \
    --tlscacert /etc/docker/ssl/ca.pem
    --tlscert /etc/docker/ssl/cert.pem
    --tlskey /etc/docker/ssl/key.pem
    --discovery-opt kv.cacertfile=/etc/docker/ssl/ca.pem
    --discovery-opt kv.certfile=/etc/docker/ssl/cert.pem
    --discovery-opt kv.keyfile=/etc/docker/ssl/key.pem
    --advertise 192.168.122.47:3376
    etcd://192.168.122.47:2379

Signed-off-by: Daniel Hiltgen [email protected]

@dhiltgen
Copy link
Contributor Author

dhiltgen commented Oct 2, 2015

If we choose to make any changes to the CLI args, we probably want to keep them in sync with moby/moby#16644

@dhiltgen
Copy link
Contributor Author

dhiltgen commented Oct 5, 2015

Once docker/libkv#75 is merged and vendored into swarm, I'll squash the second commit on this change.

@dhiltgen
Copy link
Contributor Author

dhiltgen commented Oct 7, 2015

Added workaround for libkv/etcd https bug in case we decide not to bump libkv version. The fix should be benign if we do decide to bump libkv

@abronan
Copy link
Contributor

abronan commented Oct 8, 2015

LGTM. (CI unhappy but this will be fixed soon)

@abronan
Copy link
Contributor

abronan commented Oct 8, 2015

Also go vet is complaining, not sure why 😕

@thaJeztah
Copy link
Member

FYI moby/moby#16644 was merged

@abronan
Copy link
Contributor

abronan commented Oct 9, 2015

ping @docker/swarm-maintainers

@aluzzardi
Copy link
Contributor

@dhiltgen It's a bit strange to use --cluster-store-opt since we don't have a --cluster-store flag like the engine does.

Perhaps --discovery-opt?

@dhiltgen
Copy link
Contributor Author

@aluzzardi I was shooting for consistency with engine, but I'm fine with your proposed flag names. I'll make the change now and will push an update to the PR shortly.

@aluzzardi
Copy link
Contributor

Does anyone have an option on flags consistency?

/cc @docker/swarm-maintainers

@dhiltgen dhiltgen force-pushed the tls_kv branch 2 times, most recently from 4d72eb5 to 2a75173 Compare October 12, 2015 19:56
@dhiltgen
Copy link
Contributor Author

Updated with new flag name: --discovery-opt

@@ -14,7 +14,7 @@ var (
Name: "list",
ShortName: "l",
Usage: "List nodes in a cluster",
Flags: []cli.Flag{flTimeout},
Flags: []cli.Flag{flTimeout, flClusterStoreOpt},
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably change all the occurrences mentioning ClusterStoreOpt or ClusterOpt to DiscoveryOpt.

@dhiltgen
Copy link
Contributor Author

Renamed the internal var's to match the flag name change.

This adds TLS support into the KV store for swarm.  The manage, join,
and list commands all have a new CLI argument, matching the docker engine
discovery backend.  This required adding the tlsconfig utility
package from docker engine.

Here's an example showing re-use of the cluster certs for the KV store:

    swarm manage --tlsverify \
        --tlscacert /etc/docker/ssl/ca.pem
        --tlscert /etc/docker/ssl/cert.pem
        --tlskey /etc/docker/ssl/key.pem
        --discovery-opt kv.cacertfile=/etc/docker/ssl/ca.pem
        --discovery-opt kv.certfile=/etc/docker/ssl/cert.pem
        --discovery-opt kv.keyfile=/etc/docker/ssl/key.pem
        --advertise 192.168.122.47:3376
        etcd://192.168.122.47:2379

Signed-off-by: Daniel Hiltgen <[email protected]>
@dhiltgen
Copy link
Contributor Author

Missed a couple renames - fixed now.

@abronan
Copy link
Contributor

abronan commented Oct 12, 2015

Thanks @dhiltgen! LGTM

ping @aluzzardi for the merge

@aluzzardi
Copy link
Contributor

LGTM

aluzzardi added a commit that referenced this pull request Oct 12, 2015
Add TLS support for libkv
@aluzzardi aluzzardi merged commit 1d008a7 into docker-archive:master Oct 12, 2015
ChristianKniep pushed a commit to ChristianKniep/swarm that referenced this pull request Jul 27, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants