HDFS-4043. Namenode Kerberos Login does not use proper hostname for host qualified hdfs principal name. #4693
Conversation
This file contains 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
|
The InetAddressUtils class in this patch will also be used to address HDFS-16685. Once this patch is accepted, I'll create the pull request for HDFS-16685. |
ndimiduk
requested changes
Aug 8, 2022
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/InetAddressUtils.java
Outdated
Show resolved
Hide resolved
| localhost.getAddress()); | ||
|
|
||
| // Precondition: host name and canonical host name for unresolved returns an IP address. | ||
| assertEquals(localhost.getHostAddress(), unresolved.getHostName()); |
There was a problem hiding this comment.
The behavior of this test will vary based on the dns environment at the time it's run. At the very least, you can make this an assume statement so that the test will make no assertions when the environmental conditions are inappropriate.
For more rigorous testing, it seems like mocking of a static method becomes possible as of Mockito 3.4.0. Alternatively, I wonder if you can create an environment where you manipulate the property jdk.net.hosts.file and provide a file that you populate for the duration of the test.
There was a problem hiding this comment.
I should specify. At least in OpenJDK11, in the InetAddress class, there's a private static NameService createNameService() that makes use of this property. I don't know if this is formally documented someplace on the JVM.
There was a problem hiding this comment.
We're still support Java 8, so I avoided using Java 11 classes.
There was a problem hiding this comment.
Hmm yes, I see no mention of jdk.net.hosts.file in the InetAddress of OpenJDK8.
ndimiduk
approved these changes
Aug 12, 2022
added 5 commits
August 12, 2022 14:55
…ost qualified hdfs principal name. Perform a DNS reverse name lookup when getCanonicalHost() returns the IP address as a string.
Since the core of the problem relates to DNS interactions with private fields inside of Java language classes, this test can only verify that externally a request for a canonical host name works.
Instead of introducing a new utility class (i.e. InetAddressUtils) switch to using the existing pluggable framework for DomainNameResolver. Update default implementation DNSDomainNameResolver to protected against returning the IP address as a string from a cached value.
snmvaughan
force-pushed
the
feature/HDFS-4043
branch
from
f87f452
to
d14eeba
Compare
|
cc @fengnanli @goiri since you authored the original code |
goiri
reviewed
Aug 12, 2022
...-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/DNSDomainNameResolver.java
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
Outdated
Show resolved
Hide resolved
sunchao
approved these changes
Aug 12, 2022
...-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/DNSDomainNameResolver.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
SpotBugs is concerned that getCanonicalHostName() may return null, although the JavaDocs indicate that it will return the IP address as a String if unable to determine the FQDN. The only circumstance I can imagine where it would return null is if the IP address is null, which would mean a reverse lookup wouldn't work either.
|
🎊 +1 overall
This message was automatically generated. |
|
Thanks, merged to trunk |
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…ost qualified hdfs principal name. (apache#4693)
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
Perform a DNS reverse name lookup when getCanonicalHost() returns the IP address as a string. The standard InetAddress getCanonicalHostName() has the following issues:
How was this patch tested?
An HA configuration was deployed to Kubernetes, running Java 11. All services startup as expected. Performing a rolling restart of the JournalNodes caused failures when the NameNode communicated with the newly started JournalNode, reporting an exception that included a principal with the IP address instead of the canonical host name.
Performing the same activity after applying the patch succeeds as the DNS reverse lookup is performed to acquire the correct calculated principal.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?