feat(synthetics): add `artifactS3Encryption` property to the Canary Construct. by mazyu36 · Pull Request #30197 · aws/aws-cdk

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

      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

      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

      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

      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

Leo10Gama · 2024-10-30T18:13:41Z

Because of the nature of the encryption key and permission granting changes, I'm adding the security review label to this as well. We'll review the PR internally with security and let you know if there's any extra changes needed.

…rtifact-config

Leo10Gama

Thanks for addressing the changes! Just a few more comments about some of the recent changes.

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

Leo10Gama · 2024-11-05T19:38:19Z

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

-      isArtifactS3EncryptionModeDefined &&
-      props.artifactS3EncryptionMode !== ArtifactsEncryptionMode.KMS &&
-      isArtifactS3KmsKeyDefined
+      props.artifactS3EncryptionMode === ArtifactsEncryptionMode.S3_MANAGED &&
+      props.artifactS3KmsKey


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.

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

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

Pull request has been modified.

Leo10Gama

LGTM, thanks for the contribution!

Leo10Gama · 2024-11-06T18:55:30Z

@Mergifyio update

mergify · 2024-11-06T18:55:37Z

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/github-merit-badger.yml without workflows permission

mergify · 2024-11-06T18:55:39Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2024-11-06T18:59:53Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Pull request has been modified.

aws-cdk-automation · 2024-11-06T19:45:46Z

AWS CodeBuild CI Report

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

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

mergify · 2024-11-06T19:46:16Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2024-11-06T19:46:39Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

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

Merged

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

mazyu36 added 2 commits October 31, 2024 09:59

Merge remote-tracking branch 'upstream/main' into synthetics-canary-a…

728f9aa

…rtifact-config

update

e340755

Leo10Gama removed the needs-security-review Related to feature or issues that needs security review label Nov 4, 2024

mazyu36 force-pushed the synthetics-canary-artifact-config branch from 4aaf084 to f914da9 Compare November 5, 2024 06:35

update

fa72ee2

mazyu36 force-pushed the synthetics-canary-artifact-config branch from f914da9 to fa72ee2 Compare November 5, 2024 09:07

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

2eaacec

mazyu36 requested a review from Leo10Gama November 5, 2024 13:42

Leo10Gama previously requested changes Nov 5, 2024

View reviewed changes

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

update

45ccec0

mazyu36 requested a review from Leo10Gama November 6, 2024 11:16

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

Leo10Gama previously approved these changes Nov 6, 2024

View reviewed changes

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

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

24e06f6

Leo10Gama approved these changes Nov 6, 2024

View reviewed changes

mergify bot merged commit 1f39cb9 into aws:main Nov 6, 2024

github-actions bot locked as resolved and limited conversation to collaborators Nov 6, 2024

mazyu36 deleted the synthetics-canary-artifact-config branch November 6, 2024 22:29

-  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;

Conversation

mazyu36 commented May 15, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 commented Jun 16, 2024

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mazyu36 commented Jun 17, 2024

Uh oh!

lpizzinidev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Leo10Gama commented Oct 30, 2024

Uh oh!

Leo10Gama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Leo10Gama left a comment

Choose a reason for hiding this comment

Uh oh!

Leo10Gama commented Nov 6, 2024

Uh oh!

mergify bot commented Nov 6, 2024

❌ Mergify doesn't have permission to update

Uh oh!

mergify bot commented Nov 6, 2024

Uh oh!

mergify bot commented Nov 6, 2024

Uh oh!

aws-cdk-automation commented Nov 6, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Nov 6, 2024

Uh oh!

github-actions bot commented Nov 6, 2024

Uh oh!