Skip to content
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.

Sign up for GitHub

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

Jump to bottom

nginx: expose generated config and allow nginx reloads #57429

Merged
merged 5 commits into from Aug 21, 2019
Merged

nginx: expose generated config and allow nginx reloads #57429

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8b036d5
nginx: expose generated config and allow nginx reloads
danbst Mar 11, 2019
128d1a5
Merge branch 'master' into nginx-reload-config
danbst Aug 9, 2019
deb8cfd
rename to enableReload
danbst Aug 9, 2019
5f86231
add sleep(1) in ETag test (race condition) and rewrite rebuild-switch…
danbst Aug 18, 2019
e6ff708
aszlig nitpick
danbst Aug 18, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
33 changes: 31 additions & 2 deletions nixos/modules/services/web-servers/nginx/default.nix
View file
Open in desktop
Expand Up @@ -162,6 +162,10 @@ let
${cfg.appendConfig}
'';

configPath = if cfg.enableReload
then "/etc/nginx/nginx.conf"
else configFile;

vhosts = concatStringsSep "\n" (mapAttrsToList (vhostName: vhost:
let
onlySSL = vhost.onlySSL || vhost.enableSSL;
Expand Down Expand Up @@ -431,6 +435,16 @@ in
";
};

enableReload = mkOption {
default = false;
Copy link
Member

Choose a reason for hiding this comment

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

I thought everybody agreed to make this true by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 Maybe I'm just too cautious to change default behavior. Would you like to do that? 😁

type = types.bool;
description = ''
Reload nginx when configuration file changes (instead of restart).
The configuration file is exposed at <filename>/etc/nginx/nginx.conf</filename>.
See also <literal>systemd.services.*.restartIfChanged</literal>.
'';
};

stateDir = mkOption {
default = "/var/spool/nginx";
description = "
Expand Down Expand Up @@ -638,17 +652,32 @@ in
preStart =
''
${cfg.preStart}
${cfg.package}/bin/nginx -c ${configFile} -p ${cfg.stateDir} -t
${cfg.package}/bin/nginx -c ${configPath} -p ${cfg.stateDir} -t
'';
serviceConfig = {
ExecStart = "${cfg.package}/bin/nginx -c ${configFile} -p ${cfg.stateDir}";
ExecStart = "${cfg.package}/bin/nginx -c ${configPath} -p ${cfg.stateDir}";
ExecReload = "${pkgs.coreutils}/bin/kill -HUP $MAINPID";
Restart = "always";
RestartSec = "10s";
StartLimitInterval = "1min";
};
};

environment.etc."nginx/nginx.conf" = mkIf cfg.enableReload {
Copy link
Member

Choose a reason for hiding this comment

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

Hm, why is this only enabled if enableReload is true? Apart from that, I do have mixed feelings about exposing the config in /etc, because I do not want other applications to make certain assumptions about files /etc and generally want to keep as much things out of global paths to ensure they don't behave differently in a Nix build sandbox. That said, I can still mkForce it to an empty attrset, but maybe adding exposeConfigFile with a default of false might be a good idea? Maybe depends on how many users want it to occur in /etc versus how many do not want that behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh, the original commit actually called that right 8b036d5#diff-795fa65a7c41a479084de826e4e2e65cR460

It just people suggested that exposeConfig isn't a good name and it should actually be enableReload

Actually, I'd like to write that file in some other place, but only /etc has atomic replaces in NixOS. I don't want to move nginx.conf to runtime dir, as I'd like to have post-mortem (after nginx doesn't start) config checks.

source = configFile;
};

systemd.services.nginx-config-reload = mkIf cfg.enableReload {
wantedBy = [ "nginx.service" ];
restartTriggers = [ configFile ];
script = ''
if ${pkgs.systemd}/bin/systemctl -q is-active nginx.service ; then
${pkgs.systemd}/bin/systemctl reload nginx.service
Copy link
Member

Choose a reason for hiding this comment

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

This should be config.systemd.package, but I think systemctl is in PATH by default, so it can be just a non-absolute systemctl.

fi
'';
serviceConfig.RemainAfterExit = true;
};

