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

Jump to bottom

nixos/caddy: add support for Caddy v2 #86686

Closed
wants to merge 5 commits into from
Closed

nixos/caddy: add support for Caddy v2 #86686

wants to merge 5 commits into from

Conversation

Br1ght0ne
Copy link
Member

@Br1ght0ne Br1ght0ne commented May 3, 2020 •
edited

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
Merged
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 marked this pull request as ready for review May 8, 2020 09:37
@Br1ght0ne
Copy link
Member Author

@GrahamcOfBorg test caddy

@ofborg ofborg bot requested review from rushmorem, zimbatm and kalbasit May 8, 2020 19:45
@Br1ght0ne Br1ght0ne mentioned this pull request Jul 12, 2020
Closed
@Br1ght0ne
Copy link
Member Author

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

KamilaBorowska
KamilaBorowska reviewed Jul 16, 2020
View reviewed changes
nixos/modules/services/web-servers/caddy.nix
${cfg.package}/bin/caddy adapt \
--config ${configFile} --adapter ${cfg.adapter} > $out
'');
# TODO: validate with `caddy validate`?
Copy link
Member

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 •
edited

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.

KamilaBorowska
KamilaBorowska suggested changes Jul 16, 2020
View reviewed changes
Copy link
Member

@KamilaBorowska KamilaBorowska left a comment

Choose a reason for hiding this comment

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

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

@AluisioASG
Copy link
Contributor

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?

@KamilaBorowska
Copy link
Member

KamilaBorowska commented Aug 18, 2020 •
edited

@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.

KamilaBorowska
KamilaBorowska suggested changes Aug 18, 2020
View reviewed changes
Copy link
Member

@KamilaBorowska KamilaBorowska left a comment •
edited

Choose a reason for hiding this comment

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

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.

This was referenced Sep 5, 2020
Closed
Merged
@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

Closed in favor of #97217 by @sephii.

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

@curbengh curbengh curbengh left review comments

@KamilaBorowska KamilaBorowska KamilaBorowska requested changes

@grahamc grahamc Awaiting requested review from grahamc

@yorickvP yorickvP Awaiting requested review from yorickvP

@fpletz fpletz Awaiting requested review from fpletz

@rushmorem rushmorem Awaiting requested review from rushmorem

@zimbatm zimbatm Awaiting requested review from zimbatm

@kalbasit kalbasit Awaiting requested review from kalbasit

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: clean-up 8.has: documentation 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Br1ght0ne @AluisioASG @k2s @sephii @KamilaBorowska @curbengh