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

More correct form of /etc/hosts #27105

Merged
merged 8 commits into from Jul 30, 2017
Merged

More correct form of /etc/hosts #27105

merged 8 commits into from Jul 30, 2017

Conversation

regellosigkeitsaxiom
Copy link
Contributor

@regellosigkeitsaxiom regellosigkeitsaxiom commented Jul 3, 2017

Makes management of /etc/hosts more declarative by using networking.hosts.

Motivation for this change

It makes possible to point multiple domains to localhost in correct way: just adding 127.0.0.1 localhost.localdomain to networking.extraHosts will not always work and is incorrect.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

It adds its contents to '127.0.0.1' line of /etc/hosts
It makes possible to point multiple domains to localhost in correct way
@mention-bot
Copy link

@regellosigkeitsaxiom, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @Mic92 and @ardumont to be potential reviewers.

@@ -187,11 +199,11 @@ in
"rpc".source = pkgs.glibc.out + "/etc/rpc";

# /etc/hosts: Hostname-to-IP mappings.
"hosts".text =
"hosts".text = let foo = concatStringsSep " " cfg.extraLocalHosts; in
Copy link
Contributor

Choose a reason for hiding this comment

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

foo? Seriously?

Copy link
Member

@Mic92 Mic92 Jul 4, 2017

Choose a reason for hiding this comment

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

What my predecessor meant to say, is that foo is in general not a good name for a variable as it is too generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By naming this "variable" foo I meant that its name does not hold any significance in semantic space of whole program. It is just a technical variable for text that occurs in several (two in this case) places, an its purpose is minor increase in bug-proofness in a way that one will not change this text in one place and forget to do same in the other.

Its scope is limited to several lines, and its use is kinda obvious, so it should not be a problem (except greping source).

It is just a coding style thing, and I do not hold it dear. So if you propose how should be done, it should not be a problem at all.

Copy link
Contributor

@0xABAB 0xABAB left a comment

Choose a reason for hiding this comment

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

Good initiative, but careless implementation.

@joachifm
Copy link
Contributor

joachifm commented Jul 4, 2017

Help me understand: it seems to me the current implementation ensures that the canonical localhost mapping comes first and so always will resolve correctly (unless you override the low-level environment.etc."hosts".text attr, but that defeats this PR as well). The issue described in the linked discussion refers to situations where 127.0.0.1 resolves to something other than localhost (due to an earlier entry) but it's unclear how you can make that happen by adding stuff to extraHosts currently.

Can you elaborate?

@joachifm
Copy link
Contributor

joachifm commented Jul 4, 2017

Hrm, it occurred to me that you're probably talking about names other than localhost ...

EDIT: but then it seems like we're trying to support odd use cases; who would try to map a name to localhost and some other address in the same hosts file configuration? That just seems confused.

''
127.0.0.1 localhost
127.0.0.1 localhost ${foo}
Copy link
Member

@Mic92 Mic92 Jul 4, 2017

Choose a reason for hiding this comment

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

According to the article, it might be desirable to have the variable before localhost to set the fqdn correctly.
Maybe the smarter solution is to prepend extraHosts before the localhost sections. Then there would be no need for an extra option.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for correction about FQDN, I will adjust it.

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 for prepending extraHosts before localhost, this will cause havoc, as explained in chosen answer of the article.

@regellosigkeitsaxiom
Copy link
Contributor Author

regellosigkeitsaxiom commented Jul 7, 2017

@joachifm my use case is the following:

I sometimes work on web services for which URI's are important, say it connects to socket at fixed address foo.example.com. CORS things, web clients and stuff. When I work offline on my laptop, I start corresponding services locally and add them to 127.0.0.1 line in /etc/hosts. This way whole system behaves exactly like on live/development environment, and I can hack it offline and/or with very fast feedback.

