dimas-b commented on code in PR #3699:
URL: https://github.com/apache/polaris/pull/3699#discussion_r2789112874
##########
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(
+ key,
+ k -> {
+ LOGGER.atDebug().log("StorageCredentialCache::load");
+ StorageAccessConfig accessConfig = loader.get();
Review Comment:
Re: confusing code - TBH, IMHO the _current_ (on `main`) code is very
confusing because the reader has to think very carefully about which parameters
go through the cache and which go outside and whether they are consistent
:sweat_smile:
I think we're pretty close to "ideal" here. The "entity" this code deals
with, has already been "found" and loaded, we only need to extract the storage
configuration JSON from it in front of the cache.
`PolarisStorageIntegration` is loaded based on the storage config, which can
be done past the cache.
In each call, the key will be used in full as required for a particular
storage backend + feature flags. Different calls will have different data in
their keys, of course, but that is the idea :) It will allow one cache per JVM
to handle all use cases. We could even have different expiry strategies per
storage type/config, but that's looking to far into the future :sweat_smile:
Would renaming `StorageCredentialCacheKey` to
`StorageAccessConfigParameters` make the code clearer?
--
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]
