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: introduce several new options #147973

Merged
merged 1 commit into from
Dec 25, 2021
Merged

Conversation

aanderse
Copy link
Member

@aanderse aanderse commented Nov 30, 2021

Motivation for this change

I have started using caddy, so I added some features which I thought users coming from httpd or nginx might appreciate. Additionally, caddy doesn't have a plugin to support my DNS provider* 😞 so I added some integration with our acme module so sysadmins can delegate certificate provisioning to security.acme.certs if desired/required.

NOTE: If this the code in this PR is accepted there is one breaking change that will need to be carefully documented in the release notes: services.caddy.config (which was renamed to services.caddy.extraConfig) no longer accepts any syntax other than Caddyfile. It would be relatively trivial for users to modify their configuration.nix to account for this by using the new configFile option, for which an example is already provided.

* Though even if a plugin existed I wouldn't be able to compile a version of caddy with that plugin, or any other... more on that in a future PR/issue.

This PR probably resolves #113534 and #142787.

Things done
  • introduce logFormat, logDir, and configFile options
  • introduce hostName, listenAddresses, useACMEHost, logFormat options under virtualHosts
  • rename config -> extraConfig, ca -> acmeCA
  • drop generated json in favour of a pure Caddyfile solution
    • the introduction configFile allows the user to continue utilizing json if they wish
  • only attempt to run caddy fmt and caddy validate when possible
  • improved some documentation & code formatting
  • brought module more inline with nginx and httpd

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • 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/)
  • 21.11 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@@ -200,13 +268,19 @@ in
group = cfg.group;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to question, do we lose anything here by using DynamicUser ? You don't have to do that in this PR of course, but I was wondering if we would lose anything by not defining the user entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was running caddy as DynamicUser for a while with success. We might need to consider a few special cases, but it should probably work.

@happysalada
Copy link
Contributor

I personally don't have an issue with config being only caddyfile rather than json. I'm curious to know if there are people that use the json config.

@aanderse
Copy link
Member Author

Comments addressed, literalExample replaced so CI should pass now 🤞 I also looked through the generated options.html and everything looked good.

@happysalada
Copy link
Contributor

I've just tested this on a server.
I was a little worried about extraConfig from caddy being appended to the caddy config, because that is where I put all the common snippets to my other configurations. It did work without a problem though.
I'm personally fine with this PR.

@aanderse
Copy link
Member Author

aanderse commented Dec 3, 2021

Any word from @onny, @yorickvP, or @mkaito on whether this change is welcome or not?

@onny
Copy link
Contributor

onny commented Dec 5, 2021

This looks really cool! I could take a look that modules like Dokuwiki, Wordpress and Invoiceplane gets updated to support the new options of the Caddy module.
I guess it would still look like this:

  services.caddy = {
    enable = true;
    virtualHosts."example.com"  {
        extraConfig = ''
          root * /var/www
          file_server
          php_fastcgi unix/var/run/php-fpm/example.com.socket
        '';

@aanderse
Copy link
Member Author

aanderse commented Dec 5, 2021

I guess it would still look like this

Yeah, very compatible change, just adds a few new options that won't impact most people.

@happysalada
Copy link
Contributor

@aanderse happy to merge this, but you marked this as draft. Did you have anything else in mind ?

@aanderse
Copy link
Member Author

Was hoping to get another review before marking as ready for review... but we're really in the release cycle so I think we're good.

Final review required by @m1cr0man my acme superhero! ❤️

@aanderse aanderse marked this pull request as ready for review December 11, 2021 00:50
Copy link
Contributor

@m1cr0man m1cr0man left a comment

Choose a reason for hiding this comment

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

Spotted a typo + have a note suggestion, but otherwise happy enough with this. I may add a test for caddy to the acme suite too (in #147784 where I refactored the tests) since this will be an "officially" supported ACME integration per se.

systemd.packages = [ cfg.package ];
systemd.services.caddy = {
wants = map (hostOpts: "acme-finished-${hostOpts.useACMEHost}-.target") acmeVHosts;
after = map (hostOpts: "acme-selfsigned-${hostOpts.useACMEHost}.service") acmeVHosts;
before = map (hostOpts: "acme-${hostOpts.useACMEHost}.service") acmeVHosts;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 this is the right way to depend on acme for a web server

nixos/modules/services/web-servers/caddy/default.nix Outdated Show resolved Hide resolved
nixos/modules/services/web-servers/caddy/vhost-options.nix Outdated Show resolved Hide resolved
@jian-lin
Copy link
Contributor

I've tested this PR on my config and it works well.
This PR enables import of Caddyfile, which can be used to manage secrets.
Hope this can be merged soon.

@aanderse
Copy link
Member Author

Thanks again everyone for your reviews. If anyone wants to take a final look over or kick the tires a bit I would appreciate that. Everyone fine with merging in a few days?

@aanderse aanderse merged commit baa0e61 into NixOS:master Dec 25, 2021
@aanderse aanderse deleted the nixos/caddy branch December 25, 2021 22:01
@m1cr0man m1cr0man mentioned this pull request Dec 26, 2021
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Caddy: import Error during parsing: File to import not found
7 participants