Skip to content

Make StorageCredentialCache safe for multi-realm usage #2021

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make StorageCredentialCache safe for multi-realm usage #2021

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions ...main/java/org/apache/polaris/persistence/relational/jdbc/JdbcMetaStoreManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,7 @@ public synchronized StorageCredentialCache getOrCreateStorageCredentialCache(
RealmContext realmContext) {
if (!storageCredentialCacheMap.containsKey(realmContext.getRealmIdentifier())) {
storageCredentialCacheMap.put(
realmContext.getRealmIdentifier(),
new StorageCredentialCache(realmContext, configurationStore));
realmContext.getRealmIdentifier(), new StorageCredentialCache(configurationStore));
}

return storageCredentialCacheMap.get(realmContext.getRealmIdentifier());
Expand Down
3 changes: 1 addition & 2 deletions ...rc/main/java/org/apache/polaris/core/persistence/LocalPolarisMetaStoreManagerFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,8 +182,7 @@ public synchronized StorageCredentialCache getOrCreateStorageCredentialCache(
RealmContext realmContext) {
if (!storageCredentialCacheMap.containsKey(realmContext.getRealmIdentifier())) {
storageCredentialCacheMap.put(
realmContext.getRealmIdentifier(),
new StorageCredentialCache(realmContext, configurationStore));
realmContext.getRealmIdentifier(), new StorageCredentialCache(configurationStore));
}

