ctubbsii commented on code in PR #59:
URL: https://github.com/apache/accumulo-proxy/pull/59#discussion_r1073223649
##########
src/test/java/org/apache/accumulo/proxy/ProxyServerTest.java:
##########
@@ -69,14 +68,14 @@ public void
updateAndFlushClosesWriterOnExceptionFromAddCells() throws Exception
@Test
public void updateAndFlushClosesWriterOnExceptionFromFlush() throws
Exception {
ProxyServer server = EasyMock.createMockBuilder(ProxyServer.class)
- .addMockedMethod("getWriter", ByteBuffer.class, String.class,
WriterOptions.class)
+ .addMockedMethod("getWriter", String.class, String.class,
WriterOptions.class)
.addMockedMethod("addCellsToWriter", Map.class,
BatchWriterPlusProblem.class).createMock();
BatchWriter writer = EasyMock.createMock(BatchWriter.class);
BatchWriterPlusProblem bwpe = new BatchWriterPlusProblem();
bwpe.writer = writer;
MutationsRejectedException mre =
EasyMock.createMock(MutationsRejectedException.class);
- final ByteBuffer login = ByteBuffer.wrap("my_login".getBytes(UTF_8));
+ final String login = "my_login";
Review Comment:
The change to use a shared proxy secret instead of the previous login
mechanism is okay, but it left a bunch of tests with confusing variable names,
which make the tests hard to follow.
##########
src/test/java/org/apache/accumulo/proxy/its/ProxyDurabilityIT.java:
##########
@@ -98,6 +98,10 @@ public void testDurability() throws Exception {
proxyProps.put("tokenClass", PasswordToken.class.getName());
proxyProps.putAll(getClientProperties());
+ String login = ROOT_PASSWORD;
+
+ proxyProps.put("secret", login);
+
Review Comment:
"secret" and "login" are significantly overused as variables, constants,
etc. It's very hard to follow the tests, when the root password is being stored
in a variable called "login" that is actually the shared secret for
authenticating to the proxy. There needs to be greater clarity between all of
these, with better variable names, and better test values, and less reuse
between them... I suspect some tests "just work" accidentally, because we used
"secret" more than once, so even though we're probably passing the wrong
variable, it still passes, because both values are "secret". Everything should
have unique values.
##########
src/test/java/org/apache/accumulo/proxy/its/SimpleProxyBase.java:
##########
@@ -1232,8 +1224,8 @@ public void testSiteConfiguration() throws Exception {
// get something we know is in the site config
MiniAccumuloClusterImpl cluster = SharedMiniClusterBase.getCluster();
Map<String,String> cfg = client.getSiteConfiguration(creds);
- assertTrue(cfg.get("instance.dfs.dir")
- .startsWith(cluster.getConfig().getAccumuloDir().getAbsolutePath()));
+ assertEquals(new File(new
URI(cfg.get("instance.volumes"))).getCanonicalPath(),
+ cluster.getConfig().getAccumuloDir().getCanonicalPath());
Review Comment:
I fixed this test. It should be good now.
##########
src/test/java/org/apache/accumulo/proxy/its/SimpleProxyBase.java:
##########
@@ -1463,34 +1455,15 @@ public void userManagement() throws Exception {
client.changeLocalUserPassword(creds, user, password);
assertTrue(client.authenticateUser(creds, user,
s2pp(ByteBufferUtil.toString(password))));
}
-
- if (isKerberosEnabled()) {
Review Comment:
I'm not really sure we need any of these kerberos checks. If the
AccumuloClient is correctly created from props, that client should
transparently handle whether it uses a PasswordToken or a KerberosToken in the
user's client properties file.
There are a lot of places in the proxy where we have special handling for
kerberos... especially for the tests. I'm not actually sure if the proxy
supports, or ever supported, running as a kerberos-enabled service itself. If
that's something we want to support, it will probably require substantial
follow-on work. For now, it might just be useful to know whether the old proxy
did or did not support that.
##########
src/test/java/org/apache/accumulo/proxy/its/SimpleProxyBase.java:
##########
@@ -2640,9 +2611,7 @@ private ByteBuffer s2bb(String cf) {
}
private Map<String,String> s2pp(String cf) {
- Map<String,String> toRet = new TreeMap<>();
- toRet.put("password", cf);
- return toRet;
+ return Map.of("password", cf);
Review Comment:
This was a trivial thing I changed. I have no idea what the `s2pp` method
names were supposed to be... maybe "string to password property"?
##########
src/test/java/org/apache/accumulo/proxy/its/SimpleProxyBase.java:
##########
@@ -162,13 +163,12 @@ protected Duration defaultTimeout() {
private TestProxyClient proxyClient;
private org.apache.accumulo.proxy.thrift.AccumuloProxy.Client client;
- private static final Map<String,String> properties = new HashMap<>();
private static String hostname;
private static String proxyPrimary;
private static String clientPrincipal;
private static File clientKeytab;
- private ByteBuffer creds = null;
+ private static String creds;
Review Comment:
`creds` is another one whose name is overused and needs more clarity. In
this case, it's storing the shared secret for the proxy.
##########
src/test/java/org/apache/accumulo/proxy/its/SimpleProxyBase.java:
##########
@@ -1463,34 +1455,15 @@ public void userManagement() throws Exception {
client.changeLocalUserPassword(creds, user, password);
assertTrue(client.authenticateUser(creds, user,
s2pp(ByteBufferUtil.toString(password))));
}
-
- if (isKerberosEnabled()) {
- UserGroupInformation.loginUserFromKeytab(otherClient.getPrincipal(),
- otherClient.getKeytab().getAbsolutePath());
- final UserGroupInformation ugi = UserGroupInformation.getCurrentUser();
- // Re-login in and make a new connection. Can't use the previous one
-
- TestProxyClient otherProxyClient = null;
- try {
- otherProxyClient = new TestProxyClient(hostname, proxyPort, factory,
proxyPrimary, ugi);
- otherProxyClient.proxy().login(user, Collections.emptyMap());
- } finally {
- if (otherProxyClient != null) {
- otherProxyClient.close();
- }
- }
- } else {
- // check login with new password
- client.login(user, s2pp(ByteBufferUtil.toString(password)));
- }
}
+ @Disabled("This test needs to be reworked, because it tries to switch users")
Review Comment:
The worst tests, and the ones that need the most work, are the ones that try
to create and switch users. I disabled three of those.
##########
src/test/java/org/apache/accumulo/proxy/its/SimpleProxyBase.java:
##########
@@ -1428,7 +1420,7 @@ public void userAuthentication() throws Exception {
// check password
assertTrue(
client.authenticateUser(creds, "root",
s2pp(SharedMiniClusterBase.getRootPassword())));
- assertFalse(client.authenticateUser(creds, "root", s2pp("")));
+ assertFalse(client.authenticateUser(creds, "otheruser", s2pp("")));
Review Comment:
This test was broken, but it's not clear why. This might actually require
further investigation in Accumulo itself. It seemed to return true, regardless
of the password, if the root user tried to authenticate themselves. A good test
here might be to create a new user, and try to authenticate that new user, with
their new password and a bad password, instead of trying to test with the root
user, which is also the current user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
