Managed implementation of DNS resolver #6104

rzikm · 2024-10-04T11:54:32Z

Description

This PR brings in a C# implementation of a DNS resolver that is able to signal the TTL information together with the query results.

Main features

Async network I/O, fully cancellable
Mockable
Resolves IP Addresses (A/AAAA records) and Service records (SRV + related A/AAAA)
Transparent fallback to TCP
Autodetection of OS settings (i.e. reads nameservers from /etc/resolv.conf file)
Thread-safe

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No
Does the change require an update in our Aspire docs?
- Yes
  - Link to aspire-docs issue:
- No

Microsoft Reviewers: Open in CodeFlow

BrennanConroy

Only reviewed a subset of files. It's a large PR and has a lot of RFC reading attached to it. Will review more later.

src/Microsoft.Extensions.ServiceDiscovery.Dns/DnsServiceEndpointProvider.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/DnsServiceEndpointProviderBase.Log.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/DnsSrvServiceEndpointProvider.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsDataReader.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsPrimitives.cs

BrennanConroy · 2025-01-28T20:03:29Z

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsPrimitives.cs

+
+    }
+
+    internal static bool TryReadQName(ReadOnlySpan<byte> messageBuffer, int offset, [NotNullWhen(true)] out string? name, out int bytesRead)


messageBuffer doesn't have any endianness normalization when it's passed in. Is that problematic?

The only multibyte information is the offset in pointers and we read higher and lower byte separately and combine them correctly. Otherwise we only ever read individual bytes or (ascii) strings, so endianness should not matter.

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/NetworkInfo.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/ResolvConf.cs

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsResolver.cs

BrennanConroy · 2025-01-29T18:51:10Z

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsResolver.cs

+
+                    if (responseLength > buffer.Length)
+                    {
+                        var largerBuffer = ArrayPool<byte>.Shared.Rent(responseLength);


This is kind of dangerous, user controlled pre-allocation. Since we're limited to 65k here you could argue it's not too risky, but we should probably explicitly call it out in a comment and make sure we're okay with the risk.

Technically the array pool (ConfigurableArrayPool) could return a 65 * 2 * 2 (or more) buffer since the implementation is free to search multiple bucket sizes to find a pooled byte[].

I am curious, if we were to decide it's too risky, what would you suggest as a mitigation? We need the entire message in memory for parsing purposes (domain name compression via pointers with offsets), so I don't see us reading the contents of the message from a stream.

src/Microsoft.Extensions.ServiceDiscovery.Dns/Resolver/DnsResolver.cs

tests/Microsoft.Extensions.ServiceDiscovery.Dns.Tests/Resolver/ResolvConfTests.cs

danmoseley · 2025-02-02T20:36:03Z

@rzikm could you set expectations on this one ?

rzikm · 2025-02-03T14:37:04Z

@rzikm could you set expectations on this one ?

in parallel to this, I am also finishing Threat Model Review and will need to setup fuzzing. If there are no major distractions I hope to finish the work this month.

davidfowl · 2025-06-14T05:50:49Z

@rzikm appreciate your work on this but we don;t have any plans to go this direction in the short term. I'll close the PR but feel free to continue evolving this.

davidfowl · 2025-06-16T14:37:25Z

Reopening as I forgot we do want this change as a replacement for the DNS client in the service discovery package!

Dona278 · 2025-06-16T14:40:04Z

Thank you @davidfowl. I had just started to write a comment with issues that I've encountered with current DNS client hoping in a hand on heart for this pull!

rzikm · 2025-06-18T11:05:27Z

CI is green, I expect to merge this within a week once I confirm there are no outstanding action items from Threat Model perspective.

rzikm · 2025-06-25T07:14:55Z

Last open items on threat Model Review discussions have been closed, good to merge this after I merge latest main

davidfowl · 2025-06-25T08:24:08Z

@rzikm need you on standby in case something breaks 😄. This code has not changed in a LONG time and we just made this massive change in a minor update with no fallback. I think we should put back the old implementation and have it be possible to opt in.

rzikm · 2025-06-25T08:53:35Z

@davidfowl not that I expect stuff to break, but I expect to be on standby in case something does break.

as for opt-in vs opt-out, we can easily toggle between Managed and System.Net.Dns for A/AAAA records. For SRV records, we would have to put back the DnsClient dependency. Let me know which variation of this (and the opt-in/out mechanism) you prefer and I can put up a follow-up PR.

davidfowl · 2025-06-25T08:58:17Z

Add the DnsClient implementation back and we will keep it behind a feature flag for 1 or 2 minor versions then delete it later.

Dona278 · 2025-06-25T09:32:27Z

I can try it and leave a feedback maybe tomorrow. We already used the previous version with a lot of issues for missing resolution in kubernetes with gRPC headless services.

rzikm · 2025-06-25T09:41:05Z

missing resolution in kubernetes

speaking of kubernetes, I noticed #9913 during local testing, I am very curious if it is really a me-problem or if something can be done to increase the pit of success for aspir8 users.

Dona278 · 2025-06-25T09:54:03Z

We don't use aspir8 to generate manifest and the url used for service discovery is composed with named port and service name during setup so I can't test it.
But the next step will be switch to use aspire publish to generate manifest, I will take a look.
Regards the "old" version issue (encountered many times in a day) was "DNS name: '_portName._tcp.service-name.default.svc.cluster.local'): Non-Existent Domain" even with pods ready and with service with only 1 pod without errors or restart. I hope this refactoring will fix this.

MichaCo · 2025-07-19T07:13:58Z

Reopening as I forgot we do want this change as a replacement for the DNS client in the service discovery package!

Hi @davidfowl, @Dona278 and @rzikm
As the author of the DNS client library you used here so far, I'm really curious to hear about what the reasons are to start re-implementing your own version of a DNS resolver.

If there are technical reasons and issues I would really like to know for selfish reasons, to improve my library ofc!
I did not find any reported issues or discussions from any of you guys in my repo though.

If there are other motivations, that's fine, I don't really care much if you use my library or not.
But I'm open to collaborate with you guys if you want to.

I actually remember having some discussion with one of your team mates from Prague last year, that was more regarding your plans to add a configurable DNS resolver to the .NET framework I think.

Anyways, if there is something I can do to help, let me know.

Regarding

Regards the "old" version issue (encountered many times in a day) was "DNS name: '_portName._tcp.service-name.default.svc.cluster.local'): Non-Existent Domain" even with pods ready and with service with only 1 pod without errors or restart. I hope this refactoring will fix this.

Non-Existent Domain is a response error. That means, the DNS server you asked didn't know the answer and responded with that error code. It is very unlikely that a different resolver would change that outcome because it would receive the exact same response.

Digging a bit on google about issues with CoreDNS (the DNS system used by kubernetes) I found a bunch of reasons why you might get those kind of errors.
Do you have logs from CoreDNS when you are getting errors?

Maybe we just need some retry logic here in the app specific code to account for service changes? After all, this is a distributed system which might have some latency.

Maybe the resolver uses the wrong DNS server because of some bad system configuration?

That being said, I do not have much experience with kubernetes because I'm not using it at work, so what do I know ;)
But if you can tell me how to reproduce those kind of issues, I can take a look.

Thanks
Michael

dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Oct 4, 2024

build-analysis bot mentioned this pull request Oct 10, 2024

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

rzikm added 8 commits October 21, 2024 13:47

First integration of Resolver sources

2c217a3

Allow mocking

704e33c

Fix dispose

aff6d17

Fix tests

15db8a1

Actually run ResolvConf tests

6bda235

Add retry functionality

6fc61e0

Add logging

2b6d91e

Fix Microsoft.Extensions.ServiceDiscovery.Yarp.Tests

2137954

davidfowl requested a review from ReubenBond October 29, 2024 21:16

rzikm added 5 commits November 26, 2024 14:46

Merge remote-tracking branch 'upstream/main' into managed-resolver

c204b01

Activity WIP

2fc7de4

Fix reading CNAME with pointers in domain name segments

9b4de29

Reenable telemetry

3ad5a75

Last changes to telemetry

40f8f9b

rzikm force-pushed the managed-resolver branch from 32d94c5 to 40f8f9b Compare November 28, 2024 16:07

Use strong RNG for Transaction ID generation

e385605

BrennanConroy reviewed Jan 24, 2025

View reviewed changes

MihaZupan mentioned this pull request Jan 27, 2025

Stalled getaddrinfo syscall can lead to massive HttpClient timeouts dotnet/runtime#81023

Open

BrennanConroy reviewed Jan 29, 2025

View reviewed changes

danmoseley added area-service-discovery needs-author-action An issue or pull request that requires more info or actions from the author. labels Feb 2, 2025

rzikm added 2 commits February 3, 2025 13:44

Check against too long domain name

3cf0a9d

Disallow pointer to pointer.

6016e41

dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Feb 3, 2025

rzikm added 2 commits February 3, 2025 17:18

Code review feedback (easy fixes)

f8c189d

More code review feedback

d929889

rzikm added 5 commits June 10, 2025 13:50

Fix build

3c387e9

Fix Yarp service discovery tests

c6e3d63

Lazy TCP socket allocation in LoopbackTests

e66229a

LoopbackTestBaseLogging

da018f7

fixup! LoopbackTestBaseLogging

2697412

davidfowl closed this Jun 14, 2025

davidfowl reopened this Jun 16, 2025

rzikm and others added 4 commits June 17, 2025 09:56

Downgrade SharpFuzz

214cc9b

Fix buffer use after returning to pool

0d891cb

Merge branch 'main' into managed-resolver

46cf262

Fix build

35a5809

rzikm and others added 2 commits June 25, 2025 09:16

Merge branch 'main' into managed-resolver

fcf18cf

Remove DnsClient package ref

df785d1

rzikm merged commit 416e077 into dotnet:main Jun 25, 2025
252 checks passed

rzikm changed the title ~~[WIP] Managed implementation of DNS resolver~~ Managed implementation of DNS resolver Jun 25, 2025

davidfowl added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jun 25, 2025

rzikm mentioned this pull request Jun 30, 2025

Fallback to previous DNS service discovery #10140

Merged

16 tasks

github-actions bot locked and limited conversation to collaborators Aug 18, 2025


		}

		internal static bool TryReadQName(ReadOnlySpan<byte> messageBuffer, int offset, [NotNullWhen(true)] out string? name, out int bytesRead)

Managed implementation of DNS resolver #6104

Managed implementation of DNS resolver #6104

Uh oh!

Conversation

rzikm commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Microsoft Reviewers: Open in CodeFlow

Uh oh!

BrennanConroy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrennanConroy Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

rzikm Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BrennanConroy Jan 29, 2025

Choose a reason for hiding this comment

Uh oh!

rzikm Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Feb 2, 2025

Uh oh!

rzikm commented Feb 3, 2025

Uh oh!

davidfowl commented Jun 14, 2025

Uh oh!

davidfowl commented Jun 16, 2025

Uh oh!

Dona278 commented Jun 16, 2025

Uh oh!

rzikm commented Jun 18, 2025

Uh oh!

rzikm commented Jun 25, 2025

Uh oh!

Uh oh!

davidfowl commented Jun 25, 2025

Uh oh!

rzikm commented Jun 25, 2025

Uh oh!

davidfowl commented Jun 25, 2025

Uh oh!

Dona278 commented Jun 25, 2025

Uh oh!

rzikm commented Jun 25, 2025

Uh oh!

Dona278 commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichaCo commented Jul 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

rzikm commented Oct 4, 2024 •

edited

Loading

Dona278 commented Jun 25, 2025 •

edited

Loading