[GitHub] [accumulo-proxy] ctubbsii commented on a diff in pull request #58: WIP - Convert accumulo-proxy to single user

[GitBox <gi...@apache.org>]

ctubbsii commented on code in PR #58:
URL: https://github.com/apache/accumulo-proxy/pull/58#discussion_r1048843081


##########
src/main/thrift/proxy.thrift:
##########
@@ -346,7 +337,6 @@ service AccumuloProxy {
   )
 
   void addSplits (
-    1:binary login

Review Comment:
   I think you're probably going to need to keep the `login` field, but use it differently. It is currently used to track authenticated sessions. I think we still need that, so the preferred language client can authenticate to the proxy with some kind of shared secret. The task at hand is to eliminate the need for the preferred language client to instruct the proxy on how to authenticate to Accumulo, because the proxy should be able to authenticate to Accumulo using the client properties file. But, we still want the proxy to not just allow anybody to connect to it and use it. So, the preferred language client should have some way to authenticate to the proxy. For that, this thrift field can be reused to pass that secret shared between the preferred language client and the proxy.



##########
src/main/java/org/apache/accumulo/proxy/ProxyServer.java:
##########
@@ -188,10 +183,7 @@ public void onRemoval(RemovalNotification<UUID,ConditionalWriter> notification)
 
   public ProxyServer(Properties props) {
 
-    @SuppressWarnings("deprecation")
-    org.apache.accumulo.core.client.Instance i = new org.apache.accumulo.core.client.ZooKeeperInstance(
-        ClientConfConverter.toClientConf(props));
-    instance = i;
+    client = Accumulo.newClient().from(props).build();
 
     try {
       String tokenProp = props.getProperty("tokenClass", PasswordToken.class.getName());

Review Comment:
   I don't think you need this check anymore. `Accumulo.newClient()` should be able to set up the `client` using whatever token type it supports. That restriction was in place before because the preferred language client could only support passwords... but now that we're not logging in using the preferred language client, and the proxy is setting up the client on its own, we don't need to restrict it.



##########
src/main/java/org/apache/accumulo/proxy/ProxyServer.java:
##########
@@ -707,41 +682,41 @@ public void setLocalityGroups(ByteBuffer login, String tableName,
           groups.get(groupEntry.getKey()).add(new Text(val));
         }
       }
-      getConnector(login).tableOperations().setLocalityGroups(tableName, groups);
+      client.tableOperations().setLocalityGroups(tableName, groups);
     } catch (Exception e) {
       handleExceptionTNF(e);
     }
   }
 
   @Override
-  public void setTableProperty(ByteBuffer login, String tableName, String property, String value)

Review Comment:
   The shared secret can just be a String. We don't need it to be binary anymore.



-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org