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(stepfunctions-tasks): validate cases of action and parameters in CallAwsService #27635

Merged
merged 11 commits into from
Oct 24, 2023

Conversation

go-to-k
Copy link
Contributor

@go-to-k go-to-k commented Oct 21, 2023

This PR adds the following validations in CallAwsService.

  • action must be camelCase.
  • parameter names in parameters must be PascalCase.

See the doc: https://docs.aws.amazon.com/step-functions/latest/dg/supported-services-awssdk.html

The API action will always be camel case, and parameter names will be Pascal case. For example, you could use Step Functions API action startSyncExecution and specify its parameter as StateMachineArn.

CloudFormation fails with a following error if there are not these validations.

Deployment failed: Error: The stack named aws-stepfunctions-tasks-call-aws-service-logs-integ failed to deploy: UPDATE_ROLLBACK_COMPLETE: Resource handler returned message: "Invalid State Machine Definition: 'SCHEMA_VALIDATION_FAILED: The resource provided arn:aws:states:::aws-sdk:cloudwatchlogs:CreateLogStream is not recognized. The value is not a valid resource ARN, or the resource is not available in this region. at /States/SendTaskSuccess/Resource' (Service: AWSStepFunctions; Status Code: 400; Error Code: InvalidDefinition; Request ID: xxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx; Proxy: null)" (RequestToken: xxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx, HandlerErrorCode: InvalidRequest)

I think it is a good thing to make these errors in the synth phase, since there were actually cases of confusion as follows.

#27623 (comment)

I also thought to not validate but translate to camel (or pascal) cases. However I thought it would allow input that violates the explanation defined in the API documentation, so I decided not to.

On the other hands, the action is also used for IAM actions so the IAM actions will be to camel cases (like logs:createLogStream). But I allowed it because IAM actions are case insensitive. If a translation is a better way to do it rather than the validation, I will consider that as well.

// The prefix and the action name are case insensitive
// https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_action.html
actions: [props.iamAction ?? `${iamService}:${props.action}`],


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 p2 admired-contributor [Pilot] contributed between 13-24 PRs to the CDK labels Oct 21, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team October 21, 2023 12:56
@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 Oct 21, 2023
Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

This looks great. Validation is the way to go. Excellent error messages! Just one small change; let's write camelCase in camelCase!

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 24, 2023
@mergify mergify bot dismissed scanlonp’s stale review October 24, 2023 19:20

Pull request has been modified.

@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 Oct 24, 2023
Copy link
Contributor

@scanlonp scanlonp left a comment

Choose a reason for hiding this comment

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

Thank you!

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Oct 24, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 24, 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).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 2316877 into aws:main Oct 24, 2023
9 checks passed
@mergify
Copy link
Contributor

mergify bot commented Oct 24, 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).

@go-to-k go-to-k deleted the chore/validate-props-action-and-parameters branch October 25, 2023 00:47
mrgrain pushed a commit that referenced this pull request Nov 1, 2023
…n CallAwsService (#27635)

This PR adds the following validations in CallAwsService.

- `action` must be camelCase.
- parameter names in `parameters` must be PascalCase.

See the doc: https://docs.aws.amazon.com/step-functions/latest/dg/supported-services-awssdk.html

> The API action will always be camel case, and parameter names will be Pascal case. For example, you could use Step Functions API action startSyncExecution and specify its parameter as StateMachineArn.

CloudFormation fails with a following error if there are not these validations.

```
Deployment failed: Error: The stack named aws-stepfunctions-tasks-call-aws-service-logs-integ failed to deploy: UPDATE_ROLLBACK_COMPLETE: Resource handler returned message: "Invalid State Machine Definition: 'SCHEMA_VALIDATION_FAILED: The resource provided arn:aws:states:::aws-sdk:cloudwatchlogs:CreateLogStream is not recognized. The value is not a valid resource ARN, or the resource is not available in this region. at /States/SendTaskSuccess/Resource' (Service: AWSStepFunctions; Status Code: 400; Error Code: InvalidDefinition; Request ID: xxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx; Proxy: null)" (RequestToken: xxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxx, HandlerErrorCode: InvalidRequest)
```

I think it is a good thing to make these errors in the synth phase, since there were actually cases of confusion as follows.

#27623 (comment)

I also thought to not validate but translate to camel (or pascal) cases. However I thought it would allow input that violates the explanation defined in the API documentation, so I decided not to.

On the other hands, the `action` is also used for IAM actions so the IAM actions will be to camel cases (like `logs:createLogStream`). But I allowed it because IAM actions are case insensitive. If a translation is a better way to do it rather than the validation, I will consider that as well.

https://github.com/aws/aws-cdk/blob/09c809b52fd2eeb27ac5bbc91d425ecf54e31bf9/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/aws-sdk/call-aws-service.ts#L92-L94

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admired-contributor [Pilot] contributed between 13-24 PRs to the CDK p2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants