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/02/07 19:56:08 UTC
[GitHub] [knox] moresandeep opened a new pull request #257: KNOX-2214 -
Periodic job to evict expired tokens
moresandeep opened a new pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257
## What changes were proposed in this pull request?
This PR adds a periodic task that looks for expired tokens and then evicts them. The aim of this task to run at a low priority and be able to gracefully handle failures if there are issues.
## How was this patch tested?
Local testing
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377892313
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -278,18 +281,34 @@ protected String getTimestampDisplay(long timestamp) {
* Method that deletes expired tokens based on the token timestamp.
*/
protected void evictExpiredTokens() {
- try {
- for (final String token : getTokens()) {
- if (isExpired(token)) {
+ for (final String token : getTokens()) {
+ try {
+ if (needsEviction(token)) {
log.evictToken(getTokenDisplayText(token));
removeToken(token);
}
+ } catch (final Exception e) {
+ log.failedExpiredTokenEviction(getTokenDisplayText(token), e);
}
- } catch(final Exception e) {
- log.failedExpiredTokenEviction(e);
}
}
+ /**
+ * Method that checks if an expired token is ready to be evicted
+ * by adding configured grace period to the expiry time.
+ * @param token
+ * @return
+ */
+ protected boolean needsEviction(final String token) {
+ boolean needsEviction;
+ needsEviction = isUnknown(token); // Check if the token exist
+ if (!needsEviction) {
+ /* If it not unknown, check if it is expired and within grace period before evicting */
+ needsEviction = (getTokenExpiration(token) + TimeUnit.SECONDS.toMillis(tokenEvictionGracePeriod)) <= System.currentTimeMillis();
Review comment:
I think this whole method could be simply this single statement:
return ((getTokenExpiration(token) + TimeUnit.SECONDS.toMillis(tokenEvictionGracePeriod)) <= System.currentTimeMillis());
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r376750720
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -126,9 +130,9 @@ protected void removeToken(final String token) {
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token);
- aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max");
+ aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "TOKEN_MAX_LIFETIME_POSTFIX");
} catch (AliasServiceException e) {
- log.failedToUpdateTokenExpiration(e);
+ log.errorAccessingTokenState(e);
Review comment:
This was an error in my previous commit that @smolnar82 pointed after the commit was merged.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377658848
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -264,4 +280,29 @@ protected String getTokenDisplayText(final String token) {
return String.format(Locale.ROOT, "%s...%s", token.substring(0, 10), token.substring(token.length() - 3));
}
+ /**
+ * Method that deletes expired tokens based on the token timestamp.
+ */
+ protected void evictExpiredTokens() {
+ try {
+ for (final String token : getTokens()) {
+ if (isExpired(token)) {
Review comment:
I've been wondering if we should allow some "grace period" beyond the expiration if renewal is still possible for a given token before evicting 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r376750856
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -43,9 +48,19 @@
private final Map<String, Long> maxTokenLifetimes = new HashMap<>();
+ /* token eviction interval in seconds, default is 5 mins */
+ private long tokenEvictionInterval;
+
+ private final ScheduledExecutorService evictionScheduler = Executors.newScheduledThreadPool(1);
Review comment:
Yes, this needs to be shutdown in stop method(), thanks for catching 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377122826
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -147,4 +151,18 @@ protected void updateExpiration(final String token, long expiration) {
log.failedToUpdateTokenExpiration(e);
}
}
+
+ @Override
+ protected List<String> getTokens() {
+ List<String> allAliases = new ArrayList();
+ try {
+ allAliases = aliasService.getAliasesForCluster(AliasService.NO_CLUSTER_NAME);
+ /* only get the aliases that represent tokens and extract the current list of tokens */
+ allAliases.stream().filter(a -> a.contains("TOKEN_MAX_LIFETIME_POSTFIX")).map(a -> a.substring(0, a.indexOf("TOKEN_MAX_LIFETIME_POSTFIX")))
Review comment:
This shouldn't be a string - it should be the variable reference?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377143722
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -126,9 +130,9 @@ protected void removeToken(final String token) {
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token);
- aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max");
+ aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "TOKEN_MAX_LIFETIME_POSTFIX");
Review comment:
<facepalm> and I was wondering my *--max tokens are not getting deleted. Thanks @risdenk !
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377135710
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -126,9 +130,9 @@ protected void removeToken(final String token) {
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token);
- aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max");
+ aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "TOKEN_MAX_LIFETIME_POSTFIX");
Review comment:
I don't follow, this is a variable reference, I took out the string and created a variable "TOKEN_MAX_LIFETIME_POSTFIX".
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377665963
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -264,4 +280,29 @@ protected String getTokenDisplayText(final String token) {
return String.format(Locale.ROOT, "%s...%s", token.substring(0, 10), token.substring(token.length() - 3));
}
+ /**
+ * Method that deletes expired tokens based on the token timestamp.
+ */
+ protected void evictExpiredTokens() {
+ try {
+ for (final String token : getTokens()) {
+ if (isExpired(token)) {
+ log.evictToken(getTokenDisplayText(token));
+ removeToken(token);
+ }
+ }
+ } catch(final Exception e) {
Review comment:
Sure, that can be 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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377857775
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -264,4 +280,29 @@ protected String getTokenDisplayText(final String token) {
return String.format(Locale.ROOT, "%s...%s", token.substring(0, 10), token.substring(token.length() - 3));
}
+ /**
+ * Method that deletes expired tokens based on the token timestamp.
+ */
+ protected void evictExpiredTokens() {
+ try {
+ for (final String token : getTokens()) {
+ if (isExpired(token)) {
Review comment:
Thanks for explaining Phil, fixed it in new PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377143722
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -126,9 +130,9 @@ protected void removeToken(final String token) {
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token);
- aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max");
+ aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "TOKEN_MAX_LIFETIME_POSTFIX");
Review comment:
\<facepalm\> and I was wondering my *--max tokens are not getting deleted. Thanks @risdenk !
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r376646938
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java
##########
@@ -245,6 +245,9 @@
private static final String CLOUDERA_MANAGER_ADVANCED_SERVICE_DISCOVERY_CONF_MONITOR_INTERVAL = GATEWAY_CONFIG_FILE_PREFIX + ".cloudera.manager.advanced.service.discovery.config.monitor.interval";
private static final long DEFAULT_CLOUDERA_MANAGER_ADVANCED_SERVICE_DISCOVERY_CONF_MONITOR_INTERVAL = 30000L;
+ private static final String KNOX_TOKEN_EVICTION_INTERVAL = GATEWAY_CONFIG_FILE_PREFIX + "knox.token.eviction.interval";
Review comment:
missing dot on before `knox`. all other configs have `GATEWAY_CONFIG_FILE_PREFIX + ".abcdef"`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r376647515
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -126,9 +130,9 @@ protected void removeToken(final String token) {
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token);
- aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max");
+ aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "TOKEN_MAX_LIFETIME_POSTFIX");
} catch (AliasServiceException e) {
- log.failedToUpdateTokenExpiration(e);
+ log.errorAccessingTokenState(e);
Review comment:
Why did this log message 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377665210
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -264,4 +280,29 @@ protected String getTokenDisplayText(final String token) {
return String.format(Locale.ROOT, "%s...%s", token.substring(0, 10), token.substring(token.length() - 3));
}
+ /**
+ * Method that deletes expired tokens based on the token timestamp.
+ */
+ protected void evictExpiredTokens() {
+ try {
+ for (final String token : getTokens()) {
+ if (isExpired(token)) {
Review comment:
Hmm, wouldn't that make the token expiration time a bit ambiguous? are you suggesting that renew token should be possible until (expiry + x) seconds?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377137601
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -126,9 +130,9 @@ protected void removeToken(final String token) {
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token);
- aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max");
+ aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "TOKEN_MAX_LIFETIME_POSTFIX");
Review comment:
Yea you have `TOKEN_MAX_LIFETIME_POSTFIX` as a string with `"TOKEN_MAX_LIFETIME_POSTFIX"`. Instead of just using `TOKEN_MAX_LIFETIME_POSTFIX`.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r376647793
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -43,9 +48,19 @@
private final Map<String, Long> maxTokenLifetimes = new HashMap<>();
+ /* token eviction interval in seconds, default is 5 mins */
+ private long tokenEvictionInterval;
+
+ private final ScheduledExecutorService evictionScheduler = Executors.newScheduledThreadPool(1);
Review comment:
Where do we shut this down?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on issue #257: KNOX-2214 - Periodic
job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on issue #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#issuecomment-584816163
@pzampino Addressed your review comments with the new PR.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r376750681
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/config/impl/GatewayConfigImpl.java
##########
@@ -245,6 +245,9 @@
private static final String CLOUDERA_MANAGER_ADVANCED_SERVICE_DISCOVERY_CONF_MONITOR_INTERVAL = GATEWAY_CONFIG_FILE_PREFIX + ".cloudera.manager.advanced.service.discovery.config.monitor.interval";
private static final long DEFAULT_CLOUDERA_MANAGER_ADVANCED_SERVICE_DISCOVERY_CONF_MONITOR_INTERVAL = 30000L;
+ private static final String KNOX_TOKEN_EVICTION_INTERVAL = GATEWAY_CONFIG_FILE_PREFIX + "knox.token.eviction.interval";
Review comment:
good catch, I'll fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on issue #257: KNOX-2214 - Periodic
job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on issue #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#issuecomment-584878545
@pzampino addressed the review comments
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377890400
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -51,13 +51,16 @@
/* token eviction interval in seconds, default is 5 mins */
private long tokenEvictionInterval;
+ /* grace period (in seconds) after which an expired token should be evicted, default 5 mins */
Review comment:
nit: This comment won't change when the default value changes in the future. I try not to include values in comments
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377890180
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -278,18 +281,34 @@ protected String getTimestampDisplay(long timestamp) {
* Method that deletes expired tokens based on the token timestamp.
*/
protected void evictExpiredTokens() {
- try {
- for (final String token : getTokens()) {
- if (isExpired(token)) {
+ for (final String token : getTokens()) {
+ try {
+ if (needsEviction(token)) {
log.evictToken(getTokenDisplayText(token));
removeToken(token);
}
+ } catch (final Exception e) {
+ log.failedExpiredTokenEviction(getTokenDisplayText(token), e);
}
- } catch(final Exception e) {
- log.failedExpiredTokenEviction(e);
}
}
+ /**
+ * Method that checks if an expired token is ready to be evicted
+ * by adding configured grace period to the expiry time.
+ * @param token
+ * @return
+ */
+ protected boolean needsEviction(final String token) {
+ boolean needsEviction;
+ needsEviction = isUnknown(token); // Check if the token exist
Review comment:
If the token is unknown in this case, you're going to get an IllegalArgumentException when the subsequent removal is attempted because this statement says an unknown token -> needsEviction. I would argue that an unknown token means it has probably already been evicted.
The following call to getTokenExpiration will already invoked validateToken(), so the up-front isUnknownToken check here is unnecessary (IMO) and the conclusion is incorrect.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377137943
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -43,9 +48,19 @@
private final Map<String, Long> maxTokenLifetimes = new HashMap<>();
+ /* token eviction interval in seconds, default is 5 mins */
+ private long tokenEvictionInterval;
+
+ private final ScheduledExecutorService evictionScheduler = Executors.newScheduledThreadPool(1);
+
@Override
public void init(final GatewayConfig config, final Map<String, String> options) throws ServiceLifecycleException {
+ tokenEvictionInterval = config.getKnoxTokenEvictionInterval();
+ if (tokenEvictionInterval > 0) {
Review comment:
Yea agreed should go in start.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377660374
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -264,4 +280,29 @@ protected String getTokenDisplayText(final String token) {
return String.format(Locale.ROOT, "%s...%s", token.substring(0, 10), token.substring(token.length() - 3));
}
+ /**
+ * Method that deletes expired tokens based on the token timestamp.
+ */
+ protected void evictExpiredTokens() {
+ try {
+ for (final String token : getTokens()) {
+ if (isExpired(token)) {
+ log.evictToken(getTokenDisplayText(token));
+ removeToken(token);
+ }
+ }
+ } catch(final Exception e) {
Review comment:
Would it not be better to catch the error for each eviction attempt, and continue trying the others when one fails, rather than failing the whole attempt because of a single error?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377088643
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -43,9 +48,19 @@
private final Map<String, Long> maxTokenLifetimes = new HashMap<>();
+ /* token eviction interval in seconds, default is 5 mins */
+ private long tokenEvictionInterval;
+
+ private final ScheduledExecutorService evictionScheduler = Executors.newScheduledThreadPool(1);
+
@Override
public void init(final GatewayConfig config, final Map<String, String> options) throws ServiceLifecycleException {
+ tokenEvictionInterval = config.getKnoxTokenEvictionInterval();
+ if (tokenEvictionInterval > 0) {
Review comment:
This probably should go in start() method.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377137751
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -147,4 +151,18 @@ protected void updateExpiration(final String token, long expiration) {
log.failedToUpdateTokenExpiration(e);
}
}
+
+ @Override
+ protected List<String> getTokens() {
+ List<String> allAliases = new ArrayList();
+ try {
+ allAliases = aliasService.getAliasesForCluster(AliasService.NO_CLUSTER_NAME);
+ /* only get the aliases that represent tokens and extract the current list of tokens */
+ allAliases.stream().filter(a -> a.contains("TOKEN_MAX_LIFETIME_POSTFIX")).map(a -> a.substring(0, a.indexOf("TOKEN_MAX_LIFETIME_POSTFIX")))
Review comment:
Yea you have TOKEN_MAX_LIFETIME_POSTFIX as a string with "TOKEN_MAX_LIFETIME_POSTFIX". Instead of just using TOKEN_MAX_LIFETIME_POSTFIX.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] moresandeep commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377136083
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -147,4 +151,18 @@ protected void updateExpiration(final String token, long expiration) {
log.failedToUpdateTokenExpiration(e);
}
}
+
+ @Override
+ protected List<String> getTokens() {
+ List<String> allAliases = new ArrayList();
+ try {
+ allAliases = aliasService.getAliasesForCluster(AliasService.NO_CLUSTER_NAME);
+ /* only get the aliases that represent tokens and extract the current list of tokens */
+ allAliases.stream().filter(a -> a.contains("TOKEN_MAX_LIFETIME_POSTFIX")).map(a -> a.substring(0, a.indexOf("TOKEN_MAX_LIFETIME_POSTFIX")))
Review comment:
I don't follow, this is a variable reference, I took out the string and created a variable "TOKEN_MAX_LIFETIME_POSTFIX".
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] pzampino commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
pzampino commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377729105
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/DefaultTokenStateService.java
##########
@@ -264,4 +280,29 @@ protected String getTokenDisplayText(final String token) {
return String.format(Locale.ROOT, "%s...%s", token.substring(0, 10), token.substring(token.length() - 3));
}
+ /**
+ * Method that deletes expired tokens based on the token timestamp.
+ */
+ protected void evictExpiredTokens() {
+ try {
+ for (final String token : getTokens()) {
+ if (isExpired(token)) {
Review comment:
Yes, as we've discussed, while renewers likely will request renewals prior to expiration, they are not currently required to do so. Delaying the eviction avoids potential "unknown token" issues for renewers, and does not affect the expiration 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [knox] moresandeep merged pull request #257: KNOX-2214 - Periodic
job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
moresandeep merged pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [knox] risdenk commented on a change in pull request #257:
KNOX-2214 - Periodic job to evict expired tokens
Posted by GitBox <gi...@apache.org>.
risdenk commented on a change in pull request #257: KNOX-2214 - Periodic job to evict expired tokens
URL: https://github.com/apache/knox/pull/257#discussion_r377122729
##########
File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/AliasBasedTokenStateService.java
##########
@@ -126,9 +130,9 @@ protected void removeToken(final String token) {
try {
aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME, token);
- aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "--max");
+ aliasService.removeAliasForCluster(AliasService.NO_CLUSTER_NAME,token + "TOKEN_MAX_LIFETIME_POSTFIX");
Review comment:
This shouldn't be a string - it should be the variable reference?
----------------------------------------------------------------
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
With regards,
Apache Git Services