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

Gather more diagnostics, and prepare skipping flaky DNS tests #36072

Closed
wants to merge 11 commits into from
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
#nullable enable
using System.IO;
using System.Runtime.InteropServices;
using System.Text;
using Xunit.Abstractions;

namespace System.Net.NameResolution.Tests
{
internal static class NameResolutionTestHelper
{
/// <summary>
/// Check if name resolution (getaddrinfo) at OS level.
/// In case of failure we may force the tests fail, so we can gather diagnostics from the CI environment.
/// After collecting enough information we shall alter call sites to use throwOnFailure:false,
/// since the failure does not indicate an error within the NCL product code.
/// </summary>
public static unsafe bool EnsureNameToAddressWorks(string hostName, ITestOutputHelper? testOutput, bool throwOnFailure)
{
addrinfo hint = default;
hint.ai_family = 0; // AF_UNSPEC
hint.ai_flags = 2; // AI_CANONNAME
hint.ai_socktype = 1; // SOCK_STREAM;

addrinfo* res = default;

int err = getaddrinfo(hostName, null, &hint, &res);
freeaddrinfo(res);

if (err != 0)
{
string failureInfo =
$"Failed to resolve '{hostName}'! getaddrinfo error: {err}{Environment.NewLine}{LogUnixInfo()}";
testOutput?.WriteLine(failureInfo);
if (throwOnFailure)
{
throw new Exception(failureInfo);
}

return false;
}

return true;
}

private static string LogUnixInfo()
{
StringBuilder bld = new StringBuilder();
bld.AppendLine($"Dns.GetHostName() == {Dns.GetHostName()}");
bld.AppendLine("--- /etc/hosts ---");

string etcHosts;
try
{
etcHosts = File.ReadAllText("/etc/hosts");
}
catch (Exception ex)
{
etcHosts = $"Failed to retrieve /etc/hosts: {ex.Message}";
}

string resolvConf;
try
{
resolvConf = File.ReadAllText("/etc/resolv.conf");
}
catch (Exception ex)
{
resolvConf = $"Failed to retrieve /etc/resolv.conf: {ex.Message}";
}

bld.AppendLine(etcHosts);
bld.AppendLine("--- /etc/resolv.conf ---");
bld.AppendLine(resolvConf);
bld.AppendLine("------");
return bld.ToString();
}

#pragma warning disable 169
[StructLayout(LayoutKind.Sequential)]
unsafe struct addrinfo
{
public int ai_flags;
public int ai_family;
public int ai_socktype;
int ai_protocol;
uint ai_addrlen;
UIntPtr ai_addr;
UIntPtr ai_canonname;
addrinfo* ai_next;
};
#pragma warning restore 169

[DllImport("libc")]
private static extern unsafe int getaddrinfo(string node, string service, addrinfo* hints, addrinfo** res);

[DllImport("libc")]
private static extern unsafe void freeaddrinfo(addrinfo* res);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@antonfirsov antonfirsov May 11, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
#nullable enable
using Xunit.Abstractions;

namespace System.Net.NameResolution.Tests
{
internal static class NameResolutionTestHelper
{
public static unsafe bool EnsureNameToAddressWorks(string hostName, ITestOutputHelper? testOutput, bool throwOnFailure = true)
{
// NOP, since we do not expect DNS failures on Windows.
return true;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,26 @@

#pragma warning disable 0618 // use of obsolete methods

using System.IO;
using System.Net.Sockets;
using System.Runtime.InteropServices;
using System.Text;
using Xunit;
using Xunit.Abstractions;

namespace System.Net.NameResolution.Tests
{


public class GetHostByNameTest
{
private readonly ITestOutputHelper _output;

public GetHostByNameTest(ITestOutputHelper output)
{
_output = output;
}

[Fact]
public void DnsObsoleteBeginGetHostByName_BadName_Throws()
{
Expand Down Expand Up @@ -102,20 +115,28 @@ public void DnsObsoleteGetHostByName_IPv6String_ReturnsOnlyGivenIP()
Assert.Equal(IPAddress.IPv6Loopback, entry.AddressList[0]);
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/1488", TestPlatforms.OSX)]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArm64Process))] // [ActiveIssue("https://github.com/dotnet/runtime/issues/27622")]
[Fact]
public void DnsObsoleteGetHostByName_EmptyString_ReturnsHostName()
{
if (!NameResolutionTestHelper.EnsureNameToAddressWorks("", _output, throwOnFailure: true))
{
return;
}

IPHostEntry entry = Dns.GetHostByName("");

// DNS labels should be compared as case insensitive for ASCII characters. See RFC 4343.
Assert.Contains(Dns.GetHostName(), entry.HostName, StringComparison.OrdinalIgnoreCase);
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/1488", TestPlatforms.OSX)]
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArm64Process))] // [ActiveIssue("https://github.com/dotnet/runtime/issues/27622")]
[Fact]
public void DnsObsoleteBeginEndGetHostByName_EmptyString_ReturnsHostName()
{
if (!NameResolutionTestHelper.EnsureNameToAddressWorks("", _output, throwOnFailure: true))
{
return;
}

IPHostEntry entry = Dns.EndGetHostByName(Dns.BeginGetHostByName("", null, null));

// DNS labels should be compared as case insensitive for ASCII characters. See RFC 4343.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,73 +8,51 @@
using System.Threading.Tasks;

using Xunit;
using Xunit.Abstractions;

namespace System.Net.NameResolution.Tests
{
public class GetHostEntryTest
{
private readonly ITestOutputHelper _output;

public GetHostEntryTest(ITestOutputHelper output)
{
_output = output;
}

[Fact]
public async Task Dns_GetHostEntryAsync_IPAddress_Ok()
{
IPAddress localIPAddress = await TestSettings.GetLocalIPAddress();
await TestGetHostEntryAsync(() => Dns.GetHostEntryAsync(localIPAddress));
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/1488", TestPlatforms.OSX)]
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArm64Process))] // [ActiveIssue("https://github.com/dotnet/runtime/issues/27622")]
[Theory]
[InlineData("")]
[InlineData(TestSettings.LocalHost)]
public async Task Dns_GetHostEntry_HostString_Ok(string hostName)
{
try
{
await TestGetHostEntryAsync(() => Task.FromResult(Dns.GetHostEntry(hostName)));
}
catch (Exception ex) when (hostName == "")
if (!NameResolutionTestHelper.EnsureNameToAddressWorks(hostName, _output, throwOnFailure: true))
{
// Additional data for debugging sporadic CI failures https://github.com/dotnet/runtime/issues/1488
string actualHostName = Dns.GetHostName();
string etcHosts = "";
Exception getHostEntryException = null;
Exception etcHostsException = null;

try
{
Dns.GetHostEntry(actualHostName);
}
catch (Exception e2)
{
getHostEntryException = e2;
}

try
{
if (Environment.OSVersion.Platform != PlatformID.Win32NT)
{
etcHosts = File.ReadAllText("/etc/hosts");
}
}
catch (Exception e2)
{
etcHostsException = e2;
}

throw new Exception(
$"Failed for empty hostname.{Environment.NewLine}" +
$"Dns.GetHostName() == {actualHostName}{Environment.NewLine}" +
$"{nameof(getHostEntryException)}=={getHostEntryException}{Environment.NewLine}" +
$"{nameof(etcHostsException)}=={etcHostsException}{Environment.NewLine}" +
$"/etc/host =={Environment.NewLine}{etcHosts}",
ex);
return;
}

await TestGetHostEntryAsync(() => Task.FromResult(Dns.GetHostEntry(hostName)));
}

[ActiveIssue("https://github.com/dotnet/runtime/issues/1488", TestPlatforms.OSX)]
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotArm64Process))] // [ActiveIssue("https://github.com/dotnet/runtime/issues/27622")]
[Theory]
[InlineData("")]
[InlineData(TestSettings.LocalHost)]
public async Task Dns_GetHostEntryAsync_HostString_Ok(string hostName) =>
public async Task Dns_GetHostEntryAsync_HostString_Ok(string hostName)
{
if (!NameResolutionTestHelper.EnsureNameToAddressWorks(hostName, _output, throwOnFailure: true))
{
return;
}

await TestGetHostEntryAsync(() => Dns.GetHostEntryAsync(hostName));
}

[Fact]
public async Task Dns_GetHostEntryAsync_IPString_Ok() =>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<TargetFrameworks>$(NetCoreAppCurrent)-Windows_NT;$(NetCoreAppCurrent)-Unix</TargetFrameworks>

<AllowUnsafeBlocks>true</AllowUnsafeBlocks>
</PropertyGroup>
<ItemGroup>
<Compile Include="GetHostByAddressTest.cs" />
Expand All @@ -20,5 +22,11 @@
Link="Common\System\Net\Configuration.Sockets.cs" />
<Compile Include="$(CommonTestPath)System\Diagnostics\Tracing\TestEventListener.cs"
Link="Common\System\Diagnostics\Tracing\TestEventListener.cs" />
<Compile Include="$(CommonTestPath)System\Net\Sockets\NameResolutionTestHelper.Unix.cs"
Link="Common\System\Net\Sockets\NameResolutionTestHelper.Unix.cs"
Condition="'$(TargetsUnix)' == 'true'" />
<Compile Include="$(CommonTestPath)System\Net\Sockets\NameResolutionTestHelper.Windows.cs"
Link="Common\System\Net\Sockets\NameResolutionTestHelper.Windows.cs"
Condition="'$(TargetsWindows)' == 'true'" />
</ItemGroup>
</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
// See the LICENSE file in the project root for more information.

using System.IO;
using System.Net.NameResolution.Tests;
using System.Net.Sockets;
using System.Runtime.InteropServices;
using System.Text;
using Xunit;
using Xunit.Abstractions;

Expand Down Expand Up @@ -59,15 +61,16 @@ public void TryGetAddrInfo_HostName(bool justAddresses)
string hostName = NameResolutionPal.GetHostName();
Assert.NotNull(hostName);

SocketError error = NameResolutionPal.TryGetAddrInfo(hostName, justAddresses, out hostName, out string[] aliases, out IPAddress[] addresses, out int nativeErrorCode);
if (error == SocketError.HostNotFound && (RuntimeInformation.IsOSPlatform(OSPlatform.Linux) || RuntimeInformation.IsOSPlatform(OSPlatform.OSX)))
// On Unix, we are not guaranteed to be able to resove the local host. The ability to do so depends on the
// machine configurations, which varies by distro and is often inconsistent.
if (!NameResolutionTestHelper.EnsureNameToAddressWorks(hostName, _output, throwOnFailure: true))
{
// On Unix, we are not guaranteed to be able to resove the local host. The ability to do so depends on the
// machine configurations, which varies by distro and is often inconsistent.
return;
}

SocketError error = NameResolutionPal.TryGetAddrInfo(hostName, justAddresses, out hostName, out string[] aliases, out IPAddress[] addresses, out int nativeErrorCode);
Assert.Equal(SocketError.Success, error);

if (!justAddresses)
{
Assert.NotNull(hostName);
Expand Down Expand Up @@ -122,15 +125,15 @@ public void TryGetAddrInfo_HostName_TryGetNameInfo()
string hostName = NameResolutionPal.GetHostName();
Assert.NotNull(hostName);

SocketError error = NameResolutionPal.TryGetAddrInfo(hostName, justAddresses: false, out hostName, out string[] aliases, out IPAddress[] addresses, out int nativeErrorCode);
if (error == SocketError.HostNotFound)
// On Unix, getaddrinfo returns host not found, if all the machine discovery settings on the local network
// is turned off. Hence dns lookup for it's own hostname fails.
if (!NameResolutionTestHelper.EnsureNameToAddressWorks(hostName, _output, throwOnFailure: true))
{
// On Unix, getaddrinfo returns host not found, if all the machine discovery settings on the local network
// is turned off. Hence dns lookup for it's own hostname fails.
Assert.Equal(PlatformID.Unix, Environment.OSVersion.Platform);
return;
}

SocketError error = NameResolutionPal.TryGetAddrInfo(hostName, justAddresses: false, out hostName, out string[] aliases, out IPAddress[] addresses, out int nativeErrorCode);

Assert.Equal(SocketError.Success, error);
Assert.NotNull(hostName);
Assert.NotNull(aliases);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@
Link="Common\System\Net\Sockets\ProtocolFamily.cs" />
<Compile Include="$(CommonPath)Interop\Windows\WinSock\Interop.GetAddrInfoExW.cs"
Link="Common\Interop\Windows\WinSock\Interop.GetAddrInfoExW.cs" />
<!-- Test -->
<Compile Include="$(CommonTestPath)System\Net\Sockets\NameResolutionTestHelper.Windows.cs"
Link="Common\System\Net\Sockets\NameResolutionTestHelper.Windows.cs" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetsUnix)' == 'true' ">
<Compile Include="$(CommonPath)System\Net\SocketAddressPal.Unix.cs"
Expand Down Expand Up @@ -107,5 +110,8 @@
Link="Common\Interop\Unix\System.Native\Interop.Socket.cs" />
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.SocketAddress.cs"
Link="Common\Interop\Unix\System.Native\Interop.SocketAddress.cs" />
<!-- Test -->
<Compile Include="$(CommonTestPath)System\Net\Sockets\NameResolutionTestHelper.Unix.cs"
Link="Common\System\Net\Sockets\NameResolutionTestHelper.Unix.cs" />
</ItemGroup>
</Project>