Skip to content

coredns: add support for plugins#256710

Merged
bjornfor merged 1 commit intoNixOS:masterfrom
bct:coredns-add-plugins
Sep 27, 2023
Merged

coredns: add support for plugins#256710
bjornfor merged 1 commit intoNixOS:masterfrom
bct:coredns-add-plugins

Conversation

@bct
Copy link
Contributor

@bct bct commented Sep 22, 2023

Description of changes

Solves #146603

Usage:

coredns-fanout = pkgs.coredns.override {
  externalPlugins = [
    {name = "fanout"; repo = "github.com/networkservicemesh/fanout"; version = "v1.9.1";}
  ];
  vendorHash = "<SRI hash>";
};

This PR is based on #205649 . I've made 3 changes:

  • rebased it on master
  • added go mod vendor to the end of modBuildPhase
  • modified the externalPlugins argument to take the plugin name, repo and version
    • this allows plugins to be pinned, which was a request made on the original PR
  • dropped the list of official upstream plugins (external-plugins.nix)
    • this was convenient but not necessary. removing it means it doesn't need to be kept up to date.

Things done

  • 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/)
  • 23.11 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
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog This PR adds or changes release notes labels Sep 22, 2023
Copy link

@de11n de11n left a comment

Choose a reason for hiding this comment

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

Great work! I took a stab at this a while ago and came up with something very similar. I have a few small comments but largely LGTM.

@bct
Copy link
Contributor Author

bct commented Sep 22, 2023

I made the suggested changes.

@ofborg ofborg bot requested review from DeltaEvo, rtreffer and rushmorem September 22, 2023 16:29
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 22, 2023
Copy link

@de11n de11n left a comment

Choose a reason for hiding this comment

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

Small comments. Overall looking great.

Copy link

Choose a reason for hiding this comment

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

Extreme nit: You can build the entire string in Nix and then only echo once. But this is fine and matches the loop below it.

Copy link

@de11n de11n left a comment

Choose a reason for hiding this comment

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

I don't have merge bits, but looks great. You may want to squash your commits to make it easier for whoever does merge it.

@bct bct force-pushed the coredns-add-plugins branch from 4545f41 to c8a014a Compare September 22, 2023 20:09
@bct
Copy link
Contributor Author

bct commented Sep 22, 2023

Thanks! I've squashed it into a single commit.

Copy link
Contributor

@bjornfor bjornfor left a comment

Choose a reason for hiding this comment

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

Commit message typo: plugins tht are -> plugins that are

Solves NixOS#146603

CoreDNS has support for plugins that are added at compile time. This
exposes an argument `externalPlugins` that will build coredns with
the specified plugins.

Example:
```
coredns-fanout = pkgs.coredns.override {
  externalPlugins = [
    {name = "fanout"; repo = "github.com/networkservicemesh/fanout"; version = "v1.9.1";}
  ];
  vendorHash = "<SRI hash>";
};
```
@bct bct force-pushed the coredns-add-plugins branch from c8a014a to 37eb1c9 Compare September 22, 2023 21:30
Copy link
Contributor

@KFearsoff KFearsoff left a comment

Choose a reason for hiding this comment

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

LGTM

Regarding the "newer CoreDNS rebuilds with older vendor": do we have a precedent for handling something like this in Nixpkgs? I don't believe we do, and there's no easy way to handle it, right?

@bjornfor
Copy link
Contributor

When coredns gets updated, and I use externalPlugins, does the build continue with stale vendored code, or does it fail?

@bct
Copy link
Contributor Author

bct commented Sep 24, 2023

My understanding (based on trying the 1.10.1 -> 1.11.0 upgrade and reading https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/go/module.nix ):

The package version is part of the -go-modules derivation name. When the version changes goModules will be rebuilt. If the vendored dependencies have changed, then the vendorHash won't match and the build will fail. If the vendored dependencies haven't changed then it will build successfully.

I was worried that it might be possible for a new version to build successfully against old modules if you had a cached goModules, but I don't think this can happen since the version is part of the derivation name.

