-
Notifications
You must be signed in to change notification settings - Fork 652
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
Fix support for non-localhost endpoint targets #9977
Conversation
There was a problem hiding this 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;
Looks like there’s still some issue with |
There was a problem hiding this 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
There was a problem hiding this 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?
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, |
The biggest design choice is that I'm actively selecting IPv4 for Asp.NET specific any address syntax ( |
The next DCP update will treat the machine hostname ( |
{ | ||
if (string.Equals(host, "localhost", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
// If the host is localhost, we set it to null so that it uses the default host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is wrong
// 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) |
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dcp returns all 3?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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
<remarks />
and<code />
elements on your triple slash comments?doc-idea
templatebreaking-change
templatediagnostic
template