-
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-16686. GetJournalEditServlet fails to authorize valid Kerberos request #4724
Conversation
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.
Amusing -- this appears to be exactly the purpose that this DfsServlet
is intended to serve.
This looks like an example of a flaky test, where a previous test execution affects tests that follow. Specifically the reported failed test is |
…equest Use DfsServlet to obtain the UGI.
Changed "conf" and "servlet" to uppercase since they are static (the servlet is initialized only once).
Update the tests to ensure that the mini clusters are shutdown properly.
This update allows MiniQJMHACluster to be used in try-with-resources, ensuring that tests are cleaned-up correctly.
This update ensures that tests are cleaned-up correctly.
Create a default HDFS configuration which has test-specific data directories. This is intended to protect against interactions between test runs that might corrupt results. Each test run's data is automatically cleaned-up by JUnit.
e1039f4
to
1c8f552
Compare
🎊 +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.
The main change looks good to me, I have a few questions on the test changes
...hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/qjournal/MiniQJMHACluster.java
Show resolved
Hide resolved
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestRollingUpgrade.java
Outdated
Show resolved
Hide resolved
@@ -80,16 +82,38 @@ public static void runCmd(DFSAdmin dfsadmin, boolean success, | |||
} | |||
} | |||
|
|||
@Rule | |||
public TemporaryFolder folder= new TemporaryFolder(); |
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: space before =
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestRollingUpgrade.java
Show resolved
Hide resolved
|
||
@BeforeClass | ||
public static void setUp() throws ServletException { | ||
LogManager.getLogger(GetJournalEditServlet.class).setLevel(Level.DEBUG); |
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.
why do we want to set the logger level to DEBUG here?
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.
That was a hold over from debug testing. I'll remove it.
🎊 +1 overall
This message was automatically generated. |
The signature for close() can't throw an exception for AutoCloseable, but the definition of shutdown() didn't have to change. Move the try-catch into close().
🎊 +1 overall
This message was automatically generated. |
Merged, thanks |
Description of PR
GetJournalEditServlet uses request.getRemoteuser() to determine the remoteShortName for Kerberos authorization, which fails to match when the JournalNode uses its own Kerberos principal (e.g. jn/@).
This can be fixed by using the UserGroupInformation provided by the base DfsServlet class using the getUGI(request, conf) call.
How was this patch tested?
Integration tests were performed against an HA configuration running in Kubernetes, running Java 11. With the patch, exceptions which had previously reported expected Kerberos principals which included an IP address string were eliminated.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?