-
Notifications
You must be signed in to change notification settings - Fork 271
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change looks good to me, and it doesn't change the semantics.
While reviewing, I noticed that the cache key can become quite large, depending on the paths and storage config. Not sure whether there's a better way.
The thing that's not fully clear to me is how long a cached storage-credentials-entry is usable. I see that the cache-entry expiration is calculated in org.apache.polaris.core.storage.cache.StorageCredentialCacheEntry#getExpirationTime
, but that returns the expiration time of the cached credentials. I wonder how long a client can actually use such credentials?
@@ -465,7 +465,7 @@ public Supplier<TransactionalPersistence> getOrCreateSessionSupplier( | |||
|
|||
@Override | |||
public StorageCredentialCache getOrCreateStorageCredentialCache(RealmContext realmContext) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If RealmContext
is no longer needed, let's remove or at least deprecate-for-removal this function and add one that takes no parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have a followup PR-2022 for dealing with this method, generally speaking since StorageCredentialCache
is application-scoped and multi-realm then this method is no longer needed at all afaict.
its only getting called from a single place RealmEntityManagerFactory
so idk if deprecation would achieve much but i can add it.
* 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
@@ -32,12 +32,19 @@ public class StorageCredentialCacheEntry { | |||
public final EnumMap<StorageAccessProperty, String> credsMap; | |||
|
|||
private final ScopedCredentialsResult scopedCredentialsResult; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 Thanks, @XN137 !
c4fb8f5
to
2ad970b
Compare
rebased after conflict with 19f44d8
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Injecting the request-scoped
RealmContext
into the application-scopedStorageCredentialCache
makes things unnecessarily complicated.Similarly
StorageCredentialCacheKey
having a@Nullable callContext
makes it more difficult to reason about.Instead we can determine all realm-specific values at the time of insertion (from the
PolarisCallContext
param ofgetOrGenerateSubScopeCreds
).