Skip to content

Commit

Permalink
fix(aws-rds): ServerlessCluster.clusterArn is not correct when cluste…
Browse files Browse the repository at this point in the history
…rIdentifier includes upper cases string. (#13710)

Close #12795 

The problem is that, according to the cloudformation specification, even if the clusterIdentifier contains uppercase letters, the actual generated cluster identifier will be set to a lowercase string. But the resource ref remain the original string (that maybe contain uppercase letters).

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbcluster.html#cfn-rds-dbcluster-dbclusteridentifier

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
hedrall authored Apr 2, 2021
1 parent b2d4548 commit a8f5b6c
Show file tree
Hide file tree
Showing 8 changed files with 187 additions and 10 deletions.
9 changes: 7 additions & 2 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import * as kms from '@aws-cdk/aws-kms';
import * as logs from '@aws-cdk/aws-logs';
import * as s3 from '@aws-cdk/aws-s3';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Annotations, Duration, RemovalPolicy, Resource, Token } from '@aws-cdk/core';
import { Annotations, Duration, FeatureFlags, RemovalPolicy, Resource, Token } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { IClusterEngine } from './cluster-engine';
import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref';
Expand Down Expand Up @@ -340,11 +341,15 @@ abstract class DatabaseClusterNew extends DatabaseClusterBase {
const clusterParameterGroupConfig = clusterParameterGroup?.bindToCluster({});
this.engine = props.engine;

const clusterIdentifier = FeatureFlags.of(this).isEnabled(cxapi.RDS_LOWERCASE_DB_IDENTIFIER)
? props.clusterIdentifier?.toLowerCase()
: props.clusterIdentifier;

this.newCfnProps = {
// Basic
engine: props.engine.engineType,
engineVersion: props.engine.engineVersion?.fullVersion,
dbClusterIdentifier: props.clusterIdentifier,
dbClusterIdentifier: clusterIdentifier,
dbSubnetGroupName: this.subnetGroup.subnetGroupName,
vpcSecurityGroupIds: this.securityGroups.map(sg => sg.securityGroupId),
port: props.port ?? clusterEngineBindConfig.port,
Expand Down
18 changes: 16 additions & 2 deletions packages/@aws-cdk/aws-rds/lib/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,17 @@ import * as kms from '@aws-cdk/aws-kms';
import * as logs from '@aws-cdk/aws-logs';
import * as s3 from '@aws-cdk/aws-s3';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Duration, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/core';
import {
Duration,
FeatureFlags,
IResource,
Lazy,
RemovalPolicy,
Resource,
Stack,
Token,
} from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { DatabaseSecret } from './database-secret';
import { Endpoint } from './endpoint';
Expand Down Expand Up @@ -687,13 +697,17 @@ abstract class DatabaseInstanceNew extends DatabaseInstanceBase implements IData
});
}

const instanceIdentifier = FeatureFlags.of(this).isEnabled(cxapi.RDS_LOWERCASE_DB_IDENTIFIER)
? props.instanceIdentifier?.toLowerCase()
: props.instanceIdentifier;

this.newCfnProps = {
autoMinorVersionUpgrade: props.autoMinorVersionUpgrade,
availabilityZone: props.multiAz ? undefined : props.availabilityZone,
backupRetentionPeriod: props.backupRetention?.toDays(),
copyTagsToSnapshot: props.copyTagsToSnapshot ?? true,
dbInstanceClass: Lazy.string({ produce: () => `db.${this.instanceType}` }),
dbInstanceIdentifier: props.instanceIdentifier,
dbInstanceIdentifier: instanceIdentifier,
dbSubnetGroupName: subnetGroup.subnetGroupName,
deleteAutomatedBackups: props.deleteAutomatedBackups,
deletionProtection: defaultDeletionProtection(props.deletionProtection, props.removalPolicy),
Expand Down
9 changes: 7 additions & 2 deletions packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@ import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Resource, Duration, Token, Annotations, RemovalPolicy, IResource, Stack, Lazy } from '@aws-cdk/core';
import { Resource, Duration, Token, Annotations, RemovalPolicy, IResource, Stack, Lazy, FeatureFlags } from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { Construct } from 'constructs';
import { IClusterEngine } from './cluster-engine';
import { Endpoint } from './endpoint';
Expand Down Expand Up @@ -438,10 +439,14 @@ export class ServerlessCluster extends ServerlessClusterBase {
}),
];

const clusterIdentifier = FeatureFlags.of(this).isEnabled(cxapi.RDS_LOWERCASE_DB_IDENTIFIER)
? props.clusterIdentifier?.toLowerCase()
: props.clusterIdentifier;

