(rds): instanceUpdateBehaviour is broken with writers/readers configuration

[blimmer]

Describe the bug

According to the documentation, I should be able to use the instanceUpdateBehaviour setting with the new writers/readers (e.g., not instanceProps) configuration:

However, my cdk diff shows that even though the setting is still set, the dependency is removed (effectively undoing the ROLLING update setting):

Stack Database
Resources
[~] AWS::RDS::DBInstance db/Instance2 dbInstance2648805E1
 └─ [-] DependsOn
     └─ ["dbInstance1FD14D62F"]

Looking at the source code, instanceUpdateBehaviour is only paid attention to in the legacyCreateInstances method, called when using instanceProps

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

Line 1233 in 22a3234

function legacyCreateInstances(cluster: DatabaseClusterNew, props: DatabaseClusterBaseProps, subnetGroup: ISubnetGroup): InstanceConfig {

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

Lines 1330 to 1335 in 22a3234

	// Adding dependencies here to ensure that the instances are updated one after the other.
	if (instanceUpdateBehaviour === InstanceUpdateBehaviour.ROLLING) {
	for (let i = 1; i < instanceCount; i++) {
	instances[i].node.addDependency(instances[i-1]);
	}
	}

When using writers/readers, this parameter is ignored

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

Line 634 in 22a3234

protected _createInstances(cluster: DatabaseClusterNew, props: DatabaseClusterProps): InstanceConfig {

Expected Behavior

I expected the instanceUpdateBehaviour to work still as I transitioned from instanceProps to the new API. The property is not marked as @deprecated and the documentation explicitly shows an example of it working with writers/readers.

Current Behavior

The CloudFormation dependency is marked for removal in the cdk diff when I transition from instanceProps to the new writers/readers API:

Stack Database
Resources
[~] AWS::RDS::DBInstance db/Instance2 dbInstance2648805E1
 └─ [-] DependsOn
     └─ ["dbInstance1FD14D62F"]

Reproduction Steps

To show this issue, you can cdk synth with the instanceBehaviour specified and not specified. The CFN produced is exactly the same.

Given,

1. Create this stack in a new CDK repo:

   import * as cdk from 'aws-cdk-lib';
   import * as rds from 'aws-cdk-lib/aws-rds';
   import * as ec2 from 'aws-cdk-lib/aws-ec2';
   import { Construct } from 'constructs';
   
   export class DatabaseStack extends cdk.Stack {
     constructor(scope: Construct, id: string, props?: cdk.StackProps) {
       super(scope, id, props);
   
       const vpc = new ec2.Vpc(this, 'VPC');
       new rds.DatabaseCluster(this, 'Database', {
         engine: rds.DatabaseClusterEngine.auroraMysql({ version: rds.AuroraMysqlEngineVersion.VER_3_01_0 }),
         writer: rds.ClusterInstance.provisioned('Instance1' ),
         readers: [rds.ClusterInstance.provisioned('Instance2')],
         instanceUpdateBehaviour: rds.InstanceUpdateBehaviour.ROLLING,
         vpc,
       });
     }
   }

2. Produce a template and save it off

   > yarn cdk synth > instance-behaviour-set.yml

3. Update the stack to not set instanceUpdateBehaviour:

   import * as cdk from 'aws-cdk-lib';
   import * as rds from 'aws-cdk-lib/aws-rds';
   import * as ec2 from 'aws-cdk-lib/aws-ec2';
   import { Construct } from 'constructs';
   
   export class DatabaseStack extends cdk.Stack {
     constructor(scope: Construct, id: string, props?: cdk.StackProps) {
       super(scope, id, props);
   
       const vpc = new ec2.Vpc(this, 'VPC');
       new rds.DatabaseCluster(this, 'Database', {
         engine: rds.DatabaseClusterEngine.auroraMysql({ version: rds.AuroraMysqlEngineVersion.VER_3_01_0 }),
         writer: rds.ClusterInstance.provisioned('Instance1' ),
         readers: [rds.ClusterInstance.provisioned('Instance2')],
         // instanceUpdateBehaviour: rds.InstanceUpdateBehaviour.ROLLING, <- CHANGED
         vpc,
       });
     }
   }

4. Produce another template

   > yarn cdk synth > instance-behaviour-not-set.yml

5. Diff the two templates:

   > diff instance-behaviour-set.yml instance-behaviour-not-set.yml
   571c571
   < Done in 2.75s.
   ---
   > Done in 2.28s

Note that the instanceUpdateBehaviour property has no effect on the resulting template.

Possible Solution

1. Add the instance update behavior logic to the new _createInstances method used by the writers/readers API.
2. If instanceUpdateBehaviour does not make sense with the new API, mark it as deprecated and remove the example (linked above) from the documentation.

Additional Information/Context

No response

CDK CLI Version

2.103.0 (build d0d7547)

Framework Version

No response

Node.js Version

18.18.2

OS

MacOS

Language

TypeScript

Language Version

No response

Other information

No response

[pahud]

I believe instanceUpdateBehaviour is just temporary for transitioning to serverless v2 support and should be marked as deprecated in future versions.

[blimmer]

@pahud - I'm not sure that it's only useful in moving to Serverless V2 support. I think that when you're using standard provisioned instances, it's useful to support the ROLLING update behavior. This ensures that if instances need to be replaced, they don't all go down at once (BULK update).

Based on @spanierm42's original PR and the original issue #10595 filed by @hixi-hyi , this implementation didn't really even consider Serverless v2 when created.

Based on that, I think this issue might be best considered a regression with the new writer/readers API? What do you all think?