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

modules/avahi: Sync module defaults with upstream #361191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

frederictobiasc
Copy link
Contributor

@frederictobiasc frederictobiasc commented Dec 2, 2024

This contribution syncs the module's defaults with upstream (man avahi-daemon.conf) defaults.

Motivation is to reduce the effort for getting a standard mdns configuration.

Additionally, this contribution enables nssmdns by default.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

CC: @fpletz @SuperSandro2000


Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 2, 2024
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 2, 2024
@@ -227,7 +227,7 @@ in

nssmdns6 = lib.mkOption {
type = lib.types.bool;
default = false;
default = true;
Copy link
Member

@SuperSandro2000 SuperSandro2000 Dec 3, 2024

Choose a reason for hiding this comment

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

Suggested change
default = true;
default = false;

see description

description = "Whether to announce the locally used domain name for browsing by other hosts.";
};
};

nssmdns4 = lib.mkOption {
type = lib.types.bool;
default = false;
default = true;
Copy link
Member

Choose a reason for hiding this comment

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

This is the only option I have ever enabled on any of my clients. I think all the others are just announcing information into the network you are connecting to for no good reason and especially on public wifi we don't want to announce cpu information or why would we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

especially on public wifi we don't want to announce cpu information or why would we?

I agree that probably nobody cares about CPU information on a public Wi-Fi. However, I don't see where this does any harm.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tracking, mostly. Fingerprinting can involve basically any details of your system, so exposing as little of that as possible is valuable.

Copy link
Member

Choose a reason for hiding this comment

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

What is the use case to publish those information? Why should it be enabled by default if we don't know why we enabled it?

Also do we really want to expand the output of avahi-browse -a with services and information that we coulnd't come up with a reason?

We probably want to change nssmdns4 to default to yes because that is the most common usecase for avahi and without it, it doesn't do much but I cannot follow why we should change all the other options.

@@ -175,25 +175,25 @@ in
publish = {
enable = lib.mkOption {
type = lib.types.bool;
default = false;
default = true;
Copy link
Member

Choose a reason for hiding this comment

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

and this I only ever enabled on machines like audio or print servers.

@frederictobiasc
Copy link
Contributor Author

frederictobiasc commented Dec 3, 2024

This change reduces setup friction for most users that simply expect a working default avahi configuration when setting services.avahi.enable = true.

For most modules, NixOS users only need to activate the corresponding enable option and can expect a reasonable and working default configuration. This is currently not the case with the avahi module. The avahi module in it's current state deviates significantly from avahi's defaults. I'm convinced that this doesn't align with user's expectations set by the behaviour of other distributions.

@SuperSandro2000 Please keep in mind, this PR does not suggest enabling avahi by default. Users that want to use avahi and have concerns can still review the default configuration - as they would do on any other distribution.

@SuperSandro2000
Copy link
Member

For most modules, NixOS users only need to activate the corresponding enable option and can expect a reasonable and working default configuration.

That is not really true. For example many webservices require you to set a domain and sometimes even outright refuse to evaluate if that is not given.

Users that want to use avahi and have concerns can still review the default configuration - as they would do on any other distribution.

The default configuration shouldn't be like: you are almost certainly required to turn this off, to have a good and reasonable experience just because upstream has this or that default. We don't need to copy the bad parts of other distros where you first need to change all the defaults because they suck and are known to be bad defaults.

description = "Whether to publish user services. Will set `addresses=true`.";
};

addresses = lib.mkOption {
type = lib.types.bool;
default = false;
default = true;
description = "Whether to register mDNS address records for all local IP addresses.";
};

hinfo = lib.mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct. I compared with https://linux.die.net/man/5/avahi-daemon.conf, but the rendered manual targets an older avahi version.

Updated it accordingly.

Comment on lines 203 to 208
workstation = lib.mkOption {
type = lib.types.bool;
default = false;
default = true;
description = ''
Whether to register a service of type "_workstation._tcp" on the local LAN.
'';
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As with hinfo, I updated it. Thanks for pointing it out.

description = "Whether to allow publishing in general.";
};

userServices = lib.mkOption {
type = lib.types.bool;
default = false;
default = true;
description = "Whether to publish user services. Will set `addresses=true`.";
};

addresses = lib.mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@frederictobiasc frederictobiasc Jan 13, 2025

Choose a reason for hiding this comment

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

No, this is also required to reply to mDNS requests.

Comment on lines 182 to 186
userServices = lib.mkOption {
type = lib.types.bool;
default = false;
default = true;
description = "Whether to publish user services. Will set `addresses=true`.";
};
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no default user services. Check extraServiceFiles.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 31, 2024
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jan 13, 2025
This contribution syncs the module's default with upstream
avahi's defaults.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants