smaheshwar-pltr commented on PR #13225: URL: https://github.com/apache/iceberg/pull/13225#issuecomment-4362295328
> Also one thing which i am unclear of is how is the rest-catalog giving creds for encryption ? is it implictly assumed storage config will have creds for encryption too like KMS ? what happens in case such as vault ? how does the rest server give those creds back ? Thanks for flagging this @singhpk234, it's a great point. This PR integrates with the existing `KeyManagementClient` wiring as in https://github.com/apache/iceberg/pull/13066 - it's initialised from merged catalog properties and clients can configure it as integration tests do. The catalog properties would include the server-returned config, but that's still at a per-client(/catalog) level and not a per-table level. I think that per-table KMS credential vending is valuable - I can imagine REST servers wanting to follow the principle of least privilege and subscope KMS credentials where that's possible. I do think there's a discussion to be had - the AWS KMS client doesn't pick up the `s3.access-key-id` vended properties, and even if it did, it's debatable whether those should be used; we'd maybe want some KMS prefix, as you could imagine a setup with Azure storage and an AWS KMS, in which case it doesn't feel great to vend those KMS credentials as if they're S3 storage credentials. Thinking out loud, I wonder about KMS credential refresh if that's desirable. On this PR: I'd prefer to split PRs and discussions (we've already discussed a lot here) - in my opinion, it makes sense to ship catalog-property KMS configuration first because I think this should be supported anyway (in the same way that you can initialise a `RESTCatalog` with your own storage credentials that will be used if there's no access delegation) and is valuable as is. Introducing per-table KMS clients as a follow up makes sense to, me and shouldn't break this behaviour. I'll open an issue for the KMS credential vending discussion, and maybe we can proceed this PR, how does that sound? (+ cc @ggershinsky as I know you folks have been thinking about REST integration too, would love to hear your thoughts on this) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
