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

networking: Workaround 2693 by ignoring resolv.conf entries starting with dot #2694

Merged
merged 1 commit into from
Oct 8, 2024

Conversation

Geod24
Copy link
Contributor

@Geod24 Geod24 commented Mar 24, 2024

Currently, systems without a FQDN can be mishandled by facter for certain /etc/resolv.conf content. This was initially noticed when systemd-resolved was installed on a host without domain.

systemd-resolved stub resolver sets 'search .' as a search domain, which results in the following hostname/domain/fqdn triplet: foo, ., foo... See: https://github.com/systemd/systemd/blob/v255/src/resolve/resolv.conf#L19

This is wrong on multiple levels: first, facter does not seem to handle '.' (the root level) well, and there have been PRs to remove the trailing dot in FQDN as far back as 2012 (PR 200), leaving user with a convenient, but sometimes ambiguous string that is not fully qualified.

More fundamentally, facter sould not be using 'search' or 'domain' in resolv.conf to begin with, as those are client resolution options, and usually set for convenience, but are not part of the hostname. However, removing this lookup is much more involved, as facter has been doing this for years.

Hence, this commit implements a a middle ground solution to support top-level/domainless servers without making a breaking change.

@Geod24 Geod24 requested a review from a team as a code owner March 24, 2024 15:04
@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

@Geod24
Copy link
Contributor Author

Geod24 commented Apr 8, 2024

@cthorn42 any chance you could take a look at this ?

@joshcooper joshcooper added the bug Something isn't working label Apr 11, 2024
@ekohl
Copy link
Contributor

ekohl commented Apr 11, 2024

More fundamentally, facter sould not be using 'search' or 'domain' in resolv.conf to begin with, as those are client resolution options, and usually set for convenience, but are not part of the hostname. However, removing this lookup is much more involved, as facter has been doing this for years.

I don't think this is true.

Quoting man 1 hostname:

The FQDN (Fully Qualified Domain Name) of the system is the name that the resolver(3) returns for the host name, such as, ursula.example.com. It is usually the hostname followed by the DNS domain name (the part after the first dot). You can check the FQDN using hostname --fqdn or the domain name using dnsdomainname.

Quoting man 3 resolver:

The res_ninit() and res_init() functions read the configuration files (see resolv.conf(5)) to get the default domain name and name server address(es). If no server is given, the local host is tried. If no domain is given, that associated with the local host is used. It can be overridden with the environment variable LOCALDOMAIN. res_ninit() or res_init() is normally executed by the first call to one of the other functions. Every call to res_ninit() requires a corresponding call to res_nclose() to free memory allocated by res_ninit() and subsequent calls to res_nquery().

So on Linux libc can certainly use it.

However, removing this lookup is much more involved, as facter has been doing this for years.

You could replace it with a native implementation. Quoting man 1 hostname again:

Technically: The FQDN is the name getaddrinfo(3) returns for the host name returned by gethostname(2). The
DNS domain name is the part after the first dot.

I think that means you can use:

Socket.getaddrinfo(Socket.gethostname, nil, nil, nil, nil, Socket::AI_CANONNAME)

@h0tw1r3
Copy link
Contributor

h0tw1r3 commented Apr 12, 2024

I think most sys admins would expect puppet's fqdn / hostname to return exactly what the hostname command does.

This is a quick port of the standard linux hostname C code with the addition of falling back to parsing /etc/resolv.conf.

#!/opt/puppetlabs/puppet/bin/ruby

require 'socket'

def fqdn
	# Addrinfo.getaddrinfo
	result = Addrinfo.getaddrinfo(Socket.gethostname, nil, nil, Socket::SOCK_DGRAM, nil, Socket::AI_CANONNAME)
		.map { |a| a.canonname }
		.find { |n| !n.nil? and n.include? '.' }

	return result if !result.nil?

	# Socket.getnameinfo
	result = Socket.getifaddrs().filter_map do |ifap|
		next unless ifap.addr
		next unless (ifap.flags & Socket::IFF_LOOPBACK) == 0
		next if (ifap.flags & Socket::IFF_UP) == 0

		family = ifap.addr.afamily
		next unless (family == Socket::AF_INET || family == Socket::AF_INET6)

		next if family == Socket::AF_INET6 && ifap.addr.ipv6_linklocal?

		n = Socket.getnameinfo(ifap.addr, Socket::NI_NAMEREQD)
		n[0] if n and !n[0].nil? and n[0].include? '.'
	end.first

	return result if !result.nil?

	# Fallback to parsing /etc/resolv.conf
	content = File.read('/etc/resolv.conf', chomp: true)
	domain = if content =~ /^domain\s+(\S+)/
			 Regexp.last_match[1]
		 elsif content =~ /^search\s+(\S+)/
			 Regexp.last_match[1]
		 end
	# strip leading and trailing .
	domain.sub!(/^\.?(.*?)\.?$/, '\1') if domain

	result = [Socket.gethostname]
	result.append(domain) if domain and domain.length > 1

	result.join('.')
