feat(rds): new `DatabaseInstance.fromLookup` #33258

pcheungamz · 2025-01-31T14:27:59Z

Issue # (if applicable)

This replaces my previous PR #32901. I addressed the PR comments in this new PR.

This depends on this PR: cdklabs/cloud-assembly-schema#124.

Also depends on this CDK CLI PR: aws/aws-cdk-cli#138. That PR should be merged first and the CLI released, before this PR can be merged.

Reason for this change

Add DatabaseInstance.fromLookup() feature

Description of changes

Add CC API Context Provider. Needs this PR: feat: Add CC API based Context Query. cdklabs/cloud-assembly-schema#124
DatabaseInstance.fromLookup call CC API to get the database instance info from instanceIdentifier.
Add units tests.

Describe any new or updated permissions being added

User will need to have permission to run CloudControl API.

Description of how you validated changes

Tested with this code. I already have an RDS DB in my AWS account. I want to look it up and grant connect to a new user.
Saved to packages/@aws-cdk-testing/framework-integ/test/aws-rds/test/my-test-app.ts

import * as cdk from 'aws-cdk-lib';
import * as iam from 'aws-cdk-lib/aws-iam';
import * as rds from 'aws-cdk-lib/aws-rds';

const awsAccountId = 'XXXXXXXXXX79';
const instanceId = 'XXXXXXXXXX-instance-1';

const appWithDb = new cdk.App();
const stack = new cdk.Stack(appWithDb, 'StackWithVpc', {
  env: {
    region: 'us-east-1',
    account: awsAccountId,
  },
});

const dbFromLookup = rds.DatabaseInstance.fromLookup(stack, 'dbFromLookup', {
  instanceIdentifier: instanceId,
});

/* eslint-disable no-console */
console.log('lookup values', dbFromLookup.dbInstanceEndpointAddress, dbFromLookup.dbInstanceEndpointPort);

const consoleReadOnlyRole = new iam.Role(stack, 'TestRole', {
  assumedBy: new iam.ArnPrincipal('arn_for_trusted_principal'),
});
dbFromLookup.grantConnect(consoleReadOnlyRole, 'dbTestUser');

Ran this command:

../../aws-cdk/bin/cdk -a 'npx ts-node test/aws-rds/test/my-test-app.ts' synth

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

(This review is outdated)

aws-cdk-automation · 2025-02-06T00:13:46Z

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-10T00:13:19Z

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.

rix0rrr · 2025-02-10T10:41:07Z

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

+      const securityGroups: ec2.ISecurityGroup[] = [];
+      const dbsg: [string] = instance.DBSecurityGroups;
+      dbsg.forEach(securityGroupId => {


💅 Instead of using forEach for a list.push() side effect, it's nicer to use a list.map() to produce another list.

const securityGroups = instance.DBSecurityGroups.map(id => ec2.SecurityGroup.fromSecurityGroupid(...))

thanks. will do

rix0rrr · 2025-02-10T10:45:16Z

packages/aws-cdk-lib/aws-rds/test/instance.from-lookup.test.ts

+    Object.assign(dataObj, { ['DBInstanceArn']: 'arn:aws:rds:us-east-1:123456789012:db:instance-1' });
+    Object.assign(dataObj, { ['Endpoint.Address']: 'instance-1.testserver.us-east-1.rds.amazonaws.com' });
+    Object.assign(dataObj, { ['Endpoint.Port']: '5432' });
+    Object.assign(dataObj, { ['DbiResourceId']: 'db-ABCDEFGHI' });
+    Object.assign(dataObj, { ['DBSecurityGroups']: [] });
+    Object.assign(dataObj, { ['Identifier']: 'instance-1' });


💅 Object.assign() is a way to copy fields from object A to object B, usually useful if you don't know what fields an object might have. Since all values are constants known in advance here, we could also have done:

const dataObj = { 'DBInstanceArn': 'arn:aws...', 'Endpoint.Address': 'instance-1.testserver', 'Endpoint.Port': '5432', // ... };

Which is a lot simpler.

I ran into a linter error. I guess it is better to turn off linter for this test files than to use Object.assign.

rix0rrr · 2025-02-10T10:46:14Z

packages/aws-cdk-lib/aws-rds/test/instance.from-lookup.test.ts

+describe('DatabaseInstanceBase from lookup with DBSG', () => {
+  test('return correct instance info', () => {
+    const dataObj = {};
+    Object.assign(dataObj, { ['DBInstanceArn']: 'arn:aws:rds:us-east-1:123456789012:db:instance-1' });


💅 Same comment here.

rix0rrr · 2025-02-10T10:48:05Z

packages/aws-cdk-lib/aws-rds/test/instance.from-lookup.test.ts

+function mockDbInstanceContextProviderWith(response: Object, paramValidator?: (options: cxschema.CcApiContextQuery) => void) {
+  const previous = ContextProvider.getValue;
+  ContextProvider.getValue = (_scope: Construct, options: GetContextValueOptions) => {
+    // do some basic sanity checks
+    expect(options.provider).toEqual(cxschema.ContextProvider.CC_API_PROVIDER);
+
+    if (paramValidator) {
+      paramValidator(options.props as any);
+    }
+
+    return {
+      value: {
+        ...response,
+      } as Map<string, Map<string, any>>,
+    };
+  };
+  return previous;
+}
+
+function restoreContextProvider(previous: (scope: any, options: GetContextValueOptions) => GetContextValueResult): void {
+  ContextProvider.getValue = previous;
+}


Did you copy this from elsewhere? Because using a Jest mock is simpler than this and it's the currently recommended way to write this.

// GIVEN const mock = jest.spyOn(ContextProvider.prototype, 'getValue').mockResolvedValue(...); // WHEN // ... // THEN expect(mock).toHaveBeenCalledWith(...); mock.restore();

yes, I borrowed it from aws-kms/test/key.from-lookup.test.ts. Will change that to follow the recommended standard.

I couldn't get jest.spyOn to work with ContextProvider. I got:

Property `getValue` does not exist in the provided object

NVM, this is a static method, so I have to use

const mock = jest.spyOn(ContextProvider, 'getValue').mockReturnValue(value);

rix0rrr · 2025-02-10T10:53:28Z

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

+      const result = await cc.listResources({
+        TypeName: args.typeName,
+      });


Slightly annoying but I believe that cc.getResource and cc.listResources will return different properties (at the discretion of the resource handler implementation). getResource will definitely return everything, and listResources wil return some summarized version.

That will probably be an annoying an unexpected behavior for users in the future. The safer behavior would be to call listResources first, and then call getResource for every resource returned that way. I'm debating whether we cast that behavior in stone right now, or wait for problems to turn up...

At least this behavior should be documented somewhere.

I tested with

aws cloudcontrol list-resources --type-name AWS::RDS::DBInstance

and

aws cloudcontrol get-resource --type-name AWS::RDS::DBInstance --identifier shine-reporting-aurora-instance-1

The properties return are the same. getResource and listResources are consistent at least for DBInstance.

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

rix0rrr · 2025-02-10T10:56:52Z

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

+        const propsObj = getResultObj(propsObject, args.propertiesToReturn);
+
+        // Add the identifier back to the propsObj.
+        propsObj.Identifier = result.ResourceDescription?.Identifier;


Can we make returning Identifier part of getResultObj as well? Then that function is entirely responsible for producing the returned object, which seems nice and comprehensible, and we don't have to remember to do this in both query functions.

make sense. will do

rix0rrr · 2025-02-10T10:58:55Z

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

+          let misMatch = false;
+          matchKey.forEach((key) => {
+            const value = findJsonValue(propsObject, key);
+            if (value !== args.propertyMatch![key]) {
+              misMatch = true;
+            }
+          });


This could be formulated as:

const filters = Object.values(args.propertyMatch); const isMatch = filters.every(([key, expected] => { const actual = findJsonValue(propsObject, key); return propertyMatchesFilter(actual, expected); }); function propertyMatchesFilter(actual: any, expected: any) { // For now we just check for strict equality, but we can implement pattern matching and fuzzy matching here later return expected === actual; }

I did 3 things here:

Use Object.values() instead of Object.keys(), so we iterate over key/value pairs (saves a lookup and some visual noise, imo).

Use list.every() instead of a forEach combined with mutating a boolean

Extract the actual comparison to a helper function, so that we have a clear location to start implementing more advanced filtering later

Thanks. will use this pattern.

I couldn't get your code to work. Object.values() doesn't give me back the key, but Object.entries() gave me both key and value. I also had trouble with every. It only gave me the first element.

This is my playground code: https://www.typescriptlang.org/play/?#code/MYewdgzgLgBAtgTwEoFNQCcAmAuGqOYA806AlmAOYA0MArmANZggDuYAfDALwwDeAUDBgByAMpQQ6AIYUUAUTDB0CAA5QUmYbmFR0tFMKqCRCzCpDkoAOgAKkqFpEBWACwBmAEyH+AXwDc-PygkLAAZqQANuroENwwAPIARgBWaNYAblIR+hAAFIj4kpgAlAH8APTlMOFRKDFWKOl1CLm5KAAeKmkaNOSYHcXcnAKVQmPBECARKFYRIBS5fR00HV3A6iUBoz6lgUmp6w1guqQoeQVoRcVWoZJyUsAAFq0A2gwoCDSZ2SgAuoNcYbGCZTGZzBYAAwA0h9cAASXjvBA+GgANSy+nhvG++h8EN2OwCQA

I got this to work

const filters = Object.entries(myRecord); filters.every((expected, index, arr) => { const key = arr[index][0]; const expected = arr[index][1]; const actual = findJsonValue(propsObject, key); return propertyMatchesFilter(actual, expected); });

Playground code: https://www.typescriptlang.org/play/?#code/MYewdgzgLgBAtgTwEoFNQCcAmAuGqOYA806AlmAOYA0MArmANZggDuYAfDALwwDeAUDBgByAMpQQ6AIYUUAUTDB0CAA5QUmYbmFR0tFMKqCRCzCpDkoAOgAKkqFpEBWACwBmAEyH+AXwDc-PwA9EEwoJCwAGakADbq6BDcMADyAEYAVmjWAG5SMfoQABSI+JKYAJQB4dAw0XEoCUlpmcDWKGC6pChFJWhllYF18RBWKNkNCIWFKAAeKlkaNOSYszRS6NII5dycAkJC1SAxKFYxIBSFy6sws-OtizDrmwDaVzMAus8ADO9rG1IIV5gFYfZ4ARneA32MHQKCgtHQYBgun0AR8A2CoWaWVGHTI3WKyD6WHKVkikjkUmAAAsps8GCgEDRcvkUJCdnxMftDsdTudCgADADSjNwABJeAyED4aAA1PL6cW8Fn6HwCgYhGDogJAA

rix0rrr · 2025-02-10T11:05:29Z

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

+
+        // Add the identifier back to the propsObj.
+        propsObj.Identifier = result.ResourceDescription?.Identifier;
+        resultObj[CcApiContextProviderPlugin.RESULTS] = [propsObj];


I see you decided to return a object with a { results: [...] } key, rather than a list directly.

I'm not against it because it's more easily extensible, but I'm not sure I see the need yet and I'm curious if there are any specific reasons to do it this wat? Just returning a direct list seems simpler to me, but I wonder if you're predicting some future change I'm not seeing yet.

I was following the Coral results within the return object that I am used to seeing. I am not tied to it. Happy to change it to a direct list, since this is more idiomatic for CDK.

rix0rrr · 2025-02-10T11:06:47Z

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

+  // THEN
+  const results = result[CcApiContextProviderPlugin.RESULTS];
+  let propsObj = results[0];
+  expect(propsObj.DBInstanceArn).toEqual('arn:aws:rds:us-east-1:123456789012:db:test-instance-1');


💅 Could also do:

expect(propsObj).toEqual({ DBInstanceArn: 'arn:...', StorageEncrypted: 'true', // ... });

Or even

expect(propsObj).toEqual(expect.objectContaining({ DBInstanceArn: 'arn:...', StorageEncrypted: 'true', // ... }));

thanks. will use this pattern.

rix0rrr · 2025-02-10T11:13:56Z

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

+        propsObj.Identifier = result.ResourceDescription?.Identifier;
+        resultObj[CcApiContextProviderPlugin.RESULTS] = [propsObj];
+      } else {
+        throw new TypeError(`Could not get resource ${args.exactIdentifier}.`);


Please add in the error message here.

And I don't agree that this is a TypeError.

Will use ContextProviderError instead.

rix0rrr · 2025-02-10T11:14:13Z

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

+        throw new TypeError(`Could not get resource ${args.exactIdentifier}.`);
+      }
+    } catch (err) {
+      throw new TypeError(`Encountered CC API error while getting resource ${args.exactIdentifier}.`);


Please add in the error message here.

And I don't agree that this is a TypeError.

will use ContextProviderError and include err in the message.

…hat as annotations.

Pull request has been modified.

codecov · 2025-03-07T16:55:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.35%. Comparing base (45623d6) to head (2a687f6).
Report is 62 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33258   +/-   ##
=======================================
  Coverage   82.35%   82.35%           
=======================================
  Files         120      120           
  Lines        6941     6941           
  Branches     1172     1172           
=======================================
  Hits         5716     5716           
  Misses       1120     1120           
  Partials      105      105

Flag	Coverage Δ
suite.unit	`82.35% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.35% <ø> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

aws-cdk-automation · 2025-03-19T00:18:02Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing

To prevent automatic closure:

Resume work on the PR
OR request an exemption by adding a comment containing 'Exemption Request' with justification e.x "Exemption Request: "
OR request clarification by adding a comment containing 'Clarification Request' with a question e.x "Clarification Request: "

This PR will automatically close in 14 days if no action is taken.

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

aws-cdk-automation · 2025-04-03T00:11:39Z

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-04-03T00:11:55Z

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.

Pull request has been modified.

aws-cdk-automation · 2025-04-04T00:11:47Z

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.

aws-cdk-automation · 2025-04-10T20:11:52Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: d5f4d9c
Result: SUCCEEDED
Build Logs (available for 30 days)

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

[feat] Add aws-rds.DatabaseInstance.fromLookup

e6b5d85

pcheungamz requested a review from a team as a code owner January 31, 2025 14:28

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

aws-cdk-automation requested a review from a team January 31, 2025 14:28

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jan 31, 2025

pcheungamz had a problem deploying to test-pipeline January 31, 2025 14:28 — with GitHub Actions Failure

aws-cdk-automation previously requested changes Jan 31, 2025

View reviewed changes

pcheungamz changed the title ~~[feat] Add aws-rds.DatabaseInstance.fromLookup~~ feat(rds) Add aws-rds.DatabaseInstance.fromLookup Jan 31, 2025

pcheungamz changed the title ~~feat(rds) Add aws-rds.DatabaseInstance.fromLookup~~ feat Add aws-rds.DatabaseInstance.fromLookup Jan 31, 2025

Merge branch 'main' into rds-cc-api-2

8fe2741

pcheungamz had a problem deploying to test-pipeline January 31, 2025 14:54 — with GitHub Actions Failure

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

Monthly PR metrics report #33266

Closed

rix0rrr requested changes Feb 10, 2025

View reviewed changes

rix0rrr reviewed Feb 10, 2025

View reviewed changes

getValue now returns a list of objects, simplifying the result

537bfa6

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

Use expect.objectContaining to check if the result is what we expected

5ad2a0a

pcheungamz had a problem deploying to test-pipeline February 10, 2025 21:18 — with GitHub Actions Failure

Add Identifier in getResultObj

f5a1c8b

pcheungamz had a problem deploying to test-pipeline February 10, 2025 21:41 — with GitHub Actions Failure

Use mock.spyOn to mockReturnValue on ContextProvider.getValue

af1e303

pcheungamz had a problem deploying to test-pipeline February 11, 2025 15:30 — with GitHub Actions Failure

Remove try-catch from fromLookup since ContextProvider already adds t…

6e85a50

…hat as annotations.

pcheungamz had a problem deploying to test-pipeline February 11, 2025 19:58 — with GitHub Actions Failure

Add typeName and CC API to the error msg

3626088

pcheungamz had a problem deploying to test-pipeline February 11, 2025 20:40 — with GitHub Actions Failure

Merge branch 'main' into rds-cc-api-2

b1bb804

kaizencc reviewed Mar 26, 2025

View reviewed changes

packages/aws-cdk-lib/aws-rds/README.md Outdated Show resolved Hide resolved

kaizencc added 2 commits March 26, 2025 10:45

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

8d6fd6e

Merge branch 'main' into rds-cc-api-2

4cb1b2c

kaizencc approved these changes Mar 26, 2025

View reviewed changes

Update README.md

2a687f6

kaizencc previously approved these changes Mar 26, 2025

View reviewed changes

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

aws-cdk-automation closed this Apr 3, 2025

github-actions bot locked as resolved and limited conversation to collaborators Apr 3, 2025

kaizencc reopened this Apr 3, 2025

Merge branch 'main' into rds-cc-api-2

93cec55

Merge branch 'main' into rds-cc-api-2

173e51b

kaizencc approved these changes Apr 3, 2025

View reviewed changes

aws-cdk-automation closed this Apr 4, 2025

kaizencc reopened this Apr 10, 2025

kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Apr 10, 2025

Merge branch 'main' into rds-cc-api-2

12e216e

kaizencc approved these changes Apr 10, 2025

View reviewed changes

Merge branch 'main' into rds-cc-api-2

d5f4d9c

mergify bot merged commit eb97d2d into aws:main Apr 10, 2025
20 checks passed

pcheungamz deleted the rds-cc-api-2 branch May 9, 2025 20:19

feat(rds): new DatabaseInstance.fromLookup #33258

feat(rds): new DatabaseInstance.fromLookup #33258

Uh oh!

Conversation

pcheungamz commented Jan 31, 2025 • edited by rix0rrr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

aws-cdk-automation left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Feb 6, 2025

Uh oh!

aws-cdk-automation commented Feb 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aws-cdk-automation commented Mar 19, 2025

Uh oh!

Uh oh!

aws-cdk-automation commented Apr 3, 2025

feat(rds): new `DatabaseInstance.fromLookup` #33258

feat(rds): new `DatabaseInstance.fromLookup` #33258

pcheungamz commented Jan 31, 2025 •

edited by rix0rrr

Loading

aws-cdk-automation left a comment •

edited

Loading

codecov bot commented Mar 7, 2025 •

edited

Loading