Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(rds): proxy for db cluster fails with model validation error #8896

Merged
merged 4 commits into from Jul 6, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 16 additions & 6 deletions packages/@aws-cdk/aws-rds/lib/proxy.ts
Expand Up @@ -408,20 +408,30 @@ export class DatabaseProxy extends cdk.Resource
this.endpoint = this.resource.attrEndpoint;

let dbInstanceIdentifiers: string[] | undefined;
if (bindResult.dbClusters) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a unit test for the case that was previously wrong and is fixed in this PR.

// support for only instances of a single cluster
dbInstanceIdentifiers = bindResult.dbClusters[0].instanceIdentifiers;
} else if (bindResult.dbInstances) {
if (bindResult.dbInstances) {
// support for only single instance
dbInstanceIdentifiers = [ bindResult.dbInstances[0].instanceIdentifier ];
}

new CfnDBProxyTargetGroup(this, 'ProxyTargetGroup', {
let dbClusterIdentifiers: string[] | undefined;
if (bindResult.dbClusters) {
dbClusterIdentifiers = bindResult.dbClusters.map((c) => c.clusterIdentifier);
}

if (!!dbInstanceIdentifiers && !!dbClusterIdentifiers) {
throw new Error('Cannot specify both dbInstanceIdentifiers and dbClusterIdentifiers');
}

const proxyTargetGroup = new CfnDBProxyTargetGroup(this, 'ProxyTargetGroup', {
dbProxyName: this.dbProxyName,
dbInstanceIdentifiers,
dbClusterIdentifiers: bindResult.dbClusters?.map((c) => c.clusterIdentifier),
dbClusterIdentifiers,
connectionPoolConfigurationInfo: toConnectionPoolConfigurationInfo(props),
});

// Currently(2020-07-04), this property must be set to default.
// https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-rds-dbproxytargetgroup.html#TargetGroupName-fn::getatt
proxyTargetGroup.addOverride('Properties.TargetGroupName', 'default');
Comment on lines +432 to +434
Copy link
Contributor

@nija-at nija-at Jul 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overrides are set for CloudFormation properties that are not yet supported in the CDK. Looks like this is a CloudFormation return value.

Why is this being set?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be a difference between the CloudFormation resource document and what CloudFormation actually does.

Currently CloudFormation requires TargetGroupName in props of ProxyTargetGroup, but it is not in CfnProxyTargetGroupProps

There is no normal way to set TargetGroupName, but through addOverride.

image

The behavior seems to be changed around DB Proxy's GA.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation.

}

/**
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-rds/test/integ.proxy.expected.json
Expand Up @@ -554,7 +554,8 @@
{
"Ref": "dbInstance4076B1EC"
}
]
],
"TargetGroupName": "default"
}
}
}
Expand Down
44 changes: 35 additions & 9 deletions packages/@aws-cdk/aws-rds/test/test.proxy.ts
@@ -1,4 +1,4 @@
import { expect, haveResource, ResourcePart } from '@aws-cdk/assert';
import { ABSENT, expect, haveResource, ResourcePart } from '@aws-cdk/assert';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as cdk from '@aws-cdk/core';
import { Test } from 'nodeunit';
Expand Down Expand Up @@ -67,6 +67,7 @@ export = {
Ref: 'InstanceC1063A87',
},
],
TargetGroupName: 'default',
},
}, ResourcePart.CompleteDefinition));

Expand Down Expand Up @@ -140,17 +141,42 @@ export = {
Ref: 'DatabaseB269D8BB',
},
],
DBInstanceIdentifiers: [
{
Ref: 'DatabaseInstance1844F58FD',
},
{
Ref: 'DatabaseInstance2AA380DEE',
},
],
TargetGroupName: 'default',
},
}, ResourcePart.CompleteDefinition));

test.done();
},

'Cannot specify both dbInstanceIdentifiers and dbClusterIdentifiers'(test: Test) {
// GIVEN
const stack = new cdk.Stack();
const vpc = new ec2.Vpc(stack, 'VPC');
const cluster = new rds.DatabaseCluster(stack, 'Database', {
engine: rds.DatabaseClusterEngine.AURORA_POSTGRESQL,
engineVersion: '10.7',
masterUser: {
username: 'admin',
},
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
vpc,
},
});

// WHEN
test.doesNotThrow(() => {
new rds.DatabaseProxy(stack, 'Proxy', {
proxyTarget: rds.ProxyTarget.fromCluster(cluster),
secret: cluster.secret!,
vpc,
});
}, /Cannot specify both dbInstanceIdentifiers and dbClusterIdentifiers/);

expect(stack).to(haveResource('AWS::RDS::DBProxyTargetGroup', {
DBInstanceIdentifiers: ABSENT,
}, ResourcePart.Properties));

test.done();
},
};