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

feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version #29856

Merged
merged 26 commits into from
Jul 2, 2024
Merged

feat(lambda): add grantInvokeLatestVersion to grant invoke only to latest function version #29856

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
4d90557
add grantInvokeV2
roger-zhangg Apr 16, 2024
a2ca2f2
add readme
roger-zhangg Apr 16, 2024
287ccc5
Merge branch 'main' into grant_invoke_v2
roger-zhangg Apr 16, 2024
1052cf1
Merge branch 'grant_invoke_v2' of github.com:roger-zhangg/aws-cdk int…
roger-zhangg Apr 16, 2024
aa9d4ca
add integ test
roger-zhangg Apr 16, 2024
fc612e3
change to grantinvoke
roger-zhangg Apr 18, 2024
3aa8b6f
Merge branch 'main' into grant_invoke_v2
roger-zhangg Apr 18, 2024
679e6a6
lint
roger-zhangg Apr 18, 2024
0857f65
typo
roger-zhangg Apr 18, 2024
34d1506
add fn
roger-zhangg Apr 18, 2024
a502cd0
test
roger-zhangg Apr 18, 2024
5e5e6c9
add grantInvokeLatestVersion
roger-zhangg May 3, 2024
c734032
add grantInvokeLatestVersion
roger-zhangg May 3, 2024
74130bb
Merge branch 'main' into grant_invoke_v2
roger-zhangg May 3, 2024
f4b873f
fix lint
roger-zhangg May 3, 2024
a7b2b36
fix integ
roger-zhangg May 3, 2024
cd2f33b
Merge branch 'main' into grant_invoke_v2
roger-zhangg May 6, 2024
2e60174
revert to original snapshot
roger-zhangg May 6, 2024
36ee9a9
fix integ test
roger-zhangg May 6, 2024
6b4a5eb
Merge branch 'main' into grant_invoke_v2
roger-zhangg May 29, 2024
aa3cd0e
Merge branch 'main' into grant_invoke_v2
roger-zhangg Jun 25, 2024
7501abd
address comments
roger-zhangg Jun 26, 2024
734aae6
add desc
roger-zhangg Jun 26, 2024
15e3e8e
Merge branch 'main' into grant_invoke_v2
roger-zhangg Jun 26, 2024
e332150
Merge branch 'main' into grant_invoke_v2
roger-zhangg Jul 1, 2024
f84e77c
Merge branch 'main' into grant_invoke_v2
mergify[bot] Jul 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ws-cdk-testing/framework-integ/test/aws-lambda/test/integ.permissions.js.snapshot/cdk.out
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion ...cdk-testing/framework-integ/test/aws-lambda/test/integ.permissions.js.snapshot/integ.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions ...k-integ/test/aws-lambda/test/integ.permissions.js.snapshot/lambda-permissions.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions ...integ/test/aws-lambda/test/integ.permissions.js.snapshot/lambda-permissions.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,34 @@
"PrincipalOrgID": "o-xxxxxxxxxx"
}
},
"MyLambdaInvokeiasez2Hq1vE2Fl1I4Yaq8UbNdwu1QsvuMXzIoTAF759EB93": {
"Type": "AWS::Lambda::Permission",
"Properties": {
"Action": "lambda:InvokeFunction",
"FunctionName": {
"Fn::GetAtt": [
"MyLambdaCCE802FB",
"Arn"
]
},
"Principal": "*",
"PrincipalOrgID": "o-yyyyyyyyyy2"
}
},
"MyLambdaInvokeeCd3Xf1YrYI9J9KZWaa7iTC3wv2MejAlHdcglgF5m4c0F884F73": {
"Type": "AWS::Lambda::Permission",
"Properties": {
"Action": "lambda:InvokeFunction",
"FunctionName": {
"Fn::GetAtt": [
"MyLambdaCCE802FB",
"Arn"
]
},
"Principal": "*",
"PrincipalOrgID": "o-xxxxxxxxxx2"
}
},
"MyLambdaFunctionUrlC2055677": {
"Type": "AWS::Lambda::Url",
"Properties": {
Expand Down
25 changes: 14 additions & 11 deletions ...-testing/framework-integ/test/aws-lambda/test/integ.permissions.js.snapshot/manifest.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

126 changes: 85 additions & 41 deletions ...-cdk-testing/framework-integ/test/aws-lambda/test/integ.permissions.js.snapshot/tree.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions packages/@aws-cdk-testing/framework-integ/test/aws-lambda/test/integ.permissions.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

The integration feels a bit lackluster here, we're only testing that the policy is synthetically correct, not that the grants work as expected

Adding integration assertions to check that the functions are invokable with the provided examples would be ideal, either with AwsApiCalls, or by adding an API Gateway to integrate the Lambdas and running HttpApiCalls

Copy link
Member Author

Choose a reason for hiding this comment

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

I do agree only testing synthetically is kind of wired, but doing AwsApiCalls seems to be testing against Lambda/IAM rather than testing CDK. So I just followed the previous test behavior

Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ fn.grantInvoke(new iam.AnyPrincipal().inOrganization('o-yyyyyyyyyy'));

fn.grantInvoke(new iam.OrganizationPrincipal('o-xxxxxxxxxx'));

fn.grantInvoke(new iam.AnyPrincipal().inOrganization('o-yyyyyyyyyy2'), { onlyGrantLatestVersion: true });
Copy link
Member

Choose a reason for hiding this comment

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

Can we able to assert that onlyGrantLatestVersion: true grants only to the specific latest version and other versions should get an error on invocation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @godwingrs22 I'm looking into this but could you give me a headup on how to write integ test on this? I think I need to assume to the role I created here. But I didn't find how can I assume role in integration test. I checked invokeFucntion here and invokefunctionprop here but nothing related to assume role is found.

Copy link
Member Author

@roger-zhangg roger-zhangg Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I had a inspiration on this from my teammate:

Setting up integration test

I can setup 2 lambda function - fn1, fn2. And a role in the integ-test.
fn1 should have multiple versions, and grants invoke lastest version to role
fn2 uses this role, and the handler of fn2 will invoke fn1 with the given version then return the result

running the integration test

In the integration test, we will only invoke fn2 by integ.assertions.invokeFunction(fn2, version). In this way we will know if the role attached to fn2 can invoke fn1 or not. That is:

When

fn1.grantInvokeLatestVersion(role)

Then
integ.assertions.invokeFunction(fn2, '$LATEST') should success
integ.assertions.invokeFunction(fn2, 1) should fail

When
fn1.grantInvokeVersion(role, 1)

Then
integ.assertions.invokeFunction(fn2, '$LATEST') should fail
integ.assertions.invokeFunction(fn2, 1) should success

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Yes this should work. However let me get a second review within my team and get back to you on this integ test handling.

Copy link
Member

Choose a reason for hiding this comment

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

@roger-zhangg , Seems the current integ test framework is designed for handling success cases. Hence we can write a success case and perform assertion if the right permission is set. This should be good enough to cover the integ test.


fn.grantInvoke(new iam.OrganizationPrincipal('o-xxxxxxxxxx2'), { onlyGrantLatestVersion: true });

const fnUrl = fn.addFunctionUrl();
const role = new iam.Role(stack, 'MyRole', {
assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'),
Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/aws-cloudfront/lib/experimental/edge-function.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,8 @@ export class EdgeFunction extends Resource implements lambda.IVersion {
public addToRolePolicy(statement: iam.PolicyStatement): void {
return this.lambda.addToRolePolicy(statement);
}
public grantInvoke(identity: iam.IGrantable): iam.Grant {
return this.lambda.grantInvoke(identity);
public grantInvoke(identity: iam.IGrantable, props?: lambda.GrantInvokeProps): iam.Grant {
return this.lambda.grantInvoke(identity, props);
Copy link
Member

Choose a reason for hiding this comment

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

nit: do we need to update the identity parameter to grantee as defined in the lambda grantInvoke method?

}
public grantInvokeUrl(identity: iam.IGrantable): iam.Grant {
return this.lambda.grantInvokeUrl(identity);
Expand Down
Loading
Loading