fix(apigatewayv2): Invalid mapping key value (#9141)

- `mappingKey` is now optional - undefinied `mappingKey` indicates the root base path mapping for custom domain - ensure the resource dependency for the api mapping and stage Fixes: #8983 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Jul 21, 2020 · c88ad5f · c88ad5f
1 parent 63a00d3
commit c88ad5f
Show file tree

Hide file tree

Showing 8 changed files with 264 additions and 110 deletions.
diff --git a/packages/@aws-cdk/aws-apigatewayv2/README.md b/packages/@aws-cdk/aws-apigatewayv2/README.md
@@ -156,10 +156,10 @@ const dn = new DomainName(stack, 'DN', {
 
 const api = new HttpApi(stack, 'HttpProxyProdApi', {
   defaultIntegration: new LambdaProxyIntegration({ handler }),
-  // https://${dn.domainName} goes to prodApi $default stage
+  // https://${dn.domainName}/foo goes to prodApi $default stage
   defaultDomainMapping: {
     domainName: dn,
-    mappingKey: '/',
+    mappingKey: 'foo',
   },
 });
 ```
@@ -170,10 +170,10 @@ To associate a specifc `Stage` to a custom domain mapping -
 api.addStage('beta', {
   stageName: 'beta',
   autoDeploy: true,
-  // https://${dn.domainName}/beta goes to the beta stage
+  // https://${dn.domainName}/bar goes to the beta stage
   domainMapping: {
     domainName: dn,
-    mappingKey: 'beta',
+    mappingKey: 'bar',
   },
 });
 ```
@@ -191,12 +191,12 @@ const apiDemo = new HttpApi(stack, 'DemoApi', {
 });
 ```
 
-The `mappingKey` determines the `path` of the URL with the custom domain. Each custom domain is only allowed
-to have one API mapping with the root(/) `mappingKey`. In the sample above, the custom domain is associated
+The `mappingKey` determines the base path of the URL with the custom domain. Each custom domain is only allowed
+to have one API mapping with undefined `mappingKey`. If more than one API mappings are specified, `mappingKey` will be required for all of them. In the sample above, the custom domain is associated
 with 3 API mapping resources across different APIs and Stages.
 
 |        API     |     Stage   |   URL  |
 | :------------: | :---------: | :----: |
-| api | $default  |   `https://${domainName}`  |
-| api | beta  |   `https://${domainName}/beta`  |
+| api | $default  |   `https://${domainName}/foo`  |
+| api | beta  |   `https://${domainName}/bar`  |
 | apiDemo | $default  |   `https://${domainName}/demo`  |
diff --git a/packages/@aws-cdk/aws-apigatewayv2/lib/http/api-mapping.ts b/packages/@aws-cdk/aws-apigatewayv2/lib/http/api-mapping.ts
@@ -10,7 +10,7 @@ import { IHttpStage } from './stage';
 export interface HttpApiMappingProps {
   /**
    * Api mapping key. The path where this stage should be mapped to on the domain
-   * @default '/'
+   * @default - undefined for the root path mapping.
    */
   readonly apiMappingKey?: string;
 
@@ -61,18 +61,48 @@ export class HttpApiMapping extends Resource implements IApiMapping {
    */
   public readonly apiMappingId: string;
 
+  /**
+   * API Mapping key
+   */
+  public readonly mappingKey?: string;
+
   constructor(scope: Construct, id: string, props: HttpApiMappingProps) {
     super(scope, id);
 
+    if ((!props.stage?.stageName) && !props.api.defaultStage) {
+      throw new Error('stage is required if default stage is not available');
+    }
+
+    const paramRe = '^[a-zA-Z0-9]*[-_.+!,$]?[a-zA-Z0-9]*$';
+    if (props.apiMappingKey && !new RegExp(paramRe).test(props.apiMappingKey)) {
+      throw new Error('An ApiMapping key may contain only letters, numbers and one of $-_.+!*\'(),');
+    }
+
+    if (props.apiMappingKey === '') {
+      throw new Error('empty string for api mapping key not allowed');
+    }
+
     const apiMappingProps: CfnApiMappingProps = {
       apiId: props.api.httpApiId,
       domainName: props.domainName.domainName,
-      stage: props.stage?.stageName ?? '$default',
+      stage: props.stage?.stageName ?? props.api.defaultStage!.stageName,
       apiMappingKey: props.apiMappingKey,
     };
 
     const resource = new CfnApiMapping(this, 'Resource', apiMappingProps);
+
+    // ensure the dependency on the provided stage
+    if (props.stage) {
+      this.node.addDependency(props.stage);
+    }
+
+    // if stage not specified, we ensure the default stage is ready before we create the api mapping
+    if (!props.stage?.stageName && props.api.defaultStage) {
+      this.node.addDependency(props.api.defaultStage!);
+    }
+
     this.apiMappingId = resource.ref;
+    this.mappingKey = props.apiMappingKey;
   }
 
 }
diff --git a/packages/@aws-cdk/aws-apigatewayv2/lib/http/api.ts b/packages/@aws-cdk/aws-apigatewayv2/lib/http/api.ts
@@ -14,6 +14,11 @@ export interface IHttpApi extends IResource {
    * @attribute
    */
   readonly httpApiId: string;
+
+  /**
+   * The default stage
+   */
+  readonly defaultStage?: HttpStage;
 }
 
 /**
@@ -129,7 +134,7 @@ export class HttpApi extends Resource implements IHttpApi {
   /**
    * default stage of the api resource
    */
-  private readonly defaultStage: HttpStage | undefined;
+  public readonly defaultStage: HttpStage | undefined;
 
   constructor(scope: Construct, id: string, props?: HttpApiProps) {
     super(scope, id);
@@ -179,6 +184,11 @@ export class HttpApi extends Resource implements IHttpApi {
         autoDeploy: true,
         domainMapping: props?.defaultDomainMapping,
       });
+
+      // to ensure the domain is ready before creating the default stage
+      if(props?.defaultDomainMapping) {
+        this.defaultStage.node.addDependency(props.defaultDomainMapping.domainName);
+      }
     }
 
     if (props?.createDefaultStage === false && props.defaultDomainMapping) {

diff --git a/packages/@aws-cdk/aws-apigatewayv2/lib/http/stage.ts b/packages/@aws-cdk/aws-apigatewayv2/lib/http/stage.ts
@@ -45,11 +45,10 @@ export interface DefaultDomainMappingOptions {
   readonly domainName: IDomainName;
 
   /**
-   * The API mapping key. Specify '/' for the root path mapping.
-   *
+   * The API mapping key. Leave it undefined for the root path mapping.
+   * @default - empty key for the root path mapping
    */
-  readonly mappingKey: string;
-
+  readonly mappingKey?: string;
 }
 
 /**
@@ -103,6 +102,8 @@ export class HttpStage extends Resource implements IStage {
         stage: this,
         apiMappingKey: props.domainMapping.mappingKey,
       });
+      // ensure the dependency
+      this.node.addDependency(props.domainMapping.domainName);
     }
 
   }

diff --git a/packages/@aws-cdk/aws-apigatewayv2/test/http/api-mapping.test.ts b/packages/@aws-cdk/aws-apigatewayv2/test/http/api-mapping.test.ts
@@ -64,6 +64,139 @@ describe('ApiMapping', () => {
     });
   });
 
+  test('apiMappingKey validation - empty string not allowed', () => {
+
+    const stack = new Stack();
+    const api = new HttpApi(stack, 'Api');
+
+    const dn = new DomainName(stack, 'DomainName', {
+      domainName,
+      certificate: Certificate.fromCertificateArn(stack, 'cert', certArn),
+    });
+
+    expect(() => {
+      new HttpApiMapping(stack, 'Mapping', {
+        api,
+        domainName: dn,
+        apiMappingKey: '',
+      });
+    }).toThrow(/empty string for api mapping key not allowed/);
+  });
+
+  test('apiMappingKey validation - single slash not allowed', () => {
+
+    const stack = new Stack();
+    const api = new HttpApi(stack, 'Api');
+
+    const dn = new DomainName(stack, 'DomainName', {
+      domainName,
+      certificate: Certificate.fromCertificateArn(stack, 'cert', certArn),
+    });
+
+    expect(() => {
+      new HttpApiMapping(stack, 'Mapping', {
+        api,
+        domainName: dn,
+        apiMappingKey: '/',
+      });
+    }).toThrow(/An ApiMapping key may contain only letters, numbers and one of/);
+  });
+
+  test('apiMappingKey validation - prefix slash not allowd', () => {
+
+    const stack = new Stack();
+    const api = new HttpApi(stack, 'Api');
+
+    const dn = new DomainName(stack, 'DomainName', {
+      domainName,
+      certificate: Certificate.fromCertificateArn(stack, 'cert', certArn),
+    });
+
+    expect(() => {
+      new HttpApiMapping(stack, 'Mapping', {
+        api,
+        domainName: dn,
+        apiMappingKey: '/foo',
+      });
+    }).toThrow(/An ApiMapping key may contain only letters, numbers and one of/);
+  });
+
+  test('apiMappingKey validation - slash in the middle not allowed', () => {
+
+    const stack = new Stack();
+    const api = new HttpApi(stack, 'Api');
+
+    const dn = new DomainName(stack, 'DomainName', {
+      domainName,
+      certificate: Certificate.fromCertificateArn(stack, 'cert', certArn),
+    });
+
+    expect(() => {
+      new HttpApiMapping(stack, 'Mapping', {
+        api,
+        domainName: dn,
+        apiMappingKey: 'foo/bar',
+      });
+    }).toThrow(/An ApiMapping key may contain only letters, numbers and one of/);
+  });
+
+  test('apiMappingKey validation - trailing slash not allowed', () => {
+
+    const stack = new Stack();
+    const api = new HttpApi(stack, 'Api');
+
+    const dn = new DomainName(stack, 'DomainName', {
+      domainName,
+      certificate: Certificate.fromCertificateArn(stack, 'cert', certArn),
+    });
+
+    expect(() => {
+      new HttpApiMapping(stack, 'Mapping', {
+        api,
+        domainName: dn,
+        apiMappingKey: 'foo/',
+      });
+    }).toThrow(/An ApiMapping key may contain only letters, numbers and one of/);
+  });
+
+  test('apiMappingKey validation - special character in the prefix not allowed', () => {
+
+    const stack = new Stack();
+    const api = new HttpApi(stack, 'Api');
+
+    const dn = new DomainName(stack, 'DomainName', {
+      domainName,
+      certificate: Certificate.fromCertificateArn(stack, 'cert', certArn),
+    });
+
+    expect(() => {
+      new HttpApiMapping(stack, 'Mapping', {
+        api,
+        domainName: dn,
+        apiMappingKey: '^foo',
+      });
+    }).toThrow(/An ApiMapping key may contain only letters, numbers and one of/);
+  });
+
+  test('apiMappingKey validation - multiple special character not allowed', () => {
+
+    const stack = new Stack();
+    const api = new HttpApi(stack, 'Api');
+
+    const dn = new DomainName(stack, 'DomainName', {
+      domainName,
+      certificate: Certificate.fromCertificateArn(stack, 'cert', certArn),
+    });
+
+    expect(() => {
+      new HttpApiMapping(stack, 'Mapping', {
+        api,
+        domainName: dn,
+        apiMappingKey: 'foo.*$',
+      });
+    }).toThrow(/An ApiMapping key may contain only letters, numbers and one of/);
+  });
+
   test('import mapping', () => {
 
     const stack = new Stack();
@@ -77,7 +210,6 @@ describe('ApiMapping', () => {
     const mapping = new HttpApiMapping(stack, 'Mapping', {
       api,
       domainName: dn,
-      apiMappingKey: '/',
     });
 
     const imported = HttpApiMapping.fromHttpApiMappingAttributes(stack, 'ImportedMapping', {

diff --git a/packages/@aws-cdk/aws-apigatewayv2/test/http/domain-name.test.ts b/packages/@aws-cdk/aws-apigatewayv2/test/http/domain-name.test.ts
@@ -106,7 +106,6 @@ describe('DomainName', () => {
       createDefaultStage: true,
       defaultDomainMapping: {
         domainName: dn,
-        mappingKey: '/',
       },
     });
 
@@ -142,7 +141,6 @@ describe('DomainName', () => {
         createDefaultStage: false,
         defaultDomainMapping: {
           domainName: dn,
-          mappingKey: '/',
         },
       });
     };