Allow HostUrl to remap both address and port #12521
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12521Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12521" |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors network context handling in value resolution by consolidating the networkContext parameter into the ValueProviderContext object. The main change is replacing the separate NetworkIdentifier? networkContext parameter with logic that derives the network identifier from the context using a new GetNetworkIdentifier() extension method.
Key changes:
- Introduced
ValueProviderExtensions.GetNetworkIdentifier()helper to extract network context fromValueProviderContext - Removed
networkContextparameters from multiple methods inResourceExtensions.csand replaced calls with context-based resolution - Updated
ValueProviderContextto includeExecutionContextproperty instead ofServices - Enhanced
HostUrlwith more sophisticated container host name mapping logic that accounts for DCP configuration
Reviewed Changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting/ApplicationModel/ValueProviderExtensions.cs | New extension method to extract NetworkIdentifier from ValueProviderContext |
| src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs | Removed networkContext parameters from multiple methods; updated to use context-based resolution |
| src/Aspire.Hosting/ApplicationModel/IValueProvider.cs | Changed ValueProviderContext.Services to ExecutionContext |
| src/Aspire.Hosting/ApplicationModel/HostUrl.cs | Enhanced logic to resolve container host names using DCP configuration and endpoint mapping |
| src/Aspire.Hosting/ApplicationModel/EndpointReference.cs | Simplified network context resolution using GetNetworkIdentifier helper |
| src/Aspire.Hosting/ApplicationModel/ConnectionStringReference.cs | Removed intermediate method and simplified to pass context directly |
| src/Aspire.Hosting/ApplicationModel/IResourceWithConnectionString.cs | Simplified GetValueAsync to pass context through directly |
| src/Aspire.Hosting/ApplicationModel/ReferenceExpression.cs | Minor code style change using target-typed new expression |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Updated container host name logic to use DCP info; cleaned up whitespace; added executionContext parameter |
| src/Aspire.Hosting/OtlpConfigurationExtensions.cs | Removed trailing whitespace |
karolz-ms
left a comment
karolz-ms
left a comment
There was a problem hiding this comment.
Looks great overall, just a few questions and suggestions
| { | ||
| // We need to consider that both the host and port may need to be remapped | ||
| var model = context.ExecutionContext.ServiceProvider.GetRequiredService<DistributedApplicationModel>(); | ||
| var targetEndpoint = model.Resources.Where(r => !r.IsContainer()) |
There was a problem hiding this comment.
Why do we need to scan the entire model??
There was a problem hiding this comment.
All we know in HostUrl is an address and port, but both the address and port may need to change if running in a container. So I'm finding out if it's the URL to an existing endpoint and then mapping that to the appropriate container network endpoint if one exists. Ideally we'd just have a way to get the endpoints from the dashboard directly here and deprecate HostUrl entirely, but that's a larger change.
|
/backport to release/13.0 |
|
Started backporting to release/13.0: https://github.com/dotnet/aspire/actions/runs/18964385455 |
3 participants
When dealing with
HostUrlvalues (such as for the otel endpoint references) we need to consider that we may have to remap both the address and port if in a tunnel scenario. This PR allows us to find a referenced by port and remap to the equivalent endpoint on a separate network context.