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

(app-mesh): WeightedTarget construct is missing port property #26083

Closed
neovasili opened this issue Jun 22, 2023 · 3 comments · Fixed by #26114
Closed

(app-mesh): WeightedTarget construct is missing port property #26083

neovasili opened this issue Jun 22, 2023 · 3 comments · Fixed by #26114
Labels
@aws-cdk/aws-appmesh Related to AWS App Mesh effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@neovasili
Copy link
Contributor

Describe the bug

While the CfnRoute give us the ability to define a route with weighted targets with a port, the L2 construct WeightedTarget does not have that property (only have virtual_node and weight), leveraging us to use L1 constructs instead if we want to use the port specification.

Port is required for routes to nodes that have multiple listeners.

Expected Behavior

WeightedTarget L2 construct also let us specify the port option.

Current Behavior

WeightedTarget L2 construct does not have the property port.

Reproduction Steps

Example in python; the following is not valid code:

appmesh.RouteSpec.grpc(
    weighted_targets=[appmesh.WeightedTarget(virtual_node=node, port=myPortNumber)],
    match=appmesh.GrpcRouteMatch(port=myPortNumber),
)

We need to use L1 construct, to get the same result:

appmesh.CfnRoute.RouteSpecProperty(
    grpc_route=appmesh.CfnRoute.GrpcRouteProperty(
        action=appmesh.CfnRoute.GrpcRouteActionProperty(
            weighted_targets=[appmesh.CfnRoute.WeightedTargetProperty(
                virtual_node=node.virtual_node_name,
                weight=1,
                port=myPortNumber,
            )],
        ),
        match=appmesh.CfnRoute.GrpcRouteMatchProperty(port=myPortNumber),
    ),
)

Possible Solution

Add the Port property to the construct interface, since is already a property in the CloudFormation specs.

Additional Information/Context

No response

CDK CLI Version

2.85.0

Framework Version

2.85.0

Node.js Version

v16.14.2

OS

MacOs Ventura

Language

Python

Language Version

python 3.11.1

Other information

I have checked through the CDK docs that typescript CDK version have the same issue, so I guess this applies to all languages available.

I can work in a fix if there is no reason to not work on this fix.

@neovasili neovasili added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 22, 2023
@github-actions github-actions bot added the @aws-cdk/aws-appmesh Related to AWS App Mesh label Jun 22, 2023
@pahud
Copy link
Contributor

pahud commented Jun 22, 2023

Yes we should add the port property here

export interface WeightedTarget {
/**
* The VirtualNode the route points to
*/
readonly virtualNode: IVirtualNode;
/**
* The weight for the target
*
* @default 1
*/
readonly weight?: number;
}

And probably modify here as well

function renderWeightedTargets(weightedTargets: WeightedTarget[]): CfnRoute.WeightedTargetProperty[] {
const renderedTargets: CfnRoute.WeightedTargetProperty[] = [];
for (const t of weightedTargets) {
renderedTargets.push({
virtualNode: t.virtualNode.virtualNodeName,
weight: t.weight == undefined ? 1 : t.weight,
});
}
return renderedTargets;
}

@pahud pahud added p2 feature-request A feature should be added or improved. effort/medium Medium work item – several days of effort and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 22, 2023
@neovasili
Copy link
Contributor Author

Thanks @pahud!

Already created a PR with the required changes, hope this fits you well 😄

@mergify mergify bot closed this as completed in #26114 Jul 7, 2023
mergify bot pushed a commit that referenced this issue Jul 7, 2023
As described in the related issue, `WeightedTarget` L2 construct was missing `port` property which is already present in the L1 construct `CfnRoute`.

This PR adds the missing `port` property to `WeightedTarget` L2 construct.

The PR includes unit tests expansion to cover this new property appearance or absence.

Closes #26083.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Jul 7, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

tmokmss pushed a commit to tmokmss/aws-cdk that referenced this issue Jul 9, 2023
As described in the related issue, `WeightedTarget` L2 construct was missing `port` property which is already present in the L1 construct `CfnRoute`.

This PR adds the missing `port` property to `WeightedTarget` L2 construct.

The PR includes unit tests expansion to cover this new property appearance or absence.

Closes aws#26083.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
bmoffatt pushed a commit to bmoffatt/aws-cdk that referenced this issue Jul 29, 2023
As described in the related issue, `WeightedTarget` L2 construct was missing `port` property which is already present in the L1 construct `CfnRoute`.

This PR adds the missing `port` property to `WeightedTarget` L2 construct.

The PR includes unit tests expansion to cover this new property appearance or absence.

Closes aws#26083.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-appmesh Related to AWS App Mesh effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants