fix(custom-resources): not work fromSdkCalls using sdk v3 format #27268
Conversation
github-actions
bot
added
admired-contributor
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
the
pr/needs-community-review
WinterYukky
changed the title
There was a problem hiding this comment.
Thanks for helping out, but wait a minute.
It seems these changes are predicated on the fact that the user would be specifying the SDKv3 service name. For backwards compatibility, we also need to let people specify the SDKv2 service name (even though the call will be serviced using SDKv3).
So I'd rather see the following transform:
SDKv3 package name -> SDKv2 service name -> IAM permissions name
Or the alternative:
SDKv2 service name -> SDKv3 package name -> IAM permissions name
In any case: a deterministic order of mapping.
As opposed to what it feels to me like we have now:
SDKv2 service name (if using) -> IAM permissions name
SDKv3 package name (if using) -> IAM permissions name
Because the current is harder to test.
aws-cdk-automation
removed
the
pr/needs-community-review
|
Thanks for the pointers! I will take it from here myself I think. I'm bit bothered by the current state of the code. |
|
Thanks your reviewing @rix0rrr ! |
|
By the way, If we can add |
mergify
bot
dismissed
rix0rrr’s stale review
Pull request has been modified.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
aws-cdk-automation
added
the
pr/needs-community-review
I know. |
In the `AwsCustomResource` and the `Assertions` libraries, we advertise
accepting all of the following service name formats:
* The SDKv3 service name: `api-gateway`
* The full SDKv3 package name: `@aws-sdk/client-api-gateway`
* The SDKv2 constructor name: `APIGateway`
* The SDKv2 constructor name in all lower case: `apigateway`
And the following action name formats:
* The API call name: `GetRestApi`
* The API call name with a lowercase starting letter method name: `getRestApi`
* The SDKv3 command class name: `GetRestApiCommand`
However, the code that was taking care of mapping service names into an
IAM name was not handling all cases correctly. There was also an issue
with some commands that end in the word `"Command"`, like ECS's
`ExecuteCommand`, which according to the rules above should work both
written as `ExecuteCommand` as well as `ExecuteCommandCommand`: we did
not have enough information to know if we saw the string
`ExecuteCommand`, whether we should interpret it as `Execute` or
`ExecuteCommand`.
Also, we were recommending to use the full SDKv3 package name and class
name formats:
```
{
service: '@aws-sdk/client-api-gateway',
action: 'GetRestApiCommand',
}
```
Which looks ugly (imo) and leaks too many of the underlying
implementation details.
This PR changes the following:
- Deprecate the `sdk-api-metadata.json` we extracted from SDKv2.
- From SDKv3 models, extract a new `sdk-v3-metadata.json` which contains
the following information:
- IAM prefix for every service
- A list of APIs that end in the word `Command`, so we can
disambiguate around these.
- From `aws-sdk-codemod`, extract a mapping from SDKv2 service names to
SDKv3 service names (replacing the copy/pasted code we used to have
with a build-time extraction).
- Unfortunately, both of these mappings are duplicated: once for the
construct library, and once for the handlers. I did not want to go
into deduplicating between these for now.
- At runtime, we now map a potential V2 service name to a V3 service
name, then look up the V3 metadata to determine the IAM prefix and
the normalized action name.
- There was a lot of duplication between the `assertions` handler and
the `AwsCustomResource` handler. Introduce a new `ApiCall` class that
unifies the behavior between these two call sites.
- Change the recommendation in the README from using SDKv3 names to
using shorter form names (`api-gateway` and `GetRestApi`).
Fixes #27255, closes #27268, closes #27270.
In the `AwsCustomResource` and the `Assertions` libraries, we advertise accepting all of the following service name formats:
* The SDKv3 service name: `api-gateway`
* The full SDKv3 package name: `@aws-sdk/client-api-gateway`
* The SDKv2 constructor name: `APIGateway`
* The SDKv2 constructor name in all lower case: `apigateway`
And the following action name formats:
* The API call name: `GetRestApi`
* The API call name with a lowercase starting letter method name: `getRestApi`
* The SDKv3 command class name: `GetRestApiCommand`
However, the code that was taking care of mapping service names into an IAM name was not handling all cases correctly. There was also an issue with some commands that end in the word `"Command"`, like ECS's `ExecuteCommand`, which according to the rules above should work both written as `ExecuteCommand` as well as `ExecuteCommandCommand`: we did not have enough information to know if we saw the string `ExecuteCommand`, whether we should interpret it as `Execute` or `ExecuteCommand`.
Also, we were recommending to use the full SDKv3 package name and class name formats:
```
{
service: '@aws-sdk/client-api-gateway',
action: 'GetRestApiCommand',
}
```
Which looks ugly (imo) and leaks too many of the underlying implementation details.
This PR changes the following:
- Deprecate the `sdk-api-metadata.json` we extracted from SDKv2.
- From SDKv3 models, extract a new `sdk-v3-metadata.json` which contains the following information:
- IAM prefix for every service
- A list of APIs that end in the word `Command`, so we can disambiguate around these.
- From `aws-sdk-codemod`, extract a mapping from SDKv2 service names to SDKv3 service names (replacing the copy/pasted code we used to have with a build-time extraction).
- Unfortunately, both of these mappings are duplicated: once for the construct library, and once for the handlers. I did not want to go into deduplicating between these for now.
- At runtime, we now map a potential V2 service name to a V3 service name, then look up the V3 metadata to determine the IAM prefix and the normalized action name.
- There was a lot of duplication between the `assertions` handler and the `AwsCustomResource` handler (and to a lesser degree, the `events.ApiCall` handler), around loading SDKs and coercing values. Introduce a new `ApiCall` class that unifies the behavior between these call sites.
- Change the recommendation in the README from using SDKv3 names to using shorter form names (`api-gateway` and `GetRestApi`).
- Add "dynamic reuqire" protection to the `esbuild` commands for custom resources.
Fixes #27255, closes #27268, closes #27270, closes #27395.
----
*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Summary
Add support AWS SDK v3 format to
AwsCustomResourcePolicy.fromSdkCalls.How to fix this?
There are services that cannot be addressed simply by excluding
@aws-sdk/client-from the service name like a@aws-sdk/client-emr(in iam service iselasticmapreduce). There is a similar issue with AWS SDK v2, and in AWS SDK v2, IAM service names are resolved from API metadata. However, API Metadata is only compatible with AWS SDK v2, and since it cannot be used with v3, I created a script to register the v3 package name in API Metadata and made it compatible.Other
There are integration tests for AWS SDK v2 and v3 in
packages/@aws-cdk-testing/framework-integ/test/test/aws-custom-resource/integ.aws-customs-custom-resource.ts, but the AWS SDK has already been changed to V3 and both were an AWS SDK v3 test using the v2 format. So I updated my tests for v3 and reused them for testing the v3 format.Closes #27255.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license