fix(apigateway): unable to enable cors with a root proxy and LambdaRestApi #5249

lukehedger · 2019-11-28T20:20:19Z

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.

mergify · 2019-11-28T20:20:57Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-28T20:32:32Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at

Thanks for adding these tests as a mechanism of repro!

Unfortunately, this can't be merged into our code base as is.

Do you intend to update this PR with the fix for this issue, or was your intention to simply provide a repro?

lukehedger · 2019-12-13T14:34:48Z

@nija-at Understood. I have not looked into the solution but this PR could be updated to include a fix.

mergify · 2019-12-16T12:33:25Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:00:50Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:10Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

eladb · 2019-12-16T14:10:32Z

@nija-at I'll pick this up.

Pull request has been modified.

mergify · 2019-12-16T14:21:59Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

aws-cdk-automation · 2019-12-16T14:22:51Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2019-12-16T14:38:50Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:38:51Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

mergify · 2019-12-16T14:52:23Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:24Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

mergify · 2019-12-16T15:17:30Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:17:31Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

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.

aws-cdk-automation · 2019-12-16T18:41:12Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-12-16T18:43:28Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Add tests for enabling CORS on API Gateway LambdaRestApi

695f3c9

lukehedger requested review from eladb and nija-at as code owners November 28, 2019 20:20

SomayaB assigned nija-at Nov 29, 2019

nija-at previously requested changes Dec 13, 2019

View reviewed changes

Merge branch 'master' into lambda-rest-api-cors

bb00931

eladb changed the title ~~Add tests for enabling CORS on API Gateway LambdaRestApi~~ fix(apigateway): unable to use cors with a root proxy and LambdaRestApi Dec 16, 2019

eladb changed the title ~~fix(apigateway): unable to use cors with a root proxy and LambdaRestApi~~ fix(apigateway): unable to enable cors with a root proxy and LambdaRestAPI Dec 16, 2019

eladb changed the title ~~fix(apigateway): unable to enable cors with a root proxy and LambdaRestAPI~~ fix(apigateway): unable to enable cors with a root proxy and LambdaRestApi Dec 16, 2019

eladb requested a review from nija-at December 16, 2019 18:18

Merge branch 'master' into lambda-rest-api-cors

7c3e798

nija-at approved these changes Dec 16, 2019

View reviewed changes

eladb merged commit f3d5fc9 into aws:master Dec 16, 2019

lukehedger deleted the lambda-rest-api-cors branch January 2, 2020 09:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(apigateway): unable to enable cors with a root proxy and LambdaRestApi #5249

fix(apigateway): unable to enable cors with a root proxy and LambdaRestApi #5249

lukehedger commented Nov 28, 2019 •

edited by eladb

Loading

mergify bot commented Nov 28, 2019

aws-cdk-automation commented Nov 28, 2019

nija-at left a comment

lukehedger commented Dec 13, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

eladb commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 16, 2019

aws-cdk-automation commented Dec 16, 2019

fix(apigateway): unable to enable cors with a root proxy and LambdaRestApi #5249

fix(apigateway): unable to enable cors with a root proxy and LambdaRestApi #5249

Conversation

lukehedger commented Nov 28, 2019 • edited by eladb Loading

mergify bot commented Nov 28, 2019

aws-cdk-automation commented Nov 28, 2019

AWS CodeBuild CI Report

nija-at left a comment

Choose a reason for hiding this comment

lukehedger commented Dec 13, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

eladb commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 16, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 16, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 16, 2019

AWS CodeBuild CI Report

lukehedger commented Nov 28, 2019 •

edited by eladb

Loading