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

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

Closed
ADringer opened this issue Jul 9, 2024 · 6 comments · Fixed by #30795 · 4 remaining pull requests
Closed

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

ADringer opened this issue Jul 9, 2024 · 6 comments · Fixed by #30795 · 4 remaining pull requests
Labels
@aws-cdk/aws-stepfunctions-tasks bug This issue is a bug. effort/small Small work item – less than a day of effort p1

Comments

@ADringer
Copy link

ADringer commented Jul 9, 2024

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

@ADringer ADringer added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 9, 2024
@khushail 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 Jul 9, 2024
@khushail khushail assigned khushail and unassigned khushail Jul 9, 2024
@khushail khushail removed the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Jul 9, 2024
@ashishdhingra ashishdhingra self-assigned this Jul 9, 2024
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Jul 9, 2024

@ADringer Good morning. I think your issue is similar to #30662 (comment). 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
Copy link
Contributor

xazhao commented Jul 15, 2024

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

@mergify mergify bot closed this as completed in #30795 Jul 15, 2024
@mergify mergify bot closed this as completed in 5d6ace8 Jul 15, 2024
Copy link

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

1 similar comment
Copy link

⚠️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
Copy link
Contributor

tmokmss commented Jul 17, 2024

@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
Copy link
Collaborator

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.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.