feat(synthetics): add `artifactS3Encryption` property to the Canary Construct. #30197

mazyu36 · 2024-05-15T00:21:41Z

Issue # (if applicable)

Closes #30190.

Reason for this change

To select encryption options.

Description of changes

Add artifactS3Encryption property to the Canary Construct.

Description of how you validated changes

Add unit tests and integ tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mazyu36 · 2024-05-15T00:25:20Z

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

@@ -296,6 +344,31 @@ export class Canary extends cdk.Resource implements ec2.IConnectable {
      this._connections = new ec2.Connections({});
    }

+    if (props.runtime.family !== RuntimeFamily.NODEJS && props.artifactS3Encryption) {


Artifact encryption functionality is available only for canaries that use Synthetics runtime version syn-nodejs-puppeteer-3.3 or later.
However, versions prior to 3.3 are deprecated and can no longer be configured, which is why I implemented it this way.

Makes sense 👍 Can you please add a short comment about it before the check?

Thanks. I've added a comment.

lpizzinidev

Thanks 👍 Left some comments for some initial adjustments

lpizzinidev · 2024-06-15T15:56:06Z

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

+   *
+   * @see https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch_Synthetics_artifact_encryption.html
+   */
+  readonly artifactS3Encryption?: ArtifactS3Encryption;


Suggested change

readonly artifactS3Encryption?: ArtifactS3Encryption;

readonly artifactS3EncryptionMode?: ArtifactsEncryptionMode;

/**

* The KMS key used to encrypt the data.

*

* @default - A KMS key is automatically created if `artifactS3EncryptionMode` is set to `SSE_KMS`. Otherwise, no key is generated.

*/

readonly artifactS3EncryptionKey?: kms.IKey;

What about keeping this flat? (guidelines)

Thank you. Canary Construct is implemented to flatten other properties as well, so I agree that it would be better to flatten it in this case as you suggested. I will modify it to make it flat.

I've changed it to flat.

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

lpizzinidev · 2024-06-15T16:06:14Z

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

@@ -296,6 +344,31 @@ export class Canary extends cdk.Resource implements ec2.IConnectable {
      this._connections = new ec2.Connections({});
    }

+    if (props.runtime.family !== RuntimeFamily.NODEJS && props.artifactS3Encryption) {


Makes sense 👍 Can you please add a short comment about it before the check?

mazyu36 · 2024-06-16T06:30:53Z

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

@@ -296,6 +335,18 @@ export class Canary extends cdk.Resource implements ec2.IConnectable {
      this._connections = new ec2.Connections({});
    }

+    if (!cdk.Token.isUnresolved(props.artifactS3EncryptionMode) &&


I refactored the creation of artifactConfig into createArtifactConfig.
However, since kmsKey needs to be set as a property, I made it so that checking and setting are done here.

Please provide your opinion if there is a better way to do this.

I like the approach, just a couple of adjustments:

Why not move all validations inside the method?

No need to declare the public readonly encryptionKey?: kms.IKey; variable

Watch out for the typo in the function name
Suggestion:

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}.`); } 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}` }); } encryptionKey?.grantEncryptDecrypt(this.role); return { s3Encryption: { encryptionMode: props.artifactS3EncryptionMode, artifactS3KmsKeyArn: encryptionKey?.keyArn, }, }; }

mazyu36 · 2024-06-16T06:32:46Z

@lpizzinidev

Thank you for the review. I incorporated the comments and refactored.

lpizzinidev

Thanks 👍 Left comments for some final adjustments

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

lpizzinidev · 2024-06-16T15:26:29Z

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

@@ -296,6 +335,18 @@ export class Canary extends cdk.Resource implements ec2.IConnectable {
      this._connections = new ec2.Connections({});
    }

+    if (!cdk.Token.isUnresolved(props.artifactS3EncryptionMode) &&


I like the approach, just a couple of adjustments:

Why not move all validations inside the method?

No need to declare the public readonly encryptionKey?: kms.IKey; variable

Watch out for the typo in the function name
Suggestion:

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}.`); } 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}` }); } encryptionKey?.grantEncryptDecrypt(this.role); return { s3Encryption: { encryptionMode: props.artifactS3EncryptionMode, artifactS3KmsKeyArn: encryptionKey?.keyArn, }, }; }

mazyu36 · 2024-06-17T10:42:42Z

@lpizzinidev
Thank you for the review. I have addressed the comments.
(For some reason, the PR linter is showing an error...)

lpizzinidev

Thanks 👍
Not sure about the failing build. It seems like an issue with automation scripts:

error /home/runner/work/aws-cdk/aws-cdk/node_modules/@lerna/create/node_modules/nx, /home/runner/work/aws-cdk/aws-cdk/node_modules/@nx/devkit/node_modules/nx, /home/runner/work/aws-cdk/aws-cdk/node_modules/lerna/node_modules/nx: Command failed.

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

lpizzinidev

Thanks 👍

comcalvi

Minor changes only, looks great!

packages/aws-cdk-lib/aws-synthetics/README.md

comcalvi · 2024-06-20T18:25:04Z

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts

+      return undefined;
+    }
+
+    const isNodeRuntime = !cdk.Token.isUnresolved(props.runtime) && props.runtime.family === RuntimeFamily.NODEJS;


can this value ever be a Token?

Thanks. This check is redundant.
I've removed it.

packages/aws-cdk-lib/aws-synthetics/test/canary.test.ts

comcalvi · 2024-06-20T18:27:50Z

...-cdk-testing/framework-integ/test/aws-synthetics/test/integ.canary-artifact-s3-encryption.ts

+  testCases: [stack],
+});
+
+app.synth();


You don't need app.synth() at the end of new-style (eg, using IntegTest) integ tests. Old-style tests required it, but new-style ones should not use it.

Thanks. removed.

Pull request has been modified.

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

aws-cdk-automation · 2024-06-21T00:15:32Z

AWS CodeBuild CI Report

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

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

mazyu36 · 2024-06-21T00:19:39Z

@comcalvi
Thank you for the review.
I've addressed all your comments.

feat: artifact encryption

113527d

aws-cdk-automation requested a review from a team May 15, 2024 00:21

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels May 15, 2024

Merge branch 'main' into synthetics-canary-artifact-config

bc1e0c0

mazyu36 commented May 15, 2024

View reviewed changes

mazyu36 added 4 commits May 15, 2024 09:34

fix: add docs.

fee0334

fix: add docs.

3dfc533

fix: README

4abab86

fix: README

df7daf5

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

github-actions bot mentioned this pull request May 20, 2024

Weekly PR metrics report #30274

Closed

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

Monthly PR metrics report #30414

Closed

mazyu36 added 2 commits June 15, 2024 13:06

Merge branch 'main' into synthetics-canary-artifact-config

20f9030

fix: update integ test

88a8e79

lpizzinidev suggested changes Jun 15, 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 Jun 15, 2024

fix: incorporat review comments and refactor

fd44f6f

mazyu36 commented Jun 16, 2024

View reviewed changes

lpizzinidev suggested changes Jun 16, 2024

View reviewed changes

lpizzinidev mentioned this pull request Jun 16, 2024

feat(synthetics): add activeTracing, memory and timeout property to Canary class #30556

Open

1 task

mazyu36 force-pushed the synthetics-canary-artifact-config branch from 6f06849 to 5f26026 Compare June 17, 2024 10:09

lpizzinidev approved these changes Jun 17, 2024

View reviewed changes

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-synthetics/lib/canary.ts Outdated Show resolved Hide resolved

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 17, 2024

fix: incorporate review comments

4ca916c

mazyu36 force-pushed the synthetics-canary-artifact-config branch from ffa5818 to 4ca916c Compare June 18, 2024 00:43

lpizzinidev approved these changes Jun 18, 2024

View reviewed changes

comcalvi previously requested changes Jun 20, 2024

View reviewed changes

comcalvi reviewed Jun 20, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 20, 2024

Update integ.canary-artifact-s3-encryption.ts

32368cf

Merge branch 'main' into synthetics-canary-artifact-config

f3a1c8a

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 20, 2024

mazyu36 and others added 3 commits June 21, 2024 08:44

Update packages/aws-cdk-lib/aws-synthetics/test/canary.test.ts

55f42c8

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

Update packages/aws-cdk-lib/aws-synthetics/README.md

a0c8a0a

Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>

remove condition

47911d6

mazyu36 requested a review from comcalvi June 21, 2024 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(synthetics): add `artifactS3Encryption` property to the Canary Construct. #30197

feat(synthetics): add `artifactS3Encryption` property to the Canary Construct. #30197

mazyu36 commented May 15, 2024

mazyu36 May 15, 2024

lpizzinidev Jun 15, 2024

mazyu36 Jun 16, 2024

lpizzinidev left a comment

lpizzinidev Jun 15, 2024

mazyu36 Jun 15, 2024

mazyu36 Jun 16, 2024

lpizzinidev Jun 15, 2024

mazyu36 Jun 16, 2024

lpizzinidev Jun 16, 2024

mazyu36 commented Jun 16, 2024

lpizzinidev left a comment

lpizzinidev Jun 16, 2024

mazyu36 commented Jun 17, 2024

lpizzinidev left a comment

lpizzinidev left a comment

comcalvi left a comment

comcalvi Jun 20, 2024

mazyu36 Jun 21, 2024

comcalvi Jun 20, 2024

mazyu36 Jun 21, 2024

aws-cdk-automation commented Jun 21, 2024

mazyu36 commented Jun 21, 2024 •

edited

Loading

-  readonly artifactS3Encryption?: ArtifactS3Encryption;
+  readonly artifactS3EncryptionMode?: ArtifactsEncryptionMode;
+  /**
+   * The KMS key used to encrypt the data.
+   *
+   * @default - A KMS key is automatically created if `artifactS3EncryptionMode` is set to `SSE_KMS`. Otherwise, no key is generated.
+   */
+  readonly artifactS3EncryptionKey?: kms.IKey;

feat(synthetics): add artifactS3Encryption property to the Canary Construct. #30197

Are you sure you want to change the base?

feat(synthetics): add artifactS3Encryption property to the Canary Construct. #30197

Conversation

mazyu36 commented May 15, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mazyu36 commented Jun 16, 2024

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mazyu36 commented Jun 17, 2024

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev left a comment

Choose a reason for hiding this comment

comcalvi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 21, 2024

AWS CodeBuild CI Report

mazyu36 commented Jun 21, 2024 • edited Loading

feat(synthetics): add `artifactS3Encryption` property to the Canary Construct. #30197

feat(synthetics): add `artifactS3Encryption` property to the Canary Construct. #30197

mazyu36 commented Jun 21, 2024 •

edited

Loading