Skip to content

Commit

Permalink
chore(apigateway): modernize Integration.bind() (#8814)
Browse files Browse the repository at this point in the history 
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 - #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.


----

*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 committed Jul 2, 2020
1 parent 4fad9bc commit 8d41671
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 104 deletions.
57 changes: 45 additions & 12 deletions packages/@aws-cdk/aws-apigateway/lib/integration.ts
View file
Expand Up @@ -135,9 +135,33 @@ export interface IntegrationProps {
}

/**
* Result of binding an Integration to the Method
* Result of binding an Integration to a 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;

/**
* This value is included in computing the Deployment's fingerprint. When the fingerprint
* changes, a new deployment is triggered.
Expand All @@ -155,23 +179,32 @@ export interface IntegrationConfig {
* 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): IntegrationConfig | undefined {
return;
public bind(_method: Method): IntegrationConfig {
return {
options: this.props.options,
type: this.props.type,
uri: this.props.uri,
integrationHttpMethod: this.props.integrationHttpMethod,
};
}
}

Expand Down
5 changes: 3 additions & 2 deletions packages/@aws-cdk/aws-apigateway/lib/integrations/aws.ts
View file
Expand Up @@ -92,8 +92,9 @@ export class AwsIntegration extends Integration {
});
}

public bind(method: Method): IntegrationConfig | undefined {
public bind(method: Method): IntegrationConfig {
const bindResult = super.bind(method);
this.scope = method;
return;
return bindResult;
}
}
5 changes: 3 additions & 2 deletions packages/@aws-cdk/aws-apigateway/lib/integrations/lambda.ts
View file
Expand Up @@ -52,8 +52,8 @@ export class LambdaIntegration extends AwsIntegration {
this.enableTest = options.allowTestInvoke === undefined ? true : false;
}

public bind(method: Method): IntegrationConfig | undefined {
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, '.')}`;
Expand All @@ -79,6 +79,7 @@ export class LambdaIntegration extends AwsIntegration {
deploymentToken = JSON.stringify({ functionName: cfnFunction.functionName });
}
return {
...bindResult,
deploymentToken,
};
}
Expand Down
27 changes: 7 additions & 20 deletions packages/@aws-cdk/aws-apigateway/lib/method.ts
View file
@@ -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, IntegrationConfig } from './integration';
import { MockIntegration } from './integrations/mock';
import { MethodResponse } from './methodresponse';
import { IModel } from './model';
Expand Down Expand Up @@ -204,7 +204,7 @@ export class Method extends Resource {
authorizationType,
authorizerId,
requestParameters: options.requestParameters || defaultMethodOptions.requestParameters,
integration: this.renderIntegration(integration),
integration: this.renderIntegration(bindResult),
methodResponses: this.renderMethodResponses(options.methodResponses),
requestModels: this.renderRequestModels(options.requestModels),
requestValidatorId: this.requestValidatorId(options),
Expand Down Expand Up @@ -263,22 +263,9 @@ export class Method extends Resource {
return this.api.arnForExecuteApi(this.httpMethod, pathForArn(this.resource.path), 'test-invoke-stage');
}

private renderIntegration(integration: Integration): CfnMethod.IntegrationProperty {
const options = integration._props.options || { };

private renderIntegration(bindResult: IntegrationConfig): CfnMethod.IntegrationProperty {
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) {
Expand All @@ -288,12 +275,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,
Expand Down
58 changes: 58 additions & 0 deletions packages/@aws-cdk/aws-apigateway/test/test.integration.ts
View file
@@ -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();
},
};
68 changes: 0 additions & 68 deletions packages/@aws-cdk/aws-apigateway/test/test.method.ts
View file
@@ -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';
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit 8d41671

Please sign in to comment.