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

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

merged 4 commits into from Jul 6, 2020

Conversation

civilizeddev
Copy link
Contributor

@civilizeddev civilizeddev commented Jul 4, 2020

fixes #8885
follows #8476


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@nija-at nija-at changed the title fix(rds): Explicitly set 'default' as TargetGroupName of DatabaseProxy fix(rds): proxy for db cluster fails with model validation error Jul 6, 2020
@nija-at nija-at self-assigned this Jul 6, 2020
nija-at
nija-at previously requested changes Jul 6, 2020
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Thanks for posting a fix. Some comments below.

Comment on lines +432 to +434
// 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');
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.

@@ -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.

@mergify mergify bot dismissed nija-at’s stale review July 6, 2020 10:28

Pull request has been modified.

@civilizeddev civilizeddev requested a review from nija-at July 6, 2020 11:53
Comment on lines +432 to +434
// 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');
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.

@mergify
Copy link
Contributor

mergify bot commented Jul 6, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: ef3deb5
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Jul 6, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 7d47cfb into aws:master Jul 6, 2020
@civilizeddev civilizeddev deleted the fix/aws-rds-8885 branch July 6, 2020 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DatabaseProxy] Model validation failed (#: required key [TargetGroupName] not found)
3 participants