blackmwk commented on code in PR #2650:
URL: https://github.com/apache/iceberg-rust/pull/2650#discussion_r3542768478
##########
crates/iceberg/src/table.rs:
##########
@@ -113,8 +113,15 @@ impl TableBuilder {
/// If the table metadata has the `encryption.key-id` property set, a
/// [`KeyManagementClient`] must be provided here so the table can build
/// an [`EncryptionManager`]; otherwise [`Self::build`] will return an
error.
- pub fn kms_client(mut self, kms_client: Arc<dyn KeyManagementClient>) ->
Self {
- self.kms_client = Some(kms_client);
+ ///
+ /// Accepts either an `Arc<dyn KeyManagementClient>` or an
+ /// `Option<Arc<dyn KeyManagementClient>>`, so catalogs that hold an
+ /// optional client can forward it directly. Passing `None` is a no-op.
+ pub fn kms_client(
Review Comment:
This change is odd to me, we could just skip this method call if it's None.
##########
crates/iceberg/src/table.rs:
##########
@@ -113,8 +113,15 @@ impl TableBuilder {
/// If the table metadata has the `encryption.key-id` property set, a
/// [`KeyManagementClient`] must be provided here so the table can build
/// an [`EncryptionManager`]; otherwise [`Self::build`] will return an
error.
- pub fn kms_client(mut self, kms_client: Arc<dyn KeyManagementClient>) ->
Self {
- self.kms_client = Some(kms_client);
+ ///
+ /// Accepts either an `Arc<dyn KeyManagementClient>` or an
+ /// `Option<Arc<dyn KeyManagementClient>>`, so catalogs that hold an
+ /// optional client can forward it directly. Passing `None` is a no-op.
+ pub fn kms_client(
Review Comment:
Also it makes the api inconsistent with other parts.
##########
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 {
+ /// Creates a new factory that will produce clients sharing the given
master keys.
+ pub fn new(kms: &MemoryKeyManagementClient) -> Self {
Review Comment:
This doesn't make sense to me to build a client factory from a client. I
think the correct behavior should be like `MemoryKmsClientFactory` constructor
accepts an `AesKeySize`, and it generate the key table. Then passes it to
`MemoryKmsClient`.
--
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]
