From 23f69a56fe3842e8d8e3c811e6635cd9f8255eb0 Mon Sep 17 00:00:00 2001 From: BLasan Date: Thu, 25 Mar 2021 14:35:03 +0530 Subject: [PATCH 1/4] fix(rds): fail with a descriptive error if Clusters instance count is a deploy-time value fixes #13558 --- packages/@aws-cdk/aws-rds/lib/cluster.ts | 3 +++ .../@aws-cdk/aws-rds/test/cluster.test.ts | 20 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/packages/@aws-cdk/aws-rds/lib/cluster.ts b/packages/@aws-cdk/aws-rds/lib/cluster.ts index a3f06555afdb9..24bb5944b8a85 100644 --- a/packages/@aws-cdk/aws-rds/lib/cluster.ts +++ b/packages/@aws-cdk/aws-rds/lib/cluster.ts @@ -651,6 +651,9 @@ interface InstanceConfig { */ function createInstances(cluster: DatabaseClusterNew, props: DatabaseClusterBaseProps, subnetGroup: ISubnetGroup): InstanceConfig { const instanceCount = props.instances != null ? props.instances : 2; + if (Token.isUnresolved(instanceCount)) { + throw new Error('The number of instances a RDS Cluster consists of cannot be provided as a deploy-time only value!'); + } if (instanceCount < 1) { throw new Error('At least one instance is required'); } diff --git a/packages/@aws-cdk/aws-rds/test/cluster.test.ts b/packages/@aws-cdk/aws-rds/test/cluster.test.ts index 74f538be5d998..80c2603973a25 100644 --- a/packages/@aws-cdk/aws-rds/test/cluster.test.ts +++ b/packages/@aws-cdk/aws-rds/test/cluster.test.ts @@ -54,6 +54,15 @@ describe('cluster', () => { }); + test('check instanceCount is a token', () => { + const stack = testStack(); + const vpc = new ec2.Vpc(stack, 'VPC'); + expect(() => { + createRDSCluster(stack, vpc); + }).toThrow('The number of instances a RDS Cluster consists of cannot be provided as a deploy-time only value!'); + + }); + test('can create a cluster with a single instance', () => { // GIVEN const stack = testStack(); @@ -1959,3 +1968,14 @@ function testStack(app?: cdk.App) { stack.node.setContext('availability-zones:12345:us-test-1', ['us-test-1a', 'us-test-1b']); return stack; } + +function createRDSCluster(stack: any, vpc: any) { + new DatabaseCluster(stack, 'Database', { + instances: cdk.Token.asNumber('1'), + engine: DatabaseClusterEngine.AURORA, + instanceProps: { + instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL), + vpc, + }, + }); +} From 2aa662553ab179e82396bd0fce5f1f94c300c2d4 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Thu, 25 Mar 2021 16:47:32 -0700 Subject: [PATCH 2/4] Don't use a helper function in the test --- .../@aws-cdk/aws-rds/test/cluster.test.ts | 65 ++++--------------- 1 file changed, 12 insertions(+), 53 deletions(-) diff --git a/packages/@aws-cdk/aws-rds/test/cluster.test.ts b/packages/@aws-cdk/aws-rds/test/cluster.test.ts index 80c2603973a25..a26f861df0977 100644 --- a/packages/@aws-cdk/aws-rds/test/cluster.test.ts +++ b/packages/@aws-cdk/aws-rds/test/cluster.test.ts @@ -50,17 +50,23 @@ describe('cluster', () => { DeletionPolicy: 'Delete', UpdateReplacePolicy: 'Delete', }, ResourcePart.CompleteDefinition); - - }); - test('check instanceCount is a token', () => { + test('validates that the number of instances is not a deploy-time value', () => { const stack = testStack(); const vpc = new ec2.Vpc(stack, 'VPC'); - expect(() => { - createRDSCluster(stack, vpc); - }).toThrow('The number of instances a RDS Cluster consists of cannot be provided as a deploy-time only value!'); + const parameter = new cdk.CfnParameter(stack, 'Param'); + expect(() => { + new DatabaseCluster(stack, 'Database', { + instances: parameter.valueAsNumber, + engine: DatabaseClusterEngine.AURORA, + instanceProps: { + instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL), + vpc, + }, + }); + }).toThrow('The number of instances an RDS Cluster consists of cannot be provided as a deploy-time only value!'); }); test('can create a cluster with a single instance', () => { @@ -90,8 +96,6 @@ describe('cluster', () => { MasterUserPassword: 'tooshort', VpcSecurityGroupIds: [{ 'Fn::GetAtt': ['DatabaseSecurityGroup5C91FDCB', 'GroupId'] }], }); - - }); test('can create a cluster with imported vpc and security group', () => { @@ -125,8 +129,6 @@ describe('cluster', () => { MasterUserPassword: 'tooshort', VpcSecurityGroupIds: ['SecurityGroupId12345'], }); - - }); test('cluster with parameter group', () => { @@ -159,8 +161,6 @@ describe('cluster', () => { expect(stack).toHaveResource('AWS::RDS::DBCluster', { DBClusterParameterGroupName: { Ref: 'ParamsA8366201' }, }); - - }); test("sets the retention policy of the SubnetGroup to 'Retain' if the Cluster is created with 'Retain'", () => { @@ -181,8 +181,6 @@ describe('cluster', () => { DeletionPolicy: 'Retain', UpdateReplacePolicy: 'Retain', }, ResourcePart.CompleteDefinition); - - }); test('creates a secret when master credentials are not specified', () => { @@ -239,8 +237,6 @@ describe('cluster', () => { SecretStringTemplate: '{"username":"admin"}', }, }); - - }); test('create an encrypted cluster with custom KMS key', () => { @@ -270,8 +266,6 @@ describe('cluster', () => { ], }, }); - - }); test('cluster with instance parameter group', () => { @@ -303,8 +297,6 @@ describe('cluster', () => { Ref: 'ParameterGroup5E32DECB', }, }); - - }); describe('performance insights', () => { @@ -332,8 +324,6 @@ describe('cluster', () => { PerformanceInsightsRetentionPeriod: 731, PerformanceInsightsKMSKeyId: { 'Fn::GetAtt': ['Key961B73FD', 'Arn'] }, }); - - }); test('setting performance insights fields enables performance insights', () => { @@ -357,8 +347,6 @@ describe('cluster', () => { EnablePerformanceInsights: true, PerformanceInsightsRetentionPeriod: 731, }); - - }); test('throws if performance insights fields are set but performance insights is disabled', () => { @@ -379,8 +367,6 @@ describe('cluster', () => { }, }); }).toThrow(/`enablePerformanceInsights` disabled, but `performanceInsightRetention` or `performanceInsightEncryptionKey` was set/); - - }); }); @@ -401,8 +387,6 @@ describe('cluster', () => { expect(stack).toHaveResource('AWS::RDS::DBInstance', { AutoMinorVersionUpgrade: false, }); - - }); test('cluster with allow upgrade of major version', () => { @@ -422,8 +406,6 @@ describe('cluster', () => { expect(stack).toHaveResourceLike('AWS::RDS::DBInstance', { AllowMajorVersionUpgrade: true, }); - - }); test('cluster with disallow remove backups', () => { @@ -443,8 +425,6 @@ describe('cluster', () => { expect(stack).toHaveResourceLike('AWS::RDS::DBInstance', { DeleteAutomatedBackups: false, }); - - }); test('create a cluster using a specific version of MySQL', () => { @@ -471,8 +451,6 @@ describe('cluster', () => { Engine: 'aurora-mysql', EngineVersion: '5.7.mysql_aurora.2.04.4', }); - - }); test('create a cluster using a specific version of Postgresql', () => { @@ -522,8 +500,6 @@ describe('cluster', () => { // THEN expect(stack.resolve(cluster.clusterEndpoint)).not.toEqual(stack.resolve(cluster.clusterReadEndpoint)); - - }); test('imported cluster with imported security group honors allowAllOutbound', () => { @@ -549,8 +525,6 @@ describe('cluster', () => { expect(stack).toHaveResource('AWS::EC2::SecurityGroupEgress', { GroupId: 'sg-123456789', }); - - }); test('can import a cluster with minimal attributes', () => { @@ -576,8 +550,6 @@ describe('cluster', () => { expect(() => cluster.clusterReadEndpoint).toThrow(/Cannot access `clusterReadEndpoint` of an imported cluster/); expect(() => cluster.instanceIdentifiers).toThrow(/Cannot access `instanceIdentifiers` of an imported cluster/); expect(() => cluster.instanceEndpoints).toThrow(/Cannot access `instanceEndpoints` of an imported cluster/); - - }); test('imported cluster can access properties if attributes are provided', () => { @@ -599,8 +571,6 @@ describe('cluster', () => { expect(cluster.clusterReadEndpoint.socketAddress).toEqual('reader-address:3306'); expect(cluster.instanceIdentifiers).toEqual(['identifier']); expect(cluster.instanceEndpoints.map(endpoint => endpoint.socketAddress)).toEqual(['instance-addr:3306']); - - }); test('cluster supports metrics', () => { @@ -1968,14 +1938,3 @@ function testStack(app?: cdk.App) { stack.node.setContext('availability-zones:12345:us-test-1', ['us-test-1a', 'us-test-1b']); return stack; } - -function createRDSCluster(stack: any, vpc: any) { - new DatabaseCluster(stack, 'Database', { - instances: cdk.Token.asNumber('1'), - engine: DatabaseClusterEngine.AURORA, - instanceProps: { - instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL), - vpc, - }, - }); -} From aeda9df86992ea3967a05f6e7055c8d0727476b1 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Thu, 25 Mar 2021 17:09:53 -0700 Subject: [PATCH 3/4] a -> an --- packages/@aws-cdk/aws-rds/lib/cluster.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-rds/lib/cluster.ts b/packages/@aws-cdk/aws-rds/lib/cluster.ts index 24bb5944b8a85..4e4e86f6a4d7f 100644 --- a/packages/@aws-cdk/aws-rds/lib/cluster.ts +++ b/packages/@aws-cdk/aws-rds/lib/cluster.ts @@ -652,7 +652,7 @@ interface InstanceConfig { function createInstances(cluster: DatabaseClusterNew, props: DatabaseClusterBaseProps, subnetGroup: ISubnetGroup): InstanceConfig { const instanceCount = props.instances != null ? props.instances : 2; if (Token.isUnresolved(instanceCount)) { - throw new Error('The number of instances a RDS Cluster consists of cannot be provided as a deploy-time only value!'); + throw new Error('The number of instances an RDS Cluster consists of cannot be provided as a deploy-time only value!'); } if (instanceCount < 1) { throw new Error('At least one instance is required'); From 2ebbe3f3aa83492795580a8f137d055489eef716 Mon Sep 17 00:00:00 2001 From: Adam Ruka Date: Thu, 25 Mar 2021 19:47:00 -0700 Subject: [PATCH 4/4] Make the CFN Parameter in the test of type 'number' --- packages/@aws-cdk/aws-rds/test/cluster.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/@aws-cdk/aws-rds/test/cluster.test.ts b/packages/@aws-cdk/aws-rds/test/cluster.test.ts index a26f861df0977..de7c5900de836 100644 --- a/packages/@aws-cdk/aws-rds/test/cluster.test.ts +++ b/packages/@aws-cdk/aws-rds/test/cluster.test.ts @@ -55,14 +55,13 @@ describe('cluster', () => { test('validates that the number of instances is not a deploy-time value', () => { const stack = testStack(); const vpc = new ec2.Vpc(stack, 'VPC'); - const parameter = new cdk.CfnParameter(stack, 'Param'); + const parameter = new cdk.CfnParameter(stack, 'Param', { type: 'Number' }); expect(() => { new DatabaseCluster(stack, 'Database', { instances: parameter.valueAsNumber, engine: DatabaseClusterEngine.AURORA, instanceProps: { - instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL), vpc, }, });