mbutrovich commented on code in PR #2453:
URL: https://github.com/apache/iceberg-rust/pull/2453#discussion_r3275516257
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -199,8 +200,22 @@ impl Snapshot {
&self,
file_io: &FileIO,
table_metadata: &TableMetadata,
+ encryption_manager: Option<&EncryptionManager>,
) -> Result<ManifestList> {
- let manifest_list_content =
file_io.new_input(&self.manifest_list)?.read().await?;
+ let manifest_list_content = match (&self.encryption_key_id,
encryption_manager) {
+ (Some(_), None) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
Review Comment:
Can we do `ErrorKind::PreconditionFailed` or `ErrorKind::Unexpected` here?
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -199,8 +200,22 @@ impl Snapshot {
&self,
file_io: &FileIO,
table_metadata: &TableMetadata,
+ encryption_manager: Option<&EncryptionManager>,
) -> Result<ManifestList> {
- let manifest_list_content =
file_io.new_input(&self.manifest_list)?.read().await?;
+ let manifest_list_content = match (&self.encryption_key_id,
encryption_manager) {
+ (Some(_), None) => {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ "Snapshot has encryption_key_id but no EncryptionManager
configured on Table",
+ ));
+ }
+ (Some(key_id), Some(em)) => {
+ let key_metadata =
em.decrypt_manifest_list_key_metadata(key_id).await?;
+ let input = file_io.new_input(&self.manifest_list)?;
+ EncryptedInputFile::new(input, key_metadata).read().await?
+ }
+ (None, _) => file_io.new_input(&self.manifest_list)?.read().await?,
Review Comment:
It's non-obvious that there could be an intentionally unused
`EncryptionManager` here, since a table could have unencrypted snapshots in its
history. Maybe worth a one-line comment so someone doesn't change this to
`(None, None)`.
##########
crates/iceberg/src/table.rs:
##########
@@ -356,9 +392,58 @@ impl StaticTable {
}
}
+/// If the table metadata sets the `encryption.key-id` property, build an
+/// [`EncryptionManager`] for the table.
+///
+/// Returns `Ok(None)` if the property is not set. Returns an error if the
+/// property is set but no [`KeyManagementClient`] was provided.
+fn maybe_configure_encryption(
+ kms_client: Option<&Arc<dyn KeyManagementClient>>,
+ metadata: &TableMetadataRef,
+) -> Result<Option<Arc<EncryptionManager>>> {
+ let table_properties = metadata.table_properties()?;
+ let Some(table_key_id) = table_properties.encryption_key_id else {
+ return Ok(None);
+ };
+
+ // Encryption is a v3 feature: `encryption-keys` table metadata and the
+ // snapshot `key-id` field are introduced in format version 3.
+ if metadata.format_version() < FormatVersion::V3 {
+ return Err(Error::new(
+ ErrorKind::FeatureUnsupported,
+ format!(
+ "Table encryption requires format version 3, found {}",
+ metadata.format_version()
+ ),
+ ));
+ }
+
+ let kms_client = kms_client.ok_or_else(|| {
+ Error::new(
+ ErrorKind::FeatureUnsupported,
Review Comment:
Same `ErrorKind` feedback as before. Whatever we pick should be consistent.
##########
crates/iceberg/src/catalog/utils.rs:
##########
Review Comment:
Do we have a test for this code path, even just to make sure future
signature changes don't break reading manifest lists?
##########
crates/iceberg/src/spec/snapshot.rs:
##########
@@ -729,4 +745,122 @@ mod tests {
assert_eq!(v2_snapshot.parent_snapshot_id(), None);
assert_eq!(v2_snapshot.schema_id(), None);
}
+
+ use bytes::Bytes;
+
+ use crate::encryption::kms::{KeyManagementClient,
MemoryKeyManagementClient};
Review Comment:
Can we hoist these up near 539?
--
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]
