Nginx formatter #34985

Baughn · 2018-02-14T22:21:31Z

Motivation for this change

This makes nginx.conf human-readable.

While nominally the file is not meant for human consumption, in practice I commonly need to manually inspect the output of complex nginx configurations to verify that it actually looks the way I want. Being able to read it is therefore useful.

Before formatting: http://ix.io/K9v
After formatting: http://ix.io/K9k

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Tested on a live system (https://madoka.brage.info), using cherry-pick against nixos-unstable release.
Fits CONTRIBUTING.md.

grahamc · 2018-02-14T22:27:10Z

@GrahamcOfBorg test nginx

GrahamcOfBorg · 2018-02-14T22:27:45Z

Success on x86_64-linux (full log)

Partial log (click to expand)

syncing
webserver: running command: sync
webserver# [    8.165974] dhcpcd[781]: eth0: soliciting an IPv6 router
webserver: exit status 0
test script finished in 9.13s
cleaning up
killing webserver (pid 593)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/tmp/nix-build-vm-test-run-nginx.drv-0/vde1.ctl': Directory not empty
/nix/store/rgn11hzfg47r61lgx1l63fs5d272r3z5-vm-test-run-nginx

GrahamcOfBorg · 2018-02-14T22:28:10Z

Success on aarch64-linux (full log)

Partial log (click to expand)

webserver: exit status 1
syncing
webserver: running command: sync
webserver: exit status 0
test script finished in 17.67s
cleaning up
killing webserver (pid 627)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/fmv3v3gjzli3zacza0dyg0i9jrgk34z9-vm-test-run-nginx

grahamc · 2018-02-14T22:33:13Z

Very cool!

nh2 · 2018-02-15T00:15:41Z

Could we add an option for it so it can be turned off?

I want to use it myself, but I know that others (e362a3d#commitcomment-20787179) would like to not have to hard-depend on Python for changing nginx configs.

Mic92 · 2018-02-15T08:31:31Z

Maybe make it opt-in? Everybody who cares about it, can temporary turn it on or leave it on all the time.

This reverts commit 6e12406, and causes the nginx test to fail.

This fixes the nginx test failure.

Baughn · 2018-02-15T21:01:30Z

I've added an option.

I'd prefer to not default it to false, as anyone attempting to debug their nginx configs is unlikely to guess that the option exists, given that most other NixOS config files don't have this functionality.

(Which is a problem in itself, but not in scope of this PR.)

Baughn · 2018-02-15T21:02:07Z

@GrahamcOfBorg test nginx

joachifm · 2018-02-17T09:26:45Z

@GrahamcOfBorg test nginx

GrahamcOfBorg · 2018-02-17T09:27:26Z

Success on x86_64-linux (full log)

Partial log (click to expand)

webserver: running command: sync
webserver# [  OK  ] Listening on Load/Save RF Kill Switch Status /dev/rfkill Watch.
webserver# [    5.383236] input: ImExPS/2 Generic Explorer Mouse as /devices/platform/i8042/serio1/input/input4
webserver: exit status 0
test script finished in 6.12s
cleaning up
killing webserver (pid 593)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/7aww8iip618n3arzlaicg9zvwpwmgnki-vm-test-run-nginx

GrahamcOfBorg · 2018-02-17T09:27:54Z

Success on aarch64-linux (full log)

Partial log (click to expand)

webserver: exit status 1
syncing
webserver: running command: sync
webserver: exit status 0
test script finished in 17.96s
cleaning up
killing webserver (pid 627)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/build/vde1.ctl': Directory not empty
/nix/store/vqbpglkrynbzh0xilj8vfhhww7200dzn-vm-test-run-nginx

Mic92 · 2018-02-21T22:22:33Z

@Baughn ok. In the unlikely case of bugs in the formatter, it could then at least be disabled.

fpletz · 2018-02-22T05:45:10Z

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

+
+  configFileFormatted = pkgs.runCommand "nginx.conf" {
+    inherit configFileUnformatted;
+    passAsFile = [ "configFileUnformatted" ];


I do not think that these two lines are needed because the reference to configFileUnformatted in the multi-line string below is directly to the store path that writeText creates a few lines further down.

@Baughn I can confirm the above two lines are not necessary.

Could you remove them? I would love to have this merged.

timokau

See other review (just marking as changes requested).

infinisil · 2018-12-29T21:18:56Z

I'm really not in favor of this. Both pulling in python and having a new option sound like bad solutions to the problem. When we are the ones who generate such a crippled Nix file, we should also be responsible for fixing it!

The solution I'm proposing is to generate the file in a way similar to what I've done in #45470. That is, having a config option with a type of a nix value representing the configuration file in a reasonable way, then converting this to a list of lines and concatenate them all.

As a side effect this has the advantage that users are able to override default settings easily. A slight problem with this is that verbatim options can't be formatted this way, but if such a config option exists, users may not even need many verbatim options anymore anyways. Also it's a great integration into the module system with almost no cost.

As an example, this is how the start of the config as specified in nginx/default.nix could look like:

{
  config = {
    user = "${cfg.user} ${cfg.group}";
    error_log = cfg.logError;
    daemon = false;
    
    # No need for null check, null values automatically have no effect
    events = cfg.eventsConfig;
    
    http = mkIf cfg.recommendedOptimisation {
      sendfile = true;
      tcp_nopush = true;
      tcp_nodelay = true;
      keepalive_timeout = 65;
      types_hash_max_size = 2048;
    } // {
      # A list generates a key-value entry for every element
      include = [
        "${cfg.package}/conf/mime.types"
        "${cfg.package}/conf/fastcgi.conf"
        "${cfg.package}/conf/uwsgi_params"
      ];
      resolver = mkIf (cfg.resolver.addresses != [])
        "${toString cfg.resolver.addresses} ${optionalString (cfg.resolver.valid != "") "valid=${cfg.resolver.valid}"}";
      
      ssl_protocols = cfg.sslProtocols;
    };
  };
}

aanderse · 2019-04-10T01:26:47Z

Maybe this should be closed as #57979 was merged?

nginx: Add test for non-ascii config elements

f564a6e

Baughn mentioned this pull request Feb 14, 2018

nginx module: fails to generate config containing non ascii characters #22883

Closed

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) labels Feb 14, 2018

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 labels Feb 14, 2018

Baughn added 2 commits February 15, 2018 21:00

nginx: Format the config file

76ebe2c

This reverts commit 6e12406, and causes the nginx test to fail.

nginx-config-formatter: Bump to version 2017-07-26

47aa537

This fixes the nginx test failure.

Baughn force-pushed the nginx-formatter branch from 9fcac9e to 47aa537 Compare February 15, 2018 21:00

joachifm requested a review from globin February 17, 2018 09:29

fpletz reviewed Feb 22, 2018

View reviewed changes

timokau requested changes Dec 29, 2018

View reviewed changes

timokau closed this Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nginx formatter #34985

Nginx formatter #34985

Baughn commented Feb 14, 2018

grahamc commented Feb 14, 2018

GrahamcOfBorg commented Feb 14, 2018

GrahamcOfBorg commented Feb 14, 2018

grahamc commented Feb 14, 2018

nh2 commented Feb 15, 2018

Mic92 commented Feb 15, 2018 •

edited

Baughn commented Feb 15, 2018 •

edited

Baughn commented Feb 15, 2018

joachifm commented Feb 17, 2018

GrahamcOfBorg commented Feb 17, 2018

GrahamcOfBorg commented Feb 17, 2018

Mic92 commented Feb 21, 2018 •

edited

fpletz Feb 22, 2018

nh2 Jun 27, 2018

timokau left a comment

infinisil commented Dec 29, 2018

aanderse commented Apr 10, 2019

Nginx formatter #34985

Nginx formatter #34985

Conversation

Baughn commented Feb 14, 2018

Motivation for this change

Things done

grahamc commented Feb 14, 2018

GrahamcOfBorg commented Feb 14, 2018

GrahamcOfBorg commented Feb 14, 2018

grahamc commented Feb 14, 2018

nh2 commented Feb 15, 2018

Mic92 commented Feb 15, 2018 • edited

Baughn commented Feb 15, 2018 • edited

Baughn commented Feb 15, 2018

joachifm commented Feb 17, 2018

GrahamcOfBorg commented Feb 17, 2018

GrahamcOfBorg commented Feb 17, 2018

Mic92 commented Feb 21, 2018 • edited

fpletz Feb 22, 2018

Choose a reason for hiding this comment

nh2 Jun 27, 2018

Choose a reason for hiding this comment

timokau left a comment

Choose a reason for hiding this comment

infinisil commented Dec 29, 2018

aanderse commented Apr 10, 2019

Mic92 commented Feb 15, 2018 •

edited

Baughn commented Feb 15, 2018 •

edited

Mic92 commented Feb 21, 2018 •

edited