const cluster = new CfnDBCluster(this, 'Resource', {
backupRetentionPeriod: props.backupRetention?.toDays(),
databaseName: props.defaultDatabaseName,
dbClusterIdentifier: props.clusterIdentifier,
dbClusterIdentifier: clusterIdentifier,
dbClusterParameterGroupName: clusterParameterGroupConfig?.parameterGroupName,
dbSubnetGroupName: this.subnetGroup.subnetGroupName,
deletionProtection: defaultDeletionProtection(props.deletionProtection, props.removalPolicy),
Expand Down
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-rds/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/aws-secretsmanager": "0.0.0",
"@aws-cdk/core": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"constructs": "^3.3.69"
},
"homepage": "https://github.com/aws/aws-cdk",
Expand All @@ -104,6 +105,7 @@
"@aws-cdk/aws-secretsmanager": "0.0.0",
"@aws-cdk/core": "0.0.0",
"@aws-cdk/aws-s3": "0.0.0",
"@aws-cdk/cx-api": "0.0.0",
"constructs": "^3.3.69"
},
"engines": {
Expand Down
42 changes: 42 additions & 0 deletions packages/@aws-cdk/aws-rds/test/cluster.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1860,6 +1860,48 @@ describe('cluster', () => {


});

test('changes the case of the cluster identifier if the lowercaseDbIdentifier feature flag is enabled', () => {
// GIVEN
const app = new cdk.App({
context: { [cxapi.RDS_LOWERCASE_DB_IDENTIFIER]: true },
});
const stack = testStack(app);
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
const clusterIdentifier = 'TestClusterIdentifier';
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
instanceProps: { vpc },
clusterIdentifier,
});

// THEN
expect(stack).toHaveResourceLike('AWS::RDS::DBCluster', {
DBClusterIdentifier: clusterIdentifier.toLowerCase(),
});
});

test('does not changes the case of the cluster identifier if the lowercaseDbIdentifier feature flag is disabled', () => {
// GIVEN
const app = new cdk.App();
const stack = testStack(app);
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
const clusterIdentifier = 'TestClusterIdentifier';
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
instanceProps: { vpc },
clusterIdentifier,
});

// THEN
expect(stack).toHaveResourceLike('AWS::RDS::DBCluster', {
DBClusterIdentifier: clusterIdentifier,
});
});
});

test.each([
Expand Down
48 changes: 47 additions & 1 deletion packages/@aws-cdk/aws-rds/test/instance.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import * as logs from '@aws-cdk/aws-logs';
import * as s3 from '@aws-cdk/aws-s3';
import * as cdk from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { testFutureBehavior } from 'cdk-build-tools/lib/feature-flag';
import { testFutureBehavior, testLegacyBehavior } from 'cdk-build-tools/lib/feature-flag';
import * as rds from '../lib';

let stack: cdk.Stack;
Expand Down Expand Up @@ -1279,6 +1279,52 @@ describe('instance', () => {
PubliclyAccessible: true,
});
});

testFutureBehavior(
'changes the case of the cluster identifier if the lowercaseDbIdentifier feature flag is enabled',
{ [cxapi.RDS_LOWERCASE_DB_IDENTIFIER]: true }, cdk.App, (app,
) => {
// GIVEN
stack = new cdk.Stack( app );
vpc = new ec2.Vpc( stack, 'VPC' );

// WHEN
const instanceIdentifier = 'TestInstanceIdentifier';
new rds.DatabaseInstance( stack, 'DB', {
engine: rds.DatabaseInstanceEngine.mysql({
version: rds.MysqlEngineVersion.VER_8_0_19,
}),
vpc,
instanceIdentifier,
} );

// THEN
expect(stack).toHaveResource('AWS::RDS::DBInstance', {
DBInstanceIdentifier: instanceIdentifier.toLowerCase(),
});
});

testLegacyBehavior( 'does not changes the case of the cluster identifier if the lowercaseDbIdentifier feature flag is disabled', cdk.App, (app ) => {
// GIVEN
stack = new cdk.Stack( app );
vpc = new ec2.Vpc( stack, 'VPC' );

// WHEN
const instanceIdentifier = 'TestInstanceIdentifier';
new rds.DatabaseInstance( stack, 'DB', {
engine: rds.DatabaseInstanceEngine.mysql({
version: rds.MysqlEngineVersion.VER_8_0_19,
}),
vpc,
instanceIdentifier,
} );

// THEN
expect(stack).toHaveResource('AWS::RDS::DBInstance', {
DBInstanceIdentifier: instanceIdentifier,
});
});

});

test.each([
Expand Down
52 changes: 49 additions & 3 deletions packages/@aws-cdk/aws-rds/test/serverless-cluster.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as ec2 from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as cdk from '@aws-cdk/core';
import * as cxapi from '@aws-cdk/cx-api';
import { nodeunitShim, Test } from 'nodeunit-shim';
import { AuroraPostgresEngineVersion, ServerlessCluster, DatabaseClusterEngine, ParameterGroup, AuroraCapacityUnit, DatabaseSecret } from '../lib';

Expand Down Expand Up @@ -818,11 +819,56 @@ nodeunitShim({

test.done();
},
});

'changes the case of the cluster identifier if the lowercaseDbIdentifier feature flag is enabled'(test: Test) {
// GIVEN
const app = new cdk.App({
context: { [cxapi.RDS_LOWERCASE_DB_IDENTIFIER]: true },
});
const stack = testStack(app);
const clusterIdentifier = 'TestClusterIdentifier';
const vpc = ec2.Vpc.fromLookup(stack, 'VPC', { isDefault: true });

// WHEN
new ServerlessCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA_MYSQL,
vpc,
clusterIdentifier,
});

// THEN
expect(stack).to(haveResourceLike('AWS::RDS::DBCluster', {
DBClusterIdentifier: clusterIdentifier.toLowerCase(),
}));

test.done();
},

'does not change the case of the cluster identifier if the lowercaseDbIdentifier feature flag is disabled'(test: Test) {
// GIVEN
const app = new cdk.App();
const stack = testStack(app);
const clusterIdentifier = 'TestClusterIdentifier';
const vpc = ec2.Vpc.fromLookup(stack, 'VPC', { isDefault: true });

// WHEN
new ServerlessCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA_MYSQL,
vpc,
clusterIdentifier,
});

// THEN
expect(stack).to(haveResourceLike('AWS::RDS::DBCluster', {
DBClusterIdentifier: clusterIdentifier,
}));

test.done();
},
});

function testStack() {
const stack = new cdk.Stack(undefined, undefined, { env: { account: '12345', region: 'us-test-1' } });
function testStack(app?: cdk.App, id?: string): cdk.Stack {
const stack = new cdk.Stack(app, id, { env: { account: '12345', region: 'us-test-1' } });
stack.node.setContext('availability-zones:12345:us-test-1', ['us-test-1a', 'us-test-1b']);
return stack;
}
17 changes: 17 additions & 0 deletions packages/@aws-cdk/cx-api/lib/features.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,21 @@ export const S3_GRANT_WRITE_WITHOUT_ACL = '@aws-cdk/aws-s3:grantWriteWithoutAcl'
*/
export const ECS_REMOVE_DEFAULT_DESIRED_COUNT = '@aws-cdk/aws-ecs-patterns:removeDefaultDesiredCount';

/**
* ServerlessCluster.clusterIdentifier currently can has uppercase letters,
* and ServerlessCluster pass it through to CfnDBCluster.dbClusterIdentifier.
* The identifier is saved as lowercase string in AWS and is resolved as original string in CloudFormation.
*
* If this flag is not set, original value that one set to ServerlessCluster.clusterIdentifier
* is passed to CfnDBCluster.dbClusterIdentifier.
* If this flag is true, ServerlessCluster.clusterIdentifier is converted into a string containing
* only lowercase characters by the `toLowerCase` function and passed to CfnDBCluster.dbClusterIdentifier.
*
* This feature flag make correct the ServerlessCluster.clusterArn when
* clusterIdentifier contains a Upper case letters.
*/
export const RDS_LOWERCASE_DB_IDENTIFIER = '@aws-cdk/aws-rds:lowercaseDbIdentifier';

/**
* This map includes context keys and values for feature flags that enable
* capabilities "from the future", which we could not introduce as the default
Expand All @@ -126,6 +141,7 @@ export const FUTURE_FLAGS: { [key: string]: any } = {
[KMS_DEFAULT_KEY_POLICIES]: true,
[S3_GRANT_WRITE_WITHOUT_ACL]: true,
[ECS_REMOVE_DEFAULT_DESIRED_COUNT]: true,
[RDS_LOWERCASE_DB_IDENTIFIER]: true,

// We will advertise this flag when the feature is complete
// [NEW_STYLE_STACK_SYNTHESIS_CONTEXT]: 'true',
Expand All @@ -152,6 +168,7 @@ const FUTURE_FLAGS_DEFAULTS: { [key: string]: boolean } = {
[KMS_DEFAULT_KEY_POLICIES]: false,
[S3_GRANT_WRITE_WITHOUT_ACL]: false,
[ECS_REMOVE_DEFAULT_DESIRED_COUNT]: false,
[RDS_LOWERCASE_DB_IDENTIFIER]: false,
};

export function futureFlagDefault(flag: string): boolean {
Expand Down

0 comments on commit a8f5b6c

Please sign in to comment.