end

@b4ldr
Copy link
Contributor

b4ldr commented May 24, 2024

More fundamentally, facter sould not be using 'search' or 'domain' in resolv.conf to begin with, as those are client resolution options, and usually set for convenience, but are not part of the hostname. However, removing this lookup is much more involved, as facter has been doing this for years.

I don't think this is true.

I think this is definitely true for the search path but i agree using the domain part seems reasonable if there is no better info

@joshcooper
Copy link
Contributor

Thanks for your PR @Geod24!

I think the "facter has been doing this wrong all this time" issue is valid, but a different issue that would need to be addressed in Facter 5. Thanks @ekohl and @h0tw1r3 for the sample code.

Your PR makes sense to me. To get this merged could you:

  1. Remove the "More fundamentally, ..." comment from your commit message based on the discussion in this PR?

  2. Facter appears to be missing test coverage for the case where search contains multiple entries. From man 5 resolver:

       search Search list for host-name lookup.
              By  default,  the search list contains one entry, the local domain name. 

Note this isn't true for domain

              The domain directive is an obsolete name for the search directive that handles one search list entry only.

Could you add a test case for that?

  1. Facter also assumes there's only one search directive (and reads the first one that matches). However, there may be multiple directives:
              If there are multiple search directives, only the search list from the last instance is used.

I'd be fine filing a separate issue for this.

@Geod24 Geod24 force-pushed the mlang/workaround2693 branch from a60d5ac to 3bffeff Compare September 9, 2024 11:16
@Geod24
Copy link
Contributor Author

Geod24 commented Sep 9, 2024

Facter appears to be missing test coverage for the case where search contains multiple entries. From man 5 resolver:

What is the desired outcome here ?
If resolv.conf has search foo.bar example.com, do we assume the first entry or the last entry is the domain, or do we error out ?
Does this selection mechanism gets affected by what the entry is ? For example, if we pick the first entry, would we not do so if the first entry is . ?

In the meantime, I have rebased the commit and amended the message to remove the aforementioned section.

@AriaXLi
Copy link
Contributor

AriaXLi commented Sep 17, 2024

Hi @Geod24, thank you for your patience!

Regarding your questions:

If resolv.conf has search foo.bar example.com, do we assume the first entry or the last entry is the domain, or do we error out ?

We should assume the resolver will pick the first entry and should not error out

Does this selection mechanism gets affected by what the entry is ? For example, if we pick the first entry, would we not do so if the first entry is . ?

If the first entry is . followed by something else, the resolver should pick .

Let me know if anything is unclear or if you have any other questions, thank you!

@AriaXLi
Copy link
Contributor

AriaXLi commented Sep 30, 2024

This PR addresses issue #2693.

@AriaXLi AriaXLi force-pushed the mlang/workaround2693 branch 3 times, most recently from dc0056f to d337b9c Compare October 3, 2024 19:04
@AriaXLi AriaXLi force-pushed the mlang/workaround2693 branch from d337b9c to b8cf55f Compare October 4, 2024 16:11
…with dot

Currently, systems without a FQDN can be mishandled by facter for certain `/etc/resolv.conf` content.
This was initially noticed when systemd-resolved was installed on a host without domain.

systemd-resolved stub resolver sets 'search .' as a search domain, which results in the
following hostname/domain/fqdn triplet: foo, ., foo...
See: https://github.com/systemd/systemd/blob/v255/src/resolve/resolv.conf#L19

This is wrong on multiple levels: first, facter does not seem to handle '.' (the root level)
well, and there have been PRs to remove the trailing dot in FQDN as far back as 2012 (PR 200),
leaving user with a convenient, but sometimes ambiguous string that is not fully qualified.

This commit implements a a middle ground solution to support top-level/domainless servers
without making a breaking change. Additionally, it adds more coverage for various search
cases.
@AriaXLi AriaXLi force-pushed the mlang/workaround2693 branch from b8cf55f to 0181bb7 Compare October 4, 2024 16:16
@AriaXLi AriaXLi merged commit 5ab43ea into puppetlabs:main Oct 8, 2024
18 checks passed
@Geod24 Geod24 deleted the mlang/workaround2693 branch October 8, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong networking.fqdn in the presence of search . in /etc/resolv.conf
7 participants