Add KMS options to catalogs create CLI #3330
Conversation
Following up on apache#2802
| current_kms_key=options_get(Arguments.KMS_KEY_CURRENT), | ||
| allowed_kms_keys=options_get(Arguments.KMS_KEY_ALLOWED), |
There was a problem hiding this comment.
does just setting current_kms_key automatically adds the allowed_kms_keys ?
There was a problem hiding this comment.
Currently "current" is the same as "allowed"... However, I believe only "current" needs write access... but that's in Polaris java code... it does not affect CLI.
| ) | ||
| PATH_STYLE_ACCESS = "(Only for S3) Whether to use path-style-access for S3" | ||
| KMS_KEY_CURRENT = ( | ||
| "(Only for AWS S3) The AWS KMS key ARN to be used for encrypting new S3 data" |
There was a problem hiding this comment.
I am assuming this is required because we need to use this key to encrypt metadata.json ? as when we are vending creds we don't know which snapshot the client will be reading so we vend creds for all or we just give decrypt creds for allowed key and encrypt | decrypt creds for current keys ?
There was a problem hiding this comment.
Polaris does not use KMS keys directly. It only generates AWS policies that allow those keys to be used on the AWS side when S3 requests are made. But, yes, the current key is used for writing new data. Zero or more additional keys are also allowed to be used because they might be required for dealing with old files.
There was a problem hiding this comment.
Polaris does not use KMS keys directly.
wouldn't we be needing this for encrypting / decrypting metadata.json ?
additional keys are also allowed to be used because they might be required for dealing with old files
I agree with additional keys but my question was why would Polaris vends creds for old kms keys for encrypting, files are immutable, so old keys should be vended for decrypt, similarly new key should have encrypt / decrypt.
Do we vend creds for encryption and decryting for all key in our sts policy ?
There was a problem hiding this comment.
my question was why would Polaris vends creds for old kms keys for encrypting, [...]
Currently it does. However, this is beyond the scope of current PR (CLI). It's about the actual java code from #2802 :)
Normally, I'd think "additional" keys should get only decryption rights, but this may be tricky from the manual key rotation perspective.
| "(Only for S3) Indicates that Polaris should not use STS (e.g. if STS is not available)" | ||
| ) | ||
| PATH_STYLE_ACCESS = "(Only for S3) Whether to use path-style-access for S3" | ||
| KMS_KEY_CURRENT = ( |
There was a problem hiding this comment.
As these are optional and only for AWS, we may want to update client/python/apache_polaris/cli/command/catalogs.py as well for the function _has_aws_storage_info(). Here is a reference: https://github.com/apache/polaris/pull/3305/files#diff-a3e865c2a57514f7f505c706a3af70a5ac90b712f96656b513cdbfcee20c031eL181
There was a problem hiding this comment.
good point - updated
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM thanks @dimas-b !
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
4 participants
Following up on #2802
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)