feat(rds): add support for database instances #2187
Conversation
packages/@aws-cdk/aws-rds/README.md
Outdated
| @@ -45,33 +45,85 @@ By default, the master password will be generated and stored in AWS Secrets Mana | |||
| Your cluster will be empty by default. To add a default database upon construction, specify the | |||
| `defaultDatabaseName` attribute. | |||
|
|
|||
| ### Starting an Instance Database | |||
| To set up a instance database, create an instance of `DatabaseInstance`. You must | |||
There was a problem hiding this comment.
We use the terminology "define a DatabaseInstance" (instead of "create an instance").
| rule.addEventPattern({ | ||
| source: [ 'aws.rds' ], | ||
| resources: [ | ||
| this.node.stack.formatArn({ |
There was a problem hiding this comment.
Should we expose instanceArn?
| */ | ||
| export interface DatabaseInstanceNewProps { | ||
| /** | ||
| * The name of the compute and memory capacity classes. |
There was a problem hiding this comment.
Is there a sensible default here?
| /** | ||
| * Specifies if the database instance is a multiple Availability Zone deployment. | ||
| * | ||
| * @default false |
There was a problem hiding this comment.
What's the implications of making this true by default? Wouldn't that be a better practice?
There was a problem hiding this comment.
Yes, if possible I would like to a have production defaults in this construct: db.r5.large, multi az, 1000 IOPS, 100 GiB, encryption enabled with default master key, 7 days backup, enhanced monitoring, no auto minor version upgrade (the console experience when selecting Production).
Looking at the discussion we had here #2063 (comment) and the issue you raised with implicit costs to customer, would you be ok to accept these production defaults?
There was a problem hiding this comment.
@eladb any update on whether production defaults are acceptable for this construct?
There was a problem hiding this comment.
That's an interesting conversation, and I wonder if this would be something we run into in the future, sort of "presets". The RDS console definitely demonstrates that there is value in allowing people to being configuring their new database from a predefined set of options that are tailored for a use case.
Maybe we should have a similar experience:
new rds.DatabaseInstance(this, 'instance', rds.DatabaseInstancePresets.production());
// or, override some value:
new rds.DatabaseInstance(this, 'instance', rds.DatabaseInstancePresets.production({
multiAz: false
}));Something like that?
There was a problem hiding this comment.
How would you specify mandatory properties like engine and masterUsername with this API? Spread the preset and add those properties?
{
...rds.DatabaseInstancePresets.production(),
masterUsername: '...'
}| * The number of I/O operations per second (IOPS) that the database provisions. | ||
| * The value must be equal to or greater than 1000. | ||
| * | ||
| * @default no iops |
There was a problem hiding this comment.
This is actually setting provisioned IOPS. The default behavior is you don't get provisioned (aka guaranteed) IOPS... This can incur additional cost when enabled.
So yeah, I guess the phrasing can be improve to this effect. I think copying the documentation from RDS/CloudFormation should be best.
| /** | ||
| * The number of CPU cores and the number of threads per core. | ||
| * | ||
| * @default no processor features |
| * | ||
| * @default false | ||
| */ | ||
| readonly enableIAMDatabaseAuthentication?: boolean; |
There was a problem hiding this comment.
Would it be sufficient to just call this iamAuth or something more concise?
There was a problem hiding this comment.
I would go for iamAuthentication
There was a problem hiding this comment.
Ideally, there should also be a grant method to provide permissions there. And this is tricky because it requires to know the instance's Resource ID, which I don't reckon is surfaced by CloudFormation (aka requires CustomResource 😔)
There was a problem hiding this comment.
aka requires CustomResource
Another use case for #1850
| /** | ||
| * The daily time range during which automated backups are performed. | ||
| * | ||
| * @default no preference |
There was a problem hiding this comment.
Does that mean random time range or no backup?
There was a problem hiding this comment.
"no preference" means AWS chooses for you.
| /** | ||
| * The storage type. | ||
| * | ||
| * @default GP2 |
There was a problem hiding this comment.
This is not correct: the current implementation defaults to IOPS not GP2. Waiting for a reply on production defaults to update.
There was a problem hiding this comment.
This still needs to be updated (wrong default value documented). @eladb any feedback on using production defaults here ? See #2187 (comment)
Add support for database instances.
Specific classes are exposed for a source database instance (
DatabaseInstance), an instance created from a snapshot (DatabaseInstanceFromSnapshot) and a read replica instance (DatabaseInstanceReadReplica).Add construct for option groups and refactor parameter groups.
Add basic support for instance event rules.
Integration with Secrets Manager and secret rotation.
Closes #2075
Closes #1693
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.