Skip to content

Commit

Permalink
fix(apigatewayv2): integration class does not render an integration r…
Browse files Browse the repository at this point in the history
…esource (aws#17729)

Routes on APIGateway V2 can integrate with different backends. This is
achieved by creating the CFN resource `AWS::ApiGatewayV2::Integration`
that is then referenced in the resource for the Route.

Currently, the `IHttpRouteIntegration` (and `IWebSocketRouteIntegration`)
interface represents a unique backend that a route can integrate with,
using the CDK "bind" pattern.
An integration can be bound to any number of routes but should be
rendered into a single instance of `AWS::ApiGatewayV2::Integration`
resource.
To achieve this currently, the `HttpApi` (and `WebSocketApi`) class
holds a cache of all integrations defined against its routes.

This is the wrong level of caching and causes a number of problems.

1. We rely on the configuration of the `AWS::ApiGateway::Integration`
   resource to determine if one already exists.
   This means that two instances of `IHttpRouteIntegration` can result
   in rendering only one instance of `AWS::ApiGateway::Integration`
   resource.

   Users may want to intentionally generate multiple instances of
   `AWS::ApiGateway::Integration` classes with the same configuration.
   Taking away this power with CDK "magic" is just confusing.

2. Currently, we allow using the same instance of
   `IHttpRouteIntegration` (or `IWebSocketRouteIntegration`) to be bound
   to routes in different `HttpApi`. When bound to the route, the CDK
   renders an instance of `AWS::ApiGatewayV2::Integration` for each API.

   This is another "magic" that has the potential for user confusion and
   bugs.

The solution is to KeepItSimple™.

Remove the API level caching and simply cache at the level of each
integration. This ensures that each instance of `HttpRouteIntegration`
(previously `IHttpRouteIntegration`) renders to exactly one instance of
`AWS::ApiGatewayV2::Integration`.

Disallow using the same instance of `HttpRouteIntegration` across
different instances of `HttpApi`.

fixes aws#13213

BREAKING CHANGE: The interface `IHttpRouteIntegration` is replaced by
the abstract class `HttpRouteIntegration`.
* **apigatewayv2:** The interface `IWebSocketRouteIntegration` is now
  replaced by the abstract class `WebSocketRouteIntegration`.
* **apigatewayv2:** Previously, we allowed the usage of integration
  classes to be used with routes defined in multiple `HttpApi` instances
  (or `WebSocketApi` instances). This is now disallowed, and separate
  instances must be created for each instance of `HttpApi` or
  `WebSocketApi`.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
Niranjan Jayakar authored and TikiTDO committed Feb 21, 2022
1 parent 769a7f8 commit 64798af
Show file tree
Hide file tree
Showing 19 changed files with 172 additions and 155 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Template } from '@aws-cdk/assertions';
import { HttpApi, HttpIntegrationType, HttpRouteIntegrationBindOptions, IHttpRouteIntegration, PayloadFormatVersion } from '@aws-cdk/aws-apigatewayv2';
import { HttpApi, HttpIntegrationType, HttpRouteIntegrationBindOptions, HttpRouteIntegration, PayloadFormatVersion } from '@aws-cdk/aws-apigatewayv2';
import { Stack } from '@aws-cdk/core';
import { HttpJwtAuthorizer } from '../../lib';

Expand Down Expand Up @@ -59,7 +59,7 @@ describe('HttpJwtAuthorizer', () => {
});
});

