Skip to content

Conversation

@dimas-b
Copy link
Contributor

@dimas-b dimas-b commented Nov 17, 2025

Following up on #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.

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

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.
@dimas-b
Copy link
Contributor Author

dimas-b commented Nov 17, 2025

Related dev ML discussion: https://lists.apache.org/thread/dxybjf4w4or1vmpb25zq6m5gso96rr4j

break;
case "aws-cn":
roleARN = "arn:aws-cn:iam::012345678901:role/jdoe";
region = "Beijing";
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why should it be changed precisely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a unit test, it does not talk to AWS

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.

2 participants