[PATCH] conf: Don't warn if nameservers were found, but won't be advertised

[PATCH] conf: Don't warn if nameservers were found, but won't be advertised

[Stefano Brivio]

Starting from commit 3a2afde87dd1 ("conf, udp: Drop mostly duplicated
dns_send arrays, rename related fields"), we won't add to c->ip4.dns
and c->ip6.dns nameservers that can't be used by the guest or
container, and we won't advertise them.

However, the fact that we don't advertise any nameserver doesn't mean
that we didn't find any, and we should warn only if we couldn't find
any.

This is particularly relevant in case both --dns-forward and
--no-map-gw are passed, and a single loopback address is listed in
/etc/resolv.conf: we'll forward queries directed to the address
specified by --dns-forward to the loopback address we found, we
won't advertise that address, so we shouldn't warn: this is a
perfectly legitimate usage.

Reported-by: Paul Holzinger <pholzing@redhat.com>
Link: https://github.com/containers/podman/issues/19213
Fixes: 3a2afde87dd1 ("conf, udp: Drop mostly duplicated dns_send arrays, rename related fields")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 conf.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c
index 4a783b8..c50c039 100644
--- a/conf.c
+++ b/conf.c
@@ -399,6 +399,7 @@ static void get_dns(struct ctx *c)
 	int dns4_set, dns6_set, dnss_set, dns_set, fd;
 	struct fqdn *s = c->dns_search;
 	struct lineread resolvconf;
+	unsigned int added = 0;
 	char *line, *end;
 	const char *p;
 	int line_len;
@@ -427,13 +428,17 @@ static void get_dns(struct ctx *c)
 
 			if (!dns4_set &&
 			    dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) - 1
-			    && inet_pton(AF_INET, p + 1, &dns4_tmp))
+			    && inet_pton(AF_INET, p + 1, &dns4_tmp)) {
 				add_dns4(c, &dns4_tmp, &dns4);
+				added++;
+			}
 
 			if (!dns6_set &&
 			    dns6 - &c->ip6.dns[0] < ARRAY_SIZE(c->ip6.dns) - 1
-			    && inet_pton(AF_INET6, p + 1, &dns6_tmp))
+			    && inet_pton(AF_INET6, p + 1, &dns6_tmp)) {
 				add_dns6(c, &dns6_tmp, &dns6);
+				added++;
+			}
 		} else if (!dnss_set && strstr(line, "search ") == line &&
 			   s == c->dns_search) {
 			end = strpbrk(line, "\n");
@@ -459,7 +464,7 @@ static void get_dns(struct ctx *c)
 	close(fd);
 
 out:
-	if (!dns_set && dns4 == c->ip4.dns && dns6 == c->ip6.dns)
+	if (!dns_set && !added)
 		warn("Couldn't get any nameserver address");
 }
 
-- 
@@ -399,6 +399,7 @@ static void get_dns(struct ctx *c)
 	int dns4_set, dns6_set, dnss_set, dns_set, fd;
 	struct fqdn *s = c->dns_search;
 	struct lineread resolvconf;
+	unsigned int added = 0;
 	char *line, *end;
 	const char *p;
 	int line_len;
@@ -427,13 +428,17 @@ static void get_dns(struct ctx *c)
 
 			if (!dns4_set &&
 			    dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) - 1
-			    && inet_pton(AF_INET, p + 1, &dns4_tmp))
+			    && inet_pton(AF_INET, p + 1, &dns4_tmp)) {
 				add_dns4(c, &dns4_tmp, &dns4);
+				added++;
+			}
 
 			if (!dns6_set &&
 			    dns6 - &c->ip6.dns[0] < ARRAY_SIZE(c->ip6.dns) - 1
-			    && inet_pton(AF_INET6, p + 1, &dns6_tmp))
+			    && inet_pton(AF_INET6, p + 1, &dns6_tmp)) {
 				add_dns6(c, &dns6_tmp, &dns6);
+				added++;
+			}
 		} else if (!dnss_set && strstr(line, "search ") == line &&
 			   s == c->dns_search) {
 			end = strpbrk(line, "\n");
@@ -459,7 +464,7 @@ static void get_dns(struct ctx *c)
 	close(fd);
 
 out:
-	if (!dns_set && dns4 == c->ip4.dns && dns6 == c->ip6.dns)
+	if (!dns_set && !added)
 		warn("Couldn't get any nameserver address");
 }
 
-- 
2.39.2

Re: [PATCH] conf: Don't warn if nameservers were found, but won't be advertised

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3164 bytes --]

On Fri, Mar 08, 2024 at 12:25:51AM +0100, Stefano Brivio wrote:
> Starting from commit 3a2afde87dd1 ("conf, udp: Drop mostly duplicated
> dns_send arrays, rename related fields"), we won't add to c->ip4.dns
> and c->ip6.dns nameservers that can't be used by the guest or
> container, and we won't advertise them.
> 
> However, the fact that we don't advertise any nameserver doesn't mean
> that we didn't find any, and we should warn only if we couldn't find
> any.
> 
> This is particularly relevant in case both --dns-forward and
> --no-map-gw are passed, and a single loopback address is listed in
> /etc/resolv.conf: we'll forward queries directed to the address
> specified by --dns-forward to the loopback address we found, we
> won't advertise that address, so we shouldn't warn: this is a
> perfectly legitimate usage.
> 
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Link: https://github.com/containers/podman/issues/19213
> Fixes: 3a2afde87dd1 ("conf, udp: Drop mostly duplicated dns_send arrays, rename related fields")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

I don't think this is quite the right fix.  It makes sense *when*
--dns-forward is specified.  However if --dns-forward is *not*
specified, then having only localhost resolvers on the host side means
we really do have nothing the guest can use.  So I think we need to
make the behaviour explicitly conditional on the dns_match variable.
Possibly by making add_dns[46]() accept localhost addresses if
(dns_match && no_map_gw)?

> ---
>  conf.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 4a783b8..c50c039 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -399,6 +399,7 @@ static void get_dns(struct ctx *c)
>  	int dns4_set, dns6_set, dnss_set, dns_set, fd;
>  	struct fqdn *s = c->dns_search;
>  	struct lineread resolvconf;
> +	unsigned int added = 0;
>  	char *line, *end;
>  	const char *p;
>  	int line_len;
> @@ -427,13 +428,17 @@ static void get_dns(struct ctx *c)
>  
>  			if (!dns4_set &&
>  			    dns4 - &c->ip4.dns[0] < ARRAY_SIZE(c->ip4.dns) - 1
> -			    && inet_pton(AF_INET, p + 1, &dns4_tmp))
> +			    && inet_pton(AF_INET, p + 1, &dns4_tmp)) {
>  				add_dns4(c, &dns4_tmp, &dns4);
> +				added++;
> +			}
>  
>  			if (!dns6_set &&
>  			    dns6 - &c->ip6.dns[0] < ARRAY_SIZE(c->ip6.dns) - 1
> -			    && inet_pton(AF_INET6, p + 1, &dns6_tmp))
> +			    && inet_pton(AF_INET6, p + 1, &dns6_tmp)) {
>  				add_dns6(c, &dns6_tmp, &dns6);
> +				added++;
> +			}
>  		} else if (!dnss_set && strstr(line, "search ") == line &&
>  			   s == c->dns_search) {
>  			end = strpbrk(line, "\n");
> @@ -459,7 +464,7 @@ static void get_dns(struct ctx *c)
>  	close(fd);
>  
>  out:
> -	if (!dns_set && dns4 == c->ip4.dns && dns6 == c->ip6.dns)
> +	if (!dns_set && !added)
>  		warn("Couldn't get any nameserver address");
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] conf: Don't warn if nameservers were found, but won't be advertised

[Stefano Brivio]

On Fri, 8 Mar 2024 12:17:13 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Mar 08, 2024 at 12:25:51AM +0100, Stefano Brivio wrote:
> > Starting from commit 3a2afde87dd1 ("conf, udp: Drop mostly duplicated
> > dns_send arrays, rename related fields"), we won't add to c->ip4.dns
> > and c->ip6.dns nameservers that can't be used by the guest or
> > container, and we won't advertise them.
> > 
> > However, the fact that we don't advertise any nameserver doesn't mean
> > that we didn't find any, and we should warn only if we couldn't find
> > any.
> > 
> > This is particularly relevant in case both --dns-forward and
> > --no-map-gw are passed, and a single loopback address is listed in
> > /etc/resolv.conf: we'll forward queries directed to the address
> > specified by --dns-forward to the loopback address we found, we
> > won't advertise that address, so we shouldn't warn: this is a
> > perfectly legitimate usage.
> > 
> > Reported-by: Paul Holzinger <pholzing@redhat.com>
> > Link: https://github.com/containers/podman/issues/19213
> > Fixes: 3a2afde87dd1 ("conf, udp: Drop mostly duplicated dns_send arrays, rename related fields")
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> I don't think this is quite the right fix.  It makes sense *when*
> --dns-forward is specified.  However if --dns-forward is *not*
> specified, then having only localhost resolvers on the host side means
> we really do have nothing the guest can use.  So I think we need to
> make the behaviour explicitly conditional on the dns_match variable.

I was actually about to do that, then I read the text of the warning
again: "Couldn't get any nameserver address".

If there are just loopback addresses in resolv.conf, and we don't have
--dns-forward, is that claim correct? We could get them, we actually
parse them, we just don't advertise them. At the same time, we show the
user (at least without --quiet) that we don't advertise any server via
DHCP/NDP/DHCPv6: that section will be missing.

On the other hand, I guess there might be some value in giving the user
a hint if they just see name resolution failing. Maybe, if we don't use
any nameserver from resolv.conf (or from the command line), we could
say "Couldn't use any nameserver address"?

> Possibly by making add_dns[46]() accept localhost addresses if
> (dns_match && no_map_gw)?

What do you mean by "accept"? It already sets .dns_host, no matter
what. I don't think we should add loopback addresses to the list we
advertise if c->no_map_gw, because they can't be reached anyway.

Another alternative would be to automatically advertise the address
passed by --dns-forward. But the user can already specify that via
--dns, so we'd be actually losing functionality.

I was rather pondering to set .dns_host from add_dns[46]() iff it's
used (that is, if !IN6_IS_ADDR_UNSPECIFIED(&c->ip[46].dns_match) and
return some value there (maybe that's what you meant by "accept")?

Then, if any call to add_dns[46]() used any address (advertised or
mapped), we wouldn't print any warning.

I'm a bit undecided, because we'd make it more complicated for the sake
of a warning that doesn't really need to be printed anyway. But again,
it might be helpful.

-- 
Stefano

Re: [PATCH] conf: Don't warn if nameservers were found, but won't be advertised

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 5028 bytes --]

On Fri, Mar 08, 2024 at 07:05:30AM +0100, Stefano Brivio wrote:
> On Fri, 8 Mar 2024 12:17:13 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Fri, Mar 08, 2024 at 12:25:51AM +0100, Stefano Brivio wrote:
> > > Starting from commit 3a2afde87dd1 ("conf, udp: Drop mostly duplicated
> > > dns_send arrays, rename related fields"), we won't add to c->ip4.dns
> > > and c->ip6.dns nameservers that can't be used by the guest or
> > > container, and we won't advertise them.
> > > 
> > > However, the fact that we don't advertise any nameserver doesn't mean
> > > that we didn't find any, and we should warn only if we couldn't find
> > > any.
> > > 
> > > This is particularly relevant in case both --dns-forward and
> > > --no-map-gw are passed, and a single loopback address is listed in
> > > /etc/resolv.conf: we'll forward queries directed to the address
> > > specified by --dns-forward to the loopback address we found, we
> > > won't advertise that address, so we shouldn't warn: this is a
> > > perfectly legitimate usage.
> > > 
> > > Reported-by: Paul Holzinger <pholzing@redhat.com>
> > > Link: https://github.com/containers/podman/issues/19213
> > > Fixes: 3a2afde87dd1 ("conf, udp: Drop mostly duplicated dns_send arrays, rename related fields")
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > 
> > I don't think this is quite the right fix.  It makes sense *when*
> > --dns-forward is specified.  However if --dns-forward is *not*
> > specified, then having only localhost resolvers on the host side means
> > we really do have nothing the guest can use.  So I think we need to
> > make the behaviour explicitly conditional on the dns_match variable.
> 
> I was actually about to do that, then I read the text of the warning
> again: "Couldn't get any nameserver address".
> 
> If there are just loopback addresses in resolv.conf, and we don't have
> --dns-forward, is that claim correct? We could get them, we actually
> parse them, we just don't advertise them. At the same time, we show the
> user (at least without --quiet) that we don't advertise any server via
> DHCP/NDP/DHCPv6: that section will be missing.
> 
> On the other hand, I guess there might be some value in giving the user
> a hint if they just see name resolution failing. Maybe, if we don't use
> any nameserver from resolv.conf (or from the command line), we could
> say "Couldn't use any nameserver address"?

Right.  I think giving some sort of warning if we're unable to
advertise any useful nameserver to the guest is more important than
the pedantic correctness of what the message says.  Though obviously
we want to get the latter right too, ideally.

> > Possibly by making add_dns[46]() accept localhost addresses if
> > (dns_match && no_map_gw)?
> 
> What do you mean by "accept"? It already sets .dns_host, no matter
> what. I don't think we should add loopback addresses to the list we
> advertise if c->no_map_gw, because they can't be reached anyway.
> 
> Another alternative would be to automatically advertise the address
> passed by --dns-forward. But the user can already specify that via
> --dns, so we'd be actually losing functionality.

Ah.. I forgot that.  It seems weird to me that these are set
separately.  I guess that approach doesn't quite work.

What about your patch, plus a new explicit check about whether we have
something we can advertise to the guest (whether it comes from
resolv.conf or from --dns)?

> I was rather pondering to set .dns_host from add_dns[46]() iff it's
> used (that is, if !IN6_IS_ADDR_UNSPECIFIED(&c->ip[46].dns_match) and
> return some value there (maybe that's what you meant by "accept")?
> 
> Then, if any call to add_dns[46]() used any address (advertised or
> mapped), we wouldn't print any warning.

Hm, maybe.  Basically it seems to me we kind of need two different
checks: one if we have no resolvers on the host side for passt itself
to use, one if we have no resolver address we can advertise to the
guest.  Each would be suppressed in certain conditions when it's not
relevant, but those conditions are different for each check.

> I'm a bit undecided, because we'd make it more complicated for the sake
> of a warning that doesn't really need to be printed anyway. But again,
> it might be helpful.

It's a bit more of an extensive change, but a possibly conceptually
easier to understand approach would be:
	- Make dns_host an array, instead of single
	- add_dns[46]() adds things to the dns_host array, instead of
          the dns array (more or less unconditionally)
	- We generate the dns array by filtering and/or translating
	  the dns_host array, unless overridden by --dns

The two checks then become whether each of the two arrays is empty.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] conf: Don't warn if nameservers were found, but won't be advertised

[Stefano Brivio]

On Fri, 8 Mar 2024 17:33:00 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Fri, Mar 08, 2024 at 07:05:30AM +0100, Stefano Brivio wrote:
> > On Fri, 8 Mar 2024 12:17:13 +1100
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Fri, Mar 08, 2024 at 12:25:51AM +0100, Stefano Brivio wrote:  
> > > > Starting from commit 3a2afde87dd1 ("conf, udp: Drop mostly duplicated
> > > > dns_send arrays, rename related fields"), we won't add to c->ip4.dns
> > > > and c->ip6.dns nameservers that can't be used by the guest or
> > > > container, and we won't advertise them.
> > > > 
> > > > However, the fact that we don't advertise any nameserver doesn't mean
> > > > that we didn't find any, and we should warn only if we couldn't find
> > > > any.
> > > > 
> > > > This is particularly relevant in case both --dns-forward and
> > > > --no-map-gw are passed, and a single loopback address is listed in
> > > > /etc/resolv.conf: we'll forward queries directed to the address
> > > > specified by --dns-forward to the loopback address we found, we
> > > > won't advertise that address, so we shouldn't warn: this is a
> > > > perfectly legitimate usage.
> > > > 
> > > > Reported-by: Paul Holzinger <pholzing@redhat.com>
> > > > Link: https://github.com/containers/podman/issues/19213
> > > > Fixes: 3a2afde87dd1 ("conf, udp: Drop mostly duplicated dns_send arrays, rename related fields")
> > > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>    
> > > 
> > > I don't think this is quite the right fix.  It makes sense *when*
> > > --dns-forward is specified.  However if --dns-forward is *not*
> > > specified, then having only localhost resolvers on the host side means
> > > we really do have nothing the guest can use.  So I think we need to
> > > make the behaviour explicitly conditional on the dns_match variable.  
> > 
> > I was actually about to do that, then I read the text of the warning
> > again: "Couldn't get any nameserver address".
> > 
> > If there are just loopback addresses in resolv.conf, and we don't have
> > --dns-forward, is that claim correct? We could get them, we actually
> > parse them, we just don't advertise them. At the same time, we show the
> > user (at least without --quiet) that we don't advertise any server via
> > DHCP/NDP/DHCPv6: that section will be missing.
> > 
> > On the other hand, I guess there might be some value in giving the user
> > a hint if they just see name resolution failing. Maybe, if we don't use
> > any nameserver from resolv.conf (or from the command line), we could
> > say "Couldn't use any nameserver address"?  
> 
> Right.  I think giving some sort of warning if we're unable to
> advertise any useful nameserver to the guest is more important than
> the pedantic correctness of what the message says.  Though obviously
> we want to get the latter right too, ideally.
> 
> > > Possibly by making add_dns[46]() accept localhost addresses if
> > > (dns_match && no_map_gw)?  
> > 
> > What do you mean by "accept"? It already sets .dns_host, no matter
> > what. I don't think we should add loopback addresses to the list we
> > advertise if c->no_map_gw, because they can't be reached anyway.
> > 
> > Another alternative would be to automatically advertise the address
> > passed by --dns-forward. But the user can already specify that via
> > --dns, so we'd be actually losing functionality.  
> 
> Ah.. I forgot that.  It seems weird to me that these are set
> separately.  I guess that approach doesn't quite work.
> 
> What about your patch, plus a new explicit check about whether we have
> something we can advertise to the guest (whether it comes from
> resolv.conf or from --dns)?

It makes sense, yes. Actually, right now, to unblock Podman with that
issue, I would go ahead with my patch, then think of an appropriate
text for the other warning.

> > I was rather pondering to set .dns_host from add_dns[46]() iff it's
> > used (that is, if !IN6_IS_ADDR_UNSPECIFIED(&c->ip[46].dns_match) and
> > return some value there (maybe that's what you meant by "accept")?
> > 
> > Then, if any call to add_dns[46]() used any address (advertised or
> > mapped), we wouldn't print any warning.  
> 
> Hm, maybe.  Basically it seems to me we kind of need two different
> checks: one if we have no resolvers on the host side for passt itself
> to use, one if we have no resolver address we can advertise to the
> guest.  Each would be suppressed in certain conditions when it's not
> relevant, but those conditions are different for each check.

Right.

> > I'm a bit undecided, because we'd make it more complicated for the sake
> > of a warning that doesn't really need to be printed anyway. But again,
> > it might be helpful.  
> 
> It's a bit more of an extensive change, but a possibly conceptually
> easier to understand approach would be:
> 	- Make dns_host an array, instead of single
> 	- add_dns[46]() adds things to the dns_host array, instead of
>           the dns array (more or less unconditionally)
> 	- We generate the dns array by filtering and/or translating
> 	  the dns_host array, unless overridden by --dns
> 
> The two checks then become whether each of the two arrays is empty.

The only inconsistent side of this is that dns_host would be an array,
but we'd be using only one (we had this inconsistency in the past and
we solved it with commit 3a2afde87dd1).

Other than that it makes sense, but I wonder if we shouldn't rather
revisit the whole DNS mapping mechanism once we have the (complete)
flow table in place.

-- 
Stefano

Re: [PATCH] conf: Don't warn if nameservers were found, but won't be advertised

[Paul Holzinger]

Thanks Stefano

On 08/03/2024 00:25, Stefano Brivio wrote:
> Starting from commit 3a2afde87dd1 ("conf, udp: Drop mostly duplicated
> dns_send arrays, rename related fields"), we won't add to c->ip4.dns
> and c->ip6.dns nameservers that can't be used by the guest or
> container, and we won't advertise them.
>
> However, the fact that we don't advertise any nameserver doesn't mean
> that we didn't find any, and we should warn only if we couldn't find
> any.
>
> This is particularly relevant in case both --dns-forward and
> --no-map-gw are passed, and a single loopback address is listed in
> /etc/resolv.conf: we'll forward queries directed to the address
> specified by --dns-forward to the loopback address we found, we
> won't advertise that address, so we shouldn't warn: this is a
> perfectly legitimate usage.
>
> Reported-by: Paul Holzinger <pholzing@redhat.com>
> Link: https://github.com/containers/podman/issues/19213
> Fixes: 3a2afde87dd1 ("conf, udp: Drop mostly duplicated dns_send arrays, rename related fields")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Tested-by: Paul Holzinger <pholzing@redhat.com>