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(synthetics): add artifactS3Encryption property to the Canary Construct. #30197

Merged
merged 29 commits into from
Nov 6, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
113527d
feat: artifact encryption
mazyu36 May 14, 2024
bc1e0c0
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 May 15, 2024
fee0334
fix: add docs.
mazyu36 May 15, 2024
3dfc533
fix: add docs.
mazyu36 May 15, 2024
4abab86
fix: README
mazyu36 May 15, 2024
df7daf5
fix: README
mazyu36 May 15, 2024
20f9030
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Jun 15, 2024
88a8e79
fix: update integ test
mazyu36 Jun 15, 2024
fd44f6f
fix: incorporat review comments and refactor
mazyu36 Jun 16, 2024
4ca916c
fix: incorporate review comments
mazyu36 Jun 17, 2024
32368cf
Update integ.canary-artifact-s3-encryption.ts
mazyu36 Jun 20, 2024
f3a1c8a
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Jun 20, 2024
55f42c8
Update packages/aws-cdk-lib/aws-synthetics/test/canary.test.ts
mazyu36 Jun 20, 2024
a0c8a0a
Update packages/aws-cdk-lib/aws-synthetics/README.md
mazyu36 Jun 20, 2024
47911d6
remove condition
mazyu36 Jun 20, 2024
917e19a
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Jul 6, 2024
54c6c9f
update integ tests
mazyu36 Jul 6, 2024
0d6bee1
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Jul 24, 2024
526ea4e
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Sep 4, 2024
86c5dd4
Merge branch 'main' into synthetics-canary-artifact-config
kaizencc Sep 4, 2024
77ba3ab
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Sep 14, 2024
be3c71d
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Oct 8, 2024
0f3039d
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Oct 26, 2024
728f9aa
Merge remote-tracking branch 'upstream/main' into synthetics-canary-a…
mazyu36 Oct 31, 2024
e340755
update
mazyu36 Oct 31, 2024
fa72ee2
update
mazyu36 Nov 5, 2024
2eaacec
Merge branch 'main' into synthetics-canary-artifact-config
mazyu36 Nov 5, 2024
45ccec0
update
mazyu36 Nov 6, 2024
24e06f6
Merge branch 'main' into synthetics-canary-artifact-config
Leo10Gama Nov 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 19 additions & 12 deletions packages/aws-cdk-lib/aws-synthetics/lib/canary.ts
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,7 @@ export interface CanaryProps {
* Artifact encryption is only supported for canaries that use Synthetics runtime
* version `syn-nodejs-puppeteer-3.3` or later.
*
* @default - Artifacts are encrypted at rest using an AWS managed key
* @default - `ArtifactsEncryptionMode.KMS` is set if you specify `artifactS3KmsKey`, otherwise artifacts are encrypted at rest using an AWS managed key
*
* @see https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_artifact_encryption.html
*/
Expand Down Expand Up @@ -669,37 +669,44 @@ export class Canary extends cdk.Resource implements ec2.IConnectable {
}

private createArtifactConfig(props: CanaryProps): CfnCanary.ArtifactConfigProperty | undefined {
if (!props.artifactS3EncryptionMode) {
if (!props.artifactS3EncryptionMode && !props.artifactS3KmsKey) {
return undefined;
}

const isNodeRuntime = 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
props.artifactS3EncryptionMode === ArtifactsEncryptionMode.S3_MANAGED &&
props.artifactS3KmsKey
Copy link
Member

Choose a reason for hiding this comment

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

I can see we're no longer checking if these props are tokens. Is it possible for users to provide them as tokens? I don't think it would change much of the logic here, but if they can be tokens, we ought to have unit tests for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I understand it, these are not expected to become tokens. Therefore, the token check is unnecessary, so I removed it.

) {
throw new Error('A customer-managed KMS key was provided, but the encryption mode is not set to SSE-KMS.');
throw new Error(`A customer-managed KMS key was provided, but the encryption mode is not set to SSE-KMS, got: ${props.artifactS3EncryptionMode}.`);
}

// Only check runtime family is Node.js because versions prior to `syn-nodejs-puppeteer-3.3` are deprecated and can no longer be configured.
if (!isNodeRuntime && isArtifactS3EncryptionModeDefined) {
if (!isNodeRuntime && props.artifactS3EncryptionMode) {
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) {
encryptionKey = props.artifactS3KmsKey ?? new kms.Key(this, 'Key', { description: `Created by ${this.node.path}` });
if (props.artifactS3EncryptionMode === ArtifactsEncryptionMode.KMS && !props.artifactS3KmsKey) {
encryptionKey = new kms.Key(this, 'Key', { description: `Created by ${this.node.path}` });
} else {
encryptionKey = props.artifactS3KmsKey;
}

encryptionKey?.grantEncryptDecrypt(this.role);

let encryptionMode: ArtifactsEncryptionMode | undefined;

if (props.artifactS3KmsKey && !props.artifactS3EncryptionMode) {
encryptionMode = ArtifactsEncryptionMode.KMS;
} else {
encryptionMode = props.artifactS3EncryptionMode;
}

return {
s3Encryption: {
encryptionMode: props.artifactS3EncryptionMode,
encryptionMode,
kmsKeyArn: encryptionKey?.keyArn,
},
};
Expand Down
28 changes: 27 additions & 1 deletion packages/aws-cdk-lib/aws-synthetics/test/canary.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,32 @@ describe('artifact encryption test', () => {
});
});

test('No artifactS3EncryptionMode setting with a key is set to SSE_KMS', () => {
// GIVEN
const stack = new Stack();
const key = new kms.Key(stack, 'myKey');

// WHEN
new synthetics.Canary(stack, 'Canary', {
test: synthetics.Test.custom({
handler: 'index.handler',
code: synthetics.Code.fromInline('/* Synthetics handler code */'),
}),
runtime: synthetics.Runtime.SYNTHETICS_NODEJS_PUPPETEER_7_0,
artifactS3KmsKey: key,
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::Synthetics::Canary', {
ArtifactConfig: {
S3Encryption: {
EncryptionMode: 'SSE_KMS',
KmsKeyArn: stack.resolve(key.keyArn),
},
},
});
});

test('SSE-S3 with a key throws', () => {
const stack = new Stack();
const key = new kms.Key(stack, 'myKey');
Expand All @@ -1045,7 +1071,7 @@ describe('artifact encryption test', () => {
artifactS3EncryptionMode: synthetics.ArtifactsEncryptionMode.S3_MANAGED,
artifactS3KmsKey: key,
});
}).toThrow('A customer-managed KMS key was provided, but the encryption mode is not set to SSE-KMS.');
}).toThrow('A customer-managed KMS key was provided, but the encryption mode is not set to SSE-KMS, got: SSE_S3.');
});

test('Artifact encryption for non-Node.js runtime throws an error', () => {
Expand Down