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

Update vendor libraries to fix tls support #568

Open
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@josegonzalez
Copy link
Member

josegonzalez commented Jun 15, 2017

Closes #565

mattatcha and others added some commits Jun 14, 2017

Merge pull request #566 from Sisir-Chowdhury/vendor
Environment Variable name changed to CONSUL_CLIENT_CERT & CONSUL_CLIE…
&& go build -ldflags "-X main.Version=$(cat VERSION)" -o /bin/registrator \
&& rm -rf /go \
&& apk del --purge build-deps
FROM golang:1.8.3 as builder

This comment has been minimized.

@jnovack

jnovack Jun 27, 2017

This is the line CircleCI is choking on.

There is a no "as" provision in the version of docker that CircleCI is using.

https://docs.docker.com/engine/reference/builder/#from

@jnovack

This comment has been minimized.

Copy link

jnovack commented Jun 27, 2017

This WORKS with https://github.com/jnovack/docker-consul-registrator-self-signed when I replace gliderlabs/registrator:master with registrator:dev (after make dev)

registrator_1  | 2017/06/27 15:07:03 Using consul-tls adapter: consul-tls://consul:8080
registrator_1  | 2017/06/27 15:07:03 Connecting to backend (0/0)
registrator_1  | 2017/06/27 15:07:13 consul: current leader  172.19.0.3:8300
registrator_1  | 2017/06/27 15:07:13 Listening for Docker events ...
registrator_1  | 2017/06/27 15:07:13 Syncing services on 2 containers
registrator_1  | 2017/06/27 15:07:13 ignored: 41426f8f736d no published ports
registrator_1  | 2017/06/27 15:07:13 ignored: 05aadc7f1199 port 8301 not published on host
registrator_1  | 2017/06/27 15:07:13 ignored: 05aadc7f1199 port 8300 not published on host
registrator_1  | 2017/06/27 15:07:13 ignored: 05aadc7f1199 port 8302 not published on host
registrator_1  | 2017/06/27 15:07:13 ignored: 05aadc7f1199 port 8600 not published on host
registrator_1  | 2017/06/27 15:07:13 ignored: 05aadc7f1199 port 8600 not published on host
registrator_1  | 2017/06/27 15:07:13 ignored: 05aadc7f1199 port 8302 not published on host
registrator_1  | 2017/06/27 15:07:13 ignored: 05aadc7f1199 port 8500 not published on host
registrator_1  | 2017/06/27 15:07:13 ignored: 05aadc7f1199 port 8301 not published on host
consul_1       |     2017/06/27 15:07:13 [INFO] agent: Synced service '41426f8f736d:dockerconsulregistratorselfsigned_consul_1:8080'
registrator_1  | 2017/06/27 15:07:13 added: 05aadc7f1199 41426f8f736d:dockerconsulregistratorselfsigned_consul_1:8080

(Note: Regression testing not performed.. just tested consul-tls)

@jnovack

This comment has been minimized.

Copy link

jnovack commented Jul 16, 2017

After some more testing with the make build version, I want to take issue with InsecureSkipVerify: false,. I'd like to politely request it be made an environment variable. There are times when I don't feel we must use DNS and verify the certificate, although, yes, good practice.

I noticed that during a test when attempting to connect with IP address, it was complaining about IP SANs, and I REALLY didn't want to have to regenerate my certificate with an IP subject_alternative_name. (Ultimately, I did.) Additionally, if the cert was from a public CA, none of them (for good reason) will sign a request with an RFC-1918 address-ranged IP SAN.

8e6db13#diff-e0cd2c42b7de4225cc3b6c0268572906R43

@dj-wasabi

This comment has been minimized.

Copy link

dj-wasabi commented Aug 1, 2017

Is there an ETA on this? It fixes the issues I have right now..

@maschinetheist

This comment has been minimized.

Copy link

maschinetheist commented Aug 25, 2017

Any hope of getting this merged?

@josegonzalez

This comment has been minimized.

Copy link
Member

josegonzalez commented Aug 25, 2017

Whenever circleci supports AS.

@mattatcha

This comment has been minimized.

Copy link
Member

mattatcha commented Aug 25, 2017

@jnovack

This comment has been minimized.

Copy link

jnovack commented Dec 8, 2017

CircleCI supports many newer versions of Docker. Just tell it to use v17.05 or greater for AS support!

https://circleci.com/docs/2.0/building-docker-images/#docker-version

image

@jnovack

This comment has been minimized.

Copy link

jnovack commented Apr 30, 2018

Has this project staled out?

@josegonzalez

This comment has been minimized.

Copy link
Member

josegonzalez commented Apr 30, 2018

I just triggered a rebuild. Lets see how its handled.

@jnovack

This comment has been minimized.

Copy link

jnovack commented Apr 30, 2018

Alright.. looking good on CircleCI. w00t. I appreciate the quick response!

I'm new to Go, so please forgive my newbish question, are all the vendor/ libraries supposed to be in the commit? Says "Files Changed 1,110". That does not seem right to me. :/

CAFile: os.Getenv("CONSUL_CACERT"),
CertFile: os.Getenv("CONSUL_CLIENT_CERT"),
KeyFile: os.Getenv("CONSUL_CLIENT_KEY"),
InsecureSkipVerify: false,

This comment has been minimized.

@jnovack

jnovack Apr 30, 2018

I humbly and politely request InsecureSkipVerify be an environment variable (default: false).

Perhaps os.Getenv("CONSUL_INSECURE")? Keeps the same ominous "visual warning" as the words "InsecureSkipVerify" when setting to true.

@GJRTimmer

This comment has been minimized.

Copy link

GJRTimmer commented Aug 2, 2018

Anyone any idea when this will be merged. TLS will not work without this.

@josegonzalez

This comment has been minimized.

Copy link
Member

josegonzalez commented Aug 2, 2018

@GJRTimmer whenever merge conflicts are resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment