nixos/caddy: Fix default log file for http:// hostnames #371802
Conversation
github-actions
bot
added
6.topic: nixos
|
This fixes |
github-actions
bot
added
10.rebuild-darwin: 0
|
Might be relevant: caddyserver/caddy#6766
This might be caused by Line 73 in 5634f52 I added this tests to ensure caddy with plugins don't regress overtime, but it seems that we need to update this hash every time there's been an update. The test was able to successfully run for that update PR because of the hash didn't change and it just used the cached version... |
| @@ -58,7 +58,7 @@ in | |||
| logFormat = mkOption { | |||
| type = types.lines; | |||
| default = '' | |||
| output file ${cfg.logDir}/access-${config.hostName}.log | |||
| output file ${cfg.logDir}/access-${lib.replaceStrings [ "/" ] [ "_" ] config.hostName}.log | |||
There was a problem hiding this comment.
IIRC we can also force proxied endpoint to use https by prefixing the hostname with https:// right? Would it be better if we remove https:// or http:// all together? I don't really have a preference here, I just want to hear your thoughts on this
There was a problem hiding this comment.
You're right. That would give us nicer filenames for sure. If someone defines http://example.com and https://example.com, both of them would go to access-example.com.log (does Caddy open the file twice then and does it do proper locking? - edit: looks like it deduplicates loggers by file name here). A catch-all written as just https:// is apparently also allowed and would be access-.log then.
I'm open for both options. We really just need a default that works, since it's very easy to provide a custom log file name anyways.
There was a problem hiding this comment.
Let's go with the current version. Could you also fix the hash mismatch and see if the test runs?
There was a problem hiding this comment.
Sure. With the fixed hash, the test runs fine.
Caddy hostnames can begin with http:// to disable automatic HTTPS. The default value for services.caddy.<host>.logFormat puts the hostname in the log filename, resulting in a broken path. Similarly, multiple space-separated host names would not work before. Since version 2.9.0 (commit 7c52e7a), caddy fails to start if it cannot open the log file. This caused NixOS test failures (e.g., nixosTests.dokuwiki).
this is new to me 🤣 both caddy and dokuwiki tests pass |
wegank
added
the
12.approvals: 1
Caddy hostnames can begin with http:// to disable automatic HTTPS. The default value for
services.caddy.<host>.logFormatputs the hostname in the log filename, resulting in a broken path.Since version 2.9.0 (commit 7c52e7a), caddy fails to start if it cannot open the log file. This caused NixOS test failures (e.g., nixosTests.dokuwiki).
See also #327743
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.