feat(app-staging-synthesizer-alpha): require passing `stagingBucketEncryption` and note that we intend to default to `S3_MANAGED` in the future #28978

blimmer · 2024-02-04T00:29:02Z

Issue # (if applicable)

Relates to #28815

Reason for this change

The App Staging Synthesizer is great - I've moved to using it for most of my stacks. However, the current default uses a Customer-Managed KMS key, which costs $1/month.

The default synthesizer bucket uses SSE-S3 encryption by default. This is nice because users do not incur additional fees for a KMS key.

In my opinion, SSE-S3 is good enough for most people. If folks need additional security, they should opt-in to SSE-KMS, which they can do via the stagingBucketEncryption property @msambol introduced with #28903.

Description of changes

With guidance from @kaizencc below, this PR makes stagingBucketEncryption a required property, with a user-facing note that we intend to default to S3_MANAGED as the module is stablized.

Description of how you validated changes

Updated unit tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

BREAKING CHANGE: stagingBucketEncryption property is now required. For existing apps, specify BucketEncryption.KMS to retain existing behavior. For new apps, choose the bucket encryption that makes most sense for your use case. BucketEncryption.S3_MANAGED is available and is intended to be the default when this module is stabilized.

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.

kaizencc

Hi @blimmer! The reason why we didn't do this in #28815 is:

Apparently, AWS best practice is to use KMS keys
The migration story for people who previously used the default KMS style is murky. It looks like if you change the encryption of a bucket, only new items in the bucket will have the new encryption type, leaving the other objects reliant on the KMS key we just removed.

For the second reason, I think the best we can do is a feature flag. For the first reason, I'm not sure if we even want to update the default and go against "best practice"

blimmer · 2024-02-05T21:01:02Z

Hey @kaizencc - thanks for your response.

Apparently, AWS best practice is to use KMS keys

Could you tell me more about this? On the S3 Security Best Practices page. I see them discussing the different options, but I don't see KMS as a strong preference.

I only use a separate KMS key for a bucket if the data is really sensitive. CDK assets don't feel particularly sensitive to me, and it's $12/year/app staging application.

If it really was an important best practice, shouldn't the default CDK Synthesizer also use a custom KMS key?

The migration story for people who previously used the default KMS style is murky. It looks like if you change the encryption of a bucket, only new items in the bucket will have the new encryption type, leaving the other objects reliant on the KMS key we just removed.

I agree that this isn't ideal, and had trouble writing a concise fix for this in my BREAKING CHANGES section.

That said, I feel fairly strongly that this is the direction we should go. The extra cost for the KMS key doesn't feel worth it to me. And it feels like the best time to make this breaking change is during the Alpha phase, where we warn people that we can and will break things at any point.

If they don't want to deal with the re-encryption of the bucket, they can simply set stagingBucketEncryption to BucketEncryption.KMS and everything remains the same.

What do you think?

kaizencc · 2024-02-06T15:12:52Z

Hi @blimmer, here's what we talked about internally.

While we do not promise stability in alpha modules, users still expect it. Therefore, its a nonstarter to flip the default of a property with no clear migration path.

We don't want to add a feature flag to an alpha module, because that is a can of worms we don't want to open. Feature flags are for stable modules that require backwards compatibility.

So, here's the proposal I have for you.

We make the stagingBucketEncryption property required. A breaking change, but one that does not involve behavioral changes. Most people can upgrade by simply supplying stagingBucketEncryption: s3.Encryption.KMS and see no diff. New users can grapple with which encryption method best suits their needs. We then add a giant FIXME to the source code that details that when stabilizing this module, we want to make the property optional again with S3_MANAGED as default.

Let me know if that sounds reasonable to you!

…fault

…on by default" This reverts commit 2e504da.

…required property

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.

blimmer · 2024-02-10T17:26:01Z

packages/@aws-cdk/app-staging-synthesizer-alpha/test/app-staging-synthesizer.test.ts