security.acme.certs = filterAttrs (n: v: v != {}) (
let
vhostsConfigs = mapAttrsToList (vhostName: vhostConfig: vhostConfig) virtualHosts;
Expand Down
60 changes: 43 additions & 17 deletions nixos/tests/nginx.nix
View file
Open in desktop
Expand Up @@ -3,15 +3,15 @@
# generated virtual hosts config.
# 2. whether the ETag header is properly generated whenever we're serving
# files in Nix store paths

# 3. nginx doesn't restart on configuration changes (only reloads)
import ./make-test.nix ({ pkgs, ... }: {
name = "nginx";
meta = with pkgs.stdenv.lib.maintainers; {
maintainers = [ mbbx6spp ];
};

nodes = let
commonConfig = { pkgs, ... }: {
nodes = {
webserver = { pkgs, lib, ... }: {
services.nginx.enable = true;
services.nginx.commonHttpConfig = ''
log_format ceeformat '@cee: {"status":"$status",'
Expand All @@ -32,30 +32,42 @@ import ./make-test.nix ({ pkgs, ... }: {
location /favicon.ico { allow all; access_log off; log_not_found off; }
'';
};

services.nginx.virtualHosts.localhost = {
root = pkgs.runCommand "testdir" {} ''
mkdir "$out"
echo hello world > "$out/index.html"
'';
};
};
in {
webserver = commonConfig;

newwebserver = { pkgs, lib, ... }: {
imports = [ commonConfig ];
services.nginx.virtualHosts.localhost = {
root = lib.mkForce (pkgs.runCommand "testdir2" {} ''
mkdir "$out"
echo hello world > "$out/index.html"
'');
};
services.nginx.enableReload = true;

nesting.clone = [
{
services.nginx.virtualHosts.localhost = {
root = lib.mkForce (pkgs.runCommand "testdir2" {} ''
mkdir "$out"
echo content changed > "$out/index.html"
'');
};
}

{
services.nginx.virtualHosts."1.my.test".listen = [ { addr = "127.0.0.1"; port = 8080; }];
}

{
services.nginx.package = pkgs.nginxUnstable;
}
];
};

};

testScript = { nodes, ... }: let
newServerSystem = nodes.newwebserver.config.system.build.toplevel;
switch = "${newServerSystem}/bin/switch-to-configuration test";
etagSystem = "${nodes.webserver.config.system.build.toplevel}/fine-tune/child-1";
justReloadSystem = "${nodes.webserver.config.system.build.toplevel}/fine-tune/child-2";
reloadRestartSystem = "${nodes.webserver.config.system.build.toplevel}/fine-tune/child-3";
in ''
my $url = 'http://localhost/index.html';

Expand All @@ -77,9 +89,23 @@ import ./make-test.nix ({ pkgs, ... }: {

subtest "check ETag if serving Nix store paths", sub {
my $oldEtag = checkEtag;
$webserver->succeed('${switch}');
$webserver->succeed("${etagSystem}/bin/switch-to-configuration test >&2");
$webserver->sleep(1); # race condition
my $newEtag = checkEtag;
die "Old ETag $oldEtag is the same as $newEtag" if $oldEtag eq $newEtag;
};

subtest "config is reloaded on nixos-rebuild switch", sub {
$webserver->succeed("${justReloadSystem}/bin/switch-to-configuration test >&2");
$webserver->waitForOpenPort("8080");
$webserver->fail("journalctl -u nginx | grep -q -i stopped");
$webserver->succeed("journalctl -u nginx | grep -q -i reloaded");
};

subtest "restart when nginx package changes", sub {
$webserver->succeed("${reloadRestartSystem}/bin/switch-to-configuration test >&2");
$webserver->waitForUnit("nginx");
$webserver->succeed("journalctl -u nginx | grep -q -i stopped");
};
'';
})