My specific problem is, that if I am connected to network with no Internet connection, accessing second /etc/hosts 127.0.0.1 line hangs. I do not know exactly how this works, but setting up /etc/hosts correctly helps. (I may be missing or distorting some details, but it bugs me nevertheless).

@regellosigkeitsaxiom
Copy link
Contributor Author

regellosigkeitsaxiom commented Jul 7, 2017

@volth I agree with you point that extra option for such trivial and rare thing is overkill.

However I think that doing it exactly the way you proposed will break config transparency. Main reason is that option is named extraHosts and given its type it is expected to append its content to config file verbatim. Handling line starting with 127.0.0.1 in a special way breaks this convention, makes it less transparent to user, needs parsing of raw text, which is easy both in implementation and introducing bugs, and surely is not the best thing given that Nix Expression is a typed language.

Simply dropping 127.0.0.1 localhost if user specified 127.0.0.1 something will give user a chance to shoot its own leg by forgetting to add localhost, which I consider a bad thing.

I suppose that this form should better implement you intention:

{
  networking.hosts = {
    "127.0.0.1" = [ "foo.example.com" ];
    "192.168.0.2" = [ "fileserver.local" "nameserver.local" ];
  };
}

This way it can be trivially and safely merged into /etc/hosts in expected way.

@joachifm
Copy link
Contributor

joachifm commented Jul 8, 2017

@regellosigkeitsaxiom would using a higher merge priority work for you? As in, specifying

extraHosts = mkBefore ''
  127.0.0.1 foo.bar.baz
  127.0.0.1 a.b.c
''

Then these entries should come immediately after the standard localhost mappings in the resulting hosts file. That'd accomplish the same thing as this new option, right?

@regellosigkeitsaxiom
Copy link
Contributor Author

regellosigkeitsaxiom commented Jul 8, 2017

@joachifm if I understood you correctly, resulting file will be:

127.0.0.1 foo.bar.baz
127.0.0.1 a.b.c
127.0.0.1 localhost

This will break many things.

My aim is to have the file:

127.0.0.1 foo.bar.baz a.b.c localhost

@joachifm
Copy link
Contributor

joachifm commented Jul 8, 2017

@regellosigkeitsaxiom no, the current implementation emits 127.0.0.1 localhost before extraHosts.

@regellosigkeitsaxiom regellosigkeitsaxiom changed the title Added networking.extraLocalHosts option More correct form of /etc/hosts Jul 8, 2017
@regellosigkeitsaxiom
Copy link
Contributor Author

Removed networking.extraLocalHosts
Added networking.hosts as described in comment in this thread.
Also added option networking.fqdn, just to be able to pin FQDN in one place.

default = null;
example = "foo.example.com";
description = ''
Full qualified domain name, if any.
Copy link
Contributor

Choose a reason for hiding this comment

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

Fully

Copy link
Contributor

@0xABAB 0xABAB left a comment

Choose a reason for hiding this comment

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

I have no further comments after the details I mentioned have been addressed.

Thanks!

then concatStringsSep " " ( filter (x : x != "localhost" ) ( getAttr "::1" cfg.hosts))
else "";
otherHosts = allToString ( removeAttrs cfg.hosts [ "127.0.0.1" "::1" ]);
maybeFQDN = if cfg.fqdn == null then "" else cfq.fqdn;
Copy link
Member

Choose a reason for hiding this comment

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

we have lib.optionalString for this.

userLocalHosts =
if builtins.hasAttr "127.0.0.1" cfg.hosts
then concatStringsSep " " ( filter (x : x != "localhost" ) ( getAttr "127.0.0.1" cfg.hosts))
else "";
Copy link
Member

@Mic92 Mic92 Jul 8, 2017

Choose a reason for hiding this comment

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

can be also shortened with optionalString.

userLocalHosts6 =
if builtins.hasAttr "::1" cfg.hosts
then concatStringsSep " " ( filter (x : x != "localhost" ) ( getAttr "::1" cfg.hosts))
else "";
Copy link
Member

Choose a reason for hiding this comment

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

same here.

Also dangerous typo fix
@vcunat
Copy link
Member

vcunat commented Jul 10, 2017

It seems clear that having the same IP on multiple lines is bad.

I don't know these UNIX* details well, but do you have any idea why hostname and domainname is configured separately/independently of localhost's fully-qualified DNS name? IMHO it feels messy when those differ. If there's no good use case, perhaps we could link their configs in nixos, at least as defaults...

@regellosigkeitsaxiom
Copy link
Contributor Author

regellosigkeitsaxiom commented Jul 11, 2017

@vcunat will this configuration work for your purpose?

{
  networking.hostname="foobar";
  networking.fqdn = cfg.networking.hostname + ".example.com.";
  networking.hosts = {
    "93.184.216.34" = [ cfg.networking.fqdn ];
  };
}

While it might be nice to have options like networking.domain and networking.publicIP, I have no idea what places beside /etc/hosts they should affect (and if nowadays they make any sense at all), so I myself would rather refrain from adding them until I gain this knowledge.

Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

Seems OK.

@vcunat
Copy link
Member

vcunat commented Jul 16, 2017

BTW, we do have networking.domain already. I don't know of any particular "use cases", I was just wondering about the stuff.

@regellosigkeitsaxiom
Copy link
Contributor Author

@volth will resulting hosts be overwritten with every nixos-rebuild? If so, it will silently destroy configurations and make system less predictable for forgetful user.

If it will be switched by something like networking.hostsMutable and will be false by default, it should be fine with most users. Also maybe if set to true then hosts should not be managed by nix at all.

I am against including it into this PR as it seems an entirely different feature to me.

@regellosigkeitsaxiom
Copy link
Contributor Author

@volth Maybe something like networking.hostsMutable = { "no" | "ephemeral" | "unmanaged" }?

@fpletz fpletz added this to the 17.09 milestone Jul 29, 2017
@fpletz
Copy link
Member

fpletz commented Jul 29, 2017

What is going on here? Are there comments by @volth missing? I see @regellosigkeitsaxiom answering him but he isn't a participant on this issue. Github bug?

Copy link
Member

@fpletz fpletz left a comment

Choose a reason for hiding this comment

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

Thanks! I welcome the change to a more declarative way to specify hosts. 👍

@regellosigkeitsaxiom
Copy link
Contributor Author

@fpletz actually there are more than ten comments missing, and not only from @volth. I have no idea what is going on.

@vcunat vcunat merged commit 635ecd8 into NixOS:master Jul 30, 2017
vcunat added a commit that referenced this pull request Jul 30, 2017
@vcunat
Copy link
Member

vcunat commented Jul 30, 2017

I suppose mutableHosts mode could be added, but that seems a significantly larger change, and we most likely want to keep this static way as well.

@NeQuissimus
Copy link
Member

Oh, I am just catching up on this...

Can we not deprecate extraHosts and just keep it at the bottom of the /etc/hosts output?!
I like using https://github.com/StevenBlack/hosts and add lots of entries that way...

@vcunat
Copy link
Member

vcunat commented Jul 31, 2017

@volth: that would result into the file being writable (by root) but overwritten on each activation, right? I wonder what's the use case – is there some SW that wants to write into it?

@NeQuissimus: right, adding a list from a (potentially large) file is something we do want to support. I should've noticed. Undeprecating extraHosts seems the best way ATM.

@regellosigkeitsaxiom
Copy link
Contributor Author

regellosigkeitsaxiom commented Jul 31, 2017

@NeQuissimus technically, extraHosts is working as always. Nix just emits a warning during rebuild.

I submitted #27791 which removes this warning.

@edolstra
Copy link
Member

It's not exactly clear to me what this PR does to 127.0.0.1. If it adds a mapping for the hostname, then that's incorrect - that's done automatically via nss_myhostname.

@regellosigkeitsaxiom
Copy link
Contributor Author

regellosigkeitsaxiom commented Jul 31, 2017

@edolstra it makes possible to concatenate all mappings to 127.0.0.1 into one line:

127.0.0.1 foo.bar localhost

Via extraHosts it can only be done like this:

127.0.0.1 localhost
127.0.0.1 foo.bar

which sometimes leads to problems as /etc/hosts actually works as two-way mapping. This PR ensures that every IP occurs only once, and 127.0.0.1 is treated specially only in a way that it guarantees that it will map to localhost.

Regarding networking.fqdn: according to, for example, this, first column of /etc/hosts should always contain FQDN of the server. networking.fqdn emits its contents in first column of 127.0.0.1. Since in "vanilla" linux it is done manually and such FQDN can differ from hostname ++ domain name, I added it as a separate option to support such use cases (however I do not know if those actually exist).

@edolstra
Copy link
Member

Mapping the FQDN to 127.0.0.1 is very very wrong. For example, consider a web server configuration that tells the server to listen on ${fqdn}. Then it will now suddenly listen on 127.0.0.1, which is probably not what you want.

nss_myhostname addresses this by making the hostname map to some public IP, with 127.0.0.1 only as a fallback, IIRC.

@regellosigkeitsaxiom
Copy link
Contributor Author

regellosigkeitsaxiom commented Jul 31, 2017

@edolstra In a sense, it will not "suddenly" listen on 127.0.0.1 as networking.fqdn did not exist prior to this PR. But you point that it can lead to misunderstandings and cause a mess seems clear to me. Then should networking.fqdn be removed entirely? Or should something like fqdnIP added?

@regellosigkeitsaxiom
Copy link
Contributor Author

regellosigkeitsaxiom commented Jul 31, 2017

@volth in my case I go with nixos-rebuild. If I do not update channels, it takes only couple of seconds. Does you config includes something that depends of /etc/hosts?

If I were in you shoes, I'd try to include into configuration.nix custom post-install script which flips /etc/hosts into regular file.

@vcunat
Copy link
Member

vcunat commented Jul 31, 2017

It's a fallback because myhostname is tried after (public) DNS. https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/config/nsswitch.nix#L19

@vcunat
Copy link
Member

vcunat commented Jul 31, 2017

I do sometimes remove symlinks in /etc/ and replace them by temporary changed copies, e.g. for /etc/nix/machines. It seems to work OK; having nixos do copies instead of symlinks would make that slightly easier, though I'm not certain if it would be worth it.

@edolstra
Copy link
Member

@regellosigkeitsaxiom Well, prior to this PR, the FQDN would be looked up in DNS, which would return an address other than 127.0.0.1, assuming the FQDN exists in DNS.

BTW, the hostname manpage says:

Technically: The FQDN is the name gethostbyname(2) returns for the host name returned by gethostname(2).
There is no mention of 127.0.0.1 there.

Possibly relevant: https://bugzilla.redhat.com/show_bug.cgi?id=530343

In any case, let's not mess with hostname mappings in /etc/hosts since it will have unpredictable results.

@vcunat
Copy link
Member

vcunat commented Jul 31, 2017

The hostname(1) manpage doesn't really ensure me in this matter:

-f, --fqdn, --long Display the FQDN (Fully Qualified Domain Name). A FQDN consists of a short host name and the DNS domain name. Unless you are using bind or NIS for host lookups you can change the FQDN and the DNS domain name (which is part of the FQDN) in the /etc/hosts file.

The related docs seem to mainly describe the implementation, IMHO, and not the (intended) meaning of it. We can't significantly affect that mess, except perhaps building some clearer abstraction above it.

In any case, I believe we should allow changing the 127.0.0.1 + ::1 lines declaratively – adding more aliases (which is what this PR did), even though we may not be recommending to do so. As noted, by default this PR changes nothing. We might perhaps improve the descriptions to better explain what the options do.

