Gather more diagnostics, and prepare skipping flaky DNS tests

[antonfirsov]

Test failures in #1488 and #27622 are suspected to originate from infrastructure/OS/configuration issues, we assume that the underlying OS calls should also fail.

This PR adds a utility to detect these failures and depending on a parameter:

1. Either skip the test
2. Or make the tests fail with detailed diagnostics

The plan is to first go with option 2. everywhere, then raise individual issues on possible new test failures with the current infrastructure. After collecting sufficient information, we can switch individual tests to go with option 1.

Fixes #1488, closes #27622 (already fixed)

Related past discussions: #28324, #16549

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[antonfirsov]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[stephentoub]

All of this P/Invoking is unlikely to be portable across various Unix implementations. It's why we use C shims as an intermediary for the production source.

[wfurt]

I was suggesting to use gethostbyname() as it takes string returns NULL or result so it should be easy to check and be portable IMHO. We chat about it with @antonfirsov and the goal is to check if system can resolve it self without depending on production code we are trying to test. We do not need to process the answer in any detailed way. We could still move it to Interop to make it more portable.

[antonfirsov]

I was suggesting to use gethostbyname()

@wfurt the main reason for choosing freeaddrinfo was because of gethostbyname being marked obsolete. Also: freeaddrinfo provides a trivial way to also get the error code, which I find useful.

All of this P/Invoking is unlikely to be portable across various Unix implementations.

@stephentoub while this is true in general, I don't see any reason to worry about it in this particular case. The addrinfo structure seems binary compatible across unixes, and the lines responsible for the query in our own PAL code are also portable.

[stephentoub]

The addrinfo structure seems binary compatible across unixes

We've seen numerous cases where the actual layout differed in header files from what was shown in the man pages, including but not limited to when targeting 32-bit vs 64-bit.

and the lines responsible for the query in our own PAL code are also portable.

Those are compiled by the C compiler, which sees the actual header files. That's the primary reason the native shims exist.

[davidsh]

I also agree that we should not p/invoke directly but only use native C shims.

[wfurt]

Yes, I understand that @antonfirsov. But it gets you out of the structure business. Since it has pointers, the size will differ. socklen_t seems to be fixed to 32bit but I'm not sure if that is mandatory for al OSes.
Shim as @stephentoub suggested would be portable path.

[antonfirsov]

OK, I understand the issue now.

From the various options I see, the simplest and safest one seems to be the use of gethostbyname as @wfurt suggested. Looks like there are several existing cases where we P/Invoke into similar simple methods in test code. Is there any strong counter-argument against using gethostbyname?

[stephentoub]

If you mean the:

struct hostent *gethostbyname(const char *name);

function, and we treat the return as an opaque pointer, e.g.

public static extern IntPtr gethostbyname(string name);

I'm ok with that.

[antonfirsov]

The configuration net5.0-OSX-Debug-x64-CoreCLR_release-OSX.1014.Amd64.Open is failing to resolve it's own hostname returned by gethostname() with the following info being printed:

System.Exception : Failed to resolve 'dci-mac-build-167'! \nDns.GetHostName() == dci-mac-build-167
--- /etc/hosts ---
##
# Host Database
#
# localhost is used to configure the loopback interface
# when the system is booting. Do not change this entry.
##
127.0.0.1   localhost
255.255.255.255    broadcasthost
::1 localhost
--- /etc/resolv.conf ---
#
# macOS Notice
#
# This file is not consulted for DNS hostname resolution, address
# resolution, or the DNS query routing mechanism used by most
# processes on this system.
#
# To view the DNS configuration used by this system, use:
# scutil --dns
#
# SEE ALSO
# dns-sd(1), scutil(8)
#
# This file is automatically generated.
#
domain redmond.corp.microsoft.com
nameserver 10.50.50.50
nameserver 10.50.10.50
------

@wfurt do you think there's anything useful here to address the issue at infrastructure level?

[wfurt]

This is not too surprising and I cannot resolve 'dci-mac-build-167' from corp net either.
I think it may be on different domain or subnet. That would probably be true for Azure machines as well. The reason why Linux work is that in most cases installer puts name into /etc/host.
But that is not requirement AFAIK.

cc: @MattGal

[MattGal]

This is not too surprising and I cannot resolve 'dci-mac-build-167' from corp net either.
I think it may be on different domain or subnet. That would probably be true for Azure machines as well. The reason why Linux work is that in most cases installer puts name into /etc/host.
But that is not requirement AFAIK.

cc: @MattGal

This tracks with my understanding of the way these machines are meant to be configured, and I thought was why we use localhost / 127.0.01 for such testing. To add one thing, our MacOS machines are not meant to even be accessible from corpnet (they're on a special walled-off version of it) and instead are generally accessed via KVM.

[antonfirsov]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

After having some discussions offline, it turned out that there is space for improvement in the behavior of our product code.

Problem

There's no guarantee that getaddrinfo() (or gethostbyname()) can resolve the systems's own hostname (returned by gethostname()) (unless it has been added manually to /etc/hosts). On CI we have a case where the behavior depends on macOS version: works for 10.13, fails for 10.14.

Currently we have a strict validation logic in the related PAL code:

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Lines 285 to 289 in 3fda6ef

	int result = getaddrinfo((const char*)address, NULL, &hint, &info);
	if (result != 0)
	{
	return ConvertGetAddrInfoAndGetNameInfoErrorsToPal(result);
	}

This may lead to failing Dns.GetHostByName("") or Dns.GetHostEntry("") calls on some systems, which is an unexpected behavior (bug?), that does not comply with our documentation:

If an empty string is passed as the hostNameOrAddress argument, then this method returns the IPv4 and IPv6 addresses of the local host.

Suggestion

We already have a logic handling hostname as a special case and returning local IP's thanks to dotnet/corefx#41764:

runtime/src/libraries/Native/Unix/System.Native/pal_networking.c

Lines 318 to 323 in 3fda6ef

	result = gethostname((char*)name, _POSIX_HOST_NAME_MAX);
	bool includeIPv4Loopback = true;
	bool includeIPv6Loopback = true;
	if (result == 0 && strcasecmp((const char*)address, name) == 0)

If getaddrinfo() returns 0, we can run this comparison earlier and handle the special case instead of failing. Seems like a relatively easy change with no risks.

@stephentoub @davidsh @ManickaP thoughts?

[antonfirsov]

Note GitHub actions team managed to find and solve the macOS issue in their environment:
actions/runner-images#1042 (comment)

[stephentoub]

@antonfirsov, are you still working on this?

[antonfirsov]

Closing for now, as we started to consider product code improvements instead. The problem I'm trying to solve is better tracked in #36849 (and maybe #42185).