singhpk234 commented on code in PR #18503:
URL: https://github.com/apache/iceberg/pull/18503#discussion_r4365588891
##########
format/spec.md:
##########
@@ -2104,6 +2109,11 @@ Snapshot timestamp changes:
* When committed while the table is at version 4 or higher, a snapshot's
`timestamp-ms` must be strictly greater than its parent snapshot's
`timestamp-ms` on the same branch.
* Snapshots with no parent and snapshots that existed before a table was
upgraded to v4 are not constrained.
+Snapshot encryption key changes:
+
+* Writers must store the root manifest file's encryption key directly in the
snapshot's `encryption-key` field and must not write `key-id`
+* Snapshots that existed before a table was upgraded to v4 may continue to use
`key-id`
Review Comment:
why may ? those snapshot are anyways immutable
##########
format/spec.md:
##########
@@ -1124,6 +1126,8 @@ The snapshot summary's `operation` field is used by some
operations, like snapsh
For other optional snapshot summary fields, see [Appendix
F](#optional-snapshot-summary-fields).
+A snapshot's `key-id` and `encryption-key` fields are mutually exclusive and
must not both be present. When committed while the table is at version 4 or
higher, writers must store the snapshot's encryption key in `encryption-key`.
Review Comment:
```suggestion
A snapshot's `key-id` and `encryption-key` fields are mutually exclusive and
both must not be present. When committed while the table is at version 4 or
higher, writers must store the snapshot's encryption key in `encryption-key`.
```
##########
format/spec.md:
##########
@@ -1107,11 +1107,13 @@ A snapshot consists of the following fields:
| _required_ | **`sequence-number`** | A monotonically increasing
long that tracks the order of changes to a table |
| _required_ | **`timestamp-ms`** | A timestamp when the
snapshot was created, used for garbage collection and table inspection |
| _required_ | **`root-manifest`** | The location of the root
manifest for this snapshot |
+ | _required_ | **`root-manifest-size-in-bytes`** | Size of the root
manifest file in bytes |
Review Comment:
can you please elaorate why this is required for encryption ?
##########
format/spec.md:
##########
@@ -1461,18 +1465,19 @@ If a table has no deletes or only deletion vectors,
implementations are encourag
#### Encryption Keys
-Keys used for table encryption can be tracked in table metadata as a list
named `encryption-keys`. The schema of each key is a struct with the following
fields:
-=== "v1 - v3"
- | v1 | v2 | v3 | Field name | Type
| Description |
-
|----|----|------------|-------------------------------|-----------------------|-------------|
- | | | _required_ | **`key-id`** | `string`
| ID of the encryption key |
- | | | _required_ | **`encrypted-key-metadata`** | `string`
| Encrypted key and metadata, base64 encoded [1] |
- | | | _optional_ | **`encrypted-by-id`** | `string`
| Optional ID of the key used to encrypt or wrap `key-metadata` |
- | | | _optional_ | **`properties`** | `map<string,
string>` | A string to string map of additional metadata used by the table's
encryption scheme |
+Keys used for table encryption can be tracked in table metadata as a list
named `encryption-keys`, or embedded directly in the metadata object that
references the encrypted file, such as the `encryption-key` field of a v4
snapshot. The schema of each key is a struct with the following fields:
+=== "v1 - v4"
+ | v1 and v2 | v3 | v4 | Field name |
Type | Description |
+
|-----------|------------|------------|-------------------------------|-----------------------|-------------|
+ | | _required_ | _optional_ | **`key-id`** |
`string` | ID of the encryption key [2] |
+ | | _required_ | _required_ | **`encrypted-key-metadata`** |
`string` | Encrypted key and metadata, base64 encoded [1] |
+ | | _optional_ | _optional_ | **`encrypted-by-id`** |
`string` | Optional ID of the key used to encrypt or wrap
`key-metadata` |
+ | | _optional_ | _optional_ | **`properties`** |
`map<string, string>` | A string to string map of additional metadata used by
the table's encryption scheme |
Notes:
1. The format of encrypted key metadata is determined by the table's
encryption scheme and can be a wrapped format specific to the table's KMS
provider.
+2. `key-id` is required for keys tracked in the table metadata
`encryption-keys` list, because they are referenced by ID. It is not set for
keys embedded directly in the metadata object that references the encrypted
file, such as the `encryption-key` field of a v4 snapshot.
Review Comment:
> `key-id` is required for keys tracked in the table metadata
`encryption-keys` list, because they are referenced by ID
Technically KEK's key-id is tables key-id ... which is redundant here, even
though we have it in the spec here our implementation always
https://github.com/apache/iceberg/blob/0e4a68cec1affe64aa711f67351f0f8254b5d00a/core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java#L116
--
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]
