tokoko commented on code in PR #3699:
URL: https://github.com/apache/polaris/pull/3699#discussion_r2788817649
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java:
##########
@@ -95,94 +77,27 @@ private long maxCacheDurationMs(RealmConfig realmConfig) {
}
/**
- * Either get from the cache or generate a new entry for a scoped creds
+ * Get cached credentials or load new ones using the provided supplier.
*
- * @param storageCredentialsVendor the credential vendor used to generate a
new scoped creds if
- * needed
- * @param polarisEntity the polaris entity that is going to scoped creds
- * @param allowListOperation whether allow list action on the provided read
and write locations
- * @param allowedReadLocations a set of allowed to read locations
- * @param allowedWriteLocations a set of allowed to write locations.
- * @param polarisPrincipal the principal requesting credentials
- * @param refreshCredentialsEndpoint optional endpoint for credential refresh
- * @param credentialVendingContext context containing metadata for session
tags (catalog,
- * namespace, table, roles) for audit/correlation purposes
- * @return the a map of string containing the scoped creds information
+ * @param key the cache key
+ * @param realmConfig realm configuration for cache duration settings
+ * @param loader supplier that produces scoped credentials on cache miss;
may throw on error
+ * @return the storage access config with scoped credentials
*/
- public StorageAccessConfig getOrGenerateSubScopeCreds(
- @Nonnull StorageCredentialsVendor storageCredentialsVendor,
- @Nonnull PolarisEntity polarisEntity,
- boolean allowListOperation,
- @Nonnull Set<String> allowedReadLocations,
- @Nonnull Set<String> allowedWriteLocations,
- @Nonnull PolarisPrincipal polarisPrincipal,
- Optional<String> refreshCredentialsEndpoint,
- @Nonnull CredentialVendingContext credentialVendingContext) {
- RealmContext realmContext = storageCredentialsVendor.getRealmContext();
- RealmConfig realmConfig = storageCredentialsVendor.getRealmConfig();
- if (!isTypeSupported(polarisEntity.getType())) {
- diagnostics.fail(
- "entity_type_not_suppported_to_scope_creds", "type={}",
polarisEntity.getType());
- }
-
- boolean includePrincipalNameInSubscopedCredential =
-
realmConfig.getConfig(FeatureConfiguration.INCLUDE_PRINCIPAL_NAME_IN_SUBSCOPED_CREDENTIAL);
- boolean includeSessionTags =
-
realmConfig.getConfig(FeatureConfiguration.INCLUDE_SESSION_TAGS_IN_SUBSCOPED_CREDENTIAL);
-
- // When session tags are enabled, the cache key needs to include:
- // 1. The credential vending context to avoid returning cached credentials
with different
- // session tags (catalog/namespace/table/roles/traceId)
- // 2. The principal, because the polaris:principal session tag is included
in AWS credentials
- // and we must not serve credentials tagged for principal A to
principal B
- // When session tags are disabled, we only include principal if explicitly
configured.
- //
- // Note: The trace ID is controlled at the source
(StorageAccessConfigProvider). When
- // INCLUDE_TRACE_ID_IN_SESSION_TAGS is disabled, the context's traceId is
left empty,
- // which allows efficient caching across requests with different trace IDs.
- boolean includePrincipalInCacheKey =
- includePrincipalNameInSubscopedCredential || includeSessionTags;
- // When session tags are disabled, use empty context to ensure consistent
cache key behavior
- CredentialVendingContext contextForCacheKey =
- includeSessionTags ? credentialVendingContext :
CredentialVendingContext.empty();
- StorageCredentialCacheKey key =
- StorageCredentialCacheKey.of(
- realmContext.getRealmIdentifier(),
- polarisEntity,
- allowListOperation,
- allowedReadLocations,
- allowedWriteLocations,
- refreshCredentialsEndpoint,
- includePrincipalInCacheKey ? Optional.of(polarisPrincipal) :
Optional.empty(),
- contextForCacheKey);
- Function<StorageCredentialCacheKey, StorageCredentialCacheEntry> loader =
- k -> {
- LOGGER.atDebug().log("StorageCredentialCache::load");
- // Use credentialVendingContext from the cache key for correctness.
- // This ensures we use the same context that was used for cache key
comparison.
- ScopedCredentialsResult scopedCredentialsResult =
- storageCredentialsVendor.getSubscopedCredsForEntity(
- polarisEntity,
- allowListOperation,
- allowedReadLocations,
- allowedWriteLocations,
- polarisPrincipal,
- refreshCredentialsEndpoint,
- k.credentialVendingContext());
- if (scopedCredentialsResult.isSuccess()) {
- long maxCacheDurationMs = maxCacheDurationMs(realmConfig);
- return new StorageCredentialCacheEntry(
- scopedCredentialsResult.getStorageAccessConfig(),
maxCacheDurationMs);
- }
- LOGGER
- .atDebug()
- .addKeyValue("errorMessage",
scopedCredentialsResult.getExtraInformation())
- .log("Failed to get subscoped credentials");
- throw new UnprocessableEntityException(
- "Failed to get subscoped credentials: %s",
- scopedCredentialsResult.getExtraInformation());
- };
- return cache.get(key, loader).toAccessConfig();
+ public StorageAccessConfig getOrLoad(
+ StorageCredentialCacheKey key,
+ RealmConfig realmConfig,
+ Supplier<StorageAccessConfig> loader) {
+ long maxCacheDurationMs = maxCacheDurationMs(realmConfig);
+ return cache
+ .get(
Review Comment:
I can revert, but I have to say my limited (ai-assisted) research convinced
me that there's no point in using a `LoadingCache` with a dummy loader if
you're always calling a get with an explicit loader. The behavior should be the
same with either. might be wrong, though. happy to trust you on this.
--
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]