@@ -16,7 +16,7 @@ describe(AppStagingSynthesizer, () => {

  beforeEach(() => {
    app = new App({
-      defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID }),
+      defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, stagingBucketEncryption: BucketEncryption.S3_MANAGED }),


For most of these tests, I used S3_MANAGED, since we intend to make this the default in the future.

blimmer · 2024-02-10T17:26:47Z

packages/@aws-cdk/app-staging-synthesizer-alpha/test/integ.synth-default-resources.ts

@@ -17,6 +18,7 @@ const app = new App({
 const stack = new Stack(app, 'synthesize-default-resources', {
  synthesizer: AppStagingSynthesizer.defaultResources({
    appId: APP_ID_MAX, // this has implications on the overall template size
+    stagingBucketEncryption: BucketEncryption.KMS,


I left this as KMS to avoid changing the integration test. @msambol already added an S3_MANAGED integration test with #28903.

blimmer · 2024-02-10T17:31:29Z

@kaizencc - thanks for the guidance. I've made the requested changes. This is ready for re-review.

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

kaizencc

Thanks @blimmer! Hope this helps the usability of this package

packages/@aws-cdk/app-staging-synthesizer-alpha/README.md

mergify · 2024-02-13T21:38:23Z

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

aws-cdk-automation · 2024-02-13T23:01:07Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 2409e18
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2024-02-13T23:02:01Z

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

@msambol

…cryption` and note that we intend to default to `S3_MANAGED` in the future (#28978) ### Issue # (if applicable) Relates to #28815 ### Reason for this change The App Staging Synthesizer is great - I've moved to using it for most of my stacks. However, the current default uses a Customer-Managed KMS key, which costs $1/month. The default synthesizer bucket uses SSE-S3 encryption by default. This is nice because users do not incur additional fees for a KMS key. In my opinion, SSE-S3 is good enough for most people. If folks need additional security, they should opt-in to SSE-KMS, which they can do via the `stagingBucketEncryption` property @msambol introduced with #28903. ### Description of changes With guidance from @kaizencc [below](#28978 (comment)), this PR makes `stagingBucketEncryption` a required property, with a user-facing note that we intend to default to `S3_MANAGED` as the module is stablized. ### Description of how you validated changes Updated unit tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) BREAKING CHANGE: `stagingBucketEncryption` property is now required. For existing apps, specify `BucketEncryption.KMS` to retain existing behavior. For new apps, choose the bucket encryption that makes most sense for your use case. `BucketEncryption.S3_MANAGED` is available and is intended to be the default when this module is stabilized. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation requested a review from a team February 4, 2024 00:29

github-actions bot added admired-contributor [Pilot] contributed between 13-24 PRs to the CDK p2 labels Feb 4, 2024

aws-cdk-automation previously requested changes Feb 4, 2024

View reviewed changes

blimmer changed the title ~~feat!(app-staging-synthesizer-alpha): use S3-Managed encryption by default~~ feat(app-staging-synthesizer-alpha)!: use S3-Managed encryption by default Feb 4, 2024

blimmer changed the title ~~feat(app-staging-synthesizer-alpha)!: use S3-Managed encryption by default~~ feat(app-staging-synthesizer-alpha): use S3-Managed encryption by default Feb 4, 2024

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 4, 2024

kaizencc reviewed Feb 5, 2024

View reviewed changes

blimmer added 3 commits February 10, 2024 10:06

feat!(app-staging-synthesizer-alpha): use S3-Managed encryption by de…

2e504da

…fault

Revert "feat!(app-staging-synthesizer-alpha): use S3-Managed encrypti…

192611a

…on by default" This reverts commit 2e504da.

feat!(app-staging-synthesizer-alpha): make stagingBucketEncryption a …

9249e1c

…required property

blimmer force-pushed the use-s3-managed-by-default branch from 72fefbe to 9249e1c Compare February 10, 2024 17:25

aws-cdk-automation previously requested changes Feb 10, 2024

View reviewed changes

blimmer commented Feb 10, 2024

View reviewed changes

blimmer changed the title ~~feat(app-staging-synthesizer-alpha): use S3-Managed encryption by default~~ feat(app-staging-synthesizer-alpha): require passing stagingBucketEncryption and note that we intend to default to S3_MANAGED in the future Feb 10, 2024

blimmer requested a review from kaizencc February 10, 2024 17:31

blimmer added 2 commits February 10, 2024 11:11

fix: readme

d9c0b7a

more fixes

1cbf737

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 10, 2024

kaizencc added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Feb 13, 2024

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 13, 2024

kaizencc approved these changes Feb 13, 2024

View reviewed changes

packages/@aws-cdk/app-staging-synthesizer-alpha/README.md Outdated Show resolved Hide resolved

Apply suggestions from code review

a033993

kaizencc approved these changes Feb 13, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 13, 2024

Merge branch 'main' into use-s3-managed-by-default

2409e18

mergify bot merged commit fc8b955 into aws:main Feb 13, 2024
9 checks passed

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

Monthly PR metrics report - Feb 2023 #29328

Closed

This was referenced Apr 25, 2024

⬆️ Update aws-cdk monorepo to v2.145.0 WtfJoke/carbon-aware-serverless-jobs#92

Merged

chore(deps): update aws-cdk monorepo to v2.144.0 stekern/cdk-constructs#53

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app-staging-synthesizer-alpha): require passing `stagingBucketEncryption` and note that we intend to default to `S3_MANAGED` in the future #28978

feat(app-staging-synthesizer-alpha): require passing `stagingBucketEncryption` and note that we intend to default to `S3_MANAGED` in the future #28978

blimmer commented Feb 4, 2024 •

edited by kaizencc

Loading

aws-cdk-automation left a comment

kaizencc left a comment

blimmer commented Feb 5, 2024

kaizencc commented Feb 6, 2024

aws-cdk-automation left a comment

blimmer Feb 10, 2024

blimmer Feb 10, 2024

blimmer commented Feb 10, 2024

kaizencc left a comment

mergify bot commented Feb 13, 2024

aws-cdk-automation commented Feb 13, 2024

mergify bot commented Feb 13, 2024

feat(app-staging-synthesizer-alpha): require passing stagingBucketEncryption and note that we intend to default to S3_MANAGED in the future #28978

feat(app-staging-synthesizer-alpha): require passing stagingBucketEncryption and note that we intend to default to S3_MANAGED in the future #28978

Conversation

blimmer commented Feb 4, 2024 • edited by kaizencc 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

kaizencc left a comment

Choose a reason for hiding this comment

blimmer commented Feb 5, 2024

kaizencc commented Feb 6, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

blimmer Feb 10, 2024

Choose a reason for hiding this comment

blimmer Feb 10, 2024

Choose a reason for hiding this comment

blimmer commented Feb 10, 2024

kaizencc left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 13, 2024

aws-cdk-automation commented Feb 13, 2024

AWS CodeBuild CI Report

mergify bot commented Feb 13, 2024

feat(app-staging-synthesizer-alpha): require passing `stagingBucketEncryption` and note that we intend to default to `S3_MANAGED` in the future #28978

feat(app-staging-synthesizer-alpha): require passing `stagingBucketEncryption` and note that we intend to default to `S3_MANAGED` in the future #28978

blimmer commented Feb 4, 2024 •

edited by kaizencc

Loading