Skip to content

Commit

Permalink
fix(apigateway): unable to enable cors with a root proxy and LambdaRe…
Browse files Browse the repository at this point in the history
…stApi (#5249)

If a proxy resource is added to the root, it will reflect any `addMethod` calls to the root resource, which causes two OPTIONS methods to be created when CORS is specified (one directly against the root resource and one by the proxy).

This uncovered a more general bug where if the proxy resource was added to the root, and the root already included a method of a certain kind, we will get a duplicate.

The fix is to avoid the proxy `addMethod` reflection on the root in case there is already a method by that type on the root.

This, indirectly, also fixes #5232, where CORS cannot be used with `LambdaRestApi`, which is basically a root proxy.
  • Loading branch information
lukehedger authored and Elad Ben-Israel committed Dec 16, 2019
1 parent f456b93 commit f3d5fc9
Show file tree
Hide file tree
Showing 4 changed files with 132 additions and 2 deletions.
5 changes: 4 additions & 1 deletion packages/@aws-cdk/aws-apigateway/lib/resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,10 @@ export class ProxyResource extends Resource {
// In case this proxy is mounted under the root, also add this method to
// the root so that empty paths are proxied as well.
if (this.parentResource && this.parentResource.path === '/') {
this.parentResource.addMethod(httpMethod, integration, options);
// skip if the root resource already has this method defined
if (!(this.parentResource.node.tryFindChild(httpMethod) instanceof Method)) {
this.parentResource.addMethod(httpMethod, integration, options);
}
}
return super.addMethod(httpMethod, integration, options);
}
Expand Down
59 changes: 58 additions & 1 deletion packages/@aws-cdk/aws-apigateway/test/test.cors.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { countResources, expect, haveResource } from '@aws-cdk/assert';
import lambda = require('@aws-cdk/aws-lambda');
import { Duration, Stack } from '@aws-cdk/core';
import { Test } from 'nodeunit';
import apigw = require('../lib');
Expand Down Expand Up @@ -645,6 +646,62 @@ export = {
}
}), /Invalid "allowOrigins" - cannot mix "\*" with specific origins: https:\/\/bla\.com,\*,https:\/\/specific/);

test.done();
},

'defaultCorsPreflightOptions can be used to specify CORS for all resource tree [LambdaRestApi]'(test: Test) {
// GIVEN
const stack = new Stack();

const handler = new lambda.Function(stack, 'handler', {
handler: 'index.handler',
code: lambda.Code.fromInline('boom'),
runtime: lambda.Runtime.NODEJS_10_X,
});

// WHEN
new apigw.LambdaRestApi(stack, 'lambda-rest-api', {
handler,
defaultCorsPreflightOptions: {
allowOrigins: ['https://amazon.com'],
}
});

// THEN
expect(stack).to(countResources('AWS::ApiGateway::Method', 4)); // two ANY and two OPTIONS resources
expect(stack).to(haveResource('AWS::ApiGateway::Method', {
HttpMethod: 'OPTIONS',
ResourceId: {
"Fn::GetAtt": [
"lambdarestapiAAD10924",
"RootResourceId"
]
},
}));
expect(stack).to(haveResource('AWS::ApiGateway::Method', {
HttpMethod: 'OPTIONS',
ResourceId: {
Ref: "lambdarestapiproxyE3AE07E3"
}
}));
test.done();
},

'CORS and proxy resources'(test: Test) {
// GIVEN
const stack = new Stack();

// WHEN
const api = new apigw.RestApi(stack, 'API', {
defaultCorsPreflightOptions: { allowOrigins: [ '*' ] }
});

api.root.addProxy();

// THEN
expect(stack).to(haveResource('AWS::ApiGateway::Method', {
HttpMethod: 'OPTIONS',
}));
test.done();
}
};
};
56 changes: 56 additions & 0 deletions packages/@aws-cdk/aws-apigateway/test/test.lambda-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,4 +199,60 @@ export = {

test.done();
},

'LambdaRestApi defines a REST API with CORS enabled'(test: Test) {
// GIVEN
const stack = new cdk.Stack();

const handler = new lambda.Function(stack, 'handler', {
handler: 'index.handler',
code: lambda.Code.fromInline('boom'),
runtime: lambda.Runtime.NODEJS_10_X,
});

// WHEN
new apigw.LambdaRestApi(stack, 'lambda-rest-api', {
handler,
defaultCorsPreflightOptions: {
allowOrigins: ['https://aws.amazon.com'],
allowMethods: ['GET', 'PUT']
}
});

// THEN
expect(stack).to(haveResource('AWS::ApiGateway::Method', {
HttpMethod: 'OPTIONS',
ResourceId: { Ref: 'lambdarestapiproxyE3AE07E3' },
Integration: {
IntegrationResponses: [
{
ResponseParameters: {
"method.response.header.Access-Control-Allow-Headers": "'Content-Type,X-Amz-Date,Authorization,X-Api-Key,X-Amz-Security-Token,X-Amz-User-Agent'",
"method.response.header.Access-Control-Allow-Origin": "'https://aws.amazon.com'",
"method.response.header.Vary": "'Origin'",
"method.response.header.Access-Control-Allow-Methods": "'GET,PUT'",
},
StatusCode: "204"
}
],
RequestTemplates: {
"application/json": "{ statusCode: 200 }"
},
Type: "MOCK"
},
MethodResponses: [
{
ResponseParameters: {
"method.response.header.Access-Control-Allow-Headers": true,
"method.response.header.Access-Control-Allow-Origin": true,
"method.response.header.Vary": true,
"method.response.header.Access-Control-Allow-Methods": true,
},
StatusCode: "204"
}
]
}));

test.done();
}
};
14 changes: 14 additions & 0 deletions packages/@aws-cdk/aws-apigateway/test/test.resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,20 @@ export = {
test.done();
},

'if proxy is added to root, proxy methods are only added if they are not defined already on the root resource'(test: Test) {
// GIVEN
const stack = new Stack();
const api = new apigw.RestApi(stack, 'api');
api.root.addMethod('POST');
const proxy = api.root.addProxy({ anyMethod: false });

// WHEN
proxy.addMethod('POST');

// THEN
test.done();
},

'url for a resource'(test: Test) {
// GIVEN
const stack = new Stack();
Expand Down

0 comments on commit f3d5fc9

Please sign in to comment.