Skip to content

Commit

Permalink
chore: remove an additional "prepare" from apigateway
Browse files Browse the repository at this point in the history 
Follow up on #9410 and remove an additional usage of `prepare` in the API Gateway library. In this case we leveraged `prepare()` to record dependencies between all Deployment resources and all Method resources in the APIGW.

The solution is to perform two-sided bookkeeping while methods/deployments are defined and record their dependencies in-band.

We also refactored the way the code in `LatestDeployment` to be slightly more readable.
  • Loading branch information
Elad Ben-Israel committed Aug 4, 2020
1 parent e3ae645 commit a715ae0
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 56 deletions.
88 changes: 34 additions & 54 deletions packages/@aws-cdk/aws-apigateway/lib/deployment.ts
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { CfnResource, Construct, Lazy, RemovalPolicy, Resource, Stack } from '@aws-cdk/core';
import * as crypto from 'crypto';
import { Construct, Lazy, RemovalPolicy, Resource } from '@aws-cdk/core';
import { CfnDeployment } from './apigateway.generated';
import { IRestApi, RestApi, SpecRestApi } from './restapi';
import { IRestApi, RestApi, SpecRestApi, RestApiBase } from './restapi';

export interface DeploymentProps {
/**
Expand Down Expand Up @@ -77,6 +77,10 @@ export class Deployment extends Resource {

this.api = props.api;
this.deploymentId = Lazy.stringValue({ produce: () => this.resource.ref });

if (props.api instanceof RestApiBase) {
props.api._attachDeployment(this);
}
}

/**
Expand All @@ -90,30 +94,6 @@ export class Deployment extends Resource {
public addToLogicalId(data: any) {
this.resource.addToLogicalId(data);
}

/**
* Hook into synthesis before it occurs and make any final adjustments.
*/
protected prepare() {
if (this.api instanceof RestApi) {
// Ignore IRestApi that are imported

/*
* https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-deployment.html
* Quoting from CloudFormation's docs - "If you create an AWS::ApiGateway::RestApi resource and its methods (using AWS::ApiGateway::Method) in
* the same template as your deployment, the deployment must depend on the RestApi's methods. To create a dependency, add a DependsOn attribute
* to the deployment. If you don't, AWS CloudFormation creates the deployment right after it creates the RestApi resource that doesn't contain
* any methods, and AWS CloudFormation encounters the following error: The REST API doesn't contain any methods."
*/

/*
* Adding a dependency between LatestDeployment and Method construct, using ConstructNode.addDependencies(), creates additional dependencies
* between AWS::ApiGateway::Deployment and the AWS::Lambda::Permission nodes (children under Method), causing cyclic dependency errors. Hence,
* falling back to declaring dependencies between the underlying CfnResources.
*/
this.api.methods.map(m => m.node.defaultChild as CfnResource).forEach(m => this.resource.addDependsOn(m));
}
}
}

interface LatestDeploymentResourceProps {
Expand All @@ -122,9 +102,9 @@ interface LatestDeploymentResourceProps {
}

class LatestDeploymentResource extends CfnDeployment {
private hashComponents = new Array<any>();

private api: IRestApi;
private readonly hashComponents = new Array<any>();
private readonly originalLogicalId: string;
private readonly api: IRestApi;

constructor(scope: Construct, id: string, props: LatestDeploymentResourceProps) {
super(scope, id, {
Expand All @@ -133,31 +113,8 @@ class LatestDeploymentResource extends CfnDeployment {
});

this.api = props.restApi;

const originalLogicalId = Stack.of(this).getLogicalId(this);

this.overrideLogicalId(Lazy.stringValue({ produce: ctx => {
const hash = [ ...this.hashComponents ];

if (this.api instanceof RestApi || this.api instanceof SpecRestApi) { // Ignore IRestApi that are imported

// Add CfnRestApi to the logical id so a new deployment is triggered when any of its properties change.
const cfnRestApiCF = (this.api.node.defaultChild as any)._toCloudFormation();
hash.push(ctx.resolve(cfnRestApiCF));
}

let lid = originalLogicalId;

// if hash components were added to the deployment, we use them to calculate
// a logical ID for the deployment resource.
if (hash.length > 0) {
const md5 = crypto.createHash('md5');
hash.map(x => ctx.resolve(x)).forEach(c => md5.update(JSON.stringify(c)));
lid += md5.digest('hex');
}

return lid;
}}));
this.originalLogicalId = this.stack.getLogicalId(this);
this.overrideLogicalId(Lazy.stringValue({ produce: () => this.calculateLogicalId() }));
}

/**
Expand All @@ -173,4 +130,27 @@ class LatestDeploymentResource extends CfnDeployment {

this.hashComponents.push(data);
}

private calculateLogicalId() {
const hash = [ ...this.hashComponents ];

if (this.api instanceof RestApi || this.api instanceof SpecRestApi) { // Ignore IRestApi that are imported

// Add CfnRestApi to the logical id so a new deployment is triggered when any of its properties change.
const cfnRestApiCF = (this.api.node.defaultChild as any)._toCloudFormation();
hash.push(this.stack.resolve(cfnRestApiCF));
}

let lid = this.originalLogicalId;

// if hash components were added to the deployment, we use them to calculate
// a logical ID for the deployment resource.
if (hash.length > 0) {
const md5 = crypto.createHash('md5');
hash.map(x => this.stack.resolve(x)).forEach(c => md5.update(JSON.stringify(c)));
lid += md5.digest('hex');
}

return lid;
}
}
40 changes: 38 additions & 2 deletions packages/@aws-cdk/aws-apigateway/lib/restapi.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { IVpcEndpoint } from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import { CfnOutput, Construct, IResource as IResourceBase, Resource, Stack } from '@aws-cdk/core';
import { CfnOutput, Construct, IResource as IResourceBase, Resource, Stack, CfnResource } from '@aws-cdk/core';
import { ApiDefinition } from './api-definition';
import { ApiKey, ApiKeyOptions, IApiKey } from './api-key';
import { CfnAccount, CfnRestApi } from './apigateway.generated';
Expand Down Expand Up @@ -248,7 +248,6 @@ export interface SpecRestApiProps extends RestApiBaseProps {
* Base implementation that are common to various implementations of IRestApi
*/
export abstract class RestApiBase extends Resource implements IRestApi {

/**
* Checks if the given object is an instance of RestApiBase.
* @internal
Expand Down Expand Up @@ -384,6 +383,15 @@ export abstract class RestApiBase extends Resource implements IRestApi {
ignore(method);
}

/**
* Associates a Deployment resource with this REST API.
*
* @internal
*/
public _attachDeployment(deployment: Deployment) {
ignore(deployment);
}

protected configureCloudWatchRole(apiResource: CfnRestApi) {
const role = new iam.Role(this, 'CloudWatchRole', {
assumedBy: new iam.ServicePrincipal('apigateway.amazonaws.com'),
Expand Down Expand Up @@ -550,6 +558,11 @@ export class RestApi extends RestApiBase {
*/
public readonly methods = new Array<Method>();

/**
* This list of deployments bound to this RestApi
*/
private readonly deployments = new Array<Deployment>();

constructor(scope: Construct, id: string, props: RestApiProps = { }) {
super(scope, id, props);

Expand Down Expand Up @@ -627,6 +640,29 @@ export class RestApi extends RestApiBase {
*/
public _attachMethod(method: Method) {
this.methods.push(method);

// add this method as a dependency to all deployments defined for this api
// when additional deployments are added, _attachDeployment is called and
// this method will be added there.
for (const dep of this.deployments) {
dep.node.addDependency(method.node.defaultChild as CfnResource);
}
}

/**
* Attaches a deployment to this REST API.
*
* @internal
*/
public _attachDeployment(deployment: Deployment) {
this.deployments.push(deployment);

// add all methods that were already defined as dependencies of this deployment
// when additional methods are added, _attachMethod is called and it will be
// added as a dependency to this deployment.
for (const method of this.methods) {
deployment.node.addDependency(method.node.defaultChild as CfnResource);
}
}

/**
Expand Down

0 comments on commit a715ae0

Please sign in to comment.