class DummyRouteIntegration implements IHttpRouteIntegration {
class DummyRouteIntegration extends HttpRouteIntegration {
public bind(_: HttpRouteIntegrationBindOptions) {
return {
payloadFormatVersion: PayloadFormatVersion.VERSION_2_0,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Match, Template } from '@aws-cdk/assertions';
import { HttpApi, HttpIntegrationType, HttpRouteIntegrationBindOptions, IHttpRouteIntegration, PayloadFormatVersion } from '@aws-cdk/aws-apigatewayv2';
import { HttpApi, HttpIntegrationType, HttpRouteIntegrationBindOptions, HttpRouteIntegration, PayloadFormatVersion } from '@aws-cdk/aws-apigatewayv2';
import { Code, Function, Runtime } from '@aws-cdk/aws-lambda';
import { Duration, Stack } from '@aws-cdk/core';
import { HttpLambdaAuthorizer, HttpLambdaResponseType } from '../../lib';
Expand Down Expand Up @@ -170,7 +170,7 @@ describe('HttpLambdaAuthorizer', () => {
});
});

class DummyRouteIntegration implements IHttpRouteIntegration {
class DummyRouteIntegration extends HttpRouteIntegration {
public bind(_: HttpRouteIntegrationBindOptions) {
return {
payloadFormatVersion: PayloadFormatVersion.VERSION_2_0,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Template } from '@aws-cdk/assertions';
import { HttpApi, HttpIntegrationType, HttpRouteIntegrationBindOptions, IHttpRouteIntegration, PayloadFormatVersion } from '@aws-cdk/aws-apigatewayv2';
import { HttpApi, HttpIntegrationType, HttpRouteIntegrationBindOptions, HttpRouteIntegration, PayloadFormatVersion } from '@aws-cdk/aws-apigatewayv2';
import { UserPool } from '@aws-cdk/aws-cognito';
import { Stack } from '@aws-cdk/core';
import { HttpUserPoolAuthorizer } from '../../lib';
Expand Down Expand Up @@ -112,7 +112,7 @@ describe('HttpUserPoolAuthorizer', () => {
});
});

class DummyRouteIntegration implements IHttpRouteIntegration {
class DummyRouteIntegration extends HttpRouteIntegration {
public bind(_: HttpRouteIntegrationBindOptions) {
return {
payloadFormatVersion: PayloadFormatVersion.VERSION_2_0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
HttpRouteIntegrationBindOptions,
HttpRouteIntegrationConfig,
HttpMethod,
IHttpRouteIntegration,
HttpRouteIntegration,
ParameterMapping,
PayloadFormatVersion,
} from '@aws-cdk/aws-apigatewayv2';
Expand Down Expand Up @@ -34,8 +34,9 @@ export interface HttpProxyIntegrationProps {
/**
* The HTTP Proxy integration resource for HTTP API
*/
export class HttpProxyIntegration implements IHttpRouteIntegration {
export class HttpProxyIntegration extends HttpRouteIntegration {
constructor(private readonly props: HttpProxyIntegrationProps) {
super();
}

public bind(_: HttpRouteIntegrationBindOptions): HttpRouteIntegrationConfig {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {
HttpIntegrationType,
HttpRouteIntegrationBindOptions,
HttpRouteIntegrationConfig,
IHttpRouteIntegration,
HttpRouteIntegration,
PayloadFormatVersion,
ParameterMapping,
} from '@aws-cdk/aws-apigatewayv2';
Expand Down Expand Up @@ -37,9 +37,10 @@ export interface LambdaProxyIntegrationProps {
/**
* The Lambda Proxy integration resource for HTTP API
*/
export class LambdaProxyIntegration implements IHttpRouteIntegration {
export class LambdaProxyIntegration extends HttpRouteIntegration {

constructor(private readonly props: LambdaProxyIntegrationProps) {
super();
}

public bind(options: HttpRouteIntegrationBindOptions): HttpRouteIntegrationConfig {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
HttpIntegrationType,
HttpRouteIntegrationBindOptions,
HttpRouteIntegrationConfig,
IHttpRouteIntegration,
HttpRouteIntegration,
PayloadFormatVersion,
HttpMethod,
IVpcLink,
Expand Down Expand Up @@ -37,7 +37,7 @@ export interface VpcLinkConfigurationOptions {
*
* @internal
*/
export abstract class HttpPrivateIntegration implements IHttpRouteIntegration {
export abstract class HttpPrivateIntegration extends HttpRouteIntegration {
protected httpMethod = HttpMethod.ANY;
protected payloadFormatVersion = PayloadFormatVersion.VERSION_1_0; // 1.0 is required and is the only supported format
protected integrationType = HttpIntegrationType.HTTP_PROXY;
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {
IWebSocketRouteIntegration,
WebSocketRouteIntegration,
WebSocketIntegrationType,
WebSocketRouteIntegrationBindOptions,
WebSocketRouteIntegrationConfig,
Expand All @@ -21,8 +21,10 @@ export interface LambdaWebSocketIntegrationProps {
/**
* Lambda WebSocket Integration
*/
export class LambdaWebSocketIntegration implements IWebSocketRouteIntegration {
constructor(private props: LambdaWebSocketIntegrationProps) {}
export class LambdaWebSocketIntegration extends WebSocketRouteIntegration {
constructor(private props: LambdaWebSocketIntegrationProps) {
super();
}

bind(options: WebSocketRouteIntegrationBindOptions): WebSocketRouteIntegrationConfig {
const route = options.route;
Expand Down
5 changes: 0 additions & 5 deletions packages/@aws-cdk/aws-apigatewayv2/lib/common/base.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import * as cloudwatch from '@aws-cdk/aws-cloudwatch';
import { Resource } from '@aws-cdk/core';
import { IntegrationCache } from '../private/integration-cache';
import { IApi } from './api';
import { ApiMapping } from './api-mapping';
import { DomainMappingOptions, IStage } from './stage';
Expand All @@ -12,10 +11,6 @@ import { DomainMappingOptions, IStage } from './stage';
export abstract class ApiBase extends Resource implements IApi {
abstract readonly apiId: string;
abstract readonly apiEndpoint: string;
/**
* @internal
*/
protected _integrationCache: IntegrationCache = new IntegrationCache();

public metric(metricName: string, props?: cloudwatch.MetricOptions): cloudwatch.Metric {
return new cloudwatch.Metric({
Expand Down
35 changes: 2 additions & 33 deletions packages/@aws-cdk/aws-apigatewayv2/lib/http/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { IApi } from '../common/api';
import { ApiBase } from '../common/base';
import { DomainMappingOptions } from '../common/stage';
import { IHttpRouteAuthorizer } from './authorizer';
import { IHttpRouteIntegration, HttpIntegration, HttpRouteIntegrationConfig } from './integration';
import { HttpRouteIntegration } from './integration';
import { BatchHttpRouteOptions, HttpMethod, HttpRoute, HttpRouteKey } from './route';
import { IHttpStage, HttpStage, HttpStageOptions } from './stage';
import { VpcLink, VpcLinkProps } from './vpc-link';
Expand Down Expand Up @@ -71,12 +71,6 @@ export interface IHttpApi extends IApi {
* Add a new VpcLink
*/
addVpcLink(options: VpcLinkProps): VpcLink

/**
* Add a http integration
* @internal
*/
_addIntegration(scope: Construct, config: HttpRouteIntegrationConfig): HttpIntegration;
}

/**
Expand All @@ -99,7 +93,7 @@ export interface HttpApiProps {
* An integration that will be configured on the catch-all route ($default).
* @default - none
*/
readonly defaultIntegration?: IHttpRouteIntegration;
readonly defaultIntegration?: HttpRouteIntegration;

/**
* Whether a default stage and deployment should be automatically created.
Expand Down Expand Up @@ -286,31 +280,6 @@ abstract class HttpApiBase extends ApiBase implements IHttpApi { // note that th

return vpcLink;
}

/**
* @internal
*/
public _addIntegration(scope: Construct, config: HttpRouteIntegrationConfig): HttpIntegration {
const { configHash, integration: existingIntegration } = this._integrationCache.getIntegration(scope, config);
if (existingIntegration) {
return existingIntegration as HttpIntegration;
}

const integration = new HttpIntegration(scope, `HttpIntegration-${configHash}`, {
httpApi: this,
integrationType: config.type,
integrationUri: config.uri,
method: config.method,
connectionId: config.connectionId,
connectionType: config.connectionType,
payloadFormatVersion: config.payloadFormatVersion,
secureServerName: config.secureServerName,
parameterMapping: config.parameterMapping,
});
this._integrationCache.saveIntegration(scope, config, integration);

return integration;
}
}

/**
Expand Down
42 changes: 39 additions & 3 deletions packages/@aws-cdk/aws-apigatewayv2/lib/http/integration.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
/* eslint-disable quotes */
import { Resource } from '@aws-cdk/core';
import * as crypto from 'crypto';
import { Resource, Stack } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { CfnIntegration } from '../apigatewayv2.generated';
import { IIntegration } from '../common';
Expand Down Expand Up @@ -191,11 +192,46 @@ export interface HttpRouteIntegrationBindOptions {
/**
* The interface that various route integration classes will inherit.
*/
export interface IHttpRouteIntegration {
export abstract class HttpRouteIntegration {
private integration?: HttpIntegration;

/**
* Internal method called when binding this integration to the route.
* @internal
*/
public _bindToRoute(options: HttpRouteIntegrationBindOptions): { readonly integrationId: string } {
if (this.integration && this.integration.httpApi.node.addr !== options.route.httpApi.node.addr) {
throw new Error('A single integration cannot be associated with multiple APIs.');
}

if (!this.integration) {
const config = this.bind(options);

this.integration = new HttpIntegration(options.scope, `HttpIntegration-${hash(config)}`, {
httpApi: options.route.httpApi,
integrationType: config.type,
integrationUri: config.uri,
method: config.method,
connectionId: config.connectionId,
connectionType: config.connectionType,
payloadFormatVersion: config.payloadFormatVersion,
secureServerName: config.secureServerName,
parameterMapping: config.parameterMapping,
});

function hash(x: any) {
const stringifiedConfig = JSON.stringify(Stack.of(options.scope).resolve(x));
const configHash = crypto.createHash('md5').update(stringifiedConfig).digest('hex');
return configHash;
}
}
return { integrationId: this.integration.integrationId };
}

/**
* Bind this integration to the route.
*/
bind(options: HttpRouteIntegrationBindOptions): HttpRouteIntegrationConfig;
public abstract bind(options: HttpRouteIntegrationBindOptions): HttpRouteIntegrationConfig;
}

/**
Expand Down
10 changes: 4 additions & 6 deletions packages/@aws-cdk/aws-apigatewayv2/lib/http/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { CfnRoute, CfnRouteProps } from '../apigatewayv2.generated';
import { IRoute } from '../common';
import { IHttpApi } from './api';
import { IHttpRouteAuthorizer } from './authorizer';
import { IHttpRouteIntegration } from './integration';
import { HttpRouteIntegration } from './integration';

/**
* Represents a Route for an HTTP API.
Expand Down Expand Up @@ -88,7 +88,7 @@ export interface BatchHttpRouteOptions {
/**
* The integration to be configured on this route.
*/
readonly integration: IHttpRouteIntegration;
readonly integration: HttpRouteIntegration;
}

/**
Expand Down Expand Up @@ -149,13 +149,11 @@ export class HttpRoute extends Resource implements IHttpRoute {
this.httpApi = props.httpApi;
this.path = props.routeKey.path;

const config = props.integration.bind({
const config = props.integration._bindToRoute({
route: this,
scope: this,
});

const integration = props.httpApi._addIntegration(this, config);

const authBindResult = props.authorizer ? props.authorizer.bind({
route: this,
scope: this.httpApi instanceof Construct ? this.httpApi : this, // scope under the API if it's not imported
Expand All @@ -181,7 +179,7 @@ export class HttpRoute extends Resource implements IHttpRoute {
const routeProps: CfnRouteProps = {
apiId: props.httpApi.apiId,
routeKey: props.routeKey.key,
target: `integrations/${integration.integrationId}`,
target: `integrations/${config.integrationId}`,
authorizerId: authBindResult?.authorizerId,
authorizationType: authBindResult?.authorizationType ?? 'NONE', // must be explicitly NONE (not undefined) for stack updates to work correctly
authorizationScopes,
Expand Down

This file was deleted.

25 changes: 0 additions & 25 deletions packages/@aws-cdk/aws-apigatewayv2/lib/websocket/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,12 @@ import { Construct } from 'constructs';
import { CfnApi } from '../apigatewayv2.generated';
import { IApi } from '../common/api';
import { ApiBase } from '../common/base';
import { WebSocketRouteIntegrationConfig, WebSocketIntegration } from './integration';
import { WebSocketRoute, WebSocketRouteOptions } from './route';

/**
* Represents a WebSocket API
*/
export interface IWebSocketApi extends IApi {
/**
* Add a websocket integration
* @internal
*/
_addIntegration(scope: Construct, config: WebSocketRouteIntegrationConfig): WebSocketIntegration
}

/**
Expand Down Expand Up @@ -100,25 +94,6 @@ export class WebSocketApi extends ApiBase implements IWebSocketApi {
}
}

/**
* @internal
*/
public _addIntegration(scope: Construct, config: WebSocketRouteIntegrationConfig): WebSocketIntegration {
const { configHash, integration: existingIntegration } = this._integrationCache.getIntegration(scope, config);
if (existingIntegration) {
return existingIntegration as WebSocketIntegration;
}

const integration = new WebSocketIntegration(scope, `WebSocketIntegration-${configHash}`, {
webSocketApi: this,
integrationType: config.type,
integrationUri: config.uri,
});
this._integrationCache.saveIntegration(scope, config, integration);

return integration;
}

/**
* Add a new route
*/
Expand Down
Loading

0 comments on commit 64798af

Please sign in to comment.