rdblue commented on code in PR #12162:
URL: https://github.com/apache/iceberg/pull/12162#discussion_r1940256313


##########
format/spec.md:
##########
@@ -889,6 +892,9 @@ Table metadata consists of the following fields:
 | _optional_ | _optional_ | _optional_ | **`partition-statistics`**  | A list 
(optional) of [partition statistics](#partition-statistics).                    
                                                                                
                                                                                
                                                                                
                                                          |
 |            |            | _optional_ | **`row-lineage`**           | A 
boolean, defaulting to false, setting whether or not to track the creation and 
updates to rows in the table. See [Row Lineage](#row-lineage).                  
                                                                                
                                                                                
                                                                |
 |            |            | _optional_ | **`next-row-id`**           | A 
`long` higher than all assigned row IDs; the next snapshot's `first-row-id`. 
See [Row Lineage](#row-lineage).                                                
                                                                                
                                                                                
                                                                  |
+|            |            | _optional_ | **`key-cache`**             | A list 
of encryption keys (key-id/key-wrap pairs), used to encrypt the manifest list 
file key metadata. See [Snapshot](#key-metadata-key-id).                        
                                                                                
                                                                                
                                                            |
+|            |            | _optional_ | **`key-id`**                | The ID 
of the encryption key that encrypts the manifest list key metadata. See 
[Snapshot](#key-metadata-key-id).                                               
                                                                                
                                                                                
                                                                  |
+|            |            | _optional_ | **`key-wrap`**              | Wrapped 
(encrypted) metadata encryption key. Wrapping can for example be done in a Mey 
Management Service (KMS).                                                       
                                                                                
                                                                                
                                                          |

Review Comment:
   Are these intended to be fields inside of `key-cache`? I think that keys in 
table metadata should be a list of objects and those objects should have a 
table defining the structure. That's how we've added other structures, like 
schemas: "A list of schemas, stored as objects with `schema-id`".
   
   Something like this:
   ```
   _optional_ | **`encryption-keys`** | A list of encryption keys, stored as 
objects. |
   
   ...
   
   Each encryption key within the `encryption-keys` table metadata field is a 
struct with the following fields:
   | v1/v2 | v3 | Field name | Type | Description |
   |----|----|------------|------|-------------|
   |  | _required_ | **`key-id`** | `string` | ID used to refer to this key in 
table metadata. |
   ...
   ```
   
   One thing we can do to make this more flexible is to allow each key to have 
a key ID that encrypted it, or to indicate that it was wrapped by KMS. That 
would allow using the same `encryption-keys` list for multiple strategies:
   * Wrap each snapshot key using KMS
   * Wrap a table key with KMS that is used to encrypt snapshot keys
       * Re-encrypt with the new KMS key when rotating
       * Leave the existing KMS-wrapped key when rotating



-- 
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]

Reply via email to