From 0640622eb1fb3ca4182e71aa3a48def88458ecec Mon Sep 17 00:00:00 2001
From: Christian Theune <ct@flyingcircus.io>
Date: Tue, 14 Jan 2025 16:44:08 +0100
Subject: [PATCH] nixos/varnish: fix stateDir to allow direct use of
 `varnishadm`

---
 .../services/web-servers/varnish/default.nix  | 47 +++++++++++--------
 nixos/tests/varnish.nix                       |  8 +++-
 pkgs/servers/varnish/default.nix              |  2 +-
 3 files changed, 35 insertions(+), 22 deletions(-)

diff --git a/nixos/modules/services/web-servers/varnish/default.nix b/nixos/modules/services/web-servers/varnish/default.nix
index 033dc631289ba..6b3ff33d23487 100644
--- a/nixos/modules/services/web-servers/varnish/default.nix
+++ b/nixos/modules/services/web-servers/varnish/default.nix
@@ -8,6 +8,24 @@
 let
   cfg = config.services.varnish;
 
+  # Varnish has very strong opinions and very complicated code around handling
+  # the stateDir. After a lot of back and forth, we decided that we a)
+  # do not want a configurable option here, as most of the handling depends
+  # on the version and the compile time options. Putting everything into
+  # /var/run (RAM backed) is absolutely recommended by Varnish anyways.
+  # We do need to pay attention to the version-dependend variations, though!
+  stateDir =
+    if
+      (lib.versionOlder cfg.package.version "7")
+    # Remove after Varnish 6.0 is gone. In 6.0 varnishadm always appends the
+    # hostname (by default) and can't be nudged to not use any name. This has
+    # long changed by 7.5 and can be used without the host name.
+    then
+      "/var/run/varnish/${config.networking.hostName}"
+    # Newer varnish uses this:
+    else
+      "/var/run/varnishd";
+
   commandLine =
     "-f ${pkgs.writeText "default.vcl" cfg.config}"
     +
@@ -17,6 +35,14 @@ let
          }' -r vmod_path";
 in
 {
+  imports = [
+    (lib.mkRemovedOptionModule [
+      "services"
+      "varnish"
+      "stateDir"
+    ] "The `stateDir` option never was functional or useful. varnish uses compile-time settings.")
+  ];
+
   options = {
     services.varnish = {
       enable = lib.mkEnableOption "Varnish Server";
@@ -42,14 +68,6 @@ in
         '';
       };
 
-      stateDir = lib.mkOption {
-        type = lib.types.path;
-        default = "/run/varnish/${config.networking.hostName}";
-        defaultText = lib.literalExpression ''"/run/varnish/''${config.networking.hostName}"'';
-        description = ''
-          Directory holding all state for Varnish to run. Note that this should be a tmpfs in order to avoid performance issues and crashes.
-        '';
-      };
       extraModules = lib.mkOption {
         type = lib.types.listOf lib.types.package;
         default = [ ];
@@ -76,24 +94,15 @@ in
       description = "Varnish";
       wantedBy = [ "multi-user.target" ];
       after = [ "network.target" ];
-      preStart = lib.mkIf (!(lib.hasPrefix "/run/" cfg.stateDir)) ''
-        mkdir -p ${cfg.stateDir}
-        chown -R varnish:varnish ${cfg.stateDir}
-      '';
-      postStop = lib.mkIf (!(lib.hasPrefix "/run/" cfg.stateDir)) ''
-        rm -rf ${cfg.stateDir}
-      '';
       serviceConfig = {
         Type = "simple";
         PermissionsStartOnly = true;
-        ExecStart = "${cfg.package}/sbin/varnishd -a ${cfg.http_address} -n ${cfg.stateDir} -F ${cfg.extraCommandLine} ${commandLine}";
+        ExecStart = "${cfg.package}/sbin/varnishd -a ${cfg.http_address} -n ${stateDir} -F ${cfg.extraCommandLine} ${commandLine}";
         Restart = "always";
         RestartSec = "5s";
         User = "varnish";
         Group = "varnish";
-        RuntimeDirectory = lib.mkIf (lib.hasPrefix "/run/" cfg.stateDir) (
-          lib.removePrefix "/run/" cfg.stateDir
-        );
+        RuntimeDirectory = lib.removePrefix "/var/run/" stateDir;
         AmbientCapabilities = "cap_net_bind_service";
         NoNewPrivileges = true;
         LimitNOFILE = 131072;
diff --git a/nixos/tests/varnish.nix b/nixos/tests/varnish.nix
index ec9f058537374..ee60a9e392fa4 100644
--- a/nixos/tests/varnish.nix
+++ b/nixos/tests/varnish.nix
@@ -56,8 +56,12 @@ import ./make-test-python.nix (
 
       client.wait_until_succeeds("curl -f http://varnish/nix-cache-info");
 
-      client.wait_until_succeeds("nix-store -r ${testPath}");
-      client.succeed("${testPath}/bin/hello");
+      client.wait_until_succeeds("nix-store -r ${testPath}")
+      client.succeed("${testPath}/bin/hello")
+
+      output = varnish.succeed("varnishadm status")
+      print(output)
+      assert "Child in state running" in output, "Unexpected varnishadm response"
     '';
   }
 )
diff --git a/pkgs/servers/varnish/default.nix b/pkgs/servers/varnish/default.nix
index b9e4b0de6c3ec..4c0a7afb09865 100644
--- a/pkgs/servers/varnish/default.nix
+++ b/pkgs/servers/varnish/default.nix
@@ -54,7 +54,7 @@ let
         ++ lib.optional stdenv.hostPlatform.isDarwin libunwind
         ++ lib.optional stdenv.hostPlatform.isLinux jemalloc;
 
-      buildFlags = [ "localstatedir=/var/spool" ];
+      buildFlags = [ "localstatedir=/var/run" ];
 
       postPatch = ''
         substituteInPlace bin/varnishtest/vtc_main.c --replace /bin/rm "${coreutils}/bin/rm"