RDS: DatabaseClusterFromSnapshot should allow setting copyTagsToSnapshot

[blimmer]

Describe the bug

Today, the copyTagsToSnapshot property exists on DatabaseClusterProps not DatabaseClusterBaseProps:

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

Lines 497 to 511 in 789e8d2

	export interface DatabaseClusterProps extends DatabaseClusterBaseProps {
	/**
	* Credentials for the administrative user
	*
	* @default - A username of 'admin' (or 'postgres' for PostgreSQL) and SecretsManager-generated password
	*/
	readonly credentials?: Credentials;
	/**
	* Whether to copy tags to the snapshot when a snapshot is created.
	*
	* @default: true
	*/
	readonly copyTagsToSnapshot?: boolean;
	}

It's also not set in the DatabaseClusterFromSnapshot construct, so if you switch a DatabaseCluster to DatabaseClusterFromSnapshot, you'll get this diff:

[~] AWS::RDS::DBCluster CdkPipelinesPrototype-dev-us-east-1/Database/Database/cdk-pipeline-example-dev-Cluster DatabasecdkpipelineexampledevClusterC6927963 replace
 ├─ [-] CopyTagsToSnapshot
 │   └─ true
<trimmed>

This is especially noticeable because the value defaults to true in the L2 construct, which is different from the CloudFormation default (false):

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

Lines 505 to 510 in 789e8d2

	/**
	* Whether to copy tags to the snapshot when a snapshot is created.
	*
	* @default: true
	*/
	readonly copyTagsToSnapshot?: boolean;

Unlike other values that are explicitly unset when using DatabaseClusterFromSnapshot (e.g., KmsKeyId, MasterUsername, MasterUserPassword - see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbcluster.html#cfn-rds-dbcluster-kmskeyid), the docs for CopyTagsToSnapshot don't specify that you shouldn't set the value when restoring from a snapshot.

Expected Behavior

I expected to be able to switch from a DatabaseCluster to a DatabaseClusterFromSnapshot while retaining the same default behaviors.

Current Behavior

Currently, the CopyTagsToSnapshot value shows as being unset (which will change to the CloudFormation default of false) when changing from a DatabaseCluster to DatabaseClusterFromSnapshot.

Reproduction Steps

https://github.com/blimmer/cdk-bug-reports/compare/rds-copy-tags-to-snapshot?expand=1

In this simple stack:

import { Stack, StackProps } from 'aws-cdk-lib';
import { Vpc } from 'aws-cdk-lib/aws-ec2';
import { AuroraPostgresEngineVersion, DatabaseCluster, DatabaseClusterEngine, DatabaseClusterFromSnapshot } from 'aws-cdk-lib/aws-rds';
import { Construct } from 'constructs';

export class CdkBugReportsStack extends Stack {
  constructor(scope: Construct, id: string, props?: StackProps) {
    super(scope, id, props);

    const vpc = new Vpc(this, 'VPC')
    new DatabaseCluster(this, 'DatabaseCluster', {
      engine: DatabaseClusterEngine.auroraPostgres({ version: AuroraPostgresEngineVersion.VER_13_4 }),
      instanceProps: {
        vpc
      }
    })

    new DatabaseClusterFromSnapshot(this, 'DatabaseClusterFromSnapshot', {
      snapshotIdentifier: 'arn:aws:rds:us-east-1:12345678910:cluster-snapshot:rds:example-2022-04-12-09-26',
      engine: DatabaseClusterEngine.auroraPostgres({ version: AuroraPostgresEngineVersion.VER_13_4 }),
      instanceProps: {
        vpc
      }
    })
  }
}

The resource with the id DatabaseCluster has CopyTagsToSnapshot set to true (the default in the L2 construct):

"DatabaseCluster68FC2945": {
  "Type": "AWS::RDS::DBCluster",
  "Properties": {
    "Engine": "aurora-postgresql",
    "CopyTagsToSnapshot": true,

However, the one restored from the snapshot does not have this value set:

"DatabaseClusterFromSnapshotB2BFF6A0": {
  "Type": "AWS::RDS::DBCluster",
  "Properties": {
    "Engine": "aurora-postgresql",
	// CopyTagsToSnapshot is missing

Possible Solution

I think copyTagsToSnapshot should be moved up to DatabaseClusterBaseProps and should default to true for DatabaseClusterFromSnapshot for consistency.

Additional Information/Context

You can work around this temporarily by setting the value on the L1 construct, e.g.:

const db = new DatabaseClusterFromSnapshot(this, 'DatabaseClusterFromSnapshot', {
  snapshotIdentifier: 'arn:aws:rds:us-east-1:12345678910:cluster-snapshot:rds:example-2022-04-12-09-26',
  engine: DatabaseClusterEngine.auroraPostgres({ version: AuroraPostgresEngineVersion.VER_13_4 }),
  instanceProps: {
    vpc
  }
});
(db.node.defaultChild as CfnDBCluster).copyTagsToSnapshot = true;

CDK CLI Version

2.20.0

Framework Version

No response

Node.js Version

16.14.0

OS

macOS

Language

Typescript

Language Version

No response

Other information

No response

[skinny85]

Thanks for opening the issue @blimmer!

But I wonder: does this setting make sense on DatabaseClusterFromSnapshot? Can this Database also create a snapshot?

Thanks,
Adam

[blimmer]

@skinny85 I believe so, yes! The new DatabaseClusterFromSnapshot is just a regular ol' DatabaseCluster and will produce snapshots according to the backup configuration. You can also take manual snapshots of the restored cluster, as well.

[skinny85]

OK, so I guess this is just a matter of moving copyTagsToSnapshot from DatabaseClusterProps to DatabaseClusterBaseProps, and then the logic of using it from DatabaseCluster here to DatabaseClusterNew (somewhere here most likely)?

[blimmer]

Yep, that sounds like how I would think about solving it, too.

[skinny85]

We encourage community contributions, so if you'd like to open us a Pull Request adding this feature, that would be fantastic! 😉 Our "Contributing" guide: https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md.

[AnuragMohapatra]

Hi,

I would like to pick this change and help in implementing it, will open a PR with the solution soon.

Let me know in case of any concerns.

[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.