Core: Add support for HashiCorp Vault KMS client#16075
Conversation
ebyhr
force-pushed
the
ebi/hashicorp-vault
branch
from
a988363 to
7e97bf6
Compare
ebyhr
force-pushed
the
ebi/hashicorp-vault
branch
2 times, most recently
from
fe0aed2 to
a896b32
Compare
ebyhr
force-pushed
the
ebi/hashicorp-vault
branch
2 times, most recently
from
d0d8db7 to
c458222
Compare
ebyhr
force-pushed
the
ebi/hashicorp-vault
branch
from
c458222 to
6bc3124
Compare
|
I think we should talk on the dev list about adding a new project and supporting a new KMS client. |
ebyhr
force-pushed
the
ebi/hashicorp-vault
branch
3 times, most recently
from
a96a6d5 to
a4ff3a3
Compare
|
tysm @ebyhr for picking this up. I will test this KMS implementation against our staging env on Monday and get back to you if I spot anything. |
caushie-akamai
left a comment
caushie-akamai
left a comment
There was a problem hiding this comment.
I tried reproducing this today with the following steps:
- Build jar
./gradlew build :iceberg-hashicorp:build -DsparkVersions=3.5 -x test -x integrationTest -x revapi --parallel - Copy iceberg jar + hashicorp jar into spark
- Spawn spark-sql shell with vault configuration
But i am getting a java.lang.NoClassDefFoundError: org/apache/hc/core5/http/HttpEntity
Should we also shade http jars in here or would it be better to change the imports to the shaded version? WDYT
|
|
||
| String configuredVaultToken = newProperties.get(VaultProperties.VAULT_TOKEN_PROP); | ||
| appRoleId = newProperties.get(VaultProperties.VAULT_ROLE_ID_PROP); | ||
| appSecretId = newProperties.get(VaultProperties.VAULT_SECRET_ID_PROP); |
There was a problem hiding this comment.
What are your thoughts about using an env var instead of explicitly specifying secretID in the config?
I believe this would be beneficial for folks who use spark-operator in k8s and would not like sensitive credentials to live in the spark config.
Internally we also grab the secretID from an environment variable to avoid this.
There was a problem hiding this comment.
Environment variables don't align well with Trino's catalog-based architecture, because in theory the value can differ per catalog (even if that's not practical). I've added support for both properties and environment variables.
There was a problem hiding this comment.
sounds good to me, this way we are supporting both query engines.
| if (httpClient == null) { | ||
| synchronized (this) { | ||
| if (httpClient == null) { | ||
| httpClient = HttpClients.createDefault(); |
There was a problem hiding this comment.
WDYT about also allowing clients to configure SSL config in the http client? There may be folks who do a mTLS vault deployment or who need to trust their vault deployment's CA.
I believe the bettercloud vault had a similar solution: https://github.com/BetterCloud/vault-java-driver/blob/master/src/main/java/com/bettercloud/vault/SslConfig.java#L44
I am trying to find if such a thing is also possible with the apache http client
There was a problem hiding this comment.
Adding TLS support with the Apache HTTP client is straightforward. However, I'd like to keep the initial PR focused. Can we add TLS support in a follow-up?
Co-Authored-By: Endi Caushi <42871239+mrendi29@users.noreply.github.com>
|
@caushie-akamai I've added the following lines to iceberg-hashicorp project in build.gradle: Could you please confirm whether the NoClassDefFoundError issue has been resolved? |
|
TYSM @ebyhr , i'll try to test this against staging tomorrow morning and get back to you |
4 participants
Restores #14451 with additional changes:
hashicorptoencryption.kms-typehashicorp/vaultdocker imageMailing list: https://lists.apache.org/thread/1269w5pzoy723sr1c6xxq8jg02zcf3on
Fixes #14437