You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Hugo-WB (via GitHub)" <gi...@apache.org> on 2026/06/16 16:47:50 UTC
[PR] Core: Only add required encrypted keys to metadata [iceberg]
Hugo-WB opened a new pull request, #16837:
URL: https://github.com/apache/iceberg/pull/16837
Addresses: https://github.com/apache/iceberg/issues/16352
After putting up: https://github.com/apache/iceberg/pull/16353 which cleaned encryption keys at expire_snapshot time by removing the encryption key associated with the removed snapshot.
When writing E2E tests for above, it wasn't working due to [HiveTableOperations::doCommit](https://github.com/apache/iceberg/blob/69063fb162e835c3dc1ff7bbcf5526029771c6d0/hive-metastore/src/main/java/org/apache/iceberg/hive/HiveTableOperations.java#L253) always adding all keys from the EncryptionManager into the TableMetadata.
From my rough understanding, sorry if this is wrong 😅, `EncryptionManager` is independent of `TableMetadata` and they are only reconciled at commit time.
This leads to issues when trying to remove keys. Even if the EncryptedKey is removed from the base metadata, and the new `metadata` does not have the key, since the `EncryptionManager` is built on top of the base metadata, it will still have the old keys.
This PR proposes to fix the above by filtering the keys added to the metadata file to only the keys that are still referenced/needed by the new metadata file.
This effectively does cleanup at runtime.
Ideas for alternative solutions (Open to ideas here, I think I have a rather limited view here of how things are correlated with each-other):
- Rewrite EncryptionManager to be built on top of TableMetadata. Use TableMetadata as source of truth.
- EncryptionManager does not store state, delegates to TableMetadata.
- Alterations to EncryptionManager are immediately reflected within its version of TableMetadata
- E.g. adding encryption keys or removing encryption keys.
- Rewrite EncryptionManager to be more tightly coupled with TableMetadata.
- Such that when RemoveSnapshot -> remove encryption key from EncryptionManager.
- Keep the "adding of all encryption keys from EncryptionManager" at commit time.
This also related to the REST catalog implementation of encryption: https://github.com/apache/iceberg/pull/13225
The above PR has the same issue as the Hive implementation, where it will always add all keys from the `EncryptionManager`.
--
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] Core: Only add required encrypted keys to metadata [iceberg]
Posted by "Hugo-WB (via GitHub)" <gi...@apache.org>.
Hugo-WB closed pull request #16837: Core: Only add required encrypted keys to metadata
URL: https://github.com/apache/iceberg/pull/16837
--
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] Core: Only add required encrypted keys to metadata [iceberg]
Posted by "Hugo-WB (via GitHub)" <gi...@apache.org>.
Hugo-WB commented on PR #16837:
URL: https://github.com/apache/iceberg/pull/16837#issuecomment-4732584854
Closing in favor of: https://github.com/apache/iceberg/pull/16353
I think makes sense to just include it there.
I was also misunderstanding that this would fix the expiration issue on this own, but since we only restricting the keys we are adding, this doesn't solve removing encryption keys on it's own.
--
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