Migrates the java client authentication to Flight Auth v2 #3423
Conversation
There are also plumbing improvements that allow constructing sessions with specific authentication values, and the sharing of authenticated channels without extraneous duplication of logic. Fixes deephaven#3285
devinrsmith
added
DocumentationNeeded
java-client
ReleaseNotesNeeded
| @@ -33,6 +32,9 @@ interface Builder extends BarrageSessionFactoryBuilder { | |||
|
|
|||
| Builder allocator(@BindsInstance BufferAllocator bufferAllocator); | |||
|
|
|||
| Builder authenticationTypeAndValue( | |||
| @BindsInstance @Nullable @Named("authenticationTypeAndValue") String authenticationTypeAndValue); | |||
There was a problem hiding this comment.
Is this sufficient? Does this allow for bi-directional authentication schemes?
There was a problem hiding this comment.
No.
I think we want a working example we can interact with to come up with a better interface. Either way, the structure in main right now only supports anonymous authentication, and this adds on the ability for one-and-done type auth.
That said, I'm not generally happy w/ the structure of BarrageSubcomponent, FlightSubcomponent, SessionSubcomponent (I know I was the one who made them). We may want to remove some of these layers of abstractions or provide better interfaces for users to hook into.
|
|
||
| public String toAuthenticationTypeAndValue() { | ||
| if (mtls != null && mtls) { | ||
| // todo |
There was a problem hiding this comment.
Is this a lost comment? Or should we add an issue tag?
There was a problem hiding this comment.
Meant to remove. We may eventually want proper string constants somewhere publicly accessible for authentication methods we support out of the box.
| @@ -20,6 +20,10 @@ public class FlightSession implements AutoCloseable { | |||
|
|
|||
| public static FlightSession of(SessionImpl session, BufferAllocator incomingAllocator, | |||
| ManagedChannel channel) { | |||
| // Note: this pattern of FlightClient owning the ManagedChannel does not mesh well with the idea that some | |||
There was a problem hiding this comment.
This comment is golden. Thanks; well said.
| this.serialManager = TableHandleManagerSerial.of(this); | ||
| this.batchManager = TableHandleManagerBatch.of(this, config.mixinStacktrace()); | ||
| this.pingJob = config.executor().scheduleAtFixedRate( | ||
| () -> bearerChannel.config().getConfigurationConstants( |
There was a problem hiding this comment.
Cristian pointed out that when the server was unavailable that the client retried in an excessive repetitive manner. Have you verified that this no longer happens? I believed it would require setting alternate grpc call options.
Also, Colin pointed out that possibly ConfigService might run on another host (say, for a cluster of instances) - should we add a ping grpc call directly to the SessionService in the event that we're hitting envoy and being routed to different hosts?
There was a problem hiding this comment.
I've taken out that more "complicated" retry logic, and figured that using a fixed job that "pings" multiple times during the timeout interval is "good enough". (I don't see much value in some sort of backoff schedule... either we successfully reauth before the timeout interval, or we don't.)
I'd very much be in favor of a simple no-op "ping" RPC in SessionService; although, I think @niloc132 might have opinions? The general advice might be "use Handshake", but the code as written right now doesn't depend on Flight. Right now I'm essentially (ab)using ConfigService#getConfigurationConstants since I know the user is authorized for it.
|
Labels indicate documentation is required. Issues for documentation have been opened: How-to: https://github.com/deephaven/deephaven.io/issues/2223 |
There are also plumbing improvements that allow constructing sessions with specific authentication values, and the sharing of authenticated channels without extraneous duplication of logic.
Fixes #3285