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) Cors Support #1242

Merged
merged 39 commits into from
Aug 12, 2019
Merged

(feat) Cors Support #1242

merged 39 commits into from
Aug 12, 2019

Conversation

viksrivat
Copy link
Contributor

Issue #, if available:

Description of changes:
Adds Cors support to ApiGateway. This completes the #1009 pr

Checklist:

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

samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/local/apigw/local_apigw_service.py Outdated Show resolved Hide resolved
tests/integration/local/start_api/test_start_api.py Outdated Show resolved Hide resolved
@jfuss
Copy link
Contributor

jfuss commented Jul 15, 2019

@viksrivat Can you handle the merge conflicts here? I will do a quick pass over once you do that.

viksrivat and others added 3 commits July 16, 2019 09:21
ApiGateway::RestApi only supports within methods section. This requires
parsing the requestParameters section of the ApiGateway resource. Since
we currently don'd do this, the best functionality is to ignore cors in
RestApi
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/commands/local/lib/sam_api_provider.py Show resolved Hide resolved
samcli/local/apigw/local_apigw_service.py Outdated Show resolved Hide resolved
samcli/local/apigw/local_apigw_service.py Outdated Show resolved Hide resolved
samcli/local/apigw/local_apigw_service.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jfuss jfuss left a comment

Choose a reason for hiding this comment

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

Some of this will need to be changed based on other prs you have in process. Once the stage pr is merged, you should rebase here and handle the comments. Overall, this looks good and would love to get this into a release soon, as this will be a great addition for customers.

samcli/commands/local/lib/sam_api_provider.py Outdated Show resolved Hide resolved
samcli/local/apigw/local_apigw_service.py Outdated Show resolved Hide resolved
samcli/local/apigw/local_apigw_service.py Outdated Show resolved Hide resolved
samcli/local/apigw/local_apigw_service.py Outdated Show resolved Hide resolved
@viksrivat viksrivat changed the base branch from develop to start-api/cfn July 26, 2019 18:03
samcli/commands/local/lib/provider.py Outdated Show resolved Hide resolved
if cors_prop and isinstance(cors_prop, dict):
allow_methods = cors_prop.get("AllowMethods", ','.join(Route.ANY_HTTP_METHODS))

if allow_methods and "OPTIONS" not in allow_methods and "options" not in allow_methods:
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to normalize allow_methods here, just comparing OPTIONS and options is not enough, unless SAM or API gateway only allow these two ways for the strings.


try:
event = self._construct_event(request, self.port, self.api.binary_media_types, self.api.stage_name,
self.api.stage_variables)
self.api.stage_variables, cors_headers)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right? Cors_headers always get added to the event?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I believe CORS needs to be added to every request.

samcli/local/apigw/local_apigw_service.py Outdated Show resolved Hide resolved
@sriram-mv
Copy link
Contributor

Is this branch ready to merge to candidate 20 branch? I see there are new commits added after the approval.

@viksrivat
Copy link
Contributor Author

The only changes that I added were updating some of the integration tests for Travis CI to pass.

@jfuss jfuss added the stage/accepted Accepted and will be fixed label Aug 7, 2019
@jfuss jfuss changed the base branch from start-api/cfn to develop August 7, 2019 21:28
@jfuss
Copy link
Contributor

jfuss commented Aug 7, 2019

Updated this to track and merge into develop.

@viksrivat viksrivat changed the base branch from develop to start-api/cfn August 9, 2019 17:06
@viksrivat viksrivat changed the base branch from start-api/cfn to develop August 9, 2019 17:06
@jfuss
Copy link
Contributor

jfuss commented Aug 10, 2019

@viksrivat Can you fix the flake8 errors:
tests/unit/commands/local/lib/test_sam_api_provider.py:1065:9: E303 too many blank lines (2)

Running make pr locally and making sure it passes should cover unit and linting.

@jfuss jfuss merged commit ec19bfb into aws:develop Aug 12, 2019
viksrivat added a commit to viksrivat/aws-sam-cli that referenced this pull request Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stage/accepted Accepted and will be fixed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants