Skip to content

Consul key/value collision #47

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

sberryman
Copy link

Not sure if this is a common use-case but is something I ran into recently. I have several front end load balancers where I want to perform SSL termination. Since I don’t want to run several Consul clusters I ran into an issue where they all use the same keys. I’m also not sure if I have caught all the places where nginx is hard coded but this DOES work in production for me.

@@ -127,7 +128,7 @@ case "$1" in
acquireLeader
;;
watch)
/usr/local/bin/consul-template -config /etc/acme/watch.hcl -consul $CONSUL_HOST:8500
/usr/local/bin/consul-template -config /etc/acme/watch.hcl -consul-addr $CONSUL_HOST:8500
Copy link
Author

Choose a reason for hiding this comment

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

This was changed as a warning was output to stdout by consul-template

@@ -1 +1,2 @@
{{if key "nginx/acme/cert"}}{{key "nginx/acme/cert"}}{{end}}
{{ $service_name := env "SERVICE_NAME" }}
Copy link
Author

Choose a reason for hiding this comment

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

WARNING!!! No default SERVICE_NAME

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 have a default service name nginx.

Copy link
Author

Choose a reason for hiding this comment

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

Right, honestly I'm not sure how to do it and I didn't have time yesterday to look. Just needed it working to show off a few things

Copy link
Contributor

Choose a reason for hiding this comment

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

This appears fixed with {{ $service_name := or $service_name "nginx" }} below.

@sberryman
Copy link
Author

@tgross Warning, I've never used golang and not sure how to add a default value for templating. Right now there is no default SERVICE_NAME for the environment variable on acme templates. However, it defaults to nginx in the two bash scripts that were changed.

See the example below:
https://github.com/sberryman/autopilotpattern-nginx/blob/91b5d21f689c1154bfc4bbe2a7021a6dd3c5cbaa/etc/acme/templates/privkey.ctmpl#L1

I haven't had time to look up adding defaults.

@tgross
Copy link
Contributor

tgross commented Apr 12, 2017

I've never used golang and not sure how to add a default value for templating.

Unfortunately default has to be added by every implementation of the rendered. ContainerPilot has it but consul-template does not. You want something along the lines of: {{ $myvar := or $myothervar "value" }}

@sberryman
Copy link
Author

sberryman commented Apr 12, 2017

@tgross added a default service_name to the templates and tested on joyent. I've noticed curl showing an error when SSL is enabled though.

No matter what I end up using for the hostname I always get an error resolving the host.
2017/04/12 16:36:29 curl: (6) Could not resolve host: XYZ" XYZ being the hostname I picked. SSL works but that error shows up in the logs every 10 seconds.

Update:
I believe this is the command causing the error

/usr/bin/curl --insecure --fail --silent --show-error --output /dev/null --header \"HOST: {{ .ACME_DOMAIN }}\" https://localhost/nginx-health

Update 2:
The {{ .ACME_DOMAIN }} resolves to 127.0.0.1 inside the container.

@tgross
Copy link
Contributor

tgross commented Apr 12, 2017

I believe this is the command causing the error

/usr/bin/curl --insecure --fail --silent \
    --show-error --output /dev/null \
    --header \"HOST: {{ .ACME_DOMAIN }}\" \
    https://localhost/nginx-health

If that were the case I don't think we'd be seeing an attempt to resolve that hostname, right? Which hostname is the error Could not resolve host: XYZ" for: ACME_DOMAIN or CONSUL?

@sberryman
Copy link
Author

sberryman commented Apr 12, 2017

@tgross - Here is what I'm doing:

ACME_DOMAIN=cms.dev.famishednow.net

  • Deployed my branch to joyent along with a backing service and consul
  • docker exec -it front_cms_nginx_1 /bin/bash
  • Output below
# export | grep ACME_DOMAIN
declare -x ACME_DOMAIN="cms.dev.famishednow.net"

