feat(apigateway): support multi-level paths for custom domains #22463

corymhall · 2022-10-11T19:46:02Z

This PR adds support for multi-level paths in api mappings for custom domains. This is a unique case because in order to create multi-level mappings for RestApis (ApiGateway v1) you have to use the ApiGateway v2 API.

The aws-apigatewayv2 package is currently an alpha module so this cannot depend on that module which is why I used the L1 level to implement this support.

I thought about deprecating the v1 api (BasePathMapping), but that is still a valid API (and is required if you have an EDGE domain name). The experience I landed on was to mostly make it transparent to users. When users create a DomainName, it will now create either a BasePathMapping or an ApiMapping depending on whether they provide a multi-level basePath. I did have to introduce a new addApiMapping method since addBasePathMapping has a return type of BasePathMapping.

I also removed the validation that prevented users from adding additional basePaths if a (none) basePath was already created. It seems like that limitation was removed at some point and I have added an integration test to confirm.

fixes #15904

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

This PR adds support for multi-level paths in api mappings for custom domains. This is a unique case because in order to create multi-level mappings for RestApis (ApiGateway v1) you have to use the ApiGateway v2 API. The aws-apigatewayv2 package is currently an alpha module so this cannot depend on that module which is why I used the L1 level to implement this support. I thought about deprecating the v1 api (BasePathMapping), but that is still a valid API (and is required if you have an EDGE domain name). The experience I landed on was to mostly make it transparent to users. When users create a DomainName, it will now create either a BasePathMapping or an ApiMapping depending on whether they provide a multi-level basePath. I did have to introduce a new `addApiMapping` method since `addBasePathMapping` has a return type of `BasePathMapping`. I also removed the validation that prevented users from adding additional basePaths if a (none) basePath was already created. It seems like that limitation was removed at some point and I have added an integration test to confirm. fixes #15904

gitpod-io · 2022-10-11T19:46:11Z

corymhall · 2022-10-11T19:48:19Z

packages/@aws-cdk/aws-apigateway/package.json

@@ -132,6 +135,11 @@
      "lib/apigatewayv2.js"
    ]
  },
+  "pkglint": {
+    "exclude": [
+      "no-experimental-dependencies"


I need to depend on the apigatewayv2 L1s. This doesn't cause aws-apigateway to depend on aws-apigatewayv2-alpha, it's just makes you fail faster if you add a dependency to an experimental V2. ubergen will strip out the L2s from aws-apigatewayv2.

Naumel

👋 I have a number of nitpicks, and a couple of comments related to validations.

Nothing that could not be followed-up on in a short PR after merging, but adding the label in case we should have a chat when everyone is online.

Naumel · 2022-10-12T09:42:38Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

+   *
+   * @default - map requests from the domain root (e.g. `example.com`).
+   */
+  readonly basePath?: string;


[nit] Should this be more restrictive and having another name?

path in the case of additional mapping options, which must be specified, allowing only the basePath mapping to be undefined.

Or perhaps we can walk through this example:

declare const acmCertificateForExampleCom: any; declare const restApi: apigateway.RestApi; declare const secondRestApi: apigateway.RestApi; const domain = new apigateway.DomainName(this, 'custom-domain', { domainName: 'example.com', certificate: acmCertificateForExampleCom, mapping: restApi, }); domain.addApiMapping(secondRestApi.deploymentStage, { // not specified, thus undefined, just like above });

Which API gets to be called from /?

What about apiPath instead? In your example the addApiMapping will throw an error because you are trying to map undefined which has already been mapped when you created the DomainName.

Naumel · 2022-10-12T09:45:29Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

      this.addBasePathMapping(props.mapping, {
        basePath: props.basePath,
      });
+    } else if (props.mapping && multiLevel) {
+      this.addApiMapping(props.mapping.deploymentStage, {
+        basePath: props.basePath,


[nit] The renaming point would (for me) make it easier to read the snippet here.

Naumel · 2022-10-12T09:46:21Z

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

+      if (this.endpointType === EndpointType.EDGE) {
+        throw new Error('multi-level basePath is only supported when endpointType is EndpointType.REGIONAL');
+      }
+      if (this.securityPolicy && this.securityPolicy !== SecurityPolicy.TLS_1_2) {


[nit] Should this be a class constant and just referenced here?

What do you mean?

packages/@aws-cdk/aws-apigateway/lib/domain-name.ts

mergify · 2022-10-18T17:03:31Z

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

aws-cdk-automation · 2022-10-19T00:59:56Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0fe3a4f
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-10-19T01:00:16Z

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

…2463) This PR adds support for multi-level paths in api mappings for custom domains. This is a unique case because in order to create multi-level mappings for RestApis (ApiGateway v1) you have to use the ApiGateway v2 API. The aws-apigatewayv2 package is currently an alpha module so this cannot depend on that module which is why I used the L1 level to implement this support. I thought about deprecating the v1 api (BasePathMapping), but that is still a valid API (and is required if you have an EDGE domain name). The experience I landed on was to mostly make it transparent to users. When users create a DomainName, it will now create either a BasePathMapping or an ApiMapping depending on whether they provide a multi-level basePath. I did have to introduce a new `addApiMapping` method since `addBasePathMapping` has a return type of `BasePathMapping`. I also removed the validation that prevented users from adding additional basePaths if a (none) basePath was already created. It seems like that limitation was removed at some point and I have added an integration test to confirm. fixes aws#15904 ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation requested a review from a team October 11, 2022 19:46

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Oct 11, 2022

Merge branch 'main' into corymhall/apigateway/basepathmappings

3bad1d8

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 11, 2022

corymhall commented Oct 11, 2022

View reviewed changes

Naumel added the pr/do-not-merge This PR should not be merged at this time. label Oct 12, 2022

Naumel approved these changes Oct 12, 2022

View reviewed changes

updating multi-path validation logic

d1bc22c

corymhall removed the pr/do-not-merge This PR should not be merged at this time. label Oct 18, 2022

Merge branch 'main' into corymhall/apigateway/basepathmappings

0fe3a4f

mergify bot merged commit cdc5753 into aws:main Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(apigateway): support multi-level paths for custom domains #22463

feat(apigateway): support multi-level paths for custom domains #22463

corymhall commented Oct 11, 2022

gitpod-io bot commented Oct 11, 2022

corymhall Oct 11, 2022

Naumel left a comment

Naumel Oct 12, 2022

corymhall Oct 12, 2022

Naumel Oct 12, 2022

Naumel Oct 12, 2022

corymhall Oct 12, 2022

mergify bot commented Oct 18, 2022

aws-cdk-automation commented Oct 19, 2022

mergify bot commented Oct 19, 2022

feat(apigateway): support multi-level paths for custom domains #22463

feat(apigateway): support multi-level paths for custom domains #22463

Conversation

corymhall commented Oct 11, 2022

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Oct 11, 2022

corymhall Oct 11, 2022

Choose a reason for hiding this comment

Naumel left a comment

Choose a reason for hiding this comment

Naumel Oct 12, 2022

Choose a reason for hiding this comment

corymhall Oct 12, 2022

Choose a reason for hiding this comment

Naumel Oct 12, 2022

Choose a reason for hiding this comment

Naumel Oct 12, 2022

Choose a reason for hiding this comment

corymhall Oct 12, 2022

Choose a reason for hiding this comment

mergify bot commented Oct 18, 2022

aws-cdk-automation commented Oct 19, 2022

AWS CodeBuild CI Report

mergify bot commented Oct 19, 2022