Skip to content

Commit

Permalink
feat(rds): unify ParameterGroup and ClusterParameterGroup
Browse files Browse the repository at this point in the history 
The classes ParameterGroup and ClusterParameterGroup were basically identical;
they only differed in the L1 that was created in their scope.
Unify the classes by adding a bind()-like protocol between ParameterGroup and cluster/instance,
which allows us to only have a single class instead of 2.

Fixes #8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead
  • Loading branch information
@skinny85
skinny85 committed Jul 9, 2020
1 parent 1bc8dfa commit 40c3e40
Show file tree
Hide file tree
Showing 6 changed files with 161 additions and 89 deletions.
12 changes: 7 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 @@ -7,7 +7,7 @@ import { CfnDeletionPolicy, Construct, Duration, RemovalPolicy, Resource, Token
import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref';
import { DatabaseSecret } from './database-secret';
import { Endpoint } from './endpoint';
import { ClusterParameterGroup, IParameterGroup } from './parameter-group';
import { IParameterGroup, ParameterGroup } from './parameter-group';
import { BackupProps, DatabaseClusterEngine, InstanceProps, Login, RotationMultiUserOptions } from './props';
import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
import { CfnDBCluster, CfnDBInstance, CfnDBSubnetGroup } from './rds.generated';
Expand Down Expand Up @@ -422,12 +422,12 @@ export class DatabaseCluster extends DatabaseClusterBase {
throw new Error(`No parameter group family found for database engine ${props.engine.name} with version ${props.engineVersion}.` +
'Failed to set the correct cluster parameters for s3 import and export roles.');
}
clusterParameterGroup = new ClusterParameterGroup(this, 'ClusterParameterGroup', {
clusterParameterGroup = new ParameterGroup(this, 'ClusterParameterGroup', {
family: parameterGroupFamily,
});
}

if (clusterParameterGroup instanceof ClusterParameterGroup) { // ignore imported ClusterParameterGroup
if (clusterParameterGroup instanceof ParameterGroup) { // ignore imported ClusterParameterGroup
if (s3ImportRole) {
clusterParameterGroup.addParameter('aurora_load_from_s3_role', s3ImportRole.roleArn);
}
Expand All @@ -437,6 +437,7 @@ export class DatabaseCluster extends DatabaseClusterBase {
}
}
}
const clusterParameterGroupConfig = clusterParameterGroup?.bindToCluster({});

const cluster = new CfnDBCluster(this, 'Resource', {
// Basic
Expand All @@ -446,7 +447,7 @@ export class DatabaseCluster extends DatabaseClusterBase {
dbSubnetGroupName: subnetGroup.ref,
vpcSecurityGroupIds: securityGroups.map(sg => sg.securityGroupId),
port: props.port,
dbClusterParameterGroupName: clusterParameterGroup && clusterParameterGroup.parameterGroupName,
dbClusterParameterGroupName: clusterParameterGroupConfig?.parameterGroupName,
associatedRoles: clusterAssociatedRoles.length > 0 ? clusterAssociatedRoles : undefined,
// Admin
masterUsername: secret ? secret.secretValueFromJson('username').toString() : props.masterUser.username,
Expand Down Expand Up @@ -504,6 +505,7 @@ export class DatabaseCluster extends DatabaseClusterBase {
});
}

const instanceParameterGroupConfig = props.instanceProps.parameterGroup?.bindToInstance({});
for (let i = 0; i < instanceCount; i++) {
const instanceIndex = i + 1;

Expand All @@ -524,7 +526,7 @@ export class DatabaseCluster extends DatabaseClusterBase {
publiclyAccessible,
// This is already set on the Cluster. Unclear to me whether it should be repeated or not. Better yes.
dbSubnetGroupName: subnetGroup.ref,
dbParameterGroupName: props.instanceProps.parameterGroup && props.instanceProps.parameterGroup.parameterGroupName,
dbParameterGroupName: instanceParameterGroupConfig?.parameterGroupName,
monitoringInterval: props.monitoringInterval && props.monitoringInterval.toSeconds(),
monitoringRoleArn: monitoringRole && monitoringRole.roleArn,
});
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-rds/lib/instance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -769,12 +769,13 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
throw new Error(`timezone property can be configured only for Microsoft SQL Server, not ${props.engine.name}`);
}

const instanceParameterGroupConfig = props.parameterGroup?.bindToInstance({});
this.sourceCfnProps = {
...this.newCfnProps,
allocatedStorage: props.allocatedStorage ? props.allocatedStorage.toString() : '100',
allowMajorVersionUpgrade: props.allowMajorVersionUpgrade,
dbName: props.databaseName,
dbParameterGroupName: props.parameterGroup && props.parameterGroup.parameterGroupName,
dbParameterGroupName: instanceParameterGroupConfig?.parameterGroupName,
engine: props.engine.name,
engineVersion: props.engineVersion,
licenseModel: props.licenseModel,
Expand Down
167 changes: 96 additions & 71 deletions packages/@aws-cdk/aws-rds/lib/parameter-group.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,53 +2,50 @@ import { Construct, IResource, Lazy, Resource } from '@aws-cdk/core';
import { CfnDBClusterParameterGroup, CfnDBParameterGroup } from './rds.generated';

/**
* A parameter group
* Options for {@link IParameterGroup.bindToCluster}.
* Empty for now, but can be extended later.
*/
export interface IParameterGroup extends IResource {
/**
* The name of this parameter group
*
* @attribute
*/
export interface ParameterGroupClusterBindOptions {
}

/**
* The type returned from {@link IParameterGroup.bindToCluster}.
*/
export interface ParameterGroupClusterConfig {
/** The name of this parameter group. */
readonly parameterGroupName: string;
}

/**
* A new cluster or instance parameter group
* Options for {@link IParameterGroup.bindToInstance}.
* Empty for now, but can be extended later.
*/
abstract class ParameterGroupBase extends Resource implements IParameterGroup {
/**
* Imports a parameter group
*/
public static fromParameterGroupName(scope: Construct, id: string, parameterGroupName: string): IParameterGroup {
class Import extends Resource implements IParameterGroup {
public readonly parameterGroupName = parameterGroupName;
}
return new Import(scope, id);
}
export interface ParameterGroupInstanceBindOptions {
}

/**
* The name of the parameter group
*/
public abstract readonly parameterGroupName: string;
/**
* The type returned from {@link IParameterGroup.bindToInstance}.
*/
export interface ParameterGroupInstanceConfig {
/** The name of this parameter group. */
readonly parameterGroupName: string;
}

/**
* A parameter group.
* Represents both a cluster parameter group,
* and an instance parameter group.
*/
export interface IParameterGroup extends IResource {
/**
* Parameters of the parameter group
* Method called when this Parameter Group is used when defining a database cluster.
*/
protected parameters?: { [key: string]: string } = {};
bindToCluster(options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig;

/**
* Add a parameter to this parameter group
*
* @param key The key of the parameter to be added
* @param value The value of the parameter to be added
* Method called when this Parameter Group is used when defining a database instance.
*/
public addParameter(key: string, value: string) {
if (!this.parameters) {
this.parameters = {};
}
this.parameters[key] = value;
}
bindToInstance(options: ParameterGroupInstanceBindOptions): ParameterGroupInstanceConfig;
}

/**
Expand Down Expand Up @@ -76,60 +73,88 @@ export interface ParameterGroupProps {
}

/**
* A parameter group
* A parameter group.
* Represents both a cluster parameter group,
* and an instance parameter group.
*
* @resource AWS::RDS::DBParameterGroup
*/
export class ParameterGroup extends ParameterGroupBase {
export class ParameterGroup extends Resource implements IParameterGroup {
/**
* The name of the parameter group
* Imports a parameter group
*/
public readonly parameterGroupName: string;

constructor(scope: Construct, id: string, props: ParameterGroupProps) {
super(scope, id);
public static fromParameterGroupName(scope: Construct, id: string, parameterGroupName: string): IParameterGroup {
class Import extends Resource implements IParameterGroup {
public readonly parameterGroupName = parameterGroupName;

this.parameters = props.parameters ? props.parameters : {};
public bindToCluster(_options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig {
return { parameterGroupName };
}

const resource = new CfnDBParameterGroup(this, 'Resource', {
description: props.description || `Parameter group for ${props.family}`,
family: props.family,
parameters: Lazy.anyValue({ produce: () => this.parameters }),
});
public bindToInstance(_options: ParameterGroupInstanceBindOptions): ParameterGroupInstanceConfig {
return { parameterGroupName };
}
}

this.parameterGroupName = resource.ref;
return new Import(scope, id);
}
}

/**
* Construction properties for a ClusterParameterGroup
*/
// tslint:disable-next-line:no-empty-interface
export interface ClusterParameterGroupProps extends ParameterGroupProps {

}
/**
* A cluster parameter group
*
* @resource AWS::RDS::DBClusterParameterGroup
*/
export class ClusterParameterGroup extends ParameterGroupBase {
/**
* The name of the parameter group
* Parameters of the parameter group
*/
public readonly parameterGroupName: string;
protected parameters?: { [key: string]: string } = {};
private readonly family: string;
private readonly description?: string;

constructor(scope: Construct, id: string, props: ClusterParameterGroupProps) {
private clusterCfnGroup?: CfnDBClusterParameterGroup;
private instanceCfnGroup?: CfnDBParameterGroup;

constructor(scope: Construct, id: string, props: ParameterGroupProps) {
super(scope, id);

this.family = props.family;
this.description = props.description;
this.parameters = props.parameters ? props.parameters : {};
}

public bindToCluster(_options: ParameterGroupClusterBindOptions): ParameterGroupClusterConfig {
if (!this.clusterCfnGroup) {
const parentScope = this.instanceCfnGroup ?? this;
this.clusterCfnGroup = new CfnDBClusterParameterGroup(parentScope, 'Resource', {
description: this.description || `Cluster parameter group for ${this.family}`,
family: this.family,
parameters: Lazy.anyValue({ produce: () => this.parameters }),
});
}
return {
parameterGroupName: this.clusterCfnGroup.ref,
};
}

const resource = new CfnDBClusterParameterGroup(this, 'Resource', {
description: props.description || `Cluster parameter group for ${props.family}`,
family: props.family,
parameters: Lazy.anyValue({ produce: () => this.parameters }),
});
public bindToInstance(_options: ParameterGroupInstanceBindOptions): ParameterGroupInstanceConfig {
if (!this.instanceCfnGroup) {
const parentScope = this.clusterCfnGroup ?? this;
this.instanceCfnGroup = new CfnDBParameterGroup(parentScope, 'Resource', {
description: this.description || `Parameter group for ${this.family}`,
family: this.family,
parameters: Lazy.anyValue({ produce: () => this.parameters }),
});
}
return {
parameterGroupName: this.instanceCfnGroup.ref,
};
}

this.parameterGroupName = resource.ref;
/**
* Add a parameter to this parameter group
*
* @param key The key of the parameter to be added
* @param value The value of the parameter to be added
*/
public addParameter(key: string, value: string) {
if (!this.parameters) {
this.parameters = {};
}
this.parameters[key] = value;
}
}
5 changes: 2 additions & 3 deletions packages/@aws-cdk/aws-rds/test/integ.cluster.ts
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
import * as ec2 from '@aws-cdk/aws-ec2';
import * as kms from '@aws-cdk/aws-kms';
import * as cdk from '@aws-cdk/core';
import { DatabaseCluster, DatabaseClusterEngine } from '../lib';
import { ClusterParameterGroup } from '../lib/parameter-group';
import { DatabaseCluster, DatabaseClusterEngine, ParameterGroup } from '../lib';

const app = new cdk.App();
const stack = new cdk.Stack(app, 'aws-cdk-rds-integ');

const vpc = new ec2.Vpc(stack, 'VPC', { maxAzs: 2 });

const params = new ClusterParameterGroup(stack, 'Params', {
const params = new ParameterGroup(stack, 'Params', {
family: 'aurora5.6',
description: 'A nice parameter group',
parameters: {
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-rds/test/test.cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import * as kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
import * as cdk from '@aws-cdk/core';
import { Test } from 'nodeunit';
import { ClusterParameterGroup, DatabaseCluster, DatabaseClusterEngine, ParameterGroup } from '../lib';
import { DatabaseCluster, DatabaseClusterEngine, ParameterGroup } from '../lib';

export = {
'creating a Cluster also creates 2 DB Instances'(test: Test) {
Expand Down Expand Up @@ -120,7 +120,7 @@ export = {
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
const group = new ClusterParameterGroup(stack, 'Params', {
const group = new ParameterGroup(stack, 'Params', {
family: 'hello',
description: 'bye',
parameters: {
Expand Down Expand Up @@ -886,7 +886,7 @@ export = {
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

const parameterGroup = new ClusterParameterGroup(stack, 'ParameterGroup', {
const parameterGroup = new ParameterGroup(stack, 'ParameterGroup', {
family: 'family',
parameters: {
key: 'value',
Expand Down
Loading

0 comments on commit 40c3e40

Please sign in to comment.