Skip to content

Workaround container networking limitations#1203

Merged
davidfowl merged 5 commits intomainfrom
davidfowl/container-networking
Dec 6, 2023
Merged

Workaround container networking limitations#1203
davidfowl merged 5 commits intomainfrom
davidfowl/container-networking

Conversation

@davidfowl
Copy link
Copy Markdown
Contributor

@davidfowl davidfowl commented Dec 5, 2023

  • Until we setup container networking, make sure containers can talk to other containers when using WithReference. This won't work when using raw strings/urls.

Preview2 mitigation for #128

Fixes #1197

@ghost ghost added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Dec 5, 2023
@davidfowl davidfowl requested a review from karolz-ms December 5, 2023 06:45
@mitchdenny
Copy link
Copy Markdown
Member

Taking bets on how long this work around will hang around for?

Comment on lines +72 to 80
// PrepareProxylessServices();

await CreateContainerSingletonsAsync(cancellationToken).ConfigureAwait(false);
// await CreateContainerSingletonsAsync(cancellationToken).ConfigureAwait(false);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bwateratmsft @karolz-ms I ended up disabling this to make the PR work. Any guidance here on how this is supposed to work would be appreciated.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a conversation about this in #886. Long story short, we need more time and design to enable this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI unused code was causing code analysis warnings. I've removed it here - #1286. Source is still available in the file history 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Comment thread src/Aspire.Hosting/Utils/HostNameResolver.cs Outdated
Comment on lines +72 to 80
// PrepareProxylessServices();

await CreateContainerSingletonsAsync(cancellationToken).ConfigureAwait(false);
// await CreateContainerSingletonsAsync(cancellationToken).ConfigureAwait(false);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had a conversation about this in #886. Long story short, we need more time and design to enable this.

- Until we setup container networking, make sure containers can talk to other containers when using WithReference. This won't work when using raw strings/urls.
@davidfowl davidfowl force-pushed the davidfowl/container-networking branch from 6d5bb03 to 0958953 Compare December 6, 2023 00:39
@davidfowl davidfowl marked this pull request as ready for review December 6, 2023 01:43
@davidfowl
Copy link
Copy Markdown
Contributor Author

cc @eerhardt This change disables proxyless containers for preview2 until we have more runway to fix the other issues

@davidfowl davidfowl merged commit a3ed2b5 into main Dec 6, 2023
@davidfowl davidfowl deleted the davidfowl/container-networking branch December 6, 2023 01:47
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AddReference between containers

4 participants