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

Choose a reason for hiding this comment

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

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

@danderson
Copy link
Contributor Author

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 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

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 November 1, 2020 01:06
@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 Compare November 1, 2020 05:38
@danderson danderson marked this pull request as ready for review November 1, 2020 05:39
@danderson
Copy link
Contributor Author

@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>
@aanderse
Copy link
Member

aanderse commented Nov 6, 2020

Thanks @danderson 🎉

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

Successfully merging this pull request may close these issues.

None yet

3 participants