-
Notifications
You must be signed in to change notification settings - Fork 674
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
SOLR-16951: Cache client pkiAuth headers for a second #1921
Conversation
private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV2Tokens = | ||
new ConcurrentHashMap<>(); | ||
|
||
private static final Duration cacheExpiryTime = Duration.ofSeconds(1); |
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.
How about we make this relative to MAX_VALIDITY
? like MAX_VALIDITY * 0.5
, or MAX_VALIDITY * 0.1
by default?
String token; | ||
|
||
private CachedToken(Instant generatedAt, String token) { | ||
this.generatedAt = generatedAt; |
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.
not a big deal, but if we just store the expiration time instead of the generation time we don't have to subtract every time
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.
+1 to the above and maybe also add a method isExpired()
to simplify the code
AtomicReference<CachedToken> tokenRef = | ||
cachedV1Tokens.computeIfAbsent(usr, u -> new AtomicReference<>(generateToken(u))); |
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'm wondering if we need to consider limiting the size of this map in some way? can the different users be ever growing (probably not with the OOTB authentication, but maybe with plugins?)
assert usr != null; | ||
String s = usr + " " + System.currentTimeMillis(); | ||
byte[] payload = s.getBytes(UTF_8); | ||
byte[] payloadCipher = publicKeyHandler.getKeyPair().encrypt(ByteBuffer.wrap(payload)); | ||
String base64Cipher = Base64.getEncoder().encodeToString(payloadCipher); | ||
log.trace("generateToken: usr={} token={}", usr, base64Cipher); | ||
return myNodeName + " " + base64Cipher; | ||
return new CachedToken(Instant.now(), myNodeName + " " + base64Cipher); |
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.
Instant.now(),
could move into the CachedToken constructor, doesn't need to be specified here.
return tokenRef.get().token; | ||
} | ||
|
||
private synchronized String getTokenV2(String usr) { |
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.
the 2 methods getToken
and getTokenV2
are very similar. would you consider unifying into a single one? (one that gets a map and a 'token generator' function)
.map(PKIAuthenticationPlugin.this::generateToken) | ||
.ifPresent(token -> request.header(HEADER, token)); | ||
.map(PKIAuthenticationPlugin.this::getToken) | ||
.ifPresent(token -> request.headers(mutable -> mutable.add(HEADER, token))); |
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.
minor: maybe 'put' is a better method for this header
private synchronized String getTokenV2(String usr) { | ||
AtomicReference<CachedToken> tokenRef = | ||
cachedV2Tokens.computeIfAbsent(usr, u -> new AtomicReference<>(generateTokenV2(u))); | ||
if (tokenRef.get().generatedAt.isBefore(Instant.now().minus(cacheExpiryTime))) { |
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.
just wondering how this performs if it would be a single ConcurrentHashMap#compute
method instead of all the locks and checks. is there a benchmark for this change?
This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the [email protected] mailing list. Thank you for your contribution! |
This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. |
https://issues.apache.org/jira/browse/SOLR-16951
Also increase the TTL from 5 seconds to 10.
This is a WIP for now. probably need to think some more about it.