-
-
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-server: init module #247118
Conversation
d71a278
to
e06c8c0
Compare
|
||
rm -rf ${stateDir}/web-ui | ||
mkdir -p ${stateDir}/web-ui | ||
cp -R ${pkgs.netbird-dashboard}/* ${stateDir}/web-ui |
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.
Do you have to consistently recreate it or could something like rsync with appropriate flags could update the new files?
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.
(explain why this is needed also)
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 needed because some configuration values, e.g. the auth endpoint are hardcoded in the web-ui and replaced from env varibles with envsubst. I suppose that we could also pass those variables to the dashboard package and set them at build time.
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 quite unfortunate that they require envsubst instead of accessing the environment variables directly... I don't like copying each time but it seems like this is currently necessary - unless upstream changes the process.
On that note: In netbirdio/dashboard#242 one of the maintainers (?) noted that they would accept contributions to make this process easier to host without docker. I imagine that a simple PR to upstream that replaces this envsubst by just accessing process.env.SomeVar
in their config.ts would solve this for us and for them.
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 mean, we can also do it ourselves with substituteAll while building the dashboard, but we need to decide if we want to rebuild the dashboard as a whole every time an option change, which would take some time
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.
IMO an upstream solution seems to be aligned with their goals anyway, so only if they refuse I would start resorting to alternatives.
}: | ||
|
||
buildNpmPackage rec { | ||
pname = "netbird-dashboard"; |
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.
How close is the dashboard the original package? Should it be a passthru derivation of pkgs.netbird
?
i.e. pkgs.netbird.dashboard
?
e06c8c0
to
c0df691
Compare
managementFile = managementFormat.generate "config.json" cfg.managementConfig; | ||
in | ||
{ | ||
meta.maintainers = with maintainers; [ thubrecht ]; |
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 should be a meta.doc
, explaining how to actually use this module. There are a lot of options and it's not clear what a working "production"-like configuration should look like. A NixOS test is also desirable.
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 bit of documentation and also reduced the amount of settings that the end user has to provide by default. I don't have the time right now to write a complicated test, as I believe we need to setup multiple nodes, one being an sso and try adding nodes to the network in the test and until the start of september I will be a bit busy.
c0df691
to
d58d17f
Compare
ea1030f
to
c1b4607
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.
Could you add a nixos tests that verifies that this is working as intended?
|
||
rm -rf ${stateDir}/web-ui | ||
mkdir -p ${stateDir}/web-ui | ||
cp -R ${pkgs.netbird-dashboard}/* ${stateDir}/web-ui |
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 quite unfortunate that they require envsubst instead of accessing the environment variables directly... I don't like copying each time but it seems like this is currently necessary - unless upstream changes the process.
On that note: In netbirdio/dashboard#242 one of the maintainers (?) noted that they would accept contributions to make this process easier to host without docker. I imagine that a simple PR to upstream that replaces this envsubst by just accessing process.env.SomeVar
in their config.ts would solve this for us and for them.
42972c0
to
f4d6e3c
Compare
I just stumbled upon this after being 90% done with setting this up for myself. services.coturn = mkIf cfg.createCoturn {
enable = true;
lt-cred-mech = true;
no-cli = true;
extraConfig = ''
fingerprint
no-software-attribute
external-ip=${domain}
'';
};
systemd.services.coturn.serviceConfig = mkIf cfg.createCoturn {
EnvironmentFile = "/run/secrets/netbird";
ExecStart = lib.mkForce ''
${pkgs.coturn}/bin/turnserver -c /run/coturn/turnserver.cfg --user="''${COTURN_USER}:''${COTURN_PASSWORD}"
'';
}; Also, the createNginx option could optionally accept a |
RuntimeDirectory = "netbird-mgmt"; | ||
StateDirectory = "netbird-mgmt"; | ||
WorkingDirectory = stateDir; | ||
EnvironmentFile = [ settingsFile ]; |
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.
Additionally there could be a user defined secrets file here. Is a bit more convenient to have all the variables in one file
extraConfig = '' | ||
fingerprint | ||
|
||
user=${settings.TURN_USER}:${builtins.toString settings.TURN_PASSWORD} |
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.
As I said earlier, it is possible to pass this via the command like. Also the config should probably set external-ip
to NETBIRD_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.
external-ip
expects an ip address, which is not what NETBIRD_DOMAIN
contains
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.
Still works as expected though. I tested this with coturn behind two layers of NAT
f4d6e3c
to
fde2bbc
Compare
For comparison, I had to vendor this module and adapt it to fit my needs like this: https://github.com/jvanbruegge/server-config/blob/master/modules/netbird-server.nix In the nginx config, only the webserver directive is used, the other two grpc endpoints are already handled by HAProxy |
I took the module and reworked it a bit as well as updated everything to the newest versions:
There are still a few thing missing in this draft most notably:
Notable things I've changedSplit modulesas far as I see there is no reason that the dashboard and the server need to be on the same host. dashboard templatingI've changed the dashboard templating to use a derivation so it's only done once at build time and not on every restart. Used RFC 42 style settingsInstead of this weird back and forth from Environment to templated json I've directly used json. I think this enables user to better configure the server to their need but it also forces us to always keep the defaults updated and might make configuring harder as the json values are pretty undocumented. OIDC autoconfigTurns out netbird is able to automatically get the oidc config from the AutoConfigEndpoint. It does so automatically and overwrites any values set in the config so we can just skip those. Things I'm still unsure aboutsecret managementSome of those secrets are not a secret at all. Namely, the dashboard client secret is templated into the statically served frontend and thus open for anyone who can reach the webpage. For this reason I would oppose exposing this secret as a IDP managementnetbird has an IDP manager which can be used to read additional information from the IDP in a way that standart oidc does not support. But this only work if you use one of their supported IDPs, which I do not so again I'm unable to check if my config works for this case of if we need additional things. device AuthorizationSame thing my IDP does no provide any device authorization so I can't test my config. Nginx setupSo far I've only exposed the nginx config for serving the frontend. You also may need additional config if you use nginx as a reverse proxy to correctly route all the other parts, management and signal. But as I don't want to assume people use nginx as a reverse proxy I think it would be better to just document what endpoints need to be reachable and let people set it up themselves. |
7e67019
to
78c4742
Compare
So, I rewrote the whole thing, thanks a lot for your work @PatrickDaG Split modulesEach part is now activable independently :
This split configuration also allows to setup nginx only for the parts that we want @jvanbruegge, the use of certificates for the coturn server is also independent now. There is also the option to enable everything at once with SecretsTo use secrets in the management config, there is nifty function https://github.com/NixOS/nixpkgs/blob/master/nixos/lib/utils.nix#L174 so we can either pass a value directly as a string that will end up in the store, or we can instead pass I added the option to pass the turn user password as a secret as well, using the same mechanism that the coturn module uses for the auth-secret |
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.
Thanks for the work. Looks really good.
I've found some things I feel like we could improve. If you don't feel like it, I can also do it in the next week or so.
96e516a
to
7affa8c
Compare
@Tom-Hubrecht What's your status for this? Do you still have time and motivation to work on it? If not, I can offer to help out to get this merged. |
I'll have time friday to address your comments, and then we should be good to go |
7affa8c
to
9be4f0d
Compare
Looks great. Thanks for all the work. |
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 excellent work, almost idiomatic.
There's only one slight problem, it's the usage of the enableNginx
pattern.
AFAIK the good way to do this is to provide a nginx
subconfiguration fragment which reuse the NGINX types (look in other modules how they do it) and once you do something like services.netbird.dashboard.nginx.enable = true;
or if the subfragment is non-null, you make use of NGINX.
This way, we can easily separate webservers implementation from the rest.
I do not deem this a blocker for this enormous PR and this is future work for anyone who care about other webservers and this ecosystem.
9be4f0d
to
061bdea
Compare
061bdea
to
6e480a8
Compare
Wow, thank you to all involved :) going to give this try 👍🏻 |
@Tom-Hubrecht I'm currently using this module and I'm curious why a Coturn submodule was added when a Coturn module already existed in NixOS? If I'm already using the other one should I point to that instead or should I run an instance from each module? |
Because the current module does not allow specifying secrets so I had to create an overlay, under the hood it uses |
Description of changes
Adds a package for netbird dashboard and creates a module for self-hosting the netbird management server.
Things done
sandbox = true
set innix.conf
? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)