Remove example, make Nginx config extensible, simplify configurations #37

jasonpincin · 2016-10-21T00:56:42Z

The overall goal of this PR is to both simplify the blueprint, and make it more extensible. Here's the high level changes:

Removed all traces of example. Examples to be done outside this repo, eventually replaced with tests.
Split Nginx config into multiple files: nginx.conf, ssl.conf, health.conf, and conf.d/site.conf
Multiple script changes to increase clarity of containerpilot.json
- One ACME task (acme checkin) instead of three
- acme watch handles details of launching consul-template daemon correctly
- Broke reload.sh out into generate-config and reload

With these changes, containerpilot.json is more comprehensible, and more importantly, users can extend the Nginx configuration in pieces by adding/replacing files in /etc/nginx/conf.d. Most users will simply need to replace /etc/nginx/conf.d/site.conf with a bare-minimum set of directives tailored for the application to get things up and running, without having to worry about SSL details or health check details, etc.

Todo:

Update README - describe available env vars CONSUL, CONSUL_AGENT, ACME_DOMAIN, ACME_ENV, as well as best practices on extending Nginx config and containerpilot.json

…template properly. Modified checkin to be usable as the only required task.

…ride.

…ty; not required. Use generate-config instead of reload.sh preStart. Get rid of example backend. Reduced ACME related tasks to one.

…ctory.

tgross · 2016-11-07T14:26:07Z

Removed all traces of example. Examples to be done outside this repo, eventually replaced with tests.

One of our stated objectives all along has been for each application blueprint to be standalone so that they can act as demonstrations of the principles we're trying to illustrate. Removing the example means that you have to use this with some other blueprint, which increases the barrier to entry for a demonstration. (Plus it'll frustrate Joyent sales folks who want to have quick-and-ready demos that don't take 10-15 minutes to stand up.)

Edit: nevermind, previously discussed.

tgross · 2016-11-07T14:29:48Z

etc/containerpilot.json

-      "command": ["/usr/local/bin/consul-template",
-                  "-config", "/etc/acme/watch.hcl",
-                  "-consul", "{{ if .CONSUL_AGENT }}localhost{{ else }}{{ if .CONSUL }}{{ .CONSUL }}{{ else }}consul{{ end }}{{ end }}:8500"],
+      "command": ["acme", "watch"],


Why not just use /usr/local/bin/consul-template -config /etc/acme/watch.hcl -consul $CONSUL_HOST:8500 directly here?

We certainly can. I just did it this way so that all the moving parts related to certificate acquisition was contained within the acme script, so that when you're looking at that you see it all, versus some pieces here, some there. Things like the CONSUL_AGENT are tucked away cleanly in the acme script, etc. Cleans up containerpilot.json a bit. Admittedly, this is purely stylistic.

Ok, that's totally reasonable. Let's keep it as you've got it here.

tgross · 2016-11-07T14:31:30Z

