feat(appmesh): add Route matching on path, query parameters, metadata, and method name #15470

Seiya6329 · 2021-07-09T06:33:34Z

Adding new match properties for Route.

For HTTP match, adding path and queryParameters. Remove prefixPath.
For gRPC match, adding metadata and method name

BREAKING CHANGE: prefixPath property in HttpRouteMatch has been renamed to path, and its type changed from string to HttpRoutePathMatch

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

…name match properties

gitpod-io · 2021-07-09T06:33:36Z

mergify · 2021-07-09T06:34:11Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

skinny85

In general, looks great! A few suggestion that I believe will greatly simplify this code.

packages/@aws-cdk/aws-appmesh/README.md

packages/@aws-cdk/aws-appmesh/lib/http-route-path-match.ts

packages/@aws-cdk/aws-appmesh/lib/private/utils.ts

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

skinny85 · 2021-07-09T20:24:18Z

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

      match: {
-        prefix: prefixPath,
+        prefix: prefixPathMatch,
+        path: wholePathMatch,
        headers: headers?.map(header => header.bind(scope).headerMatch),
        method: this.match?.method,
        scheme: this.match?.protocol,
+        queryParameters: queryParameters?.map(queryParameter => queryParameter.bind(scope).queryParameterMatch),
      },


Here's an idea: let's leave match as undefined if either this.match is undefined, or if all of its properties are undefined.

So, this would be:

Suggested change

match: {

prefix: prefixPath,

prefix: prefixPathMatch,

path: wholePathMatch,

headers: headers?.map(header => header.bind(scope).headerMatch),

method: this.match?.method,

scheme: this.match?.protocol,

queryParameters: queryParameters?.map(queryParameter => queryParameter.bind(scope).queryParameterMatch),

},

match: allPropertiesAreUndefined(this.match) ? undefined : {

prefix: pathMatchConfig.prefixPathMatch,

path: pathMatchConfig.wholePathMatch,

headers: headers?.map(header => header.bind(scope).headerMatch),

method: this.match?.method,

scheme: this.match?.protocol,

queryParameters: queryParameters?.map(queryParameter => queryParameter.bind(scope).queryParameterMatch),

},

Actually match is the required property so would not be able to set undefined.

In addition, we set prefix to / if neither prefix and path are defined so we are guaranteed to have at least one property be defined.

I see all of the properties of the CfnGatewayRoute.HttpGatewayRouteMatchProperty are opitonal.

Is there some CloudFormation validation that errors out if an empty object is passed as its value, or is it legal to just omit all of the properties? Because if it's legal, let's just do that.

Is there some CloudFormation validation that errors out if an empty object is passed as its value

Yes, CloudFormation has a validation to prohibit passing empty value or empty array.

is it legal to just omit all of the properties

No, CloudFormation also validates if least one type of match is specified.

Since CDK has a validation to check empty array and non-array property accepts an object so I believe CDK-side has all validation that are needed.

packages/@aws-cdk/aws-appmesh/lib/private/utils.ts

Pull request has been modified.

skinny85

Looks great @Seiya6329! Just a few finishing touches, and this will be ready for merging.

packages/@aws-cdk/aws-appmesh/lib/http-route-path-match.ts

packages/@aws-cdk/aws-appmesh/lib/private/utils.ts

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

Pull request has been modified.

skinny85

Looks great @Seiya6329!

packages/@aws-cdk/aws-appmesh/lib/http-route-path-match.ts

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts

Pull request has been modified.

mergify · 2021-07-13T01:03:54Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-07-13T01:41:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 8a5e924
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-07-13T01:42:11Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Seiya6329 · 2021-07-13T05:03:27Z

@skinny85 - Thank you very much for making the change on the last suggestions!

…, and method name (aws#15470) Adding new match properties for `Route`. - For HTTP match, adding `path` and `queryParameters`. Remove `prefixPath`. - For gRPC match, adding `metadata` and `method name` BREAKING CHANGE: `prefixPath` property in `HttpRouteMatch` has been renamed to `path`, and its type changed from `string` to `HttpRoutePathMatch` ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Seiya6329 added 3 commits July 8, 2021 22:39

feature(appmesh): adding path, query parameter, metadata, and method …

26ac35d

…name match properties

Fixing doc and adding array length validation

3016916

Fixing line

9c90211

Seiya6329 changed the title ~~feature(appmesh): adding path, query parameter, metadata, and method name match properties~~ feat(appmesh): adding path, query parameter, metadata, and method name match properties Jul 9, 2021

Seiya6329 changed the title ~~feat(appmesh): adding path, query parameter, metadata, and method name match properties~~ feat(appmesh): adding path, query parameter, metadata, and method name match properties to Route Jul 9, 2021

skinny85 previously requested changes Jul 9, 2021

View reviewed changes

Renaming the startsWith method and adding a type to method

09a0dbe

Seiya6329 requested a review from skinny85 July 11, 2021 01:10

Seiya6329 added 3 commits July 12, 2021 09:43

Removing wholePathMatch variable

e1b4ed9

Removing path-compare file

e579091

Linting route-spec.ts

1893319

skinny85 previously requested changes Jul 12, 2021

View reviewed changes

Addressing the 2nd route of feedback

01d3aea

Seiya6329 requested a review from skinny85 July 12, 2021 23:41

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Jul 13, 2021

skinny85 previously approved these changes Jul 13, 2021

View reviewed changes

packages/@aws-cdk/aws-appmesh/lib/http-route-path-match.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-appmesh/lib/http-route-path-match.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-appmesh/lib/route-spec.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

8ce374c

skinny85 changed the title ~~feat(appmesh): adding path, query parameter, metadata, and method name match properties to Route~~ feat(appmesh): add Route matching on path, query parameters, metadata, and method name Jul 13, 2021

skinny85 removed the pr/do-not-merge This PR should not be merged at this time. label Jul 13, 2021

skinny85 approved these changes Jul 13, 2021

View reviewed changes

Merge branch 'master' into routeIngressV2

8a5e924

mergify bot merged commit eeeec5d into aws:master Jul 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(appmesh): add Route matching on path, query parameters, metadata, and method name #15470

feat(appmesh): add Route matching on path, query parameters, metadata, and method name #15470

Seiya6329 commented Jul 9, 2021 •

edited by skinny85

Loading

gitpod-io bot commented Jul 9, 2021

mergify bot commented Jul 9, 2021

skinny85 left a comment

skinny85 Jul 9, 2021

Seiya6329 Jul 9, 2021

skinny85 Jul 9, 2021

Seiya6329 Jul 9, 2021

Seiya6329 Jul 9, 2021

skinny85 left a comment

skinny85 left a comment

mergify bot commented Jul 13, 2021

aws-cdk-automation commented Jul 13, 2021

mergify bot commented Jul 13, 2021

Seiya6329 commented Jul 13, 2021

feat(appmesh): add Route matching on path, query parameters, metadata, and method name #15470

feat(appmesh): add Route matching on path, query parameters, metadata, and method name #15470

Conversation

Seiya6329 commented Jul 9, 2021 • edited by skinny85 Loading

gitpod-io bot commented Jul 9, 2021

mergify bot commented Jul 9, 2021

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Jul 9, 2021

Choose a reason for hiding this comment

Seiya6329 Jul 9, 2021

Choose a reason for hiding this comment

skinny85 Jul 9, 2021

Choose a reason for hiding this comment

Seiya6329 Jul 9, 2021

Choose a reason for hiding this comment

Seiya6329 Jul 9, 2021

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Jul 13, 2021

aws-cdk-automation commented Jul 13, 2021

AWS CodeBuild CI Report

mergify bot commented Jul 13, 2021

Seiya6329 commented Jul 13, 2021

Seiya6329 commented Jul 9, 2021 •

edited by skinny85

Loading