BTW, if you use gethostbyaddress on 127.0.0.1 or ::1, I believe /etc/hosts will take priority and get the first entry for it, i.e. you should get either localhost or networking.fqdn (if you explicitly set that option). My DNS intuition about the function would prefer returning the real FQDN of that machine, i.e. the name that public DNS would resolve to this machine's public IP, though getting back localhost might be more correct in some ways.

( builtins.hasAttr "::1" cfg.hosts )
( concatStringsSep " " ( remove "localhost" cfg.hosts."::1" ));
otherHosts = allToString ( removeAttrs cfg.hosts [ "127.0.0.1" "::1" ]);
maybeFQDN = optionalString ( cfg.fqdn != null ) cfg.fqdn;
Copy link
Member

@aszlig aszlig Jul 31, 2017

Choose a reason for hiding this comment

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

Wouldn't it make more sense to put the following into config instead?

{
  networking.hosts = {
    "127.0.0.1" = [ "localhost" ];
  } // optionalAttrs cfg.enableIPv6 {
    "::1" = [ "localhost" ];
  };
}

That way the value in environment.etc.hosts.text can be reduced to:

let
  mkEntry = ip: hosts: "${ip} ${concatStringsSep " " hosts}";
in ''
  ${concatStringsSep "\n" (mapAttrsToList mkEntry cfg.hosts)}
  ${cfg.extraHosts}
''

Copy link
Member

Choose a reason for hiding this comment

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

The point of much of the fuss in this PR is that the first name after an address has a "special meaning" in /etc/hosts.

Copy link
Member

Choose a reason for hiding this comment

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

Still, as the fqdn option was removed, we might as well simplify the implementation...

Copy link
Member

Choose a reason for hiding this comment

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

@vcunat: We could still use mkOrder 900 on [ "localhost" ] so that users can override it with mkBefore and mkAfter, or even go further and change the type of the hosts option to something like this:

types.attrsOf (submodule {
  options.canonical = mkOption { type = types.str; ... };
  options.aliases = mkOption { type = types.listOf types.str; ... };
})

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, right, that would also be a possibility.

Copy link
Member

Choose a reason for hiding this comment

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

@regellosigkeitsaxiom: A different way to handle this would be the other way around, mapping canonical hostnames to IP addresses. But either way, we can still provide our own merge function instead of the default one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can still provide our own merge function instead of the default one.

This. Although I have no idea how to implement it.

mapping canonical hostnames to IP addresses.

This is good too. Current implementation misses check that no alias mapped to two different IP addresses, but this way it can be done trivially.

In my mind, it looks like this:

{
  networking.hosts.aliases = {
    "foo.example.com" = "127.0.0.2";
    "bar.example.com" = "127.0.0.2";
  };
  networking.hosts.canonical = {
    "foobar.example.com" = "127.0.0.2";
  };
}

which will emit 127.0.0.2 foobar.example.com foo.example.com bar.example.com.

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is a bit verbose, but let me write an implementation which allows both ways (host -> ip and ip -> host(s))...

Copy link
Member

Choose a reason for hiding this comment

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

Btw. this already should merge as expected:

$ nix-instantiate nixos --eval --strict --arg configuration '{ imports = [ { networking.hosts."127.0.0.1".canonical = "aaa"; } ]; networking.hosts."127.0.0.1".canonical = "bbb"; }' -A config.networking.hosts 
error: The unique option `networking.hosts.127.0.0.1.canonical' is defined multiple times, in `<unknown-file>' and `<unknown-file>'.
(use ‘--show-trace’ to show detailed location information)
$

Copy link
Member

Choose a reason for hiding this comment

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

See #27804.

@NeQuissimus
Copy link
Member

Thank you @regellosigkeitsaxiom. Generally, deprecated things go away sooner or later, which is why I was concerned when I saw the warning :) We are all good now from what I can tell.

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

10 participants