@bjornfor
Copy link
Contributor

The package version is part of the -go-modules derivation name. When the version changes goModules will be rebuilt. If the vendored dependencies have changed, then the vendorHash won't match and the build will fail. If the vendored dependencies haven't changed then it will build successfully.

Perfect. Thanks!

@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Sep 25, 2023
@delroth delroth added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Sep 26, 2023
@bjornfor bjornfor merged commit 95e6680 into NixOS:master Sep 27, 2023
@bjornfor
Copy link
Contributor

Thanks!

@evrim
Copy link
Contributor

evrim commented Sep 29, 2023

sry to barge in like this but coredns is failing in staging-next with the following, mite be related.

Below is the build log of derivation /nix/store/fy12vfg6j6v86j997is0chhnypis75c9-coredns-1.11.0.drv. It was built on ssh://localhost.

unpacking sources
unpacking source archive /nix/store/9g5sy1favi7bawr3ynjdid2ssj0hnwv3-source
source root is source
patching sources
updateAutotoolsGnuConfigScriptsPhase
configuring
building
Building subPackage .
# github.com/quic-go/quic-go/internal/handshake
vendor/github.com/quic-go/quic-go/internal/handshake/crypto_setup.go:362:37: cannot use h.allow0RTT (variable of type bool) as tls.QUICSessionTicketOptions value in argument to h.conn.SendSessionTicket

Tried to upgrade to 1.11.1 but no luck.

@bct
Copy link
Contributor Author

bct commented Sep 29, 2023

This looks like the same issue: quic-go/quic-go#4021

In that case the issue is that quic-go was incompatible with go 1.21. I see that the default version of go was bumped to 1.21 3 days ago: #254878

@evrim
Copy link
Contributor

evrim commented Sep 29, 2023

So, what mite be the correct approach here; we can just do buildGo120Module to build and it will succeed I guess. How about other options?

@bct
Copy link
Contributor Author

bct commented Sep 29, 2023

coredns 1.11.1 is supposed to have the fix (coredns/coredns#6252). it builds successfully for me on staging-next:

diff --git a/pkgs/servers/dns/coredns/default.nix b/pkgs/servers/dns/coredns/default.nix
index 2dcfc538be45..8078018625a2 100644
--- a/pkgs/servers/dns/coredns/default.nix
+++ b/pkgs/servers/dns/coredns/default.nix
@@ -4,7 +4,7 @@
 , fetchFromGitHub
 , installShellFiles
 , externalPlugins ? []
-, vendorHash ? "sha256-TvIswNQ7DL/MtYmMSxXf+VqKHcmzZVZwohOCvRWxBkY="
+, vendorHash ? "sha256-tp22jj6DNnYFQhtAFW2uLo10ty//dyNqIDH2egDgbOw="
 }:

 let
@@ -14,13 +14,13 @@ let
     builtins.map ({name, repo, version}: "${repo}@${version}") attrs;
 in buildGoModule rec {
   pname = "coredns";
-  version = "1.11.0";
+  version = "1.11.1";

   src = fetchFromGitHub {
     owner = "coredns";
     repo = "coredns";
     rev = "v${version}";
-    sha256 = "sha256-Mn8hOsODTlnl6PJaevMcyIKkIx/1Lk2HGA7fSSizR20=";
+    sha256 = "sha256-XZoRN907PXNKV2iMn51H/lt8yPxhPupNfJ49Pymdm9Y=";
   };

   inherit vendorHash;

I can put up a PR for this tonight.

@evrim
Copy link
Contributor

evrim commented Sep 29, 2023

Oh that's great, thnx!

@bct
Copy link
Contributor Author

bct commented Sep 30, 2023

#258136

@jpds jpds mentioned this pull request Oct 3, 2023
bct added a commit to bct/nix-config that referenced this pull request Nov 2, 2023
support for plugins was merged upstream
NixOS/nixpkgs#256710
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants