feat(s3-deployment): added property `outputObjectKeys` for BucketDeployment #30944

mrlikl · 2024-07-24T22:07:34Z

Issue # (if applicable)

Reason for this change

The CR lambda is essentially sending back the same data in the response which is hitting the limit for close to 50 object uploads.

Particularly this is being a limitation when using servicecatalog.ProductStack, if there are local assets beyond a particular number, the Custom::CDKBucketDeployment would fail with the error Response object is too long which is a hard limit of 4096 bytes.

Description of changes

Added a new property to control the custom resource sending large data and hitting the 4096 bytes limit even though the deployment operation is successful.
The property outputObjectKeys has been set to false by default for the service catalog product so that the error does not occur.

Description of how you validated changes

Validated using a sample stack with the property set and confirmed the behavior. Also, the existing deployments would be unaffected.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

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.

Leo10Gama

Thanks for the PR! Just a few comments and concerns to be addressed.

Leo10Gama · 2024-07-29T22:28:28Z

...s/@aws-cdk/custom-resource-handlers/lib/aws-s3-deployment/bucket-deployment-handler/index.py

@@ -135,7 +136,7 @@ def cfn_error(message=None):
        cfn_send(event, context, CFN_SUCCESS, physicalResourceId=physical_id, responseData={
            # Passing through the ARN sequences dependencees on the deployment
            'DestinationBucketArn': props.get('DestinationBucketArn'),
-            'SourceObjectKeys': props.get('SourceObjectKeys'),
+            **({'SourceObjectKeys': props.get('SourceObjectKeys')} if output_object_keys else {})


I'm not too familiar on what the ** operator does in this context?

This operator is to merge the content of that dictionary with the parent dictionary

Leo10Gama · 2024-07-29T22:39:14Z

packages/aws-cdk-lib/aws-servicecatalog/lib/private/product-stack-synthesizer.ts

@@ -104,6 +104,7 @@ export class ProductStackSynthesizer extends cdk.StackSynthesizer {
        serverSideEncryption: this.serverSideEncryption,
        serverSideEncryptionAwsKmsKeyId: this.serverSideEncryptionAwsKmsKeyId,
        memoryLimit: this.memoryLimit,
+        outputObjectKeys: false,


Is setting this behaviour to false for this one module the right call? If old users have existing setups that rely on this property being true and the object keys being sent, then this acts as a breaking change for them. We can default this to true to preserve existing behaviour, and if people are running into this issue, they can set this attribute themselves.

ProductStackSynthesizer construct is a private construct that is not available for the customers, and also the bucketDeployment is also is not accessible by the customers, so I think it is Ok to set this property to false.

Leo10Gama · 2024-07-29T22:49:51Z

...ages/@aws-cdk-testing/framework-integ/test/aws-s3-deployment/test/integ.bucket-deployment.ts

+    new s3deploy.BucketDeployment(this, 'DeployMe6', {
+      sources: [s3deploy.Source.asset(path.join(__dirname, 'my-website-second'))],
+      destinationBucket: bucket6,
+      retainOnDelete: false, // default is true, which will block the integration test cleanup
+      outputObjectKeys: false,
+    });


Can we confirm that this test fails with outputObjectKeys = true? That is, will this BucketDeployment make a response object that exceeds the 4K character count, but deploys successfully when we do not include the object keys in the output?

I added a new integration Test case that test the large number of assets that should fail if the new property is not there

aws-cdk-automation · 2024-08-20T00:11:28Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2024-08-27T00:11:29Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2024-08-27T00:11:46Z

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.

Pull request has been modified.

aws-cdk-automation

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.

moelasmar · 2024-09-16T05:09:05Z

I am not able to push large files to this PR, I copied this PR to a new one #31452 ... I will close this one in favor of the new one

github-actions · 2024-09-16T05:09:22Z

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-cdk-automation · 2024-09-16T05:10:40Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 0c9a5b7
Result: FAILED
Build Logs (available for 30 days)

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

add prop outputObjectKeys

aa52965

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Jul 24, 2024

aws-cdk-automation requested a review from a team July 24, 2024 22:07

github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Jul 24, 2024

aws-cdk-automation previously requested changes Jul 24, 2024

View reviewed changes

mrlikl changed the title ~~(aws-s3-deployment): add a new property outputObjectKeys to control the custom resource from sending large data back~~ bug(aws-s3-deployment): add a new property outputObjectKeys to control the custom resource from sending large data back Jul 24, 2024

mrlikl changed the title ~~bug(aws-s3-deployment): add a new property outputObjectKeys to control the custom resource from sending large data back~~ bug(s3-deployment): add a new property outputObjectKeys to control the custom resource from sending large data back Jul 24, 2024

mrlikl changed the title ~~bug(s3-deployment): add a new property outputObjectKeys to control the custom resource from sending large data back~~ bug(s3-deployment): added property outputObjectKeys for BucketDeployment Jul 24, 2024

mrlikl changed the title ~~bug(s3-deployment): added property outputObjectKeys for BucketDeployment~~ feat(s3-deployment): added property outputObjectKeys for BucketDeployment Jul 24, 2024

mrlikl changed the title ~~feat(s3-deployment): added property outputObjectKeys for BucketDeployment~~ fix(s3-deployment): added property outputObjectKeys for BucketDeployment Jul 24, 2024

mrlikl added 3 commits July 25, 2024 04:13

integ test

41dee1c

unit tests

acc3ca9

fix integ tests

497fd70

fix integ tests

645baf7

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 25, 2024

Merge branch 'main' into s3-deploy

fae5718

github-actions bot mentioned this pull request Jul 29, 2024

Weekly PR metrics report #30970

Closed

Leo10Gama previously requested changes Jul 29, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jul 29, 2024

mrlikl requested a review from Leo10Gama July 30, 2024 13:10

github-actions bot mentioned this pull request Aug 1, 2024

Monthly PR metrics report #30999

Open

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Aug 27, 2024

aws-cdk-automation closed this Aug 27, 2024

github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2024

moelasmar reopened this Sep 14, 2024

moelasmar added 2 commits September 13, 2024 23:32

merge

27c5fec

re-run integration testing after resolving the conflicts

d2e1094

aws unlocked this conversation Sep 14, 2024

moelasmar added 2 commits September 14, 2024 18:00

fix failing integ test

17fa596

fix failing integ test

29f7f41

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 15, 2024

moelasmar changed the title ~~fix(s3-deployment): added property outputObjectKeys for BucketDeployment~~ feat(s3-deployment): added property outputObjectKeys for BucketDeployment Sep 15, 2024

aws-cdk-automation previously requested changes Sep 15, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 15, 2024

update readme

1299bc8

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Sep 15, 2024

moelasmar added 6 commits September 15, 2024 21:31

add integ test to test too many sources

e610d05

add snapshot q

ed61309

add snapshot 2

036f0b0

add snapshot 3

40559e0

add snapshot 4

ca7afbb

add snapshot 6

0c9a5b7

moelasmar closed this Sep 16, 2024

github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s3-deployment): added property `outputObjectKeys` for BucketDeployment #30944

feat(s3-deployment): added property `outputObjectKeys` for BucketDeployment #30944

mrlikl commented Jul 24, 2024 •

edited

Loading

aws-cdk-automation left a comment

Leo10Gama left a comment

Leo10Gama Jul 29, 2024

moelasmar Sep 14, 2024

Leo10Gama Jul 29, 2024

moelasmar Sep 14, 2024

Leo10Gama Jul 29, 2024

moelasmar Sep 16, 2024

aws-cdk-automation commented Aug 20, 2024

aws-cdk-automation commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

aws-cdk-automation left a comment

moelasmar commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

aws-cdk-automation commented Sep 16, 2024

feat(s3-deployment): added property outputObjectKeys for BucketDeployment #30944

feat(s3-deployment): added property outputObjectKeys for BucketDeployment #30944

Conversation

mrlikl commented Jul 24, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Leo10Gama left a comment

Choose a reason for hiding this comment

Leo10Gama Jul 29, 2024

Choose a reason for hiding this comment

moelasmar Sep 14, 2024

Choose a reason for hiding this comment

Leo10Gama Jul 29, 2024

Choose a reason for hiding this comment

moelasmar Sep 14, 2024

Choose a reason for hiding this comment

Leo10Gama Jul 29, 2024

Choose a reason for hiding this comment

moelasmar Sep 16, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 20, 2024

aws-cdk-automation commented Aug 27, 2024

github-actions bot commented Aug 27, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

moelasmar commented Sep 16, 2024

github-actions bot commented Sep 16, 2024

aws-cdk-automation commented Sep 16, 2024

AWS CodeBuild CI Report

feat(s3-deployment): added property `outputObjectKeys` for BucketDeployment #30944

feat(s3-deployment): added property `outputObjectKeys` for BucketDeployment #30944

mrlikl commented Jul 24, 2024 •

edited

Loading