etc/containerpilot.json

      "poll": 10,
      "ttl": 25,
      "interfaces": ["eth1", "eth0"]
    }{{ if .ACME_DOMAIN }},
    {
+      "name": "nginx-ssl",
+      "port": 443,
+      "health": "acme init && curl -kfsSo /dev/null https://localhost/nginx-health",


If we put -k here then we're intentionally bypassing SSL cert verification, which I feel like it'd be really useful to include. Is it feasible to switch out localhost for {{ .ACME_DOMAIN }} here so that we can ensure SSL is working in the health check?

Yeah, I agree. The real issue here is ACME_ENV. When set to staging, -k is required because we're dealing with test, invalid certs. When we're dealing with production as a value of that environment, then we could swap the host, although that feels a little risky as there's no guarantee that DNS is resolving to our container, etc... not sure off the top of my head whether just passing Host: header would be enough. Handling all these cases may be complex enough to warrant moving the test into a script instead of cramming all the branches into containerpilot.json, thoughts?

Ah, it's the staging bit that I was forgetting here. Does LE have a recommended way to test the staging cert? Like maybe if there's a signing root that we can include in our container we can test locally safely?

Handling all these cases may be complex enough to warrant moving the test into a script instead of cramming all the branches into containerpilot.json, thoughts?

That's probably not a bad idea if we need to implement a lot of logic. We've found that tends to be the case for a lot of the blueprints as we start probing all their edge cases!

Does LE have a recommended way to test the staging cert?

Good question; I'll look into that.

Is it feasible to switch out localhost for {{ .ACME_DOMAIN }} here so that we can ensure SSL is working in the health check?

Bit of a chicken and egg here, as the health check must pass prior to the service registering and accepting requests for the domain, even if we assume DNS is correct. Perhaps we append a line to /etc/hosts:

127.0.0.1 $ACME_DOMAIN

... prior to test...

tgross · 2016-11-07T14:34:32Z

etc/nginx/ssl.conf

+ssl_certificate_key /var/www/ssl/privkey.pem;
+ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
+ssl_prefer_server_ciphers on;
+ssl_ciphers 'ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-DSS-AES128-GCM-SHA256:kEDH+AESGCM:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA384:ECDHE-RSA-AES256-SHA:ECDHE-ECDSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-DSS-AES128-SHA256:DHE-RSA-AES256-SHA256:DHE-DSS-AES256-SHA:DHE-RSA-AES256-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:AES:CAMELLIA:DES-CBC3-SHA:!aNULL:!eNULL:!EXPORT:!DES:!RC4:!MD5:!PSK:!aECDH:!EDH-DSS-DES-CBC3-SHA:!EDH-RSA-DES-CBC3-SHA:!KRB5-DES-CBC3-SHA';


I'd like to recommend that we put a link to where this configuration came from in a comment, so that a) we can occasionally check on it to see if it needs updating, and b) we lend a bit of community-approval legitimacy to the SSL config. Maybe another comment would be that it generates a "A" rating, etc.

tgross · 2016-11-07T14:36:18Z

Dockerfile

-# A minimal Nginx container including ContainerPilot and a simple virtualhost config
-FROM nginx:latest
+# A minimal Nginx container including ContainerPilot
+FROM nginx:1.11


As long as we're doing an update we might consider tossing in ContainerPilot 2.4.4 which reduces some log spam.

tgross · 2016-11-07T14:38:19Z

bin/generate-config

+fi
+
+# Generate a conf.d config file for every corresponding cond.d Consul template
+for f in $(ls -1 /etc/nginx/templates/conf.d/)


Not sure if you missed it in the consul-template docs but we can render multiple files from a single invocation, which should reduce the number of API calls to Consul. Their example is as follows:

$ consul-template \ -consul my.consul.internal:6124 \ -template "/tmp/nginx.ctmpl:/var/nginx/nginx.conf:service nginx restart" \ -template "/tmp/redis.ctmpl:/var/redis/redis.conf:service redis restart" \ -template "/tmp/haproxy.ctmpl:/var/haproxy/haproxy.conf"

I updated the 2nd block to use this approach, as there's a static list of files there. For the first block, the files would primarily be user supplied, so we need to loop over the list.

…_ENV=staging. Break container pilot health check logic out into health-check script.

jasonpincin · 2016-11-08T01:04:32Z

All feedback now incorporated:

Added comment to ssl.conf to link to mozilla for configuration suggestions
Upgraded to ContainerPilot 2.4.4
Reduced number of calls to consul-template in generate-config
Broke health check logic out of containerpilot.json, placed in health-check script
Dropped -k from curl tests, using --cacert option of curl to use LE's staging CA cert when ACME_ENV=staging

tgross · 2017-02-03T16:58:31Z

Carrying all these commits in #41. Closing this PR.

jasonpincin added 9 commits October 19, 2016 15:16

Add nginx-ssl interface (non-public)

c4a4ed6

Removing example

602e0b5

Exit without error when not leader. Added watch, which starts consul-…

24ed42d

…template properly. Modified checkin to be usable as the only required task.

Seperated generate-config and reload for clarity in containerpilot.json

80300a2

Broke nginx config out into multiple files, suitable for partial over…

a70f486

…ride.

Do not do debug logging by default. Axed absolute paths for readabili…

f7a8a63

…ty; not required. Use generate-config instead of reload.sh preStart. Get rid of example backend. Reduced ACME related tasks to one.

Updated for restructured Nginx config and nginx config templates dire…

3789a55

…ctory.

Removed .sh extensions for consistency

4eb70c0

Pin Nginx container version

06491cb

tgross reviewed Nov 7, 2016

View reviewed changes

jasonpincin added 4 commits November 7, 2016 14:17

Update to ContainerPilot 2.4.4

15fc76d

Link to mozilla SSL best practices

ac6aeea

reduce calls to consul-template

faf5c39

No -k option on curl tests. Use LE Staging CA Cert for curl when ACME…

855c6bc

…_ENV=staging. Break container pilot health check logic out into health-check script.

tgross mentioned this pull request Dec 12, 2016

Add support for multiple BACKEND by having them space-separated #40

Closed

tgross mentioned this pull request Feb 3, 2017

[WIP] Functional testing #41

Closed

tgross closed this Feb 3, 2017

tgross mentioned this pull request Feb 3, 2017

Functional testing #42

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove example, make Nginx config extensible, simplify configurations #37

Remove example, make Nginx config extensible, simplify configurations #37

jasonpincin commented Oct 21, 2016

tgross commented Nov 7, 2016 •

edited

Loading

tgross Nov 7, 2016

jasonpincin Nov 7, 2016

tgross Nov 7, 2016

tgross Nov 7, 2016 •

edited

Loading

jasonpincin Nov 7, 2016

tgross Nov 7, 2016

jasonpincin Nov 7, 2016

jasonpincin Nov 7, 2016

tgross Nov 7, 2016

tgross Nov 7, 2016

tgross Nov 7, 2016

jasonpincin Nov 8, 2016

jasonpincin commented Nov 8, 2016

tgross commented Feb 3, 2017

Remove example, make Nginx config extensible, simplify configurations #37

Remove example, make Nginx config extensible, simplify configurations #37

Conversation

jasonpincin commented Oct 21, 2016

tgross commented Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgross Nov 7, 2016 • edited Loading

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

jasonpincin commented Nov 8, 2016

tgross commented Feb 3, 2017

tgross commented Nov 7, 2016 •

edited

Loading

tgross Nov 7, 2016 •

edited

Loading