nixos/wireguard: allow setting a readable name for peers #71737
Conversation
|
cc @grahamc for review or merge or reviewer assignment |
| keyToUnitName = replaceChars | ||
| [ "/" "-" " " "+" "=" ] | ||
| [ "-" "\\x2d" "\\x20" "\\x2b" "\\x3d" ]; | ||
| unitName = keyToUnitName peer.publicKey; | ||
| unitName = keyToUnitName name; |
There was a problem hiding this comment.
What happens if two peers are given the same name?
There was a problem hiding this comment.
Testing the PR, it looks like there is no warning or error; the first peer listed with the same name "wins" and gets put into the systemd unit.
There was a problem hiding this comment.
I feel like this calls for a change of the current peers data structure, from "list of sets" to "set of sets", e.g.:
networking.wireguard.interfaces = {
business = {
# […]
peers = {
fileserver = { publicKey = "…"; allowedIPs = ["…"]; };
webserver = { publicKey = "…"; allowedIPs = ["…"]; };
};
};
};
This would be a breaking change for existing configurations, but it seems to me like the only sane way to properly handle the name collision issue based on the current implementation.
On the other hand, instead of having individual service units per peer, the whole networking.wireguard implementation could also simply make use of systemd's native WireGuard support in systemd-networkd, so we'd end up with far less complexity and implementation overhead.
As since systemd 243, systemd-networkd also supports PrivateKeyFile=, this would also allow to take the requirement to keep the private key out of the configuration into account.
And as systemd-networkd simply provides a Description= attribute for each .netdev, the whole thing would work just fine with the existing data structure and potential name collisions, as it wouldn't be used as primary key anymore.
|
ping? |
|
An bonus use of the friendly name introduced by this PR would be the Prometheus exporter for Wireguard, which has a feature to export friendly names if given a config file with names as |
|
What @zopieux said. Merging this would be awesome. How is the status on this? |
|
Basically: it's easy enough to keep as a local patch in my nixpkgs and I don't care enough to go through and make the invasive changes that people have suggested (changing the whole configuration format). I agree the suggested format would be better, but the time I would have to put into it isn't worth it for me. If someone wants to go and take this over and work on the suggested changes, be my guest, and I'll just close this PR in favor of yours :) |
|
I have exams coming up. If there is no progress on this until in ~3 Weeks I will take this over. |
These are used in NixOS/nixpkgs#71737
|
Wouldn't make it more sense to prefix the peer-specific unit name with the interface name, but keep the hash behind that? Also, prefixing the peer-specific units with the wireguard interface name solves the unit file name clash, if we still add a |
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
|
@f4814 three weeks are over 😉 |
stale
bot
removed
the
2.status: stale
|
Yeah, sorry. I have moved to wg-quick, as it fits my usecase much better. So I'm not really interested in fixing this anymore. |
|
Let's close this until someone finds time to finish this up. |
|
an updated version of the rejected patch for nixpkgs master / 21.11 From 0d31b9131b0b29941b93e5c303d18199e988132b Mon Sep 17 00:00:00 2001
From: Pierre Bourdon <delroth@gmail.com>
Date: Wed, 23 Oct 2019 02:56:25 +0200
Subject: [PATCH] nixos/wireguard: allow setting a readable name for peers
---
nixos/modules/services/networking/wireguard.nix | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/nixos/modules/services/networking/wireguard.nix b/nixos/modules/services/networking/wireguard.nix
index 55b84935b6c..1b11fe8e452 100644
--- a/nixos/modules/services/networking/wireguard.nix
+++ b/nixos/modules/services/networking/wireguard.nix
@@ -148,6 +148,12 @@ let
options = {
+ name = mkOption {
+ default = null;
+ type = with types; nullOr str;
+ description = "An optional name for the peer.";
+ };
+
publicKey = mkOption {
example = "xTIBA5rboUvnH4htodjb6e697QjLERt1NAB4mZqp8Dg=";
type = types.str;
@@ -271,18 +277,19 @@ let
'';
};
- peerUnitServiceName = interfaceName: publicKey: dynamicRefreshEnabled:
+ peerUnitServiceName = interfaceName: name: dynamicRefreshEnabled:
let
keyToUnitName = replaceChars
[ "/" "-" " " "+" "=" ]
[ "-" "\\x2d" "\\x20" "\\x2b" "\\x3d" ];
- unitName = keyToUnitName publicKey;
+ unitName = keyToUnitName name;
refreshSuffix = optionalString dynamicRefreshEnabled "-refresh";
in
"wireguard-${interfaceName}-peer-${unitName}${refreshSuffix}";
generatePeerUnit = { interfaceName, interfaceCfg, peer }:
let
+ name = if peer.name != null then peer.name else peer.publicKey;
psk =
if peer.presharedKey != null
then pkgs.writeText "wg-psk" peer.presharedKey
@@ -295,10 +302,10 @@ let
# We generate a different name (a `-refresh` suffix) when `dynamicEndpointRefreshSeconds`
# to avoid that the same service switches `Type` (`oneshot` vs `simple`),
# with the intent to make scripting more obvious.
- serviceName = peerUnitServiceName interfaceName peer.publicKey dynamicRefreshEnabled;
+ serviceName = peerUnitServiceName interfaceName name dynamicRefreshEnabled;
in nameValuePair serviceName
{
- description = "WireGuard Peer - ${interfaceName} - ${peer.publicKey}";
+ description = "WireGuard Peer - ${interfaceName} - ${name}";
requires = [ "wireguard-${interfaceName}.service" ];
after = [ "wireguard-${interfaceName}.service" ];
wantedBy = [ "multi-user.target" "wireguard-${interfaceName}.service" ];
--
2.33.0
|
|
maybe a new PR would be better than a patch on a closed one? |
Motivation for this change
Makes the systemd unit names more easily mappable to what peer they correspond to (I don't memorize peer public keys :-) ).
Things done
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)