Fallback to previous DNS service discovery #10140
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements a fallback mechanism for DNS service discovery, allowing the system to revert to previous DNS resolution implementations if needed. Key changes include:
- Introducing conditional registration for DNS resolvers based on a fallback flag.
- Modifying the IDnsResolver interface and adding a new FallbackDnsResolver implementation.
- Updating project files to add and version the DnsClient package.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Extensions.ServiceDiscovery.Dns/ServiceDiscoveryDnsServiceCollectionExtensions.cs | Adds conditional logic to register either DnsResolver or FallbackDnsResolver based on a fallback flag. |
| src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/IDnsResolver.cs | Updates the interface by removing an unused parameter. |
| src/Microsoft.Extensions.ServiceDiscovery.Dns/Microsoft.Extensions.ServiceDiscovery.Dns.csproj | Adds a PackageReference for DnsClient. |
| src/Microsoft.Extensions.ServiceDiscovery.Dns/FallbackDnsResolver.cs | Introduces a new fallback resolver implementation using DnsClient. |
| Directory.Packages.props | Updates the package version for DnsClient. |
src/Microsoft.Extensions.ServiceDiscovery.Dns/ServiceDiscoveryDnsServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
| if (AppContext.TryGetSwitch("Microsoft.Extensions.ServiceDiscovery.Dns.UseDnsClientFallback", out var value)) | ||
| { | ||
| return value; | ||
| } | ||
|
|
||
| var envVar = Environment.GetEnvironmentVariable("MICROSOFT_EXTENSIONS_SERVICE_DISCOVERY_DNS_USE_DNSCLIENT_FALLBACK"); | ||
| if (envVar is not null && (envVar.Equals("true", StringComparison.OrdinalIgnoreCase) || envVar.Equals("1"))) |
There was a problem hiding this comment.
I couldn't find an example how you do feature flags in Aspire, so I used the same mechanism as we use in .NET runtime.
I am open to name suggestions, I honestly could not think of a better name.
| } | ||
|
|
||
| var lookupMapping = new Dictionary<string, List<AddressResult>>(); | ||
| foreach (var record in queryResult.Additionals.OfType<AddressRecord>()) |
There was a problem hiding this comment.
We checked CNameRecord before:
foreach (var record in result.Additionals.Where(x => x is AddressRecord or CNameRecord))
Is that something we still should handle here?
There was a problem hiding this comment.
RFC explicitly prohibits aliases in the target of the SRV record
Target
The domain name of the target host. There MUST be one or more
address records for this name, the name MUST NOT be an alias (in
the sense of RFC 1034 or RFC 2181). Implementors are urged, but
not required, to return the address record(s) in the Additional
Data section. Unless and until permitted by future standards
action, name compression is not to be used for this field.
So we shouldn't need to look for CNAME records.
src/Microsoft.Extensions.ServiceDiscovery.Dns/ServiceDiscoveryDnsServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
There weren't previously any tests which used the DnsClient.NET code. It was always mocked out (and still is in many of the tests) I ran the test suite locally both with the new resolver and fallback enabled and they pass. |
|
The CI Failures seem to be unrelated, but I don't know how to get past this check, @BrennanConroy can you help here? |
Re-ran and looks like this is green now |
3 participants
Description
Follow up work based on #6104 (comment), this allows going back to the previous DNS resolution implementations (System.Net.Dns for IP resolution, DnsClient.NET for SRV resolution)
cc @davidfowl
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate