nixos/caddy: validate at build-time

[Stunkymonkey]

its nice to validate the config before reloading/restarting. same as nginx

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[sephii]

We used to have this done in the build phase (which, in my opinion, is a better place to have it) before it was removed in 27b7132 (because the way it was done caused an import from derivation, but I think it can be easily adapted). How about doing the check in the build phase?

[Stunkymonkey]

@sephii doing this while building is an ever better idea. To catch the errors as early as possible. I am marking my MR as draft and ping you again if I have something. Thanks for the fast response.

[Stunkymonkey]

@sephii validating is a bit weird. caddy validate results into:

       > 2025/01/27 21:46:30.999 INFO    using config from file  {"file": "/nix/store/kz8lm6k81lsmziphpq0mfhdrv4nx049v-Caddyfile-formatted/Caddyfile"}
       > 2025/01/27 21:46:31.009 INFO    adapted config to JSON  {"adapter": "caddyfile"}
       > Error: setting up custom log 'log1': opening log writer using &logging.FileWriter{Filename:"/var/log/caddy/access-http:__localhost:8081.log", Mode:0x0, Roll:(*bool)(nil), RollSizeMB:0, RollCompress:(*bool)(nil), RollLocalTime:false, RollKeep:0, RollKeepDays:0}: mkdir /var: permission denied

it always wants to start and log the events, which can not work... so then i discovered caddy adapt, which validates the config itself. But somehow the json-test is not working any more, because of:

       last 1 log lines:
       > Error: Unexpected '{' on a new line; did you mean to place the '{' on the previous line?, at /nix/store/f2927acpq53srgy3sl722ssqj4yrn60k-caddy.json:10
       For full logs, run 'nix log /nix/store/b6ccpz1rfqvnj4ysdm4qmf5m4wb7r4vd-validate-caddy-config.drv'.

but the config is a valid json... not sure why caddy does this: https://github.com/caddyserver/caddy/blob/9996d6a70ba76a94dfc90548f25fbac0ce9da497/caddyconfig/caddyfile/parse.go#L656

[sephii]

Sorry I’m very busy these days. I’ll give it a look soon, maybe next week!

[Stunkymonkey]

@sephii no problem. thanks for reaching out.

[Stunkymonkey]

I am currently stuck here caddyserver/caddy#6788 (comment) .

[mohammed90]

caddy adapt, which validates the config itself

That's not what adapt does. It doesn't validate config. It only converts it from whatever format the specified adapter accepts to JSON. The validation is something separate.

[Stunkymonkey]

@sephii i finally managed to put something together. please take your time.

[sephii]

Ok I finally got some time for this, thanks for your patience. Based on your code I worked out a different implementation that does not rely on an assertion but on running the adapt command in the Caddyfile generation (inspired by the way it’s done in the nginx module).

I’ll let you check it out and make changes to your PR if you think it’s good, otherwise please let me know.

[Stunkymonkey]

@sephii thanks for this. much more cleaner and simpler. lets not over-complicate it.

[sephii]

Could you please also revert the change to the adapterParam variable which is not necessary anymore?

[Stunkymonkey]

@sephii should be correct now. thanks for your help and your suggestions.

[sephii]

Looks good to me, thanks!

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/5269

[Stunkymonkey]

@sephii are you fine with merging? I could do it, or do you want additional reviewer?

[sephii]

If you can merge it, please do!

[jmbaur]

This breaks cross-compiled nixos configs, since you can't run a non native binary on a different kind of build machine.

[sephii]

Would it be enough to pass cfg.package as a native build input on line 53 and then just call caddy adapt?

[cything]

Have you tested this with environment variables? I use an environment variable for API token and the build fails with Error: parsing caddyfile tokens for 'tls': missing API token. This is the part it's complaining about:

(common) {
          encode zstd gzip
          header Strict-Transport-Security "max-age=63072000; includeSubDomains; preload"
          tls {
            dns cloudflare {$CLOUDFLARE_KEY}
            resolvers 1.1.1.1 8.8.8.8
          }
        }

The environment variable is obviously not available at build time.

[sephii]

This seems indeed incompatible with local validation. How about we add a validateConfigFile toggle option (like the nginx module)?

I suggest we also add an entry to the release notes since this change is not as backwards compatible as I thought (or set validateConfigFile to false by default, what’s your opinion on this?).

[cything]

This seems indeed incompatible with local validation. How about we add a validateConfigFile toggle option (like the nginx module)?

If this breaks everyone passing API tokens with the environmentFile option, we definitely need this.

I suggest we also add an entry to the release notes since this change is not as backwards compatible as I thought (or set validateConfigFile to false by default, what’s your opinion on this?).

If this is limited to environment variables, we could set validateConfigFile to false if environmentFile is set and default to true otherwise. And add a description under validateConfigFile explaining why.

[Stunkymonkey]

maybe we should merge #393806 until we figured out the rest of the problems

sorry for the delayed response, I was ill for a few days

[6543]

there was #380894 already dealing with this stuff ...

[6543]

@Stunkymonkey you removed the nativeBuildInputs = [ cfg.package ]; - have you tested that it works in cross-compile environments now!

[6543]

PS: we should make it a config to disable it! as there are cases where it will fail!

example: have static own ssl certs on the target host but not on the build host ...

[6543]

ok I have updated #380894 ...

[sephii]

there was #380894 already dealing with this stuff ...

This PR was created on Jan 26, your PR was created on Feb 10.

Your PR seems to suffer from the same problem that was mentioned in this discussion regarding the use of environment variables in the Caddy config. We should either disable the configuration check at build time by default, or find a way to only enable it if we’re sure the config can be checked locally.

[6543]

the default setup can be enabled only quite complex can not ... and in this case the option can simply be disabled ...