Declarative nginx module with ACME support #15862

globin · 2016-05-31T13:51:31Z

Motivation for this change

Make the nginx config declaritive and therefore to easily generate locations/vhosts e.g. for many domains on a reverse proxy.

Things done

Fits CONTRIBUTING.md.
Tested excessively on multiple production machines since around february.

mention-bot · 2016-05-31T13:51:34Z

By analyzing the blame information on this pull request, we identified @shlevy, @domenkozar and @roconnor to be potential reviewers

trishume · 2016-05-31T15:13:07Z

I'm not a maintainer or anything, but I reviewed the whole PR and it looks great 👍

shlevy · 2016-05-31T15:27:25Z

nixos/modules/services/web-servers/nginx/default.nix

    daemon off;

    ${cfg.config}

-    ${optionalString (cfg.httpConfig != "") ''


This changes the semantics of cfg.httpConfig significantly. Do we need that?

shlevy · 2016-05-31T15:27:57Z

Any chance you can separate out the declarative bits from the ACME bits? It's a lot to look at currently.

groxxda · 2016-05-31T15:38:57Z

nixos/modules/services/web-servers/nginx/default.nix

+      gzip_disable "msie6";
+      gzip_proxied any;
+      gzip_comp_level 9;
+      gzip_buffers 16 8k;


On platforms with 8k memory pages this equals the default and on platforms with 4k memory pages this is most probably inferior to the default (32 4k on such platforms). I recommend removing this.

globin · 2016-05-31T15:39:06Z

It's actually separated quite nicely in the commits, why I left them separate for now?

groxxda · 2016-05-31T15:45:34Z

nixos/modules/services/web-servers/nginx/default.nix

+      proxy_connect_timeout   90;
+      proxy_send_timeout      90;
+      proxy_read_timeout      90;
+      proxy_buffers           32 4k;


Are 32 buffers needed? Settings this forces 4k buffers on systems with 8k memory pages. Below: proxy_buffer_size defaults to two proxy_buffers.

shlevy · 2016-05-31T16:12:16Z

Eh, so I'm a bit skeptical that we need all these defaults hard-coded in. This is less flexible than before and could be easily replicated by saving the common snippet in some "recommended config" string.

This is not policy everywhere, but IMO unless a setting needs to affect multiple components (e.g. set something in the config file and change the port that systemd socket activates on) we should just let users set config file contents themselves.

groxxda · 2016-05-31T16:20:40Z

nixos/modules/services/web-servers/nginx/vhost-options.nix

+      type = types.str;
+      default = "0.0.0.0";
+      description = ''
+        Host which to proxy requests to if acme challenge is not found. Useful


Nitpick: I'm not a native speaker but this reads wrong.
Maybe: "Host to which the request is forwarded if ACME challenge is not found." ?

groxxda · 2016-05-31T16:36:51Z

I'm happy to finally see these updates to the nginx module.
But I agree with @shlevy that this is likely to be a breaking change for existing users. Instead of replacing the httpConfig setting and giving it new semantics, we could keep it and only use the new defaults if it's empty. We'd probably need something like httpConfigAppend.
I think I've seen such an approach in other modules.

Also: Does this work when the user wants no SSL and does not set sslCertificate{,Key} ? Not sure about whether types.path allows null.

And last thing for now: I dislike the mixup of enableSsl and forceSsl.
Either add an assertion that forceSsl -> enableSsl or remove most checks if ssl with if enableSsl where apropriate.

domenkozar · 2016-06-01T10:49:30Z

I'm very 👎 on changing nginx defaults this way. I've already reverted a similar change #9693 and you can see how already these two PRs really differ a lot.

There are a lot of assumptions in here, for example that you'll be serving everything through HTTPS and redirect HTTP to it. Or that you'll always gzip all plaintext. I agree this should be the default, but it may also break lots of stuff. For example some applications (even Hydra has a bug report) don't handle reverse proxy url rewriting with HTTPS.

I think nginx composes nicely at the moment, so including this somehow to be an external module would be optimal.

I'm +1 adding nginx modules under optional switch that follow some upstream very well documented convention (for example hardened TLS based on mozilla recommendations), but I think if we all start pulling in our defaults, we'll be all very angry in a year.

Note: this is my very opinionated view, I'm open for a discussion :)

fpletz · 2016-06-01T11:38:30Z

I agree that we should remove those defaults. Will rebase the whole PR and/or remove commits soon.

However, what about the other improvements like options for virtual hosts and the acme support?

domenkozar · 2016-06-01T11:43:31Z

@fpletz I think those should be fine since they're optional and all "typed".

bobvanderlinden · 2016-06-01T19:22:31Z

nixos/modules/services/web-servers/nginx/default.nix

+      proxy_http_version      1.0;
+
+      server_tokens ${if cfg.serverTokens then "on" else "off"};
+      ${vhosts}
      ${cfg.httpConfig}


This should be appendHttpConfig to be in line with appendConfig. The original httpConfig should (I think) override all of this http { ... } configuration to be backwards compatible with its old functionality and have an option to override all of this.

bobvanderlinden · 2016-06-01T19:29:19Z

Wow, this is something I've wanted for Nginx for a while now. Thanks for sharing! This opens up a whole range of possibilities.

Do you have some example of system configuration that illustrates how it'll look? Would be nice for documentation and possibly tests.

trishume · 2016-06-01T19:29:41Z

My two cents as someone who runs a small NixOS VPS and isn't an expert at server admin: Some kind of way to enable good defaults like are in this PR would be very welcome since I'm not sure what to enable otherwise. Also wrappers of config file languages like the vhosts generator are something I really like since it limits the amount of config languages and documentation sites I have to learn to set up a NixOS VPS to just one, unlike the 10+ I need to set up a VPS with any other distro. Inline config strings are great for experts, not as nice for newbies, which is why having both is probably best.

I agree that it's probably best to maintain the existing semantics of the nginx module options though to avoid breaking things unnecessarily. I'd also be totally fine with this being a separate module, as long as it is in nikpkgs and has a searchable name including the substring "nginx" it is just as easy to find and use.

bobvanderlinden · 2016-06-01T19:43:16Z

@trishume I agree, the old configuration was very minimal. However, with such defaults it becomes a bit trickier to maintain backwards compatibility in the future. Like, we might want to add server_names_hash_bucket_size 64;, as that's something I've ran into quite early, but what if people already added this themselves using httpConfig? It can result in problems when upgrading NixOS systems.

Other distros do not have this problem, since you just take their initial configuration and keep using it. It'll never change.

We could play things safe and never change the configuration, but that won't help people using better defaults and prevents moving forward. The only way I can think of to make this reliable is adding tests. Maybe even include templated tests (functions that result in tests) or example tests so that server admins can implement their own tests to check whether all critical URLs actually work. Outside the scope of this PR though.

trishume · 2016-06-01T19:54:35Z

Yah I did mean that the "defaults" would be more like a starter pack option that enabled a bunch of recommended nginx settings. Maybe options that are off by default but you can enable like services.nginx.reccomendedSecurityOptions = true; and services.nginx.recommendedPerformanceOptions = true;. Or maybe a separate module like services.nginx-reccomendations.enable = true; with sub-options like services.nginx-reccomendations.performance = false;

bobvanderlinden · 2016-06-01T20:01:34Z

@trishume Ah, that would be nice as well. But it'll still trigger errors when such options are enabled. This PR in itself has the potential to cause configuration errors when using an existing configuration.nix. What aches me a bit is that Nginx is restarted on configuration changes instead of reloaded. There are probably good reasons for restarting, but it'll cause nginx to fully stop working when the Nginx configuration as a whole is incorrect. Since this PR has the potential to do this, I made #15906 to address this issue.

danbst · 2016-06-02T05:22:50Z

              locations."/" = {
                proxyPass = "http://localhost:3000";
              };

I think this buys little versus nginx language:

              locations."/" = ''
                proxy_pass http://localhost:3000;
              '';

and restricts many location specific options and overrides (I have some rewrites, if's, content_by_lua and so on in locations).

EDIT: didn't notice extraConfig. So my use case is still simple:

              locations."/".extraConfig = ''
                proxy_pass http://localhost:3000;
              '';

locations-options.nix

# This file defines the options that can be used both for the Apache
# main server configuration, and for the virtual hosts.

Can we support two styles for location configs (via types.lines and types.submodule)?

Also big +1 for allowing custom location matching strategies, you can now do nice hacks like

locations."= /test2".proxyPass = ...
locations."~ /test.*a".proxyPass = ...

Revert httpConfig its old behaviour and make it mutually exclusive to the new structured configuration. Adds appendHttpConfig to have the ability to write custom config in the generated http block.

Adds ::1 as allowed host and turns of access_log for the status page.

globin · 2016-07-31T22:36:31Z

@garbas: bump, I'd be happy if I don't need to merge this myself

garbas · 2016-08-01T09:01:22Z

@globin sry, weekend was extremely busy and couldn't find any time to test this. i'm just deploying this to my server for a test and lets see if something is broken afterwards.

garbas · 2016-08-01T11:09:38Z

@globin it works for me, without doing any changes to my config which is nice and ppl wont be surprised. merging it and if it does not work for somebody we can fix it or in the worse case revert this PR.

also we need to add this to changelog and to documentation. especially mentioning all those "recommended" options. you think you would have time to write it? nothing big just so people are aware of this features.

… config

This was referenced May 31, 2016

reverse proxy nixos module for nginx #9693

Closed

Adding services.nginx.http.server.<name> option #5956

Closed

fpletz added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 0.kind: enhancement Add something new 8.has: module (update) This PR changes an existing module in `nixos/` labels May 31, 2016

shlevy reviewed May 31, 2016
View reviewed changes

groxxda reviewed May 31, 2016
View reviewed changes

bobvanderlinden reviewed Jun 1, 2016
View reviewed changes

bobvanderlinden mentioned this pull request Jun 1, 2016

Nginx is restarted instead of reloaded on configuration changes #15906

Closed

groxxda mentioned this pull request Jun 12, 2016

tt-rss service: init at 16.3 #16132

Merged

7 tasks

globin and others added 14 commits July 28, 2016 11:59

nginx module: turn off basic auth on acme locations

e18f8e8

nginx module: redirect to same protocol

ff12ee3

nginx module: implement basic auth

1389455

nginx module: add option to make vhost default

3830a89

nginx module: Add default proxy headers for tomcat

e982aea

nginx module: Enable http2

de8008a

nginx module: improve documentation

5dd7cf9

nginx module: make httpConfig backward compatible

186a840

nginx module: make recommended settings optional

a294ad0

nginx module: add statusPage option

91680de

nginx: fixed duplicate http declaration

8c61b3a

nginx module: make client_max_body_size configurable

5114107

nginx module: httpConfig backward compatibility

3ccfca7

Revert httpConfig its old behaviour and make it mutually exclusive to the new structured configuration. Adds appendHttpConfig to have the ability to write custom config in the generated http block.

nginx module: improve statusPage generated code

a193fec

Adds ::1 as allowed host and turns of access_log for the status page.

globin force-pushed the nginx-module branch from 9333547 to a193fec Compare July 28, 2016 11:59

garbas merged commit 34237be into NixOS:master Aug 1, 2016

rvl mentioned this pull request Aug 9, 2016

services.nginx.config not working as described #17604

Closed

Nadrieril mentioned this pull request Aug 27, 2016

tt-rss service: Use nginx virtualhosts; improve config options #18047

Merged

7 tasks

Nadrieril added a commit to Nadrieril/nixpkgs that referenced this pull request Aug 28, 2016

tt-rss service: NixOS#15862 has been merged; enable nginx virtualhost…

789a37f

… config

fpletz deleted the nginx-module branch September 25, 2016 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declarative nginx module with ACME support #15862

Declarative nginx module with ACME support #15862

globin commented May 31, 2016

mention-bot commented May 31, 2016

trishume commented May 31, 2016

shlevy May 31, 2016

shlevy commented May 31, 2016

groxxda May 31, 2016

globin commented May 31, 2016

groxxda May 31, 2016

shlevy commented May 31, 2016

groxxda May 31, 2016

groxxda commented May 31, 2016

domenkozar commented Jun 1, 2016 •

edited

Loading

fpletz commented Jun 1, 2016

domenkozar commented Jun 1, 2016

bobvanderlinden Jun 1, 2016

bobvanderlinden commented Jun 1, 2016

trishume commented Jun 1, 2016

bobvanderlinden commented Jun 1, 2016 •

edited

Loading

trishume commented Jun 1, 2016

bobvanderlinden commented Jun 1, 2016 •

edited

Loading

danbst commented Jun 2, 2016 •

edited

Loading

globin commented Jul 31, 2016

garbas commented Aug 1, 2016

garbas commented Aug 1, 2016

Declarative nginx module with ACME support #15862

Declarative nginx module with ACME support #15862

Conversation

globin commented May 31, 2016

Motivation for this change

Things done

mention-bot commented May 31, 2016

trishume commented May 31, 2016

shlevy May 31, 2016

Choose a reason for hiding this comment

shlevy commented May 31, 2016

groxxda May 31, 2016

Choose a reason for hiding this comment

globin commented May 31, 2016

groxxda May 31, 2016

Choose a reason for hiding this comment

shlevy commented May 31, 2016

groxxda May 31, 2016

Choose a reason for hiding this comment

groxxda commented May 31, 2016

domenkozar commented Jun 1, 2016 • edited Loading

fpletz commented Jun 1, 2016

domenkozar commented Jun 1, 2016

bobvanderlinden Jun 1, 2016

Choose a reason for hiding this comment

bobvanderlinden commented Jun 1, 2016

trishume commented Jun 1, 2016

bobvanderlinden commented Jun 1, 2016 • edited Loading

trishume commented Jun 1, 2016

bobvanderlinden commented Jun 1, 2016 • edited Loading

danbst commented Jun 2, 2016 • edited Loading

globin commented Jul 31, 2016

garbas commented Aug 1, 2016

garbas commented Aug 1, 2016

domenkozar commented Jun 1, 2016 •

edited

Loading

bobvanderlinden commented Jun 1, 2016 •

edited

Loading

bobvanderlinden commented Jun 1, 2016 •

edited

Loading

danbst commented Jun 2, 2016 •

edited

Loading