return storageCredentialCacheMap.get(realmContext.getRealmIdentifier());
Expand Down
18 changes: 8 additions & 10 deletions polaris-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCache.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,10 @@ public class StorageCredentialCache {
private static final long CACHE_MAX_NUMBER_OF_ENTRIES = 10_000L;

private final LoadingCache<StorageCredentialCacheKey, StorageCredentialCacheEntry> cache;
private final RealmContext realmContext;
private final PolarisConfigurationStore configurationStore;

/** Initialize the creds cache */
public StorageCredentialCache(
RealmContext realmContext, PolarisConfigurationStore configurationStore) {
this.realmContext = realmContext;
public StorageCredentialCache(PolarisConfigurationStore configurationStore) {
this.configurationStore = configurationStore;
cache =
Caffeine.newBuilder()
Expand All @@ -69,7 +66,7 @@ public StorageCredentialCache(
0,
Math.min(
(entry.getExpirationTime() - System.currentTimeMillis()) / 2,
this.maxCacheDurationMs()));
entry.getMaxCacheDurationMs()));
return Duration.ofMillis(expireAfterMillis);
}))
.build(
Expand All @@ -80,7 +77,7 @@ public StorageCredentialCache(
}

/** How long credentials should remain in the cache. */
private long maxCacheDurationMs() {
private long maxCacheDurationMs(RealmContext realmContext) {
var cacheDurationSeconds =
configurationStore.getConfiguration(
realmContext, FeatureConfiguration.STORAGE_CREDENTIAL_CACHE_DURATION_SECONDS);
Expand Down Expand Up @@ -123,26 +120,27 @@ public AccessConfig getOrGenerateSubScopeCreds(
}
StorageCredentialCacheKey key =
new StorageCredentialCacheKey(
callCtx.getRealmContext().getRealmIdentifier(),
polarisEntity,
allowListOperation,
allowedReadLocations,
allowedWriteLocations,
callCtx);
allowedWriteLocations);
LOGGER.atDebug().addKeyValue("key", key).log("subscopedCredsCache");
Function<StorageCredentialCacheKey, StorageCredentialCacheEntry> loader =
k -> {
LOGGER.atDebug().log("StorageCredentialCache::load");
ScopedCredentialsResult scopedCredentialsResult =
credentialVendor.getSubscopedCredsForEntity(
k.getCallContext(),
callCtx,
k.getCatalogId(),
k.getEntityId(),
polarisEntity.getType(),
k.isAllowedListAction(),
k.getAllowedReadLocations(),
k.getAllowedWriteLocations());
if (scopedCredentialsResult.isSuccess()) {
return new StorageCredentialCacheEntry(scopedCredentialsResult);
long maxCacheDurationMs = maxCacheDurationMs(callCtx.getRealmContext());
return new StorageCredentialCacheEntry(scopedCredentialsResult, maxCacheDurationMs);
}
LOGGER
.atDebug()
Expand Down
9 changes: 8 additions & 1 deletion ...core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheEntry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,19 @@ public class StorageCredentialCacheEntry {
public final EnumMap<StorageAccessProperty, String> credsMap;

private final ScopedCredentialsResult scopedCredentialsResult;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated: this field is unused

private final long maxCacheDurationMs;

public StorageCredentialCacheEntry(ScopedCredentialsResult scopedCredentialsResult) {
public StorageCredentialCacheEntry(
ScopedCredentialsResult scopedCredentialsResult, long maxCacheDurationMs) {
this.scopedCredentialsResult = scopedCredentialsResult;
this.maxCacheDurationMs = maxCacheDurationMs;
this.credsMap = scopedCredentialsResult.getCredentials();
}

public long getMaxCacheDurationMs() {
return maxCacheDurationMs;
}

/** Get the expiration time in millisecond for the cached entry */
public long getExpirationTime() {
if (credsMap.containsKey(StorageAccessProperty.GCS_ACCESS_TOKEN_EXPIRES_AT)) {
Expand Down
35 changes: 14 additions & 21 deletions ...s-core/src/main/java/org/apache/polaris/core/storage/cache/StorageCredentialCacheKey.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,14 @@
*/
package org.apache.polaris.core.storage.cache;

import jakarta.annotation.Nullable;
import java.util.Objects;
import java.util.Set;
import org.apache.polaris.core.PolarisCallContext;
import org.apache.polaris.core.context.CallContext;
import org.apache.polaris.core.entity.PolarisEntity;
import org.apache.polaris.core.entity.PolarisEntityConstants;

public class StorageCredentialCacheKey {

private final String realmId;
private final long catalogId;

/** The serialized string of the storage config. */
Expand All @@ -44,18 +42,13 @@ public class StorageCredentialCacheKey {

private final Set<String> allowedWriteLocations;

/**
* The callContext is passed to be used to fetch subscoped creds, but is not used to hash/equals
* as part of the cache key.
*/
private @Nullable PolarisCallContext callContext;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect the removal of this field is the key of this PR?

It totally makes sense to not keep per-request state around.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes but also removal of the realmContext field is important as well, as it is request scope but was being used inside the expiry logic of the cache.


public StorageCredentialCacheKey(
String realmId,
PolarisEntity entity,
boolean allowedListAction,
Set<String> allowedReadLocations,
Set<String> allowedWriteLocations,
@Nullable PolarisCallContext callContext) {
Set<String> allowedWriteLocations) {
this.realmId = realmId;
this.catalogId = entity.getCatalogId();
this.storageConfigSerializedStr =
entity
Expand All @@ -65,10 +58,10 @@ public StorageCredentialCacheKey(
this.allowedListAction = allowedListAction;
this.allowedReadLocations = allowedReadLocations;
this.allowedWriteLocations = allowedWriteLocations;
this.callContext = callContext;
if (this.callContext == null) {
this.callContext = CallContext.getCurrentContext().getPolarisCallContext();
}
}

public String getRealmId() {
return realmId;
}

public long getCatalogId() {
Expand All @@ -95,16 +88,13 @@ public Set<String> getAllowedWriteLocations() {
return allowedWriteLocations;
}

public @Nullable PolarisCallContext getCallContext() {
return callContext;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
StorageCredentialCacheKey cacheKey = (StorageCredentialCacheKey) o;
return catalogId == cacheKey.getCatalogId()
return Objects.equals(realmId, cacheKey.getRealmId())
&& catalogId == cacheKey.getCatalogId()
&& Objects.equals(storageConfigSerializedStr, cacheKey.getStorageConfigSerializedStr())
&& allowedListAction == cacheKey.allowedListAction
&& Objects.equals(allowedReadLocations, cacheKey.allowedReadLocations)
Expand All @@ -114,6 +104,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
return Objects.hash(
realmId,
catalogId,
storageConfigSerializedStr,
allowedListAction,
Expand All @@ -124,7 +115,9 @@ public int hashCode() {
@Override
public String toString() {
return "StorageCredentialCacheKey{"
+ "catalogId="
+ "realmId="
+ realmId
+ ", catalogId="
+ catalogId
+ ", storageConfigSerializedStr='"
+ storageConfigSerializedStr
Expand Down
Loading
Loading