Add MinIO coverage for AWS-shaped KMS vended credentials#4034
Add MinIO coverage for AWS-shaped KMS vended credentials#4034castanhas98 wants to merge 2 commits intoapache:mainfrom
Conversation
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks for your contribution, @castanhas98 ! Adding these tests sounds reasonable to me 👍 Just one minor comment.
| includeRoleArn ? Optional.of(TEST_ROLE_ARN) : Optional.empty(), | ||
| Optional.of(false))) { | ||
| TableIdentifier id = createTableAndVerifyMetadata(restCatalog); | ||
| if (expectFailure) { |
There was a problem hiding this comment.
Would you mind refactoring expected failures and successes into two separate test methods? I believe it would be easier for the reader to follow the test logic and analyse failures, should they occur.
There was a problem hiding this comment.
Refactored this into separate success and failure tests so the expected behavior is easier to scan and diagnose. I also reran the targeted MinIO/KMS integration tests locally after the split. Thanks for the suggestion.
dimas-b
left a comment
dimas-b
left a comment
There was a problem hiding this comment.
Thanks, @castanhas98 !
I suppose we can merge on Mon if there are no other review concerns.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
@castanhas98 : please run spotless checks locally - I believe the first CI run failed because of that. |
|
@dimas-b Thanks for the review! I ran spotless on the previous commit, so we should be good on that front. |
3 participants
Summary
This PR adds focused MinIO integration coverage for the AWS-shaped vended-credentials/KMS edge case.
It adds a matrix that varies
regionandroleArnindependently withkmsUnavailable=falseand verifies that only theregion + roleArncase fails duringloadTable, with the expected MinIO/KMS-scoping error.It also adds a companion test showing that the same AWS-shaped configuration succeeds when
kmsUnavailable=true, and refactors the test helper so the failing path still cleans up table state correctly.Why this change is needed
The existing MinIO coverage exercised standard vended-credentials behavior, but it did not cover the AWS-shaped read-only KMS path that is triggered only when both
regionandroleArnare set while KMS remains enabled.That path is important because Polaris will add the wildcard KMS ARN for read-only AWS-style access, and MinIO STS rejects that resource as invalid. This PR makes that edge case explicit and locks in the expected
kmsUnavailablebehavior.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)