-
Notifications
You must be signed in to change notification settings - Fork 417
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
Enable container-to-container service communication #5628
Enable container-to-container service communication #5628
Conversation
…port in a container
src/Aspire.Hosting/ApplicationModel/ContainerNetworkResource.cs
Outdated
Show resolved
Hide resolved
Errors here:
We need to update host names in the tests |
Now the port is wrong 😄 |
Beyond the small changes to ports and host names, it seems like using the docker network slows down the startup of these containers or breaks logs in some way that causes tests to fail. |
@@ -208,7 +204,7 @@ public async Task MultipleRedisInstanceProducesCorrectRedisHostsVariable(string | |||
DistributedApplicationOperation.Run, | |||
TestServiceProvider.Instance); | |||
|
|||
Assert.Equal($"myredis1:{containerHost}:5001:0,myredis2:host2:5002:0", config["REDIS_HOSTS"]); | |||
Assert.Equal($"myredis1:{redis1.Resource.Name}:6379:0,myredis2:myredis2:6379:0", config["REDIS_HOSTS"]); |
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 don't quite understand why these ports are changing for these tests?
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.
Container to container requires using the in-container target port instead of the public port for connections
With the container network creation, is this something that might benefit from being persistent by default? What I'm thinking is that if we made it persistent it would allow developers to spin up containers manually and attach it to that network if they want to. |
Is now a good time to discuss Podman testing? At the moment all of our testing is done against Docker/Docker Desktop, but we say we support Podman but don't do any validation in our CI. It shouldn't block our PR, I'll raise an issue for it specifically. |
Long term I think it may be cleaner to work out how we want to expose container networks as a primitive in the resource builder API and allow users to configure their networks (including persistent networks to support this container attach scenario) that way. |
My main concern is that the V1 approach in this PR is taking an extremely naive approach that depends on a lot of assumptions that could fall apart if we allow non-Aspire managed containers to attach to the network. Depending on tightly managed AppHost scoped networks makes the assumptions a bit more reasonable. |
Other failures are gone! Thanks for the bug fix in dcp @danegsta! @radical the failure is: docker command 'CreateContainer' returned with non-zero exit code 1: command output: Stdout: '' Stderr: 'Unable to find image 'netaspireci.azurecr.io/library/mongo-express:1.0' locally\nError response from daemon: manifest for netaspireci.azurecr.io/library/mongo-express:1.0 not found: manifest unknown: manifest tagged by "1.0" is not found How do we get container images on our test feed? |
I can add this one.
https://github.com/radical/aspire/blob/tests-run-scripts/docs/updating-container-registry.md |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Description
This PR implements a custom container network for an Aspire AppHost run and uses it to support direct container-to-container service communication without going through service proxies on the host. This should allow scenarios such as sidecar management containers to work with more container runtime environments. For now we're hardcoding container-to-container connections in individual sidecar resources until we design and implement an updated API for connection service discovery that can better support these scenarios.
Fixes #5584
Checklist
<remarks />
and<code />
elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow