mbutrovich commented on code in PR #2701:
URL: https://github.com/apache/iceberg-rust/pull/2701#discussion_r3667471552
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -85,19 +90,34 @@ impl ParquetWriterBuilder {
/// Currently translates the content-defined-chunking keys
/// (`write.parquet.content-defined-chunking.*`); other keys fall back to
/// parquet-rs defaults.
- pub fn from_table_properties(table_props: &TableProperties, schema:
SchemaRef) -> Self {
+ ///
+ /// When `encryption.key-id` is set, records the DEK length.
Review Comment:
Doc comment above this says "When `encryption.key-id` is set, records the
DEK length." Accurate, but might be worth a second line noting that the DEK
itself is generated per-file in `build()` (line ~138), not here:
`from_table_properties` only records the *size* to use later. Minor, only
raising it because it took a moment to trace where the actual key material gets
minted while checking comment above.
##########
crates/iceberg/src/writer/file_writer/parquet_writer.rs:
##########
@@ -85,19 +90,34 @@ impl ParquetWriterBuilder {
/// Currently translates the content-defined-chunking keys
/// (`write.parquet.content-defined-chunking.*`); other keys fall back to
/// parquet-rs defaults.
- pub fn from_table_properties(table_props: &TableProperties, schema:
SchemaRef) -> Self {
+ ///
+ /// When `encryption.key-id` is set, records the DEK length.
+ pub fn from_table_properties(table_props: &TableProperties, schema:
SchemaRef) -> Result<Self> {
let cdc = table_props.cdc_enabled.then_some(CdcOptions {
min_chunk_size: table_props.cdc_min_chunk_size,
max_chunk_size: table_props.cdc_max_chunk_size,
norm_level: table_props.cdc_norm_level,
});
+
// TODO: translate the remaining write.parquet.* keys (e.g.
compression-codec,
// row-group-size-bytes, page-size-bytes).
// This constructor is intended to be the single place that maps them.
let props = WriterProperties::builder()
.set_content_defined_chunking(cdc)
.build();
- Self::new_with_match_mode(props, schema, FieldMatchMode::Id)
+
+ let data_encryption_key_size = table_props
+ .encryption_key_id
+ .is_some()
+ .then(||
AesKeySize::from_key_length(table_props.encryption_data_key_length))
+ .transpose()?;
Review Comment:
This derives whether to encrypt purely from
`table_props.encryption_key_id.is_some()`, independent of whether the `Table`
this came from actually has an `EncryptionManager`.
Traced this through: today the two can't disagree at the point a `Table` is
built. `EncryptionManager::from_table_metadata`
(`crates/iceberg/src/encryption/manager.rs:122-136`) returns an error if
`encryption.key-id` is set on the table's metadata but no `KeyManagementClient`
was supplied to `TableBuilder`. So a freshly-built `Table` always has
`encryption_manager()` in sync with
`table.metadata().table_properties().encryption_key_id`.
But `Table::with_metadata()` (`crates/iceberg/src/table.rs:212-215`) swaps
in new metadata without recomputing `encryption_manager` (that field is only
set once, in `TableBuilder::build()`). If a commit turns on `encryption.key-id`
for the first time and the caller keeps using the same in-memory `Table` handle
afterward (via `with_metadata`) rather than reloading from the catalog,
`ParquetWriterBuilder::from_table_properties` would see the new
`encryption_key_id` and start minting DEKs and encrypting data files, while
`transaction/snapshot.rs`'s manifest writer (which branches on
`self.table.encryption_manager()`, e.g.
`crates/iceberg/src/transaction/snapshot.rs:257`) would still take the
unencrypted path off the stale cached value. That would leave a freshly
generated, unwrapped per-file DEK sitting in a plaintext manifest, the thing
the two-layer envelope design is built to avoid.
This is in `table.rs`, outside this PR's diff, so it isn't this PR's bug to
fix, and it's possible normal usage always reloads the `Table` from the catalog
after a property-changing commit and never hits this. Worth either confirming
that path is genuinely unreachable, or filing an issue to make `with_metadata`
keep `encryption_manager` in sync (or recompute it lazily from current metadata
+ a stored `kms_client`, rather than caching once at construction).
--
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]
