Skip to content

Commit

Permalink
feat(rds): performance insights for DatabaseCluster instances (#10092)
Browse files Browse the repository at this point in the history 
Adds the EnablePerformanceInsights and related props to `InstanceProps` for
instances within a cluster.

_Note:_ I opted not to try to coalesce `InstanceProps`,
`DatabaseInstanceNewProps`, and `DatabaseInstanceSourceProps` in this PR; there
are a ton of overlapping properties, but it's not immediately clear which fields
are relevant for cluster instances vs standalone instances. I think
investigating and validiting how to combine these is a significantly larger
task.

fixes #7957

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@njlynch
njlynch committed Sep 2, 2020
1 parent b72daf3 commit 9c1b0c1
Show file tree
Hide file tree
Showing 5 changed files with 194 additions and 28 deletions.
22 changes: 17 additions & 5 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref';
import { DatabaseSecret } from './database-secret';
import { Endpoint } from './endpoint';
import { IParameterGroup } from './parameter-group';
import { BackupProps, InstanceProps, Login, RotationMultiUserOptions } from './props';
import { BackupProps, InstanceProps, Login, PerformanceInsightRetention, RotationMultiUserOptions } from './props';
import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
import { CfnDBCluster, CfnDBInstance, CfnDBSubnetGroup } from './rds.generated';

Expand Down Expand Up @@ -563,8 +563,9 @@ export class DatabaseCluster extends DatabaseClusterBase {
throw new Error('At least one instance is required');
}

const instanceProps = props.instanceProps;
// Get the actual subnet objects so we can depend on internet connectivity.
const internetConnected = props.instanceProps.vpc.selectSubnets(props.instanceProps.vpcSubnets).internetConnectivityEstablished;
const internetConnected = instanceProps.vpc.selectSubnets(instanceProps.vpcSubnets).internetConnectivityEstablished;

let monitoringRole;
if (props.monitoringInterval && props.monitoringInterval.toSeconds()) {
Expand All @@ -576,15 +577,21 @@ export class DatabaseCluster extends DatabaseClusterBase {
});
}

const instanceType = props.instanceProps.instanceType ?? ec2.InstanceType.of(ec2.InstanceClass.T3, ec2.InstanceSize.MEDIUM);
const instanceParameterGroupConfig = props.instanceProps.parameterGroup?.bindToInstance({});
const enablePerformanceInsights = instanceProps.enablePerformanceInsights
|| instanceProps.performanceInsightRetention !== undefined || instanceProps.performanceInsightEncryptionKey !== undefined;
if (enablePerformanceInsights && instanceProps.enablePerformanceInsights === false) {
throw new Error('`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set');
}

