aparchoudhary commented on code in PR #5443:
URL: https://github.com/apache/hadoop/pull/5443#discussion_r1131961554
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/security/RouterDelegationTokenSecretManager.java:
##########
@@ -37,16 +33,27 @@
import java.io.IOException;
import java.nio.ByteBuffer;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Map;
-import java.util.Set;
import java.util.Base64;
/**
- * A Router specific delegation token secret manager.
+ * A Router specific delegation token secret manager and is designed to be
stateless.
* The secret manager is responsible for generating and accepting the password
* for each token.
+ *
+ * Behavioural Differences from AbstractDelegationTokenSecretManager
+ * 1) Master Key - Each instance of Router will have its own master key and
each instance rolls its own master key.
+ * Thus there is no concept of a global current key.
+ * The requirement to generate new master keys / delegation tokens is to
generate unique INTEGER keys,
+ * which the state store is responsible for (Autoincrement is one of the
ways to achieve this).
+ * This key will be regenerated on service restart and thus there is no
requirement of an explicit restore mechanism.
+ * Current master key will be stored in memory on each instance and will be
used to generate new tokens.
+ * Master key will be looked up from the state store for Validation /
renewal, etc of tokens.
+ *
+ * 2) Token Expiry - It doesn't take care of token removal on expiry.
Review Comment:
Can we also mention about master key expiry?
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java:
##########
@@ -434,16 +446,17 @@ private void updateCurrentKey() throws IOException {
DelegationKey newKey = new DelegationKey(newCurrentId, System
.currentTimeMillis()
+ keyUpdateInterval + tokenMaxLifetime, generateSecret());
- //Log must be invoked outside the lock on 'this'
- logUpdateMasterKey(newKey);
synchronized (this) {
+ storeDelegationKey(newKey);
Review Comment:
While I understand that we shouldn't update currentKey when some other
thread is using it, but does the storeDelegationKey(newKey) also need to be
inside the synchronized block?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-router/src/main/java/org/apache/hadoop/yarn/server/router/security/RouterDelegationTokenSecretManager.java:
##########
@@ -91,283 +117,173 @@ private boolean shouldIgnoreException(Exception e) {
* @param newKey DelegationKey
*/
@Override
- public void storeNewMasterKey(DelegationKey newKey) {
+ protected void storeNewMasterKey(DelegationKey newKey) throws IOException {
try {
federationFacade.storeNewMasterKey(newKey);
- } catch (Exception e) {
- if (!shouldIgnoreException(e)) {
- LOG.error("Error in storing master key with KeyID: {}.",
newKey.getKeyId());
- ExitUtil.terminate(1, e);
- }
+ } catch (YarnException e) {
+ e.printStackTrace();
+ throw new IOException(e); // Wrap YarnException as an IOException to
adhere to storeNewMasterKey contract
}
}
/**
- * The Router Supports Remove the master key.
- * During this Process, Facade will call the specific StateStore to remove
the MasterKey.
- *
- * @param delegationKey DelegationKey
+ * no-op as expiry of stored keys is upto the state store in a stateless
secret manager
*/
@Override
public void removeStoredMasterKey(DelegationKey delegationKey) {
- try {
- federationFacade.removeStoredMasterKey(delegationKey);
- } catch (Exception e) {
- if (!shouldIgnoreException(e)) {
- LOG.error("Error in removing master key with KeyID: {}.",
delegationKey.getKeyId());
- ExitUtil.terminate(1, e);
- }
- }
+
}
/**
- * The Router Supports Store new Token.
- *
- * @param identifier RMDelegationToken
- * @param renewDate renewDate
- * @throws IOException IO exception occurred.
+ * no-op as we are storing entire token with info in storeToken()
*/
@Override
- public void storeNewToken(RMDelegationTokenIdentifier identifier,
- long renewDate) throws IOException {
- try {
- federationFacade.storeNewToken(identifier, renewDate);
Review Comment:
In the SQLFederationStateStore I see we are making 2 queries, one to add the
new token, 2nd again to get the token. Is the 2nd query needed? (Not able to
add review comment there so added 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
