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

fallback hostname fixes #5366

Merged
merged 3 commits into from
Feb 17, 2017
Merged

Conversation

poettering
Copy link
Member

A follow-up for #5175, actually enforcing the fallback hostname also for resolved, and for the hostname PID1 sets if none is configured.

Fixes: #5253.

@poettering
Copy link
Member Author

Hmm,

Found container virtualization none.
Assertion 'imp.fd >= 0' failed at ../src/test/test-journal-importer.c:43, function test_basic_parsing(). Aborting.
FAIL: test-journal-importer (code: 134)
====== test-acl-util =======

CI fialure appears unrelated... I wonder what's going on there...

@evverx
Copy link
Member

evverx commented Feb 16, 2017

I wonder what's going on there...

This is the result of #5298

This

diff --git a/Makefile.am b/Makefile.am
index 10839e9..a70ebe6 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2469,7 +2469,7 @@ test_journal_importer_SOURCES = \
 test_journal_importer_LDADD = \
        libsystemd-shared.la

-EXTRA_DIST += \
+TEST_DATA_FILES += \
        test/journal-data/journal-1.txt \
        test/journal-data/journal-2.txt

fixes that test

evverx added a commit to evverx/systemd that referenced this pull request Feb 16, 2017
Fixes:
```
Found container virtualization none.
Assertion 'imp.fd >= 0' failed at ../src/test/test-journal-importer.c:43, function test_basic_parsing(). Aborting.
FAIL: test-journal-importer (code: 134)
```
See systemd#5366 (comment)
@martinpitt
Copy link
Contributor

Right, this started failing as PR #4526 was merged with test failures (that introduced the new test journal test files).

@martinpitt
Copy link
Contributor

Defaulting to "localhost" and sending "linux" out to the local network seems really arbitrary and inconsistent to me. I see how resolving "localhost" over mdns/llmnr is actively dangerous, but if multiple machines in a network are all called "linux" that's not really much better.

Forgive the naïve question, but is it an option to simply not send a host name at all in this case? If you don't set one, I don't think it's reasonable to expect being able to use and resolve a host name.

(Tests are fixed, btw, I retried them.)

@poettering
Copy link
Member Author

but if multiple machines in a network are all called "linux" that's not really much better.

well, it means the collision logic ensures that one of the systems will pick a different name. both llmnr and mdns have strategies to deal with that in place.

In the case of mdns this is particular important, as dns-sd services refer to mdns hostnames, and while hostnames might conflict service names might not, but in order not to break them you need the hostnames. Now, we currently don't implement service registration in resolved, but the goal definitely is to, and we shouldn't really alter behaviour if we gain support for them eventually.

So yeah, they might not be overly useful as long as humans want to identify machines in the network, but for stuff built on this we need them, it's the way mDNS/DNS-SD is defined...

Hope that makes sense...

…tname_malloc()

Currently, if the hostname is not set gethostname_malloc() defaults to
the "sysname", which is "linux" on Linux. Let's change that to also
honour the compile-time fallback hostname as specified on the configure
command line.
…ostname

When /etc/hostname isn't set, default to the configured compile-time
fallback hostname instead of "localhost" for the kernel hostname.
This changes resolved to use the compile-time fallback hostname the
configured one is not set. Note that if the local hostname is set to
"localhost" then we'll instead default to "linux" here, as for
mDNS/LLMNR exposing "localhost" is actively dangerous.
Copy link
Contributor

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation; I assumed that a hostname was mandatory for mDNS, but I had to ask.. :-)

@martinpitt martinpitt added the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Feb 17, 2017
@poettering
Copy link
Member Author

ci passed, merging

@poettering poettering merged commit ea2aa03 into systemd:master Feb 17, 2017
@keszybz
Copy link
Member

keszybz commented Feb 19, 2017

FTR, looks good. I booted without /etc/hostname now, and everything seems to behave as expected. Thanks for fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed pid1 resolve util-lib
Development

Successfully merging this pull request may close these issues.

#5175 doesn't look right (configurable build-time default hostname)
4 participants