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.

Sign up for GitHub

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

fix(apigatewayv2): integration class does not render an integration resource #17729

Merged
merged 3 commits into from
Nov 26, 2021
Merged

fix(apigatewayv2): integration class does not render an integration resource #17729

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea43862
fix(apigatewayv2): integration class does not render an integration r…
Nov 25, 2021
cca8290
fix build
Nov 26, 2021
f0288a6
Merge branch 'master' into nija-at/apigwv2-integrations
Nov 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-apigatewayv2-authorizers/test/http/jwt.test.ts
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-apigatewayv2-authorizers/test/http/lambda.test.ts
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-apigatewayv2-authorizers/test/http/user-pool.test.ts
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions packages/@aws-cdk/aws-apigatewayv2-integrations/lib/http/http-proxy.ts
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions packages/@aws-cdk/aws-apigatewayv2-integrations/lib/http/lambda.ts
View file
Open in desktop
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
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-apigatewayv2-integrations/lib/http/private/integration.ts
View file
Open in desktop
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
8 changes: 5 additions & 3 deletions packages/@aws-cdk/aws-apigatewayv2-integrations/lib/websocket/lambda.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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));
Copy link
Contributor

@otaviomacedo otaviomacedo Nov 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if it can happen with Stacks (maybe resolve() already takes care of that), but if the order of the fields change or if the presence or absence of whitespaces change, the hash will be different.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whitespace is not a problem here. Ordering maybe. I'll check.

Copy link
Contributor Author

@nija-at nija-at Nov 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ordering is actually a bigger problem here than I thought.

This PR is effectively not changing the logic of hashing. Just moving it to a different place (previously integration-cache.ts).

Do you mind if I take this on as a follow up PR? Easier to review as well.
I have a different solution in mind that effectively removes the need to hash.

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
View file
Open in desktop
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
29 changes: 0 additions & 29 deletions packages/@aws-cdk/aws-apigatewayv2/lib/private/integration-cache.ts
View file
Open in desktop

This file was deleted.

25 changes: 0 additions & 25 deletions packages/@aws-cdk/aws-apigatewayv2/lib/websocket/api.ts
View file
Open in desktop
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