feat(s3-deployment): support securityGroups in BucketDeploymentProps

[drduhe]

Issue 33229

closes #33229

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.
• Testing has been implemented at a unit test and integration test level for all new logic..
• Improved unit testing patterns through all other unit tests in this module.

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]

(This review is outdated)

[drduhe]

This now has integration tests and documentation for the new feature and the missing tests/documentation for the related VPC feature previously implemented.

[drduhe]

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

[codecov]

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]

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

[Abogical]

@drduhe Yes, you can make them run sequentially by using the --parallel-regions option. You can use --parallel-regions us-east-1 which will only use us-east-1 region to deploy, effectively making the process sequential.

[drduhe]

@drduhe Yes, you can make them run sequentially by using the --parallel-regions option. You can use --parallel-regions us-east-1 which will only use us-east-1 region to deploy, effectively making the process sequential.

But this will just make it run sequentially in my local dev deployment right? How would we enforce they get run sequentially as part of the production build that happens in the Github pipeline?

[Abogical]

@drduhe The Github pipeline currently only checks that the snapshots are matching. It doesn't currently deploy the snapshots automatically.

[drduhe]

Ack - running sequentially now

[drduhe]

Ok, sorry for the delay but it took like 8+ hours to run all the tests sequentially but they all this passed when I ran it them this final time - I cleaned up the other integration tests the grouping as well with this PR. See results below from my final deployment / tests this evening:

(base) 5ce91e835887:framework-integ drduhe$ yarn integ --directory test/aws-s3-deployment/test --update-on-failed --parallel-regions us-east-1
yarn run v1.22.22
(node:4686) [DEP0169] DeprecationWarning: `url.parse()` behavior is not standardized and prone to errors that have security implications. Use the WHATWG URL API instead. CVEs are not issued for `url.parse()` vulnerabilities.
(Use `node --trace-deprecation ...` to show where the warning was created)
$ integ-runner --unstable=toolkit-lib-engine --language javascript --directory test/aws-s3-deployment/test --update-on-failed --parallel-regions us-east-1

Verifying integration test snapshots...

  UNCHANGED  integ.bucket-deployment-signcontent 4.005s
  UNCHANGED  integ.bucket-deployment-loggroup 4.13s
  UNCHANGED  integ.bucket-deployment-cloudfront 4.25s
  UNCHANGED  integ.bucket-deployment-substitution-with-role 4.301s
  UNCHANGED  integ.bucket-deployment-cross-nested-stack-source 4.318s
  UNCHANGED  integ.bucket-deployment-substitution-with-destination-key 4.375s
  UNCHANGED  integ.bucket-deployment-deployed-bucket 4.396s
  UNCHANGED  integ.bucket-deployment-security-groups-single 4.617s
  UNCHANGED  integ.bucket-deployment-cross-stack-source 5.016s
  UNCHANGED  integ.bucket-deployment-security-groups-empty 5.234s
  UNCHANGED  integ.bucket-deployment-security-groups-efs 5.363s
  UNCHANGED  integ.bucket-deployment-cross-stack-ssm-source 5.28s
  UNCHANGED  integ.bucket-deployment-security-groups-multiple 5.45s
  UNCHANGED  integ.bucket-deployment-data 5.942s
  UNCHANGED  integ.bucket-deployment-large-file 6.444s
  UNCHANGED  integ.bucket-deployment-substitution 3.526s
  UNCHANGED  integ.bucket-deployment-vpc-config 3.798s
  UNCHANGED  integ.bucket-deployment-vpc-custom-subnets 3.775s
  UNCHANGED  integ.bucket-deployment-vpc-basic 4.041s
  UNCHANGED  integ.bucket-deployment-vpc-subnet-selection 3.56s
  UNCHANGED  integ.bucket-deployment-vpc-security-groups 3.692s
  UNCHANGED  integ.bucket-deployment-vpc-efs 4.104s
  UNCHANGED  integ.bucket-deployment 4.457s
  UNCHANGED  integ.bucket-deployment-big-response 9.301s

Snapshot Results: 

Tests:    24 passed, 24 total

Running integration tests for failed tests...

Running in parallel across regions: us-east-1

Test Results: 

Tests:    0 passed, 0 total
✨  Done in 10.82s.

[Abogical]

LGTM. Thanks!

Pull request has been modified.

[drduhe]

Fixed Rosetta README.md errors. Not sure why the request-cli-integ-test / cli-changes (pull_request_target)](https://github.com/aws/aws-cdk/actions/runs/19072300010/job/54478171302?pr=33233) is failing now.

[drduhe]

Lemme know if I need to do anything specific - happy to work this down today so it doesn't lose traction again.

[Abogical]

request-cli-integ-test is not a required workflow to pass atm.

Appreciate your efforts on this @drduhe , I want to see this PR done as well. 😅

[Abogical]

@drduhe Please don't force push commits. Its hard to see what changes you made since last review when you do this.

[drduhe]

@drduhe Please don't force push commits. Its hard to see what changes you made since last review when you do this.

@Abogical - Ah, I won't do this moving forward, I realize now you support squashing on the merge and I should have left my changes as atomic commits. Pushing another commit now targeting the remaining failures in the README.md reported by Rosetta.

---

Update: It seems to be passing the Rosetta linting workflow now.

[mergify]

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]

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/codecov-upload.yml without workflows permission.

You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again.
If you think this was a flaky issue, you can requeue the pull request, without updating it, by posting a @mergifyio requeue comment.

[mergify]

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

[github-actions]

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.