feat(rds): change the default retention policy of Cluster and DB Instance

[skinny85]

Commit Message

feat(rds): change the default retention policy of Cluster and DB Instance

The 'Snapshot' retention policy is a special one used only for RDS.
It deletes the underlying resource, but before doing that,
creates a snapshot of it, so that the data is not lost.
Use the 'Snapshot' policy instead of 'Retain',
for the DatabaseCluster and DbInstance resources.

Fixes #3298

BREAKING CHANGE: the default retention policy for RDS Cluster and DbInstance is now 'Snapshot'

End Commit Message

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[skinny85]

@jogold I would love to get your opinion on this change as well!

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 419dd37
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[jogold]

@jogold I would love to get your opinion on this change as well!

Looks good. I just wonder if this change would better fit in core/cfn-resource.ts as SNAPSHOT is supported also by other resources (ec2 volumes, elasticache, neptune and redshift)?

aws-cdk/packages/@aws-cdk/core/lib/cfn-resource.ts

Lines 102 to 105 in 6f75f22

	/**
	* Sets the deletion policy of the resource based on the removal policy specified.
	*/
	public applyRemovalPolicy(policy: RemovalPolicy | undefined, options: RemovalPolicyOptions = {}) {

[skinny85]

@jogold thanks, you're probably right. Let me try to add the changes needed in @aws-cdk/core to this PR.

I wanted to also ask you about the behavior for DbInstance's retention policies. This is what the CloudFormation documentation says on the subject:

1. For AWS::RDS::DBInstance resources that don't specify the DBClusterIdentifier property, AWS CloudFormation saves a snapshot of the DB instance.

2. For AWS::RDS::DBInstance resources that do specify the DBClusterIdentifier property, AWS CloudFormation deletes the DB instance.

However, in the current PR, I actually change that behavior: in the DatabaseCluster construct, I assign Snapshot retention to the created DbInstance resources, even though they all have the dbClusterIdentifier filled. Same in the DatabaseInstance construct: if the removalPolicy was not passed explicitly in the props, I set it always to Snapshot.

What is your opinion on this? Do the changes make sense to you, or should we instead by default simply have the same behavior CloudFormation has?

[jogold]

Cluster

• For the cluster you are going back to the CF default, which is Snapshot
• For the instances you are forcing to Snapshot but you could do the same as for the cluster?

Stand-alone instance

You are going from forcing Retain to forcing Snapshot if unspecified.

For me Snapshot is as safe as Retain but cheaper. It's not identical in terms of availability but I think it's a good compromise.

[skinny85]

@jogold thanks for the response. Some clarifications below:

Cluster

• For the cluster you are going back to the CF default, which is Snapshot

Yes.

• For the instances you are forcing to Snapshot but you could do the same as for the cluster?

For instances created inside DatabaseCluster, we are forcing Snapshot, which is not the CloudFormation default (because the AWS::RDS::DBInstance resources created inside DatabaseCluster have the DBClusterIdentifier property always filled, the CloudFormation default for them is actually Delete, like I mentioned here: #8023 (comment) ).

Stand-alone instance

You are going from forcing Retain to forcing Snapshot if unspecified.

For me Snapshot is as safe as Retain but cheaper. It's not identical in terms of availability but I think it's a good compromise.

Right. Here's my question then:

• I'm always forcing Snapshot for DatabaseInstance, even if they have the dBClusterIdentifier property filled. Is that correct? Is it even possible for DatabaseInstance that is not created inside DatabaseCluster to have the dBClusterIdentifier property set? (I can't easily tell from the code whether that's possible or not)

Would appreciate your thoughts on this topic!

[jogold]

• For the instances you are forcing to Snapshot but you could do the same as for the cluster?

For instances created inside DatabaseCluster, we are forcing Snapshot, which is not the CloudFormation default (because the AWS::RDS::DBInstance resources created inside DatabaseCluster have the DBClusterIdentifier property always filled, the CloudFormation default for them is actually Delete, like I mentioned here: #8023 (comment) ).

Sorry for the confusion, the same here meant going back to CF default. Since we have Snapshot on the cluster, I'm not sure we need it on the instance. But let me check that again.

Is it even possible for DatabaseInstance that is not created inside DatabaseCluster to have the dBClusterIdentifier property set?

It's not possible for a DatabaseInstance (L2) to have the dBClusterIdentifier property. Instances created in a cluster use the L1 CfnDBInstance. This actually means that we could use the CF default for a DatabaseInstance. But looking again at the doc now I see that those defaults apply only to the DeletionPolicy and not to the UpdateReplacePolicy. For stand alone instances this is critical because a change in any property that requires a replacement could lead to data loss. The right solution for stand-alone instances seems to be: use CF default for DeletionPolicy (= Snapshot), force Snapshot for UpdateReplacePolicy. The core method applyRemovalPolicy currently doesn't handle this case.

[skinny85]

@jogold @nija-at thanks a lot for your comments. I've incorporated all of your feedback in the newest revision. Please re-review when you have the chance!

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 5a32b1a
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[nija-at]

Should this be a 'feat' change, i.e, do we want this listed in the 'Features' section of the changelog? I usually mark these as 'chore'. (breaking changes callout will be picked up either way)

Added 'do-not-merge' to address some comments here.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: 0351e7c
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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