dimas-b commented on code in PR #17155:
URL: https://github.com/apache/iceberg/pull/17155#discussion_r3571716091
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2147,9 +2147,16 @@ components:
to supply access via any or none of the requested mechanisms.
- Specific properties and handling for `vended-credentials` and
`remote-signing`
+ Specific properties and handling for `vended-credentials`,
`remote-signing`, and `kms-vended-credentials`
are documented in the `LoadTableResult` schema section of this spec
document.
+ For encrypted table operations, delegated credential sourcing should
be consistent. If a catalog
+ honors REST access delegation for an encrypted table operation, it
should provide all delegated
+ credentials required for that operation, including storage credentials
and KMS credentials. Clients
+ must not be required to combine catalog-vended storage credentials
with client-local or static KMS
Review Comment:
nit: "must not be required to combine ..." I support this approach, but the
exact phrase does not feel like it fits within the API "spec". Perhaps
rephrasing along the lines of "catalogs should provide all credentials required
for storage access...". WDYT?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -2158,9 +2165,10 @@ components:
enum:
- vended-credentials
- remote-signing
+ - kms-vended-credentials
Review Comment:
This value is mentioned, but not explicitly described in the doc comment
(above).
Does it subsume `vended-credentials`?.. Or should clients declare both (in
case KMS is involved)?
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -3665,6 +3673,43 @@ components:
additionalProperties:
type: string
+ KeyManagementCredential:
+ type: object
+ description: |
+ Provider-specific credential config for accessing one or more KMS keys
required by an encrypted
+ table operation.
+
+ The key-management provider is advertised in catalog configuration,
such as `encryption.kms-type`
+ returned from `/v1/config`. The `config` map contains
provider-specific properties for the
+ selected key-management provider.
+
+ Clients should select the credential config by matching KMS key
identifiers referenced by table
+ encryption metadata, such as `EncryptedKey.encrypted-by-id`, against
`kms-key-ids`. If no
+ key-management credential matches a KMS key ID required by table
encryption metadata, the client
+ must fail the operation with an authorization or missing-credential
error.
Review Comment:
"Must" is too strong here, IMHO. Why do we require the client to fail on
something that is essentially a server error? If the client has other means of
accessing the necessary file, why not allow it to proceed with the operation?
I'd say "can" here. WDYT?
--
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]
