Skip to content

Commit

Permalink
feat(rds): unify ParameterGroup and ClusterParameterGroup (aws#8959)
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 aws#8932

BREAKING CHANGE: the class ClusterParameterGroup has been removed -
  use ParameterGroup instead

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
skinny85 authored Jul 16, 2020
1 parent 846a222 commit 17b690b
Show file tree
Hide file tree
Showing 14 changed files with 259 additions and 186 deletions.
70 changes: 11 additions & 59 deletions packages/@aws-cdk/aws-rds/lib/cluster-engine.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import * as iam from '@aws-cdk/aws-iam';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import * as core from '@aws-cdk/core';
import { ClusterParameterGroup, IParameterGroup, ParameterGroup } from './parameter-group';
import { ParameterGroupFamilyMapping } from './private/parameter-group-family-mapping';
import { compare } from './private/version';
import { IEngine } from './engine';
import { IParameterGroup, ParameterGroup } from './parameter-group';
import { calculateParameterGroupFamily, ParameterGroupFamilyMapping } from './private/parameter-group-family-mapping';

/**
* The extra options passed to the {@link IClusterEngine.bindToCluster} method.
Expand Down Expand Up @@ -54,25 +54,7 @@ export interface ClusterEngineConfig {
/**
* The interface representing a database cluster (as opposed to instance) engine.
*/
export interface IClusterEngine {
/** The type of the engine, for example "aurora-mysql". */
readonly engineType: string;

/**
* The exact version of a given engine.
*
* @default - default version for the given engine type
*/
readonly engineVersion?: string;

/**
* The family to use for ParameterGroups using this engine.
* This is usually equal to "<engineType><engineMajorVersion>",
* but can sometimes be a variation of that.
* You can pass this property when creating new ClusterParameterGroup.
*/
readonly parameterGroupFamily: string;

export interface IClusterEngine extends IEngine {
/** The application used by this engine to perform rotation for a single-user scenario. */
readonly singleUserRotationApplication: secretsmanager.SecretRotationApplication;

Expand Down Expand Up @@ -101,17 +83,19 @@ abstract class ClusterEngineBase implements IClusterEngine {
public readonly singleUserRotationApplication: secretsmanager.SecretRotationApplication;
public readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication;

private readonly parameterGroupFamilies?: ParameterGroupFamilyMapping[];
private readonly defaultPort?: number;

constructor(props: ClusterEngineBaseProps) {
this.engineType = props.engineType;
this.singleUserRotationApplication = props.singleUserRotationApplication;
this.multiUserRotationApplication = props.multiUserRotationApplication;
this.parameterGroupFamilies = props.parameterGroupFamilies;
this.defaultPort = props.defaultPort;
this.engineVersion = props.engineVersion;
this.parameterGroupFamily = this.establishParameterGroupFamily();
const parameterGroupFamily = calculateParameterGroupFamily(props.parameterGroupFamilies, props.engineVersion);
if (parameterGroupFamily === undefined) {
throw new Error(`No parameter group family found for database engine ${this.engineType} with version ${this.engineVersion}.`);
}
this.parameterGroupFamily = parameterGroupFamily;
}

public bindToCluster(scope: core.Construct, options: ClusterEngineBindOptions): ClusterEngineConfig {
Expand All @@ -128,46 +112,14 @@ abstract class ClusterEngineBase implements IClusterEngine {
* if one wasn't provided by the customer explicitly.
*/
protected abstract defaultParameterGroup(scope: core.Construct): IParameterGroup | undefined;

private establishParameterGroupFamily(): string {
const ret = this.calculateParameterGroupFamily();
if (ret === undefined) {
throw new Error(`No parameter group family found for database engine ${this.engineType} with version ${this.engineVersion}.`);
}
return ret;
}

/**
* Get the latest parameter group family for this engine. Latest is determined using semver on the engine major version.
* When `engineVersion` is specified, return the parameter group family corresponding to that engine version.
* Return undefined if no parameter group family is defined for this engine or for the requested `engineVersion`.
*/
private calculateParameterGroupFamily(): string | undefined {
if (this.parameterGroupFamilies === undefined) {
return undefined;
}
const engineVersion = this.engineVersion;
if (engineVersion !== undefined) {
const family = this.parameterGroupFamilies.find(x => engineVersion.startsWith(x.engineMajorVersion));
if (family) {
return family.parameterGroupFamily;
}
} else if (this.parameterGroupFamilies.length > 0) {
const sorted = this.parameterGroupFamilies.slice().sort((a, b) => {
return compare(a.engineMajorVersion, b.engineMajorVersion);
}).reverse();
return sorted[0].parameterGroupFamily;
}
return undefined;
}
}

abstract class MySqlClusterEngineBase extends ClusterEngineBase {
public bindToCluster(scope: core.Construct, options: ClusterEngineBindOptions): ClusterEngineConfig {
const config = super.bindToCluster(scope, options);
const parameterGroup = options.parameterGroup ?? (options.s3ImportRole || options.s3ExportRole
? new ClusterParameterGroup(scope, 'ClusterParameterGroup', {
family: this.parameterGroupFamily,
? new ParameterGroup(scope, 'ClusterParameterGroup', {
engine: this,
})
: config.parameterGroup);
if (options.s3ImportRole) {
Expand Down
6 changes: 4 additions & 2 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,7 @@ export class DatabaseCluster extends DatabaseClusterBase {
parameterGroup: props.parameterGroup,
});
const clusterParameterGroup = props.parameterGroup ?? clusterEngineBindConfig.parameterGroup;
const clusterParameterGroupConfig = clusterParameterGroup?.bindToCluster({});

const cluster = new CfnDBCluster(this, 'Resource', {
// Basic
Expand All @@ -424,7 +425,7 @@ export class DatabaseCluster extends DatabaseClusterBase {
dbSubnetGroupName: subnetGroup.ref,
vpcSecurityGroupIds: securityGroups.map(sg => sg.securityGroupId),
port: props.port ?? clusterEngineBindConfig.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 @@ -483,6 +484,7 @@ 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({});
for (let i = 0; i < instanceCount; i++) {
const instanceIndex = i + 1;

Expand All @@ -503,7 +505,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
26 changes: 26 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/engine.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
/**
* A common interface for database engines.
* Don't implement this interface directly,
* instead implement one of the known sub-interfaces,
* like IClusterEngine and IInstanceEngine.
*/
export interface IEngine {
/** The type of the engine, for example "mysql". */
readonly engineType: string;

/**
* The exact version of the engine that is used,
* for example "5.1.42".
*
* @default - use the default version for this engine type
*/
readonly engineVersion?: string;

/**
* The family to use for ParameterGroups using this engine.
* This is usually equal to "<engineType><engineMajorVersion>",
* but can sometimes be a variation of that.
* You can pass this property when creating new ParameterGroup.
*/
readonly parameterGroupFamily: string;
}
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-rds/lib/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export * from './engine';
export * from './cluster';
export * from './cluster-ref';
export * from './cluster-engine';
Expand Down
23 changes: 10 additions & 13 deletions packages/@aws-cdk/aws-rds/lib/instance-engine.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as ec2 from '@aws-cdk/aws-ec2';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import * as core from '@aws-cdk/core';
import { ParameterGroupFamilyMapping } from './private/parameter-group-family-mapping';
import { IEngine } from './engine';
import { calculateParameterGroupFamily, ParameterGroupFamilyMapping } from './private/parameter-group-family-mapping';

/**
* The options passed to {@link IInstanceEngine.bind}.
Expand All @@ -26,18 +27,7 @@ export interface InstanceEngineConfig {
/**
* Interface representing a database instance (as opposed to cluster) engine.
*/
export interface IInstanceEngine {
/** The type of the engine, for example "mysql". */
readonly engineType: string;

/**
* The exact version of the engine that is used,
* for example "5.1.42".
*
* @default - use the default version for this engine type
*/
readonly engineVersion?: string;

export interface IInstanceEngine extends IEngine {
/** The application used by this engine to perform rotation for a single-user scenario. */
readonly singleUserRotationApplication: secretsmanager.SecretRotationApplication;

Expand All @@ -61,6 +51,7 @@ interface InstanceEngineBaseProps {
abstract class InstanceEngineBase implements IInstanceEngine {
public readonly engineType: string;
public readonly engineVersion?: string;
public readonly parameterGroupFamily: string;
public readonly singleUserRotationApplication: secretsmanager.SecretRotationApplication;
public readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication;

Expand All @@ -69,6 +60,11 @@ abstract class InstanceEngineBase implements IInstanceEngine {
this.singleUserRotationApplication = props.singleUserRotationApplication;
this.multiUserRotationApplication = props.multiUserRotationApplication;
this.engineVersion = props.version;
const parameterGroupFamily = calculateParameterGroupFamily(props.parameterGroupFamilies, props.version);
if (parameterGroupFamily === undefined) {
throw new Error(`No parameter group family found for database engine ${this.engineType} with version ${this.engineVersion}.`);
}
this.parameterGroupFamily = parameterGroupFamily;
}

public bindToInstance(_scope: core.Construct, options: InstanceEngineBindOptions): InstanceEngineConfig {
Expand Down Expand Up @@ -231,6 +227,7 @@ class OracleSe2InstanceEngine extends OracleInstanceEngine {
super({
engineType: 'oracle-se2',
parameterGroupFamilies: [
{ engineMajorVersion: '11.2', parameterGroupFamily: 'oracle-se2-11.2' },
{ engineMajorVersion: '12.1', parameterGroupFamily: 'oracle-se2-12.1' },
{ engineMajorVersion: '12.2', parameterGroupFamily: 'oracle-se2-12.2' },
{ engineMajorVersion: '18', parameterGroupFamily: 'oracle-se2-18' },
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-rds/lib/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -678,12 +678,13 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
props.engine.bindToInstance(this, props);
this.instanceType = props.instanceType ?? ec2.InstanceType.of(ec2.InstanceClass.M5, ec2.InstanceSize.LARGE);

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.engineType,
engineVersion: props.engine.engineVersion,
licenseModel: props.licenseModel,
Expand Down
Loading

0 comments on commit 17b690b

Please sign in to comment.