-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(rds): instance engine lifecycle support #34719
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
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for the PR. There are almost no problems, but I left a few very minor comments.
|
||
// For simplicity, get a public snapshot | ||
new rds.DatabaseInstanceFromSnapshot(stack, 'FromSnapshot', { | ||
snapshotIdentifier: 'arn:aws:rds:us-east-1:484907511898:snapshot:vuln-test-db-snapshot-prod', |
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.
Is this account ID not AWS official's but yours? In my opinion, tests shouldn't depend on your or someone's account with hard coding.
I know that the way has already used in integ.instance-from-cluster-snapshot.ts
, but there is a possibility that the account or the snapshot will be deleted, then developers who run this test will be confused. (Furthermore, although an AWS account ID is not confidential information, it would be good not to disclosed carelessly.)
How about using environment variables or any variables (such as process.env.CDK_INTEG_ACCOUNT || process.env.CDK_DEFAULT_ACCOUNT
, stack.account
, etc.)?
And it would be better to write a comment to need to create the resource and the snapshot manually before running the test. (Alternatively, we may also create them in preDeploy
of hooks
in IntegTest
and delete them in postDestroy
, but it may take a long time and be difficult.)
Or, we could create a custom resource and create a snapshot of sourceInstance
within it and return that. But it might be a flaky test so it might be good the first choice.
What do you think?
*Even if this approach is continued, it would be good to add some comments about what this ID refers to and how developers should handle it (whether it is okay to use as is).
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.
Thanks for your review. To explain, I don't even know whose snapshot is that. We can find that in RDS console > Snapshot > Public and grab which ever compatible with our setting.
I was doing the same for integ.instance-from-cluster-snapshot.ts
, and now that snapshot has been deleted. Agree that this should be improved for future reader.
Also really appreciate your sharing on preDeploy
and hooks
, will definitely try it out for simpler test.
Detail comment on how to get the snapshot ID added.
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.
Thanks for the change. Confirmed it.
/*
* For simplicity, this integration test uses a public snapshot.
* By the time you rerun, the snapshot might already be deleted.
*
* How to get another compatible public snapshot:
* * aws rds describe-db-snapshots --include-public --snapshot-type public --query "DBSnapshots[?Engine=='mysql' && EngineVersion=='8.0.40']" --output table
*
* Or find one in AWS Console > RDS > Snapshots > Public
*/
However, as far as I can see, there are currently only four snapshots of the relevant engine in public. Furthermore, it is unclear who left them there and for what purpose. In other words, there is a possibility that there will be none when executing this integ in future.
In addition, if the snapshot referenced in the test is deleted and a new snapshot ID is specified, the instance will be replaced with the new one. Then it will cause a destructive change in the integ test. Also, if the original snapshot does not exist, an error will occur when creating a stack with the existing snapshot, causing the test to fail unless you specify --disable-update-workflow
or stackUpdateWorkflow: false
. Anyway, Tests should be as stable as possible.
If you specify a property that is different from the previous snapshot restore property, a new DB instance is restored from the specified DBSnapshotIdentifier property, and the original DB instance is deleted.
Therefore, I prefer the following approaches. How about trying different approaches?
How about using environment variables or any variables (such as process.env.CDK_INTEG_ACCOUNT || process.env.CDK_DEFAULT_ACCOUNT, stack.account, etc.)?
And it would be better to write a comment to need to create the resource and the snapshot manually before running the test. (Alternatively, we may also create them in preDeploy of hooks in IntegTest and delete them in postDestroy, but it may take a long time and be difficult.)Or, we could create a custom resource and create a snapshot of sourceInstance within it and return that. But it might be a flaky test so it might be good the first choice.
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.
Found an existing implementation of another integ, so I made it general for any later work requiring snapshot.
Also tried to do the same for integ.instance-from-cluster-snapshot
, but the problem is DB instance can only be restored from a MultiAZ DB cluster - which is currently not yet supported by L2 construct as L2 DBCluster only support Aurora. It is still possible with L1 and some modifications on the Snapshoter
. Please let me know if you want me to do so.
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.
I see. Then, let's revert the current change in packages/@aws-cdk-testing/framework-integ/test/aws-rds/test/integ.instance-from-cluster-snapshot.ts
. That change could be considered separately from this PR.
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.
Yeah sure thanks for understanding, integ.instance-from-cluster-snapshot
reverted
* | ||
* @default undefined - AWS RDS default setting is `EngineLifecycleSupport.OPEN_SOURCE_RDS_EXTENDED_SUPPORT` | ||
*/ | ||
readonly engineLifecycleSupport?: EngineLifecycleSupport; |
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.
This setting applies only to RDS for MySQL and RDS for PostgreSQL.
It would be good to write it in description or validate it if possible.
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.
Thank you for this, engine validation added.
Co-authored-by: Kenta Goto (k.goto) <[email protected]>
@@ -1079,6 +1090,11 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa | |||
this.engine = props.engine; | |||
|
|||
const engineType = props.engine.engineType; | |||
|
|||
if (props.engineLifecycleSupport && !['mysql', 'postgres'].includes(engineType)) { | |||
throw new ValidationError(`Engine '${engineType}' does not support engine lifecycle support`, this); |
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.
This would be more helpful.
throw new ValidationError(`Engine '${engineType}' does not support engine lifecycle support`, this); | |
throw new ValidationError(`'engineLifecycleSupport' can only be specified for RDS for MySQL and RDS for PostgreSQL, got: '${engineType}'`, this); |
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.
Thanks this is updated
@@ -1492,6 +1508,10 @@ export class DatabaseInstanceReadReplica extends DatabaseInstanceNew implements | |||
throw new ValidationError(`Cannot set 'backupRetention', as engine '${engineDescription(props.sourceDatabaseInstance.engine)}' does not support automatic backups for read replicas`, this); | |||
} | |||
|
|||
if (props.sourceDatabaseInstance.engine?.engineType && props.engineLifecycleSupport && !['mysql', 'postgres'].includes(props.sourceDatabaseInstance.engine.engineType)) { | |||
throw new ValidationError(`Engine '${props.sourceDatabaseInstance.engine.engineType}' does not support engine lifecycle support`, this); |
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.
Same as above
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.
Thanks this is updated
const engineType = props.sourceDatabaseInstance.engine?.engineType; | ||
if (engineType && props.engineLifecycleSupport && !['mysql', 'postgres'].includes(engineType)) { | ||
throw new ValidationError(`'engineLifecycleSupport' can only be specified for RDS for MySQL and RDS for PostgreSQL, got: '${engineType}'`, this); | ||
} | ||
|
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.
Could you please change the following together?
const instance = new CfnDBInstance(this, 'Resource', {
// ...
// ...
- engine: shouldPassEngine ? props.sourceDatabaseInstance.engine?.engineType : undefined,
+ engine: shouldPassEngine ? engineType : undefined,
// ...
// ...
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.
Sure this is updated
|
||
// For simplicity, get a public snapshot | ||
new rds.DatabaseInstanceFromSnapshot(stack, 'FromSnapshot', { | ||
snapshotIdentifier: 'arn:aws:rds:us-east-1:484907511898:snapshot:vuln-test-db-snapshot-prod', |
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.
I see. Then, let's revert the current change in packages/@aws-cdk-testing/framework-integ/test/aws-rds/test/integ.instance-from-cluster-snapshot.ts
. That change could be considered separately from this PR.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Thanks. Approved!
Thanks for your review @go-to-k 💯 |
Issue # (if applicable)
Closes #34492
Reason for this change
Cluster has this param but Instance doesn't
Description of changes
Describe any new or updated permissions being added
Description of how you validated changes
Unit + Integ
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license