-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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/netbird: harden and extend options #287236
base: master
Are you sure you want to change the base?
Conversation
0cf761f
to
4179661
Compare
Signed-off-by: Krzysztof Nazarewski <gpg@kdn.im>
4179661
to
0a1d920
Compare
0a1d920
to
add2bf7
Compare
add2bf7
to
26373ef
Compare
{ config | ||
, lib | ||
, pkgs | ||
, ... |
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.
Reformatting this should not be commited
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.
this is what nixpkgs-fmt
does, as far as i remember the code should be formatted with it?
suffix = mkOption { | ||
type = str; | ||
default = "-${name}"; | ||
description = '' | ||
Suffix to use for this further naming of the service pieces. | ||
''; | ||
}; |
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.
This seems overly complicated, as it is only used for defaults, you should use the name for this
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 is also used for the wrapper, but yeah might make more sense to not drop -wt0
from the default config. The raw "unwrapped" package should work with it anyway.
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 made more sense when there were 3 wrappers
nixos/tests/netbird.nix
Outdated
@@ -12,6 +12,7 @@ import ./make-test-python.nix ({ pkgs, lib, ... }: | |||
}; | |||
}; | |||
|
|||
# TODO: confirm DynamicUser is working if/after netbird server is implemented |
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.
There is no netbird server in this PR
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.
yeah, but it is very much relevant for the test, there is no confirmation the client actually works until we have a server to connect to.
exports = pipe config.environment [ | ||
(mapAttrsToList (name: default: | ||
# Couldn't default with `''${${name}:${escapeShellArg default}}` | ||
# because `escapeShellArg` wraps values in single quotes, | ||
# which in turn works differenty (or rather doesn't) than double-quotes | ||
# in this shell parameter expansion | ||
''test "''${${name}+X}" == X || export ${toShellVar name default}'')) | ||
(concatStringsSep "\n") | ||
]; | ||
in | ||
pkgs.writeShellScriptBin "netbird${config.suffix}" '' | ||
${exports} | ||
${lib.getExe' cfg.package "netbird"} "$@" | ||
''; |
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.
This is very weird, nixpkgs has a machinery already to wrap programs c.f. https://nixos.org/manual/nixpkgs/stable/#fun-wrapProgram
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 pretty sure it would be significantly longer with wrapProgram
, something like:
mkDerivation {
pname = "xxx";
installPhase =
let
exports = pipe config.environment [
(mapAttrsToList (name: default:
"--set-default '${name}' '${default}'"))
(concatStringsSep " ")
];
in
"wrapProgram ${lib.getExe' cfg.package "netbird"} $out/bin/netbird${config.suffix} ${exports}";
}
actually makes more sense after prototyping it and seeing with own eyes
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.
You can do something like
stdenv.mkDerivation {
intallPhase = ''
wrapProgram ${lib.getExe cfg.package} $out/bin/netbird${config.suffix} ${
concatMapStringsSep " " ({ name, value }: "--set-default '${name}' '${default}'") (
attrsToList config.environment
)
}
'';
The issue I had was not because of the size of the code, but reinventing the wheel ^^
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.
not so short anymore:
let
makeWrapperArgs = flatten (mapAttrsToList
(key: value: [ "--set-default" key value ])
config.environment
);
in
pkgs.stdenv.mkDerivation {
name = "${cfg.package.name}-wrapper-${name}";
meta.mainProgram = config.name;
nativeBuildInputs = with pkgs; [ makeWrapper ];
phases = [ "installPhase" ];
installPhase = ''
mkdir -p "$out/bin"
makeWrapper ${lib.getExe cfg.package} "$out/bin/${config.name}" \
${escapeShellArgs makeWrapperArgs}
'';
}
|
||
A single daemon is fully operational. | ||
|
||
Multiple daemons will interfere with each other's DNS resolution, but otherwise should remain operational. |
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.
When the networks have different domains, there is no interference, the only issue is when then share the same domain
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 don't think the domain is configurable at all on the Netbird server side?
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 is when you self-host your server
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.
FYI:
Hello[@nazarewk, we plan a custom DNS zone support coming next quarter. That would allow you to create custom domains for applications and peers
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.
Custom DNS zone support is already implemented for self-hosted,
${pkgs.netbird}/bin/netbird-mgmt management \
--dns-domain dns.domain
Will place every machine under dns.domain subdomain: machine1.dns.domain
, machine2.dns.domain
a734f1e
to
35a7c67
Compare
name = mkOption { | ||
type = str; | ||
default = name; | ||
apply = value: "netbird-${value}"; |
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 think it would be better style to force the prefix in the relevant locations, otherwise /var/lib/${cfg.name}
will always look problematic since it implies any name is possible.
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.
sounds reasonable
de1630c
to
f3047d3
Compare
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 think you'll have to adjust the release notes to apply to 24.11 now that 24.05 is out the door. But the important details look fine to me, so this is good to go from my side. We can iron out any possible details separately if necessary.
7e67c54
to
89b5ecf
Compare
89b5ecf
to
e8588b2
Compare
e8588b2
to
5b5f809
Compare
There are still changes to |
Yeah, the nixos manual command refused to pass, assumed it was expecting me to change it? |
I guess it complains because you cannot edit old release notes |
it refused to pass without changes https://github.com/NixOS/nixpkgs/actions/runs/9399057023/job/25885722356 |
5b5f809
to
8356170
Compare
8356170
to
79b769d
Compare
79b769d
to
2de52c1
Compare
2de52c1
to
72e61ff
Compare
6947525
to
41ea15c
Compare
41ea15c
to
5681331
Compare
5681331
to
b11a6de
Compare
b11a6de
to
43d2daf
Compare
Description of changes
I have recently extensively tested and fixed all features of Netbird in my own implementation of multi-instance Netbird installations.
While doing so I discovered another multi-instance implementation got merged into nixpkgs #246055 which is slightly different, but still a solid base to upstream the rest of my changes:
DynamicUserit's own user with minimal set of permissionsopenFirewall
by defaultI think it's a pretty good time to upstream, because I will be extensively using it at work: just launched my first Colmena-managed NixOS into GCE.
There are plans to support multi-account connections on the same daemon in Q2/2024 (see the slack message), but it's not known what shape it will take at all.
I decided to implement following significant changes:
tunnels
toclients
, because a wordtunnel
does not exist in Netbird's nomenclature (unlike some other VPNs) and is pretty misleading. Alsoclients.*
play nicely with my plan to implement aserver
in near future.{name, ...}: name
->client: client.name
) because they make the code very hard to follow and update with increased number of options,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.