-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
caddy: add suport for compiling Caddy with plugins #358586
base: master
Are you sure you want to change the base?
Conversation
}; | ||
withPlugins = | ||
{ plugins |
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.
Quick question, should we make the build fail if a plugin is provided without the version string?
For example:
pkgs.caddy.withPlugins {
plugins = [ "github.com/caddy-dns/[email protected]" "github.com/caddy-dns/cloudflare" ];
hash = "sha256-AoW35l7QkXunjBzZ43IlyU3UkVXw2D4eyc1jx8xpT0U=";
}
After testing this on my darwin machine, I have the following:
$ /nix/store/icp2z20hpf2ps7g4n5rzqdkg5qsjp38z-caddy-2.8.4/bin/caddy build-info
...
dep github.com/caddy-dns/cloudflare v0.0.0-20240703190432-89f16b99c18e
dep github.com/caddy-dns/powerdns v1.0.1
...
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.
Yes, I think I can add an assertion.
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.
I have added an assertion (using the first non-versioned plugin as an example).
pluginsList = lib.concatMapStrings (plugin: "${plugin}-") pluginsSorted; | ||
pluginsHash = builtins.hashString "sha1" pluginsList; | ||
in stdenv.mkDerivation { | ||
pname = "caddy-src-with-plugins-${pluginsHash}"; |
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.
Small nit, imo pluginsHash
makes the build log a bit too long, maybe caddy-src-with-plugins
is good enough here?
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.
It's there to ensure a cached build is not used when adding/removing a plugin. This was one of the request in #317881 (comment).
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.
I have switched to md5 to reduce the length a bit. We could also use a subset of the hash if it's still too long.
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.
I'm not sure if using md5 is a good idea, IIRC it's considered outdated and insecure (citation needed)?
I took a look into the comment you linked, it seems like it'd be a good idea to include a test to make sure specified plugins are properly installed. I came up with the following but don't have time to dig deeper (feel free to take whatever you need):
diff --git a/pkgs/by-name/ca/caddy/package.nix b/pkgs/by-name/ca/caddy/package.nix
index eea6894ce328..c052da5ef290 100644
--- a/pkgs/by-name/ca/caddy/package.nix
+++ b/pkgs/by-name/ca/caddy/package.nix
@@ -116,6 +116,31 @@ buildGoModule {
outputHash = hash;
outputHashAlgo = "sha256";
};
+
+ doInstallCheck = true;
+ installCheckPhase = ''
+ runHook preInstallCheck
+
+ build_info="$($out/bin/caddy build-info)"
+
+ for plugin in ''${plugins[@]}; do
+ # this won't work :(
+ echo $plugin
+ url=$(echo "$plugin" | cut -d'@' -f1)
+ version=$(echo "$plugin" | cut -d'@' -f2)
+ echo $url
+ echo $version
+
+ if echo "$build_info" | grep -q "$url[[:space:]]*$version"; then
+ echo "$plugin found in build-info"
+ else
+ echo "$plugin not found in build-info" >&2
+ exit 1
+ fi
+ done
+
+ runHook postInstallCheck
+ '';
});
};
For testing:
nom-build --expr 'with import ./. { }; caddy.withPlugins { plugins = [ "github.com/caddy-dns/[email protected]" "github.com/caddy-dns/[email protected]" ]; hash = "sha256-AoW35l7QkXunjBzZ43IlyU3UkVXw2D4eyc1jx8xpT0U="; }'
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.
I have added a postInstallCheck as you suggest.
As for the MD5, this is not meant to be secure, just a safety to ensure the user does not forget to update the hash when modifying plugins. A mechanism like this was requested in the original PR.
6068ba7
to
72a4b4b
Compare
This adds a `withPlugins` function to Caddy package. ```nix services.caddy = { enable = true; package = pkgs.caddy.withPlugins { plugins = [ "github.com/caddy-dns/[email protected]" ]; hash = "sha256-F/jqR4iEsklJFycTjSaW8B/V3iTGqqGOzwYBUXxRKrc="; }; }; ```
72a4b4b
to
2144da1
Compare
This adds a
withPlugins
function to Caddy package.Fix: #14671
This is an alternative to #317881 and it relies on xcaddy. Looking at #317881 (comment), I am still missing tests. I am unsure if this is a build test (in
checkPhase
) or a NixOS test. The remaining requirements should be OK (notably use of xcaddy and FOD).The release notes are missing, I'll add them once this gets a chance to be accepted.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.