Skip to content

Add proxyless endpoint support - #2314

Merged
Brennan (BrennanConroy) merged 7 commits into
mainfrom
brecon/proxyless
Feb 23, 2024
Merged

Add proxyless endpoint support#2314
Brennan (BrennanConroy) merged 7 commits into
mainfrom
brecon/proxyless

Conversation

@BrennanConroy

@BrennanConroy Brennan (BrennanConroy) commented Feb 20, 2024 •
edited by dotnet-policy-service Bot
Loading

Copy link
Copy Markdown
Member

Closes #1637

Microsoft Reviewers: Open in CodeFlow

@ghost ghost added the area-codeflow for labeling automated codeflow. intentionally a different color! label Feb 20, 2024
@davidfowl

David Fowler (davidfowl) commented Feb 20, 2024

Copy link
Copy Markdown
Collaborator

Can you make a playground example with this feature?

davidfowl
David Fowler (davidfowl) reviewed Feb 20, 2024
View reviewed changes
Comment thread playground/ProxylessEndToEnd/ProxylessEndToEnd.AppHost/Program.cs Outdated
{
ea.UriScheme = "http";
ea.Port = 12345;
ea.IsProxied = false;

@davidfowl David Fowler (davidfowl) Feb 20, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this the only way to use it? Can you set it using the other WithEndpoint overloads?

@BrennanConroy Brennan (BrennanConroy) Feb 20, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The other WithEndpoint overloads are a mess, bunch of default parameters. To use it with an existing one you need to do:
ApiService.Resource.Annotations.OfType<EndpointAnnotation>().First().IsProxied = false;

@davidfowl David Fowler (davidfowl) Feb 20, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we add more the optional parameter to the base overload?

davidfowl
David Fowler (davidfowl) reviewed Feb 20, 2024
View reviewed changes
Comment thread playground/ProxylessEndToEnd/ProxylessEndToEnd.AppHost/Program.cs Outdated
builder.AddProject<Projects.ProxylessEndToEnd_ApiService>("api")
.WithEndpoint("http", ea =>
{
ea.UriScheme = "http";

@davidfowl David Fowler (davidfowl) Feb 20, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this required?

@BrennanConroy Brennan (BrennanConroy) Feb 20, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes. We do "magic" today in the other WithEndpoint overload where the name is also the scheme.

davidfowl
David Fowler (davidfowl) reviewed Feb 20, 2024
View reviewed changes
Comment thread src/Aspire.Hosting/Dcp/ApplicationExecutor.cs Outdated
@davidfowl

David Fowler (davidfowl) commented Feb 20, 2024

Copy link
Copy Markdown
Collaborator

A couple of questions:

  1. What happens to the service discovery information with this change? Does it work well? Does it affect anything?
  2. What happens if I set IsProxied false and don't choose a host port? What happens when this is a container?
  3. Do we still get an AllocatedEndpoint annotation when IsProxied = false?

@BrennanConroy

Brennan (BrennanConroy) commented Feb 20, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

What happens to the service discovery information with this change? Does it work well? Does it affect anything?

Since I'm not an expert here I can't say for sure, but referencing a proxyless endpoint from another project works so far.

What happens if I set IsProxied false and don't choose a host port? What happens when this is a container?

Throws for missing port. Containers aren't proxied so the flag does nothing I think? Will have to verify behavior here.

Do we still get an AllocatedEndpoint annotation when IsProxied = false?

Yes, that's needed for .WithReference(projectWithProxylessEndpoint)

@davidfowl

David Fowler (davidfowl) commented Feb 20, 2024

Copy link
Copy Markdown
Collaborator

Since I'm not an expert here I can't say for sure, but referencing a proxyless endpoint from another project works so far.

This is what matters. We just need to make sure that calling WithReference continues to work and that URLs show up in the dashboard.

Throws for missing port. Containers aren't proxied so the flag does nothing I think? Will have to verify behavior here.

Containers are proxied by default. We have 3 ports:

  1. The proxy port
  2. The host port (you have to look in docker desktop to see this today)
  3. The container port

My assumption is that when you say IsProxied=false, for a container, should throw and force you to specify a host port.

Yes, that's needed for .WithReference(projectWithProxylessEndpoint)

Excellent. Can you add a container with a proxyless port in your playground sample.

@davidfowl David Fowler (davidfowl) mentioned this pull request Feb 20, 2024
Closed
karolz-ms
Karol Zadora-Przylecki (karolz-ms) approved these changes Feb 21, 2024
View reviewed changes

@karolz-ms Karol Zadora-Przylecki (karolz-ms) left a comment

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.

ApplicationExecutor changes look good

davidfowl
David Fowler (davidfowl) reviewed Feb 21, 2024
View reviewed changes
Comment thread tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs
}
}

[LocalOnlyFact("docker")]

@davidfowl David Fowler (davidfowl) Feb 21, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do you have any tests that use the new APIs?

@BrennanConroy Brennan (BrennanConroy) Feb 21, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

https://github.com/dotnet/aspire/pull/2314/files#diff-4af17e3376a4a26b4c6e09721a127087cae9306d3e4dd806ef76359e91a6c3e9R502
?

BrennanConroy
Brennan (BrennanConroy) commented Feb 22, 2024
View reviewed changes
Comment thread tests/Aspire.Hosting.Tests/DistributedApplicationTests.cs Outdated
@BrennanConroy
Brennan (BrennanConroy) marked this pull request as ready for review February 22, 2024 23:38
@BrennanConroy
Brennan (BrennanConroy) merged commit 8de9cb7 into main Feb 23, 2024
@BrennanConroy
Brennan (BrennanConroy) deleted the brecon/proxyless branch February 23, 2024 03:30
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@davidfowl David Fowler (davidfowl) davidfowl approved these changes
@karolz-ms Karol Zadora-Przylecki (karolz-ms) karolz-ms approved these changes
@mitchdenny Mitch Denny (mitchdenny) Awaiting requested review from mitchdenny mitchdenny is a code owner

Assignees

@BrennanConroy Brennan (BrennanConroy)

Labels

area-codeflow for labeling automated codeflow. intentionally a different color! area-orchestrator

Projects

None yet

Milestone

preview 4 (Mar)

Development

Successfully merging this pull request may close these issues.

Support proxy-less endpoints

4 participants

@BrennanConroy @davidfowl @karolz-ms @mitchdenny