Go client authentication support #3584
Conversation
chipkent
added
DocumentationNeeded
arrow
enhancement
authentication
ReleaseNotesNeeded
| ticker := time.NewTicker(timeout) | ||
|
|
||
| go func() { | ||
| for { |
There was a problem hiding this comment.
In C++ we did things a little differently. You may or may not decide they are important enough to do.
Cristian and I were quite worried about the case where a message might be lost, dropped, or massively delayed. Sending handshakes at a fixed interval, as you do here, is only reliable if they are all delivered, and delivered in a timely fashion.
To try to be more robust about this, we used the following approach instead.
- We snoop all request/response traffic and we notice changes in auth token as they happen. This means when there is other traffic we don't bother sending a handshake at all, because we were able to snoop the token from some other response. We only do a handshake if there has been a
timeout-sized period of no traffic at all. - The code basically has two states: normal (I have received some kind of response in the last timeout period) and handshaking (I've not received any kind of response in the last timeout period). When it enters the "handshaking" state, the code sends handshakes pretty aggressively (like once every 5 seconds). This allows the token refresh thing to be somewhat robust against network freezes or connection drops. But then, any response from the server (whether a normal response or a handshake response) tends to put the code back into the "normal" state, so in the common case you only send one handshake and then drop back to the normal state.
- In order to account for the vagaries of round trip time and connection drops, the code keeps track of timeouts based on the local time when a (request or handshake) was sent rather than when a response was received. For example, if I sent a (request or handshake) at 12:00:00 and I receive a response at 12:00:20, I will do my timeout calculations based on when I sent the request, not when I received the response. In other words if the timeout is 5 minutes, I will consider going to handshake mode at 12:05:00, not 12:05:20.
Basically the C++ code follows this kind of pseudocode:
At any request
Note current local time
send the request
At any response to a request or a handshake
note the new auth header (maybe don't bother copying it if it hasn't changed)
reset the `handshake deadline` to the (time of the corresponding request) plus 5 minutes
The handshake thread:
forever:
sleep until handshake deadline
if current time < handshake deadline # deadline has advanced in the meantime
continue loop
note current handshake request time
send handshake message asynchronously
set handshake deadline to now + 5 seconds
# the above is pessimistic, because the response to this handshake message,
# if received, will undergo standard auth header and deadline processing and
# will advance the deadline forward ~5 minutes
end forever
There was a problem hiding this comment.
@jcferretti proposed (and I agree) that we should defer this work to a future PR.
| func GetAuth() string { | ||
| auth := os.Getenv("DH_AUTH") | ||
| if auth == "" { | ||
| return "Anonymous" |
There was a problem hiding this comment.
Should this be Constants.DefaultAuth (or whatever) rather than the literal string "Anonymous"?
There was a problem hiding this comment.
I don't think this should be using the constant. This is set to match the DH instance that is spun up to test against. We have no guarantee that the instance will be using the default connection mechanism, so I'm not completely comfortable linking them.
There was a problem hiding this comment.
My opinion is weak on this, so if you think it should be changed, I will.
There was a problem hiding this comment.
I'm not deeply invested in either choice. I assumed it was an oversight. If not, that's fine.
|
Labels indicate documentation is required. Issues for documentation have been opened: How-to: https://github.com/deephaven/deephaven.io/issues/2337 |
|
This PR will break example Go clients, and these clients are not being automatically run in our docs tests. |
Support authentication in the go client.
Resolves #3523