fix(rds): standardize removal policies and deletion protection (#10412)

Currently, database instances and clusters have different behaviors for
removal policies and deletion protection. This fix standardizes the behavior
and logical code paths so RDS behaves consistently.

The new logic is that we will only set `deletionProtection` if the RemovalPolicy
has been set to RETAIN. Otherwise (with SNAPSHOT or DELETE), deletion protection
will be disabled (by default).

BREAKING CHANGE: Cluster now has deletionProtection enabled if its removal policy is `RETAIN`
* **rds**: Instance now has deletionProtection enabled by default only if its removal policy is `RETAIN`

----

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

packages/@aws-cdk/aws-rds/lib/cluster.ts

@@ -4,13 +4,13 @@ import * as kms from '@aws-cdk/aws-kms';
 import * as logs from '@aws-cdk/aws-logs';
 import * as s3 from '@aws-cdk/aws-s3';
 import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
-import { Annotations, CfnDeletionPolicy, Construct, Duration, RemovalPolicy, Resource, Token } from '@aws-cdk/core';
+import { Annotations, Construct, Duration, RemovalPolicy, Resource, Token } from '@aws-cdk/core';
 import { IClusterEngine } from './cluster-engine';
 import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref';
 import { DatabaseSecret } from './database-secret';
 import { Endpoint } from './endpoint';
 import { IParameterGroup } from './parameter-group';
-import { setupS3ImportExport } from './private/util';
+import { applyRemovalPolicy, defaultDeletionProtection, setupS3ImportExport } from './private/util';
 import { BackupProps, InstanceProps, Login, PerformanceInsightRetention, RotationMultiUserOptions } from './props';
 import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
 import { CfnDBCluster, CfnDBClusterProps, CfnDBInstance, CfnDBSubnetGroup } from './rds.generated';
@@ -82,7 +82,7 @@ interface DatabaseClusterBaseProps {
   /**
    * Indicates whether the DB cluster should have deletion protection enabled.
    *
-   * @default false
+   * @default - true if ``removalPolicy`` is RETAIN, false otherwise
    */
   readonly deletionProtection?: boolean;
 
@@ -335,7 +335,7 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
       port: props.port ?? clusterEngineBindConfig.port,
       dbClusterParameterGroupName: clusterParameterGroupConfig?.parameterGroupName,
       associatedRoles: clusterAssociatedRoles.length > 0 ? clusterAssociatedRoles : undefined,
-      deletionProtection: props.deletionProtection,
+      deletionProtection: defaultDeletionProtection(props.deletionProtection, props.removalPolicy),
       // Admin
       backupRetentionPeriod: props.backup?.retention?.toDays(),
       preferredBackupWindow: props.backup?.preferredWindow,
@@ -344,19 +344,6 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
       enableCloudwatchLogsExports: props.cloudwatchLogsExports,
     };
   }
