Resolve IPV4Host from the allocated address when TargetHost is customized - #19973
Conversation
…ized EndpointProperty.IPV4Host returned the literal "127.0.0.1" for any endpoint resolved in the localhost network context, without consulting the endpoint's configured or allocated address. Setting TargetHost to a specific address, such as "[::1]", therefore changed the DCP port binding and the allocated endpoint while leaving every IPV4Host consumer pointed at 127.0.0.1 -- most visibly the SQL Server connection string, whose health check and AddDatabase step then fail with "connection refused". The loopback literal is only the right answer while the endpoint address is "localhost". Gate the shortcut on NormalizeTargetHost mapping TargetHost to localhost, which keeps the literal for the default, a *.localhost TLD, a wildcard bind (0.0.0.0, ::) and an arbitrary machine name, and falls through to the allocated address when TargetHost names one specific address. Fixes microsoft#19972 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HpvU2U6aPPWcG4XdPGLVNk
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19973Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19973" |
There was a problem hiding this comment.
🟡 Changes recommended
The public IPV4Host documentation now contradicts its new ability to return IPv6 addresses.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes custom TargetHost handling so endpoint consumers use the allocated address instead of always using IPv4 loopback.
Changes:
- Gates the
127.0.0.1shortcut on normalized localhost binding. - Adds regression tests for IPv6 and localhost-TLD targets.
File summaries
| File | Description |
|---|---|
src/Aspire.Hosting/ApplicationModel/EndpointReference.cs |
Resolves customized target hosts from allocation. |
tests/Aspire.Hosting.Tests/EndpointReferenceTests.cs |
Covers custom IPv6 and localhost-TLD behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
The enum member was documented as "The IPv4 address of the endpoint", which no longer holds once a TargetHost naming one specific address resolves to whatever the orchestrator allocated. Spell out both halves of the contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HpvU2U6aPPWcG4XdPGLVNk
|
@microsoft-github-policy-service agree |
|
My primary concern with this PR is that service discovery—particularly for containers—has many layers where being intentional about IPv4 versus IPv6 is important. I’m resistant to changing I agree that Aspire has a broader problem to solve around IPv4 and IPv6 networking, but I believe that solution will require new endpoint semantics. For example, we might introduce an There are also gaps in how container endpoints are configured that this change does not address. A complete solution likely requires independent controls for the public binding, proxy producer address, and workload listener address. We will also need controls over the default container network’s address-family configuration: IPv4-only, IPv6-only, or dual-stack. Until those pieces are modeled separately, I don’t think changing |
David Negstad (danegsta)
left a comment
There was a problem hiding this comment.
I'm concerned about the implications of this change. I agree that there's a gap here that we need to improve, but this change has implications for networking and service discovery that need to be considered very carefully and probably as part of a larger overhaul to better support intentional and explicit IPv6 usage for various parts of the service networking stack.
|
Thanks, this makes sense. I agree that changing The underlying issue seems to be that we're currently conflating several different endpoint concepts through I think the cleaner direction is to leave For containers, that would still need to be paired with separate controls for the public binding, proxy producer address, and workload listener, plus explicit IPv4/IPv6/dual-stack network configuration. I'm happy to rework this in that direction. Would you prefer that broader endpoint/networking model to be designed in a separate issue first and close/narrow this PR, rather than extending #19973 into that larger change? |
|
Since the broader IPv4/IPv6 problem clearly extends beyond the scope of the relatively small changes in this PR, would it make sense to narrow this PR to the parts that are independently safe and useful, merge those, and open a separate issue for the larger endpoint/networking model? That follow-up issue could cover IPHost/IPV6Host, separating public binding from the proxy producer and workload listener addresses, and explicit IPv4/IPv6/dual-stack container network configuration. I think that would let us preserve the existing IPV4Host contract while still keeping the useful incremental changes from this PR, rather than expanding this PR into a much larger networking redesign. |
Fixes #19972
Problem
EndpointProperty.IPV4Hostreturned the literal"127.0.0.1"for any endpoint resolved in the localhost network context, without ever consulting the endpoint's configured or allocated address:So
WithEndpoint("tcp", e => { e.TargetHost = "[::1]"; e.IsProxied = false; })changes the DCP port binding and the allocated endpoint, but everyIPV4Hostconsumer stays pointed at127.0.0.1. The most visible casualty isSqlServerServerResource, whose connection string is built fromIPV4Host(src/Aspire.Hosting.SqlServer/SqlServerServerResource.cs:50) — thesql_checkhealth check and theAddDatabasecreation step both fail with "connection refused" because nothing is listening on127.0.0.1:1433. The issue reports the same127.0.0.1data source for all eightTargetHostvariants tried.Fix
The loopback literal is only the correct answer while the endpoint address is
localhost.DcpModelUtilities.NormalizeTargetHostalready encodes exactly that distinction, so the shortcut is now gated on it:TargetHostIPV4Hostlocalhostlocalhost127.0.0.1— unchangedmyapp.dev.localhostlocalhost127.0.0.1— unchanged0.0.0.0/[::]localhost127.0.0.1— unchangedmachine-namelocalhost127.0.0.1— unchanged[::1][::1]10.0.0.110.0.0.1Wildcard binds and arbitrary machine names keep the literal, since their allocated address is
localhostand substituting it would defeat the purpose ofIPV4Host(avoiding a name that may resolve to::1). Only aTargetHostnaming one specific address falls through toResolveValueWithAllocatedAddress(), resolving to the same valueEndpointProperty.Hostreturns.Two details worth a reviewer's eye:
EndpointReference.EndpointAnnotationOrDefault, which returns null instead of throwing, so an undefined endpoint still answers immediately rather than turning this property into a missing-endpoint exception.ApplicationModelintoAspire.Hosting.DcpforNormalizeTargetHost. Reusing it keeps one source of truth for the wildcard and machine-name mapping; happy to move that helper somewhere more neutral if you'd prefer.An endpoint with a customized
TargetHostnow waits for allocation instead of answering synchronously. That matchesEndpointProperty.Host, and the default path still returns immediately.Tests
Added to
tests/Aspire.Hosting.Tests/EndpointReferenceTests.cs:GetValueAsync_IPV4Host_WithCustomTargetHost_UsesAllocatedAddress—TargetHost = "[::1]"does not complete before allocation and resolves to[::1]. Verified this fails on the unpatched source.GetValueAsync_IPV4Host_WithLocalhostTldTargetHost_ReturnsImmediately— a*.localhostTLD still returns127.0.0.1immediately.EndpointReferenceTests,ExpressionResolverTests(98 tests) andDcp.DcpExecutorTests(277) pass.Aspire.Hosting.SqlServer.Tests: 56 pass, 9 fail because no Docker daemon is available on this machine.The end-to-end SQL Server scenario from the issue needs a container runtime and was not run.
🤖 Generated with Claude Code
https://claude.ai/code/session_01HpvU2U6aPPWcG4XdPGLVNk