Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Feb 13, 2024
23 changes: 12 additions & 11 deletions packages/@aws-cdk/app-staging-synthesizer-alpha/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 }),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
stack = new Stack(app, 'Stack', {
env: {
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -253,6 +253,7 @@ describe(AppStagingSynthesizer, () => {
defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({
appId: APP_ID,
deployTimeFileAssetLifetime: Duration.days(1),
stagingBucketEncryption: BucketEncryption.KMS,
}),
});
stack = new Stack(app, 'Stack', {
Expand All @@ -277,7 +278,6 @@ describe(AppStagingSynthesizer, () => {
Status: 'Enabled',
}]),
},
// When stagingBucketEncryption is not specified, it should be KMS for backwards compatibility
BucketEncryption: {
ServerSideEncryptionConfiguration: [
{
Expand Down Expand Up @@ -470,6 +470,7 @@ describe(AppStagingSynthesizer, () => {
defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({
appId: APP_ID,
imageAssetVersionCount: 1,
stagingBucketEncryption: BucketEncryption.S3_MANAGED,
}),
});
stack = new Stack(app, 'Stack', {
Expand Down Expand Up @@ -513,6 +514,7 @@ describe(AppStagingSynthesizer, () => {
defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({
appId: APP_ID,
autoDeleteStagingAssets: false,
stagingBucketEncryption: BucketEncryption.S3_MANAGED,
}),
});
stack = new Stack(app, 'Stack', {
Expand Down Expand Up @@ -544,6 +546,7 @@ describe(AppStagingSynthesizer, () => {
defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({
appId: APP_ID,
stagingStackNamePrefix: prefix,
stagingBucketEncryption: BucketEncryption.S3_MANAGED,
}),
});
stack = new Stack(app, 'Stack', {
Expand Down Expand Up @@ -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;/);
});
Expand Down
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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', {
Expand Down Expand Up @@ -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', {
Expand Down Expand Up @@ -79,6 +82,7 @@ describe('Boostrap Roles', () => {
deploymentIdentities: DeploymentIdentities.defaultBootstrapRoles({
bootstrapRegion: 'us-west-2',
}),
stagingBucketEncryption: BucketEncryption.S3_MANAGED,
}),
});

Expand All @@ -100,6 +104,7 @@ describe('Boostrap Roles', () => {
deploymentIdentities: DeploymentIdentities.defaultBootstrapRoles({
bootstrapRegion: 'us-west-2',
}),
stagingBucketEncryption: BucketEncryption.S3_MANAGED,
}),
});

Expand All @@ -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', {
Expand Down Expand Up @@ -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', {
Expand Down Expand Up @@ -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', {
Expand Down Expand Up @@ -209,6 +217,7 @@ describe('Boostrap Roles', () => {
defaultStackSynthesizer: AppStagingSynthesizer.defaultResources({
bootstrapQualifier: 'abcdef',
appId: APP_ID,
stagingBucketEncryption: BucketEncryption.S3_MANAGED,
}),
});
new Stack(app, 'Stack', {
Expand Down Expand Up @@ -245,4 +254,4 @@ function synthStack(app: App) {

// THEN
return asm.getStackArtifact('Stack');
}
}
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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./);
});

Expand All @@ -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./);
});

Expand All @@ -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 \('-'\)/);
});

Expand All @@ -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.',
Expand All @@ -49,4 +54,4 @@ describe('default staging stack', () => {
].join('\n'));
});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}),
});

Expand Down
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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', {
Expand Down Expand Up @@ -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', {
Expand Down Expand Up @@ -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', {
Expand Down