-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-28110 Align TestShadeSaslAuthenticationProvider between differe… #5434
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,18 +17,23 @@ | |
| */ | ||
| package org.apache.hadoop.hbase.security.provider.example; | ||
|
|
||
| import static org.junit.Assert.assertEquals; | ||
| import static org.junit.Assert.assertFalse; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertThrows; | ||
| import static org.junit.Assert.assertTrue; | ||
| import static org.junit.Assert.fail; | ||
|
|
||
| import java.io.BufferedWriter; | ||
| import java.io.File; | ||
| import java.io.IOException; | ||
| import java.io.OutputStreamWriter; | ||
| import java.io.PrintWriter; | ||
| import java.io.StringWriter; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.security.PrivilegedExceptionAction; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Map.Entry; | ||
| import org.apache.hadoop.conf.Configuration; | ||
|
|
@@ -61,8 +66,11 @@ | |
| import org.apache.hadoop.hbase.testclassification.SecurityTests; | ||
| import org.apache.hadoop.hbase.util.Bytes; | ||
| import org.apache.hadoop.hbase.util.CommonFSUtils; | ||
| import org.apache.hadoop.hbase.util.Pair; | ||
| import org.apache.hadoop.ipc.RemoteException; | ||
| import org.apache.hadoop.minikdc.MiniKdc; | ||
| import org.apache.hadoop.security.UserGroupInformation; | ||
| import org.apache.hadoop.security.token.SecretManager.InvalidToken; | ||
| import org.junit.AfterClass; | ||
| import org.junit.Before; | ||
| import org.junit.BeforeClass; | ||
|
|
@@ -71,9 +79,15 @@ | |
| import org.junit.Test; | ||
| import org.junit.experimental.categories.Category; | ||
| import org.junit.rules.TestName; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import org.apache.hbase.thirdparty.com.google.common.base.Throwables; | ||
|
|
||
| @Category({ MediumTests.class, SecurityTests.class }) | ||
| public class TestShadeSaslAuthenticationProvider { | ||
| private static final Logger LOG = | ||
| LoggerFactory.getLogger(TestShadeSaslAuthenticationProvider.class); | ||
|
|
||
| @ClassRule | ||
| public static final HBaseClassTestRule CLASS_RULE = | ||
|
|
@@ -219,28 +233,79 @@ public Void run() throws Exception { | |
|
|
||
| @Test | ||
| public void testNegativeAuthentication() throws Exception { | ||
| // Validate that we can read that record back out as the user with our custom auth'n | ||
| final Configuration clientConf = new Configuration(CONF); | ||
| clientConf.setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER, 3); | ||
| try (Connection conn = ConnectionFactory.createConnection(clientConf)) { | ||
| UserGroupInformation user1 = | ||
| UserGroupInformation.createUserForTesting("user1", new String[0]); | ||
| user1.addToken( | ||
| ShadeClientTokenUtil.obtainToken(conn, "user1", "not a real password".toCharArray())); | ||
| // Server will close the connection directly once auth failed, so at client side, we do not | ||
| // know what is the real problem so we will keep retrying, until reached the max retry times | ||
| // limitation | ||
| assertThrows("Should not successfully authenticate with HBase", | ||
| RetriesExhaustedException.class, () -> user1.doAs(new PrivilegedExceptionAction<Void>() { | ||
| List<Pair<String, Class<? extends Exception>>> params = new ArrayList<>(); | ||
| // ZK based connection will fail on the master RPC | ||
| params.add(new Pair<String, Class<? extends Exception>>( | ||
| // ZKConnectionRegistry is package-private | ||
| HConstants.ZK_CONNECTION_REGISTRY_CLASS, RetriesExhaustedException.class)); | ||
|
|
||
| params.forEach((pair) -> { | ||
| LOG.info("Running negative authentication test for client registry {}, expecting {}", | ||
| pair.getFirst(), pair.getSecond().getName()); | ||
| // Validate that we can read that record back out as the user with our custom auth'n | ||
| final Configuration clientConf = new Configuration(CONF); | ||
| clientConf.setInt(HConstants.HBASE_CLIENT_RETRIES_NUMBER, 3); | ||
| clientConf.set(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY, pair.getFirst()); | ||
| try (Connection conn = ConnectionFactory.createConnection(clientConf)) { | ||
| UserGroupInformation user1 = | ||
| UserGroupInformation.createUserForTesting("user1", new String[0]); | ||
| user1.addToken( | ||
| ShadeClientTokenUtil.obtainToken(conn, "user1", "not a real password".toCharArray())); | ||
|
|
||
| LOG.info("Executing request to HBase Master which should fail"); | ||
| user1.doAs(new PrivilegedExceptionAction<Void>() { | ||
| @Override | ||
| public Void run() throws Exception { | ||
| try (Connection conn = ConnectionFactory.createConnection(clientConf);) { | ||
| conn.getAdmin().listTableDescriptors(); | ||
| fail("Should not successfully authenticate with HBase"); | ||
| } catch (Exception e) { | ||
| LOG.info("Caught exception in negative Master connectivity test", e); | ||
| assertEquals("Found unexpected exception", pair.getSecond(), e.getClass()); | ||
| validateRootCause(Throwables.getRootCause(e)); | ||
| } | ||
| return null; | ||
| } | ||
| }); | ||
|
|
||
| LOG.info("Executing request to HBase RegionServer which should fail"); | ||
| user1.doAs(new PrivilegedExceptionAction<Void>() { | ||
| @Override | ||
| public Void run() throws Exception { | ||
| try (Connection conn = ConnectionFactory.createConnection(clientConf); | ||
| Table t = conn.getTable(tableName)) { | ||
| t.get(new Get(Bytes.toBytes("r1"))); | ||
| return null; | ||
| fail("Should not successfully authenticate with HBase"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ditto. |
||
| } catch (Exception e) { | ||
| LOG.info("Caught exception in negative RegionServer connectivity test", e); | ||
| assertEquals("Found unexpected exception", pair.getSecond(), e.getClass()); | ||
| validateRootCause(Throwables.getRootCause(e)); | ||
| } | ||
| return null; | ||
| } | ||
| })); | ||
| }); | ||
| } catch (InterruptedException e) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just throw these exceptions out? We are in a UT... |
||
| LOG.error("Caught interrupted exception", e); | ||
| Thread.currentThread().interrupt(); | ||
| return; | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| void validateRootCause(Throwable rootCause) { | ||
| LOG.info("Root cause was", rootCause); | ||
| if (rootCause instanceof RemoteException) { | ||
| RemoteException re = (RemoteException) rootCause; | ||
| IOException actualException = re.unwrapRemoteException(); | ||
| assertEquals(InvalidToken.class, actualException.getClass()); | ||
| } else { | ||
| StringWriter writer = new StringWriter(); | ||
| rootCause.printStackTrace(new PrintWriter(writer)); | ||
| String text = writer.toString(); | ||
| assertTrue("Message did not contain expected text", | ||
| text.contains("Connection reset by peer")); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assertion is different between master and branch-2. For master we get: Upon digging found the same issue was found and fixed in HBASE-23881. Can backport as part of HBASE-24179. Or as another commit here. Let me know how we can proceed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can ping @joshelser to see if he still have time to do the backport, if not I think we can do it by ourselves.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also have a local backport patch for the HBASE-23881 with which expected error is thrown. But not really sure if the fix is correct/complete. So let's wait and see if he can take up. |
||
| } | ||
| } | ||
| } | ||
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.
Better use assertThrows 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.
Sure this patch just syncs code changes with master as is. Could we:
Please, let me know how you want me to proceed.
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.
ping @Apache9, let me know how to proceed
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 suggest we backport HBASE-23881 first?
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.
Sure I felt it would be better if these tests are in place for a easier test for backport. With backport we could fix the assert to be as expected. But yes let me do it other way around.
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.
OK. For assertThrows, I think we can file another issue to fix it. Just aligning the tests for all branches first is fine for me.
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.
Cool, let me create another JIRA to remove usage of assert throws. So that this JIRA can go in 1st and the new one fixing review comments across all branches next.