dimas-b commented on code in PR #3699:
URL: https://github.com/apache/polaris/pull/3699#discussion_r2996471300
##########
runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java:
##########
@@ -91,74 +145,53 @@ public PolarisStorageIntegrationProviderImpl(
if (polarisStorageConfigurationInfo == null) {
return null;
}
- PolarisStorageIntegration<T> storageIntegration;
- switch (polarisStorageConfigurationInfo.getStorageType()) {
- case S3:
- Optional<AwsCredentialsProvider> awsCreds = stsCredentials;
- if (awsCreds.isEmpty() && storageConfiguration != null) {
- if (realmConfig != null
- &&
realmConfig.getConfig(FeatureConfiguration.RESOLVE_CREDENTIALS_BY_STORAGE_NAME))
{
- awsCreds =
- Optional.of(
- storageConfiguration.stsCredentials(
- polarisStorageConfigurationInfo.getStorageName()));
- } else {
- awsCreds = Optional.of(storageConfiguration.stsCredentials());
- }
- }
- storageIntegration =
- (PolarisStorageIntegration<T>)
- new AwsCredentialsStorageIntegration(
- (AwsStorageConfigurationInfo)
polarisStorageConfigurationInfo,
- stsClientProvider,
- awsCreds);
- break;
- case GCS:
- storageIntegration =
- (PolarisStorageIntegration<T>)
- new GcpCredentialsStorageIntegration(
- (GcpStorageConfigurationInfo)
polarisStorageConfigurationInfo,
- gcpCredsProvider.get(),
- ServiceOptions.getFromServiceLoader(
- HttpTransportFactory.class, NetHttpTransport::new));
- break;
- case AZURE:
- storageIntegration =
- (PolarisStorageIntegration<T>)
- new AzureCredentialsStorageIntegration(
- (AzureStorageConfigurationInfo)
polarisStorageConfigurationInfo);
- break;
- case FILE:
- storageIntegration =
- new PolarisStorageIntegration<>((T)
polarisStorageConfigurationInfo, "file") {
- @Override
- public StorageAccessConfig getSubscopedCreds(
- @Nonnull RealmConfig realmConfig,
- boolean allowListOperation,
- @Nonnull Set<String> allowedReadLocations,
- @Nonnull Set<String> allowedWriteLocations,
- @Nonnull PolarisPrincipal polarisPrincipal,
- Optional<String> refreshCredentialsEndpoint,
- @Nonnull CredentialVendingContext credentialVendingContext) {
- // FILE storage does not support credential vending
- return
StorageAccessConfig.builder().supportsCredentialVending(false).build();
- }
+ return (PolarisStorageIntegration<T>)
+ switch (polarisStorageConfigurationInfo.getStorageType()) {
Review Comment:
How about we rename `getStorageIntegrationForConfig()` to
`getStorageIntegrationForType()`?
I do not think we can expect the bulk of config data to be used in producing
a `PolarisStorageIntegration` instance.
I believe old callers of this method (e.g.
`AbstractTransactionalPersistence.createStorageIntegration()`) are not very
meaningful now that storage config is generated by application-scoped beans...
so we might as well make it explicit.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/io/StorageAccessConfigProvider.java:
##########
@@ -131,14 +144,30 @@ public StorageAccessConfig getStorageAccessConfig(
CredentialVendingContext credentialVendingContext =
buildCredentialVendingContext(tableIdentifier, resolvedPath);
+ RealmConfig realmConfig = callContext.getRealmConfig();
+
+ // Get the right integration for this storage type
+ String storageConfigStr =
+ storageInfoEntity
+ .getInternalPropertiesAsMap()
+ .get(PolarisEntityConstants.getStorageConfigInfoPropertyName());
+ PolarisStorageConfigurationInfo storageConfig =
+ PolarisStorageConfigurationInfo.deserialize(storageConfigStr);
+ var integration =
storageIntegrationProvider.getStorageIntegrationForConfig(storageConfig);
Review Comment:
This `integration` feeds directly into the `getOrLoadSubscopedCreds()` call
below. I think we can further simplify the flow and obtain
`StorageAccessConfig` directly from `storageIntegrationProvider`.
See my other comment about older callers to
`getStorageIntegrationForConfig()` becoming useless (as far as I can tell).
##########
runtime/service/src/main/java/org/apache/polaris/service/storage/PolarisStorageIntegrationProviderImpl.java:
##########
@@ -31,56 +31,110 @@
import java.util.Optional;
import java.util.Set;
import java.util.function.Supplier;
-import org.apache.polaris.core.auth.PolarisPrincipal;
import org.apache.polaris.core.config.FeatureConfiguration;
import org.apache.polaris.core.config.RealmConfig;
+import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.storage.CredentialVendingContext;
import org.apache.polaris.core.storage.PolarisStorageActions;
import org.apache.polaris.core.storage.PolarisStorageConfigurationInfo;
import org.apache.polaris.core.storage.PolarisStorageIntegration;
import org.apache.polaris.core.storage.PolarisStorageIntegrationProvider;
import org.apache.polaris.core.storage.StorageAccessConfig;
import org.apache.polaris.core.storage.aws.AwsCredentialsStorageIntegration;
-import org.apache.polaris.core.storage.aws.AwsStorageConfigurationInfo;
import org.apache.polaris.core.storage.aws.StsClientProvider;
import
org.apache.polaris.core.storage.azure.AzureCredentialsStorageIntegration;
-import org.apache.polaris.core.storage.azure.AzureStorageConfigurationInfo;
+import org.apache.polaris.core.storage.cache.StorageAccessConfigParameters;
+import org.apache.polaris.core.storage.cache.StorageCredentialCache;
import org.apache.polaris.core.storage.gcp.GcpCredentialsStorageIntegration;
-import org.apache.polaris.core.storage.gcp.GcpStorageConfigurationInfo;
import software.amazon.awssdk.auth.credentials.AwsCredentialsProvider;
@ApplicationScoped
public class PolarisStorageIntegrationProviderImpl implements
PolarisStorageIntegrationProvider {
- private final StsClientProvider stsClientProvider;
- private final Optional<AwsCredentialsProvider> stsCredentials;
- private final Supplier<GoogleCredentials> gcpCredsProvider;
- private final StorageConfiguration storageConfiguration;
- private final RealmConfig realmConfig;
+ private final AwsCredentialsStorageIntegration awsIntegration;
+ private volatile GcpCredentialsStorageIntegration gcpIntegration;
+ private volatile AzureCredentialsStorageIntegration azureIntegration;
+ private final PolarisStorageIntegration<?> fileIntegration;
+ private final Supplier<GcpCredentialsStorageIntegration>
gcpIntegrationSupplier;
+ private final Supplier<AzureCredentialsStorageIntegration>
azureIntegrationSupplier;
@SuppressWarnings("CdiInjectionPointsInspection")
@Inject
public PolarisStorageIntegrationProviderImpl(
StorageConfiguration storageConfiguration,
StsClientProvider stsClientProvider,
RealmConfig realmConfig,
- Clock clock) {
- this.storageConfiguration = storageConfiguration;
- this.stsClientProvider = stsClientProvider;
- this.stsCredentials = Optional.empty();
- this.gcpCredsProvider = storageConfiguration.gcpCredentialsSupplier(clock);
- this.realmConfig = realmConfig;
+ Clock clock,
+ StorageCredentialCache cache) {
+ Supplier<RealmConfig> realmConfigSupplier = () -> realmConfig;
Review Comment:
I'm not sure we need a `Supplier` here. `realmConfig` is going to be a CDI
proxy bridging from the application scope of this bean to the current request
scope of `RealmConfig`. We should be able to pass that proxy to
`AwsCredentialsStorageIntegration` (and others) "as is".
The `Supplier` does not change the fact that the config is a CDI proxy.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
