feat(encryption): [11/N] KMS factory trait and catalog wiring by xanderbailey · Pull Request #2650 · apache/iceberg-rust

xanderbailey · 2026-06-15T09:03:00Z

Which issue does this PR close?

Working towards: Support client-side encryption #2034

What changes are included in this PR?

Adds KmsClientFactory to the catalog builder to allow a catalog to build one KMS client per catalog, this mirrors Java's implementation . Note in rust we don't have reflection so using Java's encryption.kms-impl isn't an option for us. This is a very similar pattern to what we do for the StorageFactory so I believe the pattern is idiomatic for us.

Are these changes tested?

mbutrovich

Thanks @xanderbailey! This reads well. The factory mirrors StorageFactory, the client is created exactly once in load() with errors propagated, and it is forwarded uniformly to every TableBuilder across all six catalogs. The kms_client(impl Into<Option<...>>) forwarding is a nice ergonomic, and the docs are clear. A few non-blocking questions, mostly about how this connects to the read path and to Comet.

mbutrovich · 2026-06-30T19:48:51Z

+    ///     .load("my_catalog", props)
+    ///     .await?;
+    /// ```
+    fn with_kms_client_factory(self, kms_client_factory: Arc<dyn KmsClientFactory>) -> Self;


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.

mbutrovich · 2026-06-30T19:50:08Z

+    /// 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(


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.

mbutrovich · 2026-06-30T19:51:13Z

+    /// use std::sync::Arc;
+    ///
+    /// let catalog = MyCatalogBuilder::default()
+    ///     .with_kms_client_factory(Arc::new(MyKmsClientFactory))


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.

Yeah I suspect this is desired

mbutrovich · 2026-06-30T19:51:32Z

+    master_key_size: AesKeySize,
+}
+
+impl MemoryKmsClientFactory {


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.

Nice idea to have an end-to-end test for the in-memory catalog / kms. Added end-to-end test

xanderbailey · 2026-07-08T07:51:33Z

@blackmwk I think this is ready for review when you have a moment! Thanks in advance!

blackmwk

Thanks @xanderbailey for this pr, just finised first round of review.

blackmwk · 2026-07-08T09:24:47Z

+    /// 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(


This change is odd to me, we could just skip this method call if it's None.

Also it makes the api inconsistent with other parts.

I did it like this because else in all call sites you'd need:

if let Some(c) = &self.kms_client { builder = builder.kms_client(c.clone()); }

which doesn't read very nicely in my opinion.

Would just Option<...> be better rather than the Into?

Seems like we get away with this for other fields because they're actually always present when we use this builder but KMS client is legitimately optional.

It sound reasonable to me. From a user's point, I accept both apis. But from a maintainer's point, I prefer the api without Option, since it's easier to maintain api consistency, otherwise we need to remember that for optional fields, we should accept Option, and other fields we don't.

Sorry maybe I didn't quite follow, what's the recommendation here? You're happy with this API or should we change? To me it doesn't seem crazy for a genuinely optional parameter to have an optional builder.

I mean I'm still in favor of removing the Option, since other apis in this crate adopt this approach, e.g. if a parameter is optional, user just don't call the function to set it. It would be easier to ensure api consistency in this repo.

remove optional builder, let me know what you think.

blackmwk

Thanks @xanderbailey for this pr!

mbutrovich mentioned this pull request Jun 15, 2026

feat(encryption) [9/N] Read encrypted parquet data-files #2584

Open

mbutrovich reviewed Jun 30, 2026

View reviewed changes

xanderbailey added 2 commits July 7, 2026 22:34

feat(encryption): [11/N] KMS factory trait and catalog wiring

36435e9

tests

5bda7fb

xanderbailey force-pushed the xb/kms_factory branch from 03df8c2 to 5bda7fb Compare July 7, 2026 21:34

xanderbailey added 2 commits July 7, 2026 23:01

end-to-end test

9008d7d

fmt

108fd41

xanderbailey requested a review from mbutrovich July 7, 2026 22:10

blackmwk reviewed Jul 8, 2026

View reviewed changes

xanderbailey added 3 commits July 8, 2026 16:30

Update in-memory factory

0e47498

remove optional builder

797890a

fix

1047075

blackmwk approved these changes Jul 9, 2026

View reviewed changes

blackmwk merged commit facecaf into apache:main Jul 9, 2026
21 checks passed

Uh oh!

Conversation

xanderbailey commented Jun 15, 2026

Which issue does this PR close?

What changes are included in this PR?

Are these changes tested?

Uh oh!

mbutrovich left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xanderbailey commented Jul 8, 2026

Uh oh!

blackmwk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blackmwk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants