Implement DnsResolver for Windows#129302
Conversation
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
This PR adds a new DNS query API surface area to System.Net.NameResolution (new Dns.Resolve* static entry points plus an instance-based DnsResolver), and implements the Windows PAL using DnsQueryEx. It also introduces functional tests, including deterministic loopback-server-driven tests for record parsing/handling.
Changes:
- Adds new public APIs:
Dns.Resolve*(A/AAAA/SRV/MX/TXT/CNAME/PTR/NS),DnsResolver,DnsResolverOptions, record structs,DnsResult<T>, andDnsResponseCode. - Implements Windows DNS querying/parsing via
DnsQueryEx(sync + async paths), plus non-Windows PNSE stubs. - Adds functional tests (outer-loop network tests + loopback DNS server tests) and extends NameResolution telemetry to handle
string[]answers.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/System.Net.NameResolution.Functional.Tests.csproj | Adds new functional test sources to the project |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/LoopbackDnsServer.cs | Adds an in-process UDP/TCP loopback DNS server for deterministic tests |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResponseBuilder.cs | Adds a fluent builder for DNS response bytes used by loopback tests |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverTest.cs | Adds outer-loop functional tests for the new APIs (real DNS) |
| src/libraries/System.Net.NameResolution/tests/FunctionalTests/DnsResolverLoopbackTest.cs | Adds deterministic loopback-server-driven tests for parsing/behavior/metrics |
| src/libraries/System.Net.NameResolution/src/System/Net/NameResolutionTelemetry.cs | Extends telemetry answer handling to support string[] |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResult.cs | Introduces DnsResult<T> envelope type |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResponseCode.cs | Introduces DnsResponseCode enum |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.Windows.cs | Implements Windows PAL via DnsQueryEx, record parsing, cancellation, glue, TTL extraction |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverPal.Unsupported.cs | Adds non-Windows PNSE PAL stubs |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolverOptions.cs | Adds options type for custom server selection |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsResolver.cs | Adds instance-based resolver API + telemetry wrapping |
| src/libraries/System.Net.NameResolution/src/System/Net/DnsRecords.cs | Adds record structs (Address/Srv/Mx/Txt/CName/Ptr/Ns) |
| src/libraries/System.Net.NameResolution/src/System/Net/Dns.Resolve.cs | Adds new static Dns.Resolve* APIs backed by a default resolver |
| src/libraries/System.Net.NameResolution/src/System/Net/Dns.cs | Makes Dns partial to host the new Dns.Resolve* partial |
| src/libraries/System.Net.NameResolution/src/System.Net.NameResolution.csproj | Wires new sources and Windows Dnsapi interop into the build |
| src/libraries/System.Net.NameResolution/src/Resources/Strings.resx | Adds SR string for unsupported custom DNS ports |
| src/libraries/System.Net.NameResolution/ref/System.Net.NameResolution.cs | Adds new public API contract entries |
| src/libraries/Common/src/Interop/Windows/Interop.Libraries.cs | Adds Libraries.Dnsapi constant |
| src/libraries/Common/src/Interop/Windows/Dnsapi/Interop.DnsTypes.cs | Adds DnsQueryEx/DNS_RECORD-related structs/constants |
| src/libraries/Common/src/Interop/Windows/Dnsapi/Interop.DnsApi.cs | Adds LibraryImport declarations for DnsQueryEx/DnsCancelQuery/DnsFree |
MihaZupan
left a comment
There was a problem hiding this comment.
First pass through.
Just curious, will there be a way to opt-in to using the managed implementation even on Windows, or would there be no reason to want to do that anyway?
| public System.Net.DnsResult<System.Net.AddressRecord> ResolveAddresses(string name) { throw null; } | ||
| public System.Net.DnsResult<System.Net.AddressRecord> ResolveAddresses(string name, System.Net.Sockets.AddressFamily addressFamily) { throw null; } | ||
| public System.Net.DnsResult<System.Net.SrvRecord> ResolveSrv(string name) { throw null; } | ||
| public System.Net.DnsResult<System.Net.MxRecord> ResolveMx(string name) { throw null; } | ||
| public System.Net.DnsResult<System.Net.TxtRecord> ResolveTxt(string name) { throw null; } | ||
| public System.Net.DnsResult<System.Net.CNameRecord> ResolveCName(string name) { throw null; } | ||
| public System.Net.DnsResult<System.Net.PtrRecord> ResolvePtr(string name) { throw null; } | ||
| public System.Net.DnsResult<System.Net.NsRecord> ResolveNs(string name) { throw null; } |
There was a problem hiding this comment.
| public System.Net.DnsResult<System.Net.AddressRecord> ResolveAddresses(string name) { throw null; } | |
| public System.Net.DnsResult<System.Net.AddressRecord> ResolveAddresses(string name, System.Net.Sockets.AddressFamily addressFamily) { throw null; } | |
| public System.Net.DnsResult<System.Net.SrvRecord> ResolveSrv(string name) { throw null; } | |
| public System.Net.DnsResult<System.Net.MxRecord> ResolveMx(string name) { throw null; } | |
| public System.Net.DnsResult<System.Net.TxtRecord> ResolveTxt(string name) { throw null; } | |
| public System.Net.DnsResult<System.Net.CNameRecord> ResolveCName(string name) { throw null; } | |
| public System.Net.DnsResult<System.Net.PtrRecord> ResolvePtr(string name) { throw null; } | |
| public System.Net.DnsResult<System.Net.NsRecord> ResolveNs(string name) { throw null; } |
I think we should talk about this with the team :)
My stance continues to be that exposing sync APIs here is a mistake, especially if we have real plans to eventually add support for resolution over HTTPS/QUIC.
The fact that e.g. Resolve silently has 2x the latency compared to the async variant makes the argument that we need sync APIs for perf wrong IMO.
There was a problem hiding this comment.
I am also not entirely happy with having synchronous versions as well, we can bring it in a team discussion and push back if more of us feel strongly about this :)
|
I've worked with DNS apis here before so here's what I wanted to note:
|
| [OuterLoop("Binds the loopback DNS port 53 and issues real DnsQueryEx calls.")] | ||
| [Collection(nameof(DisableParallelization))] | ||
| [PlatformSpecific(TestPlatforms.Windows)] | ||
| public class DnsResolverLoopbackTest : IClassFixture<WindowsLoopbackServer> | ||
| { | ||
| private static DnsResolver CreateResolver(LoopbackDnsServer server) | ||
| => new DnsResolver(new DnsResolverOptions { Servers = { server.EndPoint } }); |
Replacement for #129302 (thanks, `gh stack submit` 😠) Implements the API approved in #19443: New Dns.Resolve*[Async] static methods for A/AAAA/SRV/MX/TXT/CNAME/PTR/NS records. New DnsResolver / DnsResolverOptions for instance-based resolution with optional custom DNS servers. Record types AddressRecord, SrvRecord, MxRecord, TxtRecord, CNameRecord, PtrRecord, NsRecord. DnsResponseCode enum and DnsResult envelope carrying ResponseCode, Records, and NegativeCacheTtl. Windows implementation uses DnsQueryEx. Non-Windows platforms get PlatformNotSupportedException stubs pending follow-up implementations (splitting to multiple stacked PRs for easier review). --------- Co-authored-by: rzikm <rzikm@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Replacement for #129302 (thanks, `gh stack submit` 😠) Implements the API approved in #19443: New Dns.Resolve*[Async] static methods for A/AAAA/SRV/MX/TXT/CNAME/PTR/NS records. New DnsResolver / DnsResolverOptions for instance-based resolution with optional custom DNS servers. Record types AddressRecord, SrvRecord, MxRecord, TxtRecord, CNameRecord, PtrRecord, NsRecord. DnsResponseCode enum and DnsResult envelope carrying ResponseCode, Records, and NegativeCacheTtl. Windows implementation uses DnsQueryEx. Non-Windows platforms get PlatformNotSupportedException stubs pending follow-up implementations (splitting to multiple stacked PRs for easier review). --------- Co-authored-by: rzikm <rzikm@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Implements the API approved in #19443:
Windows implementation uses DnsQueryEx. Non-Windows platforms get PlatformNotSupportedException stubs pending follow-up implementations (splitting to multiple stacked PRs for easier review).