feat(rds): support performance insights configuration at cluster level #31385

go-to-k · 2024-09-10T09:26:09Z

Issue # (if applicable)

Closes #31375 .

Reason for this change

Properties for Performance Insights at cluster level are supported in L1, but not in L2.

Description of changes

Added the properties in props for Database Cluster.

Description of how you validated changes

Both 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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

go-to-k · 2024-09-11T14:21:25Z

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

@@ -476,14 +476,14 @@ to use for the instance:
 declare const vpc: ec2.Vpc;

 const iopsInstance = new rds.DatabaseInstance(this, 'IopsInstance', {
-  engine: rds.DatabaseInstanceEngine.mysql({ version: rds.MysqlEngineVersion.VER_8_0_30 }),


VER_8_0_30 is deprecated.

go-to-k · 2024-09-11T14:29:40Z

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

+However, if Performance Insights is enabled at the cluster level, it is not possible to specify a different value for the instance
+level than the cluster level.


If Performance Insights was enabled at cluster and instance level, and the values of the performanceInsightRetention or performanceInsightEncryptionKey were different for the cluster and the instance, CFn failed.

If performanceInsightRetentions are different, CFn occurred:

Resource handler returned message: "PerformanceInsightsRetentionPeriod conflicts with cluster level parameter. ...“

If performanceInsightEncryptionKeys are different, CFn occurred:

Resource handler returned message: "PerformanceInsightsKMSKeyId conflicts with cluster level parameter. …”

However, if Performance Insights was enabled at cluster and instance level and the values were the same for the cluster and the instance, CFn did not fail.

If the value of enablePerformanceInsights was different for the cluster and the instance, it did not result in an error. If it was true in the cluster and false in the instance, it did not result in an error but was also enabled in the instance.

===

I wanted to implement validation of this behaviour. However, the instance does not have Performance Insights settings publicly, so it is not accessible from the cluster. Therefore, to implement validation, the structure of the instances had to be changed significantly, so I decided not to do it in this PR.

the structure of the instances had to be changed significantly,

Isn't exposing the props in AuroraClusterInstance and adding validation in validateClusterInstances enough? 🤔

I added the validation, what do you think?

The cluster has a writer and readers as IAuroraClusterInstance not AuroraClusterInstance, so I added the variable performanceInsightsEnabled in IAuroraClusterInstance interface.

And the legacyCreateInstances function for the database cluster is not a method, I also added a public variable performanceInsightsEnabled in DatabaseClusterNew.

packages/aws-cdk-lib/aws-rds/lib/cluster.ts

packages/aws-cdk-lib/aws-rds/test/cluster.test.ts

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

lpizzinidev · 2024-09-15T13:49:14Z

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

+However, if Performance Insights is enabled at the cluster level, it is not possible to specify a different value for the instance
+level than the cluster level.


the structure of the instances had to be changed significantly,

Isn't exposing the props in AuroraClusterInstance and adding validation in validateClusterInstances enough? 🤔

go-to-k · 2024-09-17T15:12:44Z

@lpizzinidev

Thanks for your review.
I have made the changes, so could you please take a look at them?

packages/aws-cdk-lib/aws-rds/lib/cluster.ts

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

lpizzinidev · 2024-09-18T15:35:07Z

packages/aws-cdk-lib/aws-rds/lib/cluster.ts

+        throw new Error(`\`performanceInsightEncryptionKey\` for each instance must be the same as the one at cluster level, got instance '${instance.node.id}': '${instance.performanceInsightEncryptionKey.keyArn}', cluster: '${this.performanceInsightEncryptionKey.keyArn}'`);
+      }
+      // Even if both of cluster and instance keys are unresolved, check if they are the same token.
+      if (compared === TokenComparison.BOTH_UNRESOLVED && clusterKeyArn !== instanceKeyArn) {


Not sure about the validity of this condition 🤔
Given that we're already ignoring the ONE_UNRESOLVED case, we could maybe skip the check for BOTH_UNRESOLVED as well

I was certainly wondering about this myself because I had never seen such an implementation. However, I thought that if only one of the values is a token, it is impossible to compare them in any way, but if both are tokens, they can be compared by check whether both are the same token.

I thought that excluding this case would diminish the validation as it would mean skipping the most common cases, such as the following. What do you think?

new DatabaseCluster(stack, 'Database', { engine: DatabaseClusterEngine.AURORA, vpc, performanceInsightEncryptionKey: new kms.Key(stack, 'Key1'), writer: ClusterInstance.provisioned('writer', { performanceInsightEncryptionKey: new kms.Key(stack, 'Key2'), }), });

packages/aws-cdk-lib/aws-rds/lib/cluster.ts

packages/aws-cdk-lib/aws-rds/test/cluster.test.ts

split split for instanceProps

go-to-k · 2024-09-19T12:14:20Z

@lpizzinidev

I have reflected on your points and replied to them, could you take another look at them?

lpizzinidev

Thanks 👍

…-cluster-ps

GavinZZ

Thanks for contributing! Left some minor feedbacks

GavinZZ · 2024-10-10T22:20:18Z

packages/aws-cdk-lib/aws-rds/lib/aurora-cluster-instance.ts

+
+  /**
+   * Whether Performance Insights is enabled
+   */
+  readonly performanceInsightsEnabled?: boolean;
+
+  /**
+   * The amount of time, in days, to retain Performance Insights data.
+   */
+  readonly performanceInsightRetention?: PerformanceInsightRetention;
+
+  /**
+   * The AWS KMS key for encryption of Performance Insights data.
+   */
+  readonly performanceInsightEncryptionKey?: kms.IKey;


Why do they need to be introduced in the interface?

The reason for this is that in order to validate instance's settings in the DatabaseClusterNew class, the properties need to be added to the interface. Otherwise, the class cannot access the properties.

see: #31385 (comment)

The writer and reader variables are not class type but the interface type (IAuroraClusterInstance), so added to the interface.

/** * Perform validations on the cluster instances */ private validateClusterInstances(writer: IAuroraClusterInstance, readers: IAuroraClusterInstance[]): void { if (writer.type === InstanceType.SERVERLESS_V2) { this.hasServerlessInstance = true; } validatePerformanceInsightsSettings(this, { nodeId: writer.node.id, performanceInsightsEnabled: writer.performanceInsightsEnabled, performanceInsightRetention: writer.performanceInsightRetention, performanceInsightEncryptionKey: writer.performanceInsightEncryptionKey, });

GavinZZ · 2024-10-10T22:21:27Z

packages/aws-cdk-lib/aws-rds/lib/aurora-cluster-instance.ts

+    this.performanceInsightsEnabled = enablePerformanceInsights;
+    this.performanceInsightRetention = enablePerformanceInsights
+      ? (props.performanceInsightRetention || PerformanceInsightRetention.DEFAULT)
+      : undefined;
+    this.performanceInsightEncryptionKey = props.performanceInsightEncryptionKey;


Why is this needed instead of the old way

This is just to set it to new variables added to the AuroraClusterInstance instance variables.

L467-469

public readonly performanceInsightsEnabled: boolean; public readonly performanceInsightRetention?: PerformanceInsightRetention; public readonly performanceInsightEncryptionKey?: kms.IKey;

GavinZZ · 2024-10-10T22:22:27Z

packages/aws-cdk-lib/aws-rds/lib/cluster.ts

+    Annotations.of(cluster).addWarningV2(
+      '@aws-cdk/aws-rds:instancePerformanceInsightsOverridden',
+      `Performance Insights is enabled on cluster '${cluster.node.id}' at cluster level, but disabled for ${target}. `+
+      'However, Performance Insights for this instance will also be automatically enabled if enabled at cluster level.',


should we just throw an error here to match the bebhaviour with the bottom checks?

CFn can deploy with this configuration. It just means that the instance configuration is overridden by the Aurora service layer not CFn.

If it can be deployed as the correct behaviour in CFn but not in CDK, it will block valid deployments. This is why I have made it a warning here.

What do you think?

That's good to know. I agree with your approach instead if it's deployable.

GavinZZ

LGTM

mergify · 2024-10-15T19:48:10Z

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).

aws-cdk-automation · 2024-10-15T20:20:23Z

AWS CodeBuild CI Report

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

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

mergify · 2024-10-15T20:20:51Z

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-10-15T20:21:06Z

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(rds): support performance insights for cluster

4e69dda

github-actions bot added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Sep 10, 2024

aws-cdk-automation requested a review from a team September 10, 2024 09:26

github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Sep 10, 2024

go-to-k changed the title ~~feat(rds): support performance insights for cluster~~ feat(rds): support performance insights configuration for cluster Sep 10, 2024

aws-cdk-automation previously requested changes Sep 10, 2024

View reviewed changes

docs

ab08d5c

go-to-k changed the title ~~feat(rds): support performance insights configuration for cluster~~ feat(rds): support performance insights configuration at cluster level Sep 10, 2024

tests

fe141a7

go-to-k marked this pull request as ready for review September 10, 2024 11:52

go-to-k marked this pull request as draft September 10, 2024 11:54

README

2a1d34d

go-to-k force-pushed the aurora-cluster-ps branch from 1c091be to 2a1d34d Compare September 10, 2024 12:58

go-to-k marked this pull request as ready for review September 10, 2024 14:15

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

go-to-k marked this pull request as draft September 11, 2024 05:37

README

05ba170

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

go-to-k added 4 commits September 11, 2024 22:18

README

2395181

readme and integ

b963715

README and integ snapshots

2854844

README

f5ae273

go-to-k marked this pull request as ready for review September 11, 2024 14:20

go-to-k commented Sep 11, 2024

View reviewed changes

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

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

github-actions bot mentioned this pull request Sep 16, 2024

Weekly PR metrics report #31456

Closed

go-to-k added 2 commits September 17, 2024 22:57

add an unit test and change README

217d945

add validation for Performance Insights duplicate

cf56890

lpizzinidev suggested changes Sep 17, 2024

View reviewed changes

packages/aws-cdk-lib/aws-rds/lib/cluster.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-rds/lib/cluster.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-rds/README.md Outdated Show resolved Hide resolved

validate keys and retentions

b9e1188

lpizzinidev suggested changes Sep 18, 2024

View reviewed changes

go-to-k added 3 commits September 19, 2024 01:23

remove redundant tests

a167d1f

to single method

c0c937f

use test.each

e5d5463

split split for instanceProps

go-to-k force-pushed the aurora-cluster-ps branch from bf16a1c to e5d5463 Compare September 19, 2024 12:12

lpizzinidev approved these changes Sep 19, 2024

View reviewed changes

Merge branch 'main' into aurora-cluster-ps

80513c1

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

Merge branch 'main' of https://github.com/go-to-k/aws-cdk into aurora…

480ac14

…-cluster-ps

GavinZZ requested changes Oct 10, 2024

View reviewed changes

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

GavinZZ approved these changes Oct 15, 2024

View reviewed changes

Merge branch 'main' into aurora-cluster-ps

3b39874

mergify bot merged commit 7d6bf77 into aws:main Oct 15, 2024
9 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): support performance insights configuration at cluster level #31385

feat(rds): support performance insights configuration at cluster level #31385

go-to-k commented Sep 10, 2024 •

edited

Loading

aws-cdk-automation left a comment

go-to-k Sep 11, 2024

go-to-k Sep 11, 2024

lpizzinidev Sep 15, 2024

go-to-k Sep 17, 2024 •

edited

Loading

lpizzinidev Sep 15, 2024

go-to-k commented Sep 17, 2024

lpizzinidev Sep 18, 2024

go-to-k Sep 18, 2024 •

edited

Loading

go-to-k commented Sep 19, 2024

lpizzinidev left a comment

GavinZZ left a comment

GavinZZ Oct 10, 2024

go-to-k Oct 11, 2024 •

edited

Loading

GavinZZ Oct 10, 2024

go-to-k Oct 11, 2024

GavinZZ Oct 10, 2024

go-to-k Oct 11, 2024 •

edited

Loading

GavinZZ Oct 15, 2024

GavinZZ left a comment

mergify bot commented Oct 15, 2024

aws-cdk-automation commented Oct 15, 2024

mergify bot commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

		However, if Performance Insights is enabled at the cluster level, it is not possible to specify a different value for the instance
		level than the cluster level.

feat(rds): support performance insights configuration at cluster level #31385

feat(rds): support performance insights configuration at cluster level #31385

Conversation

go-to-k commented Sep 10, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation 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

go-to-k Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k commented Sep 17, 2024

Choose a reason for hiding this comment

go-to-k Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

go-to-k commented Sep 19, 2024

lpizzinidev left a comment

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

go-to-k Oct 11, 2024 • edited Loading

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

go-to-k Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinZZ left a comment

Choose a reason for hiding this comment

mergify bot commented Oct 15, 2024

aws-cdk-automation commented Oct 15, 2024

AWS CodeBuild CI Report

mergify bot commented Oct 15, 2024

github-actions bot commented Oct 15, 2024

go-to-k commented Sep 10, 2024 •

edited

Loading

go-to-k Sep 17, 2024 •

edited

Loading

go-to-k Sep 18, 2024 •

edited

Loading

go-to-k Oct 11, 2024 •

edited

Loading

go-to-k Oct 11, 2024 •

edited

Loading