Skip to content

Fix support for non-localhost endpoint targets #9977

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 24 commits into from
Jun 30, 2025

Conversation

danegsta
Copy link
Member

@danegsta danegsta commented Jun 20, 2025

Description

TargetHost wasn't being plumbed through properly to all endpoint/service resources, which was preventing us from binding properly to addresses other than localhost. This ensures that containers, executables, and service proxies bind to the configured address for an endpoint. For addresses bound to 0.0.0.0 or [::] (all network interfaces), the current machine hostname is used as the allocated endpoint address. For Asp.NET addresses using +, *, etc. to indicate all interfaces, we default to IPv4 0.0.0.0 unless it isn't supported on any network interfaces (in which case we'll try to bind to [::]).

Effectively this means:

TargetHost = "localhost" => service listens on localhost, URLs use localhost

TargetHost = "0.0.0.0" => service listens on all IPv4 interfaces, URLs use hostname

TargetHost = "[::]" => service listens on all IPv6 interfaces, URLs use hostname

ASPNETCORE_URLS="https://+:" => service listens on all IPv4 interfaces if supported, all IPv6 interfaces otherwise; URLs use the hostname

Fixes #9853
Fixes #9085

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?

@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 20, 2025
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 fixes issues with endpoint binding by ensuring that the TargetHost value is correctly used instead of defaulting to "localhost".

  • Removed conditional logic that forced proxied endpoints to use "localhost" in ProjectResourceBuilderExtensions.
  • Updated DcpExecutor to correctly use TargetHost for service endpoints and container port specifications.

Reviewed Changes

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

File Description
src/Aspire.Hosting/ProjectResourceBuilderExtensions.cs Removed redundant logic condition and updated URL construction to use TargetHost directly.
src/Aspire.Hosting/Dcp/DcpExecutor.cs Replaced hardcoded "localhost" with TargetHost for allocated endpoints and updated service producer annotations.
Comments suppressed due to low confidence (1)

src/Aspire.Hosting/ProjectResourceBuilderExtensions.cs:759

  • The comment at this line is now outdated since the logic for forcing 'localhost' has been removed. Consider updating or removing the comment to avoid confusion.
                    processedHttpsPort = true;

@danegsta
Copy link
Member Author

danegsta commented Jun 21, 2025

Looks like there’s still some issue with + and other Asp.NET alternatives to 0.0.0.0 in launch settings; I’ll figure out where they’re making it into the endpoints un-normalized.

@danegsta danegsta requested a review from mitchdenny as a code owner June 23, 2025 19:27
Copy link
Member

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple things to consider, not blocking the merge IMO

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Not sure about this change yet. I've love to know what happens when dns is broken?

@dotnet-policy-service dotnet-policy-service bot added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jun 23, 2025
@danegsta
Copy link
Member Author

Not sure about this change yet. I've love to know what happens when dns is broken?

Current design falls back to "localhost" if we can't resolve a hostname for the machine. Looking at the runtime, it seems that at least in some situations, Dns.GetHostName() falls back to Environment.MachineName, which is suitable for local network addressing and doesn't require any DNS resolution to retrieve.

@danegsta
Copy link
Member Author

The biggest design choice is that I'm actively selecting IPv4 for Asp.NET specific any address syntax (+, *, etc.) where Asp.NET seems to default to IPv6. The headache is that there seems to be cases where IPv6 is technically present, but there isn't a "public" address the hostname can resolve to. In the rare event that IPv4 isn't supported at all, we'll fall back to IPv6.

@danegsta danegsta added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 24, 2025
@danegsta
Copy link
Member Author

The next DCP update will treat the machine hostname (Environment.MachineName) as equivalent to both [::] and 0.0.0.0, so this update takes advantage of that to avoid having to detect IPv4/IPv6 support. I've set NO-MERGE as things won't work entirely as expected until DCP is updated.

@danegsta danegsta closed this Jun 25, 2025
@danegsta danegsta reopened this Jun 25, 2025
@davidfowl davidfowl dismissed their stale review June 26, 2025 05:36

changes were mere!

{
if (string.Equals(host, "localhost", StringComparison.OrdinalIgnoreCase))
{
// If the host is localhost, we set it to null so that it uses the default host
Copy link
Member

@davidfowl davidfowl Jun 26, 2025

Choose a reason for hiding this comment

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

This comment is wrong

Suggested change
// If the host is localhost, we set it to null so that it uses the default host

return host;
}

// Kestrel treats any other value than localhost or an IP address as the equivalent of [::] (or 0.0.0.0 for IPv4)
Copy link
Member

Choose a reason for hiding this comment

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

+, * and machine name

sp.EndpointAnnotation.AllocatedEndpoint = new AllocatedEndpoint(
sp.EndpointAnnotation,
"localhost",
sp.EndpointAnnotation.TargetHost.Replace("0.0.0.0", hostname).Replace("[::]", hostname).Replace("::", hostname),
Copy link
Member

Choose a reason for hiding this comment

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

dcp returns all 3?

Copy link
Member

Choose a reason for hiding this comment

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

How important is this part of the change?

Copy link
Member Author

Choose a reason for hiding this comment

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

These would all be possible user configured values; main thing is that they're all forms of specifying "bind to all network interfaces", but we still need to use an actual specific address (or hostname) to resolve the resource.

We're normalizing the user provided config here rather than relying on the DCP effective address (which would do the same thing), as the timing on this logic assumes we know a valid address/endpoint at this point, which isn't guaranteed for a proxyless service that targets 0.0.0.0/[::]. This is related to the same problem as proxyless containers with random ports.

We don't support dynamic endpoint allocation yet, but the service objects in DCP may not have actually finished allocating addresses yet (in proxyless mode, DCP services need a running child resource before they can complete the allocation step). However our service discovery logic currently expects service endpoint allocation to be fully complete before creating any resources. We get around this by taking advantage of certain guarantees about the configured endpoints to allow us to "fake it" since we can know what will be allocated.

@danegsta danegsta merged commit d1261dd into dotnet:main Jun 30, 2025
252 checks passed
@dbreshears dbreshears removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Persistent sql server not accessible via loopback network device Binding container to non-localhost or 0.0.0.0 does not work
4 participants