feat(encryption) [7/N] Read encrypted manifest list#2453
Conversation
| io: &FileIO, | ||
| metadata: &TableMetadata, | ||
| metadata_location: Option<&str>, | ||
| encryption_manager: Option<&EncryptionManager>, |
There was a problem hiding this comment.
public api change here
| &self, | ||
| file_io: &FileIO, | ||
| table_metadata: &TableMetadata, | ||
| encryption_manager: Option<&EncryptionManager>, |
There was a problem hiding this comment.
public api change here.
| /// | ||
| /// 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( |
There was a problem hiding this comment.
Wasn't exactly sure if this should live here or if it's a concern of the EM?
| metadata_location: Option<String>, | ||
| metadata: Option<TableMetadataRef>, | ||
| identifier: Option<TableIdent>, | ||
| kms_client: Option<Arc<dyn KeyManagementClient>>, |
There was a problem hiding this comment.
Thanks for working on the encryption support @xanderbailey, this is really great.
I think TableMetadata::table_properties has all the information we need to construct either a KeyManagementClient or a EncryptionManager. Do you think we could build them as part of the builder instead of passing KeyManagementClient in the API?
There was a problem hiding this comment.
Thanks for taking a look! In Java's case the kms is configured at the catalog level (CatalogProperties.ENCRYPTION_KMS_*) and I think we should do the same, the catalog will take some KmsFactory and construct the KMS for the table? WDYT?
2 participants
Which issue does this PR close?
What changes are included in this PR?
Wires encryption support into the
Tabletype so that encrypted manifest lists can be read. Adds anEncryptionManageraccessor onTableand ensures it is supplied wherever manifest lists are loaded from snapshots.Also threads the
EncryptionManagerthroughpurge_table/drop_table_dataso that dropping an encrypted table can read its manifest lists and clean up referenced files correctly.A design decision was made whilst reviewing the RFC to keep the EM out of
FileIoand pass it around separately which turns out to be much cleaner.Note that we aren't exposing any sort of configuration for the KMS when building the tables in the catalogs. That will come later. That PR is deliberately coming later to minimise the public surface area whilst this feature is under development.
Are these changes tested?
Yes