[WIP]HIVE-29228: Support vended credentials for S3#6458
[WIP]HIVE-29228: Support vended credentials for S3#6458okumin wants to merge 1 commit intoapache:masterfrom
Conversation
okumin
force-pushed
the
HIVE-29228-vended-credentials
branch
from
0e22fb9 to
56887bb
Compare
| return response; | ||
| } | ||
|
|
||
| if (accessDelegationModes.contains(AccessDelegationMode.VENDED_CREDENTIALS)) { |
There was a problem hiding this comment.
could we do small refactor
private LoadTableResponse attachCredentials(
Set<AccessDelegationMode> modes,
TableIdentifier ident,
LoadTableResponse response) {
if (credentialProvider == null) {
return response;
}
if (modes.contains(AccessDelegationMode.VENDED_CREDENTIALS)) {
return withVendedCredentials(ident, response);
}
if (modes.contains(AccessDelegationMode.REMOTE_SIGNING)) {
LOG.warn("Remote signing is not supported. Ignoring...");
}
return response;
}
private LoadTableResponse withVendedCredentials(
TableIdentifier ident,
LoadTableResponse response) {
var credentials = credentialProvider.vend(
ident,
response.tableMetadata().location());
return LoadTableResponse.builder()
.withTableMetadata(response.tableMetadata())
.addAllConfig(response.config())
.addAllCredentials(credentials)
.build();
}
| Map<String, String> queryParams, | ||
| Object body, | ||
| Consumer<ErrorResponse> errorHandler) { | ||
| Object body, Consumer<ErrorResponse> errorHandler) { |
There was a problem hiding this comment.
please move errorHandler to a new line
| if (MetastoreConf.getBoolVar(configuration, ConfVars.ICEBERG_CATALOG_VENDED_CREDENTIALS_ENABLED)) { | ||
| vendedCredentialProvider = new IcebergVendedCredentialProvider(configuration); | ||
| } else { | ||
| vendedCredentialProvider = null; |
| case "vended-credentials" -> AccessDelegationMode.VENDED_CREDENTIALS; | ||
| case "remote-signing" -> AccessDelegationMode.REMOTE_SIGNING; | ||
| default -> { | ||
| LOG.warn("Unknown access delegation mode: {}", header); |
There was a problem hiding this comment.
maybe skip logging empty
if (!header.isEmpty()) {
LOG.warn("Unknown access delegation mode: {}", header);
}
| <dependency> | ||
| <groupId>org.apache.hive</groupId> | ||
| <artifactId>hive-exec</artifactId> | ||
| <version>${hive.version}</version> | ||
| <classifier>core</classifier> | ||
| <scope>provided</scope> |
There was a problem hiding this comment.
rest-catalog now depends on a hive-exec-core? scope was test-only before
| /** | ||
| * An S3 location. | ||
| */ | ||
| class S3Location { |
There was a problem hiding this comment.
should this be in common or server?
| /** | ||
| * A VendedCredentialProvider specified for Amazon S3. | ||
| */ | ||
| public class S3VendedCredentialProvider implements VendedCredentialProvider { |
There was a problem hiding this comment.
would we need something similar for abfs? ozone probably could reuse S3 vending model.
CredentialProvider interface allows to plug other providers like IDBrokerCredentialProvider or ABFSCredentialProvider, is that right?
| /** | ||
| * The list of I/O operations to access a storage system. | ||
| */ | ||
| public enum StorageOperation { | ||
| LIST, READ, CREATE, DELETE, |
There was a problem hiding this comment.
Is WRITE/PUT already covered by CREATE?
|
|
||
| // AWS Integration tests don't run by default. You need to set up your environment. | ||
| // | ||
| // ACCOUNT_ID={your AWS account ID} |
There was a problem hiding this comment.
could we configure it with docker cluster?
please see 350c4d4#diff-c712864f40434ee3783c8a59a6a21c92f5c8406b687452fe78971291d54ec8f7R36-R46
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
henrib
left a comment
henrib
left a comment
There was a problem hiding this comment.
1-May be vending should be aware of the storage operation.
2-An interface would open to other integrations, delegation to a different/centralized vending mechanism (Knox + Ranger comes to mind).
| * @param location the table location | ||
| * @return the vended credentials | ||
| */ | ||
| public List<Credential> vend(TableIdentifier identifier, String location) { |
There was a problem hiding this comment.
Why don't we pass the storage operation here ? Instead of seeking all allowed operations, this would allow checking if only that operation is allowed and act upon it.
| * The list of I/O operations to access a storage system. | ||
| */ | ||
| public enum StorageOperation { | ||
| LIST, READ, CREATE, DELETE, |
There was a problem hiding this comment.
I agree; we should have the 'meta' (LIST, CREATE, DELETE) and the 'data' (READ-ONLY, READ-WRITE).
|
|
||
| public IcebergVendedCredentialProvider(Configuration conf) { | ||
| this.catalog = MetaStoreUtils.getDefaultCatalog(conf); | ||
| this.vendedCredentialProvider = new CompositeVendedCredentialProvider(conf); |
There was a problem hiding this comment.
This should be an interface; implementation should be fully configurable (class name in config).
4 participants
What changes were proposed in this pull request?
Why are the changes needed?
Does this PR introduce any user-facing change?
How was this patch tested?