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

chore(integ-tests): add waiterProvider to IApiCall #27844

Merged
merged 14 commits into from
Dec 21, 2023

Conversation

go-to-k
Copy link
Contributor

@go-to-k go-to-k commented Nov 5, 2023

This PR changes to add the waiterProvider property to an IApiCall for awsApiCall in integ-tests-alpha.

By default awsApiCall in integ tests, the AwsApiCall construct will automatically add the correct IAM policies to allow the Lambda function to make the API call. It does this based on the service and api that is provided. In the following example the service is SQS and the api is receiveMessage so it will create a policy with Action: 'sqs:ReceiveMessage'.

const integ = new IntegTest(app, 'Integ', {
  testCases: [stack],
});
integ.assertions.awsApiCall('SQS', 'receiveMessage', {
  QueueUrl: 'url',
});

There are some cases where the permissions do not exactly match the service/api call, for example the S3 listObjectsV2 api. In these cases it is possible to add the correct policy by accessing the provider object.

const apiCall = integ.assertions.awsApiCall('S3', 'listObjectsV2', {
  Bucket: 'mybucket',
});

apiCall.provider.addToRolePolicy({
  Effect: 'Allow',
  Action: ['s3:GetObject', 's3:ListBucket'],
  Resource: ['*'],
});

On the other hand, there is the case to use waitForAssertions when using awsApiCall in integ tests. This causes apiCall to have a waiterProvider property in addition to provider.

const apiCall = integ.assertions.awsApiCall('S3', 'listObjectsV2', {
  Bucket: 'mybucket',
}).expect(ExpectedResult.objectLike({
  KeyCount: 1,
})).waitForAssertions({
  interval: cdk.Duration.seconds(30),
  totalTimeout: cdk.Duration.minutes(10),
});

In the case, waiterProvider actually calls to the service/api, so it should have the proper policies.

However a type of a return value of apiCall is IApiCall interface so that the interface has a provider property, waiterProvider is not in IApiCall but in AwsApiCall.

Then it cannot take the policies without casting the following. (apiCall instanceof AwsApiCall)

if (apiCall instanceof AwsApiCall) {
  apiCall.waiterProvider?.addToRolePolicy({
    Effect: 'Allow',
    Action: ['s3:GetObject', 's3:ListBucket'],
    Resource: ['*'],
  });
}

So I add waiterProvider to IApiCall, so that it can take the policies without casting:

// if (apiCall instanceof AwsApiCall) {
  apiCall.waiterProvider?.addToRolePolicy({
    Effect: 'Allow',
    Action: ['s3:GetObject', 's3:ListBucket'],
    Resource: ['*'],
  });
//}

In my opinion, I see no negative impact from this.


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

@github-actions github-actions bot added the p2 label Nov 5, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team November 5, 2023 14:23
@github-actions github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Nov 5, 2023
@go-to-k go-to-k marked this pull request as ready for review November 5, 2023 14:34
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Nov 5, 2023
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

Thanks, just a minor documentation adjustment on the current implementation.

Also:

  • I think that an integration test should be added to verify that the policy is working as expected.
  • Any reason why waiterProvider shouldn't be an abstract property in ApiCallBase and used in HttpApiCall as well after this change?

Comment on lines 26 to 28
* access the AssertionsProvider for the waiter state machine.
* This can be used to add additional IAM policies
* the the provider role policy
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* access the AssertionsProvider for the waiter state machine.
* This can be used to add additional IAM policies
* the the provider role policy
* Access the AssertionsProvider for the waiter state machine.
* This can be used to add additional IAM policies
* to the provider role policy.

Can you please adjust the documentation for provider as well?

@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 7, 2023

I think that an integration test should be added to verify that the policy is working as expected.

Yes, I will add the test.

Any reason why waiterProvider shouldn't be an abstract property in ApiCallBase and used in HttpApiCall as well after this change?

I think there is probably no case to use waiterProvider with HttpApiCall. This is because public waiterProvider can be used primarily to add additional IAM policies to a provider's role policy, but HttpApiCall does not require additional IAM policies. So I don't think waiterProvider needs to be an abstract property in ApiCallBase.

@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 7, 2023

I think that an integration test should be added to verify that the policy is working as expected.

This integ-tests-alpha module does not have its own integ tests. So, do I just include the code that confirms this change in the integration test for the module of my choice somewhere?

Also, is unit testing alone not enough? If it is just a policy check, it seems to me that a unit test alone would suffice.

@lpizzinidev
Copy link
Contributor

@go-to-k
You can write an integration test in the test directory of the module (similar to what is done here).

We've integration tests for the provider.addToRolePolicy method, but not for the waiterProvider one, so I think it would be nice to have (given the fact that it will be only used in other integration tests).

Also, it should be documented here.

@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 8, 2023

@lpizzinidev