const instanceType = instanceProps.instanceType ?? ec2.InstanceType.of(ec2.InstanceClass.T3, ec2.InstanceSize.MEDIUM);
const instanceParameterGroupConfig = instanceProps.parameterGroup?.bindToInstance({});
for (let i = 0; i < instanceCount; i++) {
const instanceIndex = i + 1;
const instanceIdentifier = props.instanceIdentifierBase != null ? `${props.instanceIdentifierBase}${instanceIndex}` :
props.clusterIdentifier != null ? `${props.clusterIdentifier}instance${instanceIndex}` :
undefined;

const publiclyAccessible = props.instanceProps.vpcSubnets && props.instanceProps.vpcSubnets.subnetType === ec2.SubnetType.PUBLIC;
const publiclyAccessible = instanceProps.vpcSubnets && instanceProps.vpcSubnets.subnetType === ec2.SubnetType.PUBLIC;

const instance = new CfnDBInstance(this, `Instance${instanceIndex}`, {
// Link to cluster
Expand All @@ -595,6 +602,11 @@ export class DatabaseCluster extends DatabaseClusterBase {
// Instance properties
dbInstanceClass: databaseInstanceType(instanceType),
publiclyAccessible,
enablePerformanceInsights: enablePerformanceInsights || instanceProps.enablePerformanceInsights, // fall back to undefined if not set
performanceInsightsKmsKeyId: instanceProps.performanceInsightEncryptionKey?.keyArn,
performanceInsightsRetentionPeriod: enablePerformanceInsights
? (instanceProps.performanceInsightRetention || PerformanceInsightRetention.DEFAULT)
: undefined,
// This is already set on the Cluster. Unclear to me whether it should be repeated or not. Better yes.
dbSubnetGroupName: subnetGroup.ref,
dbParameterGroupName: instanceParameterGroupConfig?.parameterGroupName,
Expand Down
33 changes: 11 additions & 22 deletions packages/@aws-cdk/aws-rds/lib/instance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { Endpoint } from './endpoint';
import { IInstanceEngine } from './instance-engine';
import { IOptionGroup } from './option-group';
import { IParameterGroup } from './parameter-group';
import { RotationMultiUserOptions } from './props';
import { PerformanceInsightRetention, RotationMultiUserOptions } from './props';
import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
import { CfnDBInstance, CfnDBInstanceProps, CfnDBSubnetGroup } from './rds.generated';

Expand Down Expand Up @@ -243,21 +243,6 @@ export enum StorageType {
IO1 = 'io1'
}

/**
* The retention period for Performance Insight.
*/
export enum PerformanceInsightRetention {
/**
* Default retention period of 7 days.
*/
DEFAULT = 7,

/**
* Long term retention period of 2 years.
*/
LONG_TERM = 731
}

/**
* Construction properties for a DatabaseInstanceNew
*/
Expand Down Expand Up @@ -415,7 +400,7 @@ export interface DatabaseInstanceNewProps {
/**
* Whether to enable Performance Insights for the DB instance.
*
* @default false
* @default - false, unless ``performanceInsightRentention`` or ``performanceInsightEncryptionKey`` is set.
*/
readonly enablePerformanceInsights?: boolean;

Expand Down Expand Up @@ -584,6 +569,12 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData
this.cloudwatchLogsRetentionRole = props.cloudwatchLogsRetentionRole;
this.enableIamAuthentication = props.iamAuthentication;

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

if (props.domain) {
this.domainId = props.domain;
this.domainRole = props.domainRole || new iam.Role(this, 'RDSDirectoryServiceRole', {
Expand All @@ -606,16 +597,14 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData
deletionProtection,
enableCloudwatchLogsExports: this.cloudwatchLogsExports,
enableIamDatabaseAuthentication: Lazy.anyValue({ produce: () => this.enableIamAuthentication }),
enablePerformanceInsights: props.enablePerformanceInsights,
enablePerformanceInsights: enablePerformanceInsights || props.enablePerformanceInsights, // fall back to undefined if not set,
iops,
monitoringInterval: props.monitoringInterval && props.monitoringInterval.toSeconds(),
monitoringRoleArn: monitoringRole && monitoringRole.roleArn,
multiAz: props.multiAz,
optionGroupName: props.optionGroup && props.optionGroup.optionGroupName,
performanceInsightsKmsKeyId: props.enablePerformanceInsights
? props.performanceInsightEncryptionKey && props.performanceInsightEncryptionKey.keyArn
: undefined,
performanceInsightsRetentionPeriod: props.enablePerformanceInsights
performanceInsightsKmsKeyId: props.performanceInsightEncryptionKey?.keyArn,
performanceInsightsRetentionPeriod: enablePerformanceInsights
? (props.performanceInsightRetention || PerformanceInsightRetention.DEFAULT)
: undefined,
port: props.port ? props.port.toString() : undefined,
Expand Down
36 changes: 36 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/props.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,27 @@ export interface InstanceProps {
* @default no parameter group
*/
readonly parameterGroup?: IParameterGroup;

/**
* Whether to enable Performance Insights for the DB instance.
*
* @default - false, unless ``performanceInsightRentention`` or ``performanceInsightEncryptionKey`` is set.
*/
readonly enablePerformanceInsights?: boolean;

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

/**
* The AWS KMS key for encryption of Performance Insights data.
*
* @default - default master key
*/
readonly performanceInsightEncryptionKey?: kms.IKey;
}

/**
Expand Down Expand Up @@ -127,3 +148,18 @@ export interface RotationMultiUserOptions {
*/
readonly automaticallyAfter?: Duration;
}

/**
* The retention period for Performance Insight.
*/
export enum PerformanceInsightRetention {
/**
* Default retention period of 7 days.
*/
DEFAULT = 7,

/**
* Long term retention period of 2 years.
*/
LONG_TERM = 731
}
79 changes: 78 additions & 1 deletion packages/@aws-cdk/aws-rds/test/test.cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import * as logs from '@aws-cdk/aws-logs';
import * as s3 from '@aws-cdk/aws-s3';
import * as cdk from '@aws-cdk/core';
import { Test } from 'nodeunit';
import { AuroraMysqlEngineVersion, AuroraPostgresEngineVersion, DatabaseCluster, DatabaseClusterEngine, ParameterGroup } from '../lib';
import { AuroraMysqlEngineVersion, AuroraPostgresEngineVersion, DatabaseCluster, DatabaseClusterEngine, ParameterGroup, PerformanceInsightRetention } from '../lib';

export = {
'creating a Cluster also creates 2 DB Instances'(test: Test) {
Expand Down Expand Up @@ -293,6 +293,83 @@ export = {

},

'performance insights': {
'cluster with all performance insights properties'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
masterUser: {
username: 'admin',
},
instanceProps: {
vpc,
enablePerformanceInsights: true,
performanceInsightRetention: PerformanceInsightRetention.LONG_TERM,
performanceInsightEncryptionKey: new kms.Key(stack, 'Key'),
},
});

expect(stack).to(haveResource('AWS::RDS::DBInstance', {
EnablePerformanceInsights: true,
PerformanceInsightsRetentionPeriod: 731,
PerformanceInsightsKMSKeyId: { 'Fn::GetAtt': ['Key961B73FD', 'Arn'] },
}));

test.done();
},

'setting performance insights fields enables performance insights'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
masterUser: {
username: 'admin',
},
instanceProps: {
vpc,
performanceInsightRetention: PerformanceInsightRetention.LONG_TERM,
},
});

expect(stack).to(haveResource('AWS::RDS::DBInstance', {
EnablePerformanceInsights: true,
PerformanceInsightsRetentionPeriod: 731,
}));

test.done();
},

'throws if performance insights fields are set but performance insights is disabled'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

test.throws(() => {
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
masterUser: {
username: 'admin',
},
instanceProps: {
vpc,
enablePerformanceInsights: false,
performanceInsightRetention: PerformanceInsightRetention.DEFAULT,
},
});
}, /`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set/);

test.done();
},
},

'create a cluster using a specific version of MySQL'(test: Test) {
// GIVEN
const stack = testStack();
Expand Down
52 changes: 52 additions & 0 deletions packages/@aws-cdk/aws-rds/test/test.instance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { ABSENT, countResources, expect, haveResource, ResourcePart, haveResourc
import * as ec2 from '@aws-cdk/aws-ec2';
import * as targets from '@aws-cdk/aws-events-targets';
import { ManagedPolicy, Role, ServicePrincipal, AccountPrincipal } from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as lambda from '@aws-cdk/aws-lambda';
import * as logs from '@aws-cdk/aws-logs';
import * as cdk from '@aws-cdk/core';
Expand Down Expand Up @@ -911,4 +912,55 @@ export = {

test.done();
},

'performance insights': {
'instance with all performance insights properties'(test: Test) {
new rds.DatabaseInstance(stack, 'Instance', {
engine: rds.DatabaseInstanceEngine.mysql({ version: rds.MysqlEngineVersion.VER_8_0_19 }),
masterUsername: 'admin',
vpc,
enablePerformanceInsights: true,
performanceInsightRetention: rds.PerformanceInsightRetention.LONG_TERM,
performanceInsightEncryptionKey: new kms.Key(stack, 'Key'),
});

expect(stack).to(haveResource('AWS::RDS::DBInstance', {
EnablePerformanceInsights: true,
PerformanceInsightsRetentionPeriod: 731,
PerformanceInsightsKMSKeyId: { 'Fn::GetAtt': ['Key961B73FD', 'Arn'] },
}));

test.done();
},

'setting performance insights fields enables performance insights'(test: Test) {
new rds.DatabaseInstance(stack, 'Instance', {
engine: rds.DatabaseInstanceEngine.mysql({ version: rds.MysqlEngineVersion.VER_8_0_19 }),
masterUsername: 'admin',
vpc,
performanceInsightRetention: rds.PerformanceInsightRetention.LONG_TERM,
});

expect(stack).to(haveResource('AWS::RDS::DBInstance', {
EnablePerformanceInsights: true,
PerformanceInsightsRetentionPeriod: 731,
}));

test.done();
},

'throws if performance insights fields are set but performance insights is disabled'(test: Test) {
test.throws(() => {
new rds.DatabaseInstance(stack, 'Instance', {
engine: rds.DatabaseInstanceEngine.mysql({ version: rds.MysqlEngineVersion.VER_8_0_19 }),
masterUsername: 'admin',
vpc,
enablePerformanceInsights: false,
performanceInsightRetention: rds.PerformanceInsightRetention.DEFAULT,
});
}, /`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set/);

test.done();
},
},
};

0 comments on commit 9c1b0c1

Please sign in to comment.