mbutrovich commented on code in PR #2650:
URL: https://github.com/apache/iceberg-rust/pull/2650#discussion_r3501435439
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -152,6 +153,28 @@ pub trait CatalogBuilder: Default + Debug + Send + Sync {
/// ```
fn with_storage_factory(self, storage_factory: Arc<dyn StorageFactory>) ->
Self;
+ /// Set a [`KmsClientFactory`] to enable table encryption.
+ ///
+ /// When provided, the catalog calls the factory once during
+ /// [`load`](Self::load) with the catalog properties to create a shared
+ /// [`KeyManagementClient`](crate::encryption::KeyManagementClient).
+ /// That client is then passed to each table's `TableBuilder` so tables
+ /// with `encryption.key-id` set can construct an `EncryptionManager`.
+ ///
+ /// # Example
+ ///
+ /// ```rust,ignore
+ /// use iceberg::CatalogBuilder;
+ /// use iceberg::encryption::kms::KmsClientFactory;
+ /// use std::sync::Arc;
+ ///
+ /// let catalog = MyCatalogBuilder::default()
+ /// .with_kms_client_factory(Arc::new(MyKmsClientFactory))
+ /// .load("my_catalog", props)
+ /// .await?;
+ /// ```
+ fn with_kms_client_factory(self, kms_client_factory: Arc<dyn
KmsClientFactory>) -> Self;
Review Comment:
As [11/N] of a stack, wiring the catalog KMS client to the Table's
EncryptionManager is the whole job here. The read path can't consume it until a
later part connects the EncryptionManager to file reads (the FileKeyResolver
seam from your prototype), so there's nothing to change here. The one thing
worth keeping in mind for that later PR: the seam should be feedable by either
this KmsClientFactory-built client (native KMS) or an external resolver (a
Comet-style JVM key source), since Comet resolves keys JVM-side and would not
set a KmsClientFactory. Flagging it now only so the two entry points converge
on one EncryptionManager/resolver seam rather than forking.
##########
crates/iceberg/src/catalog/mod.rs:
##########
@@ -152,6 +153,28 @@ pub trait CatalogBuilder: Default + Debug + Send + Sync {
/// ```
fn with_storage_factory(self, storage_factory: Arc<dyn StorageFactory>) ->
Self;
+ /// Set a [`KmsClientFactory`] to enable table encryption.
+ ///
+ /// When provided, the catalog calls the factory once during
+ /// [`load`](Self::load) with the catalog properties to create a shared
+ /// [`KeyManagementClient`](crate::encryption::KeyManagementClient).
+ /// That client is then passed to each table's `TableBuilder` so tables
+ /// with `encryption.key-id` set can construct an `EncryptionManager`.
+ ///
+ /// # Example
+ ///
+ /// ```rust,ignore
+ /// use iceberg::CatalogBuilder;
+ /// use iceberg::encryption::kms::KmsClientFactory;
+ /// use std::sync::Arc;
+ ///
+ /// let catalog = MyCatalogBuilder::default()
+ /// .with_kms_client_factory(Arc::new(MyKmsClientFactory))
Review Comment:
Adding this as a required method breaks external CatalogBuilder
implementors. A default impl that ignored the factory would silently drop
encryption config, which is worse, so requiring it is probably the right call.
Just worth calling out the breakage explicitly (changelog) so downstream
implementors are not surprised.
##########
crates/iceberg/src/encryption/kms/factory.rs:
##########
@@ -0,0 +1,51 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Factory trait for creating [`KeyManagementClient`] instances.
+
+use std::collections::HashMap;
+use std::fmt::Debug;
+use std::sync::Arc;
+
+use async_trait::async_trait;
+
+use super::KeyManagementClient;
+use crate::Result;
+
+/// Factory for creating a [`KeyManagementClient`] from catalog properties.
+///
+/// Replaces Java's reflection-based `encryption.kms-impl` +
`initialize(properties)`
+/// pattern. Users provide an implementation of this trait to the catalog
builder via
+///
[`CatalogBuilder::with_kms_client_factory`](crate::CatalogBuilder::with_kms_client_factory).
+///
+/// The catalog calls [`create_kms_client`](Self::create_kms_client) **once**
during
+/// catalog initialization with the catalog's properties. The resulting client
is
+/// shared across all tables in the catalog and passed to each table's
+/// [`EncryptionManager`](crate::encryption::EncryptionManager) via
+/// `TableBuilder::kms_client(...)`.
+#[async_trait]
+pub trait KmsClientFactory: Debug + Send + Sync {
+ /// Create a [`KeyManagementClient`] from catalog properties.
+ ///
+ /// Called once during catalog initialization. Properties may include
+ /// KMS endpoint, region, credentials, or any backend-specific
+ /// configuration needed to construct the client.
+ async fn create_kms_client(
Review Comment:
I first wondered if this should select the KMS per table, but the spec model
is catalog-scoped KMS: per docs/docs/encryption.md the KMS is a catalog
property (encryption.kms-type / encryption.kms-impl), and the per-table axis is
the encryption.key-id master-key property, not a per-table KMS impl. So one
client per catalog is right, and create_kms_client(&self.config.props) matches
Java's KeyManagementClient.initialize(properties = catalog properties) exactly.
No change on scope. The one thing to confirm is the other axis: that per-table
encryption.key-id is honored when the EncryptionManager is built (the PR body
implies it is). The recurring "key_metadata is implementation-specific" point
(relevant to #2584/#2586, not this PR) is spec-backed too: format/spec.md field
131 and the encryption-keys section.
##########
crates/iceberg/src/encryption/kms/memory.rs:
##########
@@ -142,6 +143,39 @@ impl MemoryKeyManagementClient {
}
}
+/// Factory for creating [`MemoryKeyManagementClient`] instances.
+///
+/// This factory creates a fresh in-memory KMS client for each call.
+/// Useful for testing encryption flows without a real KMS backend.
+#[derive(Debug, Clone)]
+pub struct MemoryKmsClientFactory {
+ master_keys: Arc<RwLock<HashMap<String, SensitiveBytes>>>,
+ master_key_size: AesKeySize,
+}
+
+impl MemoryKmsClientFactory {
Review Comment:
MemoryKmsClientFactory is the natural test seam, but I do not see a test
that a factory-configured catalog yields a table whose EncryptionManager
actually holds the client. A small test (build catalog
with_kms_client_factory(MemoryKmsClientFactory), load a table, assert the
client reached the EncryptionManager) would lock the wiring in. Good fit for
the loader test suite.
--
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]
