-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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 RDS DatabaseInstance.fromLookup #32901
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,7 +17,8 @@ import * as kms from '../../aws-kms'; | |
import * as logs from '../../aws-logs'; | ||
import * as s3 from '../../aws-s3'; | ||
import * as secretsmanager from '../../aws-secretsmanager'; | ||
import { ArnComponents, ArnFormat, Duration, FeatureFlags, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, Tokenization } from '../../core'; | ||
import * as cxschema from '../../cloud-assembly-schema'; | ||
import { ArnComponents, ArnFormat, ContextProvider, Duration, FeatureFlags, IResource, Lazy, RemovalPolicy, Resource, Stack, Token, Tokenization } from '../../core'; | ||
import * as cxapi from '../../cx-api'; | ||
|
||
/** | ||
|
@@ -132,6 +133,49 @@ export interface DatabaseInstanceAttributes { | |
* A new or imported database instance. | ||
*/ | ||
export abstract class DatabaseInstanceBase extends Resource implements IDatabaseInstance { | ||
/** | ||
* Lookup an existing DatabaseInstance using instanceIdentifier. | ||
*/ | ||
public static fromLookup(scope: Construct, id: string, options: DatabaseInstanceLookupOptions): IDatabaseInstance { | ||
const response: {[key: string]: any} = ContextProvider.getValue(scope, { | ||
provider: cxschema.ContextProvider.CC_API_PROVIDER, | ||
props: { | ||
typeName: 'AWS::RDS::DBInstance', | ||
exactIdentifier: options.instanceIdentifier, | ||
propertiesToReturn: [ | ||
'DBInstanceArn', | ||
'Endpoint.Address', | ||
'Endpoint.Port', | ||
'DbiResourceId', | ||
'DBSecurityGroups', | ||
], | ||
} as cxschema.CcApiContextQuery, | ||
Comment on lines
+144
to
+152
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? 😁 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the type casting required here? How do we guarantee type correctness? If so, please add a comment explaining why. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. We should use the keyword |
||
dummyValue: {}, | ||
}).value; | ||
|
||
const instance = response[options.instanceIdentifier]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You need to account for the fact that it wasn't found, right? What does that do? |
||
|
||
// Get ISecurityGroup from securityGroupId | ||
const securityGroups: ec2.ISecurityGroup[] = []; | ||
const dbsg: [string] = instance.DBSecurityGroups; | ||
dbsg.forEach(securityGroupId => { | ||
const securityGroup = ec2.SecurityGroup.fromSecurityGroupId( | ||
scope, | ||
`LSG-${securityGroupId}`, | ||
securityGroupId, | ||
); | ||
Comment on lines
+162
to
+166
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We probably need to add an option to the properties object, and pass it here to The difference is whether we will try to add rules to them. |
||
securityGroups.push(securityGroup); | ||
}); | ||
|
||
return this.fromDatabaseInstanceAttributes(scope, id, { | ||
instanceEndpointAddress: instance['Endpoint.Address'], | ||
port: instance['Endpoint.Port'], | ||
instanceIdentifier: options.instanceIdentifier, | ||
securityGroups: securityGroups, | ||
instanceResourceId: instance.DbiResourceId, | ||
}); | ||
} | ||
|
||
/** | ||
* Import an existing database instance. | ||
*/ | ||
|
@@ -1125,6 +1169,16 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa | |
} | ||
} | ||
|
||
/** | ||
* Properties for looking up an existing DatabaseInstance. | ||
*/ | ||
export interface DatabaseInstanceLookupOptions { | ||
/** | ||
* The instance identifier of the DatabaseInstance | ||
*/ | ||
readonly instanceIdentifier: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue this PR is trying to close says to use |
||
} | ||
|
||
/** | ||
* Construction properties for a DatabaseInstance. | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
import { Construct } from 'constructs'; | ||
import * as cxschema from '../../cloud-assembly-schema'; | ||
import { ContextProvider, GetContextValueOptions, GetContextValueResult, Lazy, Stack } from '../../core'; | ||
import * as rds from '../lib'; | ||
|
||
describe('DatabaseInstanceBase from lookup', () => { | ||
test('return correct instance info', () => { | ||
const dataObj = {}; | ||
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']: [] }); | ||
const resultObj = {}; | ||
Object.assign(resultObj, { ['instance-1']: dataObj }); | ||
|
||
const previous = mockDbInstanceContextProviderWith(resultObj, options => { | ||
expect(options.exactIdentifier).toEqual('instance-1'); | ||
}); | ||
|
||
const stack = new Stack(undefined, undefined, { env: { region: 'us-east-1', account: '123456789012' } }); | ||
const instance = rds.DatabaseInstance.fromLookup(stack, 'MyInstance', { | ||
instanceIdentifier: 'instance-1', | ||
}); | ||
|
||
expect(instance.instanceIdentifier).toEqual('instance-1'); | ||
expect(instance.dbInstanceEndpointAddress).toEqual('instance-1.testserver.us-east-1.rds.amazonaws.com'); | ||
expect(instance.dbInstanceEndpointPort).toEqual('5432'); | ||
expect(instance.instanceResourceId).toEqual('db-ABCDEFGHI'); | ||
|
||
restoreContextProvider(previous); | ||
}); | ||
}); | ||
|
||
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' }); | ||
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']: ['dbsg-1', 'dbsg-2'] }); | ||
const resultObj = {}; | ||
Object.assign(resultObj, { ['instance-1']: dataObj }); | ||
|
||
const previous = mockDbInstanceContextProviderWith(resultObj, options => { | ||
expect(options.exactIdentifier).toEqual('instance-1'); | ||
}); | ||
|
||
const stack = new Stack(undefined, undefined, { env: { region: 'us-east-1', account: '123456789012' } }); | ||
const instance = rds.DatabaseInstance.fromLookup(stack, 'MyInstance', { | ||
instanceIdentifier: 'instance-1', | ||
}); | ||
|
||
expect(instance.instanceIdentifier).toEqual('instance-1'); | ||
expect(instance.dbInstanceEndpointAddress).toEqual('instance-1.testserver.us-east-1.rds.amazonaws.com'); | ||
expect(instance.dbInstanceEndpointPort).toEqual('5432'); | ||
expect(instance.instanceResourceId).toEqual('db-ABCDEFGHI'); | ||
expect(instance.connections.securityGroups.length).toEqual(2); | ||
|
||
restoreContextProvider(previous); | ||
}); | ||
}); | ||
|
||
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; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?