-
Notifications
You must be signed in to change notification settings - Fork 753
Enable universal support for container-to-host communication #12361
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
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 12361Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 12361" |
14dcd58 to
ab6ad2f
Compare
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 enables universal container-to-host communication by leveraging a new container tunnel capability in DCP. The changes introduce a network-aware model where EndpointReference objects are resolved in the context of specific networks (represented by NetworkIdentifier). This allows endpoints to be resolved differently depending on whether they're accessed from localhost, container networks, or public internet contexts.
Key changes:
- Introduced
NetworkIdentifiertype and network-aware value resolution viaINetworkAwareValueProvider - Modified
EndpointReferenceandEndpointAnnotationto support multiple allocated endpoints per network context - Added container tunnel proxy infrastructure to enable container-to-host connectivity
- Refactored expression resolution to be network-context-aware instead of using a single "container host name"
Reviewed Changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting/ApplicationModel/Network.cs | Introduces NetworkIdentifier and KnownNetworkIdentifiers/KnownHostNames for network context modeling |
| src/Aspire.Hosting/ApplicationModel/EndpointAnnotation.cs | Adds support for multiple allocated endpoints per network and tracks endpoint references |
| src/Aspire.Hosting/ApplicationModel/EndpointReference.cs | Modified to resolve endpoints in specific network contexts |
| src/Aspire.Hosting/ApplicationModel/ExpressionResolver.cs | Refactored to use NetworkIdentifier instead of containerHostName parameter |
| src/Aspire.Hosting/ApplicationModel/INetworkAwareValueProvider.cs | New interface for network-aware value resolution |
| src/Aspire.Hosting/Dcp/DcpExecutor.cs | Major refactoring to create container tunnels and handle network-specific endpoint allocation |
| src/Aspire.Hosting/Dcp/Model/ContainerTunnel.cs | New DCP model types for container tunnel proxy functionality |
| tests/**/*.cs | Updated tests to use new network-aware APIs and fix broken assumptions about container host names |
Comments suppressed due to low confidence (1)
src/Aspire.Hosting/ApplicationModel/EndpointReference.cs:1
- Add space between XML tag and 'object' in the returns documentation
// Licensed to the .NET Foundation under one or more agreements.
src/Aspire.Hosting/ApplicationModel/INetworkAwareValueProvider.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/INetworkAwareValueProvider.cs
Outdated
Show resolved
Hide resolved
992e539 to
2ad7c72
Compare
src/Aspire.Hosting/ApplicationModel/ResourceUrlsCallbackContext.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ResourceUrlsCallbackContext.cs
Outdated
Show resolved
Hide resolved
8b77a29 to
157de6d
Compare
| /// <summary> | ||
| /// The identifier of the network that serves as the context for value resolution. | ||
| /// </summary> | ||
| public NetworkIdentifier? Network { get; init; } |
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.
I assume we expect this value to not be set in certain scenarios. For example, I'd expect this to be null during deployment mode. If so, how do we think about modeling the ValueProviderContext under different execution contexts?
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.
It is a property bag-like, so I can imagine stuffing some deployment-mode data into it in future. Maybe, as you suggested offline, have a proper property bag (dictionary) with data on it too.
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.
I think we could model this like:
class ValueProviderContext
{
public IServiceProvider? Services { get; init; }
public IResource? Caller { get; init; }
}
class NetworkValueProviderContext : ValueProviderContext
{
public NetworkIdentifier? Network { get; init; }
} 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.
cc @eerhardt
| .WithHttpEndpoint(name: "primary") | ||
| .WithEndpoint("primary", ep => | ||
| { | ||
| ep.AllocatedEndpoint = new AllocatedEndpoint(ep, "localhost", 90, targetPortExpression: """{{- portForServing "container1_primary" -}}"""); |
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.
Is this needed? What happens if we leave this as is?
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.
Yes. If this change is reverted, the test will hang forever, waiting for endpoint address to be allocated.
The reason is we are simulating endpoint allocation and then testing container-to-container communication. So we need an AllocatedEndpoint in container network space.
Endpoint.AllocatedEndpoint = something allocates endpoint in localhost network space.
Description
Leverages the container tunnel capability that has been recently added to DCP to provide container-to-host connectivity independent from what is supported by container orchestrators.
To make this work, I had to make some changes to how
EndpointReferenceswork. Specifically,EndpointReferencesare not resolved in context of a network (represented by abstractNetworkIdentifier). They are also tracked by theirEndpointAnnotationso we can answer questions like "who is referencing this particularEndpoint".Marking as draft for now because this is a big change that affects the core of Aspire application model. It needs more testing, automated tests, and most likely, fixes to existing tests.
Fixes #6547
Checklist
<remarks />and<code />elements on your triple slash comments?doc-ideatemplatebreaking-changetemplatediagnostictemplate