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): retain cluster and instances on deletion and replacement #2063

Merged
merged 4 commits into from
Mar 26, 2019
Merged

feat(rds): retain cluster and instances on deletion and replacement #2063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6ea3ede
feat(rds): enable storage encryption by default
jogold Mar 20, 2019
ba7282a
Remove default encryption
jogold Mar 20, 2019
2c19587
Merge branch 'master' into rds-kms
jogold Mar 22, 2019
4920f07
Add DeletionPolicy and UpdateReplacePolicy
jogold Mar 22, 2019
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
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-rds/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ When the master password is generated and stored in AWS Secrets Manager, it can

Rotation of the master password is also supported for an existing cluster:
```ts
new rds.RotationSingleUser(stack, 'Rotation', {
new RotationSingleUser(stack, 'Rotation', {
secret: importedSecret,
engine: DatabaseEngine.Oracle,
target: importedCluster,
Expand Down
34 changes: 30 additions & 4 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import ec2 = require('@aws-cdk/aws-ec2');
import kms = require('@aws-cdk/aws-kms');
import secretsmanager = require('@aws-cdk/aws-secretsmanager');
import cdk = require('@aws-cdk/cdk');
import { IClusterParameterGroup } from './cluster-parameter-group';
Expand Down Expand Up @@ -72,9 +73,19 @@ export interface DatabaseClusterProps {
defaultDatabaseName?: string;

/**
* ARN of KMS key if you want to enable storage encryption
* Whether to enable storage encryption
*
* @default false
*/
storageEncrypted?: boolean
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 we have an issue with this default due to implicit costs to customers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even when using the default master key? The cost is rather small compared to the cost of RDS. Isn't the CDK supposed to enforce best (security) practices?

But OK, I see that the same philosophy has been adopted for S3 buckets. You want me to change this?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also not sure I'm comfortable with replacing everyone's RDS instances. As far as I know, that will destroy their data, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

If you change the default I'm all for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also not sure I'm comfortable with replacing everyone's RDS instances. As far as I know, that will destroy their data, right?

Yes, and the solution lies in here https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-attribute-updatereplacepolicy.html for resources such as database instances

Copy link
Contributor

Choose a reason for hiding this comment

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

My experience with RDS is limited, but is it possible to replace the database without loss of data? It doesn't look so, right?

I'm MAYBE willing to accept loss of availabillity (even though that's not great either), by means of a Delete-to-Snapshot and then Restore-from-Snapshot, but given that CloudFormation will do the CREATE before the DELETE, doesn't seem like we can sequence those correctly in one deployment.

Copy link
Contributor

Choose a reason for hiding this comment

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

While you're in here, would you mind adding a property to control DeletionPoliycy and UpdateReplacePolicy? I'm horrified to see those aren't being set yet.

I think one property to control both policies should be fine, and it should default to Retain.


/**
* The KMS key for storage encryption. If specified `storageEncrypted`
* will be set to `true`.
*
* @default default master key
*/
kmsKeyArn?: string;
kmsKey?: kms.IEncryptionKey;

/**
* A daily time range in 24-hours UTC format in which backups preferably execute.
Expand All @@ -91,6 +102,14 @@ export interface DatabaseClusterProps {
* @default No parameter group
*/
parameterGroup?: IClusterParameterGroup;

/**
* The CloudFormation policy to apply when the cluster and its instances
* are removed from the stack or replaced during an update.
*
* @default Retain
*/
deleteReplacePolicy?: cdk.DeletionPolicy
}

/**
Expand Down Expand Up @@ -261,10 +280,14 @@ export class DatabaseCluster extends DatabaseClusterBase implements IDatabaseClu
preferredMaintenanceWindow: props.preferredMaintenanceWindow,
databaseName: props.defaultDatabaseName,
// Encryption
kmsKeyId: props.kmsKeyArn,
storageEncrypted: props.kmsKeyArn ? true : false,
kmsKeyId: props.kmsKey && props.kmsKey.keyArn,
storageEncrypted: props.kmsKey ? true : props.storageEncrypted
});

const deleteReplacePolicy = props.deleteReplacePolicy || cdk.DeletionPolicy.Retain;
cluster.options.deletionPolicy = deleteReplacePolicy;
cluster.options.updateReplacePolicy = deleteReplacePolicy;

this.clusterIdentifier = cluster.ref;
this.clusterEndpoint = new Endpoint(cluster.dbClusterEndpointAddress, cluster.dbClusterEndpointPort);
this.readerEndpoint = new Endpoint(cluster.dbClusterReadEndpointAddress, cluster.dbClusterEndpointPort);
Expand Down Expand Up @@ -303,6 +326,9 @@ export class DatabaseCluster extends DatabaseClusterBase implements IDatabaseClu
dbSubnetGroupName: subnetGroup.ref,
});

instance.options.deletionPolicy = deleteReplacePolicy;
instance.options.updateReplacePolicy = deleteReplacePolicy;

// We must have a dependency on the NAT gateway provider here to create
// things in the right order.
instance.node.addDependency(internetConnected);
Expand Down
13 changes: 9 additions & 4 deletions packages/@aws-cdk/aws-rds/test/integ.cluster-rotation.lit.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,6 @@
]
]
},
"StorageEncrypted": false,
"VpcSecurityGroupIds": [
{
"Fn::GetAtt": [
Expand All @@ -659,7 +658,9 @@
]
}
]
}
},
"DeletionPolicy": "Retain",
"UpdateReplacePolicy": "Retain"
},
"DatabaseInstance1844F58FD": {
"Type": "AWS::RDS::DBInstance",
Expand All @@ -677,7 +678,9 @@
"VPCPrivateSubnet1DefaultRouteAE1D6490",
"VPCPrivateSubnet2DefaultRouteF4F5CFD2",
"VPCPrivateSubnet3DefaultRoute27F311AE"
]
],
"DeletionPolicy": "Retain",
"UpdateReplacePolicy": "Retain"
},
"DatabaseInstance2AA380DEE": {
"Type": "AWS::RDS::DBInstance",
Expand All @@ -695,7 +698,9 @@
"VPCPrivateSubnet1DefaultRouteAE1D6490",
"VPCPrivateSubnet2DefaultRouteF4F5CFD2",
"VPCPrivateSubnet3DefaultRoute27F311AE"
]
],
"DeletionPolicy": "Retain",
"UpdateReplacePolicy": "Retain"
},
"DatabaseRotationSecurityGroup17736B63": {
"Type": "AWS::EC2::SecurityGroup",
Expand Down
14 changes: 10 additions & 4 deletions packages/@aws-cdk/aws-rds/test/integ.cluster.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,9 @@
]
}
]
}
},
"DeletionPolicy": "Retain",
"UpdateReplacePolicy": "Retain"
},
"DatabaseInstance1844F58FD": {
"Type": "AWS::RDS::DBInstance",
Expand All @@ -502,7 +504,9 @@
"DependsOn": [
"VPCPublicSubnet1DefaultRoute91CEF279",
"VPCPublicSubnet2DefaultRouteB7481BBA"
]
],
"DeletionPolicy": "Retain",
"UpdateReplacePolicy": "Retain"
},
"DatabaseInstance2AA380DEE": {
"Type": "AWS::RDS::DBInstance",
Expand All @@ -520,7 +524,9 @@
"DependsOn": [
"VPCPublicSubnet1DefaultRoute91CEF279",
"VPCPublicSubnet2DefaultRouteB7481BBA"
]
],
"DeletionPolicy": "Retain",
"UpdateReplacePolicy": "Retain"
}
}
}
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-rds/test/integ.cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ const cluster = new DatabaseCluster(stack, 'Database', {
vpc
},
parameterGroup: params,
kmsKeyArn: kmsKey.keyArn,
kmsKey,
});

cluster.connections.allowDefaultPortFromAnyIpv4('Open to the world');
Expand Down
55 changes: 48 additions & 7 deletions packages/@aws-cdk/aws-rds/test/test.cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { expect, haveResource } from '@aws-cdk/assert';
import { expect, haveResource, ResourcePart } from '@aws-cdk/assert';
import ec2 = require('@aws-cdk/aws-ec2');
import kms = require('@aws-cdk/aws-kms');
import cdk = require('@aws-cdk/cdk');
import { Test } from 'nodeunit';
import { ClusterParameterGroup, DatabaseCluster, DatabaseClusterEngine } from '../lib';
Expand All @@ -25,12 +26,21 @@ export = {

// THEN
expect(stack).to(haveResource('AWS::RDS::DBCluster', {
Engine: "aurora",
DBSubnetGroupName: { Ref: "DatabaseSubnets56F17B9A" },
MasterUsername: "admin",
MasterUserPassword: "tooshort",
VpcSecurityGroupIds: [ {"Fn::GetAtt": ["DatabaseSecurityGroup5C91FDCB", "GroupId"]}]
}));
Properties: {
Engine: "aurora",
DBSubnetGroupName: { Ref: "DatabaseSubnets56F17B9A" },
MasterUsername: "admin",
MasterUserPassword: "tooshort",
VpcSecurityGroupIds: [ {"Fn::GetAtt": ["DatabaseSecurityGroup5C91FDCB", "GroupId"]}]
},
DeletionPolicy: 'Retain',
UpdateReplacePolicy: 'Retain'
}, ResourcePart.CompleteDefinition));

expect(stack).to(haveResource('AWS::RDS::DBInstance', {
DeletionPolicy: 'Retain',
UpdateReplacePolicy: 'Retain'
}, ResourcePart.CompleteDefinition));

test.done();
},
Expand Down Expand Up @@ -232,6 +242,37 @@ export = {
}
}));

test.done();
},

'create an encrypted cluster with custom KMS key'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.VpcNetwork(stack, 'VPC');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AuroraMysql,
masterUser: {
username: 'admin'
},
instanceProps: {
instanceType: new ec2.InstanceTypePair(ec2.InstanceClass.Burstable2, ec2.InstanceSize.Small),
vpc
},
kmsKey: new kms.EncryptionKey(stack, 'Key')
});

// THEN
expect(stack).to(haveResource('AWS::RDS::DBCluster', {
KmsKeyId: {
'Fn::GetAtt': [
'Key961B73FD',
'Arn'
]
}
}));

test.done();
}
};
Expand Down