-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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.
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
HADOOP-18581 : Handle Server KDC re-login when Server and Client run … #5248
Conversation
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@surendralilhore , thank you for the patch. I entered a few questions. Additionally, can you please describe if you've been able to simulate the problem in testing to confirm that this patch fixes it? (I assume unit testing isn't practical for this.)
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Outdated
Show resolved
Hide resolved
...mon-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not going to review the security details as i am not confident in my knowledge of UGI. i steer clear of IPC too, as it really belongs to the HDFS and YARN teams.
did some shallow log/layout comments
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Outdated
Show resolved
Hide resolved
...mon-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
Outdated
Show resolved
Hide resolved
...mon-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I requested just a few more style nitpick cleanups. After that, I'll be +1 for this. However, I would like to see at least one more committer review and +1 given the sensitivity of these code paths.
...mon-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
Show resolved
Hide resolved
...mon-project/hadoop-common/src/main/java/org/apache/hadoop/security/UserGroupInformation.java
Outdated
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
It has been a while since last time I check the security code. My understanding fades away as security is complex and risky. Consider my comments non-binding here. It makes sense to have to force re-login for addressing the issue.
|
Thanks @liuml07 for review.
Adding UT for this scenario is difficult. Passing Sasl message to server without any proper channel and making it fail is difficult.
Changed
Re-login logic extracted in new method and made it synchronized to avoid multiple re-relogin in concurrent scenario. |
💔 -1 overall
This message was automatically generated. |
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/ipc/Server.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Thank you, @surendralilhore .
Stating once again that I'd like to see at least one more committer +1 on it.
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @cnauroth , @liuml07 , @ayushtkn and @steveloughran for review.. |
Handle re-login in Server when client, server running in same JVM and client trying to re-login, but it fails.
For example, NameNode is server but in same JVM journal node client also running to push to edit logs. When JN client try to re-login and it fails, it will destroy server service ticket also and NameNode not able to server client request. We can see the below error logs in NameNode log file.