Skip to content

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Feb 20, 2024

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
Copy link
Member

Can you make a playground example with this feature?

{
ea.UriScheme = "http";
ea.Port = 12345;
ea.IsProxied = false;
Copy link
Member

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?

Copy link
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;

Copy link
Member

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?

builder.AddProject<Projects.ProxylessEndToEnd_ApiService>("api")
.WithEndpoint("http", ea =>
{
ea.UriScheme = "http";
Copy link
Member

Choose a reason for hiding this comment

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

Is this required?

Copy link
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
Copy link
Member

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
Copy link
Member Author

BrennanConroy commented Feb 20, 2024

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
Copy link
Member

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.

Copy link
Member

@karolz-ms karolz-ms left a comment

Choose a reason for hiding this comment

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

ApplicationExecutor changes look good

@@ -454,6 +455,185 @@ await foreach(var resource in s.WatchAsync<Executable>(cancellationToken: token)
}
}

[LocalOnlyFact("docker")]
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

@BrennanConroy BrennanConroy marked this pull request as ready for review February 22, 2024 23:38
@mitchdenny mitchdenny added this to the preview 4 (Mar) milestone Feb 23, 2024
@BrennanConroy BrennanConroy merged commit 8de9cb7 into main Feb 23, 2024
@BrennanConroy 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.
Labels
area-codeflow for labeling automated codeflow. intentionally a different color! area-orchestrator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support proxy-less endpoints
4 participants