Partial revert of #76542's strict hostname checks #94022

grahamc · 2020-07-27T18:32:19Z

Motivation for this change

See #94011 for context, but in general this check is a bit too strict.

Things done

Some networks depend on the hostname being a FQDN, and migrating is not a trivial task. See the tracking issue in NixOS#94011, and the PR containing the reverted commit: NixOS#76542 This reverts commit 993baa5.

grahamc · 2020-07-27T18:32:58Z

nixos/lib/test-driver/test-driver.py

@@ -216,7 +216,7 @@ def __init__(self, args: Dict[str, Any]) -> None:
            if cmd:
                match = re.search("run-(.+)-vm$", cmd)
                if match:
-                    self.name = match.group(1)
+                    self.name = match.group(1).replace(".", "_")


This file is named after the hostname, but later on this Python code uses the machine.name value as a Python variable name.

I guess the problem doesn't just apply to .'s but all kinds of allowed characters? e.g. -?

True, though I wasn't sure just how far I should go. I could add - too. Should I do more?

andir

IMHO this looks fine. I'd like to hear from @zimbatm (and other involved) parties what their thought on this is. As I outlined in the related issue I kinda gave up on the split of what a hostname is long ago for various reasons.

primeos

Before we merge this we also need to update the release notes in nixos/doc/manual/release-notes/rl-2009.xml.

Other than that I'm fine if we don't enforce the recommendations / best practices anymore (enforcing a recommended syntax was always critical and for that reason I guess I'd actually even prefer only documenting the recommendation in the option instead of enforcing it - but personally I'm fine with it either way and I'd prefer the current implementation if it where only enforced by Linux/systemd).

primeos · 2020-07-27T20:59:44Z

nixos/modules/tasks/network-interfaces.nix

-      # reasons (as undocumented feature):
-      type = types.strMatching
-        "^$|^[[:alpha:]]([[:alnum:]_-]{0,61}[[:alnum:]])?$";
+      type = types.str;


I think it would be best if we'd still limit the maximum length to 63 characters as that is actually a strict limit.

primeos · 2020-07-27T21:01:10Z

nixos/modules/tasks/network-interfaces.nix

-        contain the domain part. This means that the hostname must start with a
-        letter, end with a letter or digit, and have as interior characters only
-        letters, digits, and hyphen. The maximum length is 63 characters.
-        Additionally it is recommended to only use lower-case characters.


I would still keep that part or a similar version as a recommendation (or at least refer to hostname(5)).

vcunat

I don't really have an opinion on this. If it's not rare to break the previous strict check in practice, this PR's approach sounds right.

Docs in Linux and POSIX don't seem clear, with man hostname.5 just "recommending" it to be a single label. I agree with this PR still "enforcing" that

fqdn = hostName + (optionalString (domain != null) ".${domain}");

flokli

I still think most of the logic on why simply using the FQDN as hostname from systemd/systemd#15894 (comment) applies to us as well. Doing this is a bad idea, and works in some usecases "mostly by accident".

IMHO we should follow that logic, and keep the NixOS module code simple. Previously, we had some error-prone "if hostname is a fqdn, do this, else do that" logic in multiple places (and hadn't it in some others).
I was also guilty of using the FQDN as hostname, and it broke in other places (and required quirky workarounds).

Generally, I think people who still need to stick with the old behaviour, and can't switch, can maintain a revert of that functionality in a fork, as suggested by @primeos in #94011.

If that really is not an option, and for some reasons these "large corporate users" need some time to switch, I wouldn't object too much about adding a "footgun" with a heavy warning sign for one release to opt-out from the assertion failure (and without any additional logic inside the rest of the module system).

People should really fix their broken setups, instead of nixpkgs carrying workarounds.

blitz · 2020-07-28T20:58:40Z

I agree with @flokli. Having a warning for an extended period seems like a good middle ground between breaking users and having subtly b0rken setups.

andir · 2020-07-28T23:57:40Z

On Tue, Jul 28, 2020, 19:16 Florian Klink ***@***.***> wrote: IMHO we should follow that logic, and keep the NixOS module code simple. Previously, we had some error-prone "if hostname is a fqdn, do this, else do that" logic in multiple places (and hadn't it in some others). I was also using the FQDN as hostname, and it broke in other places (and required quirky workarounds).

What did break and what were those workarounds? Having read the linked systemd issue I am still not convinced of what you are arguing for.

…

flokli · 2020-07-29T11:10:56Z

Avahi for example fails to run with an error message if config.networking.hostName is a FQDN (and services.avahi.hostName and services.avahi.domainName need to be set to prevent this).

In general, supporting both things increases complexity, having the FQDN is discouraged, and I don't see much reason to just revert this.

arianvp · 2020-07-29T13:20:30Z

Another place where this might be a problem is in nixos-rebuild --flake

nixos-rebuild will use /proc/sys/kernel/hostname as the default flake attribute name; but that might be formatted as an FQDN .

However since recently nix-instantiate silently ignores attributes with dots in their name NixOS/nix#3798 (comment) this will lead to issues

aanderse · 2020-08-15T11:31:10Z

Depending on the outcome of this conversation nixops may need to be updated:

    deployment.targetHost = mkDefault config.networking.hostName;

This default value should be updated to include the domain now as well.

arianvp · 2020-08-15T11:39:07Z

This breaks at least digitalocean and Packet.net too by the way; which include . in hostnames by default

flokli · 2020-08-16T12:56:37Z

@arianvp can you elaborate how this breaks these machines? If we're just talking about hostnames being sent over DHCP, that should still work (hostnamectl set-hostname --transient foo.bar.baz)

flokli · 2020-08-17T20:55:43Z

Also note the packet hostnames are only suggested, and probably are not intended to be used as FQDNs anyways: fra2-x1.small.x86-01. I doubt small.x86-01 is meant as the domain part.

primeos · 2020-09-05T12:18:29Z

I'll go ahead and close this as we also have #94011 which seems more appropriate for the current discussion. We can of course re-open this PR if necessary.

Since NixOS#76542 this workaround is required to use a FQDN as hostname. See NixOS#94011 and NixOS#94022 for the related discussion. Due to some potential/unresolved issues (legacy software, backward compatibility, etc.) we're documenting this workaround [0]. [0]: NixOS#94011 (comment)

Since #76542 this workaround is required to use a FQDN as hostname. See #94011 and #94022 for the related discussion. Due to some potential/unresolved issues (legacy software, backward compatibility, etc.) we're documenting this workaround [0]. [0]: #94011 (comment)

Since NixOS#76542 this workaround is required to use a FQDN as hostname. See NixOS#94011 and NixOS#94022 for the related discussion. Due to some potential/unresolved issues (legacy software, backward compatibility, etc.) we're documenting this workaround [0]. [0]: NixOS#94011 (comment) (cherry picked from commit 4a600af)

grahamc added 3 commits July 27, 2020 13:32

Revert "nixos: Require networking.hostName to be a valid DNS label"

87be60d

Some networks depend on the hostname being a FQDN, and migrating is not a trivial task. See the tracking issue in NixOS#94011, and the PR containing the reverted commit: NixOS#76542 This reverts commit 993baa5.

nixos python test driver: handle machines with periods in hostnames

35a0c0a

nixos: hostname tests: test FQDNs in the hostName

24078cd

grahamc requested a review from tfc as a code owner July 27, 2020 18:32

grahamc commented Jul 27, 2020

View reviewed changes

grahamc mentioned this pull request Jul 27, 2020

Requiring hostname to be a single domain label is fairly heavy handed for some networks #94011

Open

ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 27, 2020

andir reviewed Jul 27, 2020

View reviewed changes

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 27, 2020

primeos requested changes Jul 27, 2020

View reviewed changes

primeos mentioned this pull request Jul 27, 2020

nixos/networking: Add the FQDN and hostname to /etc/hosts #76542

Merged

14 tasks

vcunat reviewed Jul 28, 2020

View reviewed changes

flokli requested changes Jul 28, 2020

View reviewed changes

aanderse mentioned this pull request Aug 15, 2020

20.09 Feature freeze items #95475

Closed

primeos added this to the 20.09 milestone Aug 15, 2020

primeos closed this Sep 5, 2020

primeos removed this from the 20.09 milestone Sep 5, 2020

primeos mentioned this pull request Oct 10, 2020

doc: Document a workaround for using an FQDN as hostname #100151

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial revert of #76542's strict hostname checks #94022

Partial revert of #76542's strict hostname checks #94022

grahamc commented Jul 27, 2020

grahamc Jul 27, 2020

andir Jul 27, 2020

grahamc Jul 27, 2020

andir left a comment

primeos left a comment

primeos Jul 27, 2020

primeos Jul 27, 2020

vcunat left a comment

flokli left a comment •

edited

Loading

blitz commented Jul 28, 2020

andir commented Jul 28, 2020 via email

flokli commented Jul 29, 2020

arianvp commented Jul 29, 2020 •

edited

Loading

aanderse commented Aug 15, 2020

arianvp commented Aug 15, 2020

flokli commented Aug 16, 2020

flokli commented Aug 17, 2020

primeos commented Sep 5, 2020

Partial revert of #76542's strict hostname checks #94022

Partial revert of #76542's strict hostname checks #94022

Conversation

grahamc commented Jul 27, 2020

Motivation for this change

Things done

grahamc Jul 27, 2020

Choose a reason for hiding this comment

andir Jul 27, 2020

Choose a reason for hiding this comment

grahamc Jul 27, 2020

Choose a reason for hiding this comment

andir left a comment

Choose a reason for hiding this comment

primeos left a comment

Choose a reason for hiding this comment

primeos Jul 27, 2020

Choose a reason for hiding this comment

primeos Jul 27, 2020

Choose a reason for hiding this comment

vcunat left a comment

Choose a reason for hiding this comment

flokli left a comment • edited Loading

Choose a reason for hiding this comment

blitz commented Jul 28, 2020

andir commented Jul 28, 2020 via email

flokli commented Jul 29, 2020

arianvp commented Jul 29, 2020 • edited Loading

aanderse commented Aug 15, 2020

arianvp commented Aug 15, 2020

flokli commented Aug 16, 2020

flokli commented Aug 17, 2020

primeos commented Sep 5, 2020

flokli left a comment •

edited

Loading

arianvp commented Jul 29, 2020 •

edited

Loading