From 9249e1cf15c065046966d95e1948395321d2cebe Mon Sep 17 00:00:00 2001 From: Ben Limmer Date: Sat, 10 Feb 2024 10:24:42 -0700 Subject: [PATCH] feat!(app-staging-synthesizer-alpha): make stagingBucketEncryption a required property --- .../app-staging-synthesizer-alpha/README.md | 23 +++++++++-------- .../lib/default-staging-stack.ts | 25 +++++++++++++------ .../test/app-staging-synthesizer.test.ts | 10 +++++--- .../test/bootstrap-roles.test.ts | 11 +++++++- .../test/default-staging-stack.test.ts | 7 +++++- .../test/integ.synth-default-resources.ts | 2 ++ .../test/per-env-staging-factory.test.ts | 4 +++ 7 files changed, 58 insertions(+), 24 deletions(-) diff --git a/packages/@aws-cdk/app-staging-synthesizer-alpha/README.md b/packages/@aws-cdk/app-staging-synthesizer-alpha/README.md index 8664303213f26..fd7326cc63066 100644 --- a/packages/@aws-cdk/app-staging-synthesizer-alpha/README.md +++ b/packages/@aws-cdk/app-staging-synthesizer-alpha/README.md @@ -267,19 +267,20 @@ const app = new App({ ### Staging Bucket Encryption -By default, the staging resources will be stored in an S3 Bucket with KMS encryption. To use -SSE-S3, set `stagingBucketEncryption` to `BucketEncryption.S3_MANAGED`. +You must explicitly specify the encryption type for the staging bucket via the `stagingBucketEncryption` property. In +future versions of this package, the default will be `BucketEncryption.S3_MANAGED`. -```ts -import { BucketEncryption } from 'aws-cdk-lib/aws-s3'; +In previous versions of this package, the default was to use KMS encryption for the staging bucket. KMS keys cost +$1/month, which could result in unexpected costs for users who are not aware of this. As we stabilize this module +we intend to make the default S3-managed encryption, which is free. However, the migration path from KMS to S3 +managed encryption for existing buckets is not straightforward. Therefore, for now, this property is required. -const app = new App({ - defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ - appId: 'my-app-id', - stagingBucketEncryption: BucketEncryption.S3_MANAGED, - }), -}); -``` +If you have an existing staging bucket encrypted with a KMS key, you will likely want to set this property to +`BucketEncryption.KMS`. If you are creating a new staging bucket, you can set this property to +`BucketEncryption.S3_MANAGED` to avoid the cost of a KMS key. + +You can learn more about choosing a bucket encryption type in the +[S3 documentation](https://docs.aws.amazon.com/AmazonS3/latest/userguide/serv-side-encryption.html). ## Using a Custom Staging Stack per Environment diff --git a/packages/@aws-cdk/app-staging-synthesizer-alpha/lib/default-staging-stack.ts b/packages/@aws-cdk/app-staging-synthesizer-alpha/lib/default-staging-stack.ts index 70d5cfd65fbe3..e91910750cf3e 100644 --- a/packages/@aws-cdk/app-staging-synthesizer-alpha/lib/default-staging-stack.ts +++ b/packages/@aws-cdk/app-staging-synthesizer-alpha/lib/default-staging-stack.ts @@ -64,9 +64,18 @@ export interface DefaultStagingStackOptions { /** * Encryption type for staging bucket * - * @default - s3.BucketEncryption.KMS + * In future versions of this package, the default will be BucketEncryption.S3_MANAGED. + * + * In previous versions of this package, the default was to use KMS encryption for the staging bucket. KMS keys cost + * $1/month, which could result in unexpected costs for users who are not aware of this. As we stabilize this module + * we intend to make the default S3-managed encryption, which is free. However, the migration path from KMS to S3 + * managed encryption for existing buckets is not straightforward. Therefore, for now, this property is required. + * + * If you have an existing staging bucket encrypted with a KMS key, you will likely want to set this property to + * BucketEncryption.KMS. If you are creating a new staging bucket, you can set this property to + * BucketEncryption.S3_MANAGED to avoid the cost of a KMS key. */ - readonly stagingBucketEncryption?: s3.BucketEncryption; + readonly stagingBucketEncryption: s3.BucketEncryption; /** * Pass in an existing role to be used as the file publishing role. @@ -226,7 +235,7 @@ export class DefaultStagingStack extends Stack implements IStagingResources { private readonly appId: string; private readonly stagingBucketName?: string; - private stagingBucketEncryption?: s3.BucketEncryption; + private stagingBucketEncryption: s3.BucketEncryption; /** * File publish role ARN in asset manifest format @@ -267,7 +276,11 @@ export class DefaultStagingStack extends Stack implements IStagingResources { this.deployRoleArn = props.deployRoleArn; this.stagingBucketName = props.stagingBucketName; + + // FIXME: when stabilizing this module, we should make `stagingBucketEncryption` optional, defaulting to S3_MANAGED. + // See https://github.com/aws/aws-cdk/pull/28978#issuecomment-1930007176 for details on this decision. this.stagingBucketEncryption = props.stagingBucketEncryption; + const specializer = new StringSpecializer(this, props.qualifier); this.providedFileRole = props.fileAssetPublishingRole?._specialize(specializer); @@ -369,11 +382,7 @@ export class DefaultStagingStack extends Stack implements IStagingResources { this.ensureFileRole(); let key = undefined; - if (this.stagingBucketEncryption === s3.BucketEncryption.KMS || this.stagingBucketEncryption === undefined) { - if (this.stagingBucketEncryption === undefined) { - // default is KMS as an AWS best practice, and for backwards compatibility - this.stagingBucketEncryption = s3.BucketEncryption.KMS; - } + if (this.stagingBucketEncryption === s3.BucketEncryption.KMS) { key = this.createBucketKey(); } diff --git a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/app-staging-synthesizer.test.ts b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/app-staging-synthesizer.test.ts index 9b0b502a967b1..8e9f389b2c191 100644 --- a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/app-staging-synthesizer.test.ts +++ b/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 }), }); stack = new Stack(app, 'Stack', { env: { @@ -63,7 +63,7 @@ describe(AppStagingSynthesizer, () => { test('stack template is in the asset manifest - environment tokens', () => { const app2 = new App({ - defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID }), + defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, stagingBucketEncryption: BucketEncryption.S3_MANAGED }), }); const accountToken = Token.asString('111111111111'); const regionToken = Token.asString('us-east-2'); @@ -253,6 +253,7 @@ describe(AppStagingSynthesizer, () => { defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, deployTimeFileAssetLifetime: Duration.days(1), + stagingBucketEncryption: BucketEncryption.KMS, }), }); stack = new Stack(app, 'Stack', { @@ -277,7 +278,6 @@ describe(AppStagingSynthesizer, () => { Status: 'Enabled', }]), }, - // When stagingBucketEncryption is not specified, it should be KMS for backwards compatibility BucketEncryption: { ServerSideEncryptionConfiguration: [ { @@ -470,6 +470,7 @@ describe(AppStagingSynthesizer, () => { defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, imageAssetVersionCount: 1, + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); stack = new Stack(app, 'Stack', { @@ -513,6 +514,7 @@ describe(AppStagingSynthesizer, () => { defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, autoDeleteStagingAssets: false, + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); stack = new Stack(app, 'Stack', { @@ -544,6 +546,7 @@ describe(AppStagingSynthesizer, () => { defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, stagingStackNamePrefix: prefix, + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); stack = new Stack(app, 'Stack', { @@ -573,6 +576,7 @@ describe(AppStagingSynthesizer, () => { expect(() => new App({ defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: Lazy.string({ produce: () => 'appId' }), + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), })).toThrowError(/AppStagingSynthesizer property 'appId' may not contain tokens;/); }); diff --git a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/bootstrap-roles.test.ts b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/bootstrap-roles.test.ts index 63ff52e77e3fe..022e4a2cb603a 100644 --- a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/bootstrap-roles.test.ts +++ b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/bootstrap-roles.test.ts @@ -1,5 +1,6 @@ import * as fs from 'fs'; import { App, Stack, CfnResource } from 'aws-cdk-lib'; +import { BucketEncryption } from 'aws-cdk-lib/aws-s3'; import * as cxschema from 'aws-cdk-lib/cloud-assembly-schema'; import { APP_ID, isAssetManifest } from './util'; import { AppStagingSynthesizer, BootstrapRole, DeploymentIdentities } from '../lib'; @@ -14,6 +15,7 @@ describe('Boostrap Roles', () => { const app = new App({ defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: 'super long app id that needs to be cut', + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); const stack = new Stack(app, 'Stack', { @@ -47,6 +49,7 @@ describe('Boostrap Roles', () => { lookupRole: BootstrapRole.fromRoleArn(LOOKUP_ROLE), deploymentRole: BootstrapRole.fromRoleArn(DEPLOY_ACTION_ROLE), }), + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); const stack = new Stack(app, 'Stack', { @@ -79,6 +82,7 @@ describe('Boostrap Roles', () => { deploymentIdentities: DeploymentIdentities.defaultBootstrapRoles({ bootstrapRegion: 'us-west-2', }), + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); @@ -100,6 +104,7 @@ describe('Boostrap Roles', () => { deploymentIdentities: DeploymentIdentities.defaultBootstrapRoles({ bootstrapRegion: 'us-west-2', }), + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); @@ -118,6 +123,7 @@ describe('Boostrap Roles', () => { defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, fileAssetPublishingRole: BootstrapRole.fromRoleArn('arn:aws:iam::123456789012:role/S3Access'), + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); const stack = new Stack(app, 'Stack', { @@ -148,6 +154,7 @@ describe('Boostrap Roles', () => { defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, imageAssetPublishingRole: BootstrapRole.fromRoleArn('arn:aws:iam::123456789012:role/ECRAccess'), + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); const stack = new Stack(app, 'Stack', { @@ -180,6 +187,7 @@ describe('Boostrap Roles', () => { defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, deploymentIdentities: DeploymentIdentities.cliCredentials(), + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); const stack = new Stack(app, 'Stack', { @@ -209,6 +217,7 @@ describe('Boostrap Roles', () => { defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ bootstrapQualifier: 'abcdef', appId: APP_ID, + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); new Stack(app, 'Stack', { @@ -245,4 +254,4 @@ function synthStack(app: App) { // THEN return asm.getStackArtifact('Stack'); -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/default-staging-stack.test.ts b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/default-staging-stack.test.ts index ffb00dd7b1259..f2df4b2a671b4 100644 --- a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/default-staging-stack.test.ts +++ b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/default-staging-stack.test.ts @@ -1,4 +1,5 @@ import { App } from 'aws-cdk-lib'; +import { BucketEncryption } from 'aws-cdk-lib/aws-s3'; import { testWithXRepos } from './util'; import { DefaultStagingStack } from '../lib'; @@ -16,6 +17,7 @@ describe('default staging stack', () => { expect(() => new DefaultStagingStack(app, 'stack', { appId: 'a'.repeat(21), qualifier: 'qualifier', + stagingBucketEncryption: BucketEncryption.S3_MANAGED, })).toThrowError(/appId expected no more than 20 characters but got 21 characters./); }); @@ -24,6 +26,7 @@ describe('default staging stack', () => { expect(() => new DefaultStagingStack(app, 'stack', { appId: 'ABCDEF', qualifier: 'qualifier', + stagingBucketEncryption: BucketEncryption.S3_MANAGED, })).toThrowError(/appId only accepts lowercase characters./); }); @@ -32,6 +35,7 @@ describe('default staging stack', () => { expect(() => new DefaultStagingStack(app, 'stack', { appId: 'ca$h', qualifier: 'qualifier', + stagingBucketEncryption: BucketEncryption.S3_MANAGED, })).toThrowError(/appId expects only letters, numbers, and dashes \('-'\)/); }); @@ -41,6 +45,7 @@ describe('default staging stack', () => { expect(() => new DefaultStagingStack(app, 'stack', { appId, qualifier: 'qualifier', + stagingBucketEncryption: BucketEncryption.S3_MANAGED, })).toThrowError([ `appId ${appId} has errors:`, 'appId expected no more than 20 characters but got 40 characters.', @@ -49,4 +54,4 @@ describe('default staging stack', () => { ].join('\n')); }); }); -}); \ No newline at end of file +}); diff --git a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/integ.synth-default-resources.ts b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/integ.synth-default-resources.ts index 37bfa697f2794..5e21a52cf1464 100644 --- a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/integ.synth-default-resources.ts +++ b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/integ.synth-default-resources.ts @@ -2,6 +2,7 @@ import * as path from 'path'; import * as integ from '@aws-cdk/integ-tests-alpha'; import { App, Stack } from 'aws-cdk-lib'; import * as lambda from 'aws-cdk-lib/aws-lambda'; +import { BucketEncryption } from 'aws-cdk-lib/aws-s3'; import { APP_ID_MAX } from './util'; import { AppStagingSynthesizer } from '../lib'; @@ -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, }), }); diff --git a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/per-env-staging-factory.test.ts b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/per-env-staging-factory.test.ts index a5001aaa9f2f4..d865f86637b00 100644 --- a/packages/@aws-cdk/app-staging-synthesizer-alpha/test/per-env-staging-factory.test.ts +++ b/packages/@aws-cdk/app-staging-synthesizer-alpha/test/per-env-staging-factory.test.ts @@ -1,4 +1,5 @@ import { App, Stack } from 'aws-cdk-lib'; +import { BucketEncryption } from 'aws-cdk-lib/aws-s3'; import { APP_ID } from './util'; import { AppStagingSynthesizer } from '../lib'; @@ -8,6 +9,7 @@ describe('per environment cache', () => { const app = new App({ defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); new Stack(app, 'Stack1', { @@ -36,6 +38,7 @@ describe('per environment cache', () => { const app = new App({ defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); new Stack(app, 'Stack1', { @@ -64,6 +67,7 @@ describe('per environment cache', () => { const app = new App({ defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({ appId: APP_ID, + stagingBucketEncryption: BucketEncryption.S3_MANAGED, }), }); new Stack(app, 'Stack1', {