You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@knox.apache.org by GitBox <gi...@apache.org> on 2020/08/31 11:47:11 UTC
[GitHub] [knox] smolnar82 opened a new pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
smolnar82 opened a new pull request #371:
URL: https://github.com/apache/knox/pull/371
## What changes were proposed in this pull request?
- Let end-users customize the size/TTL attributes of the cache in `DefaultKeystoreService` I introduced while working on [KNOX-2136](https://issues.apache.org/jira/browse/KNOX-2136)
- `AliasBasedTokenStateService.updateExpiration` updates the data in-memory and let the background thread does its job
- if `AliasBasedTokenStateService` is selected as token state service, a new background thread is run when the service is started which reads previously persisted relevant aliases into the in-memory collections (in `DefaultTokenStateService`). With this change, it's not needed to override `getTokens` in `AliasBasedTokenStateService` which is only used by the reaper thread when evicting expired tokens. Instead, the token eviction logic will wait until everything is loaded.
- `DefaultTokenStateService.tokenExpirations` and `DefaultTokenStateService.maxTokenLifetimes` became `ConcurrentHashMap`s -> get rid of many of the synchronization blocks -> easier to read and maintain the code and `CHM` performs better than synchronization + `HashMap`
## How was this patch tested?
Updated and ran JUnit tests as well as executed the performance test tool multiple times to make sure I achieved the performance goals I was looking for with these changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #371:
URL: https://github.com/apache/knox/pull/371#discussion_r485133225
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultAliasService.java
##########
@@ -194,6 +195,24 @@ public void removeAliasesForCluster(String clusterName, Set<String> aliases) thr
return getPasswordFromAliasForCluster(NO_CLUSTER_NAME, alias);
}
+ //Overriding the default behavior as we want to avoid loading the keystore N-times from the file system
+ @Override
Review comment:
I'm going to re-check and change other methods too if needed.
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/services/security/AliasService.java
##########
@@ -54,6 +54,8 @@ void generateAliasForCluster(String clusterName, String alias)
char[] getPasswordFromAliasForGateway(String alias)
throws AliasServiceException;
+ Map<String, char[]> getPasswordAliasMapForGateway() throws AliasServiceException;
Review comment:
I'll change it.
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -122,6 +129,44 @@ public void start() throws ServiceLifecycleException {
statePersistenceInterval,
TimeUnit.SECONDS);
}
+
+ // Loading ALL entries from __gateway-credentials.jceks could be VERY time-consuming (it took a bit more than 19 minutes to load 12k aliases
+ // during my tests).
+ // Therefore, it's safer to do it in a background thread than just make the service start hang until it's finished
+ final ExecutorService gatewayCredentialsLoader = Executors.newSingleThreadExecutor(new BasicThreadFactory.Builder().namingPattern("GatewayCredentialsLoader").build());
+ gatewayCredentialsLoader.execute(this::loadGatewayCredentialsOnStartup);
+ }
+
+ private void loadGatewayCredentialsOnStartup() {
+ try {
+ log.loadingGatewayCredentialsOnStartup();
+ final long start = System.currentTimeMillis();
+ final Map<String, char[]> passwordAliasMap = aliasService.getPasswordAliasMapForGateway();
+ String alias, tokenId;
+ long expiration, maxLifeTime;
+ int count = 0;
+ for (Map.Entry<String, char[]> passwordAliasMapEntry : passwordAliasMap.entrySet()) {
+ alias = passwordAliasMapEntry.getKey();
+ if (alias.endsWith(TOKEN_MAX_LIFETIME_POSTFIX)) {
Review comment:
Sure; I'll add documentation.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] pzampino commented on a change in pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #371:
URL: https://github.com/apache/knox/pull/371#discussion_r480391677
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -344,36 +320,44 @@ protected void validateToken(final String tokenId) throws IllegalArgumentExcepti
}
}
- protected String getTimestampDisplay(long timestamp) {
+ private String getTimestampDisplay(long timestamp) {
return Instant.ofEpochMilli(timestamp).toString();
}
/**
* Method that deletes expired tokens based on the token timestamp.
*/
protected void evictExpiredTokens() {
- Set<String> tokensToEvict = new HashSet<>();
-
- for (final String tokenId : getTokens()) {
- try {
- if (needsEviction(tokenId)) {
- log.evictToken(tokenId);
- tokensToEvict.add(tokenId); // Add the token to the set of tokens to evict
+ if (readyForEviction()) {
+ final Set<String> tokensToEvict = new HashSet<>();
+
+ for (final String tokenId : getTokenIds()) {
+ try {
+ if (needsEviction(tokenId)) {
+ log.evictToken(tokenId);
+ tokensToEvict.add(tokenId); // Add the token to the set of tokens to evict
+ }
+ } catch (final Exception e) {
+ log.failedExpiredTokenEviction(tokenId, e);
}
- } catch (final Exception e) {
- log.failedExpiredTokenEviction(tokenId, e);
}
- }
- if (!tokensToEvict.isEmpty()) {
- try {
- removeTokens(tokensToEvict);
- } catch (UnknownTokenException e) {
- log.failedExpiredTokenEviction(e);
+ if (!tokensToEvict.isEmpty()) {
+ try {
+ removeTokens(tokensToEvict);
+ } catch (UnknownTokenException e) {
+ log.failedExpiredTokenEviction(e);
+ }
}
+ } else {
+ log.skipEviction();
}
}
+ protected boolean readyForEviction() {
+ return true;
Review comment:
Why is this method (and associated invocation) required if the answer is always 'true'?
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/services/security/AliasService.java
##########
@@ -54,6 +54,8 @@ void generateAliasForCluster(String clusterName, String alias)
char[] getPasswordFromAliasForGateway(String alias)
throws AliasServiceException;
+ Map<String, char[]> getPasswordAliasMapForGateway() throws AliasServiceException;
Review comment:
Why not simply getGatewayAliases(), more similar to other signatures in this interface? Why the need to cite the return type in the method signature?
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -122,6 +129,44 @@ public void start() throws ServiceLifecycleException {
statePersistenceInterval,
TimeUnit.SECONDS);
}
+
+ // Loading ALL entries from __gateway-credentials.jceks could be VERY time-consuming (it took a bit more than 19 minutes to load 12k aliases
+ // during my tests).
+ // Therefore, it's safer to do it in a background thread than just make the service start hang until it's finished
+ final ExecutorService gatewayCredentialsLoader = Executors.newSingleThreadExecutor(new BasicThreadFactory.Builder().namingPattern("GatewayCredentialsLoader").build());
+ gatewayCredentialsLoader.execute(this::loadGatewayCredentialsOnStartup);
+ }
+
+ private void loadGatewayCredentialsOnStartup() {
+ try {
+ log.loadingGatewayCredentialsOnStartup();
+ final long start = System.currentTimeMillis();
+ final Map<String, char[]> passwordAliasMap = aliasService.getPasswordAliasMapForGateway();
+ String alias, tokenId;
+ long expiration, maxLifeTime;
+ int count = 0;
+ for (Map.Entry<String, char[]> passwordAliasMapEntry : passwordAliasMap.entrySet()) {
+ alias = passwordAliasMapEntry.getKey();
+ if (alias.endsWith(TOKEN_MAX_LIFETIME_POSTFIX)) {
Review comment:
Some comments here might be helpful for future viewers. I understand what is being done here, but others may not.
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultAliasService.java
##########
@@ -194,6 +195,24 @@ public void removeAliasesForCluster(String clusterName, Set<String> aliases) thr
return getPasswordFromAliasForCluster(NO_CLUSTER_NAME, alias);
}
+ //Overriding the default behavior as we want to avoid loading the keystore N-times from the file system
+ @Override
Review comment:
Does something similar need to be done for cluster-specific aliases?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #371:
URL: https://github.com/apache/knox/pull/371#discussion_r485142436
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/security/impl/DefaultAliasService.java
##########
@@ -194,6 +195,24 @@ public void removeAliasesForCluster(String clusterName, Set<String> aliases) thr
return getPasswordFromAliasForCluster(NO_CLUSTER_NAME, alias);
}
+ //Overriding the default behavior as we want to avoid loading the keystore N-times from the file system
+ @Override
Review comment:
So, in this case, the cluster we use is `__gateway` and the only reason this method was added is to avoid exhausting I/O operations in the new `AliasBasedTokenStateService.loadGatewayCredentialsOnStartup()` method.
Therefore I don't see if other methods should be changes within the scope of this task.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on pull request #371:
URL: https://github.com/apache/knox/pull/371#issuecomment-689092096
Thanks, @pzampino for your valuable review comments. I addressed them and submitted a new commit. Please check it out and let me know if there is anything else that should be done or I can merge my changes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 merged pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
smolnar82 merged pull request #371:
URL: https://github.com/apache/knox/pull/371
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #371:
URL: https://github.com/apache/knox/pull/371#discussion_r480077126
##########
File path: gateway-server/src/test/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateServiceTest.java
##########
@@ -195,7 +204,8 @@ public void testAddAndRemoveTokenIncludesCache() throws Exception {
* Verify that the token state reaper includes token state which has not been cached, so it's not left in the keystore
* forever.
*/
- @Test
+ @Ignore("I'm not sure if this is a valid use case since we have everything in the cache when eviction takes place")
Review comment:
@pzampino As you can see I only ignored this test case for now because I'm not sure if it makes sense going forward. Since the eviction thread checks everything in the cache we may remove this test case. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #371:
URL: https://github.com/apache/knox/pull/371#discussion_r485130067
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -344,36 +320,44 @@ protected void validateToken(final String tokenId) throws IllegalArgumentExcepti
}
}
- protected String getTimestampDisplay(long timestamp) {
+ private String getTimestampDisplay(long timestamp) {
return Instant.ofEpochMilli(timestamp).toString();
}
/**
* Method that deletes expired tokens based on the token timestamp.
*/
protected void evictExpiredTokens() {
- Set<String> tokensToEvict = new HashSet<>();
-
- for (final String tokenId : getTokens()) {
- try {
- if (needsEviction(tokenId)) {
- log.evictToken(tokenId);
- tokensToEvict.add(tokenId); // Add the token to the set of tokens to evict
+ if (readyForEviction()) {
+ final Set<String> tokensToEvict = new HashSet<>();
+
+ for (final String tokenId : getTokenIds()) {
+ try {
+ if (needsEviction(tokenId)) {
+ log.evictToken(tokenId);
+ tokensToEvict.add(tokenId); // Add the token to the set of tokens to evict
+ }
+ } catch (final Exception e) {
+ log.failedExpiredTokenEviction(tokenId, e);
}
- } catch (final Exception e) {
- log.failedExpiredTokenEviction(tokenId, e);
}
- }
- if (!tokensToEvict.isEmpty()) {
- try {
- removeTokens(tokensToEvict);
- } catch (UnknownTokenException e) {
- log.failedExpiredTokenEviction(e);
+ if (!tokensToEvict.isEmpty()) {
+ try {
+ removeTokens(tokensToEvict);
+ } catch (UnknownTokenException e) {
+ log.failedExpiredTokenEviction(e);
+ }
}
+ } else {
+ log.skipEviction();
}
}
+ protected boolean readyForEviction() {
+ return true;
Review comment:
It's overridden in `AliasBasedTokenStateService` where the returned value depends on the state of loading all aliases from `__gateway-credentials.jckes`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #371:
URL: https://github.com/apache/knox/pull/371#discussion_r485150809
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -122,6 +129,44 @@ public void start() throws ServiceLifecycleException {
statePersistenceInterval,
TimeUnit.SECONDS);
}
+
+ // Loading ALL entries from __gateway-credentials.jceks could be VERY time-consuming (it took a bit more than 19 minutes to load 12k aliases
+ // during my tests).
+ // Therefore, it's safer to do it in a background thread than just make the service start hang until it's finished
+ final ExecutorService gatewayCredentialsLoader = Executors.newSingleThreadExecutor(new BasicThreadFactory.Builder().namingPattern("GatewayCredentialsLoader").build());
+ gatewayCredentialsLoader.execute(this::loadGatewayCredentialsOnStartup);
+ }
+
+ private void loadGatewayCredentialsOnStartup() {
+ try {
+ log.loadingGatewayCredentialsOnStartup();
+ final long start = System.currentTimeMillis();
+ final Map<String, char[]> passwordAliasMap = aliasService.getPasswordAliasMapForGateway();
+ String alias, tokenId;
+ long expiration, maxLifeTime;
+ int count = 0;
+ for (Map.Entry<String, char[]> passwordAliasMapEntry : passwordAliasMap.entrySet()) {
+ alias = passwordAliasMapEntry.getKey();
+ if (alias.endsWith(TOKEN_MAX_LIFETIME_POSTFIX)) {
Review comment:
Done.
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/services/security/AliasService.java
##########
@@ -54,6 +54,8 @@ void generateAliasForCluster(String clusterName, String alias)
char[] getPasswordFromAliasForGateway(String alias)
throws AliasServiceException;
+ Map<String, char[]> getPasswordAliasMapForGateway() throws AliasServiceException;
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [knox] smolnar82 commented on a change in pull request #371: KNOX-2408 - Improved AliasBasedTokenState service and house-keeping
Posted by GitBox <gi...@apache.org>.
smolnar82 commented on a change in pull request #371:
URL: https://github.com/apache/knox/pull/371#discussion_r485150849
##########
File path: gateway-spi/src/main/java/org/apache/knox/gateway/services/security/AliasService.java
##########
@@ -54,6 +54,8 @@ void generateAliasForCluster(String clusterName, String alias)
char[] getPasswordFromAliasForGateway(String alias)
throws AliasServiceException;
+ Map<String, char[]> getPasswordAliasMapForGateway() throws AliasServiceException;
Review comment:
Changed it to `getPasswordsForGateway`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org