-
Notifications
You must be signed in to change notification settings - Fork 9.2k
HDFS-16895. NamenodeHeartbeatService should use credentials of logged… #5324
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
Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| } | ||
|
|
||
| @Test | ||
| public void testNamendodeHeartbeatWithSecurity() throws Exception { |
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.
Nit: Typo "namendode" -> "namenode"
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.
Fixed. Thanks!
| // Read the stats from JMX (optional) | ||
| updateJMXParameters(webAddress, report); | ||
| // Read the stats from JMX (optional) using the login user credentials | ||
| SecurityUtil.doAsLoginUser((PrivilegedExceptionAction<Void>) () -> { |
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.
It feels like we should do this at a higher level eg. NamenodeHeartbeatService.periodicInvoke(). That way the other operations, such as the rpc call to each NN, also run as the login user.
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.
Only the JMX call is having issues authenticating, but I agree we should expect all requests to be made in the same context. I'll expand the scope of this.
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
omalley
left a comment
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 LGTM
Thanks, Hector!
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
… in user
Description of PR
Ensuring that the NamenodeHeartbeatService uses the credentials of the logged in user before calling the JMX API. Without this fix, requests to secured JMX APIs will log the following error:
"Cannot parse JMX output for Hadoop:service=NameNode,name=FSNamesystem* from server"
How was this patch tested?
Deployed to a test cluster, in which the JMX API is secured.
Ran kdestroy on the router to ensure no credentials are taken from kinit'ed users.
Validated that NamenodeHeartbeatService is able to complete the JMX request with no errors.
Added unit test to validate report is generated when using a secured cluster
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?