avahi: set service directory and refactor module #62153

WilliButz · 2019-05-28T12:20:15Z

Re-opened PR against staging, includes changes proposed in #61945.

Motivation for this change

Before this PR avahi used to look for service definitions in etc/avahi/services in its store path.
First this leads to the situation mentioned in #60939 and it also makes adding custom services quite user-unfriendly, because they would have to be added to the build output of avahi.

Things done

Package:

For the build AVAHI_SERVICE_DIR is now set to /etc/avahi/services.

Module:

Types are now specified for all options.
The fixed uid and gid for the avahi user have been removed
and the avahi user is now in the avahi group.
The the generic opening of the firewall for UDP port 5353 is
now optional, but still defaults to true.

The option extraServiceFiles was added to specify avahi
service definitions, which are then placed in /etc/avahi/services.

Manual:

Added a section mentioning the changes for avahi.

fixes #60939

/cc @infinisil

WilliButz · 2019-05-28T12:23:31Z

@GrahamcOfBorg test avahi

peterhoeg · 2019-05-30T09:44:46Z

Good initiative!

Couple of things:

a. avahi upstream provides a systemd unit file - we don't need our own service def (except for the activation)
b. Why are we removing the static uid/gid?

WilliButz · 2019-05-30T12:08:42Z

@peterhoeg

a. avahi upstream provides a systemd unit file - we don't need our own service def (except for the activation)

Regarding avahi-daemon.socket the unit file shipped with avahi is not different from the one in nixpkgs.
Our definition for avahi-daemon.service has a small diff compared to upstream:

we do not specify ExecReload, which I think makes sense, because we do not have our avahi config file in the build output of avahi
we specify the extra path entries and LD_LIBRARY_PATH
we don't speficy the unit's Alias

As I am not quite sure what you meant, could you maybe elaborate on why we don't need our own service definition?

b. Why are we removing the static uid/gid?

The service doesn't handle a lot of files and the ones that are handled by avahi don't rely on a static uid. Also, as the user was not even in the avahi group until now, the gid was never used - instead the group nogroup was.

Maybe you thought of a reason to keep the static uid?

peterhoeg · 2019-05-31T09:52:17Z

a. avahi upstream provides a systemd unit file - we don't need our own service def (except for the activation)

As I am not quite sure what you meant, could you maybe elaborate on why we don't need our own service definition?

We can get the unit files from upstream avahi by doing this:

systemd.packages = with pkgs; [ avahi ];

And then simply override the bits that we want to change and leave the rest as-is.

Maybe you thought of a reason to keep the static uid?

The problem is that the uid could (would) be something else on another system which isn't great. We either want to keep it the same across the board (hence the ids.nix) or ideally use DynamicUser = true; if supported to have systemd handle it for us.

fpletz · 2019-05-31T13:55:52Z

The problem is that the uid could (would) be something else on another system which isn't great.

The avahi service does not manage any persistent state so it shouldn't really matter. Why are different uids a problem?

ideally use DynamicUser = true; if supported to have systemd handle it for us.

Avahi drops the privileges itself to a user with a username that is compiled in. It has to be run as root and the user has to exist.

Avahi does support running as non-root but the code still expects the hardcoded username to exist. That's why we can't switch to DynamicUser. Fixing avahi is out of scope for this PR though.

a. avahi upstream provides a systemd unit file - we don't need our own service def (except for the activation)

The upstream systemd service unit is pretty basic and in that case I actually prefer defining the whole service in the NixOS module. When avahi will eventually support DynamicUser or other systemd shenanigans we have to touch our service definition anyway.

fpletz · 2019-05-31T13:58:36Z

@worldofpeace Do you have any remarks since you requested self-review?

nixos/modules/services/networking/avahi-daemon.nix

elseym

just a few typos :)

nixos/modules/services/networking/avahi-daemon.nix

WilliButz · 2019-05-31T18:31:30Z

I added the suggested changes and rebased on staging.

worldofpeace · 2019-05-31T23:32:16Z

@worldofpeace Do you have any remarks since you requested self-review?

Yes, it was in queue to review. I have the time now so it's incoming shortly if it's still needed.

worldofpeace

Just some more cleanup should be done to the options.
Feedback on the release note also.

nixos/doc/manual/release-notes/rl-1909.xml

nixos/modules/services/networking/avahi-daemon.nix

worldofpeace · 2019-05-31T23:47:45Z

nixos/modules/services/networking/avahi-daemon.nix

-        description = ''
-          Extra config to append to avahi-daemon.conf.
-        '';
+      domain = mkOption {


nitpick: this option's name is pretty ambiguous. There's already domainName to be confusing so perhaps

Suggested change

domain = mkOption {

announceDomain = mkOption {

Sounds like a good idea to me, though these option names come from avahi and I think it would therefore be better to keep them.
Regarding the ambiguity, I think this is not much of a problem, because the two options are on different levels.

worldofpeace · 2019-05-31T23:50:36Z

nixos/modules/services/networking/avahi-daemon.nix


+    cacheEntriesMax = mkOption {


Suggested change

cacheEntriesMax = mkOption {

maxCacheEntries = mkOption {

Feels backwards to me. Options renamed should use mkRenamedOptionModule btw.

Same as with the other option.

nixos/modules/services/networking/avahi-daemon.nix

WilliButz · 2019-06-02T13:24:31Z

I updated the PR to include most of the suggested changes. I'd like to wait for feedback on the renaming of the two options before adding these changes.
Also rebased on staging.

worldofpeace · 2019-06-02T21:03:21Z

I updated the PR to include most of the suggested changes. I'd like to wait for feedback on the renaming of the two options before adding these changes.
Also rebased on staging.

The renaming of the options were essentially nitpicks and didn't really need a response.
This looks good to me 👍 thank you for being so responsive to our requests.

Lastly, it's always a good idea to actually test the service. And specifically extraServiceFiles.
I'll try to perform this today in a NixOS vm.

worldofpeace · 2019-06-03T22:05:50Z

Actually, perhaps https://github.com/NixOS/nixpkgs/blob/bac6f67aaa8fa87425b6f5cc046534ba4b734d9c/nixos/tests/avahi.nix should be updated to test extraServiceFiles?

Avahi now uses `/etc/avahi/services` instead of its store path to look for files with service definitions.

Types are now specified for all options. The fixed uid and gid for the avahi user have been removed and the user avahi is now in the group avahi. The the generic opening of the firewall for UDP port 5353 is now optional, but still defaults to true. The option `extraServiceFiles` was added to specify avahi service definitions, which are then placed in `/etc/avahi/services`.

WilliButz · 2019-06-04T00:14:05Z

@worldofpeace done. Took me a while to build staging after rebasing though :)
@GrahamcOfBorg test avahi

worldofpeace

On my side of things this looks good 🌸

peterhoeg · 2019-06-04T05:03:27Z

The avahi service does not manage any persistent state so it shouldn't really matter. Why are different uids a problem?

It isn't a problem per se, but currently we have this nice guarantee that the ids are all the same across systems which is no longer the case for no gain.

Avahi does support running as non-root but the code still expects the hardcoded username to exist. That's why we can't switch to DynamicUser. Fixing avahi is out of scope for this PR though.

Fully agree.

a. avahi upstream provides a systemd unit file - we don't need our own service def (except for the activation)

The upstream systemd service unit is pretty basic and in that case I actually prefer defining the whole service in the NixOS module. When avahi will eventually support DynamicUser or other systemd shenanigans we have to touch our service definition anyway.

I think as a matter of principle, we should use upstream - we really don't gain anything by maintaining our own.

nixos/modules/services/networking/avahi-daemon.nix

See #62153 (comment)

worldofpeace · 2019-06-06T20:00:03Z

Only comment left that may be blocking is this is a principal of whether to use systemd.packages or define everything manually within the module. (raised by @peterhoeg)

I generally tend to prefer if there's a service upstream that we could use, and they're practically identical...
If we were to use it we'd have to establish the alias manually though like network-manager

nixpkgs/pkgs/tools/networking/network-manager/default.nix

Line 102 in 97aba37

    
               ln -s $out/etc/systemd/system/NetworkManager.service $out/etc/systemd/system/dbus-org.freedesktop.NetworkManager.service

No real loss in leaving it as is, no?

peterhoeg · 2019-06-07T03:31:23Z

I generally tend to prefer if there's a service upstream that we could use

IMHO, the best kind of code is the one you don't have to write.

globin · 2019-06-11T14:04:21Z

I think this PR is definitely ready enough to be merged, any further minor issues like using the systemd unit from upstream can be addressed in a follow-up PR. This should not have to be blocked as it definitely improves the module a lot already.

ciil approved these changes May 28, 2019

View reviewed changes

ofborg bot added 6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) labels May 28, 2019

ofborg bot requested a review from lovek323 May 28, 2019 12:49

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 2501-5000 labels May 28, 2019

worldofpeace self-requested a review May 28, 2019 16:46

fadenb approved these changes May 30, 2019

View reviewed changes

fpletz approved these changes May 31, 2019

View reviewed changes

elseym suggested changes May 31, 2019

View reviewed changes

nixos/modules/services/networking/avahi-daemon.nix Outdated Show resolved Hide resolved

elseym suggested changes May 31, 2019

View reviewed changes

nixos/modules/services/networking/avahi-daemon.nix Outdated Show resolved Hide resolved

nixos/modules/services/networking/avahi-daemon.nix Outdated Show resolved Hide resolved

nixos/modules/services/networking/avahi-daemon.nix Outdated Show resolved Hide resolved

WilliButz force-pushed the avahi-refactor branch from 3ccf430 to 1f6b356 Compare May 31, 2019 18:24

worldofpeace suggested changes Jun 1, 2019

View reviewed changes

WilliButz force-pushed the avahi-refactor branch 3 times, most recently from c2b4fdc to ede8f45 Compare June 2, 2019 13:19

WilliButz added 5 commits June 4, 2019 00:22

avahi: set AVAHI_SERVICE_DIR to well-known location

c260e9e

Avahi now uses `/etc/avahi/services` instead of its store path to look for files with service definitions.

nixos/ids: remove avahi uid/gid

1800e49

nixos/doc: add section about avahi changes

229e783

nixos/tests/avahi: add test for extra service definitions

dbf4be4

WilliButz force-pushed the avahi-refactor branch from ede8f45 to dbf4be4 Compare June 4, 2019 00:12

worldofpeace approved these changes Jun 4, 2019

View reviewed changes

elseym approved these changes Jun 4, 2019

View reviewed changes

globin approved these changes Jun 5, 2019

View reviewed changes

flokli previously requested changes Jun 5, 2019

View reviewed changes

nixos/modules/services/networking/avahi-daemon.nix Show resolved Hide resolved

globin requested a review from flokli June 5, 2019 15:58

flokli approved these changes Jun 6, 2019

View reviewed changes

globin merged commit 68c30f0 into NixOS:staging Jun 11, 2019

WilliButz deleted the avahi-refactor branch June 11, 2019 18:11

WilliButz mentioned this pull request Jul 26, 2019

Avahi advertises ssh/sftp because they happen to be the samples that ship with the package #60939

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avahi: set service directory and refactor module #62153

avahi: set service directory and refactor module #62153

WilliButz commented May 28, 2019

WilliButz commented May 28, 2019

peterhoeg commented May 30, 2019

WilliButz commented May 30, 2019

peterhoeg commented May 31, 2019

fpletz commented May 31, 2019

fpletz commented May 31, 2019

elseym left a comment

WilliButz commented May 31, 2019

worldofpeace commented May 31, 2019

worldofpeace left a comment

worldofpeace May 31, 2019

WilliButz Jun 1, 2019

worldofpeace May 31, 2019

WilliButz Jun 1, 2019

WilliButz commented Jun 2, 2019

worldofpeace commented Jun 2, 2019

worldofpeace commented Jun 3, 2019

WilliButz commented Jun 4, 2019

worldofpeace left a comment

peterhoeg commented Jun 4, 2019

worldofpeace commented Jun 6, 2019

peterhoeg commented Jun 7, 2019

globin commented Jun 11, 2019

avahi: set service directory and refactor module #62153

avahi: set service directory and refactor module #62153

Conversation

WilliButz commented May 28, 2019

Motivation for this change

Things done

Package:

Module:

Manual:

fixes #60939

WilliButz commented May 28, 2019

peterhoeg commented May 30, 2019

WilliButz commented May 30, 2019

peterhoeg commented May 31, 2019

fpletz commented May 31, 2019

fpletz commented May 31, 2019

elseym left a comment

Choose a reason for hiding this comment

WilliButz commented May 31, 2019

worldofpeace commented May 31, 2019

worldofpeace left a comment

Choose a reason for hiding this comment

worldofpeace May 31, 2019

Choose a reason for hiding this comment

WilliButz Jun 1, 2019

Choose a reason for hiding this comment

worldofpeace May 31, 2019

Choose a reason for hiding this comment

WilliButz Jun 1, 2019

Choose a reason for hiding this comment

WilliButz commented Jun 2, 2019

worldofpeace commented Jun 2, 2019

worldofpeace commented Jun 3, 2019

WilliButz commented Jun 4, 2019

worldofpeace left a comment

Choose a reason for hiding this comment

peterhoeg commented Jun 4, 2019

worldofpeace commented Jun 6, 2019

peterhoeg commented Jun 7, 2019

globin commented Jun 11, 2019