tokoko commented on code in PR #3699:
URL: https://github.com/apache/polaris/pull/3699#discussion_r3327910393
##########
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:
you're right, that was a poor implementation, I shouldn't have included
integration itself as an aux. still I don't like the idea of re-deriving entire
PolarisStorageIntegration object. the reason is that the first time it's
instantiated, the point it to build a key, the second time around you build a
credential. inputs for each are not necessarily identical, I changed session
name derivation in AWS to highlight it. I switched to deriving session name
during cache key generation and then simply using resolved string in the cache
key directly. in other words, cache key generation needs entire
CredentialVendingContext (which includes principal name), while credential
compute needs only the session name string that's already resolved.
Reinstantiating PolarisStorageIntegration with entire CredentialVendingContext
is therefore wasteful.
I switched to providing fields like `stsClientProvider` and
`credentialResolver` to cache key as aux fields instead of integration itself.
the key then calls a **static** `compute` method on the integration which takes
in only what it absolutely needs and params are coming from the fields in the
cache key. let me know what you think.
--
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]
