AWS: KeyManagementClient implementation that works with AWS KMS#13136
AWS: KeyManagementClient implementation that works with AWS KMS#13136amogh-jahagirdar merged 4 commits intoapache:mainfrom
Conversation
|
I will take a look as well. Thanks ! |
|
Sorry for the delay, I'm taking a look |
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
I still need to take a pass over tests but left some comments!
| * encrypting/decrypting keys with a KMS-managed master key, (by referencing its key ID), and for | ||
| * the generation of new encryption keys. | ||
| */ | ||
| public class AwsKeyManagementClient implements KeyManagementClient { |
There was a problem hiding this comment.
I think this will need to be serializable so that it can be used in a distributed setting (since an EncryptionManager instance can also be serialized, and generally we'd expect an EncryptionManager to also contain an instance of KeyManagementClient
There was a problem hiding this comment.
Yeah KeyManagementClient does implement Serializable as is (though looking at the implementation, wonder how necessary that is - it's transient in the StandardEncryptionmanager both now and after #7770, cc @ggershinsky)
There was a problem hiding this comment.
As @smaheshwar-pltr points it out the class implements Serializable by interface but I also have doubts whether that's actually needed. KMS interactions for table encryption only concerns the master key which I'd expect to take place on driver / coordinator nodes, encrypting/decrypting the metadata json or manifest lists. Everything else down to the data files will not require a KMS.
|
|
||
| /** A minimum client interface to connect to a key management service (KMS). */ | ||
| interface KeyManagementClient extends Serializable, Closeable { | ||
| public interface KeyManagementClient extends Serializable, Closeable { |
There was a problem hiding this comment.
I think this will necessarily need to be public so that implementations can reside in other modules, so I do agree with this change.
It'd be good if @rdblue can confirm that or if some other implementation pattern was intended here.
There was a problem hiding this comment.
Definitely looks like something which needs to be public
aws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java
Outdated
Show resolved
Hide resolved
pvary
left a comment
pvary
left a comment
There was a problem hiding this comment.
@amogh-jahagirdar: Any more comments?
aws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/integration/java/org/apache/iceberg/aws/TestKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/main/java/org/apache/iceberg/aws/AwsKeyManagementClient.java
Outdated
Show resolved
Hide resolved
aws/src/main/java/org/apache/iceberg/aws/AwsKeyManagementClient.java
Outdated
Show resolved
Hide resolved
To be used in table encryption for: - wrapping/unwrapping encryption keys - generating data keys (available specs: AES_256, AES_128) Added integration test for verification.
amogh-jahagirdar
left a comment
amogh-jahagirdar
left a comment
There was a problem hiding this comment.
Thanks @szlta looks good to me. Thanks @smaheshwar-pltr @pvary @nastra for reviewing!
|
Thanks everyone for the reviews! |
6 participants
To be used in table encryption for:
Added integration test for verification.