-
-  protected setRemovalPolicy(cluster: CfnDBCluster, removalPolicy?: RemovalPolicy) {
-    // if removalPolicy was not specified,
-    // leave it as the default, which is Snapshot
-    if (removalPolicy) {
-      cluster.applyRemovalPolicy(removalPolicy);
-    } else {
-      // The CFN default makes sense for DeletionPolicy,
-      // but doesn't cover UpdateReplacePolicy.
-      // Fix that here.
-      cluster.cfnOptions.updateReplacePolicy = CfnDeletionPolicy.SNAPSHOT;
-    }
-  }
 }
 
 /**
@@ -515,7 +502,7 @@ export class DatabaseCluster extends DatabaseClusterNew {
       defaultPort: ec2.Port.tcp(this.clusterEndpoint.port),
     });
 
-    this.setRemovalPolicy(cluster, props.removalPolicy);
+    applyRemovalPolicy(cluster, props.removalPolicy);
 
     if (secret) {
       this.secret = secret.attach(this);
@@ -613,7 +600,7 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew {
       defaultPort: ec2.Port.tcp(this.clusterEndpoint.port),
     });
 
-    this.setRemovalPolicy(cluster, props.removalPolicy);
+    applyRemovalPolicy(cluster, props.removalPolicy);
 
     setLogRetention(this, props);
     createInstances(this, props, this.subnetGroup);
@@ -720,7 +707,7 @@ function createInstances(cluster: DatabaseClusterNew, props: DatabaseClusterBase
     // Because of that, in this case,
     // we can safely use the CFN default of Delete for DbInstances with dbClusterIdentifier set.
     if (props.removalPolicy) {
-      instance.applyRemovalPolicy(props.removalPolicy);
+      applyRemovalPolicy(instance, props.removalPolicy);
     }
 
     // We must have a dependency on the NAT gateway provider here to create

packages/@aws-cdk/aws-rds/lib/instance.ts

@@ -5,13 +5,13 @@ import * as kms from '@aws-cdk/aws-kms';
 import * as logs from '@aws-cdk/aws-logs';
 import * as s3 from '@aws-cdk/aws-s3';
 import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
-import { CfnDeletionPolicy, Construct, Duration, IResource, Lazy, RemovalPolicy, Resource, SecretValue, Stack, Token } from '@aws-cdk/core';
+import { Construct, Duration, IResource, Lazy, RemovalPolicy, Resource, SecretValue, Stack, Token } from '@aws-cdk/core';
 import { DatabaseSecret } from './database-secret';
 import { Endpoint } from './endpoint';
 import { IInstanceEngine } from './instance-engine';
 import { IOptionGroup } from './option-group';
 import { IParameterGroup } from './parameter-group';
-import { engineDescription, setupS3ImportExport } from './private/util';
+import { applyRemovalPolicy, defaultDeletionProtection, engineDescription, setupS3ImportExport } from './private/util';
 import { PerformanceInsightRetention, RotationMultiUserOptions } from './props';
 import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
 import { CfnDBInstance, CfnDBInstanceProps, CfnDBSubnetGroup } from './rds.generated';
@@ -468,7 +468,7 @@ export interface DatabaseInstanceNewProps {
   /**
    * Indicates whether the DB instance should have deletion protection enabled.
    *
-   * @default true
+   * @default - true if ``removalPolicy`` is RETAIN, false otherwise
    */
   readonly deletionProtection?: boolean;
 
@@ -626,7 +626,6 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData
       });
     }
 
-    const deletionProtection = props.deletionProtection !== undefined ? props.deletionProtection : true;
     const storageType = props.storageType || StorageType.GP2;
     const iops = storageType === StorageType.IO1 ? (props.iops || 1000) : undefined;
 
@@ -660,7 +659,7 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData
       dbInstanceIdentifier: props.instanceIdentifier,
       dbSubnetGroupName: subnetGroup.ref,
       deleteAutomatedBackups: props.deleteAutomatedBackups,
-      deletionProtection,
+      deletionProtection: defaultDeletionProtection(props.deletionProtection, props.removalPolicy),
       enableCloudwatchLogsExports: this.cloudwatchLogsExports,
       enableIamDatabaseAuthentication: Lazy.anyValue({ produce: () => this.enableIamAuthentication }),
       enablePerformanceInsights: enablePerformanceInsights || props.enablePerformanceInsights, // fall back to undefined if not set,
@@ -956,7 +955,7 @@ export class DatabaseInstance extends DatabaseInstanceSource implements IDatabas
     const portAttribute = Token.asNumber(instance.attrEndpointPort);
     this.instanceEndpoint = new Endpoint(instance.attrEndpointAddress, portAttribute);
 
-    applyInstanceDeletionPolicy(instance, props.removalPolicy);
+    applyRemovalPolicy(instance, props.removalPolicy);
 
     if (secret) {
       this.secret = secret.attach(this);
@@ -1052,7 +1051,7 @@ export class DatabaseInstanceFromSnapshot extends DatabaseInstanceSource impleme
     const portAttribute = Token.asNumber(instance.attrEndpointPort);
     this.instanceEndpoint = new Endpoint(instance.attrEndpointAddress, portAttribute);
 
-    applyInstanceDeletionPolicy(instance, props.removalPolicy);
+    applyRemovalPolicy(instance, props.removalPolicy);
 
     if (secret) {
       this.secret = secret.attach(this);
@@ -1127,7 +1126,7 @@ export class DatabaseInstanceReadReplica extends DatabaseInstanceNew implements
     const portAttribute = Token.asNumber(instance.attrEndpointPort);
     this.instanceEndpoint = new Endpoint(instance.attrEndpointAddress, portAttribute);
 
-    applyInstanceDeletionPolicy(instance, props.removalPolicy);
+    applyRemovalPolicy(instance, props.removalPolicy);
 
     this.setLogRetention();
   }
@@ -1143,14 +1142,3 @@ function renderProcessorFeatures(features: ProcessorFeatures): CfnDBInstance.Pro
 
   return featuresList.length === 0 ? undefined : featuresList;
 }
-
-function applyInstanceDeletionPolicy(cfnDbInstance: CfnDBInstance, removalPolicy: RemovalPolicy | undefined): void {
-  if (!removalPolicy) {
-    // the default DeletionPolicy is 'Snapshot', which is fine,
-    // but we should also make it 'Snapshot' for UpdateReplace policy
-    cfnDbInstance.cfnOptions.updateReplacePolicy = CfnDeletionPolicy.SNAPSHOT;
-  } else {
-    // just apply whatever removal policy the customer explicitly provided
-    cfnDbInstance.applyRemovalPolicy(removalPolicy);
-  }
-}

packages/@aws-cdk/aws-rds/lib/private/util.ts

@@ -1,6 +1,6 @@
 import * as iam from '@aws-cdk/aws-iam';
 import * as s3 from '@aws-cdk/aws-s3';
-import { Construct } from '@aws-cdk/core';
+import { Construct, CfnDeletionPolicy, CfnResource, RemovalPolicy } from '@aws-cdk/core';
 import { IInstanceEngine } from '../instance-engine';
 
 /** Common base of `DatabaseInstanceProps` and `DatabaseClusterBaseProps` that has only the S3 props */
@@ -59,3 +59,24 @@ export function setupS3ImportExport(
 export function engineDescription(engine: IInstanceEngine) {
   return engine.engineType + (engine.engineVersion?.fullVersion ? `-${engine.engineVersion.fullVersion}` : '');
 }
+
+export function applyRemovalPolicy(cfnDatabase: CfnResource, removalPolicy?: RemovalPolicy): void {
+  if (!removalPolicy) {
+    // the default DeletionPolicy is 'Snapshot', which is fine,
+    // but we should also make it 'Snapshot' for UpdateReplace policy
+    cfnDatabase.cfnOptions.updateReplacePolicy = CfnDeletionPolicy.SNAPSHOT;
+  } else {
+    // just apply whatever removal policy the customer explicitly provided
+    cfnDatabase.applyRemovalPolicy(removalPolicy);
+  }
+}
+
+/**
+ * By default, deletion protection is disabled.
+ * Enable if explicitly provided or if the RemovalPolicy has been set to RETAIN
+ */
+export function defaultDeletionProtection(deletionProtection?: boolean, removalPolicy?: RemovalPolicy): boolean | undefined {
+  return deletionProtection !== undefined
+    ? deletionProtection
+    : (removalPolicy === RemovalPolicy.RETAIN ? true : undefined);
+}

packages/@aws-cdk/aws-rds/test/integ.instance-s3.expected.json

@@ -569,7 +569,6 @@
         "DBSubnetGroupName": {
           "Ref": "DatabaseSubnetGroup7D60F180"
         },
-        "DeletionProtection": true,
         "Engine": "sqlserver-se",
         "EngineVersion": "14.00.3192.2.v1",
         "LicenseModel": "license-included",

packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json

@@ -636,7 +636,6 @@
         "DBSubnetGroupName": {
           "Ref": "InstanceSubnetGroupF2CBA54F"
         },
-        "DeletionProtection": true,
         "EnableCloudwatchLogsExports": [
           "trace",
           "audit",
@@ -1122,4 +1121,4 @@
       "Description": "Artifact hash for asset \"74a1cab76f5603c5e27101cb3809d8745c50f708b0f4b497ed0910eb533d437b\""
     }
   }
-}
+}

packages/@aws-cdk/aws-rds/test/integ.proxy.expected.json

@@ -428,7 +428,6 @@
         "DBSubnetGroupName": {
           "Ref": "dbInstanceSubnetGroupD062EC9E"
         },
-        "DeletionProtection": true,
         "Engine": "postgres",
         "MasterUsername": {
           "Fn::Join": [

packages/@aws-cdk/aws-rds/test/test.instance.ts

@@ -57,7 +57,6 @@ export = {
         DBSubnetGroupName: {
           Ref: 'InstanceSubnetGroupF2CBA54F',
         },
-        DeletionProtection: true,
         EnableCloudwatchLogsExports: [
           'trace',
           'audit',