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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 18 commits into from
Oct 15, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4e69dda
feat(rds): support performance insights for cluster
go-to-k Sep 10, 2024
ab08d5c
docs
go-to-k Sep 10, 2024
fe141a7
tests
go-to-k Sep 10, 2024
2a1d34d
README
go-to-k Sep 10, 2024
05ba170
README
go-to-k Sep 11, 2024
2395181
README
go-to-k Sep 11, 2024
b963715
readme and integ
go-to-k Sep 11, 2024
2854844
README and integ snapshots
go-to-k Sep 11, 2024
f5ae273
README
go-to-k Sep 11, 2024
217d945
add an unit test and change README
go-to-k Sep 17, 2024
cf56890
add validation for Performance Insights duplicate
go-to-k Sep 17, 2024
b9e1188
validate keys and retentions
go-to-k Sep 18, 2024
a167d1f
remove redundant tests
go-to-k Sep 18, 2024
c0c937f
to single method
go-to-k Sep 19, 2024
e5d5463
use test.each
go-to-k Sep 19, 2024
80513c1
Merge branch 'main' into aurora-cluster-ps
go-to-k Sep 24, 2024
480ac14
Merge branch 'main' of https://github.com/go-to-k/aws-cdk into aurora…
go-to-k Sep 27, 2024
3b39874
Merge branch 'main' into aurora-cluster-ps
mergify[bot] Oct 15, 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
4 changes: 2 additions & 2 deletions packages/aws-cdk-lib/aws-rds/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1275,8 +1275,8 @@ To enable Performance Insights at the instance level, set the same properties fo

In this way, different settings can be applied to different instances in a cluster.

**Note:** 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.
**Note:** If Performance Insights is enabled at the cluster level, it is also automatically enabled for each instance. If specified, Performance Insights
for each instance require the same retention period and encryption key as the cluster level.

```ts
declare const vpc: ec2.Vpc;
Expand Down
27 changes: 21 additions & 6 deletions packages/aws-cdk-lib/aws-rds/lib/aurora-cluster-instance.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,16 @@ export interface IAuroraClusterInstance extends IResource {
* 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;
}

class AuroraClusterInstance extends Resource implements IAuroraClusterInstance {
Expand All @@ -455,6 +465,8 @@ class AuroraClusterInstance extends Resource implements IAuroraClusterInstance {
public readonly tier: number;
public readonly instanceSize?: string;
public readonly performanceInsightsEnabled: boolean;
public readonly performanceInsightRetention?: PerformanceInsightRetention;
public readonly performanceInsightEncryptionKey?: kms.IKey;
constructor(scope: Construct, id: string, props: AuroraClusterInstanceProps) {
super(
scope,
Expand Down Expand Up @@ -486,11 +498,16 @@ class AuroraClusterInstance extends Resource implements IAuroraClusterInstance {
const engine = props.cluster.engine!;
const enablePerformanceInsights = props.enablePerformanceInsights
|| props.performanceInsightRetention !== undefined || props.performanceInsightEncryptionKey !== undefined;
this.performanceInsightsEnabled = enablePerformanceInsights;
if (enablePerformanceInsights && props.enablePerformanceInsights === false) {
throw new Error('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set');
}

this.performanceInsightsEnabled = enablePerformanceInsights;
this.performanceInsightRetention = enablePerformanceInsights
? (props.performanceInsightRetention || PerformanceInsightRetention.DEFAULT)
: undefined;
this.performanceInsightEncryptionKey = props.performanceInsightEncryptionKey;
Comment on lines +505 to +509
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed instead of the old way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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;


const instanceParameterGroup = props.parameterGroup ?? (
props.parameters
? FeatureFlags.of(this).isEnabled(AURORA_CLUSTER_CHANGE_SCOPE_OF_INSTANCE_PARAMETER_GROUP_WITH_EACH_PARAMETERS)
Expand Down Expand Up @@ -518,11 +535,9 @@ class AuroraClusterInstance extends Resource implements IAuroraClusterInstance {
dbInstanceClass: props.instanceType ? databaseInstanceType(instanceType) : undefined,
publiclyAccessible,
preferredMaintenanceWindow: props.preferredMaintenanceWindow,
enablePerformanceInsights: enablePerformanceInsights || props.enablePerformanceInsights, // fall back to undefined if not set
performanceInsightsKmsKeyId: props.performanceInsightEncryptionKey?.keyArn,
performanceInsightsRetentionPeriod: enablePerformanceInsights
? (props.performanceInsightRetention || PerformanceInsightRetention.DEFAULT)
: undefined,
enablePerformanceInsights: this.performanceInsightsEnabled || props.enablePerformanceInsights, // fall back to undefined if not set
performanceInsightsKmsKeyId: this.performanceInsightEncryptionKey?.keyArn,
performanceInsightsRetentionPeriod: this.performanceInsightRetention,
// only need to supply this when migrating from legacy method.
// this is not applicable for aurora instances, but if you do provide it and then
// change it it will cause an instance replacement
Expand Down
138 changes: 115 additions & 23 deletions packages/aws-cdk-lib/aws-rds/lib/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import * as kms from '../../aws-kms';
import * as logs from '../../aws-logs';
import * as s3 from '../../aws-s3';
import * as secretsmanager from '../../aws-secretsmanager';
import { Annotations, ArnFormat, Duration, FeatureFlags, Lazy, RemovalPolicy, Resource, Stack, Token } from '../../core';
import { Annotations, ArnFormat, Duration, FeatureFlags, Lazy, RemovalPolicy, Resource, Stack, Token, TokenComparison } from '../../core';
import * as cxapi from '../../cx-api';

/**
Expand Down Expand Up @@ -612,6 +612,16 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
*/
public readonly performanceInsightsEnabled: boolean;

/**
* The amount of time, in days, to retain Performance Insights data.
*/
public readonly performanceInsightRetention?: PerformanceInsightRetention;

/**
* The AWS KMS key for encryption of Performance Insights data.
*/
public readonly performanceInsightEncryptionKey?: kms.IKey;

protected readonly serverlessV2MinCapacity: number;
protected readonly serverlessV2MaxCapacity: number;

Expand Down Expand Up @@ -718,11 +728,16 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {

const enablePerformanceInsights = props.enablePerformanceInsights
|| props.performanceInsightRetention !== undefined || props.performanceInsightEncryptionKey !== undefined;
this.performanceInsightsEnabled = enablePerformanceInsights;
if (enablePerformanceInsights && props.enablePerformanceInsights === false) {
throw new Error('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set');
}

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

this.newCfnProps = {
// Basic
engine: props.engine.engineType,
Expand Down Expand Up @@ -763,11 +778,9 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
copyTagsToSnapshot: props.copyTagsToSnapshot ?? true,
domain: this.domainId,
domainIamRoleName: this.domainRole?.roleName,
performanceInsightsEnabled: enablePerformanceInsights || props.enablePerformanceInsights, // fall back to undefined if not set
performanceInsightsKmsKeyId: props.performanceInsightEncryptionKey?.keyArn,
performanceInsightsRetentionPeriod: enablePerformanceInsights
? (props.performanceInsightRetention || PerformanceInsightRetention.DEFAULT)
: undefined,
performanceInsightsEnabled: this.performanceInsightsEnabled || props.enablePerformanceInsights, // fall back to undefined if not set
performanceInsightsKmsKeyId: this.performanceInsightEncryptionKey?.keyArn,
performanceInsightsRetentionPeriod: this.performanceInsightRetention,
};
}

Expand Down Expand Up @@ -831,14 +844,10 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
* Perform validations on the cluster instances
*/
private validateClusterInstances(writer: IAuroraClusterInstance, readers: IAuroraClusterInstance[]): void {
let performanceInsightsDuplicate = false;

if (writer.type === InstanceType.SERVERLESS_V2) {
this.hasServerlessInstance = true;
}
if (writer.performanceInsightsEnabled && this.performanceInsightsEnabled) {
performanceInsightsDuplicate = true;
}
this.validatePerformanceInsightsSettings(writer);
if (readers.length > 0) {
const sortedReaders = readers.sort((a, b) => a.tier - b.tier);
const highestTierReaders: IAuroraClusterInstance[] = [];
Expand Down Expand Up @@ -867,9 +876,7 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
if (reader.tier <= 1) {
noFailoverTierInstances = false;
}
if (reader.performanceInsightsEnabled && this.performanceInsightsEnabled) {
performanceInsightsDuplicate = true;
}
this.validatePerformanceInsightsSettings(reader);
}

const hasOnlyServerlessReaders = hasServerlessReader && !hasProvisionedReader;
Expand Down Expand Up @@ -905,9 +912,45 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
}
}
}
}

/**
* Validate Performance Insights settings
*/
private validatePerformanceInsightsSettings(instance: IAuroraClusterInstance): void {
// If Performance Insights is enabled on the cluster, the one for each instance will be enabled as well.
if (this.performanceInsightsEnabled && instance.performanceInsightsEnabled === false) {
Annotations.of(this).addWarningV2(
'@aws-cdk/aws-rds:instancePerformanceInsightsOverridden',
`Performance Insights is enabled on cluster '${this.node.id}' at cluster level, but disabled for instance '${instance.node.id}'. `+
'However, Performance Insights for this instance will also be automatically enabled if enabled at cluster level.',
);
}

if (performanceInsightsDuplicate) {
throw new Error('Cannot enable Performance Insights on both the cluster and the instances');
// If `performanceInsightRetention` is enabled on the cluster, the same parameter for each instance must be
// undefined or the same as the value at cluster level.
if (
this.performanceInsightRetention &&
instance.performanceInsightRetention &&
instance.performanceInsightRetention !== this.performanceInsightRetention
) {
throw new Error(`\`performanceInsightRetention\` for each instance must be the same as the one at cluster level, got instance '${instance.node.id}': ${instance.performanceInsightRetention}, cluster: ${this.performanceInsightRetention}`);
}

// If `performanceInsightEncryptionKey` is enabled on the cluster, the same parameter for each instance must be
// undefined or the same as the value at cluster level.
if (this.performanceInsightEncryptionKey && instance.performanceInsightEncryptionKey) {
const clusterKeyArn = this.performanceInsightEncryptionKey.keyArn;
const instanceKeyArn = instance.performanceInsightEncryptionKey.keyArn;
const compared = Token.compareStrings(clusterKeyArn, instanceKeyArn);

if (compared === TokenComparison.DIFFERENT) {
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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

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'),
  }),
});

throw new Error('`performanceInsightEncryptionKey` for each instance must be the same as the one at cluster level');
}
}
}

Expand Down Expand Up @@ -1430,9 +1473,15 @@ function legacyCreateInstances(cluster: DatabaseClusterNew, props: DatabaseClust
if (enablePerformanceInsights && instanceProps.enablePerformanceInsights === false) {
throw new Error('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set');
}
if (enablePerformanceInsights && cluster.performanceInsightsEnabled) {
throw new Error('Cannot enable Performance Insights on both the cluster and the instances');
}
const performanceInsightRetention = enablePerformanceInsights
? (instanceProps.performanceInsightRetention || PerformanceInsightRetention.DEFAULT)
: undefined;
legacyValidatePerformanceInsightsSettingsWithCluster(
cluster,
enablePerformanceInsights,
performanceInsightRetention,
instanceProps.performanceInsightEncryptionKey,
);

const instanceType = instanceProps.instanceType ?? ec2.InstanceType.of(ec2.InstanceClass.T3, ec2.InstanceSize.MEDIUM);

Expand Down Expand Up @@ -1469,9 +1518,7 @@ function legacyCreateInstances(cluster: DatabaseClusterNew, props: DatabaseClust
(instanceProps.vpcSubnets && instanceProps.vpcSubnets.subnetType === ec2.SubnetType.PUBLIC),
enablePerformanceInsights: enablePerformanceInsights || instanceProps.enablePerformanceInsights, // fall back to undefined if not set
performanceInsightsKmsKeyId: instanceProps.performanceInsightEncryptionKey?.keyArn,
performanceInsightsRetentionPeriod: enablePerformanceInsights
? (instanceProps.performanceInsightRetention || PerformanceInsightRetention.DEFAULT)
: undefined,
performanceInsightsRetentionPeriod: performanceInsightRetention,
// This is already set on the Cluster. Unclear to me whether it should be repeated or not. Better yes.
dbSubnetGroupName: subnetGroup.subnetGroupName,
dbParameterGroupName: instanceParameterGroupConfig?.parameterGroupName,
Expand Down Expand Up @@ -1514,3 +1561,48 @@ function legacyCreateInstances(cluster: DatabaseClusterNew, props: DatabaseClust
function databaseInstanceType(instanceType: ec2.InstanceType) {
return 'db.' + instanceType.toString();
}

/**
* Validate Performance Insights settings for legacy instanceProps with cluster level settings
*/
function legacyValidatePerformanceInsightsSettingsWithCluster(
go-to-k marked this conversation as resolved.
Show resolved Hide resolved
cluster: DatabaseClusterNew,
enableInstance: boolean,
instanceRetention?: PerformanceInsightRetention,
instanceKey?: kms.IKey,
): void {
// If Performance Insights is enabled on the cluster, the one for instanceProps will be enabled as well.
if (cluster.performanceInsightsEnabled && enableInstance === false) {
Annotations.of(cluster).addWarningV2(
'@aws-cdk/aws-rds:legacyInstancePerformanceInsightsOverridden',
`Performance Insights is enabled on cluster '${cluster.node.id}' at cluster level, but disabled for instanceProps. `+
'However, Performance Insights for this instance will also be automatically enabled if enabled at cluster level.',
);
}

// If `performanceInsightRetention` is enabled on the cluster, the same parameter for instanceProps must be
// undefined or the same as the value at cluster level.
if (
cluster.performanceInsightRetention &&
instanceRetention &&
instanceRetention !== cluster.performanceInsightRetention
) {
throw new Error(`\`performanceInsightRetention\` for each instance must be the same as the one at cluster level, got \`instanceProps\`: ${instanceRetention}, cluster: ${cluster.performanceInsightRetention}`);
}

// If `performanceInsightEncryptionKey` is enabled on the cluster, the same parameter for each instance must be
// undefined or the same as the value at cluster level.
if (cluster.performanceInsightEncryptionKey && instanceKey) {
const clusterKeyArn = cluster.performanceInsightEncryptionKey.keyArn;
const instanceKeyArn = instanceKey.keyArn;
const compared = Token.compareStrings(clusterKeyArn, instanceKeyArn);

if (compared === TokenComparison.DIFFERENT) {
throw new Error(`\`performanceInsightEncryptionKey\` for each instance must be the same as the one at cluster level, got \`instanceProps\`: '${instanceKey.keyArn}', cluster: '${cluster.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) {
throw new Error('`performanceInsightEncryptionKey` for each instance must be the same as the one at cluster level');
}
}
}
Loading