(aws-docdb): setting up cluster with retention policy == snapshot breaks internal security group

[kornicameister]

Describe the bug

Setting up a cluster with removalPolicy: cdk.RemovalPolicy.SNAPSHOT fails to synth because ec2.SecurityGroup does not supoort said RemovalPolicy

Expected Behavior

cdk synth executed correctly.

Current Behavior

cdk synth fails with Error: AWS::EC2::SecurityGroup does not support snapshot removal policy

Reproduction Steps

#!/usr/bin/env node
import * as cdk from 'aws-cdk-lib';
import * as docDb from 'aws-cdk-lib/aws-docdb';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
import 'source-map-support/register';

const app = new cdk.App();
const env = {
  account: 'your account',
  region: 'your region',
};
const stack = new cdk.Stack(app, 'DocDB', {
  env,
});

new docDb.DatabaseCluster(stack, 'Cluster', {
  removalPolicy: cdk.RemovalPolicy.SNAPSHOT,
  masterUser: {
    username: 'test',
    password: cdk.SecretValue.unsafePlainText('test'),
  },
  instanceType: ec2.InstanceType.of(
    ec2.InstanceClass.T4G,
    ec2.InstanceSize.MEDIUM,
  ),
  vpc: ec2.Vpc.fromLookup(stack, 'VPC', { isDefault: true }),
});

Possible Solution

1. Fallback to retaining security group if document db has snapshot policy set
2. Do not set retention policy inside construct

Personally I prefer the 2nd idea, because setting a retention policy inside Construct does seem to be a bit of an odd idea.
I think setting up one with CDK defaults make more sense and if user wants to change the default it can always create a security group before hand and pass it as prop

Additional Information/Context

No response

CDK CLI Version

2.121.1

Framework Version

No response

Node.js Version

20.8

OS

MacOS sierra

Language

TypeScript

Language Version

5.3

Other information

No response

[kornicameister]

Huh...funny enough the problem is around DocDB.
Apparently it does not support delete with snapshot.

I guess it's my bad on that part.

However, I would still bring up to discussion a part where removal policy of group is set via escape hatch

[pahud]

Yes I think we should fix here to avoid this error.

aws-cdk/packages/aws-cdk-lib/aws-docdb/lib/cluster.ts

Lines 423 to 427 in 45b8398

	// HACK: Use an escape-hatch to apply a consistent removal policy to the
	// security group so we don't get errors when trying to delete the stack
	(securityGroup.node.defaultChild as CfnResource).applyRemovalPolicy(props.removalPolicy, {
	applyToUpdateReplacePolicy: true,
	});

[kornicameister]

Would it work to jest get rid of escape hatch?

[kornicameister]

@pahud would you mind checking on the linked PR from @lpizzinidev.
There's a discussion that tackles the problem of using an escape and in general downgrading removal policies if L2 component's children do not support particular removal policy i.e.

1. we spin up DocumentDBCluster with snapshot removal policy
2. we downgrade removal policy for security group to destroy because it does not support snapshot

In short, there's a bit of a pickle between @lpizzinidev and myself :-) questioning the design of downgrading policies without explicit intention of a user/caller that creates given L2 construct.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.