# ping $ACME_DOMAIN
PING cms.dev.famishednow.net (127.0.0.1): 56 data bytes
64 bytes from 127.0.0.1: icmp_seq=0 ttl=255 time=0.054 ms
64 bytes from 127.0.0.1: icmp_seq=1 ttl=255 time=0.052 ms
64 bytes from 127.0.0.1: icmp_seq=2 ttl=255 time=0.060 ms

# /usr/bin/curl --insecure --fail --silent --show-error --output /dev/null --header \"HOST: $ACME_DOMAIN\" https://localhost/nginx-health
curl: (6) Could not resolve host: cms.dev.famishednow.net"
Active connections: 1 
server accepts handled requests
 1989 1989 2065 
Reading: 0 Writing: 1 Waiting: 0

Obviously the request is working and consul shows the test as passing. However I see that damn curl error in the logs every 10 seconds.

Update: Why are you even using the HOST header? The request passes without it, I'm guessing you are assuming the NGINX config is using server_name in the config?

@tgross
Copy link
Contributor

tgross commented Apr 13, 2017

PING cms.dev.famishednow.net (127.0.0.1)

This is the bit I'm confused by. Why does the name you're using resolve to localhost? That's not going to work with Let's Encrypt anyways, right?

Update: Why are you even using the HOST header? The request passes without it, I'm guessing you are assuming the NGINX config is using server_name in the config?

Yes, which is a pretty safe assumption under TLS. I'm not sure why we're including --insecure on this request though.

@sberryman
Copy link
Author

I was confused by it until I realized you are injecting $ACME_DOMAIN into /etc/hosts.
https://github.com/autopilotpattern/nginx/blob/master/bin/health-check#L8-L14

LetsEncrypt runs just fine, in fact I was blow away how well and quickly it worked. 100x easier than setting up certificates the old fashion manual way.

Right now the only issue I see is the error showing up in the logs. It doesn't cause any issues related to functionality. Even the consul health check shows as passing so it isn't that big of a deal.

@@ -1 +1,3 @@
{{if key "nginx/acme/cert"}}{{key "nginx/acme/cert"}}{{end}}
{{ $service_name := env "SERVICE_NAME" }}
{{ $service_name := or $service_name "nginx" }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not saying the following with certainty, but there's a chance we'll also (or alternatively) want to set SERVICE_NAME to nginx as a default value in the Dockerfile. That would result in reliably getting a env var, even if the user doesn't supply one.

If I remember correctly, that syntax would look like:

ENV SERVICE_NAME =${SERVICE_NAME:-nginx}

bin/acme Outdated
CONSUL_HOST_DEFAULT=${CONSUL:-consul}
if [ "${CONSUL_AGENT}" != "" ]; then
CONSUL_HOST_DEFAULT="localhost"
fi
CONSUL_HOST=${CONSUL_HOST:-$CONSUL_HOST_DEFAULT}
CONSUL_ROOT="http://${CONSUL_HOST}:8500/v1"
CONSUL_KEY_ROOT="${CONSUL_ROOT}/kv/nginx"
CONSUL_KEY_ROOT="${CONSUL_ROOT}/kv/${SERVICE_NAME}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another place where we'd need a default value.

@misterbisson
Copy link
Contributor

@sberryman I think this looks good. There's one place where SERVICE_NAME is being used without a default, but I've also got a question about a different or additional approach to setting that default in another comment.

@sberryman
Copy link
Author

@misterbisson I have moved on past nginx and started using traefik for my project instead. I am more than happy to up the PR though. Still using the autopilot pattern for pretty much everything though which has been working out great!

@misterbisson
Copy link
Contributor

@sberryman I think this was a useful change, so if you're up for it I'd love to update this PR. Also, I'd love to hear more about how you're using Traefik.

@sberryman
Copy link
Author

@misterbisson lets see what you think of those quick changes. We can move the Traefik conversation to another spot if you would like. I can go over what I'm using containerpilot for and the rest of my stack. All of which is hosted on Triton of course.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants