-
-
Notifications
You must be signed in to change notification settings - Fork 14.9k
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 (take 2) #97217
Conversation
- Update configuration syntax - Add filalex77 as a maintainer
Rename legacy v1 to `caddy1`
e6c4b64
to
f250296
Compare
Thanks @sephii, I fully support your effort. I'll take a look at the tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides failing tests, LGTM
@ofborg test caddy |
From what I can tell, the issue is that Nix doesn't recognize paths in a configuration file as being inputs. {
"apps": {
"http": {
"servers": {
"srv0": {
"automatic_https": {
"skip": [
"localhost"
]
},
"listen": [
":80"
],
"routes": [
{
"handle": [
{
"handler": "subroute",
"routes": [
{
"handle": [
{
"handler": "vars",
"root": "/nix/store/496k1zx0li2mp94xnydn2smk1gbybw2n-testdir"
},
{
"encodings": {
"gzip": {}
},
"handler": "encode"
},
{
"handler": "file_server",
"hide": [
"/nix/store/69yh559dagwrhfa8bahpm9v99sd2qkg8-Caddyfile"
]
}
]
}
]
}
],
"match": [
{
"host": [
"localhost"
]
}
],
"terminal": true
}
]
}
}
},
"tls": {
"automation": {
"policies": [
{
"issuer": {
"ca": "https://acme-v02.api.letsencrypt.org/directory",
"email": "",
"module": "acme"
}
}
]
}
}
}
} Neither This likely occurs due to adaptedConfig = importJSON (pkgs.runCommand "caddy-config-adapted.json" { } ''
${cfg.package}/bin/caddy adapt \
--config ${configFile} --adapter ${cfg.adapter} > $out
'');
configJSON = pkgs.writeText "caddy-config.json" (builtins.toJSON
(recursiveUpdate adaptedConfig tlsConfig)); |
}; | ||
}]; | ||
}; | ||
adaptedConfig = importJSON (pkgs.runCommand "caddy-config-adapted.json" { } '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid use of importJSON
here and merge JSON files in a derivation. The following should work.
adaptedConfig = pkgs.runCommand "caddy-config-adapted.json" { } ''
${cfg.package}/bin/caddy adapt \
--config ${configFile} --adapter ${cfg.adapter} > $out
'';
tlsJSON = pkgs.writeText "tls.json" (builtins.toJSON tlsConfig);
configJSON = pkgs.runCommand "caddy-config.json" { } ''
${pkgs.jq}/bin/jq -s '.[0] * .[1]' ${adaptedConfig} ${tlsJSON} > $out
'';
f250296
to
94ed860
Compare
The tests should now pass, thanks for the help! |
@ofborg test caddy |
I don't think this is a regression caused by this PR, but for some reason, it's emitting a StartLimitIntervalSec in the wrong section (should be under [Unit] as of some time in 2017 according to the mailing list). Here's the generated service file: [Unit]
After=network-online.target
Description=Caddy web server
Wants=network-online.target
[Service]
# ... snip ...
StartLimitIntervalSec=14400
TimeoutStopSec=5s
Type=simple
User=caddy
|
You're right, it should be moved to the |
@sephii I should file a PR and fix all of them as it's relatively trivial. ~~Not sure when I'll have time to do that though.~~Well, the following bug is a blocker on my work, so I guess I have time now! |
In terms of actual bugs in this PR though: There's a bug introduced in this PR where we are doing a bit of hamfisted overwriting of the {pkgs, ...}: {
services.caddy.package = pkgs.caddy;
services.caddy.enable = true;
services.caddy.agree = true;
services.caddy.email = "letsencrypt@catgirl.enterprises";
services.caddy.ca = "https://acme-staging-v02.api.letsencrypt.org/directory";
services.caddy.config = ''
test.catgirl.enterprises:443 {
encode zstd gzip
log
tls internal
reverse_proxy localhost:3000
}
'';
} is making the following bad config: {
"apps": {
"http": {
"servers": {
"srv0": {
"listen": [
":443"
],
"routes": [
{
"match": [
{
"host": [
"test.catgirl.enterprises"
]
}
],
"handle": [
{
"handler": "subroute",
"routes": [
{
"handle": [
{
"encodings": {
"gzip": {},
"zstd": {}
},
"handler": "encode"
},
{
"handler": "reverse_proxy",
"upstreams": [
{
"dial": "localhost:3000"
}
]
}
]
}
]
}
],
"terminal": true
}
],
"logs": {}
}
}
},
"tls": {
"automation": {
"policies": [
{
"issuer": {
"ca": "https://acme-staging-v02.api.letsencrypt.org/directory",
"email": "letsencrypt@catgirl.enterprises",
"module": "acme"
}
}
]
}
}
}
} Compare the tls section to what caddy adapt produces for the equivalent caddyfile with the content of {
"apps": {
"http": {
"servers": {
"srv0": {
"listen": [
":443"
],
"routes": [
{
"match": [
{
"host": [
"test.catgirl.enterprises"
]
}
],
"handle": [
{
"handler": "subroute",
"routes": [
{
"handle": [
{
"encodings": {
"gzip": {},
"zstd": {}
},
"handler": "encode"
},
{
"handler": "reverse_proxy",
"upstreams": [
{
"dial": "localhost:3000"
}
]
}
]
}
]
}
],
"terminal": true
}
],
"logs": {}
}
}
},
"tls": {
"automation": {
"policies": [
{
"subjects": [
"test.catgirl.enterprises"
],
"issuer": {
"module": "internal"
}
}
]
}
}
}
} |
Update: it looks like it's that for some reason jq is not merging arrays while doing a |
I think the following jq should do the trick: |
Motivation for this change
The
caddy
module allows you to use Caddy 2 but it's broken (#92992). This PR is a new take at #86686, which has been dormant for a few weeks now. Since I'd really like to get Caddy 2 support in 20.09 I'm opening a new PR with the requested changes.The tests pass when running the tests interactively ((fixed!)nix-build caddy.nix -A driver
and thenstart_all()
followed bytest_script()
), but they fail when running them non-interactively (nix-build caddy.nix
) because the test directory created in the test cannot be found. Since I can't figure out why, any help is appreciated.I would also like to bump the version to 2.1.1 instead of 2.0, but doing it gives me some go errors I don't know how to fix (eg.(fixed!)gopkg.in/yaml.v2@v2.3.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
), so if someone knows how to make it work, it would be nice.Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)