(WIP) Remove KMS policies when KMS is not configured #3445
Conversation
| if (hasCurrentKey || hasAllowedKeys) { | ||
| policyBuilder.addStatement(allowKms.build()); | ||
| } else if (!canWrite) { | ||
| } else if (!canWrite && region != null && accountId != null) { |
There was a problem hiding this comment.
It looks like this else condition will now be evaluated when hasCurrentKey == true and hasAllowedKeys == false, but it was not evaluated in that case before 🤔 WDYT?
There was a problem hiding this comment.
Thanks for the quick review @dimas-b and that is a fair point which I missed. It appears this is really specific for handling s3 and s3-compatible and I am not sure if the above logic will break or not due to lack of infra to test. I had made the needed code changes to preserve this workflow. Please take another look when you get a chance.
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
|
CC: @fabio-rizzo-01 |
| if (currentMetadata != null) { | ||
| tableFileIO = | ||
| loadFileIOForTableLike( |
There was a problem hiding this comment.
How is that related to KMS policies?
There was a problem hiding this comment.
If it's just a parallel bugfix, I'd prefer to make it in a separate PR for the sake of clarity 🤔 WDYT?
There was a problem hiding this comment.
Appear to be a parallel bug. I am waiting for testing from reporter then I can split this into two PRs. Currently if we merged current PR, it will trigger diff error from reporter.
MonkeyCanCode
changed the title
|
Close this one and opened three different PRs for easier review: |
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
2 participants
This PR addressed issues reported in #3440 where when end-user is not using KMS encryption for S3, Polaris still enforces in-lines policies which contains KMS related policies. While fixing this issue, I noticed our read-only policy for kMS is a bit too wide where
GenerateDataKeyandGenerateDataKeyWithoutPlaintextshould be belongs to write operation instead of read. Thus, this PR also addresses this issue.Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)