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.

Sign up for GitHub

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

Jump to bottom

nixos/navidrome: init module and test #106008

Closed
wants to merge 2 commits into from
Closed

nixos/navidrome: init module and test #106008

wants to merge 2 commits into from

Conversation

nyanloutre
Copy link
Member

@nyanloutre nyanloutre commented Dec 5, 2020 •
edited

Co-authored-by: aciceri andrea.ciceri@autistici.org

Motivation for this change

Continuing work from #92621

Added systemd security options

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.

aanderse
aanderse reviewed Dec 6, 2020
View reviewed changes
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.

I've left some comments I hope you find useful. Let me know if you have any questions. Thanks for contributing!

nixos/modules/services/audio/navidrome.nix Show resolved Hide resolved
nixos/modules/services/audio/navidrome.nix Outdated Show resolved Hide resolved
nixos/modules/services/audio/navidrome.nix Outdated Show resolved Hide resolved
@nyanloutre @aciceri
nixos/navidrome: init module and test
187cf64 
Co-authored-by: aciceri <andrea.ciceri@autistici.org>
aanderse
aanderse reviewed Dec 6, 2020
View reviewed changes
pkgs/servers/misc/navidrome/default.nix
@@ -20,13 +26,17 @@ stdenv.mkDerivation rec {
installPhase = ''
mkdir -p $out/bin
cp navidrome $out/bin
mkdir -p $out/lib/systemd/system
Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. We don't compile from source. Presumably because of node.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like it. Unfortunately with this only x86_64 is supported.

Copy link
Member

Choose a reason for hiding this comment

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

I think I've seen someone build from source and use the binary release just for the node assets before 🤔 Sure, that is a bit hacky, but might be worth investigating in a future PR.

aanderse
aanderse reviewed Dec 8, 2020
View reviewed changes
nixos/modules/services/audio/navidrome.nix
BindReadOnlyPaths = [
builtins.storeDir
] ++ lib.optional (cfg.settings ? MusicFolder) cfg.settings.MusicFolder;
CapabilityBoundingSet = "";
Copy link
Member

Choose a reason for hiding this comment

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

No longer required, provided by upstream unit. And others.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can't find it and my pull request has not been merge

Copy link
Contributor

Choose a reason for hiding this comment

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

Upstream does indeed not have this yet.
And the mentioned PR which adds it (navidrome/navidrome#677) has not been merged yet.

@prusnak prusnak removed their request for review December 10, 2020 12:50
@aanderse
Copy link
Member

ping @nyanloutre on status of this PR.

@ambroisie
Copy link
Contributor

Hey, I am interested in having this module merged. Apart from the merge conflict, is there anything else that needs to be done?

I do not know if @nyanloutre wanted to add more to it, but I could lend a hand if I can.

@DeeUnderscore
Copy link
Contributor

I've been running this for a while. Without upstream navidrome/navidrome#677 getting merged, you need to override AssertPathExists in the systemd unit to make this PR work, but otherwise it seems to work.

If you wanna override it, you need something like:

systemd.services.navidrome = {
  unitConfig = {
    AssertPathExists = [ "" ];
  };

  # … rest of the config here
};

so I guess what's to be done with this PR depends on what gets done with the upstream PR?

@ambroisie
Copy link
Contributor

Is there a reason that the user/group under which the service is running cannot be configured? Is it assumed that any needed permission adjustement should be made by adding navidrome to groups as needed?

@Sohalt
Copy link
Contributor

Sohalt commented Mar 4, 2021

@ambroisie The service runs using DynamicUser, i.e. systemd automatically (and nondeterministically) assigns a uid to the service. Where would you need to make permission adjustments? The service automatically makes the settings.MusicFolder directory accessible to navidrome.

@ambroisie
Copy link
Contributor

@Sohalt does that mean that for example, if I have a lidarr service running as lidarr/media (user/group respectively) and make my music folder read/write accessible to the media group, then systemd won't be causing issues regarding access rights?

@deluan
Copy link

deluan commented May 9, 2021

Hey @Sohalt, regarding navidrome/navidrome#677 I don't think using the systemd file from Navidrome's contrib folder is a good idea. Files in that folder are meant to be used as examples, as they are not used by Navidrome itself. The files are not even maintained properly, meaning that if something changes in Navidrome that breaks those files, they are not updated as they are not tested. I'm actually thinking about removing them from the main repo and adding them to the documentation site as what they are: examples.

Thoughts?

@deluan deluan mentioned this pull request May 9, 2021
Merged
@ambroisie
Copy link
Contributor

Ping, what's keeping this from being merged? The systemd unit has been merged upstream, and the unstable channel has the latest version packaged.

@aanderse
Copy link
Member

@deluan thank you for confirming that we shouldn't be using the upstream unit.
@nyanloutre I'm really sorry I steered you down the wrong path here.
@ambroisie looks like we should stop using the upstream unit before this is merged.

Anyone volunteering to finish this PR off?

@ambroisie
Copy link
Contributor

@aanderse I'm ready to work on it if @nyanloutre can't finish it :-)

@ambroisie ambroisie mentioned this pull request Jun 26, 2021
Merged
11 tasks
@ambroisie
Copy link
Contributor

I have rebased and opened a new MR, at #128201.

@aciceri
Copy link
Member

aciceri commented Sep 13, 2021

Duplicate of #128201

@aciceri aciceri marked this as a duplicate of #128201 Sep 13, 2021
@aciceri aciceri closed this Sep 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sohalt Sohalt Sohalt left review comments

@aanderse aanderse aanderse left review comments

@rnhmjoj rnhmjoj Awaiting requested review from rnhmjoj

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@nyanloutre @aanderse @ambroisie @DeeUnderscore @Sohalt @deluan @aciceri @rnhmjoj @SuperSandro2000