Skip to content

HDFS-17849 : Fix for Namenode crashed while cleaning up Expired Delegation tokens of older realm #8054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
+6 −1
Open

HDFS-17849 : Fix for Namenode crashed while cleaning up Expired Delegation tokens of older realm #8054

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6005bdd
Ignoring the exception in removeTokenForOwnerStats to remove expired …
Nov 4, 2025
03afebc
Split long log message into multiple lines for readability
Nov 5, 2025
010c58c
Changes to only catch IllegalArgumentException
Nov 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...ava/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,7 +858,12 @@ private void removeExpiredToken() throws IOException {
long renewDate = entry.getValue().getRenewDate();
if (renewDate < now) {
expiredTokens.add(entry.getKey());
removeTokenForOwnerStats(entry.getKey());
try {
removeTokenForOwnerStats(entry.getKey());
Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @arunreddyav for your report and contribution, I am a little confused the token could be leak when thrown exception here. I think the smooth way is config the hadoop.security.auth_to_local when change the realm. What do you think about? Thanks again.

Copy link
Author

@arunreddyav arunreddyav Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The token will not be leaked as I'm catching the exception and cleaned up in the logExpireTokens(expiredTokens);.
  • Including the older rules under hadoop.security.auth_to_local could be a possible approach; however, the customer prefers not to include the older rules for security reasons (for ex :- when moved to more secure zone old keytabs should not be allowed)

Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Got it. Make sense to me. However tokenOwnerStats could not be clean, this is one nit issue.
  2. 'when moved to more secure zone old keytabs should not be allowed' - I think this should be resolved at KDC side not Hadoop.

Copy link
Author

@arunreddyav arunreddyav Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review @Hexiaoqiao . Our customers create cluster does a sample job checks with local kerberoes. Once initial setup is done they will configure LDAP/Active Directory through Ambari . Once AD realm is configured they can not keep old realm based auth rules as its against the security

Copy link
Contributor

@Hexiaoqiao Hexiaoqiao Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree to add this try-catch into removeTokenForOwnerStats and make sure tokenOwnerStats also can be clean even when meet some auth issue and it's better to leave some annotation.

Copy link
Contributor

@surendralilhore surendralilhore Nov 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arunreddyav, I have a question, similar to what @Hexiaoqiao mentioned: how are we handling the cleanup of tokenOwnerStats in exception cases? Could you check if the following idea makes sense?
We need to ensure that tokenOwnerStats is cleaned up when an exception occurs. To do this, we should try to obtain the real user from the token identity, like so:

          try {
            removeTokenForOwnerStats(entry.getKey());
          } catch (IllegalArgumentException e) {
            **removeTokenForOwnerStats(entry.getKey().getRealUser().toString());**
            LOG.warn("Ignoring the exception in removeTokenForOwnerStats to remove expired " +
                    "delegation tokens from cache and proceeding to remove", e);
          }

Let's introduce a new removeTokenForOwnerStats(String) method, for example:

  private void removeTokenForOwnerStats(TokenIdent id) {
    String realOwner = getTokenRealOwner(id);
    removeTokenForOwnerStats(realOwner);
  }

  private void removeTokenForOwnerStats(String realOwner) {
    if (tokenOwnerStats.containsKey(realOwner)) {
      // unlikely to be less than 1 but in case
      if (tokenOwnerStats.get(realOwner) <= 1) {
        tokenOwnerStats.remove(realOwner);
      } else {
        tokenOwnerStats.put(realOwner, tokenOwnerStats.get(realOwner)-1);
      }
    }
  }

Copy link
Author

@arunreddyav arunreddyav Nov 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review, @surendralilhore.
entry.getKey().getRealUser().toString() will be empty for older realm tokens, so this approach may not work in those cases.

Copy link
Contributor

@surendralilhore surendralilhore Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Hexiaoqiao , any thoughts on how to clean the old token owner stats ?

} catch (Exception e) {
Copy link
Contributor

@surendralilhore surendralilhore Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please catch only IllegalArgumentException. Catching a global Exception may hide other issues or miss specific scenarios.

Copy link
Author

@arunreddyav arunreddyav Nov 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @surendralilhore for the review, I have addressed the review comment - 010c58c

Pls review.

LOG.warn("Ignoring the exception in removeTokenForOwnerStats to remove expired " +
"delegation tokens from cache and proceeding to remove", e);
}
i.remove();
}
}
Expand Down