-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
HDFS-4043. Namenode Kerberos Login does not use proper hostname for host qualified hdfs principal name. #4693
Conversation
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still support Java 8, so I avoided using Java 11 classes.
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.
Hmm yes, I see no mention of jdk.net.hosts.file
in the InetAddress
of OpenJDK8.
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.
Thanks for updating the visibility annotation.
…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.
f87f452
to
d14eeba
Compare
cc @fengnanli @goiri since you authored the original code |
...-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
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.
Looks good to me.
...-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 |
…ost qualified hdfs principal name. (apache#4693)
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-binary
files?