You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "aarushigupta132 (via GitHub)" <gi...@apache.org> on 2026/06/19 19:10:55 UTC
[PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
aarushigupta132 opened a new pull request, #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## What changes are included in this PR?
<!--
Provide a summary of the modifications in this PR. List the main changes such as new features, bug fixes, refactoring, or any other updates.
-->
## Are these changes tested?
<!--
Specify what test covers (unit test, integration test, etc.).
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "aarushigupta132 (via GitHub)" <gi...@apache.org>.
aarushigupta132 commented on code in PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677#discussion_r3578331355
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -443,28 +443,31 @@ impl<'a> SnapshotProducer<'a> {
let manifest_list_path = self.generate_manifest_list_file_path(0);
let next_seq_num = self.table.metadata().next_sequence_number();
let first_row_id = self.table.metadata().next_row_id();
- let writer = self
+
+ let raw_output = self
.table
.file_io()
- .new_output(manifest_list_path.clone())?
- .writer()
- .await?;
+ .new_output(manifest_list_path.clone())?;
+
+ let (writer, encrypted) = if let Some(em) = self.table.encryption_manager() {
+ let enc = em.encrypt(raw_output);
+ (enc.writer().await?, Some((em, enc)))
Review Comment:
agreed π , addressed
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677#discussion_r3578148166
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -485,6 +488,28 @@ impl<'a> SnapshotProducer<'a> {
let writer_next_row_id = manifest_list_writer.next_row_id();
manifest_list_writer.close().await?;
+ let mut updates = Vec::new();
+
+ let encryption_key_id = match encrypted {
+ Some((em, enc)) => {
+ let key_id = em
+ .encrypt_manifest_list_key_metadata(enc.key_metadata())
+ .await?;
+
+ updates.extend(em.with_encryption_keys(|keys| {
+ keys.values()
+ .filter(|k| self.table.metadata().encryption_key(k.key_id()).is_none())
+ .map(|k| TableUpdate::AddEncryptionKey {
+ encryption_key: k.clone(),
+ })
+ .collect::<Vec<_>>()
+ }));
+
+ Some(key_id)
+ }
+ None => None,
+ };
Review Comment:
Consider moving this to a function and unit testing it.
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -443,28 +443,31 @@ impl<'a> SnapshotProducer<'a> {
let manifest_list_path = self.generate_manifest_list_file_path(0);
let next_seq_num = self.table.metadata().next_sequence_number();
let first_row_id = self.table.metadata().next_row_id();
- let writer = self
+
+ let raw_output = self
.table
.file_io()
- .new_output(manifest_list_path.clone())?
- .writer()
- .await?;
+ .new_output(manifest_list_path.clone())?;
+
+ let (writer, encrypted) = if let Some(em) = self.table.encryption_manager() {
+ let enc = em.encrypt(raw_output);
+ (enc.writer().await?, Some((em, enc)))
Review Comment:
Rather than returning a tuple of tuples, can we just return the key metadata and the writer?
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
@@ -443,28 +443,31 @@ impl<'a> SnapshotProducer<'a> {
let manifest_list_path = self.generate_manifest_list_file_path(0);
let next_seq_num = self.table.metadata().next_sequence_number();
let first_row_id = self.table.metadata().next_row_id();
- let writer = self
+
+ let raw_output = self
.table
.file_io()
- .new_output(manifest_list_path.clone())?
- .writer()
- .await?;
+ .new_output(manifest_list_path.clone())?;
+
+ let (writer, encrypted) = if let Some(em) = self.table.encryption_manager() {
+ let enc = em.encrypt(raw_output);
Review Comment:
```suggestion
let encrypted_output = em.encrypt(raw_output);
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677#discussion_r3603531193
##########
crates/iceberg/src/transaction/append.rs:
##########
@@ -402,6 +403,91 @@ mod tests {
);
}
+ fn make_v3_encrypted_table() -> Table {
+ let json = fs::read_to_string(format!(
+ "{}/testdata/table_metadata/TableMetadataV3ValidMinimal.json",
Review Comment:
There is an encrypted metadata fixture already, are we able to use that?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677#discussion_r3604856695
##########
crates/iceberg/src/transaction/append.rs:
##########
@@ -402,6 +404,115 @@ mod tests {
);
}
+ /// See `testdata/manifests_lists/README.md`.
+ const FIXTURE_MASTER_KEY_ID: &str = "master-1";
+ const FIXTURE_MASTER_KEY_BYTES: [u8; 16] = [
+ 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e,
+ 0x0f,
+ ];
+
+ async fn make_v3_encrypted_table() -> Table {
+ let json = fs::read_to_string(format!(
+ "{}/testdata/table_metadata/TableMetadataV3ValidEncryption.json",
+ env!("CARGO_MANIFEST_DIR")
+ ))
+ .unwrap();
+ let metadata = serde_json::from_str::<TableMetadata>(&json).unwrap();
+
+ let kms = MemoryKeyManagementClient::new();
+ kms.add_master_key_bytes(
+ FIXTURE_MASTER_KEY_ID,
+ SensitiveBytes::new(FIXTURE_MASTER_KEY_BYTES),
+ )
+ .unwrap();
+
+ let file_io = FileIO::new_with_memory();
+
+ let manifest_list_bytes = fs::read(format!(
+ "{}/testdata/manifests_lists/manifest-list-v3-encrypted.avro",
+ env!("CARGO_MANIFEST_DIR")
+ ))
+ .unwrap();
+ let parent_manifest_list = metadata.current_snapshot().unwrap().manifest_list();
+ file_io
+ .new_output(parent_manifest_list)
+ .unwrap()
+ .write(manifest_list_bytes.into())
+ .await
+ .unwrap();
+
+ Table::builder()
+ .metadata(metadata)
+ .metadata_location("memory:///table/metadata/v1.json")
+ .identifier(TableIdent::from_strs(["ns1", "enc"]).unwrap())
+ .file_io(file_io)
+ .kms_client(Arc::new(kms))
+ .runtime(test_runtime())
+ .build()
+ .unwrap()
+ }
+
+ #[tokio::test]
+ async fn test_commit_with_encryption_adds_keys_and_records_snapshot_key_id() {
Review Comment:
Nice test!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk merged PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "aarushigupta132 (via GitHub)" <gi...@apache.org>.
aarushigupta132 commented on code in PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677#discussion_r3603727410
##########
crates/iceberg/src/transaction/append.rs:
##########
@@ -402,6 +403,91 @@ mod tests {
);
}
+ fn make_v3_encrypted_table() -> Table {
+ let json = fs::read_to_string(format!(
+ "{}/testdata/table_metadata/TableMetadataV3ValidMinimal.json",
Review Comment:
Thanks for the direction. I looked at the fixture, but given it is meant for the read path, I felt that it is more work to make the test work with it. I have to add a separate append to use it, meaning figure out a way to read the parent as `current-snapshot-id` already points at a encrytped snapshot?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "aarushigupta132 (via GitHub)" <gi...@apache.org>.
aarushigupta132 commented on code in PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677#discussion_r3571416918
##########
crates/iceberg/src/spec/manifest_list/writer.rs:
##########
@@ -136,6 +137,56 @@ impl ManifestListWriter {
)
}
+ /// Construct a v1 [`ManifestListWriter`] that writes to an [`EncryptedOutputFile`].
+ ///
+ /// Clone `encrypted_output.key_metadata()` before calling β the wrapper is
+ /// consumed, and the metadata is needed to call
+ /// `EncryptionManager::encrypt_manifest_list_key_metadata` after [`Self::close`].
+ pub async fn v1_from_encrypted(
+ encrypted_output: EncryptedOutputFile,
+ snapshot_id: i64,
+ parent_snapshot_id: Option<i64>,
+ ) -> Result<Self> {
+ let writer = encrypted_output.writer().await?;
+ Ok(Self::v1(writer, snapshot_id, parent_snapshot_id))
+ }
+
+ /// Construct a v2 [`ManifestListWriter`] that writes to an [`EncryptedOutputFile`].
+ /// See [`Self::v1_from_encrypted`] for the key-metadata contract.
+ pub async fn v2_from_encrypted(
+ encrypted_output: EncryptedOutputFile,
+ snapshot_id: i64,
+ parent_snapshot_id: Option<i64>,
+ sequence_number: i64,
+ ) -> Result<Self> {
+ let writer = encrypted_output.writer().await?;
+ Ok(Self::v2(
+ writer,
+ snapshot_id,
+ parent_snapshot_id,
+ sequence_number,
+ ))
+ }
+
+ /// Construct a v3 [`ManifestListWriter`] that writes to an [`EncryptedOutputFile`].
+ /// See [`Self::v1_from_encrypted`] for the key-metadata contract.
+ pub async fn v3_from_encrypted(
Review Comment:
thank you for the review π , i have addressed this in the latest commit
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677#discussion_r3582796503
##########
crates/iceberg/src/transaction/snapshot.rs:
##########
Review Comment:
Is it possible to add a commit test to show that we add the correct updates?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(encryption) [12/N] write encrypted manifest list [iceberg-rust]
Posted by "xanderbailey (via GitHub)" <gi...@apache.org>.
xanderbailey commented on code in PR #2677:
URL: https://github.com/apache/iceberg-rust/pull/2677#discussion_r3471445966
##########
crates/iceberg/src/spec/manifest_list/writer.rs:
##########
@@ -136,6 +137,56 @@ impl ManifestListWriter {
)
}
+ /// Construct a v1 [`ManifestListWriter`] that writes to an [`EncryptedOutputFile`].
+ ///
+ /// Clone `encrypted_output.key_metadata()` before calling β the wrapper is
+ /// consumed, and the metadata is needed to call
+ /// `EncryptionManager::encrypt_manifest_list_key_metadata` after [`Self::close`].
+ pub async fn v1_from_encrypted(
+ encrypted_output: EncryptedOutputFile,
+ snapshot_id: i64,
+ parent_snapshot_id: Option<i64>,
+ ) -> Result<Self> {
+ let writer = encrypted_output.writer().await?;
+ Ok(Self::v1(writer, snapshot_id, parent_snapshot_id))
+ }
+
+ /// Construct a v2 [`ManifestListWriter`] that writes to an [`EncryptedOutputFile`].
+ /// See [`Self::v1_from_encrypted`] for the key-metadata contract.
+ pub async fn v2_from_encrypted(
+ encrypted_output: EncryptedOutputFile,
+ snapshot_id: i64,
+ parent_snapshot_id: Option<i64>,
+ sequence_number: i64,
+ ) -> Result<Self> {
+ let writer = encrypted_output.writer().await?;
+ Ok(Self::v2(
+ writer,
+ snapshot_id,
+ parent_snapshot_id,
+ sequence_number,
+ ))
+ }
+
+ /// Construct a v3 [`ManifestListWriter`] that writes to an [`EncryptedOutputFile`].
+ /// See [`Self::v1_from_encrypted`] for the key-metadata contract.
+ pub async fn v3_from_encrypted(
Review Comment:
I donβt think we need these new constructors, manifest list writer was refactored to take a FileWrite so we can just construct with the correct encrypting file writer
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org