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

fix(rds): instance identifiers and endpoints of a Cluster are blank #14394

Merged
merged 12 commits into from
Apr 29, 2021
Merged

fix(rds): instance identifiers and endpoints of a Cluster are blank #14394

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
7ba3f6d
fix(aws-rds): allow accessing instance identifiers and endpoints of a…
Apr 27, 2021
956b771
fix(aws-rds): remove default values from instance identifiers and end…
Apr 28, 2021
bae2b64
fix(aws-rds): refactor destructuring out
Apr 28, 2021
d9d014f
fix(aws-rds): refactor tests to be included into an existing unit test
Apr 28, 2021
50735c3
fix(aws-rds): remove extra empty line
Apr 28, 2021
9a83757
fix(aws-rds): reorder class fields of DatabaseCluster
Apr 29, 2021
5d88e31
fix(aws-rds): make assertion more compact
Apr 29, 2021
b22d926
fix(aws-rds): remove unnecessary empty line
Apr 29, 2021
de8419f
fix(aws-rds): include testing for DatabaseClusterFromSnapshot as well
Apr 29, 2021
2e4c564
fix(aws-rds): fix test
Apr 29, 2021
d8b37f3
Merge branch 'master' into master
mergify[bot] Apr 29, 2021
d46c481
Merge branch 'master' into vilikin_master
skinny85 Apr 29, 2021
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
14 changes: 10 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
Expand Up @@ -298,8 +298,6 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
* Never undefined.
*/
public readonly engine?: IClusterEngine;
public readonly instanceIdentifiers: string[] = [];
public readonly instanceEndpoints: Endpoint[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move these? We keep members in a specific order (public, then protected, then private). Let's keep it this way here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are actually moved to the implementing classes, so they no longer exist on the DatabaseClusterNew class. (diff looks almost as if they were just moved a bit down inside same class, but there's actually 200 rows in between)

The reason for moving them was that I could not figure out a way to keep them readonly on the base class if we needed to set them from the implementing classes.


protected readonly newCfnProps: CfnDBClusterProps;
protected readonly securityGroups: ec2.ISecurityGroup[];
Expand Down Expand Up @@ -481,6 +479,8 @@ export class DatabaseCluster extends DatabaseClusterNew {
public readonly clusterEndpoint: Endpoint;
public readonly clusterReadEndpoint: Endpoint;
public readonly connections: ec2.Connections;
public readonly instanceIdentifiers: string[];
public readonly instanceEndpoints: Endpoint[];

/**
* The secret attached to this cluster
Expand Down Expand Up @@ -533,7 +533,9 @@ export class DatabaseCluster extends DatabaseClusterNew {
}

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

/**
Expand Down Expand Up @@ -602,6 +604,8 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew {
public readonly clusterEndpoint: Endpoint;
public readonly clusterReadEndpoint: Endpoint;
public readonly connections: ec2.Connections;
public readonly instanceIdentifiers: string[];
public readonly instanceEndpoints: Endpoint[];

constructor(scope: Construct, id: string, props: DatabaseClusterFromSnapshotProps) {
super(scope, id, props);
Expand All @@ -625,7 +629,9 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew {
cluster.applyRemovalPolicy(props.removalPolicy ?? RemovalPolicy.SNAPSHOT);

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

Expand Down
48 changes: 46 additions & 2 deletions packages/@aws-cdk/aws-rds/test/cluster.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ describe('cluster', () => {
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new DatabaseCluster(stack, 'Database', {
const cluster = new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
instances: 1,
credentials: {
Expand All @@ -97,6 +97,28 @@ describe('cluster', () => {
MasterUserPassword: 'tooshort',
VpcSecurityGroupIds: [{ 'Fn::GetAtt': ['DatabaseSecurityGroup5C91FDCB', 'GroupId'] }],
});

expect(cluster.instanceIdentifiers).toHaveLength(1);
expect(stack.resolve(cluster.instanceIdentifiers[0])).toEqual({
Ref: 'DatabaseInstance1844F58FD',
});

expect(cluster.instanceEndpoints).toHaveLength(1);
expect(stack.resolve(cluster.instanceEndpoints[0])).toEqual({
hostname: {
'Fn::GetAtt': ['DatabaseInstance1844F58FD', 'Endpoint.Address'],
},
port: {
'Fn::GetAtt': ['DatabaseB269D8BB', 'Endpoint.Port'],
},
socketAddress: {
'Fn::Join': ['', [
{ 'Fn::GetAtt': ['DatabaseInstance1844F58FD', 'Endpoint.Address'] },
':',
{ 'Fn::GetAtt': ['DatabaseB269D8BB', 'Endpoint.Port'] },
]],
},
});
});

test('can create a cluster with imported vpc and security group', () => {
vilikin marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -1635,7 +1657,7 @@ describe('cluster', () => {
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new DatabaseClusterFromSnapshot(stack, 'Database', {
const cluster = new DatabaseClusterFromSnapshot(stack, 'Database', {
engine: DatabaseClusterEngine.aurora({ version: AuroraEngineVersion.VER_1_22_2 }),
instanceProps: {
vpc,
Expand All @@ -1659,6 +1681,28 @@ describe('cluster', () => {
}, ResourcePart.CompleteDefinition);

expect(stack).toCountResources('AWS::RDS::DBInstance', 2);

expect(cluster.instanceIdentifiers).toHaveLength(2);
expect(stack.resolve(cluster.instanceIdentifiers[0])).toEqual({
Ref: 'DatabaseInstance1844F58FD',
});

expect(cluster.instanceEndpoints).toHaveLength(2);
expect(stack.resolve(cluster.instanceEndpoints[0])).toEqual({
hostname: {
'Fn::GetAtt': ['DatabaseInstance1844F58FD', 'Endpoint.Address'],
},
port: {
'Fn::GetAtt': ['DatabaseB269D8BB', 'Endpoint.Port'],
},
socketAddress: {
'Fn::Join': ['', [
{ 'Fn::GetAtt': ['DatabaseInstance1844F58FD', 'Endpoint.Address'] },
':',
{ 'Fn::GetAtt': ['DatabaseB269D8BB', 'Endpoint.Port'] },
]],
},
});
});

test('reuse an existing subnet group', () => {
Expand Down