Skip to content
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/tailscale: use upstream systemd service config. #102202

Merged
merged 1 commit into from Nov 6, 2020

Conversation

@danderson
Copy link
Contributor

@danderson danderson commented Oct 31, 2020

Signed-off-by: David Anderson dave@natulte.net

Motivation for this change

Importing systemd unit change from upstream.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Member

@aanderse aanderse left a comment

Can we just use the upstream unit directly instead? Better than playing catch-up.

@danderson
Copy link
Contributor Author

@danderson danderson commented Oct 31, 2020

I don't think so? The unit generated by NixOS modules is way different than the upstream one (e.g. upstream assumes /usr/sbin/tailscaled as the path).

Do you have an example of a module that reuses an upstream service config that I can pull from?

@aanderse
Copy link
Member

@aanderse aanderse commented Nov 1, 2020

Do you have an example of a module that reuses an upstream service config that I can pull from?

Certainly. I recently switched dnsdist over to use the upstream service.

A couple points to note:

  • there is a bug in NixOS currently so you'll have to include wantedBy = [ "multi-user.target" ]; in your module
  • tailscale upstream should better package their software to not include hard coded paths, but instead utilize the install prefix from the build system
  • until upstream has corrected the hard coded path it is easy enough to override ExecStart - just be sure to include an empty element in the list to erase all previous entries (see systemd docs on ExecStart if you're not familiar) like so: ExecStart = [ ”” "${pkgs.tailscale}/bin/tailscale --state=/var/lib/tailscale/tailscaled.state --socket=/run/tailscale/tailscaled.sock --port $PORT $FLAGS ] (and make sure you set systemd.services.tailscale.environment.PORT = toString cfg.port;)
  • if you want to improve the systemd unit the best place to do that is upstream (though doing it in the module can be acceptable as well)

It looks relatively straight forward to use the upstream unit, but if you run into any issues I would be more than happy to help.

@danderson
Copy link
Contributor Author

@danderson danderson commented Nov 1, 2020

I'm also the maintainer of the upstream unit, so using it as close to as-is as possible SGTM. We don't have a build system upstream that handles install prefixes though, the systemd unit is just what we use to generate .deb and .rpm packages. That said, I can adjust things in the tailscale derivation rather than the nixos module, so that the module doesn't need any further tweaks. I'll update this PR with fixes to the package and module, thanks for the guidance!

@danderson danderson marked this pull request as draft Nov 1, 2020
@danderson danderson force-pushed the danderson/post-stop branch from 72786e8 to 41b21c8 Nov 1, 2020
@danderson danderson changed the title nixos/tailscale: add post-stop cleanup operation. nixos/tailscale: use upstream systemd service config. Nov 1, 2020
@danderson danderson force-pushed the danderson/post-stop branch 2 times, most recently from 9011094 to 3df257d Nov 1, 2020
@danderson danderson marked this pull request as ready for review Nov 1, 2020
@danderson
Copy link
Contributor Author

@danderson danderson commented Nov 1, 2020

@aanderse Ready for another look. The package now includes the systemd unit (with correct paths and removed impure /etc reference), and the module is the minimum deviation from that to make the module parameter work. Tested successfully on my own NixOS systems.

Signed-off-by: David Anderson <dave@natulte.net>
@danderson danderson force-pushed the danderson/post-stop branch from 3df257d to 503caab Nov 4, 2020
@aanderse
Copy link
Member

@aanderse aanderse commented Nov 6, 2020

Thanks @danderson 🎉

@aanderse aanderse merged commit 33d8766 into NixOS:master Nov 6, 2020
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants