Skip to content

Commit

Permalink
fix(redshift): single-node clusters fail with node count error (#9961)
Browse files Browse the repository at this point in the history 
Single-node clusters defaulted to a `numberOfNodes` of 1; while this makes
sense, CloudFormation actually throws if any number of nodes is defined for a
single-node cluster. This change fixes this to default to `undefined` and
extends the validation for multi-node clusters.

fixes #9856

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@njlynch
njlynch committed Aug 27, 2020
1 parent 1220e4a commit 2cd3ea2
Show file tree
Hide file tree
Showing 2 changed files with 110 additions and 62 deletions.
28 changes: 20 additions & 8 deletions packages/@aws-cdk/aws-redshift/lib/cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,11 @@ export interface ClusterProps {
readonly parameterGroup?: IClusterParameterGroup;

/**
* Number of compute nodes in the cluster
* Number of compute nodes in the cluster. Only specify this property for multi-node clusters.
*
* Value must be at least 1 and no more than 100.
* Value must be at least 2 and no more than 100.
*
* @default 1
* @default - 2 if `clusterType` is ClusterType.MULTI_NODE, undefined otherwise
*/
readonly numberOfNodes?: number;

Expand Down Expand Up @@ -425,11 +425,7 @@ export class Cluster extends ClusterBase {
}

const clusterType = props.clusterType || ClusterType.MULTI_NODE;
const nodeCount = props.numberOfNodes !== undefined ? props.numberOfNodes : (clusterType === ClusterType.MULTI_NODE ? 2 : 1);

if (clusterType === ClusterType.MULTI_NODE && nodeCount < 2) {
throw new Error('Number of nodes for cluster type multi-node must be at least 2');
}
const nodeCount = this.validateNodeCount(clusterType, props.numberOfNodes);

if (props.encrypted === false && props.encryptionKey !== undefined) {
throw new Error('Cannot set property encryptionKey without enabling encryption!');
Expand Down Expand Up @@ -537,4 +533,20 @@ export class Cluster extends ClusterBase {
target: this,
});
}

private validateNodeCount(clusterType: ClusterType, numberOfNodes?: number): number | undefined {
if (clusterType === ClusterType.SINGLE_NODE) {
// This property must not be set for single-node clusters; be generous and treat a value of 1 node as undefined.
if (numberOfNodes !== undefined && numberOfNodes !== 1) {
throw new Error('Number of nodes must be not be supplied or be 1 for cluster type single-node');
}
return undefined;
} else {
const nodeCount = numberOfNodes ?? 2;
if (nodeCount < 2 || nodeCount > 100) {
throw new Error('Number of nodes for cluster type multi-node must be at least 2 and no more than 100');
}
return nodeCount;
}
}
}
144 changes: 90 additions & 54 deletions packages/@aws-cdk/aws-redshift/test/cluster.test.ts
View file
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
import { expect as cdkExpect, haveResource, ResourcePart } from '@aws-cdk/assert';
import { ABSENT, expect as cdkExpect, haveResource, ResourcePart } from '@aws-cdk/assert';
import '@aws-cdk/assert/jest';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as kms from '@aws-cdk/aws-kms';
import * as s3 from '@aws-cdk/aws-s3';
import * as cdk from '@aws-cdk/core';
import { Cluster, ClusterParameterGroup, ClusterType } from '../lib';

import { Cluster, ClusterParameterGroup, ClusterType, NodeType } from '../lib';
let stack: cdk.Stack;
let vpc: ec2.IVpc;

