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.

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

nixos/caddy: add support for Caddy v2 #86686

Closed
wants to merge 5 commits into from

Conversation

Br1ght0ne
Copy link
Member

@Br1ght0ne Br1ght0ne commented May 3, 2020

Motivation for this change

Caddy v2 was recently released.
While v2 support is necessary, configs for v1 shouldn't break (at least until 20.09).

Things done

This PR adds support for Caddy v2, and uses it by default, while still allowing users to opt-in to using Caddy v1.

Added services.caddy.adapter for supporting config formats other than Caddyfile.
See https://caddyserver.com/docs/config-adapters.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Br1ght0ne Br1ght0ne mentioned this pull request May 4, 2020
10 tasks
@Br1ght0ne Br1ght0ne changed the title nixos-caddy: add support for v2 nixos/caddy: add support for Caddy v2 May 8, 2020
@Br1ght0ne Br1ght0ne force-pushed the nixos-caddy-v2-migration branch from 25e256a to 72b9184 Compare May 8, 2020
@Br1ght0ne Br1ght0ne marked this pull request as ready for review May 8, 2020
@Br1ght0ne Br1ght0ne requested review from grahamc, xfix, yorickvP and fpletz May 8, 2020
@Br1ght0ne
Copy link
Member Author

Br1ght0ne commented May 8, 2020

@GrahamcOfBorg test caddy

@Br1ght0ne Br1ght0ne force-pushed the nixos-caddy-v2-migration branch from 598dae6 to 4d7b3c2 Compare Jul 14, 2020
@Br1ght0ne
Copy link
Member Author

Br1ght0ne commented Jul 14, 2020

@xfix I removed the commit adding --validate and rebased onto latest master.

@Br1ght0ne Br1ght0ne requested a review from xfix Jul 14, 2020
${cfg.package}/bin/caddy adapt \
--config ${configFile} --adapter ${cfg.adapter} > $out
'');
# TODO: validate with `caddy validate`?
Copy link
Member

@xfix xfix Jul 16, 2020

Choose a reason for hiding this comment

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

I don't think we actually want caddy validate as I mentioned earlier (mostly because it verifies paths exist, and it's reasonable that they may not exist in build environment due to being outside nix store).

Suggested change
# TODO: validate with `caddy validate`?

Copy link

@curbengh curbengh Jul 18, 2020

Choose a reason for hiding this comment

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

to add on, adapt validates the input config (check its syntax) by default and will return error 1 when encounter invalid input.

Copy link
Member

@xfix xfix left a comment

This looks fine, it's mergeable after the TODO comment gets deleted.

@AluisioASG
Copy link
Contributor

AluisioASG commented Aug 9, 2020

ExecReload has to be updated as well:

Signals are mostly the same, except USR1 and USR2 are no longer supported. Use the caddy reload command or the API instead to load new configuration.

@k2s
Copy link
Contributor

k2s commented Aug 9, 2020

There are 2 official upstream systemd service files with explanation at https://github.com/caddyserver/dist/tree/master/init.

In compare to v1 the Caddy v2 starts without configuration file and best practice in v2 is to use API to upload config. JSON is native format, but with simple curl POST it is possible to upload regular Caddyfile too. Caddy reloads last state after restart. All that is working with https://github.com/caddyserver/dist/blob/master/init/caddy-api.service.

In my opinion that this should be default in NixOS. The configuration file should be added to command line arguments only if additional parameters related to configuration source and provider are specified.

@sephii
Copy link
Contributor

sephii commented Aug 11, 2020

It would be really nice to have this in 20.09. :) can I help in some way?

@xfix
Copy link
Member

xfix commented Aug 18, 2020

@filalex77 What is the status of this pull request? I really would like to have this in NixOS 20.09.

My opinion is that we don't need Caddy API support for 20.09. It could be implemented in the future, but I don't think it should block Caddy v2 support.

Copy link
Member

@xfix xfix left a comment

ExecReload = "${pkgs.coreutils}/bin/kill -USR1 $MAINPID"; should be changed to use caddy reload --config ${path} instead for Caddy 2 as USR1 signal is no longer supported.

@sephii
Copy link
Contributor

sephii commented Sep 7, 2020

I've opened a new PR which addresses the remaining comments in the hope of getting this in 20.09. I've also rebased on current master. I still have problems with running tests though, any help is appreciated.

@Br1ght0ne
Copy link
Member Author

Br1ght0ne commented Sep 7, 2020

Closed in favor of #97217 by @sephii.

@Br1ght0ne Br1ght0ne closed this Sep 7, 2020
@Br1ght0ne Br1ght0ne deleted the nixos-caddy-v2-migration branch Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment