fix: IAM auth in CN RDS (#579) #582
Conversation
| final Pattern auroraDnsPattern = | ||
| Pattern.compile( | ||
| "(.+)\\.(proxy-|cluster-|cluster-ro-|cluster-custom-)?[a-zA-Z0-9]+\\.([a-zA-Z0-9\\-]+)\\.rds\\.amazonaws\\.com", | ||
| "(.+)\\.(proxy-|cluster-|cluster-ro-|cluster-custom-)?[a-zA-Z0-9]+\\.([a-zA-Z0-9\\-]+)\\.rds\\.amazonaws\\.com(\\.cn)?", |
There was a problem hiding this comment.
Why do we need the last (\.cn) in this regexp? The code checks for China url pattern further.
There was a problem hiding this comment.
I'd suggest to replocate entire method getRdsRegion() to RdsUtils.
There was a problem hiding this comment.
I was double checking this DNS pattern against the one in RdSUtils.java and it contains (\.cn)? at the end. Same thing in the aws-advanced-jdbc-wrapper. So it looks like this pattern may have been missing it.
| final Pattern auroraDnsPattern = | ||
| Pattern.compile( | ||
| "(.+)\\.(proxy-|cluster-|cluster-ro-|cluster-custom-)?[a-zA-Z0-9]+\\.([a-zA-Z0-9\\-]+)\\.rds\\.amazonaws\\.com", | ||
| "(.+)\\.(proxy-|cluster-|cluster-ro-|cluster-custom-)?[a-zA-Z0-9]+\\.([a-zA-Z0-9\\-]+)\\.rds\\.amazonaws\\.com(\\.cn)?", |
There was a problem hiding this comment.
i think there might be some confusion here (?)
the first regex was supposed to try to match non-CN endpoints
and then there was the CN-matcher for CN endpoints
do you want to merge them so one regex matches both?
I would suggest to put the regex string as a static variable and in the comments put the patterns that are valid
There was a problem hiding this comment.
Added (\\.cn)? to the end because i noticed that the pattern in RdsUtils.java has it, but it doesnt have it here.
Personally, I'd prefer not to mix them. I'd rather keep it simple with two separate patterns, than one more complex one.
I will put them in static vars are you suggested though.
edit: I think i might actually just eliminate these patterns here, and directly reference the ones that are defined in RdsUtils.java
| log.logTrace(exceptionMessage); | ||
| throw ExceptionFactory.createException(exceptionMessage); | ||
| } | ||
| matcher = chinaMatcher; |
There was a problem hiding this comment.
do you need to do something with the matcher here?
There was a problem hiding this comment.
It gets used a lil further down.
| */ | ||
| @Test | ||
| public void test_7_ValidChinaHostAndRegion() { | ||
| Assertions.assertNotNull(new AwsIamAuthenticationTokenHelper( |
There was a problem hiding this comment.
maybe you should add a couple of examples that have the .cn at the end but have other irregularities in the middle of the URL to be sure your regex is catching what you want
aaronchung-bitquill
force-pushed
the
fix/iam-cn-fix
branch
from
73d5d1d to
5283ca7
Compare
aaronchung-bitquill
force-pushed
the
fix/iam-cn-fix
branch
from
5283ca7 to
ce0b254
Compare
4 participants
Summary
fix: IAM auth in CN RDS (#579)
Description
Previously
AwsIamAuthenticationTokenHelper#getRegionaurora DNS pattern did not support CN regions. Added an aurora DNS pattern for CN region specifically.Additional Reviewers
By submitting this pull request, I confirm that my contribution is made under the terms of the GPLv2 license.