Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(apigateway): cors preflight support #4211

Merged
merged 23 commits into from
Oct 17, 2019
Merged

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Sep 23, 2019

Work in progress: adds support for CORS preflight (OPTIONS) requests on resources.
currently supports: origin, methods, headers, credentials, status code

Fixes #906

Implemented during a live twitch stream on sep 24, 2019


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

adds support for CORS preflight (OPTIONS) requests on resources.

currently supports: origin, methods, headers, credentials, status code

implemented during a live twitch stream on sep 24, 2019
@mergify
Copy link
Contributor

mergify bot commented Sep 23, 2019

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@eladb eladb self-assigned this Sep 24, 2019
@eladb eladb changed the title feat(apigateway): cors support (WIP) feat(apigateway): cors support Sep 25, 2019
@eladb eladb changed the title feat(apigateway): cors support feat(apigateway): cors preflight support Sep 25, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
@elhedran
Copy link
Contributor

Looks good and we could really, really use this. Anything I can do to help get the PR over the line?

@eladb
Copy link
Contributor Author

eladb commented Oct 16, 2019

@elhedran thanks for this nudge. I haven't had time to take this through the finish line. The main thing I'd like to add here (since I believe it might require an API change) is support recursive CORS configuration. Something like defaultCrossOriginOptions at the RestApi and Resource levels which will propagate to all child methods.

@eladb
Copy link
Contributor Author

eladb commented Oct 16, 2019

If you are interested to take a whack at it, it will be awesome! Otherwise, I hope I'll be able to find the time in the coming weeks.

@eladb eladb marked this pull request as ready for review October 17, 2019 07:55
@eladb eladb requested a review from nija-at as a code owner October 17, 2019 07:55
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

packages/@aws-cdk/aws-apigateway/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/README.md Show resolved Hide resolved
packages/@aws-cdk/aws-apigateway/lib/cors.ts Show resolved Hide resolved
* Access-Control-Allow-Methods and Access-Control-Allow-Headers headers)
* can be cached.
*
* To disable caching altogther use `disableCache: true`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't specifying 0 (or less) here be an effective way to signal "I don't want no caching"? I'm not too sure the extra flag is actually so useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec doesn't say anything about 0 and we can't express -1 as a Duration.

readonly exposeHeaders?: string[];
}

export class Cors {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please give this class a private constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Oct 17, 2019

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 0f06223 into master Oct 17, 2019
@mergify mergify bot deleted the benisrae/apigateway-cors branch October 17, 2019 09:42
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

apigateway: add explicit support for CORS
5 participants