pzampino commented on a change in pull request #350: URL: https://github.com/apache/knox/pull/350#discussion_r442827356
########## File path: gateway-server/src/main/java/org/apache/knox/gateway/services/token/impl/JournalBasedTokenStateService.java ########## @@ -0,0 +1,190 @@ +/* + * + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + * + */ +package org.apache.knox.gateway.services.token.impl; + +import org.apache.knox.gateway.config.GatewayConfig; +import org.apache.knox.gateway.services.ServiceLifecycleException; +import org.apache.knox.gateway.services.security.token.UnknownTokenException; +import org.apache.knox.gateway.services.token.impl.state.TokenStateJournalFactory; +import org.apache.knox.gateway.services.token.state.JournalEntry; +import org.apache.knox.gateway.services.token.state.TokenStateJournal; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.Set; + +public class JournalBasedTokenStateService extends DefaultTokenStateService { + + private TokenStateJournal journal; + + @Override + public void init(final GatewayConfig config, final Map<String, String> options) throws ServiceLifecycleException { + super.init(config, options); + + try { + // Initialize the token state journal + journal = TokenStateJournalFactory.create(config); + + // Load any persisted journal entries, and add them to the unpersisted state collection + List<JournalEntry> entries = journal.get(); + for (JournalEntry entry : entries) { + String id = entry.getTokenId(); + try { + long issueTime = Long.parseLong(entry.getIssueTime()); + long expiration = Long.parseLong(entry.getExpiration()); + long maxLifetime = Long.parseLong(entry.getMaxLifetime()); + + // Add the token state to memory + super.addToken(id, issueTime, expiration, maxLifetime); + + } catch (Exception e) { + log.failedToLoadJournalEntry(id, e); + } + } + } catch (IOException e) { + throw new ServiceLifecycleException("Failed to load persisted state from the token state journal", e); + } + } + + @Override + public void addToken(final String tokenId, long issueTime, long expiration, long maxLifetimeDuration) { + super.addToken(tokenId, issueTime, expiration, maxLifetimeDuration); + + try { + journal.add(tokenId, issueTime, expiration, maxLifetimeDuration); + } catch (IOException e) { + log.failedToAddJournalEntry(tokenId, e); + } + } + + @Override + public long getTokenExpiration(final String tokenId, boolean validate) throws UnknownTokenException { + // Check the in-memory collection first, to avoid costly keystore access when possible + try { + // If the token identifier is valid, and the associated state is available from the in-memory cache, then + // return the expiration from there. + return super.getTokenExpiration(tokenId, validate); + } catch (UnknownTokenException e) { + // It's not in memory + } + + if (validate) { + validateToken(tokenId); + } + + // If there is no associated state in the in-memory cache, proceed to check the journal + long expiration = 0; + try { + JournalEntry entry = journal.get(tokenId); + if (entry == null) { + throw new UnknownTokenException(tokenId); + } + + expiration = Long.parseLong(entry.getExpiration()); + super.addToken(tokenId, Review comment: The difference is due to the difference in the nature of the implementations. For AliasBasedTokenStateService, invoking _super.addToken_ would require an **additional keystore access** to get the max lifetime, and even then, I don't think the issue time is available; I think it's better to defer this additional keystore access for when the max lifetime is needed, and cache it then. Therefore, only the cached expiration is updated here. For JournalBasedTokenStateService, expiration and max lifetime are not managed separately (notice, it does not override _setMaxLifetime()_), and its _addToken_ implementation is effectively an update when the journal entry already exists. Since the max lifetime and issue time are available in this case, it seems to me more efficient to update the in-memory cache of the max lifetime here via _addToken_, rather than requiring an additional file access later. While the code differs, I believe the behavior is ultimately equivalent, and the implementations take advantage of their respective differences for a performance benefit. ---------------------------------------------------------------- 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: us...@infra.apache.org
