Skip to content

Conversation

bart-vmware
Copy link

@bart-vmware bart-vmware commented Oct 13, 2025

This PR provides the building blocks for dotnet/aspire#11740.

Extend service discovery to support HashiCorp Consul-based DNS lookups:

  • Enable specifying how to construct the DNS SRV query
  • Enable adding the Consul DNS server host/port
Microsoft Reviewers: Open in CodeFlow

@github-actions github-actions bot added the area-ai Microsoft.Extensions.AI libraries label Oct 13, 2025
@bart-vmware bart-vmware marked this pull request as ready for review October 13, 2025 13:25
@Copilot Copilot AI review requested due to automatic review settings October 13, 2025 13:25
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends service discovery to support HashiCorp Consul-based DNS lookups by implementing new DNS resolver configuration options and query customization capabilities. The changes enable flexible DNS server configuration and customizable DNS SRV query construction to support Consul service discovery patterns.

  • Refactors DNS resolver options to support runtime configuration and custom DNS servers
  • Adds configurable DNS SRV query construction for Consul-style lookups
  • Updates dependency injection patterns to support options-based configuration

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
DnsSrvServiceEndpointProviderOptions.cs Adds ResolverOptions property and GetQueryText delegate for custom DNS query construction
ResolverOptions.cs Converts from internal sealed class to public class with properties and XML documentation
DnsResolver.cs Adds Create factory method and updates constructor to handle empty server lists
DnsSrvServiceEndpointProviderFactory.cs Updates to use custom query text generation when configured
ServiceDiscoveryDnsServiceCollectionExtensions.cs Changes DI registration to use factory method
ResolvConf.cs Refactors to return server list instead of ResolverOptions
NetworkInfo.cs Refactors to return server list instead of ResolverOptions
Test files Updates test instantiations to match new constructor signatures

@davidfowl
Copy link
Member

@ReubenBond @rzikm can you take a look?

Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

Thanks for picking this one up. Comments are mostly about naming since I did not give too much thought to them since they were originally meant to be internal.

@rzikm
Copy link
Member

rzikm commented Oct 13, 2025

Also, do we not have to go through https://apireview.net/ since this is public API?

@bart-vmware
Copy link
Author

@rzikm Thanks for the review. I've pushed changes to address them. The link "Details about the pull request review procedure" at https://github.com/dotnet/extensions/blob/main/CONTRIBUTING.md#suggested-workflow is dead, so it's unclear to me what the process is. Can I assume you'll resolve each open conversation that is addressed as desired?

@bart-vmware bart-vmware force-pushed the service-discovery-consul branch from 54fa852 to 320874c Compare October 14, 2025 09:40
@bart-vmware bart-vmware requested a review from rzikm October 14, 2025 15:42
@bart-vmware bart-vmware force-pushed the service-discovery-consul branch from cc0c257 to b2404be Compare October 15, 2025 10:12
Copy link
Member

@rzikm rzikm left a comment

Choose a reason for hiding this comment

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

LGTM

@bart-vmware
Copy link
Author

Also, do we not have to go through https://apireview.net/ since this is public API?

@davidfowl Changes were reviewed and approved. Do we need public API review first, or is this ready to be merged?
And once merged, how does it land in the Aspire codebase?

@davidfowl
Copy link
Member

As long as @rzikm and @ReubenBond approve, I'm good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-ai Microsoft.Extensions.AI libraries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants