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

feat(rds): add clusterArn property to DatabaseCluster #29289

Closed
wants to merge 1 commit into from
Closed

feat(rds): add clusterArn property to DatabaseCluster #29289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions packages/aws-cdk-lib/aws-rds/lib/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,18 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
};
}

/**
* The ARN of the cluster
*/
public get clusterArn(): string {
return Stack.of(this).formatArn({
service: 'rds',
resource: 'cluster',
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
resourceName: this.clusterIdentifier,
});
}

Comment on lines +688 to +699
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just like ServerlessCluster

aws-cdk/packages/aws-cdk-lib/aws-rds/lib/serverless-cluster.ts

Lines 345 to 355 in 1f30b5d

/**
* The ARN of the cluster
*/
public get clusterArn(): string {
return Stack.of(this).formatArn({
service: 'rds',
resource: 'cluster',
arnFormat: ArnFormat.COLON_RESOURCE_NAME,
resourceName: this.clusterIdentifier,
});
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@blimmer Is this how CDK normally handles this? I ask because the output becomes a Join instead of doing a GetAtt on the resource. Trying to wrap my head around why we should do this over the GetAtt path. I tried looking at the Lambda Function L2, since there is a readonly property for the Arn.

Honestly not sure if it matters (likely not) but trying to learn different parts of this as I go. To me seems like we have two patterns going on, the one you have here and the readonly property for the Arn.

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 not sure - I just followed the pattern in the linked code. One of the core contributors might be able to tell us.

Copy link
Contributor

Choose a reason for hiding this comment

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

I asked the team directly. I did find SQS which does this differently. https://github.com/aws/aws-cdk/blob/v2.130.0/packages/aws-cdk-lib/aws-sqs/lib/queue.ts#L437-L440

const queue = new sqs.Queue(this, 'CdkExampleQueue', {
      visibilityTimeout: Duration.seconds(300)
    });

new CfnOutput(this, "arn", {
      exportName: "arn",
      value: queue.queueArn,
    })

CFN Synth

Outputs:
  arn:
    Value:
      Fn::GetAtt:
        - CdkExampleQueue7618E31B
        - Arn
    Export:
      Name: arn

Which fits my mental model better. I am not sure if there is a big difference or a reason it was done this way in RDS. Maybe one is a "legacy" way of doing things.

Generally, this looks fine. I just want to get some deeper clarity on what is typical.

Copy link
Contributor

@colifran colifran Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@jfuss Sorry to jump in - I was looking at something similar recently. This is a very typical pattern that is used in the codebase here are just a few examples:

aws-cdk/packages/aws-cdk-lib/aws-kms/lib/alias.ts

Line 69 in 0d6ef63

return Stack.of(this).formatArn({

aws-cdk/packages/aws-cdk-lib/aws-docdb/lib/instance.ts

Line 103 in 0d6ef63

return cdk.Stack.of(this).formatArn({

aws-cdk/packages/aws-cdk-lib/aws-apigateway/lib/stage.ts

Line 260 in 0d6ef63

return Stack.of(this).formatArn({

Also, this is just a "getter" for the clusterArn so I don't think we want to be producing any outputs in the synthesized template. Instead, this just allows clusterArn to be exposed for use throughout the rest of their app.

/**
* Create cluster instances
*
Expand Down
28 changes: 28 additions & 0 deletions packages/aws-cdk-lib/aws-rds/test/cluster.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4052,6 +4052,34 @@ describe('cluster', () => {
],
});
});

test('check that clusterArn property works', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

aws-cdk/packages/aws-cdk-lib/aws-rds/test/serverless-cluster.test.ts

Lines 602 to 627 in 1f30b5d

test('check that clusterArn property works', () => {
// GIVEN
const stack = testStack();
const vpc = ec2.Vpc.fromLookup(stack, 'VPC', { isDefault: true });
const cluster = new ServerlessCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA_MYSQL,
vpc,
});
const exportName = 'DbCluterArn';
// WHEN
new cdk.CfnOutput(stack, exportName, {
exportName,
value: cluster.clusterArn,
});
// THEN
expect(stack.resolve(cluster.clusterArn)).toEqual({
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':rds:us-test-1:12345:cluster:',
{ Ref: 'DatabaseB269D8BB' },
]],
});
});

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

// WHEN
const cluster = new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
writer: ClusterInstance.provisioned('writer'),
vpc,
});
new cdk.CfnOutput(stack, exportName, {
exportName,
value: cluster.clusterArn,
});

// THEN
expect(stack.resolve(cluster.clusterArn)).toEqual({
'Fn::Join': ['', [
'arn:',
{ Ref: 'AWS::Partition' },
':rds:us-test-1:12345:cluster:',
{ Ref: 'DatabaseB269D8BB' },
]],
});
});
});

test.each([
Expand Down
Loading