HDFS-16700. RBF: Record the real client IP carried by the Router in the NameNode log #4659
+145
−6
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
goiri
changed the title
goiri
reviewed
Jul 29, 2022
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.
Can we test with HADOOP_CALLER_CONTEXT_ENABLED_KEY set to true and false?
There was a problem hiding this comment.
Thanks @goiri for the comment and review.
I will update later.
|
🎊 +1 overall
This message was automatically generated. |
ZanderXu
requested changes
Jul 30, 2022
There was a problem hiding this comment.
This logic is same as NameNode#getClientMachine(), can you move it to the common module and can used by here and NameNode?
There was a problem hiding this comment.
Thanks for the heads up, I'll extract it separately and make it more generic.
There was a problem hiding this comment.
HDFS-13248 use isProxyUsers to control whether obtain client's ip and port from CallerContext. But in this PR, you plan ignore isProxyUsers?
There was a problem hiding this comment.
I don't think there is a conflict with HDFS-13248 here.
For Router, it will always pass Clientip and port to CallerContext, but NameNode may not always use it, which is not a problem with isProxyUsers.
Logging the Clientip in the log file would look clearer.
jianghuazhu
force-pushed
the
HDFS-16700
branch
2 times, most recently
from
210d1f4 to
cdbc91c
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Here are some logs from the online cluster. |
goiri
reviewed
Aug 2, 2022
There was a problem hiding this comment.
Does it make sense to refactor this into a general function and call it with the two configs?
The other interesting part would be to log the output too.
There was a problem hiding this comment.
Thanks @goiri for the comment and review.
If you want HADOOP_CALLER_CONTEXT_ENABLED_KEY to reset and take effect, MiniRouterDFSCluster needs to be restarted, so I refactored the process of starting MiniRouterDFSCluster. And use a static Configuration, which is for the NameNode.
You mean keep globalSetUp() unchanged and add static Configuration?
Another question is, do you think the new format of logging needs to be updated?
There was a problem hiding this comment.
I'll add some javadoc later.
ZanderXu
requested changes
Aug 4, 2022
There was a problem hiding this comment.
This method is duplicate with NameNode#parseSpecialValue, can you remove the method in NameNode.class and use this one?
There was a problem hiding this comment.
I agree with this suggestion.
There was a problem hiding this comment.
CLIENT_ID_STR and CLIENT_CALL_ID_STR are involved here, so I keep them for now.
It might be more appropriate to create a new jira to solve this problem?
What do you think, @ZanderXu .
There was a problem hiding this comment.
Sorry, I didn't get you idea. Just public parseSpecialValue method in CallerContext.java and remove parseSpecialValue method in NameNode.class, and modify the caller place in NameNode.java to use CallerContext.parseSpecialValue().
|
💔 -1 overall
This message was automatically generated. |
ZanderXu
reviewed
Aug 6, 2022
| * If not, return null. | ||
| */ | ||
| public static String getRealClientIp(String context) { | ||
| if (context != null && !context.equals("")) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
The string here is obtained from outside.
| * If not, return null. | ||
| */ | ||
| public static String getRealClientPort(String context) { | ||
| if (context != null && !context.equals("")) { |
There was a problem hiding this comment.
!context.equals("") -> context.isContextValid()
There was a problem hiding this comment.
The string here is obtained from outside.
|
|
||
| @Override | ||
| public String toString() { | ||
| boolean isCallerContextEnabled = conf.getBoolean( |
There was a problem hiding this comment.
I don't think this is a good idea to get the value from conf every time. And HADOOP_CALLER_CONTEXT_ENABLED_KEY is used to control whether output callerContext into audit log. We plan use this key here?
There was a problem hiding this comment.
Here, the essence is to obtain the real client ip, and it is not a problem to use HADOOP_CALLER_CONTEXT_ENABLED_KEY.
|
I have updated some, can you give some new suggestions, @goiri . |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
When applying RBF, the ip recorded in the log file saved in the NameNode is still the Router. The real client ip should be logged.
Details: HDFS-16700
How was this patch tested?
When hadoop.caller.context.enabled=true, you should see the client ip recorded in the NameNode log file.