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.

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

feat(rds): Add InstanceParameterGroup #2075

Closed
wants to merge 8 commits into from
3 changes: 3 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,8 @@ export class DatabaseCluster extends DatabaseClusterBase implements IDatabaseClu

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

const instanceParameterGroupName = props.instanceProps.parameterGroup && props.instanceProps.parameterGroup.parameterGroupName;

const instance = new CfnDBInstance(this, `Instance${instanceIndex}`, {
// Link to cluster
engine: props.engine,
Expand All @@ -301,6 +303,7 @@ export class DatabaseCluster extends DatabaseClusterBase implements IDatabaseClu
publiclyAccessible,
// This is already set on the Cluster. Unclear to me whether it should be repeated or not. Better yes.
dbSubnetGroupName: subnetGroup.ref,
dbParameterGroupName: instanceParameterGroupName,
});

// We must have a dependency on the NAT gateway provider here to create
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-rds/lib/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ export * from './props';
export * from './cluster-parameter-group';
export * from './rotation-single-user';
export * from './database-secret';
export * from './instance-parameter-group';

// AWS::RDS CloudFormation Resources:
export * from './rds.generated';
134 changes: 134 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/instance-parameter-group.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
import { CfnOutput, Construct, IConstruct, Token } from '@aws-cdk/cdk';
import { Parameters } from './props';
import { CfnDBParameterGroup } from './rds.generated';

/**
* A instance parameter group
*/
export interface IInstanceParameterGroup extends IConstruct {
/**
* Name of this parameter group
*/
readonly parameterGroupName: string;

/**
* Export this parameter group
*/
export(): InstanceParameterGroupImportProps;
}

/**
* Properties to reference a instance parameter group
*/
export interface InstanceParameterGroupImportProps {
readonly parameterGroupName: string;
}

/**
* Properties for a instance parameter group
*/
export interface InstanceParameterGroupProps {
/**
* Database family of this parameter group
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there more detail? Perhaps a link to some documentation describing what a family is?

Copy link
Author

Choose a reason for hiding this comment

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

fixed 8435164

*
* @see https://docs.aws.amazon.com/AmazonRDS/latest/APIReference/API_CreateDBParameterGroup.html#API_CreateDBParameterGroup_RequestParameters
* @example
* { family: 'MySQL5.5' }
*/
family: string;

/**
* Description for this parameter group
*/
description: string;
sam-goodwin marked this conversation as resolved.
Show resolved Hide resolved

/**
* The parameters in this parameter group
*/
parameters: Parameters;
}

