(custom-resources): AwsCustomResourcePolicy.fromSdkCalls does not work with v3 command format #27255

luxaritas · 2023-09-22T17:53:03Z

Describe the bug

Creating an AwsCustomResource with a service/action using the AWS CDK v3 format and a policy AwsCustomResourcePolicy.fromSdkCalls fails

Expected Behavior

A policy is correctly created

Current Behavior

Error: Action '@aws-sdk/client-ec2:ModifyInstanceMetadataOptionsCommand' is invalid. An action string consists of a service namespace, a colon, and the name of an action. Action names can include wildcards.
    at PolicyStatement.validatePolicyActions (/home/jonathan/Development/eterna/infra/node_modules/aws-cdk-lib/aws-iam/lib/policy-statement.js:2:3414)
    at PolicyStatement.addActions (/home/jonathan/Development/eterna/infra/node_modules/aws-cdk-lib/aws-iam/lib/policy-statement.js:2:1290)
    at new PolicyStatement (/home/jonathan/Development/eterna/infra/node_modules/aws-cdk-lib/aws-iam/lib/policy-statement.js:2:579)
    at new AwsCustomResource (/home/jonathan/Development/eterna/infra/node_modules/aws-cdk-lib/custom-resources/lib/aws-custom-resource/aws-custom-resource.js:1:5701

Reproduction Steps

The call I'm currently running into this with:

new AwsCustomResource(instance, `enableMetadataTags`, {
  onCreate: {
    service: '@aws-sdk/client-ec2',
    action: 'ModifyInstanceMetadataOptionsCommand',
    parameters: {
      InstanceId: instance.instanceId,
      InstanceMetadataTags: 'enabled'
    },
    physicalResourceId: PhysicalResourceId.of('EnableMetadataTags')
  },
  onDelete: {
    service: '@aws-sdk/client-ec2',
    action: 'ModifyInstanceMetadataOptionsCommand',
    parameters: {
      InstanceId: instance.instanceId,
      InstanceMetadataTags: 'disabled'
    }
  },
  policy: AwsCustomResourcePolicy.fromSdkCalls({
    resources: AwsCustomResourcePolicy.ANY_RESOURCE
  })
});

Possible Solution

I'm assuming to construct the correct policy, the service name needs to strip @aws-sdk/client- and the action needs to strip Command

Additional Information/Context

No response

CDK CLI Version

2.94.0 (build 987c329)

Framework Version

No response

Node.js Version

v18.17.1

OS

Linux

Language

Typescript

Language Version

No response

Other information

If my assessment is correct, this should be easy for me to submit a PR for if that would be helpful!

The text was updated successfully, but these errors were encountered:

khushail · 2023-09-25T20:37:01Z

Hi @luxaritas , thanks for reporting this.

@WinterYukky , thanks for submitting PR.

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*

github-actions · 2023-10-05T10:35:44Z

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

luxaritas added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 22, 2023

github-actions bot added the @aws-cdk/custom-resources Related to AWS CDK Custom Resources label Sep 22, 2023

khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 22, 2023

WinterYukky mentioned this issue Sep 25, 2023

fix(custom-resources): not work fromSdkCalls using sdk v3 format #27268

Closed

khushail added p2 effort/medium Medium work item – several days of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Sep 25, 2023

rix0rrr mentioned this issue Sep 27, 2023

fix(custom-resources): IAM policy incorrect for SDKv3 names #27313

Merged

github-actions bot mentioned this issue Oct 1, 2023

Monthly issue metrics report - Sep 2023 #27367

Closed

mergify bot closed this as completed in #27313 Oct 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(custom-resources): AwsCustomResourcePolicy.fromSdkCalls does not work with v3 command format #27255

(custom-resources): AwsCustomResourcePolicy.fromSdkCalls does not work with v3 command format #27255

luxaritas commented Sep 22, 2023 •

edited

Loading

khushail commented Sep 25, 2023

github-actions bot commented Oct 5, 2023

(custom-resources): AwsCustomResourcePolicy.fromSdkCalls does not work with v3 command format #27255

(custom-resources): AwsCustomResourcePolicy.fromSdkCalls does not work with v3 command format #27255

Comments

luxaritas commented Sep 22, 2023 • edited Loading

Describe the bug

Expected Behavior

Current Behavior

Reproduction Steps

Possible Solution

Additional Information/Context

CDK CLI Version

Framework Version

Node.js Version

OS

Language

Language Version

Other information

khushail commented Sep 25, 2023

github-actions bot commented Oct 5, 2023

⚠️COMMENT VISIBILITY WARNING⚠️

luxaritas commented Sep 22, 2023 •

edited

Loading