You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "HoustonPutman (via GitHub)" <gi...@apache.org> on 2023/09/13 18:04:18 UTC
[GitHub] [solr] HoustonPutman opened a new pull request, #1921: SOLR-16951: Cache client pkiAuth headers for a second
HoustonPutman opened a new pull request, #1921:
URL: https://github.com/apache/solr/pull/1921
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.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16951: Cache client pkiAuth headers for a second [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #1921:
URL: https://github.com/apache/solr/pull/1921#issuecomment-2959900913
Closing this in favor of https://github.com/apache/solr/pull/3334
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16951: Cache client pkiAuth headers for a second [solr]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman closed pull request #1921: SOLR-16951: Cache client pkiAuth headers for a second
URL: https://github.com/apache/solr/pull/1921
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1921: SOLR-16951: Cache client pkiAuth headers for a second
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1921:
URL: https://github.com/apache/solr/pull/1921#discussion_r1326396396
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -403,12 +405,12 @@ public void onBegin(Request request) {
final Optional<String> preFetchedUser = getUserFromJettyRequest(request);
if ("v1".equals(System.getProperty(SEND_VERSION))) {
preFetchedUser
- .map(PKIAuthenticationPlugin.this::generateToken)
- .ifPresent(token -> request.header(HEADER, token));
+ .map(PKIAuthenticationPlugin.this::getToken)
+ .ifPresent(token -> request.headers(mutable -> mutable.add(HEADER, token)));
Review Comment:
minor: maybe 'put' is a better method for this header
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1921: SOLR-16951: Cache client pkiAuth headers for a second
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1921:
URL: https://github.com/apache/solr/pull/1921#discussion_r1326388797
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -485,34 +487,75 @@ private Optional<String> getUser() {
}
}
+ private static class CachedToken {
+ Instant generatedAt;
+ String token;
+
+ private CachedToken(Instant generatedAt, String token) {
+ this.generatedAt = generatedAt;
+ this.token = token;
+ }
+ }
+
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV1Tokens =
+ new ConcurrentHashMap<>();
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV2Tokens =
+ new ConcurrentHashMap<>();
+
+ private static final Duration cacheExpiryTime = Duration.ofSeconds(1);
+
+ private String getToken(String usr) {
+ AtomicReference<CachedToken> tokenRef =
+ cachedV1Tokens.computeIfAbsent(usr, u -> new AtomicReference<>(generateToken(u)));
+ if (tokenRef.get().generatedAt.isBefore(Instant.now().minus(cacheExpiryTime))) {
+ synchronized (tokenRef) {
+ if (tokenRef.get().generatedAt.isBefore(Instant.now().minus(cacheExpiryTime))) {
+ tokenRef.set(generateToken(usr));
+ }
+ }
+ }
+ return tokenRef.get().token;
+ }
+
+ 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))) {
+ synchronized (tokenRef) {
+ if (tokenRef.get().generatedAt.isBefore(Instant.now().minus(cacheExpiryTime))) {
+ tokenRef.set(generateTokenV2(usr));
+ }
+ }
+ }
+ return tokenRef.get().token;
+ }
+
@SuppressForbidden(reason = "Needs currentTimeMillis to set current time in header")
- private String generateToken(String usr) {
+ private CachedToken generateToken(String usr) {
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);
Review Comment:
`Instant.now(),` could move into the CachedToken constructor, doesn't need to be specified here.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1921: SOLR-16951: Cache client pkiAuth headers for a second
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1921:
URL: https://github.com/apache/solr/pull/1921#discussion_r1326390472
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -485,34 +487,75 @@ private Optional<String> getUser() {
}
}
+ private static class CachedToken {
+ Instant generatedAt;
+ String token;
+
+ private CachedToken(Instant generatedAt, String token) {
+ this.generatedAt = generatedAt;
Review Comment:
+1 to the above and maybe also add a method `isExpired()` to simplify the code
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] SOLR-16951: Cache client pkiAuth headers for a second [solr]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1921:
URL: https://github.com/apache/solr/pull/1921#issuecomment-1912846953
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 dev@solr.apache.org mailing list. Thank you for your contribution!
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] tflobbe commented on a diff in pull request #1921: SOLR-16951: Cache client pkiAuth headers for a second
Posted by "tflobbe (via GitHub)" <gi...@apache.org>.
tflobbe commented on code in PR #1921:
URL: https://github.com/apache/solr/pull/1921#discussion_r1325142428
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -485,34 +487,75 @@ private Optional<String> getUser() {
}
}
+ private static class CachedToken {
+ Instant generatedAt;
+ String token;
+
+ private CachedToken(Instant generatedAt, String token) {
+ this.generatedAt = generatedAt;
+ this.token = token;
+ }
+ }
+
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV1Tokens =
+ new ConcurrentHashMap<>();
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV2Tokens =
+ new ConcurrentHashMap<>();
+
+ private static final Duration cacheExpiryTime = Duration.ofSeconds(1);
+
+ private String getToken(String usr) {
+ AtomicReference<CachedToken> tokenRef =
+ cachedV1Tokens.computeIfAbsent(usr, u -> new AtomicReference<>(generateToken(u)));
Review Comment:
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?)
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -485,34 +487,75 @@ private Optional<String> getUser() {
}
}
+ private static class CachedToken {
+ Instant generatedAt;
+ String token;
+
+ private CachedToken(Instant generatedAt, String token) {
+ this.generatedAt = generatedAt;
+ this.token = token;
+ }
+ }
+
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV1Tokens =
+ new ConcurrentHashMap<>();
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV2Tokens =
+ new ConcurrentHashMap<>();
+
+ private static final Duration cacheExpiryTime = Duration.ofSeconds(1);
Review Comment:
How about we make this relative to `MAX_VALIDITY`? like `MAX_VALIDITY * 0.5`, or `MAX_VALIDITY * 0.1` by default?
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -485,34 +487,75 @@ private Optional<String> getUser() {
}
}
+ private static class CachedToken {
+ Instant generatedAt;
+ String token;
+
+ private CachedToken(Instant generatedAt, String token) {
+ this.generatedAt = generatedAt;
Review Comment:
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
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1921: SOLR-16951: Cache client pkiAuth headers for a second
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1921:
URL: https://github.com/apache/solr/pull/1921#discussion_r1326406193
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -485,34 +487,75 @@ private Optional<String> getUser() {
}
}
+ private static class CachedToken {
+ Instant generatedAt;
+ String token;
+
+ private CachedToken(Instant generatedAt, String token) {
+ this.generatedAt = generatedAt;
+ this.token = token;
+ }
+ }
+
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV1Tokens =
+ new ConcurrentHashMap<>();
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV2Tokens =
+ new ConcurrentHashMap<>();
+
+ private static final Duration cacheExpiryTime = Duration.ofSeconds(1);
+
+ private String getToken(String usr) {
+ AtomicReference<CachedToken> tokenRef =
+ cachedV1Tokens.computeIfAbsent(usr, u -> new AtomicReference<>(generateToken(u)));
+ if (tokenRef.get().generatedAt.isBefore(Instant.now().minus(cacheExpiryTime))) {
+ synchronized (tokenRef) {
+ if (tokenRef.get().generatedAt.isBefore(Instant.now().minus(cacheExpiryTime))) {
+ tokenRef.set(generateToken(usr));
+ }
+ }
+ }
+ return tokenRef.get().token;
+ }
+
+ 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))) {
Review Comment:
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 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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] stillalex commented on a diff in pull request #1921: SOLR-16951: Cache client pkiAuth headers for a second
Posted by "stillalex (via GitHub)" <gi...@apache.org>.
stillalex commented on code in PR #1921:
URL: https://github.com/apache/solr/pull/1921#discussion_r1326392234
##########
solr/core/src/java/org/apache/solr/security/PKIAuthenticationPlugin.java:
##########
@@ -485,34 +487,75 @@ private Optional<String> getUser() {
}
}
+ private static class CachedToken {
+ Instant generatedAt;
+ String token;
+
+ private CachedToken(Instant generatedAt, String token) {
+ this.generatedAt = generatedAt;
+ this.token = token;
+ }
+ }
+
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV1Tokens =
+ new ConcurrentHashMap<>();
+ private volatile ConcurrentHashMap<String, AtomicReference<CachedToken>> cachedV2Tokens =
+ new ConcurrentHashMap<>();
+
+ private static final Duration cacheExpiryTime = Duration.ofSeconds(1);
+
+ private String getToken(String usr) {
+ AtomicReference<CachedToken> tokenRef =
+ cachedV1Tokens.computeIfAbsent(usr, u -> new AtomicReference<>(generateToken(u)));
+ if (tokenRef.get().generatedAt.isBefore(Instant.now().minus(cacheExpiryTime))) {
+ synchronized (tokenRef) {
+ if (tokenRef.get().generatedAt.isBefore(Instant.now().minus(cacheExpiryTime))) {
+ tokenRef.set(generateToken(usr));
+ }
+ }
+ }
+ return tokenRef.get().token;
+ }
+
+ private synchronized String getTokenV2(String usr) {
Review Comment:
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)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org