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.

Sign up for GitHub

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

Jump to bottom

fix(lambda): unable to add permissions to imported lambda functions #8828

Merged
merged 31 commits into from
Sep 21, 2020
Merged

fix(lambda): unable to add permissions to imported lambda functions #8828

merged 31 commits into from
Sep 21, 2020

Conversation

BryanPan342
Copy link
Contributor

@BryanPan342 BryanPan342 commented Jul 1, 2020 •
edited
Loading

[ISSUE]
Imported Lambda functions unable to add new resource policy

[APPROACH]
Add a check for imported Lambda Functions between the account id and the account id from imported Lambda Function. If they match, imported function can add permissions.

Fixes #7588

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

@BryanPan342 BryanPan342 marked this pull request as draft July 1, 2020 06:22
@BryanPan342 BryanPan342 added the @aws-cdk/aws-lambda Related to AWS Lambda label Jul 1, 2020
@BryanPan342
Copy link
Contributor Author

BryanPan342 commented Jul 1, 2020 •
edited
Loading

[ACTIONABLES]

  • Double check logic
  • Write unit tests
  • Update documentation

@BryanPan342
Copy link
Contributor Author

Right now my logic is to just make everything that is imported to be canAddPermissions to true. There is a conflict with the documentation as it stands with the following

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function-base.ts

Lines 182 to 187 in 9ee61eb

/**
* Whether the addPermission() call adds any permissions
*
* True for new Lambdas, false for imported Lambdas (they might live in different accounts).
*/
protected abstract readonly canCreatePermissions: boolean;

I think it's a valid point that imported Lambdas could potentially be from different accounts thus an inability to create resource policies.

I suggest implementing something similar to the way route53 implements a fromLookup through a context provider.

aws-cdk/packages/@aws-cdk/aws-route53/lib/hosted-zone.ts

Lines 95 to 98 in 9ee61eb

/**
* Lookup a hosted zone in the current account/region based on query parameters.
*/
public static fromLookup(scope: Construct, id: string, query: HostedZoneProviderProps): IHostedZone {

This would require implementing another function and updating the README, but wouldn't interfere with the current implementation of lambda which so many users currently use.

@nija-at Open to suggestions to an alternative and will revisit fresh tomorrow morning.

@BryanPan342 BryanPan342 self-assigned this Jul 1, 2020
@BryanPan342 BryanPan342 requested review from nija-at and a team July 1, 2020 06:36
@BryanPan342 BryanPan342 removed their assignment Jul 1, 2020
@SomayaB SomayaB added the pr/work-in-progress This PR is a draft and needs further work. label Jul 1, 2020
@BryanPan342 BryanPan342 marked this pull request as ready for review July 29, 2020 20:28
@BryanPan342 BryanPan342 removed the pr/work-in-progress This PR is a draft and needs further work. label Jul 31, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 31, 2020
nija-at
nija-at previously requested changes Aug 5, 2020
View reviewed changes
packages/@aws-cdk/aws-lambda/lib/function-base.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/test/integ.function-import.ts Outdated Show resolved Hide resolved
@BryanPan342 BryanPan342 requested a review from nija-at September 5, 2020 00:28
@BryanPan342 BryanPan342 dismissed nija-at’s stale review September 8, 2020 21:42

addressed review

@nija-at nija-at changed the title fix(lambda): imported lambda functions unable to add permissions fix(lambda): unable to add permissions to imported lambda functions Sep 16, 2020
nija-at
nija-at suggested changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Logic looks good. Some code comments.

packages/@aws-cdk/aws-lambda/lib/function-base.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/test/test.lambda.ts Show resolved Hide resolved
packages/@aws-cdk/aws-lambda/lib/function-base.ts Outdated Show resolved Hide resolved
@BryanPan342 BryanPan342 requested a review from nija-at September 18, 2020 16:46
@mergify
Copy link
Contributor

mergify bot commented Sep 21, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: b809d51
  • 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 Sep 21, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 9bf8e13 into aws:master Sep 21, 2020
njlynch added a commit that referenced this pull request Oct 1, 2020
@njlynch
fix(lambda): grantInvoke on imported function fails
bcfa463 
PR #8828 added the ability to add permissions on imported functions, if the
account on the stack and imported function matched. However, when this doesn't
match, attempting to call `grantInvoke` on the imported function results in a
`findChild` error.

This change guards against calling `addPermission` when `canCreatePermissions`
is false by checking if the `CfnPermission` was actually created, and adjusting
the return from `addInvoke` appropriately.

fixes #10607
@njlynch njlynch mentioned this pull request Oct 1, 2020
Merged
njlynch added a commit that referenced this pull request Oct 7, 2020
@njlynch
fix(lambda): grantInvoke on imported function fails
800c9f1 
PR #8828 added the ability to add permissions on imported functions, if the
account on the stack and imported function matched. However, when this doesn't
match, attempting to call `grantInvoke` on the imported function results in a
`findChild` error.

This change guards against calling `addPermission` when `canCreatePermissions`
is false by checking if the `CfnPermission` was actually created, and adjusting
the return from `addInvoke` appropriately.

fixes #10607
mergify bot pushed a commit that referenced this pull request Oct 12, 2020
@njlynch
fix(lambda): grantInvoke on imported function fails (#10622)
99111f7 
PR #8828 added the ability to add permissions on imported functions, if the
account on the stack and imported function matched. However, when this doesn't
match, attempting to call `grantInvoke` on the imported function results in a
`findChild` error.

This change guards against calling `addPermission` when `canCreatePermissions`
is false by checking if the `CfnPermission` was actually created, and adjusting
the return from `addInvoke` appropriately.

fixes #10607


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nija-at nija-at nija-at approved these changes

Assignees

@nija-at nija-at

Labels
@aws-cdk/aws-lambda Related to AWS Lambda contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Permission resource is not created for imported lambda function
4 participants
@BryanPan342 @aws-cdk-automation @nija-at @SomayaB