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): Missing port property in GrpcRouteMatch construct #25810

Closed
neovasili opened this issue Jun 1, 2023 · 7 comments · Fixed by #25868
Closed

(app-mesh): Missing port property in GrpcRouteMatch construct #25810

neovasili opened this issue Jun 1, 2023 · 7 comments · Fixed by #25868
Labels
@aws-cdk/aws-appmesh Related to AWS App Mesh effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@neovasili
Copy link
Contributor

Describe the bug

While the CfnRoute give us the ability to define a gRPC match route rule specifying the port, the L2 construct GrpcRouteMatch does not have that property (only have service_name, method_name and metadata), leveraging us to use L1 constructs instead if we want to use the port matcher.

Expected Behavior

GrpcRouteMatch L2 construct also let us specify the port matcher option.

Current Behavior

GrpcRouteMatch L2 construct does not have the property port.

Reproduction Steps

Example in python; the following is not valid code:

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

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,
            )],
        ),
        match=appmesh.CfnRoute.GrpcRouteMatchProperty(port=1234),
    ),
)

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.81.0

Framework Version

2.81.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.

@neovasili neovasili added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 1, 2023
@github-actions github-actions bot added the @aws-cdk/aws-appmesh Related to AWS App Mesh label Jun 1, 2023
@peterwoodworth peterwoodworth added good first issue Related to contributions. See CONTRIBUTING.md p2 effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 1, 2023
@peterwoodworth
Copy link
Contributor

Should be real simple to add, contributions are welcome

@neovasili
Copy link
Contributor Author

Thanks @peterwoodworth, I will try to create a PR with the required changes.

@neovasili
Copy link
Contributor Author

Created PR with the suggested change 😄

@peterwoodworth
Copy link
Contributor

Thanks for the PR! We'll be able to give it a full review once its passing the build, but if you have any questions or need help getting to that state feel free to ping me and ask for help

@neovasili
Copy link
Contributor Author

neovasili commented Jun 7, 2023

Thanks for the PR! We'll be able to give it a full review once its passing the build, but if you have any questions or need help getting to that state feel free to ping me and ask for help

I've updated the PR "fixing integration tests" and adding the port property to another construct I spotted during the process, but the build is still failing, although is kinda hard to find the issue, @peterwoodworth can you help me understand why is failing?

@neovasili
Copy link
Contributor Author

Hi @peterwoodworth,

Build is passing now 😄 please let me know if anything else is required.

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

This PR adds the missing `port` property to `GrpcRouteMatch` L2 construct and also adds `port` property to `GrpcGatewayRouteMatch` and `HttpRouteMatch` L2 constructs that were also missing it.

The PR includes unit and integration tests expansion to cover this new property plus a reference to the property in the appmesh README file.

Closes #25810.

----

*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

⚠️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.

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/small Small work item – less than a day of effort feature-request A feature should be added or improved. good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants