feat Add aws-rds.DatabaseInstance.fromLookup #33258

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

Issue # (if applicable)

Closes #31720

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

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

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/33258/head && git push -f origin FETCH_HEAD:test-main-pipeline), then add the 'pr-linter/cli-integ-tested' label when the pipeline succeeds.
❌ The title prefix of this pull request must be one of "feat|fix|build|chore|ci|docs|style|refactor|perf|test|revert"

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-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

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.

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

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

+  }
+
+  private async findResources(cc: ICloudControlClient, args: CcApiContextQuery): Promise<{[key: string]: any}> {
+    if (args.exactIdentifier) {


It should be an error if both exactIdentifier and propertyMatch are given. The most logical place to check would be here I think.

And it would be nicer if the the parameters to this.getResource() and this.listResources() were the relevant fields directly and all checks on the field could be supplied here, rather than the entire CcApiContextQuery objeect.

Will move the param validation here.

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.

aws-cdk-automation · 2025-02-10T21:45:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: f5a1c8b
Result: FAILED
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 requested a deployment to test-pipeline January 31, 2025 14:28 — with GitHub Actions Waiting

aws-cdk-automation 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 requested a deployment to test-pipeline January 31, 2025 14:54 — with GitHub Actions Waiting

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

Monthly PR metrics report #33266

Open

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 requested a deployment to test-pipeline February 10, 2025 20:58 — with GitHub Actions Waiting

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

5ad2a0a

pcheungamz requested a deployment to test-pipeline February 10, 2025 21:18 — with GitHub Actions Waiting

Add Identifier in getResultObj

f5a1c8b

pcheungamz requested a deployment to test-pipeline February 10, 2025 21:41 — with GitHub Actions Waiting

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat Add aws-rds.DatabaseInstance.fromLookup #33258

feat Add aws-rds.DatabaseInstance.fromLookup #33258

pcheungamz commented Jan 31, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading

aws-cdk-automation commented Feb 6, 2025

aws-cdk-automation commented Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

pcheungamz Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

rix0rrr Feb 10, 2025

pcheungamz Feb 10, 2025

aws-cdk-automation commented Feb 10, 2025

feat Add aws-rds.DatabaseInstance.fromLookup #33258

Are you sure you want to change the base?

feat Add aws-rds.DatabaseInstance.fromLookup #33258

Conversation

pcheungamz commented Jan 31, 2025 • edited Loading

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 Feb 6, 2025

aws-cdk-automation commented Feb 10, 2025

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

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 Feb 10, 2025

AWS CodeBuild CI Report

pcheungamz commented Jan 31, 2025 •

edited

Loading

aws-cdk-automation left a comment •

edited

Loading