From d8fb0b3ef79470971c7da92ea53b51141379bdf7 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Tue, 30 Jun 2020 12:44:20 +0100 Subject: [PATCH 1/2] chore(apigateway): modernize Integration.bind() Currently, the Method construct reaches into the internals of the Integration class to construct itself, via the _props() internal method. Change to a more recent pattern where the bind() returns a result that contains all of the information that the Method class requires to finalize the bind. Motivation for the change This change - https://github.com/aws/aws-cdk/pull/8813 - requires a property returned to the Method that is not user specified. The change already introduces this pattern. This PR takes it a bit further and applies the same logic to existing properties, so that there is now only one way for the Method to get the result of the bind. --- .../aws-apigateway/lib/integration.ts | 60 +++++++++++++++---- .../aws-apigateway/lib/integrations/aws.ts | 6 +- .../aws-apigateway/lib/integrations/lambda.ts | 7 ++- .../@aws-cdk/aws-apigateway/lib/method.ts | 25 ++------ 4 files changed, 63 insertions(+), 35 deletions(-) diff --git a/packages/@aws-cdk/aws-apigateway/lib/integration.ts b/packages/@aws-cdk/aws-apigateway/lib/integration.ts index d7a9ec74f3b34..9324542eec631 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/integration.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/integration.ts @@ -134,6 +134,35 @@ export interface IntegrationProps { readonly options?: IntegrationOptions; } +/** + * Result of binding an Integration to the Method. + */ +export interface IntegrationConfig { + /** + * Integration options. + * @default - no integration options + */ + readonly options?: IntegrationOptions; + + /** + * Specifies an API method integration type. + */ + readonly type: IntegrationType; + + /** + * The Uniform Resource Identifier (URI) for the integration. + * @see https://docs.aws.amazon.com/apigateway/api-reference/resource/integration/#uri + * @default - no URI. Usually applies to MOCK integration + */ + readonly uri?: string; + + /** + * The integration's HTTP method type. + * @default - no integration method specified. + */ + readonly integrationHttpMethod?: string; +} + /** * Base class for backend integrations for an API Gateway method. * @@ -141,23 +170,32 @@ export interface IntegrationProps { * or implement on your own by specifying the set of props. */ export class Integration { - constructor(private readonly props: IntegrationProps) { } - - /** - * Allows `Method` to access the integration props. - * - * @internal - */ - public get _props() { - return this.props; + constructor(private readonly props: IntegrationProps) { + const options = this.props.options || { }; + if (options.credentialsPassthrough !== undefined && options.credentialsRole !== undefined) { + throw new Error('\'credentialsPassthrough\' and \'credentialsRole\' are mutually exclusive'); + } + + if (options.connectionType === ConnectionType.VPC_LINK && options.vpcLink === undefined) { + throw new Error('\'connectionType\' of VPC_LINK requires \'vpcLink\' prop to be set'); + } + + if (options.connectionType === ConnectionType.INTERNET && options.vpcLink !== undefined) { + throw new Error('cannot set \'vpcLink\' where \'connectionType\' is INTERNET'); + } } /** * Can be overridden by subclasses to allow the integration to interact with the method * being integrated, access the REST API object, method ARNs, etc. */ - public bind(_method: Method) { - return; + public bind(_method: Method): IntegrationConfig { + return { + options: this.props.options, + type: this.props.type, + uri: this.props.uri, + integrationHttpMethod: this.props.integrationHttpMethod, + }; } } diff --git a/packages/@aws-cdk/aws-apigateway/lib/integrations/aws.ts b/packages/@aws-cdk/aws-apigateway/lib/integrations/aws.ts index cd3854e2abd22..1f95b88233ab8 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/integrations/aws.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/integrations/aws.ts @@ -1,5 +1,5 @@ import * as cdk from '@aws-cdk/core'; -import { Integration, IntegrationOptions, IntegrationType } from '../integration'; +import { Integration, IntegrationConfig, IntegrationOptions, IntegrationType } from '../integration'; import { Method } from '../method'; import { parseAwsApiCall } from '../util'; @@ -92,7 +92,9 @@ export class AwsIntegration extends Integration { }); } - public bind(method: Method) { + public bind(method: Method): IntegrationConfig { + const bindResult = super.bind(method); this.scope = method; + return bindResult; } } diff --git a/packages/@aws-cdk/aws-apigateway/lib/integrations/lambda.ts b/packages/@aws-cdk/aws-apigateway/lib/integrations/lambda.ts index fe5e81678e85e..1025734b0ed60 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/integrations/lambda.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/integrations/lambda.ts @@ -1,7 +1,7 @@ import * as iam from '@aws-cdk/aws-iam'; import * as lambda from '@aws-cdk/aws-lambda'; import { Lazy } from '@aws-cdk/core'; -import { IntegrationOptions } from '../integration'; +import { IntegrationConfig, IntegrationOptions } from '../integration'; import { Method } from '../method'; import { AwsIntegration } from './aws'; @@ -52,8 +52,8 @@ export class LambdaIntegration extends AwsIntegration { this.enableTest = options.allowTestInvoke === undefined ? true : false; } - public bind(method: Method) { - super.bind(method); + public bind(method: Method): IntegrationConfig { + const bindResult = super.bind(method); const principal = new iam.ServicePrincipal('apigateway.amazonaws.com'); const desc = `${method.api.node.uniqueId}.${method.httpMethod}.${method.resource.path.replace(/\//g, '.')}`; @@ -72,5 +72,6 @@ export class LambdaIntegration extends AwsIntegration { sourceArn: method.testMethodArn, }); } + return bindResult; } } diff --git a/packages/@aws-cdk/aws-apigateway/lib/method.ts b/packages/@aws-cdk/aws-apigateway/lib/method.ts index 8721d9e9da114..a2d96572bb1f0 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/method.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/method.ts @@ -1,7 +1,7 @@ import { Construct, Resource, Stack } from '@aws-cdk/core'; import { CfnMethod, CfnMethodProps } from './apigateway.generated'; import { Authorizer, IAuthorizer } from './authorizer'; -import { ConnectionType, Integration } from './integration'; +import { Integration } from './integration'; import { MockIntegration } from './integrations/mock'; import { MethodResponse } from './methodresponse'; import { IModel } from './model'; @@ -266,23 +266,10 @@ export class Method extends Resource { return this.renderIntegration(new MockIntegration()); } - integration.bind(this); - - const options = integration._props.options || { }; + const bindResult = integration.bind(this); + const options = bindResult.options || { }; let credentials; - if (options.credentialsPassthrough !== undefined && options.credentialsRole !== undefined) { - throw new Error('\'credentialsPassthrough\' and \'credentialsRole\' are mutually exclusive'); - } - - if (options.connectionType === ConnectionType.VPC_LINK && options.vpcLink === undefined) { - throw new Error('\'connectionType\' of VPC_LINK requires \'vpcLink\' prop to be set'); - } - - if (options.connectionType === ConnectionType.INTERNET && options.vpcLink !== undefined) { - throw new Error('cannot set \'vpcLink\' where \'connectionType\' is INTERNET'); - } - if (options.credentialsRole) { credentials = options.credentialsRole.roleArn; } else if (options.credentialsPassthrough) { @@ -292,12 +279,12 @@ export class Method extends Resource { } return { - type: integration._props.type, - uri: integration._props.uri, + type: bindResult.type, + uri: bindResult.uri, cacheKeyParameters: options.cacheKeyParameters, cacheNamespace: options.cacheNamespace, contentHandling: options.contentHandling, - integrationHttpMethod: integration._props.integrationHttpMethod, + integrationHttpMethod: bindResult.integrationHttpMethod, requestParameters: options.requestParameters, requestTemplates: options.requestTemplates, passthroughBehavior: options.passthroughBehavior, From deb2e5645e126d42c30a2e22ccafe20347e5c85c Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Wed, 1 Jul 2020 18:59:01 +0100 Subject: [PATCH 2/2] fix breaking tests --- .../aws-apigateway/test/test.integration.ts | 58 ++++++++++++++++ .../aws-apigateway/test/test.method.ts | 68 ------------------- 2 files changed, 58 insertions(+), 68 deletions(-) create mode 100644 packages/@aws-cdk/aws-apigateway/test/test.integration.ts diff --git a/packages/@aws-cdk/aws-apigateway/test/test.integration.ts b/packages/@aws-cdk/aws-apigateway/test/test.integration.ts new file mode 100644 index 0000000000000..40047b53df745 --- /dev/null +++ b/packages/@aws-cdk/aws-apigateway/test/test.integration.ts @@ -0,0 +1,58 @@ +import * as ec2 from '@aws-cdk/aws-ec2'; +import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2'; +import * as iam from '@aws-cdk/aws-iam'; +import * as cdk from '@aws-cdk/core'; +import { Test } from 'nodeunit'; +import * as apigw from '../lib'; + +export = { + 'integration "credentialsRole" and "credentialsPassthrough" are mutually exclusive'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + const role = new iam.Role(stack, 'MyRole', { assumedBy: new iam.ServicePrincipal('foo') }); + + // THEN + test.throws(() => new apigw.Integration({ + type: apigw.IntegrationType.AWS_PROXY, + options: { + credentialsPassthrough: true, + credentialsRole: role, + }, + }), /'credentialsPassthrough' and 'credentialsRole' are mutually exclusive/); + test.done(); + }, + + 'integration connectionType VpcLink requires vpcLink to be set'(test: Test) { + test.throws(() => new apigw.Integration({ + type: apigw.IntegrationType.HTTP_PROXY, + integrationHttpMethod: 'ANY', + options: { + connectionType: apigw.ConnectionType.VPC_LINK, + }, + }), /'connectionType' of VPC_LINK requires 'vpcLink' prop to be set/); + test.done(); + }, + + 'connectionType of INTERNET and vpcLink are mutually exclusive'(test: Test) { + // GIVEN + const stack = new cdk.Stack(); + const vpc = new ec2.Vpc(stack, 'VPC'); + const nlb = new elbv2.NetworkLoadBalancer(stack, 'NLB', { + vpc, + }); + const link = new apigw.VpcLink(stack, 'link', { + targets: [nlb], + }); + + // THEN + test.throws(() => new apigw.Integration({ + type: apigw.IntegrationType.HTTP_PROXY, + integrationHttpMethod: 'ANY', + options: { + connectionType: apigw.ConnectionType.INTERNET, + vpcLink: link, + }, + }), /cannot set 'vpcLink' where 'connectionType' is INTERNET/); + test.done(); + }, +}; \ No newline at end of file diff --git a/packages/@aws-cdk/aws-apigateway/test/test.method.ts b/packages/@aws-cdk/aws-apigateway/test/test.method.ts index ba5ab8c1c2c91..85111b4636882 100644 --- a/packages/@aws-cdk/aws-apigateway/test/test.method.ts +++ b/packages/@aws-cdk/aws-apigateway/test/test.method.ts @@ -1,6 +1,4 @@ import { ABSENT, expect, haveResource, haveResourceLike } from '@aws-cdk/assert'; -import * as ec2 from '@aws-cdk/aws-ec2'; -import * as elbv2 from '@aws-cdk/aws-elasticloadbalancingv2'; import * as iam from '@aws-cdk/aws-iam'; import * as lambda from '@aws-cdk/aws-lambda'; import * as cdk from '@aws-cdk/core'; @@ -327,72 +325,6 @@ export = { test.done(); }, - 'integration "credentialsRole" and "credentialsPassthrough" are mutually exclusive'(test: Test) { - // GIVEN - const stack = new cdk.Stack(); - const api = new apigw.RestApi(stack, 'test-api', { deploy: false }); - const role = new iam.Role(stack, 'MyRole', { assumedBy: new iam.ServicePrincipal('foo') }); - - // WHEN - const integration = new apigw.Integration({ - type: apigw.IntegrationType.AWS_PROXY, - options: { - credentialsPassthrough: true, - credentialsRole: role, - }, - }); - - // THEN - test.throws(() => api.root.addMethod('GET', integration), /'credentialsPassthrough' and 'credentialsRole' are mutually exclusive/); - test.done(); - }, - - 'integration connectionType VpcLink requires vpcLink to be set'(test: Test) { - // GIVEN - const stack = new cdk.Stack(); - const api = new apigw.RestApi(stack, 'test-api', { deploy: false }); - - // WHEN - const integration = new apigw.Integration({ - type: apigw.IntegrationType.HTTP_PROXY, - integrationHttpMethod: 'ANY', - options: { - connectionType: apigw.ConnectionType.VPC_LINK, - }, - }); - - // THEN - test.throws(() => api.root.addMethod('GET', integration), /'connectionType' of VPC_LINK requires 'vpcLink' prop to be set/); - test.done(); - }, - - 'connectionType of INTERNET and vpcLink are mutually exclusive'(test: Test) { - // GIVEN - const stack = new cdk.Stack(); - const api = new apigw.RestApi(stack, 'test-api', { deploy: false }); - const vpc = new ec2.Vpc(stack, 'VPC'); - const nlb = new elbv2.NetworkLoadBalancer(stack, 'NLB', { - vpc, - }); - const link = new apigw.VpcLink(stack, 'link', { - targets: [nlb], - }); - - // WHEN - const integration = new apigw.Integration({ - type: apigw.IntegrationType.HTTP_PROXY, - integrationHttpMethod: 'ANY', - options: { - connectionType: apigw.ConnectionType.INTERNET, - vpcLink: link, - }, - }); - - // THEN - test.throws(() => api.root.addMethod('GET', integration), /cannot set 'vpcLink' where 'connectionType' is INTERNET/); - test.done(); - }, - 'methodResponse set one or more method responses via options'(test: Test) { // GIVEN const stack = new cdk.Stack();