-
Notifications
You must be signed in to change notification settings - Fork 4k
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 cannot connect to cluster/instance #12953
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our contribution guide suggests using lower case for PR titles. I suppose this is in response to the guideline in conventional commits.
This should likely be "fix(rds): proxy cannot connect to cluster/instance"
By default, when creating a Proxy, we were not creating a Security Group for it, and because of that, the Proxy could not connect to the Cluster/Instance.
236cbfd
to
4bb2352
Compare
@nija-at thanks for the review! This is ready for another round. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provisional approval.
Please consider the comments below as a best practice, for this and in the future.
@@ -138,6 +136,22 @@ export = { | |||
DBInstanceIdentifiers: ABSENT, | |||
TargetGroupName: 'default', | |||
})); | |||
expect(stack).to(haveResourceLike('AWS::EC2::SecurityGroupIngress', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would much prefer to see a separate test case for these. Something like 'db proxy configures the tcp security ingress'. This keeps test cases small and understandable, rather than a large set of 'expects' and fewer test cases.
@@ -221,6 +236,10 @@ export = { | |||
'my-cluster', | |||
], | |||
})); | |||
expect(stack).to(haveResourceLike('AWS::EC2::SecurityGroup', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here.
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
By default, when creating a Proxy, we were not creating a Security Group for it, and because of that, the Proxy could not connect to the Cluster/Instance. See docs at: https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/rds-proxy.html ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
By default, when creating a Proxy, we were not creating a Security Group for it, and because of that, the Proxy could not connect to the Cluster/Instance. See docs at: https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/rds-proxy.html ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
fixes #8919 |
By default, when creating a Proxy,
we were not creating a Security Group for it,
and because of that, the Proxy could not connect to the Cluster/Instance.
See docs at: https://docs.aws.amazon.com/AmazonRDS/latest/AuroraUserGuide/rds-proxy.html
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license