feat(encryption): [9/N] Read encrypted manifest lists by xanderbailey · Pull Request #4159 · apache/iceberg-python

xanderbailey · 2026-10-26T13:51:55Z

Working towards #3222

Rationale for this change

We need to be able to read encrypted manifest lists which means we need the Table to be able to construct an EncryptionManager.

Table takes an optional kms_client, and Table.encryption_manager builds the manager from the table's current metadata.
Snapshot.manifests takes an optional encryption_manager. When the snapshot has a key-id the manifest list is decrypted, and it raises if no manager is given.
Scans and inspect pass the table's encryption manager through.
The manager is passed around separately rather than wrapped into FileIO, matching iceberg-rust ([7/N] feat(encryption) [7/N] Read encrypted manifest list iceberg-rust#2453). <- this is the main design decision to make for this PR and is worth considering carefully. I've chosen to take the approach we used in rust because it actually simplifies things down the line a lot but very happy to hear arguments for the alternative.

Catalogs don't pass a KMS client yet, that comes in a follow-up.

Are these changes tested?

Yes. A manifest list encrypted by iceberg-rust is checked in under tests/table/encryption, and scanning and inspecting that table reads it. There's also a round trip of an encrypted manifest list with entries.

Are there any user-facing changes?

Table and the scans take a new optional argument. Nothing changes for unencrypted tables.

xanderbailey · 2026-10-29T15:47:17Z

@kevinjqliu would be interested on your take here for how this looks vs how Java does it. I'm pretty happy with following the same pattern we have in rust but curious for your thoughts.

mbutrovich

Thanks @xanderbailey, first pass.

mbutrovich · 2026-11-02T22:03:53Z

+    def encryption_manager(self) -> EncryptionManager | None:
+        """The table's encryption manager, or None if the table is not encrypted.
+
+        Raises:
+            ValueError: If the table is encrypted but no key management client was provided.
+        """
+        from pyiceberg.encryption.manager import EncryptionManager
+
+        return EncryptionManager.from_table_metadata(self._kms_client, self.metadata)


Could we build the manager once per metadata version instead of on every access? EncryptionManager caches unwrapped KEKs for an hour (_unwrap_kek), but each access here creates a new manager with an empty cache, so every manifest list read calls KeyManagementClient.unwrap_key again. I wrapped the test KMS in table_v3_encrypted with a call counter. Three scan().plan_files() calls made three unwrap_key calls. inspect.all_manifests() over five snapshots made five, because _generate_manifests_table reads the property once per snapshot.

Table.metadata is reassigned on commit and refresh, so caching the manager next to the metadata it came from keeps test_encryption_manager_follows_current_metadata passing:

if self._encryption_manager is None or self._encryption_manager[0] is not self.metadata: self._encryption_manager = (self.metadata, EncryptionManager.from_table_metadata(self._kms_client, self.metadata)) return self._encryption_manager[1]

Here self._encryption_manager starts as None in __init__. With that change, three scans followed by manifests(), entries(), and all_manifests() made one unwrap_key call.

Ah this kind of thing might work, I was worried about keeping the em fresh as metadata changes so thought I'd do the expensive thing first but let me look into what you've suggested here. Thanks Matt

mbutrovich · 2026-11-02T22:04:05Z


        snapshot = self._get_snapshot(snapshot_id)
-        spec_ids = {manifest.partition_spec_id for manifest in snapshot.manifests(self.tbl.io)}
+        spec_ids = {manifest.partition_spec_id for manifest in snapshot.manifests(self.tbl.io, self.tbl.encryption_manager)}


Should the DataScan a few lines below (L401-L407) also get encryption_manager=self.tbl.encryption_manager? This line decrypts the manifest list, but that scan is built without the manager, so _plan_manifest_entries calls snapshot.manifests(io, None). On the head commit, table_v3_encrypted.inspect.partitions() raises ValueError: Snapshot 1 is encrypted, but no encryption manager was provided. With the argument added, it returns an empty table.

As I read it, this is the main cost of passing the manager separately, which bears on the design question in the description. Every caller of Snapshot.manifests and every scan constructor has to remember it, and a missed one fails only on encrypted tables. In iceberg-rust, ManifestListReader::new is pub(crate), and callers get a reader from Table::manifest_list_reader, which supplies the manager, so they can't leave it out.

mbutrovich · 2026-11-02T22:04:23Z

+def test_inspect_encrypted_table(table_v3_encrypted: Table) -> None:
+    assert table_v3_encrypted.inspect.manifests().num_rows == 0
+    assert table_v3_encrypted.inspect.all_manifests().num_rows == 0


Could this also cover entries() and partitions()? Both read the manifest list through the paths this PR changes, and partitions() would have caught the missing manager above.

Suggested change

def test_inspect_encrypted_table(table_v3_encrypted: Table) -> None:

assert table_v3_encrypted.inspect.manifests().num_rows == 0

assert table_v3_encrypted.inspect.all_manifests().num_rows == 0

def test_inspect_encrypted_table(table_v3_encrypted: Table) -> None:

assert table_v3_encrypted.inspect.manifests().num_rows == 0

assert table_v3_encrypted.inspect.all_manifests().num_rows == 0

assert table_v3_encrypted.inspect.entries().num_rows == 0

assert table_v3_encrypted.inspect.partitions().num_rows == 0

inspect.partitions(), the CLI files output and purge_table read manifest lists without the encryption manager, so they failed on encrypted tables. purge_table now builds the manager before dropping the table, so a missing KMS client no longer leaves a dropped table with unpurged files.

Building a manager on every access discarded its KEK cache, so each manifest list read called the KMS again.

…nifest-list # Conflicts: # tests/cli/test_output.py

kevinjqliu

LGTM!

I have slight nit regarding _encryption_manager tuple. It feels like a premature optimization

kevinjqliu · 2026-11-05T20:27:04Z

        self.catalog = catalog
        self.config = config
+        self._kms_client = kms_client
+        self._encryption_manager: tuple[TableMetadata, EncryptionManager | None] | None = None


nit: this seems like a code smell to me.

I'm curious why _encryption_manager is a tuple, and the encryption_manager property returns just the second part of the tuple.
the first part of the tuple is table metadata which is also already represented by self.metadata.

@property def metadata(self) -> TableMetadata: return self._metadata @metadata.setter def metadata(self, value: TableMetadata) -> None: self._metadata = value self.__dict__.pop("encryption_manager", None) @functools.cached_property def encryption_manager(self) -> EncryptionManager | None: ...

Claude had this alternative idea, I do think caching the EM might be a good idea rather than recreating it on every get? WDYT?

I pushed a commit (068ecfd) to use NamedTuple instead. I find it easier to read, lmk what you think!

kevinjqliu · 2026-11-06T17:21:29Z

i double check the content of the avro file. looks fine, used for testing

…manifests

Replace the bare (metadata, manager) tuple with a NamedTuple so the property reads as "rebuild when the metadata changed since it was built", while still swapping the pair in a single assignment. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

kevinjqliu · 2026-11-06T23:27:56Z

adding changelog label to PR for the new optional param in Table(kms_client=…)

kevinjqliu · 2026-11-06T23:30:34Z

Thanks @xanderbailey this looks good to me, and thanks @mbutrovich for the review 😄

kevinjqliu · 2026-11-06T23:41:07Z

+        if cached is None or cached.metadata is not self.metadata:
+            cached = self._encryption_manager = _CachedEncryptionManager(
+                self.metadata, EncryptionManager.from_table_metadata(self._kms_client, self.metadata)
+            )
+        return cached.manager


this logic looks a bit weird for the non-encrypted case. EncryptionManager.from_table_metadata returns None when the table is not encrypted, and the whole function returns None

Read encrypted manifest lists

0ad7405

mbutrovich reviewed Nov 2, 2026

View reviewed changes

kevinjqliu reviewed Nov 4, 2026

View reviewed changes

Comment thread tests/table/encryption/README.md Resolved

xanderbailey added 3 commits November 4, 2026 14:29

Reuse the encryption manager until table metadata changes

1d711e5

Building a manager on every access discarded its KEK cache, so each manifest list read called the KMS again.

Merge remote-tracking branch 'origin/main' into xb/encryption-read-ma…

8049b89

…nifest-list # Conflicts: # tests/cli/test_output.py

kevinjqliu approved these changes Nov 6, 2026

View reviewed changes

xanderbailey and others added 2 commits November 6, 2026 14:44

Read encrypted and plain manifest lists through one path in Snapshot.…

b0b4160

…manifests

kevinjqliu added the changelog Indicates that the PR introduces changes that require an entry in the changelog. label Nov 6, 2026

kevinjqliu enabled auto-merge November 6, 2026 23:30

kevinjqliu added this pull request to the merge queue Nov 6, 2026

kevinjqliu reviewed Nov 6, 2026

View reviewed changes

Merged via the queue into apache:main with commit 718c790 Nov 6, 2026
20 checks passed

Conversation

xanderbailey commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

Are these changes tested?

Are there any user-facing changes?

Uh oh!

xanderbailey commented Oct 29, 2026

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!

Uh oh!

kevinjqliu 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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Nov 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinjqliu commented Nov 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xanderbailey commented Oct 26, 2026 •

edited

Loading

kevinjqliu commented Nov 6, 2026 •

edited

Loading