/**
* Defina a instance parameter group
*/
export class InstanceParameterGroup extends Construct implements IInstanceParameterGroup {
/**
* Import a parameter group
*/
public static import(scope: Construct, id: string, props: InstanceParameterGroupImportProps): IInstanceParameterGroup {
return new ImportedInstanceParameterGroup(scope, id, props);
}

public readonly parameterGroupName: string;
private readonly parameters: Parameters = {};

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

const { description, family, parameters } = props;

const resource = new CfnDBParameterGroup(this, 'Resource', {
description,
family,
parameters: new Token(() => this.parameters)
});

for (const [key, value] of Object.entries(parameters || {})) {
this.setParameter(key, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you worried about using the passed in parameters object? Because you could do the following instead:

this.parameters = props.parameters || {};

}

this.parameterGroupName = resource.dbParameterGroupName;
}

/**
* Export this parameter group
*/
public export(): InstanceParameterGroupImportProps {
return {
parameterGroupName: new CfnOutput(this, 'ParameterGroupName', { value: this.parameterGroupName }).makeImportValue().toString(),
};
}

/**
* Set a single parameter in this parameter group
*/
public setParameter(key: string, value: string | undefined) {
if (value === undefined && key in this.parameters) {
delete this.parameters[key];
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't overload the setParameter function with delete behavior. We don't support union types in public APIs because they don't translate well to other languages like Java, and it is also a confusing API to call set to delete something.

setParameter is for setting parameters and removeParameter is for removing them, keep it simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sam-goodwin I think this PR is based on the existing implementation of the cluster parameter group (https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-rds/lib/cluster-parameter-group.ts implemented here #729) which proposes the same approach.

Does you comment mean that the cluster parameter group should also be refactored?

I'm currently working on a PR to support db instances and would like to have your opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

On a second pass, I realize my comment about this union type was incorrect - string | undefined will work fine because it means optional. My mistake.

That said, it does seem confusing to me that we use setParameter to delete a parameter. I wouldn't expect to (for example) use map.put(key, null) to delete a key from a Map in Java. I was unaware of the precedent set in cluster parameter groups. What is the use case for this overloaded behavior?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know, it seems to come from this discussion #729 (comment).

For me it doesn't make sense to have setParameter and removeParameter methods, parameters should be defined directly in the construction props.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. @rix0rrr can you chime in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might make sense to build the RDS parameters object incrementally, since it can be frking huge (there are many parameters to set!), so I think I might want some function to do:

lessDurabilityMorePerformance(paramGroup);

Or something. But I don't feel super strongly about this, I'm okay with removing the mutators as well.

}
if (value !== undefined) {
this.parameters[key] = value;
}
}

/**
* Remove a previously-set parameter from this parameter group
*/
public removeParameter(key: string) {
this.setParameter(key, undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

... do the delete work here.

}

/**
* Validate this construct
*/
protected validate(): string[] {
if (Object.keys(this.parameters).length === 0) {
return ['At least one parameter required, call setParameter().'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Add unit tests for this case.

Copy link
Author

Choose a reason for hiding this comment

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

fixed 9c77fbf

}
return [];
}
}

class ImportedInstanceParameterGroup extends Construct implements IInstanceParameterGroup {
public readonly parameterGroupName: string;

constructor(scope: Construct, id: string, private readonly props: InstanceParameterGroupImportProps) {
super(scope, id);

this.parameterGroupName = props.parameterGroupName;
}

public export() {
return this.props;
}
}
8 changes: 8 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/props.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import ec2 = require('@aws-cdk/aws-ec2');
import kms = require('@aws-cdk/aws-kms');
import { IInstanceParameterGroup } from './instance-parameter-group';

/**
* The engine for the database cluster
Expand Down Expand Up @@ -36,6 +37,13 @@ export interface InstanceProps {
* Security group. If not specified a new one will be created.
*/
securityGroup?: ec2.ISecurityGroup;

/**
* Additional parameters to pass to the database instance
*
* @default No parameter group
*/
parameterGroup?: IInstanceParameterGroup;
}

/**
Expand Down
75 changes: 73 additions & 2 deletions packages/@aws-cdk/aws-rds/test/test.cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { expect, haveResource } from '@aws-cdk/assert';
import ec2 = require('@aws-cdk/aws-ec2');
import cdk = require('@aws-cdk/cdk');
import { Test } from 'nodeunit';
import { ClusterParameterGroup, DatabaseCluster, DatabaseClusterEngine } from '../lib';
import { ClusterParameterGroup, DatabaseCluster, DatabaseClusterEngine, InstanceParameterGroup } from '../lib';

export = {
'check that instantiation works'(test: Test) {
Expand Down Expand Up @@ -160,6 +160,44 @@ export = {
test.done();
},

'cluster with instance parameter group'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.VpcNetwork(stack, 'VPC');

// WHEN
const instanceParameterGroup = new InstanceParameterGroup(stack, 'Params', {
family: 'mysql',
description: 'bye',
parameters: {},
});
instanceParameterGroup.setParameter('param1', 'value1');
instanceParameterGroup.setParameter('param2', 'value2');
instanceParameterGroup.removeParameter('param2');
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.Aurora,
masterUser: {
username: 'admin',
password: 'tooshort',
},
instances: 1,
instanceProps: {
instanceType: new ec2.InstanceTypePair(ec2.InstanceClass.Burstable2, ec2.InstanceSize.Small),
parameterGroup: instanceParameterGroup,
vpc
},
});

// THEN
expect(stack).to(haveResource('AWS::RDS::DBInstance', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you also verify the value of the parameter instance group?

DBParameterGroupName: {
Ref: 'ParamsA8366201',
},
}));

test.done();
},

'import/export cluster parameter group'(test: Test) {
// GIVEN
const stack = testStack();
Expand All @@ -178,6 +216,25 @@ export = {
test.done();
},

'import/export instance parameter group'(test: Test) {
// GIVEN
const stack = testStack();
const group = new InstanceParameterGroup(stack, 'Params', {
family: 'hello',
description: 'desc',
parameters: {},
});

// WHEN
const exported = group.export();
const imported = InstanceParameterGroup.import(stack, 'ImportParams', exported);

// THEN
test.deepEqual(stack.node.resolve(exported), { parameterGroupName: { 'Fn::ImportValue': 'Stack:ParamsParameterGroupNameA6B808D7' } });
test.deepEqual(stack.node.resolve(imported.parameterGroupName), { 'Fn::ImportValue': 'Stack:ParamsParameterGroupNameA6B808D7' });
Copy link
Contributor

Choose a reason for hiding this comment

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

Validate the stacks. expect(stack).to(haveResource(..)).

test.done();
},

'creates a secret when master credentials are not specified'(test: Test) {
// GIVEN
const stack = testStack();
Expand Down Expand Up @@ -233,7 +290,21 @@ export = {
}));

test.done();
}
},

'empty instance parameter group is not valid'(test: Test) {
const stack = testStack();
new InstanceParameterGroup(stack, 'Params', {
family: 'hello',
description: 'desc',
parameters: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should allow you to create an invalid instance in the first place.

});

const errorMessages = stack.node.validateTree().map(e => e.message);
test.deepEqual(errorMessages, ['At least one parameter required, call setParameter().']);

test.done();
},
};

function testStack() {
Expand Down