[PATCH 0/2] Fix nss/tst-nss-files-hosts-long on single-stack hosts (bug 24816)

[PATCH 0/2] Fix nss/tst-nss-files-hosts-long on single-stack hosts (bug 24816)

[Florian Weimer]

Our Fedora builders started running the container tests (after the
switch to systemd-nspawn), and we encountered this test failure as well.
Fix this by disabling address configuration in the getent tool.

Tested on x86_64-linux-gnu.

Florian Weimer (2):
  nss: Implement --no-addrconfig option for getent
  nss: Fix tst-nss-files-hosts-long on single-stack hosts (bug 24816)

 NEWS                           |  5 ++++-
 nss/getent.c                   | 11 ++++++++++-
 nss/tst-nss-files-hosts-long.c |  9 +++++----
 3 files changed, 19 insertions(+), 6 deletions(-)


base-commit: f278835f594740f5913001430641cf1da4878670
-- 
2.37.2

[PATCH 1/2] nss: Implement --no-addrconfig option for getent

[Florian Weimer]

The ahosts, ahostsv4, ahostsv6 commands unconditionally pass
AI_ADDRCONFIG to getaddrinfo, which is not always desired.
---
 NEWS         |  5 ++++-
 nss/getent.c | 11 ++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/NEWS b/NEWS
index ef274d1a42..d4739d93c6 100644
--- a/NEWS
+++ b/NEWS
@@ -9,7 +9,10 @@ Version 2.37
 
 Major new features:
 
-  [Add new features here]
+* The getent tool now supports the --no-addrconfig option.  When
+  present, getent no longer passes AI_ADDRCONFIG to the getaddrinfo
+  function, and the output may contain addresses of families not
+  configured on the current host.
 
 Deprecated and removed features, and other changes affecting compatibility:
 
diff --git a/nss/getent.c b/nss/getent.c
index 8178b4b470..39a42e707d 100644
--- a/nss/getent.c
+++ b/nss/getent.c
@@ -58,6 +58,8 @@ static const struct argp_option args_options[] =
   {
     { "service", 's', N_("CONFIG"), 0, N_("Service configuration to be used") },
     { "no-idn", 'i', NULL, 0, N_("disable IDN encoding") },
+    { "no-addrconfig", 'A', NULL, 0,
+      N_("disable AI_ADDRCONFIG (for ahosts*)") },
     { NULL, 0, NULL, 0, NULL },
   };
 
@@ -79,6 +81,9 @@ static struct argp argp =
 /* Additional getaddrinfo flags for IDN encoding.  */
 static int idn_flags = AI_IDN | AI_CANONIDN;
 
+/* Set to 0 by --no-addrconfig.  */
+static int addrconfig_flags = AI_ADDRCONFIG;
+
 /* Print the version information.  */
 static void
 print_version (FILE *stream, struct argp_state *state)
@@ -346,7 +351,7 @@ ahosts_keys_int (int af, int xflags, int number, char *key[])
 
   struct addrinfo hint;
   memset (&hint, '\0', sizeof (hint));
-  hint.ai_flags = (AI_V4MAPPED | AI_ADDRCONFIG | AI_CANONNAME
+  hint.ai_flags = (AI_V4MAPPED | addrconfig_flags | AI_CANONNAME
 		   | idn_flags | xflags);
   hint.ai_family = af;
 
@@ -905,6 +910,10 @@ parse_option (int key, char *arg, struct argp_state *state)
       idn_flags = 0;
       break;
 
+    case 'A':
+      addrconfig_flags = 0;
+      break;
+
     default:
       return ARGP_ERR_UNKNOWN;
     }
-- 
2.37.2

Re: [PATCH 1/2] nss: Implement --no-addrconfig option for getent

[Carlos O'Donell]

On Tue, Sep 13, 2022 at 04:35:44PM +0200, Florian Weimer via Libc-alpha wrote:
> The ahosts, ahostsv4, ahostsv6 commands unconditionally pass
> AI_ADDRCONFIG to getaddrinfo, which is not always desired.

Looking forward to a v2.

> ---
>  NEWS         |  5 ++++-
>  nss/getent.c | 11 ++++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/NEWS b/NEWS
> index ef274d1a42..d4739d93c6 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,7 +9,10 @@ Version 2.37
>  
>  Major new features:
>  
> -  [Add new features here]

Suggest:

* The getent tool now supports the --no-addrconfig option. The
  output of getent with --no-addrconfig may contain addresses
  of families not configured on the current host i.e. as-if you
  had not passed AI_ADDRCONFIG to getaddrinfo calls.  This flag
  is primarily intended for diagnostic purposes.

I think the use for diagnostics should be called out.

> +* The getent tool now supports the --no-addrconfig option.  When
> +  present, getent no longer passes AI_ADDRCONFIG to the getaddrinfo
> +  function, and the output may contain addresses of families not
> +  configured on the current host.
>  
>  Deprecated and removed features, and other changes affecting compatibility:
>  
> diff --git a/nss/getent.c b/nss/getent.c
> index 8178b4b470..39a42e707d 100644
> --- a/nss/getent.c
> +++ b/nss/getent.c
> @@ -58,6 +58,8 @@ static const struct argp_option args_options[] =
>    {
>      { "service", 's', N_("CONFIG"), 0, N_("Service configuration to be used") },
>      { "no-idn", 'i', NULL, 0, N_("disable IDN encoding") },
> +    { "no-addrconfig", 'A', NULL, 0,
> +      N_("disable AI_ADDRCONFIG (for ahosts*)") },

Suggest a more user-center textual description:

"disable filtering using host address configuration (for ahosts*)"

It's a bit of a mouthfull.

>      { NULL, 0, NULL, 0, NULL },
>    };
>  
> @@ -79,6 +81,9 @@ static struct argp argp =
>  /* Additional getaddrinfo flags for IDN encoding.  */
>  static int idn_flags = AI_IDN | AI_CANONIDN;
>  
> +/* Set to 0 by --no-addrconfig.  */
> +static int addrconfig_flags = AI_ADDRCONFIG;
> +

OK.

>  /* Print the version information.  */
>  static void
>  print_version (FILE *stream, struct argp_state *state)
> @@ -346,7 +351,7 @@ ahosts_keys_int (int af, int xflags, int number, char *key[])
>  
>    struct addrinfo hint;
>    memset (&hint, '\0', sizeof (hint));
> -  hint.ai_flags = (AI_V4MAPPED | AI_ADDRCONFIG | AI_CANONNAME
> +  hint.ai_flags = (AI_V4MAPPED | addrconfig_flags | AI_CANONNAME
>  		   | idn_flags | xflags);

OK.

>    hint.ai_family = af;
>  
> @@ -905,6 +910,10 @@ parse_option (int key, char *arg, struct argp_state *state)
>        idn_flags = 0;
>        break;
>  
> +    case 'A':
> +      addrconfig_flags = 0;
> +      break;

OK.

> +
>      default:
>        return ARGP_ERR_UNKNOWN;
>      }
> -- 
> 2.37.2
> 
> 

Re: [PATCH 1/2] nss: Implement --no-addrconfig option for getent

[Florian Weimer]

* Carlos O'Donell:

> On Tue, Sep 13, 2022 at 04:35:44PM +0200, Florian Weimer via Libc-alpha wrote:
>> The ahosts, ahostsv4, ahostsv6 commands unconditionally pass
>> AI_ADDRCONFIG to getaddrinfo, which is not always desired.
>
> Looking forward to a v2.
>
>> ---
>>  NEWS         |  5 ++++-
>>  nss/getent.c | 11 ++++++++++-
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>> 
>> diff --git a/NEWS b/NEWS
>> index ef274d1a42..d4739d93c6 100644
>> --- a/NEWS
>> +++ b/NEWS
>> @@ -9,7 +9,10 @@ Version 2.37
>>  
>>  Major new features:
>>  
>> -  [Add new features here]
>
> Suggest:
>
> * The getent tool now supports the --no-addrconfig option. The
>   output of getent with --no-addrconfig may contain addresses
>   of families not configured on the current host i.e. as-if you
>   had not passed AI_ADDRCONFIG to getaddrinfo calls.  This flag
>   is primarily intended for diagnostic purposes.
>
> I think the use for diagnostics should be called out.

I think getent ahosts* is mostly intended for diagnostic purposes
anyway, so we don't have to spell this out again.  The output format is
certainly awkward for most purposes.  So I'll use your text without the
last sentence.

>> +* The getent tool now supports the --no-addrconfig option.  When
>> +  present, getent no longer passes AI_ADDRCONFIG to the getaddrinfo
>> +  function, and the output may contain addresses of families not
>> +  configured on the current host.
>>  
>>  Deprecated and removed features, and other changes affecting compatibility:
>>  
>> diff --git a/nss/getent.c b/nss/getent.c
>> index 8178b4b470..39a42e707d 100644
>> --- a/nss/getent.c
>> +++ b/nss/getent.c
>> @@ -58,6 +58,8 @@ static const struct argp_option args_options[] =
>>    {
>>      { "service", 's', N_("CONFIG"), 0, N_("Service configuration to be used") },
>>      { "no-idn", 'i', NULL, 0, N_("disable IDN encoding") },
>> +    { "no-addrconfig", 'A', NULL, 0,
>> +      N_("disable AI_ADDRCONFIG (for ahosts*)") },
>
> Suggest a more user-center textual description:
>
> "disable filtering using host address configuration (for ahosts*)"
>
> It's a bit of a mouthfull.

I'm going with this instead:

      N_("do not filter out unsupported IPv4/IPv6 addresses (with ahosts*)") },

Thanks,
Florian

[PATCH 2/2] nss: Fix tst-nss-files-hosts-long on single-stack hosts (bug 24816)

[Florian Weimer]

getent implicitly passes AI_ADDRCONFIG to getaddrinfo by default.
Use --no-addrconfig to suppress that, so that both IPv4 and IPv6
lookups succeed even if the address family is not supported by the
host.
---
 nss/tst-nss-files-hosts-long.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/nss/tst-nss-files-hosts-long.c b/nss/tst-nss-files-hosts-long.c
index 3942cf5fca..a7697e3143 100644
--- a/nss/tst-nss-files-hosts-long.c
+++ b/nss/tst-nss-files-hosts-long.c
@@ -28,14 +28,15 @@ do_test (void)
 {
   int ret;
 
-  /* Run getent to fetch the IPv4 address for host test4.
-     This forces /etc/hosts to be parsed.  */
-  ret = system("getent ahostsv4 test4");
+  /* Run getent to fetch the IPv4 address for host test4.  This forces
+     /etc/hosts to be parsed.  Use --no-addrconfig to return addresses
+     even in an IPv6-only environment.  */
+  ret = system("getent --no-addrconfig ahostsv4 test4");
   if (ret != 0)
     FAIL_EXIT1("ahostsv4 failed");
 
   /* Likewise for IPv6.  */
-  ret = system("getent ahostsv6 test6");
+  ret = system("getent --no-addrconfig  ahostsv6 test6");
   if (ret != 0)
     FAIL_EXIT1("ahostsv6 failed");
 
-- 
2.37.2

Re: [PATCH 2/2] nss: Fix tst-nss-files-hosts-long on single-stack hosts (bug 24816)

[Carlos O'Donell]

On Tue, Sep 13, 2022 at 04:35:48PM +0200, Florian Weimer via Libc-alpha wrote:
> getent implicitly passes AI_ADDRCONFIG to getaddrinfo by default.
> Use --no-addrconfig to suppress that, so that both IPv4 and IPv6
> lookups succeed even if the address family is not supported by the
> host.

LGTM.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  nss/tst-nss-files-hosts-long.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/nss/tst-nss-files-hosts-long.c b/nss/tst-nss-files-hosts-long.c
> index 3942cf5fca..a7697e3143 100644
> --- a/nss/tst-nss-files-hosts-long.c
> +++ b/nss/tst-nss-files-hosts-long.c
> @@ -28,14 +28,15 @@ do_test (void)
>  {
>    int ret;
>  
> -  /* Run getent to fetch the IPv4 address for host test4.
> -     This forces /etc/hosts to be parsed.  */
> -  ret = system("getent ahostsv4 test4");
> +  /* Run getent to fetch the IPv4 address for host test4.  This forces
> +     /etc/hosts to be parsed.  Use --no-addrconfig to return addresses
> +     even in an IPv6-only environment.  */
> +  ret = system("getent --no-addrconfig ahostsv4 test4");

OK.

>    if (ret != 0)
>      FAIL_EXIT1("ahostsv4 failed");
>  
>    /* Likewise for IPv6.  */
> -  ret = system("getent ahostsv6 test6");
> +  ret = system("getent --no-addrconfig  ahostsv6 test6");

OK.

>    if (ret != 0)
>      FAIL_EXIT1("ahostsv6 failed");
>  
> -- 
> 2.37.2
> 

Re: [PATCH 0/2] Fix nss/tst-nss-files-hosts-long on single-stack hosts (bug 24816)

[Carlos O'Donell]

On Tue, Sep 13, 2022 at 04:35:39PM +0200, Florian Weimer via Libc-alpha wrote:
> Our Fedora builders started running the container tests (after the
> switch to systemd-nspawn), and we encountered this test failure as well.
> Fix this by disabling address configuration in the getent tool.

Two things I'd like to discuss.

(1) Change the getent default and drop AI_ADDRCONFIG.

I'm hesitant to add a new option to getent as a solution to a testing
problem. The documented description for getent ahosts talks only
about enumerating the host entries or calling getaddrinfo with
AF_UNSPEC. Could we just change the default and ignore the host
configuration? This is less conservative but logically it seems to me
that we could just drop AI_ADDRCONFIG, and add a --addrconfig option to
get back the old behaviour. What could we possibly break?

(2) Fix the test.

Alternatively the test should be checking to see if it is in a dual
stack environment or single stack environment and only call getent for
the specific case when such interfaces are enabled.

Can we resolve this entirely in tst-nss-files-hosts-long?

> Tested on x86_64-linux-gnu.
> 
> Florian Weimer (2):
>   nss: Implement --no-addrconfig option for getent
>   nss: Fix tst-nss-files-hosts-long on single-stack hosts (bug 24816)
> 
>  NEWS                           |  5 ++++-
>  nss/getent.c                   | 11 ++++++++++-
>  nss/tst-nss-files-hosts-long.c |  9 +++++----
>  3 files changed, 19 insertions(+), 6 deletions(-)
> 
> 
> base-commit: f278835f594740f5913001430641cf1da4878670
> -- 
> 2.37.2
> 

Re: [PATCH 0/2] Fix nss/tst-nss-files-hosts-long on single-stack hosts (bug 24816)

[Florian Weimer]

* Carlos O'Donell:

> On Tue, Sep 13, 2022 at 04:35:39PM +0200, Florian Weimer via Libc-alpha wrote:
>> Our Fedora builders started running the container tests (after the
>> switch to systemd-nspawn), and we encountered this test failure as well.
>> Fix this by disabling address configuration in the getent tool.
>
> Two things I'd like to discuss.
>
> (1) Change the getent default and drop AI_ADDRCONFIG.
>
> I'm hesitant to add a new option to getent as a solution to a testing
> problem. The documented description for getent ahosts talks only
> about enumerating the host entries or calling getaddrinfo with
> AF_UNSPEC. Could we just change the default and ignore the host
> configuration? This is less conservative but logically it seems to me
> that we could just drop AI_ADDRCONFIG, and add a --addrconfig option to
> get back the old behaviour. What could we possibly break?

I'm not sure why we would make such a backwards-incompatible change just
to fix a test.  It sounds even more preposterous than adding the new
option.

There have been support cases where the --no-addrconfig option would
have been useful.  Today, getent isn't a great tool for diagnosing DNS
issues, and I think this option improves the situation slightly.

> (2) Fix the test.
>
> Alternatively the test should be checking to see if it is in a dual
> stack environment or single stack environment and only call getent for
> the specific case when such interfaces are enabled.
>
> Can we resolve this entirely in tst-nss-files-hosts-long?

I think it's futile to try to replicate the AI_ADDRCONFIG behavior in
the test.

Thanks,
Florian

Re: [PATCH 0/2] Fix nss/tst-nss-files-hosts-long on single-stack hosts (bug 24816)

[Carlos O'Donell]

On Wed, Sep 14, 2022 at 11:54:47AM +0200, Florian Weimer wrote:
> * Carlos O'Donell:
> 
> > On Tue, Sep 13, 2022 at 04:35:39PM +0200, Florian Weimer via Libc-alpha wrote:
> >> Our Fedora builders started running the container tests (after the
> >> switch to systemd-nspawn), and we encountered this test failure as well.
> >> Fix this by disabling address configuration in the getent tool.
> >
> > Two things I'd like to discuss.
> >
> > (1) Change the getent default and drop AI_ADDRCONFIG.
> >
> > I'm hesitant to add a new option to getent as a solution to a testing
> > problem. The documented description for getent ahosts talks only
> > about enumerating the host entries or calling getaddrinfo with
> > AF_UNSPEC. Could we just change the default and ignore the host
> > configuration? This is less conservative but logically it seems to me
> > that we could just drop AI_ADDRCONFIG, and add a --addrconfig option to
> > get back the old behaviour. What could we possibly break?
> 
> I'm not sure why we would make such a backwards-incompatible change just
> to fix a test.  It sounds even more preposterous than adding the new
> option.

I fully agree that the most backwards compatible change is to add
an option that allows getent to operate without AI_ADDRCONFIG.

What I want to explore here is: Why use AI_ADDRCONFIG at all with
getent?

If our collective answer is: Because that's just the way we've always
done it and changing it would be a backwards incompatible change, then
I'm fine with that. I just wanted to explore that a bit.

I can see arguments both ways. I was looking for your opinion here.
My read of your opinion is that we should make the minimum backwards
compatible change.

> There have been support cases where the --no-addrconfig option would
> have been useful.  Today, getent isn't a great tool for diagnosing DNS
> issues, and I think this option improves the situation slightly.

That's a good point in favour of the new option.

> > (2) Fix the test.
> >
> > Alternatively the test should be checking to see if it is in a dual
> > stack environment or single stack environment and only call getent for
> > the specific case when such interfaces are enabled.
> >
> > Can we resolve this entirely in tst-nss-files-hosts-long?
> 
> I think it's futile to try to replicate the AI_ADDRCONFIG behavior in
> the test.

I did an audit and it looks like getent, and this specific test are
the only ones that we'd need cood like this for, and so there isn't
a win-win here with other tests.

Cheers,
Carlos.

Re: [PATCH 0/2] Fix nss/tst-nss-files-hosts-long on single-stack hosts (bug 24816)

[Florian Weimer]

* Carlos O'Donell:

> On Wed, Sep 14, 2022 at 11:54:47AM +0200, Florian Weimer wrote:
>> * Carlos O'Donell:
>> 
>> > On Tue, Sep 13, 2022 at 04:35:39PM +0200, Florian Weimer via Libc-alpha wrote:
>> >> Our Fedora builders started running the container tests (after the
>> >> switch to systemd-nspawn), and we encountered this test failure as well.
>> >> Fix this by disabling address configuration in the getent tool.
>> >
>> > Two things I'd like to discuss.
>> >
>> > (1) Change the getent default and drop AI_ADDRCONFIG.
>> >
>> > I'm hesitant to add a new option to getent as a solution to a testing
>> > problem. The documented description for getent ahosts talks only
>> > about enumerating the host entries or calling getaddrinfo with
>> > AF_UNSPEC. Could we just change the default and ignore the host
>> > configuration? This is less conservative but logically it seems to me
>> > that we could just drop AI_ADDRCONFIG, and add a --addrconfig option to
>> > get back the old behaviour. What could we possibly break?
>> 
>> I'm not sure why we would make such a backwards-incompatible change just
>> to fix a test.  It sounds even more preposterous than adding the new
>> option.
>
> I fully agree that the most backwards compatible change is to add
> an option that allows getent to operate without AI_ADDRCONFIG.
>
> What I want to explore here is: Why use AI_ADDRCONFIG at all with
> getent?

I think it dates back when it was assumed that AI_ADDRCONFIG was useful.
Back then, it looked like that if your system had IPv6 addresses
configured, it could reach the entire IPv6 Internet.

In practice, what applications should do is to get all addresses,
always, and make connections in parallel over both address families.
Filtering out addresses that are clearly unusable is just an
optimization (but glibc isn't very good at that, getting the IPv6
support status of a host the way we do it can be very expensive, or give
inaccurate results over time).

> If our collective answer is: Because that's just the way we've always
> done it and changing it would be a backwards incompatible change, then
> I'm fine with that. I just wanted to explore that a bit.

Yeah, that too.  getent ahosts is not really useful because of the
address duplication.

Thanks,
Florian