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 secret rotation to DatabaseClusterFromSnapshot #20020

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
832cd10
feat: add secret rotation to DatabaseClusterFromSnapshot
davenix-palmetto Apr 21, 2022
149a234
Merge branch 'master' into feat(rds)/add-secret-rotation-to-cluster-f…
davenix-palmetto Apr 21, 2022
6343c00
Merge branch 'master' into feat(rds)/add-secret-rotation-to-cluster-f…
davenix-palmetto Apr 22, 2022
087be25
Merge branch 'master' into feat(rds)/add-secret-rotation-to-cluster-f…
davenix-palmetto Apr 25, 2022
8f5e4fc
refactor: pull duplicative code to parent class
davenix-palmetto Apr 26, 2022
48a121f
fix: attach secret properly
davenix-palmetto Apr 27, 2022
12f43eb
docs: add required docstrings
davenix-palmetto Apr 27, 2022
935a85a
tests: add multi user test
davenix-palmetto Apr 28, 2022
cb98dd4
tests: add error thrown
davenix-palmetto Apr 28, 2022
ca54a07
Merge pull request #2 from davenix-palmetto/trying-to-refactor
davenix-palmetto Apr 28, 2022
14eb140
fix: merge upstream appropriately, resolve conflicts
davenix-palmetto Apr 28, 2022
0d06cdd
Merge branch 'master' into feat(rds)/add-secret-rotation-to-cluster-f…
davenix-palmetto Apr 28, 2022
404aa1b
Merge branch 'master' into feat(rds)/add-secret-rotation-to-cluster-f…
davenix-palmetto Apr 28, 2022
b71b43b
Update packages/@aws-cdk/aws-rds/lib/cluster.ts
davenix-palmetto Apr 28, 2022
89ebe6c
Update packages/@aws-cdk/aws-rds/lib/cluster.ts
davenix-palmetto Apr 28, 2022
0a39384
style: fix line spacings
davenix-palmetto Apr 28, 2022
2c20551
docs: bring back comments
davenix-palmetto Apr 28, 2022
f50ed87
refactor: bring credentials back to child interface
davenix-palmetto Apr 28, 2022
be05028
Merge branch 'master' into feat(rds)/add-secret-rotation-to-cluster-f…
davenix-palmetto Apr 29, 2022
324166b
tests: fix test pattern
davenix-palmetto Apr 29, 2022
a9fb890
Merge branch 'master' into feat(rds)/add-secret-rotation-to-cluster-f…
mergify[bot] Apr 30, 2022
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
153 changes: 96 additions & 57 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ export abstract class DatabaseClusterBase extends Resource implements IDatabaseC
* Identifier of the cluster
*/
public abstract readonly clusterIdentifier: string;

/**
* Identifiers of the replicas
*/
Expand Down Expand Up @@ -345,9 +346,40 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
protected readonly securityGroups: ec2.ISecurityGroup[];
protected readonly subnetGroup: ISubnetGroup;

/**
davenix-palmetto marked this conversation as resolved.
Show resolved Hide resolved
* Secret in SecretsManager to store the database cluster user credentials.
*/
public abstract readonly secret?: secretsmanager.ISecret;

/**
* The VPC network to place the cluster in.
*/
public readonly vpc: ec2.IVpc;

/**
* The cluster's subnets.
*/
public readonly vpcSubnets?: ec2.SubnetSelection;

/**
* Application for single user rotation of the master password to this cluster.
*/
public readonly singleUserRotationApplication: secretsmanager.SecretRotationApplication;

/**
* Application for multi user rotation to this cluster.
*/
public readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication;

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

this.vpc = props.instanceProps.vpc;
this.vpcSubnets = props.instanceProps.vpcSubnets;

this.singleUserRotationApplication = props.engine.singleUserRotationApplication;
this.multiUserRotationApplication = props.engine.multiUserRotationApplication;

const { subnetIds } = props.instanceProps.vpc.selectSubnets(props.instanceProps.vpcSubnets);

// Cannot test whether the subnets are in different AZs, but at least we can test the amount.
Expand Down Expand Up @@ -436,6 +468,47 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
copyTagsToSnapshot: props.copyTagsToSnapshot ?? true,
};
}

/**
* Adds the single user rotation of the master password to this cluster.
*/
public addRotationSingleUser(options: RotationSingleUserOptions = {}): secretsmanager.SecretRotation {
if (!this.secret) {
throw new Error('Cannot add a single user rotation for a cluster without a secret.');
}

const id = 'RotationSingleUser';
const existing = this.node.tryFindChild(id);
if (existing) {
throw new Error('A single user rotation was already added to this cluster.');
}

return new secretsmanager.SecretRotation(this, id, {
...applyDefaultRotationOptions(options, this.vpcSubnets),
secret: this.secret,
application: this.singleUserRotationApplication,
vpc: this.vpc,
target: this,
});
}

/**
* Adds the multi user rotation to this cluster.
*/
public addRotationMultiUser(id: string, options: RotationMultiUserOptions): secretsmanager.SecretRotation {
if (!this.secret) {
throw new Error('Cannot add a multi user rotation for a cluster without a secret.');
}

return new secretsmanager.SecretRotation(this, id, {
...applyDefaultRotationOptions(options, this.vpcSubnets),
secret: options.secret,
masterSecret: this.secret,
application: this.multiUserRotationApplication,
vpc: this.vpc,
target: this,
});
}
}

/**
Expand Down Expand Up @@ -537,21 +610,9 @@ export class DatabaseCluster extends DatabaseClusterNew {
*/
davenix-palmetto marked this conversation as resolved.
Show resolved Hide resolved
public readonly secret?: secretsmanager.ISecret;

private readonly vpc: ec2.IVpc;
private readonly vpcSubnets?: ec2.SubnetSelection;

private readonly singleUserRotationApplication: secretsmanager.SecretRotationApplication;
private readonly multiUserRotationApplication: secretsmanager.SecretRotationApplication;

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

this.vpc = props.instanceProps.vpc;
this.vpcSubnets = props.instanceProps.vpcSubnets;

this.singleUserRotationApplication = props.engine.singleUserRotationApplication;
this.multiUserRotationApplication = props.engine.multiUserRotationApplication;

const credentials = renderCredentials(this, props.engine, props.credentials);
const secret = credentials.secret;

Expand All @@ -564,6 +625,10 @@ export class DatabaseCluster extends DatabaseClusterNew {

this.clusterIdentifier = cluster.ref;

if (secret) {
this.secret = secret.attach(this);
}

// create a number token that represents the port of the cluster
const portAttribute = Token.asNumber(cluster.attrEndpointPort);
this.clusterEndpoint = new Endpoint(cluster.attrEndpointAddress, portAttribute);
Expand All @@ -575,56 +640,11 @@ export class DatabaseCluster extends DatabaseClusterNew {

cluster.applyRemovalPolicy(props.removalPolicy ?? RemovalPolicy.SNAPSHOT);

if (secret) {
this.secret = secret.attach(this);
}

setLogRetention(this, props);
const createdInstances = createInstances(this, props, this.subnetGroup);
this.instanceIdentifiers = createdInstances.instanceIdentifiers;
this.instanceEndpoints = createdInstances.instanceEndpoints;
}

/**
* Adds the single user rotation of the master password to this cluster.
*/
public addRotationSingleUser(options: RotationSingleUserOptions = {}): secretsmanager.SecretRotation {
if (!this.secret) {
throw new Error('Cannot add single user rotation for a cluster without secret.');
}

const id = 'RotationSingleUser';
const existing = this.node.tryFindChild(id);
if (existing) {
throw new Error('A single user rotation was already added to this cluster.');
}

return new secretsmanager.SecretRotation(this, id, {
...applyDefaultRotationOptions(options, this.vpcSubnets),
secret: this.secret,
application: this.singleUserRotationApplication,
vpc: this.vpc,
target: this,
});
}

/**
* Adds the multi user rotation to this cluster.
*/
public addRotationMultiUser(id: string, options: RotationMultiUserOptions): secretsmanager.SecretRotation {
if (!this.secret) {
throw new Error('Cannot add multi user rotation for a cluster without secret.');
}

return new secretsmanager.SecretRotation(this, id, {
...applyDefaultRotationOptions(options, this.vpcSubnets),
secret: options.secret,
masterSecret: this.secret,
application: this.multiUserRotationApplication,
vpc: this.vpc,
target: this,
});
}
}

/**
Expand All @@ -637,6 +657,13 @@ export interface DatabaseClusterFromSnapshotProps extends DatabaseClusterBasePro
* However, you can use only the ARN to specify a DB instance snapshot.
*/
readonly snapshotIdentifier: string;

/**
* Credentials for the administrative user
*
* @default - A username of 'admin' (or 'postgres' for PostgreSQL) and SecretsManager-generated password
*/
readonly credentials?: Credentials;
}

/**
Expand All @@ -652,16 +679,28 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew {
public readonly instanceIdentifiers: string[];
public readonly instanceEndpoints: Endpoint[];

/**
* The secret attached to this cluster
*/
public readonly secret?: secretsmanager.ISecret;

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

const credentials = renderCredentials(this, props.engine, props.credentials);
davenix-palmetto marked this conversation as resolved.
Show resolved Hide resolved
const secret = credentials.secret;

const cluster = new CfnDBCluster(this, 'Resource', {
...this.newCfnProps,
snapshotIdentifier: props.snapshotIdentifier,
});

this.clusterIdentifier = cluster.ref;

if (secret) {
this.secret = secret.attach(this);
}

// create a number token that represents the port of the cluster
const portAttribute = Token.asNumber(cluster.attrEndpointPort);
this.clusterEndpoint = new Endpoint(cluster.attrEndpointAddress, portAttribute);
Expand Down
88 changes: 87 additions & 1 deletion packages/@aws-cdk/aws-rds/test/cluster.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1074,7 +1074,7 @@ describe('cluster', () => {
});

// THEN
expect(() => cluster.addRotationSingleUser()).toThrow(/without secret/);
expect(() => cluster.addRotationSingleUser()).toThrow(/without a secret/);
});

test('throws when trying to add single user rotation multiple times', () => {
Expand Down Expand Up @@ -2049,6 +2049,92 @@ describe('cluster', () => {
});
});

test('create a cluster from a snapshot with single user secret rotation', () => {
davenix-palmetto marked this conversation as resolved.
Show resolved Hide resolved
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

const cluster = new DatabaseClusterFromSnapshot(stack, 'Database', {
engine: DatabaseClusterEngine.aurora({ version: AuroraEngineVersion.VER_1_22_2 }),
instanceProps: {
vpc,
},
snapshotIdentifier: 'mySnapshot',
});

// WHEN
cluster.addRotationSingleUser();

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::SecretsManager::RotationSchedule', {
RotationRules: {
AutomaticallyAfterDays: 30,
},
});
});

davenix-palmetto marked this conversation as resolved.
Show resolved Hide resolved
test('throws when trying to add single user rotation multiple times on cluster from snapshot', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

const cluster = new DatabaseClusterFromSnapshot(stack, 'Database', {
engine: DatabaseClusterEngine.aurora({ version: AuroraEngineVersion.VER_1_22_2 }),
instanceProps: {
vpc,
},
snapshotIdentifier: 'mySnapshot',
});

// WHEN
cluster.addRotationSingleUser();

// THEN
expect(() => cluster.addRotationSingleUser()).toThrow(/A single user rotation was already added to this cluster/);
});

test('create a cluster from a snapshot with multi user secret rotation', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's do an error case for a missing secret on multi user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like right now a secret is required in RotationMultiUserOptions, unlike in RotationSingleUserOptions, so a similar test as in addRotationSingleUser cannot be constructed for addRotationMultiUser. Can you advise a bit further? Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, I was wrong here. Your error cases are fine.

// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

const cluster = new DatabaseClusterFromSnapshot(stack, 'Database', {
engine: DatabaseClusterEngine.aurora({ version: AuroraEngineVersion.VER_1_22_2 }),
instanceProps: {
vpc,
},
snapshotIdentifier: 'mySnapshot',
});

// WHEN
const userSecret = new DatabaseSecret(stack, 'UserSecret', { username: 'user' });
cluster.addRotationMultiUser('user', { secret: userSecret.attach(cluster) });

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::SecretsManager::RotationSchedule', {
SecretId: {
Ref: 'UserSecretAttachment16ACBE6D',
},
RotationLambdaARN: {
'Fn::GetAtt': [
'DatabaseuserECD1FB0C',
'Outputs.RotationLambdaARN',
],
},
RotationRules: {
AutomaticallyAfterDays: 30,
},
});

Template.fromStack(stack).hasResourceProperties('AWS::Serverless::Application', {
Parameters: {
masterSecretArn: {
Ref: 'DatabaseSecretAttachmentE5D1B020',
},
},
});
});

test('reuse an existing subnet group', () => {
// GIVEN
const stack = testStack();
Expand Down