-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(stepfunctions-tasks): cloudwatchlogs service generates wrong action in role policy #27623
fix(stepfunctions-tasks): cloudwatchlogs service generates wrong action in role policy #27623
Conversation
…on in role policy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @go-to-k, this change looks great. Before approval, do you have a link to docs that state the correct action is logs:createLogStream
?
Also in the original issue, the user states that the action should be logs:CreateLogStream
with a capital C on Create. Another case where the docs would be helpful in confirming the correct action is being created.
Thanks for your review. It cannot be pascal case like At first, the following description is in the UserGuide reference.
https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_action.html The documentation link is also in the CDK codes. aws-cdk/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/aws-sdk/call-aws-service.ts Lines 92 to 94 in 09c809b
But the IAM actions are case insensitive, while StepFunctions SDK service integrations are case sensitive for API actions. The actions for StepFunctions must be camel case only. Also, by default, the aws-cdk/packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/aws-sdk/call-aws-service.ts Lines 21 to 26 in 09c809b
https://docs.aws.amazon.com/step-functions/latest/dg/supported-services-awssdk.html So, if you use
In this regard, I |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, thanks for the docs links! Looks good.
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). |
Pull request has been modified.
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). |
…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*
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
…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*
…on in role policy (#27623) This PR fixes the bug that a wrong action in role policy is generated when `cloudwatchlogs` service is specified. A correct action is `logs:xxx`, but current behavior is to `cloudwatchlogs:xxx` by using the `service` property. Closes #27573. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… policy (#28082) When we use CallAwsService for Step Functions task, CDK generates IAM policy to grant permission regarding the API call. However, if we specify `mwaa` as service in CallAwsService, CDK generates wrong policy statement such as `mwaa:listEnvironments`. Correct service prefix for MWAA is `airflow`. https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonmanagedworkflowsforapacheairflow.html > Amazon Managed Workflows for Apache Airflow (service prefix: airflow) provides the following service-specific resources, actions, and condition context keys for use in IAM permission policies. This PR solves the issue by adding `mwaa` into iamServiceMap. This is similar with #27623. Closes #28081 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… policy (aws#28082) When we use CallAwsService for Step Functions task, CDK generates IAM policy to grant permission regarding the API call. However, if we specify `mwaa` as service in CallAwsService, CDK generates wrong policy statement such as `mwaa:listEnvironments`. Correct service prefix for MWAA is `airflow`. https://docs.aws.amazon.com/service-authorization/latest/reference/list_amazonmanagedworkflowsforapacheairflow.html > Amazon Managed Workflows for Apache Airflow (service prefix: airflow) provides the following service-specific resources, actions, and condition context keys for use in IAM permission policies. This PR solves the issue by adding `mwaa` into iamServiceMap. This is similar with aws#27623. Closes aws#28081 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ion in role policy (#28775) When we use CallAwsService for Step Functions task, CDK generates IAM policy to grant permission regarding the API call. However, if we specify `mediapackagevod` as service in CallAwsService, CDK generates wrong policy statement such as `mediapackagevod:deleteAsset`. Correct service prefix for MediaPackageVOD is `mediapackage-vod`. https://docs.aws.amazon.com/service-authorization/latest/reference/list_awselementalmediapackagevod.html > Amazon MediaPackageVOD (service prefix: mediapackage-vod) provides the following service-specific resources, actions, and condition context keys for use in IAM permission policies. This PR solves the issue by adding mediapackagevod into iamServiceMap. This is similar with #27623 and #28082. Closes #28774. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…ion in role policy (#28775) When we use CallAwsService for Step Functions task, CDK generates IAM policy to grant permission regarding the API call. However, if we specify `mediapackagevod` as service in CallAwsService, CDK generates wrong policy statement such as `mediapackagevod:deleteAsset`. Correct service prefix for MediaPackageVOD is `mediapackage-vod`. https://docs.aws.amazon.com/service-authorization/latest/reference/list_awselementalmediapackagevod.html > Amazon MediaPackageVOD (service prefix: mediapackage-vod) provides the following service-specific resources, actions, and condition context keys for use in IAM permission policies. This PR solves the issue by adding mediapackagevod into iamServiceMap. This is similar with #27623 and #28082. Closes #28774. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR fixes the bug that a wrong action in role policy is generated when
cloudwatchlogs
service is specified.A correct action is
logs:xxx
, but current behavior is tocloudwatchlogs:xxx
by using theservice
property.Closes #27573.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license