fix(integ-tests): apply correct IAM policy to waiterProvider #28424

sakurai-ryo · 2023-12-19T12:26:25Z

Description

The following issue describes a bug where the IAM Policy is not correctly set to the calling Lambda when using invokeFunction and waitForAssertions.

Normally, when the waitForAssertions method is invoked, the necessary Policy is granted to the waiterProvider using the adPolicyStatementFromSdkCall method.

aws-cdk/packages/@aws-cdk/integ-tests-alpha/lib/assertions/sdk.ts

Line 136 in 52a5579

    
           waiter.isCompleteProvider.addPolicyStatementFromSdkCall(this.service, this.api);

In the case of a Lambda function call, the API name and the Action name of the Policy are different (invoke => invokeFunction), so the addPolicyStatementFromSdkCall method cannot grant the correct Policy.
The LambdaInvokeFunction is doing the correct Policy assignment to deal with this in the constructor.

aws-cdk/packages/@aws-cdk/integ-tests-alpha/lib/assertions/sdk.ts

Line 247 in 52a5579

    
           this.provider.addPolicyStatementFromSdkCall('Lambda', 'invokeFunction', [stack.formatArn({

However, this is not done for the waiterProvider, resulting in an access denied error.
This PR has been modified so that the correct Policy is granted to waiterProvider.

fixes #27865

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

…fix-integ-tests-invoke

sakurai-ryo · 2023-12-19T12:32:22Z

packages/@aws-cdk/integ-tests-alpha/lib/assertions/sdk.ts

@@ -250,6 +250,19 @@ export class LambdaInvokeFunction extends AwsApiCall {
      arnFormat: ArnFormat.COLON_RESOURCE_NAME,
      resourceName: props.functionName,
    })]);
+
+    // If using `waitForAssertions`, do the same for `waiterProvider` as above.
+    Aspects.of(this).add({


The Resource-based policy for Lambda is created in the part shown in the link, but it is not created here because invoking Lambda from Lambda is possible if the appropriate Policy is granted to the execution role of the invoking Lambda.

aws-cdk/packages/@aws-cdk/integ-tests-alpha/lib/assertions/sdk.ts

Line 236 in 52a5579

new CfnResource(this, 'Invoke', {

If you have any concerns, I would really appreciate it if you could let me know.

I'm fine with this but -- can you document why you've used an Aspect so that others who stumble on your code will understand the decision please

kaizencc

Thanks @sakurai-ryo! Some really minor comments but this looks good!

kaizencc · 2023-12-19T20:01:27Z

packages/@aws-cdk/integ-tests-alpha/lib/assertions/sdk.ts

@@ -250,6 +250,19 @@ export class LambdaInvokeFunction extends AwsApiCall {
      arnFormat: ArnFormat.COLON_RESOURCE_NAME,
      resourceName: props.functionName,
    })]);
+
+    // If using `waitForAssertions`, do the same for `waiterProvider` as above.
+    Aspects.of(this).add({


I'm fine with this but -- can you document why you've used an Aspect so that others who stumble on your code will understand the decision please

kaizencc · 2023-12-19T20:03:30Z

...ges/@aws-cdk/integ-tests-alpha/test/assertions/providers/integ.invoke-function-assertions.ts

Lets document here that what we are trying to verify is that Lambda.InvokeFunction exists in the integ assertions stack, not in the actual stack that is getting "tested"

Pull request has been modified.

sakurai-ryo · 2023-12-20T11:20:37Z

@kaizencc
Thanks again for your review!
As for the comments on the integ-tests part, I'm sorry if I misunderstood what you were saying, but I've added docs to sdk.ts and the integ-test file.

kaizencc

Perfect, thanks @sakurai-ryo

mergify · 2023-12-20T16:55:25Z

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

aws-cdk-automation · 2023-12-20T17:21:50Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 1844e3c
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2023-12-20T17:22:16Z

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

### Description The following issue describes a bug where the IAM Policy is not correctly set to the calling Lambda when using `invokeFunction` and `waitForAssertions`. Normally, when the `waitForAssertions` method is invoked, the necessary Policy is granted to the `waiterProvider` using the `adPolicyStatementFromSdkCall` method. https://github.com/aws/aws-cdk/blob/52a5579aa52c88bb289a7a9677c35385763c8fff/packages/%40aws-cdk/integ-tests-alpha/lib/assertions/sdk.ts#L136 In the case of a Lambda function call, the API name and the Action name of the Policy are different (invoke => invokeFunction), so the `addPolicyStatementFromSdkCall` method cannot grant the correct Policy. The `LambdaInvokeFunction` is doing the correct Policy assignment to deal with this in the constructor. https://github.com/aws/aws-cdk/blob/52a5579aa52c88bb289a7a9677c35385763c8fff/packages/%40aws-cdk/integ-tests-alpha/lib/assertions/sdk.ts#L247 However, this is not done for the `waiterProvider`, resulting in an access denied error. This PR has been modified so that the correct Policy is granted to `waiterProvider`. fixes aws#27865 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

sakurai-ryo added 5 commits December 18, 2023 11:45

tmp

a429543

fix: add test

deaa5e1

Merge branch 'master' of https://github.com/sakurai-ryo/aws-cdk into …

4cb7d47

…fix-integ-tests-invoke

chore: update snapshot

e7393a4

refactor: method, tests

a14033b

aws-cdk-automation requested a review from a team December 19, 2023 12:26

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK labels Dec 19, 2023

sakurai-ryo commented Dec 19, 2023

View reviewed changes

fix: typo

df2b392

sakurai-ryo marked this pull request as ready for review December 19, 2023 13:10

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023

kaizencc previously requested changes Dec 19, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 19, 2023

chore: add docs

ddcdeab

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 20, 2023

kaizencc approved these changes Dec 20, 2023

View reviewed changes

Merge branch 'main' into fix-integ-tests-invoke

1844e3c

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 20, 2023

mergify bot merged commit c488035 into aws:main Dec 20, 2023
12 checks passed

github-actions bot mentioned this pull request Jan 1, 2024

Monthly PR metrics report - Dec 2023 #28544

Closed

oqian mentioned this pull request Jan 10, 2024

@aws-cdk/integ-tests-alpha: (Integ-test's does not have the proper role permission) #27865

Closed

pahud mentioned this pull request Jan 10, 2024

@aws-cdk/integ-tests-alpha: Integ-test's singleton Function still does not have the proper permission to invoke Lambda Function #28655

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(integ-tests): apply correct IAM policy to waiterProvider #28424

fix(integ-tests): apply correct IAM policy to waiterProvider #28424

sakurai-ryo commented Dec 19, 2023 •

edited

Loading

sakurai-ryo Dec 19, 2023 •

edited

Loading

kaizencc Dec 19, 2023

kaizencc left a comment

kaizencc Dec 19, 2023

kaizencc Dec 19, 2023

sakurai-ryo commented Dec 20, 2023

kaizencc left a comment

mergify bot commented Dec 20, 2023

aws-cdk-automation commented Dec 20, 2023

mergify bot commented Dec 20, 2023

fix(integ-tests): apply correct IAM policy to waiterProvider #28424

fix(integ-tests): apply correct IAM policy to waiterProvider #28424

Conversation

sakurai-ryo commented Dec 19, 2023 • edited Loading

Description

sakurai-ryo Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

kaizencc Dec 19, 2023

Choose a reason for hiding this comment

kaizencc left a comment

Choose a reason for hiding this comment

kaizencc Dec 19, 2023

Choose a reason for hiding this comment

kaizencc Dec 19, 2023

Choose a reason for hiding this comment

sakurai-ryo commented Dec 20, 2023

kaizencc left a comment

Choose a reason for hiding this comment

mergify bot commented Dec 20, 2023

aws-cdk-automation commented Dec 20, 2023

AWS CodeBuild CI Report

mergify bot commented Dec 20, 2023

sakurai-ryo commented Dec 19, 2023 •

edited

Loading

sakurai-ryo Dec 19, 2023 •

edited

Loading