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

fix(custom-resources): correctly convert values to Date type #28398

Merged
merged 30 commits into from
Mar 4, 2024

Conversation

sakurai-ryo
Copy link
Contributor

@sakurai-ryo sakurai-ryo commented Dec 17, 2023

Description

The following issue reports an error that occurs when calling an API that takes the Date type as a parameter, such as GetMetricData API, from a Custom Resource Lambda function, where the parameter is passed as string type to the AWS SDK.
#27962
https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/client/cloudwatch/command/GetMetricDataCommand/#:~:text=Description-,EndTime,-Required

To resolve this error, the string type must be properly converted to Date type when calling the AWS SDK from Lambda.
In this PR, I added the conversion to Date type in the same way as the existing conversion to number and Uint8Array types.
Uint8Array: #27034
number: #27112

Major changes

update-sdkv3-parameters-model.ts script

If the type is timestamp in the smithy specification, write d to the state machine so that it can be converted to a Date type later.
https://smithy.io/2.0/spec/simple-types.html#timestamp

update-sdkv3-parameters-model.sh script was not called from anywhere, so I called it manually and updated the JSON file.
Please let me know if there is a problem.

sdk-v2-to-v3-adapter module

I added code to convert value marked d in state machine to Date type.
If the conversion to Date type fails, the Date class does not throw an exception, so the error is handled in a slightly tricky way.
Also added a unit test for this process.

integ-tests-alpha module

Added integ-test to verify that errors reported in the related issue have been resolved.
The IAM Policy added internally by the call to adPolicyStatementFromSdkCall looks like the following and does not call GetMetricData correctly, so the addToRolePolicy method was used to explicitly add a new Policy is added explicitly with the addToRolePolicy method.

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Action": [
                "monitoring:GetMetricData"
            ],
            "Resource": [
                "*"
            ],
            "Effect": "Allow"
        }
    ]
}

fixes #27962


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 the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Dec 17, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team December 17, 2023 11:04
@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Dec 17, 2023
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@sakurai-ryo sakurai-ryo marked this pull request as ready for review December 18, 2023 11:00
@sakurai-ryo
Copy link
Contributor Author

Exemption Request: Since this PR is an internal behavior fix, I don't think it is necessary to change the README.

@aws-cdk-automation aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-maintainer-review This PR needs a review from a Core Team Member labels Dec 18, 2023
@sakurai-ryo sakurai-ryo changed the title feat(sdk-v2-to-v3-adapter): add support for coercing value to Date fix(custom-resources): correctly convert values to Date type. Dec 18, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review December 18, 2023 11:06

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@sakurai-ryo sakurai-ryo changed the title fix(custom-resources): correctly convert values to Date type. fix(custom-resources): correctly convert values to Date type Dec 18, 2023
Copy link
Contributor

@kaizencc kaizencc 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 for the very detailed PR @sakurai-ryo! This looks mostly reasonable to me, but because I'm not too familiar with this part of our repo I'm going to toss it over to @mrgrain or @MrArnoldPalmer for a second look.

@@ -332,6 +338,10 @@ function isNumber(shape: SmithyShape): boolean {
isShape('byte')(shape);
}

function isDate(shape: SmithyShape): boolean {
return isShape('timestamp')(shape)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return isShape('timestamp')(shape)
return isShape('timestamp')(shape);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I fixed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mostly because I'm not too familiar with this section of the repo, can you explain how you arrived at these updates?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be coming from running the update-sdkv3-parameters-model.sh script.

Copy link
Contributor

Choose a reason for hiding this comment

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

As well as these updates...

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be coming from running the update-sdkv3-parameters-model.sh script.

@paulhcsun
Copy link
Contributor

@sakurai-ryo There are some merge conflicts that need to be addressed. The changes look good to me, @kaizencc @mrgrain are there any other questions or changes to be made?

@sakurai-ryo
Copy link
Contributor Author

Thanks @paulhcsun! Fixed conflicts.

@paulhcsun
Copy link
Contributor

hey @sakurai-ryo,

Apologies for not getting around to this. It doesn't seem like there are any other major concerns and I'm good to merge this in. It seems like the integ snapshots are outdated. Could you rerun with latest changes to update those? I'll keep an eye on this and approve when the build is happy.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@aws-cdk-automation aws-cdk-automation dismissed their stale review March 1, 2024 12:53

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@sakurai-ryo
Copy link
Contributor Author

Hi @paulhcsun
I know the CDK team is busy, so that's not a problem at all.
Thanks for taking care of this PR.
I fixed the conflicts, please check it out.

Copy link
Contributor

@paulhcsun paulhcsun left a comment

Choose a reason for hiding this comment

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

Thanks for this fix @sakurai-ryo! And thanks again for your patience and understanding.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Copy link
Contributor

mergify bot commented Mar 4, 2024

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).

@mergify mergify bot merged commit 38bdb92 into aws:main Mar 4, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort p1 pr/needs-maintainer-review This PR needs a review from a Core Team Member pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. valued-contributor [Pilot] contributed between 6-12 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

integ-tests: awsApiCall assertion is not handling dates properly for GetMetricData with CloudWatch
5 participants