From 5f260261cd2b80d68e95a5c6c54626a126d69a60 Mon Sep 17 00:00:00 2001 From: mazyu36 Date: Mon, 17 Jun 2024 12:16:40 +0900 Subject: [PATCH] fix: incorporate review comments --- .../integ.canary-artifact-s3-encryption.ts | 2 +- packages/aws-cdk-lib/aws-synthetics/README.md | 2 +- .../aws-cdk-lib/aws-synthetics/lib/canary.ts | 47 +++++++++---------- .../aws-synthetics/test/canary.test.ts | 17 ++++--- 4 files changed, 35 insertions(+), 33 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-synthetics/test/integ.canary-artifact-s3-encryption.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-synthetics/test/integ.canary-artifact-s3-encryption.ts index 29de5ae865cef..33a8e4e910996 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-synthetics/test/integ.canary-artifact-s3-encryption.ts +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-synthetics/test/integ.canary-artifact-s3-encryption.ts @@ -60,7 +60,7 @@ new Canary(stack, 'CanarySseKmsWith', { runtime: Runtime.SYNTHETICS_NODEJS_PUPPETEER_7_0, cleanup: Cleanup.LAMBDA, artifactS3EncryptionMode: ArtifactsEncryptionMode.KMS, - kmsKey: encryptKey, + artifactS3KmsKey: encryptKey, }); new IntegTest(app, 'IntegCanaryTest', { diff --git a/packages/aws-cdk-lib/aws-synthetics/README.md b/packages/aws-cdk-lib/aws-synthetics/README.md index 9f84b77d3c02b..a1ccc985f8173 100644 --- a/packages/aws-cdk-lib/aws-synthetics/README.md +++ b/packages/aws-cdk-lib/aws-synthetics/README.md @@ -271,6 +271,6 @@ const canary = new synthetics.Canary(this, 'MyCanary', { expiration: Duration.days(30), }], artifactS3EncryptionMode: synthetics.ArtifactsEncryptionMode.KMS, - kmsKey: key, + artifactS3KmsKey: key, }); ``` \ No newline at end of file diff --git a/packages/aws-cdk-lib/aws-synthetics/lib/canary.ts b/packages/aws-cdk-lib/aws-synthetics/lib/canary.ts index 2cb51c8536668..59b567350c435 100644 --- a/packages/aws-cdk-lib/aws-synthetics/lib/canary.ts +++ b/packages/aws-cdk-lib/aws-synthetics/lib/canary.ts @@ -243,11 +243,11 @@ export interface CanaryProps { readonly artifactS3EncryptionMode?: ArtifactsEncryptionMode; /** - * The KMS key to be used to encrypt the data. + * The KMS key used to encrypt canary artifacts. * - * @default - no kms key if mode = S3_MANAGED. A key will be created if one is not provided and mode = KMS. + * @default - no kms key if `artifactS3EncryptionMode` is set to `S3_MANAGED`. A key will be created if one is not provided and `artifactS3EncryptionMode` is set to `KMS`. */ - readonly kmsKey?: kms.IKey; + readonly artifactS3KmsKey?: kms.IKey; } /** @@ -297,11 +297,6 @@ export class Canary extends cdk.Resource implements ec2.IConnectable { */ public readonly artifactsBucket: s3.IBucket; - /** - * Optional KMS encryption key associated with this canary. - */ - public readonly encryptionKey?: kms.IKey; - /** * Actual connections object for the underlying Lambda * @@ -335,18 +330,6 @@ export class Canary extends cdk.Resource implements ec2.IConnectable { this._connections = new ec2.Connections({}); } - if (!cdk.Token.isUnresolved(props.artifactS3EncryptionMode) && - props.artifactS3EncryptionMode !== ArtifactsEncryptionMode.KMS && - !cdk.Token.isUnresolved(props.kmsKey) && - props.kmsKey) { - throw new Error('A customer-managed KMS key was provided, but the encryption mode is not set to SSE-KMS.'); - } - - if (props.artifactS3EncryptionMode === ArtifactsEncryptionMode.KMS) { - // Kms Key is set or generated for using `createArtifactConfig` - this.encryptionKey = props.kmsKey ?? new kms.Key(this, 'Key', { description: `Created by ${this.node.path}` }); - } - const resource: CfnCanary = new CfnCanary(this, 'Resource', { artifactS3Location: this.artifactsBucket.s3UrlForObject(props.artifactsBucketLocation?.prefix), executionRoleArn: this.role.roleArn, @@ -359,7 +342,7 @@ export class Canary extends cdk.Resource implements ec2.IConnectable { code: this.createCode(props), runConfig: this.createRunConfig(props), vpcConfig: this.createVpcConfig(props), - artifactConfig: this.createArticatConfig(props), + artifactConfig: this.createArtifactConfig(props), }); this._resource = resource; @@ -616,25 +599,39 @@ export class Canary extends cdk.Resource implements ec2.IConnectable { }; } - private createArticatConfig(props: CanaryProps): CfnCanary.ArtifactConfigProperty | undefined { + private createArtifactConfig(props: CanaryProps): CfnCanary.ArtifactConfigProperty | undefined { if (!props.artifactS3EncryptionMode) { return undefined; } const isNodeRuntime = !cdk.Token.isUnresolved(props.runtime) && props.runtime.family === RuntimeFamily.NODEJS; const isArtifactS3EncryptionModeDefined = !cdk.Token.isUnresolved(props.artifactS3EncryptionMode) && props.artifactS3EncryptionMode; + const isArtifactS3KmsKeyDefined = !cdk.Token.isUnresolved(props.artifactS3KmsKey) && props.artifactS3KmsKey; + + if (isArtifactS3EncryptionModeDefined && + props.artifactS3EncryptionMode !== ArtifactsEncryptionMode.KMS && + isArtifactS3KmsKeyDefined + ) { + throw new Error('A customer-managed KMS key was provided, but the encryption mode is not set to SSE-KMS.'); + } // Only check runtime family is nodejs because versions prior to syn-nodejs-puppeteer-3.3 are deprecated and can no longer be configured. if (!isNodeRuntime && isArtifactS3EncryptionModeDefined) { - throw new Error(`Artifact encryption is only supported for canaries that use Synthetics runtime version syn-nodejs-puppeteer-3.3 or later, got ${props.runtime.name}.`); + throw new Error(`Artifact encryption is only supported for canaries that use Synthetics runtime version \`syn-nodejs-puppeteer-3.3\` or later, got \`${props.runtime.name}\`.`); + } + + let encryptionKey: kms.IKey | undefined; + if (props.artifactS3EncryptionMode === ArtifactsEncryptionMode.KMS) { + // Kms Key is set or generated for using `createArtifactConfig` + encryptionKey = props.artifactS3KmsKey ?? new kms.Key(this, 'Key', { description: `Created by ${this.node.path}` }); } - this.encryptionKey?.grantEncryptDecrypt(this.role); + encryptionKey?.grantEncryptDecrypt(this.role); return { s3Encryption: { encryptionMode: props.artifactS3EncryptionMode, - kmsKeyArn: this.encryptionKey?.keyArn ?? undefined, + kmsKeyArn: encryptionKey?.keyArn, }, }; } diff --git a/packages/aws-cdk-lib/aws-synthetics/test/canary.test.ts b/packages/aws-cdk-lib/aws-synthetics/test/canary.test.ts index 24fed87840f01..3fcba96e4036b 100644 --- a/packages/aws-cdk-lib/aws-synthetics/test/canary.test.ts +++ b/packages/aws-cdk-lib/aws-synthetics/test/canary.test.ts @@ -844,7 +844,12 @@ describe('artifact encryption test', () => { ArtifactConfig: { S3Encryption: { EncryptionMode: 'SSE_KMS', - KmsKeyArn: stack.resolve(canary.encryptionKey?.keyArn), + KmsKeyArn: { + 'Fn::GetAtt': [ + 'CanaryKey36A631B4', + 'Arn', + ], + }, }, }, }); @@ -866,7 +871,7 @@ describe('artifact encryption test', () => { }), runtime: synthetics.Runtime.SYNTHETICS_NODEJS_PUPPETEER_7_0, artifactS3EncryptionMode: synthetics.ArtifactsEncryptionMode.KMS, - kmsKey: key, + artifactS3KmsKey: key, }); // THEN @@ -887,12 +892,12 @@ describe('artifact encryption test', () => { expect(() => { new synthetics.Canary(stack, 'Canary', { test: synthetics.Test.custom({ - handler: 'index.functionName', - code: synthetics.Code.fromAsset(path.join(__dirname, 'canaries')), + handler: 'index.handler', + code: synthetics.Code.fromInline('/* Synthetics handler code */'), }), runtime: synthetics.Runtime.SYNTHETICS_NODEJS_PUPPETEER_7_0, artifactS3EncryptionMode: synthetics.ArtifactsEncryptionMode.S3_MANAGED, - kmsKey: key, + artifactS3KmsKey: key, }); }).toThrow('A customer-managed KMS key was provided, but the encryption mode is not set to SSE-KMS.'); }); @@ -909,6 +914,6 @@ describe('artifact encryption test', () => { }), artifactS3EncryptionMode: synthetics.ArtifactsEncryptionMode.S3_MANAGED, }); - }).toThrow('Artifact encryption is only supported for canaries that use Synthetics runtime version syn-nodejs-puppeteer-3.3 or later, got syn-python-selenium-3.0.'); + }).toThrow('Artifact encryption is only supported for canaries that use Synthetics runtime version `syn-nodejs-puppeteer-3.3` or later, got `syn-python-selenium-3.0`.'); }); });