Skip to content
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

Do not set TargetPort to Port when IsProxying is also set #3372

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

ReubenBond
Copy link
Member

@ReubenBond ReubenBond commented Apr 3, 2024

When creating endpoint annotations, if a TargetPort is equal to the Port for a non-Container resource (and non-null) and IsProxying is true, that is an error: DCP cannot bind a proxy to a port which the target service is also bound to.

This PR does two things:

  1. We do not set TargetPort to Port automatically if IsProxying is true.
  2. If the resource is not a container and TargetPort == Port && IsProxying, then we throw during startup
Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Apr 3, 2024
@ReubenBond ReubenBond changed the title Always use expression when referencing endpoint port in environment variable Do not set TargetPort to Port when IsProxying is also set Apr 3, 2024
@dotnet-policy-service dotnet-policy-service bot added the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 3, 2024
@ReubenBond ReubenBond marked this pull request as draft April 4, 2024 03:46
@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 4, 2024
@ReubenBond ReubenBond marked this pull request as ready for review April 4, 2024 16:39
@ReubenBond ReubenBond enabled auto-merge (squash) April 5, 2024 21:01
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@ReubenBond ReubenBond merged commit 81e680e into main Apr 5, 2024
8 checks passed
@ReubenBond ReubenBond deleted the rebond/fix-node-test branch April 5, 2024 22:34
@davidfowl
Copy link
Member

Can you re gen the manifests? I want to see if there are any changes

@davidfowl
Copy link
Member

cc @vhvb1989

@ReubenBond
Copy link
Member Author

Can you re gen the manifests?

Is there a way to do that across all projects? We have some tests which check manifest output and they did not change

eerhardt added a commit to eerhardt/aspire that referenced this pull request Apr 5, 2024
* Do not set TargetPort to Port when IsProxied is true

* Move TargetPort check to ApplicationExecutor

* Apply port selection logic to manifest publishing

(cherry picked from commit 09f5ae9)

* Update src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

Co-authored-by: Eric Erhardt <[email protected]>

---------

Co-authored-by: Eric Erhardt <[email protected]>
@eerhardt
Copy link
Member

eerhardt commented Apr 5, 2024

Can you re gen the manifests?

Is there a way to do that across all projects? We have some tests which check manifest output and they did not change

We should really add an official way to do it to the repo, but here's a hack powershell script that I used a few weeks ago:

$playground_apps = @(
    "C:\git\aspire2\playground\AWS\AWS.AppHost\",
    "C:\git\aspire2\playground\AzureSearchEndToEnd\AzureSearch.AppHost\",
    "C:\git\aspire2\playground\AzureStorageEndToEnd\AzureStorageEndToEnd.AppHost\",
    "C:\git\aspire2\playground\bicep\BicepSample.AppHost\",
    "C:\git\aspire2\playground\cdk\CdkSample.AppHost\",
    "C:\git\aspire2\playground\CosmosEndToEnd\CosmosEndToEnd.AppHost\",
    "C:\git\aspire2\playground\CustomResources\CustomResources.AppHost",
    "C:\git\aspire2\playground\dapr\AppHost\",
    "C:\git\aspire2\playground\DatabaseMigration\DatabaseMigration.AppHost",
    "C:\git\aspire2\playground\mongo\Mongo.AppHost\",
    "C:\git\aspire2\playground\mysql\MySqlDb.AppHost\",
    "C:\git\aspire2\playground\nats\Nats.AppHost\",
    "C:\git\aspire2\playground\OpenAIEndToEnd\OpenAIEndToEnd.AppHost\",
    "C:\git\aspire2\playground\orleans\OrleansAppHost",
    "C:\git\aspire2\playground\ParameterEndToEnd\ParameterEndToEnd.AppHost\",
    "C:\git\aspire2\playground\PostgresEndToEnd\PostgresEndToEnd.AppHost\",
    "C:\git\aspire2\playground\ProxylessEndToEnd\ProxylessEndToEnd.AppHost\",
    "C:\git\aspire2\playground\seq\Seq.AppHost\",
    "C:\git\aspire2\playground\signalr\SignalRAppHost\",
    "C:\git\aspire2\playground\SqlServerEndToEnd\SqlServerEndToEnd.AppHost\",
    "C:\git\aspire2\playground\TestShop\AppHost\")

foreach ($playground_app in $playground_apps)
{
Push-Location $playground_app
dotnet run -- --publisher manifest --output-path aspire-manifest.json
Pop-Location
}

@ReubenBond
Copy link
Member Author

I ran the script and compared main vs prior to this commit and got the same results.

davidfowl added a commit that referenced this pull request Apr 6, 2024
* Added ability to resolve target port (#3305)

* Added ability to resolve target port
- Change how we process and expose target ports. This change makes it possible to avoid using any internals of DCP to access and resolve target ports from any resource.
- More launch profile processing to the project resource defaults.
- Remove logic in the manifest writing that wrote a special env variable. This should just work now  like everything else.
- Exposed TargetPort and Exists on EndpointReference.
- Use this new target property with dapr.
- Added a ReferenceExpressionBuilder which makes it possible to dynamically build a ReferenceExpression.
- Endpoints and env are evaulated earlier now so change remove
tests that add default launch profile on real projects
without allocating those endpoints.

* Remove more uses of TestProgram

* Fix tests

* Rename DcpServiceName to TargetPortExpression (#3371)

This allows us to not leak "DCP" into the public API and keeps DCP-isms contained in the DCP code.

* Do not set TargetPort to Port when IsProxying is also set (#3372)

* Do not set TargetPort to Port when IsProxied is true

* Move TargetPort check to ApplicationExecutor

* Apply port selection logic to manifest publishing

(cherry picked from commit 09f5ae9)

* Update src/Aspire.Hosting/Dcp/ApplicationExecutor.cs

Co-authored-by: Eric Erhardt <[email protected]>

---------

Co-authored-by: Eric Erhardt <[email protected]>

---------

Co-authored-by: David Fowler <[email protected]>
Co-authored-by: Reuben Bond <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators May 8, 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.

4 participants