Skip to content

Commit

Permalink
fix(rds): monitoring role is not created by default when using reader…
Browse files Browse the repository at this point in the history
…s and writers (#26006)

The [`_createInstances`](https://github.com/aws/aws-cdk/blob/4c9016a264c2fec9c0e0e3fae1d7c4216c964b31/packages/aws-cdk-lib/aws-rds/lib/cluster.ts#L635) function was not providing a default `monitoringRole` value with enabled monitoring.
This fix creates a default role as done by the [legacy code](https://github.com/aws/aws-cdk/blob/4c9016a264c2fec9c0e0e3fae1d7c4216c964b31/packages/aws-cdk-lib/aws-rds/lib/cluster.ts#L1228).

Closes #25941.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
lpizzinidev committed Jun 30, 2023
1 parent 7036c27 commit 9065b25
Show file tree
Hide file tree
Showing 2 changed files with 69 additions and 6 deletions.
21 changes: 16 additions & 5 deletions packages/aws-cdk-lib/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -632,22 +632,33 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
*
* @internal
*/
protected _createInstances(props: DatabaseClusterProps): InstanceConfig {
protected _createInstances(cluster: DatabaseClusterNew, props: DatabaseClusterProps): InstanceConfig {
const instanceEndpoints: Endpoint[] = [];
const instanceIdentifiers: string[] = [];
const readers: IAuroraClusterInstance[] = [];

let monitoringRole = props.monitoringRole;
if (!props.monitoringRole && props.monitoringInterval && props.monitoringInterval.toSeconds()) {
monitoringRole = new Role(cluster, 'MonitoringRole', {
assumedBy: new ServicePrincipal('monitoring.rds.amazonaws.com'),
managedPolicies: [
ManagedPolicy.fromAwsManagedPolicyName('service-role/AmazonRDSEnhancedMonitoringRole'),
],
});
}

// need to create the writer first since writer is determined by what instance is first
const writer = props.writer!.bind(this, this, {
monitoringInterval: props.monitoringInterval,
monitoringRole: props.monitoringRole,
monitoringRole: monitoringRole,
removalPolicy: props.removalPolicy ?? RemovalPolicy.SNAPSHOT,
subnetGroup: this.subnetGroup,
promotionTier: 0, // override the promotion tier so that writers are always 0
});
(props.readers ?? []).forEach(instance => {
const clusterInstance = instance.bind(this, this, {
monitoringInterval: props.monitoringInterval,
monitoringRole: props.monitoringRole,
monitoringRole: monitoringRole,
removalPolicy: props.removalPolicy ?? RemovalPolicy.SNAPSHOT,
subnetGroup: this.subnetGroup,
});
Expand Down Expand Up @@ -988,7 +999,7 @@ export class DatabaseCluster extends DatabaseClusterNew {
throw new Error('writer must be provided');
}

const createdInstances = props.writer ? this._createInstances(props) : legacyCreateInstances(this, props, this.subnetGroup);
const createdInstances = props.writer ? this._createInstances(this, props) : legacyCreateInstances(this, props, this.subnetGroup);
this.instanceIdentifiers = createdInstances.instanceIdentifiers;
this.instanceEndpoints = createdInstances.instanceEndpoints;
}
Expand Down Expand Up @@ -1185,7 +1196,7 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew {
if ((props.writer || props.readers) && (props.instances || props.instanceProps)) {
throw new Error('Cannot provide clusterInstances if instances or instanceProps are provided');
}
const createdInstances = props.writer ? this._createInstances(props) : legacyCreateInstances(this, props, this.subnetGroup);
const createdInstances = props.writer ? this._createInstances(this, props) : legacyCreateInstances(this, props, this.subnetGroup);
this.instanceIdentifiers = createdInstances.instanceIdentifiers;
this.instanceEndpoints = createdInstances.instanceEndpoints;
}
Expand Down
54 changes: 53 additions & 1 deletion packages/aws-cdk-lib/aws-rds/test/cluster.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1527,7 +1527,7 @@ describe('cluster', () => {
});
});

test('cluster with enabled monitoring', () => {
test('cluster with enabled monitoring (legacy)', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');
Expand Down Expand Up @@ -1584,6 +1584,58 @@ describe('cluster', () => {
});
});

test('cluster with enabled monitoring should create default role with new api', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
vpc,
writer: ClusterInstance.serverlessV2('writer'),
iamAuthentication: true,
monitoringInterval: cdk.Duration.minutes(1),
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::RDS::DBInstance', {
MonitoringInterval: 60,
MonitoringRoleArn: {
'Fn::GetAtt': ['DatabaseMonitoringRole576991DA', 'Arn'],
},
});

Template.fromStack(stack).hasResourceProperties('AWS::IAM::Role', {
AssumeRolePolicyDocument: {
Statement: [
{
Action: 'sts:AssumeRole',
Effect: 'Allow',
Principal: {
Service: 'monitoring.rds.amazonaws.com',
},
},
],
Version: '2012-10-17',
},
ManagedPolicyArns: [
{
'Fn::Join': [
'',
[
'arn:',
{
Ref: 'AWS::Partition',
},
':iam::aws:policy/service-role/AmazonRDSEnhancedMonitoringRole',
],
],
},
],
});
});

test('create a cluster with imported monitoring role', () => {
// GIVEN
const stack = testStack();
Expand Down

0 comments on commit 9065b25

Please sign in to comment.