Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions nixos/doc/manual/release-notes/rl-2505.section.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,21 @@

- `bind.cacheNetworks` now only controls access for recursive queries, where it previously controlled access for all queries.

- Caddy can now be built with plugins by using `caddy.withPlugins`, a `passthru` function that accepts an attribute set as a parameter. The `plugins` argument represents a list of Caddy plugins, with each Caddy plugin being a versioned module. The `hash` argument represents the `vendorHash` of the resulting Caddy source code with the plugins added.

Example:
```nix
services.caddy = {
enable = true;
package = pkgs.caddy.withPlugins {
plugins = [ "github.com/caddy-dns/powerdns@v1.0.1" ];
hash = "sha256-F/jqR4iEsklJFycTjSaW8B/V3iTGqqGOzwYBUXxRKrc=";
};
};
```

To get the necessary hash of the vendored dependencies, omit `hash`. The build will fail and tell you the correct value.

- `programs.fzf.keybindings` now supports the fish shell.

<!-- To avoid merge conflicts, consider adding your item at an arbitrary place in the list instead. -->
Expand Down
88 changes: 82 additions & 6 deletions pkgs/by-name/ca/caddy/package.nix
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
, testers
, installShellFiles
, stdenv
, go
, xcaddy
, cacert
, git
}:
let
version = "2.8.4";
Expand All @@ -32,7 +36,8 @@ buildGoModule {
subPackages = [ "cmd/caddy" ];

ldflags = [
"-s" "-w"
"-s"
"-w"
"-X github.com/caddyserver/caddy/v2.CustomVersion=${version}"
];

Expand Down Expand Up @@ -61,12 +66,83 @@ buildGoModule {
--zsh <($out/bin/caddy completion zsh)
'';

passthru.tests = {
inherit (nixosTests) caddy;
version = testers.testVersion {
command = "${caddy}/bin/caddy version";
package = caddy;
passthru = {
tests = {
inherit (nixosTests) caddy;
version = testers.testVersion {
command = "${caddy}/bin/caddy version";
package = caddy;
};
};
withPlugins =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but I think this is very long function to be defined at the body of the derivation. I suggest moving this to with-plugins.nix or something and importing the file here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will not block PR for my nit, so if someone else wants to merge feel free.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, moved to plugins.nix and use of callPackage. I think this is the right way, but maybe I should inherit more stuff?

{ plugins
Copy link
Member

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/powerdns@v1.0.1" "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	
...

Copy link
Member Author

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.

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 have added an assertion (using the first non-versioned plugin as an example).

, hash ? lib.fakeHash
}: caddy.overrideAttrs (finalAttrs: prevAttrs:
let
pluginsSorted = builtins.sort builtins.lessThan plugins;
pluginsList = lib.concatMapStrings (plugin: "${plugin}-") pluginsSorted;
pluginsHash = builtins.hashString "md5" pluginsList;
pluginsWithoutVersion = builtins.filter (p: !lib.hasInfix "@" p) pluginsSorted;
in
assert lib.assertMsg (builtins.length pluginsWithoutVersion == 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Another non-blocking nit, but in general builtins should be avoided because sometimes we have polyfills for older versions of Nix inside lib, so in general we should always prefer using lib.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, except for builtins.hashString as there does not seem there is an equivalent.

"All plugins should have a version (eg ${builtins.elemAt pluginsWithoutVersion 0}@x.y.z)!";
{
vendorHash = null;
subPackages = [ "." ];

src = stdenv.mkDerivation {
pname = "caddy-src-with-plugins-${pluginsHash}";
Copy link
Member

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?

Copy link
Member Author

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

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

Copy link
Member

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/powerdns@v1.0.1" "github.com/caddy-dns/cloudflare@v0.0.0-20240703190432-89f16b99c18e" ]; hash = "sha256-AoW35l7QkXunjBzZ43IlyU3UkVXw2D4eyc1jx8xpT0U="; }'

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

version = finalAttrs.version;

nativeBuildInputs = [
go
xcaddy
cacert
git
];
dontUnpack = true;
buildPhase =
let
withArgs = lib.concatMapStrings (plugin: "--with ${plugin} ") pluginsSorted;
in
''
export GOCACHE=$TMPDIR/go-cache
export GOPATH="$TMPDIR/go"
XCADDY_SKIP_BUILD=1 TMPDIR="$PWD" xcaddy build v${finalAttrs.version} ${withArgs}
(cd buildenv* && go mod vendor)
'';
installPhase = ''
mv buildenv* $out
'';

outputHashMode = "recursive";
outputHash = hash;
outputHashAlgo = "sha256";
};


doInstallCheck = true;
installCheckPhase = ''
runHook preInstallCheck

${lib.toShellVar "notfound" pluginsSorted}
while read kind module version; do
[[ "$kind" = "dep" ]] || continue
module="''${module}@''${version}"
for i in "''${!notfound[@]}"; do
if [[ ''${notfound[i]} = ''${module} ]]; then
unset 'notfound[i]'
fi
done
done < <($out/bin/caddy build-info)
if (( ''${#notfound[@]} )); then
>&2 echo "Plugins not found: ''${notfound[@]}"
Copy link
Contributor

@kusold kusold Dec 4, 2024

Choose a reason for hiding this comment

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

I ran into an issue with this check when plugin versions are specified as git-shas.

When given the input:

    plugins = [
      "github.com/caddy-dns/cloudflare@89f16b99c18ef49c8bb470a82f895bce01cbaece"
      "github.com/dulli/caddy-wol@c0d58507c9037191aa9622d531a001db619dd543"
    ];
    hash = "sha256-pKiL3bGcFVcD7r67mY9RtAEHDJc+gq3vz8DkZYssfb4=";

caddy build-info contains:

...
dep     github.com/caddy-dns/cloudflare v0.0.0-20240703190432-89f16b99c18e
...
dep     github.com/dulli/caddy-wol      v1.0.1-0.20240903185854-c0d58507c903
...

This causes this check to fail because the versions were substituted. If I update my input to the versions, it works:

plugins = [
      "github.com/caddy-dns/cloudflare@v0.0.0-20240703190432-89f16b99c18e"
      "github.com/dulli/caddy-wol@v1.0.1-0.20240903185854-c0d58507c903"
    ];
    hash = "sha256-pKiL3bGcFVcD7r67mY9RtAEHDJc+gq3vz8DkZYssfb4=";

Do you need to check for the version, or is matching the module sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

IMO checking module + version is better, and you can get the format it wants beforehand with go get and check the go.mod file

exit 1
fi

runHook postInstallCheck
'';
});
};

meta = with lib; {
Expand Down