feat(s3-deployment): support securityGroups in BucketDeploymentProps #33233

drduhe · 2025-01-30T05:51:16Z

Issue # (if applicable)

Reason for this change

The BucketDeployment construct in AWS CDK allows deploying assets to S3 buckets, often requiring a Lambda function to perform the deployment. Currently, users can specify a custom VPC via BucketDeploymentProps, ensuring the deployment happens within a restricted network.

However, many organizations require more granular network security control. While specifying a VPC is helpful, allowing custom security groups would enable teams to define specific ingress/egress rules, meeting stricter compliance and security requirements.

Description of changes

Updated BucketDeploymentProps to include an optional securityGroups?: ec2.ISecurityGroup[] property.
Modified BucketDeployment constructor to pass securityGroups to the Lambda function.
Ensured backward compatibility by keeping securityGroups optional.
Updated README to include guidance on setting vpc, vpcSubnets, and securityGroups parameters.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Added unit tests to the relevant code modules to cover feature usage.

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 fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

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.

✅ A exemption request has been requested. Please wait for a maintainer's review.

drduhe · 2025-01-30T07:26:29Z

Exemption Request - I don't think this requires a change to integration tests, per other similar PR guidance. There is also no integration tests for this module it seems.

packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts

drduhe · 2025-02-03T22:11:27Z

Any traction on getting this one looked at reviewed? I can't see the build logs as to why it is failing.

aws-cdk-automation · 2025-02-03T22:31:54Z

AWS CodeBuild CI Report

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

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

codecov · 2025-02-04T00:11:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.84%. Comparing base (fd9462c) to head (e70acf6).
Report is 26 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33233   +/-   ##
=======================================
  Coverage   80.84%   80.84%           
=======================================
  Files         236      236           
  Lines       14230    14230           
  Branches     2487     2487           
=======================================
  Hits        11504    11504           
  Misses       2442     2442           
  Partials      284      284

Flag	Coverage Δ
suite.unit	`80.84% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`79.64% <ø> (ø)`
packages/aws-cdk-lib/core	`82.14% <ø> (ø)`

pahud · 2025-02-05T18:19:52Z

Any traction on getting this one looked at reviewed? I can't see the build logs as to why it is failing.

The CI is still failing. Looks like this is the start of the failing point

aws-cdk-lib: FAIL aws-s3-deployment/test/bucket-deployment.test.ts (20.454 s)
aws-cdk-lib:   â—� different security groups create different Lambdas and single CLI
aws-cdk-lib:     Cannot find asset at /codebuild/output/src1875233622/src/github.com/aws/aws-cdk/packages/aws-cdk-lib/aws-s3-deployment/test/my-website-2
aws-cdk-lib:       173 |
aws-cdk-lib:       174 |     if (!fs.existsSync(this.sourcePath)) {
aws-cdk-lib:     > 175 |       throw new Error(`Cannot find asset at ${this.sourcePath}`);
aws-cdk-lib:           |             ^
aws-cdk-lib:       176 |     }
aws-cdk-lib:       177 |
aws-cdk-lib:       178 |     this.sourceStats = fs.statSync(this.sourcePath);
aws-cdk-lib:       at new AssetStaging (core/lib/asset-staging.ts:175:13)
aws-cdk-lib:       at new Asset (aws-s3-assets/lib/asset.ts:153:21)
aws-cdk-lib:       at Object.bind (aws-s3-deployment/lib/source.ts:132:23)
aws-cdk-lib:       at bind (aws-s3-deployment/lib/bucket-deployment.ts:395:66)
aws-cdk-lib:           at Array.map (<anonymous>)
aws-cdk-lib:       at new map (aws-s3-deployment/lib/bucket-deployment.ts:395:34)
aws-cdk-lib:       at Object.<anonymous> (aws-s3-deployment/test/bucket-deployment.test.ts:1175:3)

aaythapa

Thank you for the PR! The integration tests for this construct is included in this this dir. Could you add integ tests with assertions there? For more info about integ tests you can use this doc

aaythapa · 2025-02-07T01:03:47Z

packages/aws-cdk-lib/aws-s3-deployment/README.md

@@ -489,6 +489,65 @@ new cdk.CfnOutput(this, 'ObjectKey', {
 });
 ```

+## Specifying a Custom VPC, Subnets, and Security Groups in BucketDeployment


really nice README, thanks for adding

aaythapa · 2025-02-07T01:04:32Z

packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts

+   * Only used if 'vpc' is supplied.
+   *
+   * @default undefined - If the function is placed within a VPC and a security group is
+   * not specified, either by this or securityGroup prop, a dedicated security


there's another securityGroup prop?

aaythapa · 2025-02-07T01:04:47Z

packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts

+  /**
+   * The list of security groups to associate with the lambda handlers network interfaces.
+   *
+   * Only used if 'vpc' is supplied.


should we throw err if this is supplied without vpc?

aaythapa · 2025-02-07T01:06:06Z

packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts

+    // if the user specifies security groups, we define another singleton handler
+    // with this configuration. otherwise, it won't be possible to use multiple
+    // configurations since we have a singleton.
+    if (securityGroups && securityGroups.length > 0) {
+      const sortedSecurityGroupIds = securityGroups
+        .map(sg => sg.securityGroupId)
+        .sort() // Ensure a consistent order
+        .join('-'); // Join into a single string
+      uuid += `-${sortedSecurityGroupIds}`;
+    }


I'm not too familiar with this area of the codebase. Can you give me a short explainer on what this is doing and why we need it?

aws-cdk-automation · 2025-02-21T00:11:31Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing

To prevent automatic closure:

Resume work on the PR
OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 7 days if no action is taken.

github-actions bot added beginning-contributor feature-request p2 labels Jan 30, 2025

aws-cdk-automation requested a review from a team January 30, 2025 05:51

aws-cdk-automation requested changes Jan 30, 2025

View reviewed changes

drduhe force-pushed the issue/33229 branch from 0ae5557 to c1a1ad3 Compare January 30, 2025 06:42

drduhe requested a review from a team as a code owner January 30, 2025 06:42

drduhe force-pushed the issue/33229 branch 2 times, most recently from e9003c1 to ba8b378 Compare January 30, 2025 07:16

aws-cdk-automation added pr-linter/exemption-requested pr/needs-community-review labels Jan 30, 2025

wimlewis-amazon reviewed Jan 30, 2025

View reviewed changes

packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts Outdated Show resolved Hide resolved

drduhe force-pushed the issue/33229 branch 6 times, most recently from 8da326c to a2a7583 Compare January 30, 2025 23:19

github-actions bot added the effort/medium label Jan 30, 2025

drduhe force-pushed the issue/33229 branch 2 times, most recently from 33c979d to 71b57fa Compare January 30, 2025 23:46

github-actions bot mentioned this pull request Feb 1, 2025

Monthly PR metrics report #33266

Closed

feat(s3-deployment): support vsecurityGroups in BucketDeploymentProps

e70acf6

drduhe force-pushed the issue/33229 branch from 71b57fa to e70acf6 Compare February 3, 2025 22:11

aaythapa reviewed Feb 7, 2025

View reviewed changes

aaythapa added pr-linter/no-exemption and removed pr-linter/exemption-requested labels Feb 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s3-deployment): support securityGroups in BucketDeploymentProps #33233

feat(s3-deployment): support securityGroups in BucketDeploymentProps #33233

drduhe commented Jan 30, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

drduhe commented Jan 30, 2025 •

edited

Loading

drduhe commented Feb 3, 2025

aws-cdk-automation commented Feb 3, 2025

codecov bot commented Feb 4, 2025 •

edited

Loading

pahud commented Feb 5, 2025

aaythapa left a comment

aaythapa Feb 7, 2025

aaythapa Feb 7, 2025

aaythapa Feb 7, 2025

aaythapa Feb 7, 2025

aws-cdk-automation commented Feb 21, 2025

feat(s3-deployment): support securityGroups in BucketDeploymentProps #33233

Are you sure you want to change the base?

feat(s3-deployment): support securityGroups in BucketDeploymentProps #33233

Conversation

drduhe commented Jan 30, 2025 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

drduhe commented Jan 30, 2025 • edited Loading

drduhe commented Feb 3, 2025

aws-cdk-automation commented Feb 3, 2025

AWS CodeBuild CI Report

codecov bot commented Feb 4, 2025 • edited Loading

Codecov Report

pahud commented Feb 5, 2025

aaythapa left a comment

Choose a reason for hiding this comment

aaythapa Feb 7, 2025

Choose a reason for hiding this comment

aaythapa Feb 7, 2025

Choose a reason for hiding this comment

aaythapa Feb 7, 2025

Choose a reason for hiding this comment

aaythapa Feb 7, 2025

Choose a reason for hiding this comment

aws-cdk-automation commented Feb 21, 2025

drduhe commented Jan 30, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

drduhe commented Jan 30, 2025 •

edited

Loading

codecov bot commented Feb 4, 2025 •

edited

Loading