aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.57k stars 3.88k forks source link

stepfunctions-tasks: CallAwsServiceCrossRegion uses wrong casing for ECS #30799

Closed ADringer closed 2 months ago

ADringer commented 3 months ago

Describe the bug

Hi, I'm using the new task CallAwsServiceCrossRegion from #30061 to update an ECS service in a different region. In the task I have to pass in parameters as PascalCase but the actual call fails in the client ecs saying the service identifier has not been provided as it's expecting camelCase.

Expected Behavior

The parameters passed from the task successfully invoke the aws service. Maybe mapping the casing accordingly.

Current Behavior

The parameters are entered into the task as PascalCase:

"parameters": {
                "Cluster.$": "$.cluster",
                "Service.$": "$.service",
                "ForceNewDeployment": true
}

And this fails with the error InvalidParameterException: Service Identifier cannot be empty.

Reproduction Steps

Create a task that calls ECS e.g:

new tasks.CallAwsServiceCrossRegion(this, "ecs-update-service", {
          service: 'ecs',
          action: 'updateService',
          iamResources: ['*'],
          region: 'us-east-2',
          parameters: {
            "Cluster.$": "myCluster",
            "Service.$": "myService",
            "ForceNewDeployment": true
          },
        })

Possible Solution

Looking at the code, if ecs cdk expects all camelCase parameters, we could update this that's causing the issue:

if (props.parameters) {
      const invalidKeys = Object.keys(props.parameters).filter((key) => !key.startsWith(key[0]?.toUpperCase()));
      if (invalidKeys.length) {
        throw new Error(`parameter names must be PascalCase, got: ${invalidKeys.join(', ')}`);
      }
    }

to be something like

const camelCaseServices: String[] = ['ecs'];

if (props.parameters) {
      const invalidKeys = Object.keys(props.parameters).filter((key) => !key.startsWith(key[0]?.toUpperCase()));
      if (invalidKeys.length && !camelCaseServices.include(props.service)) {
        throw new Error(`parameter names must be PascalCase, got: ${invalidKeys.join(', ')}`);
      }
    }

So this ignores the check for the ecs service, and can add other services that require camelCase

Additional Information/Context

No response

CDK CLI Version

2.147.3

Framework Version

No response

Node.js Version

20

OS

Linux

Language

TypeScript

Language Version

No response

Other information

No response

ashishdhingra commented 2 months ago

@ADringer Good morning. I think your issue is similar to https://github.com/aws/aws-cdk/issues/30662#issuecomment-2190107842. CDK internally uses JavaScript SDK to invoke service operation for this scenario. If you refer ECS API Reference::UpdateService, the parameters cluster, service and forceNewDeployment are in camel case.

So the parameter name casing appears to be dependent on service API, Pascal case might not be true for all the service.

The validation here appears to be incorrect and should be possibly removed or adjusted for some services (this might not be scalable).

We also would need to update documentation for parameters to remove Pascal casing guidance.

Thanks, Ashish

xazhao commented 2 months ago

Hi, I think this issue exists not only in the new CallAwsServiceCrossRegion but also in the original CallAwsService because the code is the same.

github-actions[bot] commented 2 months ago

⚠️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.

github-actions[bot] commented 2 months ago

⚠️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.

tmokmss commented 2 months ago

@xazhao Actually, no, because CallAwsService uses Step Functions' native AWS SDK integration, it seems it actually only accepts PascalCase parameters. (I tried that in bedrock-runtime API.)

aws-cdk-automation commented 2 months ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.