feat(rds): add RDS DatabaseInstance.fromLookup #32901

pcheungamz · 2025-01-13T20:36:35Z

Issue # (if applicable)

#31720

Needs this PR: cdklabs/cloud-assembly-schema#124

Reason for this change

Add DatabaseInstance.fromLookup() feature

Description of changes

Use CloudControl API as ContextProvider to query for DatabaseInstance.

Describe any new or updated permissions being added

Use will need to have permission to run CloudControl API.

Description of how you validated changes

Included unit tests for CC API and DatabaseInstance changes.

Checklist

[ X ] My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.
❌ CLI code has changed. A maintainer must run the code through the testing pipeline (git fetch origin pull/32901/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

aws-cdk-automation · 2025-01-13T21:02:12Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 285be37
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr

❤️ This looks great, thanks!

I have couple of small comments.

rix0rrr · 2025-01-17T08:50:00Z

packages/aws-cdk/test/context-providers/cc-api-provider.test.ts

+  });
+
+  // THEN
+  expect(result).toEqual({});


Shouldn't we return some sort of error value? Perhaps throw an exception?

ok, will throw error instead of returning {}.

rix0rrr · 2025-01-17T08:55:27Z

packages/aws-cdk/test/context-providers/cc-api-provider.test.ts

+  });
+
+  // THEN
+  const propsObj = result['my-db-instance-1'];


I see the API is returning a dictionary { id1 -> props1, id2 -> props2, ... }.

What's the advantage of mapping by ID? I'm honestly asking because I don't know if there's a reason for it. Wouldn't it make more sense to return a list? Just [ props1, props2, ... ] ?

I don't think there is a way for the consumer of this API to know the id in advance, so they will always need to iterate over Object.entries() anyway to get both [id, props] at the same time... but the identifier will be in the propsX object already anyway? (Or at least, if it is requested, which it should be)

I see your point. Will change it to something like { results: [props1, props2, ...]}

rix0rrr · 2025-01-17T08:56:17Z

packages/aws-cdk/lib/context-providers/cc-api-provider.ts

+  public async getValue(args: CcApiContextQuery) {
+    const cloudControl = (await initContextProviderSdk(this.aws, args)).cloudControl();
+
+    const result = await this.findResources(cloudControl, args);


Is there an exception handler around this already?

What if findJsonValue throws because the user made a type in their path, will the error be handled sensibly? (I.e., descriptive error shown to the user, not a gory stack trace pointing somewhere to the innards of the CLI?)

The current code returns undefined so no property of that name is set.
Since we are throwing exceptions in findJsonValue, I will add error handling and unit tests for that.

rix0rrr · 2025-01-17T08:59:33Z

packages/aws-cdk/lib/context-providers/cc-api-provider.ts

+ * @param path
+ */
+export function findJsonValue(jsonObject: any, path: string): any {
+  return path.split('.').reduce((r, k) => r[k], jsonObject);


This is clever and short, but I think I would like some more type checking on it and throwing descriptive error messages. Likely things users are going to do wrong: mistype a key, reference a key that's missing, reference a key of the wrong type...

Some errors of the kind:

Cannot read field 'foo.bar.quux': expected an object at 'foo.bar' but got "hello" Cannot read field 'foo.bar.quux': expected an object at 'foo.bar' but got undefined

Or something to that effect.

Makes sense. I will add better error handling.

rix0rrr · 2025-01-17T09:01:28Z

packages/aws-cdk/lib/context-providers/cc-api-provider.ts

+ * @param propertiesToReturn
+ * @returns
+ */
+export function toResultObj(jsonObject: any, propertiesToReturn: string[]): {[key: string]: any} {


Can we extract all this JSON object manipulation out to a separate file? It's independent and worthwhile enough to not confine it to the "cc api provider", where no one will ever find it again :)

That makes sense. How about aws-cdk/lib/util/json.ts?

rix0rrr · 2025-01-17T09:02:33Z

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

+      dummyValue: {},
+    }).value;
+
+    const instance = response[options.instanceIdentifier];


You need to account for the fact that it wasn't found, right? What does that do?

Will add try-catch and throw an Error when there is a problem with CC API lookup.

rix0rrr · 2025-01-17T09:03:34Z

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

+  /**
+   * The instance identifier of the DatabaseInstance
+   */
+  readonly instanceIdentifier: string;


The issue this PR is trying to close says to use databaseIdentifier. Is instanceIdentifier the same thing as databaseIdentifier?

In RDS doc, DatabaseInstance has an instanceIdentifier property. So I try to be consistent with the doc. https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_rds.DatabaseInstance.html#instanceidentifier-1

rix0rrr · 2025-01-17T09:05:32Z

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

+        exactIdentifier: options.instanceIdentifier,
+        propertiesToReturn: [
+          'DBInstanceArn',
+          'Endpoint.Address',
+          'Endpoint.Port',
+          'DbiResourceId',
+          'DBSecurityGroups',
+        ],
+      } as cxschema.CcApiContextQuery,


This looks great! Does it also work? Have you tested this in your actual AWS account?

I trust that the unit tests assert that the code holds to some contract... but do we actually know the contract is the right one? 😁

rix0rrr · 2025-01-17T09:07:34Z

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

+      const securityGroup = ec2.SecurityGroup.fromSecurityGroupId(
+        scope,
+        `LSG-${securityGroupId}`,
+        securityGroupId,
+      );


