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

updated the lambda permission with the correct case #1319

Merged
merged 6 commits into from
Dec 26, 2019
Merged

updated the lambda permission with the correct case #1319

merged 6 commits into from
Dec 26, 2019

Conversation

tomellis91
Copy link
Contributor

Issue #, if available:
The default policy action that get's added to the function permissions is in the incorrect case. It is being inserted as lambda:invokeFunction instead of lambda:InvokeFunction (note the i).
This means the permissions don't work as expected.

The correct case can be seen in the documentation here:
https://docs.aws.amazon.com/lambda/latest/dg/lambda-api-permissions-ref.html

When triggering the function via API gateway as a GET request the response returns as:

Unable to determine service/operation name to be authorized

This occurred when creating a CFN resource with Type: AWS::Serverless::Function

Description of changes:
I have updated the lambda action so that it is in the correct case: lambda:InvokeFunction

When creating the lambda the default policy is being added as:
"Principal": {
"Service": "apigateway.amazonaws.com"
},
"Action": "lambda:invokeFunction",

I have not updated the tests as of yet as there are a large number of files with the lowercase character. If you are happy with the change, please add a comment and I can go and update the remaining files before the change is merged.

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

@ShreyaGangishetty ShreyaGangishetty changed the base branch from master to develop December 13, 2019 18:45
@ShreyaGangishetty ShreyaGangishetty self-assigned this Dec 13, 2019
@tomellis91
Copy link
Contributor Author

Hi @ShreyaGangishetty ,
I have updated the rest of the tests to include the correct case. There are a few conflicts that need to be resolved, however I don't have the ability to update them myself.

Is there anything else that you need from my end? I will be happy to help where possible.

Cheers,
Tom

@ShreyaGangishetty
Copy link

@tde908 I will check and resolve these merge conflicts. Thank you for submitting this PR

@codecov-io
Copy link

codecov-io commented Dec 24, 2019

Codecov Report

Merging #1319 into develop will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1319   +/-   ##
========================================
  Coverage    94.48%   94.48%           
========================================
  Files           78       78           
  Lines         4388     4388           
  Branches       871      871           
========================================
  Hits          4146     4146           
  Misses         116      116           
  Partials       126      126
Impacted Files Coverage Δ
samtranslator/model/eventsources/push.py 90.53% <100%> (ø) ⬆️
samtranslator/model/api/api_generator.py 95.13% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a29a01...edcf817. Read the comment docs.

@ShreyaGangishetty ShreyaGangishetty changed the base branch from develop to master December 24, 2019 22:56
@ShreyaGangishetty ShreyaGangishetty changed the base branch from master to develop December 24, 2019 22:56
Copy link

@ShreyaGangishetty ShreyaGangishetty left a comment

Choose a reason for hiding this comment

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

@tde908 I have resolved the merge conflicts and rebased. This PR looks good to me.

@keetonian keetonian merged commit a93bbff into aws:develop Dec 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants