-
Notifications
You must be signed in to change notification settings - Fork 463
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
Connection strings vary based on the target resource #259
Conversation
- Today connection strings are generated no matter assuming localhost is accessible from service to service. That doesn't work for container to connection comms. This passes the target resource to IDistributedApplicationResourceWithConnectionString so that it can determine the best connection string to return given the target resource.
Are you thinking this might be a solution to service binding as well? |
Jury's still out on that one, I'm not sure how we handle 1:N vs 1:many situations with service endpoints as yet. The interface could be more general purpose, but IMO that defeats the purpose of the abstraction, I think. |
// Can't use localhost because the container is running in a different network namespace. | ||
// Eventually we'll want to use the container name, but that requires a network between containers. | ||
var address = targetResource is null || !targetResource.IsContainer() ? "127.0.0.1" : "host.docker.internal"; | ||
|
||
// HACK: Use the 127.0.0.1 address because localhost is resolving to [::1] following |
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.
Should this HACK comment over up above var address
?
@@ -16,6 +16,9 @@ public string GetConnectionString() | |||
|
|||
// We should only have one endpoint for Redis for local scenarios. | |||
var endpoint = allocatedEndpoints.Single(); | |||
return endpoint.EndPointString; | |||
|
|||
var address = targetResource is null || !targetResource.IsContainer() ? endpoint.Address : "host.docker.internal"; |
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.
Does this mean we will only ever support Docker? What about other container techonlogies?
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.
There are 2, docker and podman, and when we support both we would change this to detect and be the equivalent.
@@ -5,5 +5,5 @@ namespace Aspire.Hosting.ApplicationModel; | |||
|
|||
public interface IDistributedApplicationResourceWithConnectionString : IDistributedApplicationResource | |||
{ | |||
public string? GetConnectionString(); | |||
public string? GetConnectionString(IDistributedApplicationResource? targetResource); |
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.
Why do you sometimes have IDistributedApplicationResource? targetResource
and sometimes have IDistributedApplicationResource? targetResource = null
?
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 got lazy
I'll bring this back later. |
Related to #128
This is a PR to discuss implications on API if we go this route. The other solution would be to always use host.docker.internal for container based connection strings.