We probably need to add an option to the properties object, and pass it here to fromSecurityGroupId, on whether the security groups are going to be mutable or immutable by default.

The difference is whether we will try to add rules to them. false makes more sense, but due to historical reasons the default is true... ☹️

rix0rrr · 2025-01-17T09:09:56Z

packages/aws-cdk-lib/aws-rds/README.md

+// Add the new security group to the existing security groups of the RDS instance
+instance.connections.addSecurityGroup(myNewSecurityGroup);


The example of what to do with a freshly imported database instance doesn't make a lot of sense. Adding a security group to it in this way is unlikely to be what you want (and also, this doesn't do anything because the instance itself is not owned).

Is there another example to copy/paste?

db.grantConnect and db.connections.allowDefaultPortFrom are also popular results from my search. I will use grantConnect instead.

mrgrain · 2025-01-17T09:33:06Z

packages/aws-cdk/package.json

    "@jsii/check-node": "1.104.0",
-    "@smithy/middleware-endpoint": "3.1.4",
+    "@smithy/middleware-endpoint": "4.0.0",


A new major version. This is particularly scary! Have you checked for breaking changes?

Good point. I will try to find a 3.x version that works with client-cloudcontrol.

Rico had already bump this to '^4'. I won't need to make this change.

mrgrain · 2025-01-17T09:33:09Z

packages/aws-cdk/package.json

    "@smithy/node-http-handler": "3.2.4",
    "@smithy/property-provider": "3.1.10",
    "@smithy/shared-ini-file-loader": "3.1.8",
-    "@smithy/types": "3.5.0",
+    "@smithy/types": "4.0.0",


A new major version. This is particularly scary! Have you checked for breaking changes?

Will try to find a 3.x version that works with client-cloudcontrol.

mrgrain · 2025-01-17T09:35:43Z

packages/aws-cdk/test/context-providers/cc-api-provider.test.ts

This will probably require a change to packages/aws-cdk/lib/context-providers/index.ts as well.

ok, will double check this.

mrgrain · 2025-01-17T09:35:54Z

packages/aws-cdk/package.json

Thank you for trying to be a good citizen! It's usually very much appreciated. <3

We are currently trying to keep the CLI a bit more stable. Are any of the version upgrades strictly necessary to achieve your goal - or would adding the new package be enough? If so, I'd rather just add one and keep everything else as it was.

I had to use "@aws-sdk/client-cloudcontrol": "3.723.0". I had to bump the other ones to the same version, because of a smithy error.

mrgrain · 2025-01-17T09:42:26Z

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

+          'DbiResourceId',
+          'DBSecurityGroups',
+        ],
+      } as cxschema.CcApiContextQuery,


Is the type casting required here? How do we guarantee type correctness? If so, please add a comment explaining why.

Good point. We should use the keyword satisfies instead of as here.

aws-cdk-automation · 2025-01-30T00:11:57Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-02T00:13:43Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-03T00:12:32Z

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

aws-cdk-automation · 2025-02-04T00:11:38Z

This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2025-02-11T00:11:45Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2025-02-11T00:11:59Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

feat(rds): Add DatabaseInstance.fromLookup

1a3aa8f

pcheungamz requested a review from a team as a code owner January 13, 2025 20:36

github-actions bot added p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 13, 2025

aws-cdk-automation requested a review from a team January 13, 2025 20:36

pcheungamz had a problem deploying to test-pipeline January 13, 2025 20:37 — with GitHub Actions Failure

aws-cdk-automation requested changes Jan 13, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Jan 13, 2025

pcheungamz changed the title ~~feat(rds): Add DatabaseInstance.fromLookup~~ feat(rds): add RDS DatabaseInstance.fromLookup Jan 13, 2025

Merge branch 'main' into rds-cc-api

285be37

pcheungamz had a problem deploying to test-pipeline January 13, 2025 20:58 — with GitHub Actions Failure

rix0rrr requested changes Jan 17, 2025

View reviewed changes

rix0rrr reviewed Jan 17, 2025

View reviewed changes

mrgrain reviewed Jan 17, 2025

View reviewed changes

mrgrain requested changes Jan 17, 2025

View reviewed changes

mrgrain reviewed Jan 17, 2025

View reviewed changes

pcheungamz mentioned this pull request Jan 31, 2025

feat Add aws-rds.DatabaseInstance.fromLookup #33258

Open

github-actions bot mentioned this pull request Feb 1, 2025

Monthly PR metrics report #33266

Open

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Feb 11, 2025

aws-cdk-automation closed this Feb 11, 2025

github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2025

		// Add the new security group to the existing security groups of the RDS instance
		instance.connections.addSecurityGroup(myNewSecurityGroup);

feat(rds): add RDS DatabaseInstance.fromLookup #32901

feat(rds): add RDS DatabaseInstance.fromLookup #32901

Conversation

pcheungamz commented Jan 13, 2025

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

aws-cdk-automation left a comment • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 13, 2025

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jan 30, 2025

aws-cdk-automation commented Feb 2, 2025

aws-cdk-automation commented Feb 3, 2025

aws-cdk-automation commented Feb 4, 2025

aws-cdk-automation commented Feb 11, 2025

github-actions bot commented Feb 11, 2025

aws-cdk-automation left a comment •

edited

Loading

rix0rrr Jan 17, 2025 •

edited

Loading