GCP: KeyManagementClient implementation that works with Google Cloud KMS#13334
Conversation
| implementation "com.google.cloud:google-cloud-storage" | ||
| implementation "com.google.cloud:google-cloud-bigquery" | ||
| implementation "com.google.cloud:google-cloud-core" | ||
| implementation "com.google.cloud:google-cloud-kms" |
There was a problem hiding this comment.
This is meant for engines that use our Iceberg GCP bundle to be able to work with the proposed GcpKeyManagementClient.
However, when I tested this with Spark, I noticed an issue:
com.google.cloud.kms.v1.DecryptRequest$Builder.setCiphertext(Lcom/google/protobuf/ByteString;)Lcom/google/cloud/kms/v1/DecryptRequest$Builder;
java.lang.NoSuchMethodError: com.google.cloud.kms.v1.DecryptRequest$Builder.setCiphertext(Lcom/google/protobuf/ByteString;)Lcom/google/cloud/kms/v1/DecryptRequest$Builder;
at org.apache.iceberg.gcp.GcpKeyManagementClient.unwrapKey(GcpKeyManagementClient.java:80)
This happens because DecryptRequest$Builder.setCiphertext takes a ByteString object, whose class is shaded and relocated in the gcp-bundle (check line 53 in this file). This results in changing the signature of
com.google.cloud.kms.v1.DecryptRequest$Builder.setCiphertext(Lcom/google/protobuf/ByteString;)Lcom/google/cloud/kms/v1/DecryptRequest$Builder;
to
com.google.cloud.kms.v1.DecryptRequest$Builder.setCiphertext(Lorg/apache/iceberg/gpc/shaded/com/google/protobuf/ByteString;)Lcom/google/cloud/kms/v1/DecryptRequest$Builder; in the bundle jar, hence the above error.
Engine apps not making use of the gcp-bundle jar (i.e. bringing their own google-cloud-kms and protobuf dependencies) work without issues.
I was considering the application of the same relocation logic in gcp module, and although that fixes the issue for gcp-bundle consumers, it causes a NoSuchMethodError for non-bundle consumers, so I can't seem to find way that ensures that the implementation works in both cases (like how the recently merged AwsKeyManagementClient does work). That is, without removing the relocation in line 53.
Please let me know what your opinion is @nastra, @amogh-jahagirdar, @pvary
There was a problem hiding this comment.
Update: I enhanced the current implementation so that it can work with both shaded and non-shaded protobuf dependencies. It now uses DynMethods and DynClasses utils to load the methods in a dynamic way. If the relocated class is observed it will be preferred over the original class name.
| } | ||
| } | ||
|
|
||
| static final class ByteStringReflectionUtil { |
There was a problem hiding this comment.
Why is this not private class?
Why is this final?
There was a problem hiding this comment.
Renamed to ByteStringShim and made it private. The reason it's marked as final is because it's a utility class.
- To be used in table encryption for wrapping/unwrapping encryption keys. - Refactored common OAuth2 token and refresh handler setup logic into GCPAuthUtils. Added integration tests for verification.
|
Merged to main. |
Added integration tests for verification.