Support ACME feature of autopilotpattern/nginx #34

jasonpincin · 2016-10-11T16:27:04Z

This PR updates the local Nginx configs so that the ACME features of latest autopilotpattern/nginx are supported.

misterbisson · 2016-10-11T19:18:20Z

nginx/etc/nginx/nginx-ssl.conf.ctmpl

@@ -0,0 +1,95 @@
+user  nginx;
+worker_processes  1;


It seems confusing to me to have separate config templates. It certainly introduces a lot of duplication.

Instead, can't the templating system switch config details based on conditionals in the template, rather than switching the whole template?

I personally felt it was easier to reason about and follow by making it two separate config files, simply because of the differences. By combining them, there's a lot of conditionals that increase cognitive overhead when trying to reason about what the file will look like after it's parsed. This is personal preference and I'll gladly change it (here and in the Nginx blueprint) if others feel strongly about this.

In order to determine if SSL is ready, I need to test whether two files exist on disk. I do not believe that consul-template provides a way of doing this natively, so I would do the test in the Nginx reload.sh script and pass an environment variable to the consul-template call.

I would do the test in the Nginx reload.sh script and pass an environment variable to the consul-template call.

Makes sense.

It looks like there'd be two conditional blocks (and they could be collapsed into one), but that's from the person reading it, not implementing it ;-).

I'll code up that version for comparison.

This one can be deleted/merged now that autopilotpattern/nginx#32 is upstream.

misterbisson · 2016-10-11T19:19:49Z

nginx/etc/nginx/nginx.conf.ctmpl

    log_format  main  '$remote_addr - $remote_user [$time_local] "$request" '
                      '$status $body_bytes_sent "$http_referer" '
                      '"$http_user_agent" "$http_x_forwarded_for"';

    access_log  /var/log/nginx/access.log  main;

    sendfile        on;
+    #tcp_nopush     on;


Do we want to add config items that are commented out?

We do not. I simply did not prune them. I copied these files form the Nginx repo, where that line has existed since last November, and modified them to suit.

misterbisson · 2016-10-11T19:19:50Z

nginx/etc/nginx/nginx.conf.ctmpl


        location /nginx-health {
            stub_status;
            allow 127.0.0.1;
            deny all;
+            access_log /var/log/nginx/access.log  main if=$isError;


Does this cause the image to generate log files on disk?

This corresponding line has existed for sometime; I've not checked to see if this results in an on-disk file. The line I've added, in question, will result in LESS logging whatever the case may be.

…e ACME tasks, etc.

jasonpincin · 2016-10-12T18:26:58Z

@misterbisson Here's the PR that re-combines the Nginx config templates in autopilotpattern/nginx: autopilotpattern/nginx#32

misterbisson · 2016-10-25T16:56:20Z

This will also need changes in setup.sh to add the relevant config entries (commented out):

        echo '# Nginx LetsEncrypt (ACME) config' >> _env
        echo '# be sure ACME_DOMAIN and WORDPRESS_URL are the same, if using automated SSL via LetsEncrypt' >> _env
        echo '#ACME_ENV=production' >> _env
        echo '#ACME_DOMAIN=you.custom.domain' >> _env

misterbisson · 2016-10-25T16:56:53Z

nginx/Dockerfile

@@ -1,5 +1,5 @@
 # a minimal Nginx container including containerbuddy and a simple virtulhost config
-FROM autopilotpattern/nginx:latest
+FROM autopilotpattern/nginx:1-r6.0.0


1-r6.1.0 now?

…n blocks added code to wp-config.php to pick up that header and set $_SERVER['HTTPS']='on' docker-compose.yml still contains refrences to the images I built from these files and pushed to my dockerhub.

…pincin-issue-33 tags

misterbisson · 2016-10-28T04:34:53Z

nginx/etc/nginx/nginx.conf.ctmpl

        {{ if service "wordpress" }}
        rewrite ^/wp-admin/?(.*) /wordpress/wp-admin/$1;
-        
+
        location ^~ / {
            proxy_pass http://wordpress;
            proxy_set_header Host $http_host;


I've been stuck on how I made this work before without adding HTTP_X_FORWARDED_PROTO and a conditional in the wp_config.php. It's taken me a while, but the answer is this:

proxy_set_header Host $host:$server_port;

That's actually in the sample Nginx config in #19, but I wasn't able to recognize it until looking yet again.

While at it, adding the following headers might be worthwhile as well:

proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;

misterbisson · 2016-10-28T04:35:40Z

var/www/html/wp-config.php.ctmpl

@@ -19,6 +19,12 @@ define( 'DB_PASSWORD', '{{env "MYSQL_PASSWORD"}}' );
 define( 'DB_HOST',     '{{if service "mysql-primary"}}{{with index (service "mysql-primary") 0}}{{.Address}}:{{.Port}}{{end}}{{end}}' );

 /**
+ * Handle SSL reverse proxy


See https://github.com/autopilotpattern/wordpress/pull/34/files#r85470880 for how to eliminate this conditional

Fix PR'd in jasonpincin#2 and jasonpincin#3

See autopilotpattern#34 (comment)

misterbisson · 2016-10-28T04:48:50Z

docker-compose.yml

    - 9090
  env_file: _env
  environment:
    - CONSUL_AGENT=1
+    - ACME_DOMAIN=joyent.pincin.me


Remove test content

misterbisson · 2016-10-28T04:49:01Z

docker-compose.yml

@@ -85,15 +85,17 @@ memcached:

 # Nginx as a load-balancing tier and reverse proxy
 nginx:
-  image: autopilotpattern/wordpress-nginx:latest
+  image: autopilotpattern/wordpress-nginx:branch-jasonpincin-issue-33


Remove test tag

misterbisson · 2016-10-28T04:49:13Z

docker-compose.yml

@@ -1,6 +1,6 @@
 # An demo version of WordPress for easy scaling
 wordpress:
-  image: autopilotpattern/wordpress:latest
+  image: autopilotpattern/wordpress:branch-jasonpincin-issue-33


Remove test tag

misterbisson · 2016-10-28T04:58:30Z

Looking good. I pedantically called out some changes to be reverted before merging, but you knew about those.

The thing I'm really ashamed of is how long it took me to figure out how I'd made this work previously. I didn't use the conditional (which we'd want to avoid just because it's an extra conditional on every request), but it took me forever to spot the $server_port in proxy_set_header Host $host:$server_port; that I was using. That config file is in #19 (which has a few things changed/removed for anonymity).

I PR'd those changes in jasonpincin#2 and jasonpincin#3, though if you're guessing I didn't build and test images with those changes....

misterbisson

Nice work!

🏡 🚶

misterbisson · 2016-10-28T22:41:10Z

From looking at https://hub.docker.com/r/autopilotpattern/wordpress/tags/ and https://hub.docker.com/r/autopilotpattern/wordpress-nginx/tags/, it looks like this should be tagged 4.5.3-r3 when merged.

Support ACME feature of autopilotpattern/nginx, closes autopilotpatte…

0415d6b

…rn#33

misterbisson reviewed Oct 11, 2016

View reviewed changes

jasonpincin changed the title ~~Support ACME feature of autopilotpattern/nginx~~ [WIP] Support ACME feature of autopilotpattern/nginx Oct 11, 2016

Pin Nginx container version. Update containerpilot.json to include th…

a4ef0b7

…e ACME tasks, etc.

open 443, set image tag for testing

e63df4e

misterbisson reviewed Oct 25, 2016

View reviewed changes

Jason DeWitt and others added 5 commits October 27, 2016 15:20

added setting a X_FORWARDED_PROTO to the WordPress proxy pass locatio…

5374b45

…n blocks added code to wp-config.php to pick up that header and set $_SERVER['HTTPS']='on' docker-compose.yml still contains refrences to the images I built from these files and pushed to my dockerhub.

Re-consolidated ssl/non-ssl nginx.conf.ctmpl

e00ca04

Updated nginx image

0e84d52

Test ACME_DOMAIN, pointed wordpress and nginx images at :branch-jason…

055dd0e

…pincin-issue-33 tags

setup.sh now adds ACME variables to _env

1fa3a9f

jasonpincin mentioned this pull request Oct 28, 2016

X_FORWARDED_PROTO & WP ssl fixing #35

Closed

misterbisson reviewed Oct 28, 2016

View reviewed changes

misterbisson mentioned this pull request Oct 28, 2016

Add forwarded port, remove conditional jasonpincin/wordpress#2

Closed

misterbisson added a commit to misterbisson/wordpress that referenced this pull request Oct 28, 2016

Add forwarded port, remove conditional

3b30295

See autopilotpattern#34 (comment)

misterbisson mentioned this pull request Oct 28, 2016

Add forwarded port, remove conditional jasonpincin/wordpress#3

Closed

misterbisson reviewed Oct 28, 2016

View reviewed changes

jasonpincin added 3 commits October 28, 2016 16:44

Added braces

0c0a9d4

Added headers: X-Real-IP, X-Forwarded-For

86a299d

Removed test settings

a419b23

misterbisson approved these changes Oct 28, 2016

View reviewed changes

misterbisson changed the title ~~[WIP] Support ACME feature of autopilotpattern/nginx~~ Support ACME feature of autopilotpattern/nginx Oct 28, 2016

jasonpincin merged commit 5c2279b into autopilotpattern:master Oct 31, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ACME feature of autopilotpattern/nginx #34

Support ACME feature of autopilotpattern/nginx #34

jasonpincin commented Oct 11, 2016

misterbisson Oct 11, 2016

jasonpincin Oct 11, 2016

misterbisson Oct 11, 2016

jasonpincin Oct 11, 2016

misterbisson Oct 25, 2016

misterbisson Oct 11, 2016

jasonpincin Oct 11, 2016

misterbisson Oct 11, 2016

jasonpincin Oct 11, 2016

jasonpincin commented Oct 12, 2016

misterbisson commented Oct 25, 2016 •

edited

Loading

misterbisson Oct 25, 2016

misterbisson Oct 28, 2016 •

edited

Loading

misterbisson Oct 28, 2016 •

edited

Loading

misterbisson Oct 28, 2016

misterbisson Oct 28, 2016

misterbisson Oct 28, 2016

misterbisson Oct 28, 2016 •

edited

Loading

misterbisson Oct 28, 2016

misterbisson commented Oct 28, 2016

misterbisson left a comment

misterbisson commented Oct 28, 2016 •

edited

Loading

Support ACME feature of autopilotpattern/nginx #34

Support ACME feature of autopilotpattern/nginx #34

Conversation

jasonpincin commented Oct 11, 2016

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 Oct 12, 2016

misterbisson commented Oct 25, 2016 • edited Loading

Choose a reason for hiding this comment

misterbisson Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

misterbisson Oct 28, 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

misterbisson Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

misterbisson commented Oct 28, 2016

misterbisson left a comment

Choose a reason for hiding this comment

misterbisson commented Oct 28, 2016 • edited Loading

misterbisson commented Oct 25, 2016 •

edited

Loading

misterbisson Oct 28, 2016 •

edited

Loading

misterbisson Oct 28, 2016 •

edited

Loading

misterbisson Oct 28, 2016 •

edited

Loading

misterbisson commented Oct 28, 2016 •

edited

Loading