[GitHub] [arrow] lidavidm commented on a change in pull request #7994: Flight auth redesign

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

lidavidm commented on a change in pull request #7994:
URL: https://github.com/apache/arrow/pull/7994#discussion_r472995325



##########
File path: java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightClient.java
##########
@@ -84,18 +81,15 @@
    * Create a Flight client from an allocator and a gRPC channel.
    */
   FlightClient(BufferAllocator incomingAllocator, ManagedChannel channel,
-      List<FlightClientMiddleware.Factory> middleware) {
+      List<FlightClientMiddleware.Factory> middleware, CallCredentials callCredentials) {
     this.allocator = incomingAllocator.newChildAllocator("flight-client", 0, Long.MAX_VALUE);
     this.channel = channel;
 
-    final ClientInterceptor[] interceptors;
-    interceptors = new ClientInterceptor[]{authInterceptor, new ClientInterceptorAdapter(middleware)};
-
     // Create a channel with interceptors pre-applied for DoGet and DoPut
-    this.interceptedChannel = ClientInterceptors.intercept(channel, interceptors);
+    this.interceptedChannel = ClientInterceptors.intercept(channel, new ClientInterceptorAdapter(middleware));
 
-    blockingStub = FlightServiceGrpc.newBlockingStub(interceptedChannel);
-    asyncStub = FlightServiceGrpc.newStub(interceptedChannel);
+    blockingStub = FlightServiceGrpc.newBlockingStub(interceptedChannel).withCallCredentials(callCredentials);

Review comment:
       IMO, for customizing the underlying gRPC stub, I'd rather see this added to `flight-grpc`. For instance, `GrpcCallOption` here could be re-exported from the `flight-grpc` module:
   
   https://github.com/apache/arrow/blob/maint-1.0.x/java/flight/flight-core/src/main/java/org/apache/arrow/flight/CallOptions.java#L59
   
   Then this could be implemented just as a custom call option (+ perhaps a FlightClient change to specify default call options).

##########
File path: format/Flight.proto
##########
@@ -124,11 +124,6 @@ message HandshakeRequest {
    * A defined protocol version
    */
   uint64 protocol_version = 1;
-

Review comment:
       These are format changes and will need a vote.

##########
File path: java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightClient.java
##########
@@ -156,23 +150,12 @@
   }
 
   /**
-   * Authenticates with a username and password.

Review comment:
       I don't see any need to remove existing auth methods.

##########
File path: java/flight/flight-core/src/main/java/org/apache/arrow/flight/FlightServerMiddleware.java
##########
@@ -72,6 +74,10 @@
     }
   }
 
+  default Context onAuthenticationSuccess(Context currentContext) {

Review comment:
       We shouldn't re-export gRPC types in this module. Otherwise, this would just have been `io.grpc.ServerInterceptor`.

##########
File path: java/flight/flight-core/src/main/java/org/apache/arrow/flight/auth/BasicAuthCallCredentials.java
##########
@@ -17,42 +17,39 @@
 
 package org.apache.arrow.flight.auth;
 
-import java.util.Iterator;
+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+import java.util.concurrent.Executor;
 
-import org.apache.arrow.flight.impl.Flight.BasicAuth;
+import io.grpc.CallCredentials;
+import io.grpc.Metadata;
 
 /**
  * A client auth handler that supports username and password.
  */
-public class BasicClientAuthHandler implements ClientAuthHandler {
+public final class BasicAuthCallCredentials extends CallCredentials {

Review comment:
       Again, let's not re-export gRPC types.




----------------------------------------------------------------
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.

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