From e472ee2aa580f9af3d95ff0267eef69ef5e3a7d5 Mon Sep 17 00:00:00 2001 From: Peter Bacsko Date: Tue, 9 Mar 2021 11:28:24 +0100 Subject: [PATCH] YARN-10676. Improve code quality in TestTimelineAuthenticationFilterForV1. Contributed by Szilard Nemeth. --- ...TestTimelineAuthenticationFilterForV1.java | 98 ++++++++++++------- 1 file changed, 62 insertions(+), 36 deletions(-) diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/security/TestTimelineAuthenticationFilterForV1.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/security/TestTimelineAuthenticationFilterForV1.java index 0e1310eb696..cf2db2f8c6f 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/security/TestTimelineAuthenticationFilterForV1.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-applicationhistoryservice/src/test/java/org/apache/hadoop/yarn/server/timeline/security/TestTimelineAuthenticationFilterForV1.java @@ -18,8 +18,6 @@ package org.apache.hadoop.yarn.server.timeline.security; -import static org.junit.Assert.assertTrue; - import java.io.File; import java.security.PrivilegedExceptionAction; import java.util.Arrays; @@ -49,12 +47,16 @@ import org.apache.hadoop.yarn.server.applicationhistoryservice.ApplicationHistor import org.apache.hadoop.yarn.server.timeline.MemoryTimelineStore; import org.apache.hadoop.yarn.server.timeline.TimelineStore; import static org.apache.hadoop.yarn.conf.YarnConfiguration.TIMELINE_HTTP_AUTH_PREFIX; +import static org.junit.Assert.fail; + import org.junit.AfterClass; import org.junit.Assert; import org.junit.BeforeClass; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * Test cases for authentication via TimelineAuthenticationFilter while @@ -62,17 +64,20 @@ import org.junit.runners.Parameterized; */ @RunWith(Parameterized.class) public class TestTimelineAuthenticationFilterForV1 { + private static final Logger LOG = + LoggerFactory.getLogger(TestTimelineAuthenticationFilterForV1.class); private static final String FOO_USER = "foo"; private static final String BAR_USER = "bar"; private static final String HTTP_USER = "HTTP"; + private static final String PRINCIPAL = HTTP_USER + "/localhost"; private static final File TEST_ROOT_DIR = new File( System.getProperty("test.build.dir", "target/test-dir"), TestTimelineAuthenticationFilterForV1.class.getName() + "-root"); - private static File httpSpnegoKeytabFile = new File( + private static final File httpSpnegoKeytabFile = new File( KerberosTestUtils.getKeytabFile()); - private static String httpSpnegoPrincipal = + private static final String httpSpnegoPrincipal = KerberosTestUtils.getServerPrincipal(); private static final String BASEDIR = System.getProperty("test.build.dir", "target/test-dir") + "/" @@ -100,16 +105,16 @@ public class TestTimelineAuthenticationFilterForV1 { testMiniKDC = new MiniKdc(MiniKdc.createConf(), TEST_ROOT_DIR); testMiniKDC.start(); testMiniKDC.createPrincipal( - httpSpnegoKeytabFile, HTTP_USER + "/localhost"); + httpSpnegoKeytabFile, PRINCIPAL); } catch (Exception e) { - assertTrue("Couldn't setup MiniKDC", false); + LOG.error("Failed to setup MiniKDC", e); + fail("Couldn't setup MiniKDC"); } try { testTimelineServer = new ApplicationHistoryServer(); conf = new Configuration(false); - conf.setStrings(TIMELINE_HTTP_AUTH_PREFIX + "type", - "kerberos"); + conf.setStrings(TIMELINE_HTTP_AUTH_PREFIX + "type", "kerberos"); conf.set(TIMELINE_HTTP_AUTH_PREFIX + KerberosAuthenticationHandler.PRINCIPAL, httpSpnegoPrincipal); conf.set(TIMELINE_HTTP_AUTH_PREFIX + @@ -150,8 +155,8 @@ public class TestTimelineAuthenticationFilterForV1 { testTimelineServer.init(conf); testTimelineServer.start(); } catch (Exception e) { - e.printStackTrace(); - assertTrue("Couldn't setup TimelineServer", false); + LOG.error("Failed to setup TimelineServer", e); + fail("Couldn't setup TimelineServer"); } } @@ -181,7 +186,7 @@ public class TestTimelineAuthenticationFilterForV1 { @Test public void testPutTimelineEntities() throws Exception { - KerberosTestUtils.doAs(HTTP_USER + "/localhost", new Callable() { + KerberosTestUtils.doAs(PRINCIPAL, new Callable() { @Override public Void call() throws Exception { TimelineClient client = createTimelineClientForUGI(); @@ -191,11 +196,16 @@ public class TestTimelineAuthenticationFilterForV1 { entityToStore.setEntityId("entity1"); entityToStore.setStartTime(0L); TimelinePutResponse putResponse = client.putEntities(entityToStore); - Assert.assertEquals(0, putResponse.getErrors().size()); + if (putResponse.getErrors().size() > 0) { + LOG.error("putResponse errors: {}", putResponse.getErrors()); + } + Assert.assertTrue("There were some errors in the putResponse", + putResponse.getErrors().isEmpty()); TimelineEntity entityToRead = testTimelineServer.getTimelineStore().getEntity("entity1", TestTimelineAuthenticationFilterForV1.class.getName(), null); - Assert.assertNotNull(entityToRead); + Assert.assertNotNull("Timeline entity should not be null", + entityToRead); return null; } }); @@ -203,7 +213,7 @@ public class TestTimelineAuthenticationFilterForV1 { @Test public void testPutDomains() throws Exception { - KerberosTestUtils.doAs(HTTP_USER + "/localhost", new Callable() { + KerberosTestUtils.doAs(PRINCIPAL, new Callable() { @Override public Void call() throws Exception { TimelineClient client = createTimelineClientForUGI(); @@ -216,7 +226,8 @@ public class TestTimelineAuthenticationFilterForV1 { TimelineDomain domainToRead = testTimelineServer.getTimelineStore().getDomain( TestTimelineAuthenticationFilterForV1.class.getName()); - Assert.assertNotNull(domainToRead); + Assert.assertNotNull("Timeline domain should not be null", + domainToRead); return null; } }); @@ -225,7 +236,7 @@ public class TestTimelineAuthenticationFilterForV1 { @Test public void testDelegationTokenOperations() throws Exception { TimelineClient httpUserClient = - KerberosTestUtils.doAs(HTTP_USER + "/localhost", + KerberosTestUtils.doAs(PRINCIPAL, new Callable() { @Override public TimelineClient call() throws Exception { @@ -233,43 +244,51 @@ public class TestTimelineAuthenticationFilterForV1 { } }); UserGroupInformation httpUser = - KerberosTestUtils.doAs(HTTP_USER + "/localhost", + KerberosTestUtils.doAs(PRINCIPAL, new Callable() { @Override public UserGroupInformation call() throws Exception { return UserGroupInformation.getCurrentUser(); } }); + // Let HTTP user to get the delegation for itself Token token = httpUserClient.getDelegationToken(httpUser.getShortUserName()); - Assert.assertNotNull(token); + Assert.assertNotNull("Delegation token should not be null", token); TimelineDelegationTokenIdentifier tDT = token.decodeIdentifier(); - Assert.assertNotNull(tDT); - Assert.assertEquals(new Text(HTTP_USER), tDT.getOwner()); + Assert.assertNotNull("Delegation token identifier should not be null", + tDT); + Assert.assertEquals("Owner of delegation token identifier does not match", + new Text(HTTP_USER), tDT.getOwner()); // Renew token - Assert.assertFalse(token.getService().toString().isEmpty()); + Assert.assertFalse("Service field of token should not be empty", + token.getService().toString().isEmpty()); // Renew the token from the token service address long renewTime1 = httpUserClient.renewDelegationToken(token); Thread.sleep(100); token.setService(new Text()); - Assert.assertTrue(token.getService().toString().isEmpty()); - // If the token service address is not avaiable, it still can be renewed + Assert.assertTrue("Service field of token should be empty", + token.getService().toString().isEmpty()); + // If the token service address is not available, it still can be renewed // from the configured address long renewTime2 = httpUserClient.renewDelegationToken(token); - Assert.assertTrue(renewTime1 < renewTime2); + Assert.assertTrue("renewTime2 should be later than renewTime1", + renewTime1 < renewTime2); // Cancel token - Assert.assertTrue(token.getService().toString().isEmpty()); - // If the token service address is not avaiable, it still can be canceled + Assert.assertTrue("Service field of token should be empty", + token.getService().toString().isEmpty()); + // If the token service address is not available, it still can be canceled // from the configured address httpUserClient.cancelDelegationToken(token); // Renew should not be successful because the token is canceled try { httpUserClient.renewDelegationToken(token); - Assert.fail(); + Assert.fail("Renew of delegation token should not be successful"); } catch (Exception e) { + LOG.info("Exception while renewing delegation token", e); Assert.assertTrue(e.getMessage().contains( "Renewal request for unknown token")); } @@ -280,33 +299,39 @@ public class TestTimelineAuthenticationFilterForV1 { TimelineClient fooUserClient = fooUgi.doAs( new PrivilegedExceptionAction() { @Override - public TimelineClient run() throws Exception { + public TimelineClient run() { return createTimelineClientForUGI(); } }); token = fooUserClient.getDelegationToken(httpUser.getShortUserName()); - Assert.assertNotNull(token); + Assert.assertNotNull("Delegation token should not be null", token); tDT = token.decodeIdentifier(); - Assert.assertNotNull(tDT); - Assert.assertEquals(new Text(FOO_USER), tDT.getOwner()); - Assert.assertEquals(new Text(HTTP_USER), tDT.getRealUser()); + Assert.assertNotNull("Delegation token identifier should not be null", + tDT); + Assert.assertEquals("Owner of delegation token is not the expected", + new Text(FOO_USER), tDT.getOwner()); + Assert.assertEquals("Real user of delegation token is not the expected", + new Text(HTTP_USER), tDT.getRealUser()); // Renew token as the renewer final Token tokenToRenew = token; renewTime1 = httpUserClient.renewDelegationToken(tokenToRenew); renewTime2 = httpUserClient.renewDelegationToken(tokenToRenew); - Assert.assertTrue(renewTime1 < renewTime2); + Assert.assertTrue("renewTime2 should be later than renewTime1", + renewTime1 < renewTime2); // Cancel token - Assert.assertFalse(tokenToRenew.getService().toString().isEmpty()); + Assert.assertFalse("Service field of token should not be empty", + tokenToRenew.getService().toString().isEmpty()); // Cancel the token from the token service address fooUserClient.cancelDelegationToken(tokenToRenew); // Renew should not be successful because the token is canceled try { httpUserClient.renewDelegationToken(tokenToRenew); - Assert.fail(); + Assert.fail("Renew of delegation token should not be successful"); } catch (Exception e) { + LOG.info("Exception while renewing delegation token", e); Assert.assertTrue( e.getMessage().contains("Renewal request for unknown token")); } @@ -324,8 +349,9 @@ public class TestTimelineAuthenticationFilterForV1 { try { barUserClient.getDelegationToken(httpUser.getShortUserName()); - Assert.fail(); + Assert.fail("Retrieval of delegation token should not be successful"); } catch (Exception e) { + LOG.info("Exception while retrieving delegation token", e); Assert.assertTrue(e.getCause() instanceof AuthorizationException || e.getCause() instanceof AuthenticationException); }