You can write an integration test in the test directory of the module (similar to what is done here).

Thanks for letting me know! I missed it, I'll just put it here: integ-tests-alpha/test/assertions/providers/lambda-handler/integ.waiter-provider.ts.

We've integration tests for the provider.addToRolePolicy method

I can't find this test in providers/integ.assertions.ts, which file is it?

Also, it should be documented here.

Yes, I will take it.

@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 8, 2023

@lpizzinidev

In the meantime, I've created an integ test, what do you think? (I'll change the README later.)

eaf0074

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

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

The integration test looks good (just a minor adjustment is needed in my opinion) 👍
I'll review the README documentation when ready.

I can't find this test in providers/integ.assertions.ts, which file is it?

provider.addToRolePolicy is already used in many integration tests across the project.

Comment on lines 16 to 31
integ.assertions.awsApiCall('S3', 'listObjectsV2', {
Bucket: bucket.bucketName,
MaxKeys: 1,
}).expect(ExpectedResult.objectLike({
KeyCount: 0,
})).waitForAssertions({
interval: Duration.seconds(10),
totalTimeout: Duration.minutes(3),
}).waiterProvider?.addToRolePolicy({
Effect: 'Allow',
Action: ['s3:GetObject', 's3:ListBucket'],
Resource: [
`${bucket.bucketArn}`,
`${bucket.bucketArn}/*`,
],
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
integ.assertions.awsApiCall('S3', 'listObjectsV2', {
Bucket: bucket.bucketName,
MaxKeys: 1,
}).expect(ExpectedResult.objectLike({
KeyCount: 0,
})).waitForAssertions({
interval: Duration.seconds(10),
totalTimeout: Duration.minutes(3),
}).waiterProvider?.addToRolePolicy({
Effect: 'Allow',
Action: ['s3:GetObject', 's3:ListBucket'],
Resource: [
`${bucket.bucketArn}`,
`${bucket.bucketArn}/*`,
],
});
const listObjectsCall = integ.assertions.awsApiCall('S3', 'listObjectsV2', {
Bucket: bucket.bucketName,
MaxKeys: 1,
}).expect(ExpectedResult.objectLike({
KeyCount: 0,
})).waitForAssertions({
interval: Duration.seconds(10),
totalTimeout: Duration.minutes(3),
});
listObjectsCall.waiterProvider?.addToRolePolicy({
Effect: 'Allow',
Action: ['s3:GetObject', 's3:ListBucket'],
Resource: [
`${bucket.bucketArn}`,
`${bucket.bucketArn}/*`,
],
});

To keep it cleaner.

@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 8, 2023

@lpizzinidev

Thanks for your review.

I changed them, and added the case to README. Please check them.

backoffRate: 3,
});

apiCall?.waiterProvider.addToRolePolicy({
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
apiCall?.waiterProvider.addToRolePolicy({
apiCall.waiterProvider?.addToRolePolicy({

In cases where the `waitForAssertions()` is used for the `awsApiCall`, the actual API call is executed
by the `waiterProvider`.

Same as `provider`, by default, the `AwsApiCall` construct will automatically add the correct IAM policies
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Same as `provider`, by default, the `AwsApiCall` construct will automatically add the correct IAM policies
By default, the `AwsApiCall` construct will automatically add the correct IAM policies

Comment on lines 525 to 526
In cases where the `waitForAssertions()` is used for the `awsApiCall`, the actual API call is executed
by the `waiterProvider`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
In cases where the `waitForAssertions()` is used for the `awsApiCall`, the actual API call is executed
by the `waiterProvider`.
When `waitForAssertions()` is used for the `awsApiCall`, the actual API call is executed
by the `waiterProvider` assertion provider.

@go-to-k
Copy link
Contributor Author

go-to-k commented Nov 8, 2023

@lpizzinidev

Thanks! I changed.

@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Nov 8, 2023
@SankyRed
Copy link
Contributor

Thank you for the contribution @go-to-k. The changes look good to me but, merge from main failed the build. Could you please take a look at this?

* Action: ['s3:GetObject'],
* Resource: ['*'],
* });
*/
public waiterProvider?: AssertionsProvider;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the build is failing here. Can't remove this docstring entirely since its a public API. Why was it removed in the first place?

Copy link
Contributor Author

@go-to-k go-to-k Dec 21, 2023

Choose a reason for hiding this comment

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

I see, I added it anyway.

I've moved the same explanation to IApiCall, and at few lines up public readonly provider: AssertionsProvider; had no docstring, so I removed this too. Why is it good that there is no docstring there (provider)?

export class AwsApiCall extends ApiCallBase {
  public readonly provider: AssertionsProvider;

I looked aws.json(packages/@aws-cdk/integ-tests-alpha/awslint.json), but could not find the property.

Copy link
Contributor Author

@go-to-k go-to-k Dec 21, 2023

Choose a reason for hiding this comment

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

I understood, that's because it's readonly.

@go-to-k
Copy link
Contributor Author

go-to-k commented Dec 21, 2023

@SankyRed @kaizencc

Thanks, I fixed and confirmed the CodeBuild success.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 8a1f519
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

Copy link
Contributor

@SankyRed SankyRed left a comment

Choose a reason for hiding this comment

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

The change looks good to me. Thanks for the contribution @go-to-k

Copy link
Contributor

mergify bot commented Dec 21, 2023

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).

@mergify mergify bot merged commit 9ca7ba8 into aws:main Dec 21, 2023
10 checks passed
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 21, 2023
comcalvi added a commit that referenced this pull request Dec 21, 2023
mergify bot pushed a commit that referenced this pull request Dec 21, 2023
Reverts #27844. This change broke the pipeline, pacmak fails with:

```
#STDOUT> /tmp/npm-packu6YRYj/Amazon.CDK.IntegTests.Alpha/Amazon/CDK/IntegTests/Alpha/ApiCallBase.cs(231,77): error CS0115: 'ApiCallBase._Proxy.WaiterProvider': no suitable method found to override [/tmp/npm-packu6YRYj/Amazon.CDK.IntegTests.Alpha/Amazon.CDK.IntegTests.Alpha.csproj]
```
@go-to-k go-to-k deleted the feat/waiterProvider-IApiCall branch December 21, 2023 23:34
paulhcsun pushed a commit to paulhcsun/aws-cdk that referenced this pull request Jan 5, 2024
This PR changes to add the `waiterProvider` property to an `IApiCall` for `awsApiCall` in integ-tests-alpha.

By default `awsApiCall` in integ tests, the AwsApiCall construct will automatically add the correct IAM policies to allow the Lambda function to make the API call. It does this based on the service and api that is provided. In the following example the service is SQS and the api is receiveMessage so it will create a policy with Action: 'sqs:ReceiveMessage'.

```ts
const integ = new IntegTest(app, 'Integ', {
  testCases: [stack],
});
integ.assertions.awsApiCall('SQS', 'receiveMessage', {
  QueueUrl: 'url',
});
```

There are some cases where the permissions do not exactly match the service/api call, for example the S3 listObjectsV2 api. In these cases it is possible to add the correct policy by accessing the `provider` object.

```ts
const apiCall = integ.assertions.awsApiCall('S3', 'listObjectsV2', {
  Bucket: 'mybucket',
});

apiCall.provider.addToRolePolicy({
  Effect: 'Allow',
  Action: ['s3:GetObject', 's3:ListBucket'],
  Resource: ['*'],
});
```

On the other hand, there is the case to use `waitForAssertions` when using `awsApiCall` in integ tests. This causes `apiCall` to have a `waiterProvider` property in addition to `provider`.

```ts
const apiCall = integ.assertions.awsApiCall('S3', 'listObjectsV2', {
  Bucket: 'mybucket',
}).expect(ExpectedResult.objectLike({
  KeyCount: 1,
})).waitForAssertions({
  interval: cdk.Duration.seconds(30),
  totalTimeout: cdk.Duration.minutes(10),
});
```

In the case, `waiterProvider` actually calls to the service/api, so it should have the proper policies.

However a type of a return value of `apiCall` is `IApiCall` interface so that the interface has a `provider` property, `waiterProvider` is not in `IApiCall` but in `AwsApiCall`.

Then it cannot take the policies without casting the following. (`apiCall instanceof AwsApiCall`)

```ts
if (apiCall instanceof AwsApiCall) {
  apiCall.waiterProvider?.addToRolePolicy({
    Effect: 'Allow',
    Action: ['s3:GetObject', 's3:ListBucket'],
    Resource: ['*'],
  });
}
```

So I add `waiterProvider` to `IApiCall`, so that it can take the policies without casting:

```ts
// if (apiCall instanceof AwsApiCall) {
  apiCall.waiterProvider?.addToRolePolicy({
    Effect: 'Allow',
    Action: ['s3:GetObject', 's3:ListBucket'],
    Resource: ['*'],
  });
//}
```

In my opinion, I see no negative impact from this.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
paulhcsun pushed a commit to paulhcsun/aws-cdk that referenced this pull request Jan 5, 2024
Reverts aws#27844. This change broke the pipeline, pacmak fails with:

```
#STDOUT> /tmp/npm-packu6YRYj/Amazon.CDK.IntegTests.Alpha/Amazon/CDK/IntegTests/Alpha/ApiCallBase.cs(231,77): error CS0115: 'ApiCallBase._Proxy.WaiterProvider': no suitable method found to override [/tmp/npm-packu6YRYj/Amazon.CDK.IntegTests.Alpha/Amazon.CDK.IntegTests.Alpha.csproj]
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants