Relax ARN validation logic #3071
Conversation
Following up on apache#3005, which allowed a wide range of ARN values in the validation RegEx, remove an additional explicit check for `aws-cn` being present in the ARN as a sub-string. Update existing unit tests to process `aws-cn` ARNs as common `aws` ARNs. Note: the old validation code does not look correct because it used to check for `aws-cn` anywhere in the ARN string, not just in its "partition" component.
|
Related |
| break; | ||
| case "aws-cn": | ||
| roleARN = "arn:aws-cn:iam::012345678901:role/jdoe"; | ||
| region = "Beijing"; |
There was a problem hiding this comment.
"Beijing" is not a valid AWS region name. I know this is not explicitly part of the PR - but relates heavily to the scope and should be changed before we open this.
There was a problem hiding this comment.
Why should it be changed precisely?
There was a problem hiding this comment.
This is a unit test, it does not talk to AWS
There was a problem hiding this comment.
The test never asserted on the region name.
I think it's out of scope to let Polaris validate AWS specific region names.
There was a problem hiding this comment.
@adnanhemani : are you ok with merging this PR "as is"?
There was a problem hiding this comment.
With two approvals so far, I'm assuming lazy consensus and will be merging on Nov 26 unless there are fresh comments.
There was a problem hiding this comment.
Sorry, missed this comment (please feel free to ping on Slack if I forget again in the future!). I'm fine with merging as-is, I just find it odd to write happy-path tests (even if they are unit tests) with placeholder values that are clearly wrong - but I know you did not write this particular test.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
5 participants
Following up on #3005, which allowed a wide range of ARN values in the validation RegEx, remove an additional explicit check for
aws-cnbeing present in the ARN as a sub-string.Update existing unit tests to process
aws-cnARNs as commonawsARNs.Note: the old validation code does not look correct because it used to check for
aws-cnanywhere in the ARN string, not just in its "partition" component.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)