test('check that instantiation works', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');
beforeEach(() => {
stack = testStack();
vpc = new ec2.Vpc(stack, 'VPC');
});

test('check that instantiation works', () => {
// WHEN
new Cluster(stack, 'Redshift', {
masterUser: {
Expand Down Expand Up @@ -57,8 +60,7 @@ test('check that instantiation works', () => {

test('can create a cluster with imported vpc and security group', () => {
// GIVEN
const stack = testStack();
const vpc = ec2.Vpc.fromLookup(stack, 'VPC', {
vpc = ec2.Vpc.fromLookup(stack, 'ImportedVPC', {
vpcId: 'VPC12345',
});
const sg = ec2.SecurityGroup.fromSecurityGroupId(stack, 'SG', 'SecurityGroupId12345');
Expand All @@ -83,10 +85,6 @@ test('can create a cluster with imported vpc and security group', () => {
});

test('creates a secret when master credentials are not specified', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new Cluster(stack, 'Redshift', {
masterUser: {
Expand Down Expand Up @@ -133,34 +131,88 @@ test('creates a secret when master credentials are not specified', () => {
}));
});

test('SIngle Node CLusters spawn only single node', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');
describe('node count', () => {

test('Single Node Clusters do not define node count', () => {
// WHEN
new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
clusterType: ClusterType.SINGLE_NODE,
});

// THEN
cdkExpect(stack).to(haveResource('AWS::Redshift::Cluster', {
ClusterType: 'single-node',
NumberOfNodes: ABSENT,
}));
});

// WHEN
new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
nodeType: NodeType.DC1_8XLARGE,
clusterType: ClusterType.SINGLE_NODE,
test('Single Node Clusters treat 1 node as undefined', () => {
// WHEN
new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
clusterType: ClusterType.SINGLE_NODE,
numberOfNodes: 1,
});

// THEN
cdkExpect(stack).to(haveResource('AWS::Redshift::Cluster', {
ClusterType: 'single-node',
NumberOfNodes: ABSENT,
}));
});

// THEN
cdkExpect(stack).to(haveResource('AWS::Redshift::Cluster', {
ClusterType: 'single-node',
NodeType: 'dc1.8xlarge',
NumberOfNodes: 1,
}));
test('Single Node Clusters throw if any other node count is specified', () => {
expect(() => {
new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
clusterType: ClusterType.SINGLE_NODE,
numberOfNodes: 2,
});
}).toThrow(/Number of nodes must be not be supplied or be 1 for cluster type single-node/);
});

test('Multi-Node Clusters default to 2 nodes', () => {
// WHEN
new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
clusterType: ClusterType.MULTI_NODE,
});

// THEN
cdkExpect(stack).to(haveResource('AWS::Redshift::Cluster', {
ClusterType: 'multi-node',
NumberOfNodes: 2,
}));
});

test.each([0, 1, -1, 101])('Multi-Node Clusters throw with %s nodes', (numberOfNodes: number) => {
expect(() => {
new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
},
vpc,
clusterType: ClusterType.MULTI_NODE,
numberOfNodes,
});
}).toThrow(/Number of nodes for cluster type multi-node must be at least 2 and no more than 100/);
});
});

test('create an encrypted cluster with custom KMS key', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new Cluster(stack, 'Redshift', {
masterUser: {
Expand All @@ -182,10 +234,6 @@ test('create an encrypted cluster with custom KMS key', () => {
});

test('cluster with parameter group', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
const group = new ClusterParameterGroup(stack, 'Params', {
description: 'bye',
Expand All @@ -211,8 +259,6 @@ test('cluster with parameter group', () => {

test('imported cluster with imported security group honors allowAllOutbound', () => {
// GIVEN
const stack = testStack();

const cluster = Cluster.fromClusterAttributes(stack, 'Database', {
clusterEndpointAddress: 'addr',
clusterName: 'identifier',
Expand All @@ -235,8 +281,6 @@ test('imported cluster with imported security group honors allowAllOutbound', ()

test('can create a cluster with logging enabled', () => {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');
const bucket = s3.Bucket.fromBucketName(stack, 'bucket', 'logging-bucket');

// WHEN
Expand All @@ -259,10 +303,6 @@ test('can create a cluster with logging enabled', () => {
});

test('throws when trying to add rotation to a cluster without secret', () => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
const cluster = new Cluster(stack, 'Redshift', {
masterUser: {
Expand All @@ -281,8 +321,6 @@ test('throws when trying to add rotation to a cluster without secret', () => {

test('throws validation error when trying to set encryptionKey without enabling encryption', () => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');
const key = new kms.Key(stack, 'kms-key');

// WHEN
Expand All @@ -304,8 +342,6 @@ test('throws validation error when trying to set encryptionKey without enabling

test('throws when trying to add single user rotation multiple times', () => {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');
const cluster = new Cluster(stack, 'Redshift', {
masterUser: {
masterUsername: 'admin',
Expand All @@ -323,7 +359,7 @@ test('throws when trying to add single user rotation multiple times', () => {
});

function testStack() {
const stack = new cdk.Stack(undefined, undefined, { env: { account: '12345', region: 'us-test-1' } });
stack.node.setContext('availability-zones:12345:us-test-1', ['us-test-1a', 'us-test-1b']);
return stack;
}
const newTestStack = new cdk.Stack(undefined, undefined, { env: { account: '12345', region: 'us-test-1' } });
newTestStack.node.setContext('availability-zones:12345:us-test-1', ['us-test-1a', 'us-test-1b']);
return newTestStack;
}

0 comments on commit 2cd3ea2

Please sign in to comment.