Boost CLI onLogin pref by proxyClient injection

[smallinsky]

Experimental

What

Make the tsh login function more robust by forwarding proxyClient and leveraging the proxyClient cache. .

• before 4.348 s ± 0.151 s :

  hyperfine --shell zsh 'rm -rf ~/.tsh; tsh login --proxy=teleport.example.com:443 --user=alice'
  
  Benchmark 1: rm -rf ~/.tsh; tsh login --proxy=teleport.example.com:443 --user=alice
    Time (mean ± σ):      4.348 s ±  0.151 s    [User: 0.309 s, System: 0.088 s]
    Range (min … max):    4.093 s …  4.556 s    10 runs
  

  3x Remote Connect To Cluster Calls:
  

• after 2.303 s ± 0.073 s:

  hyperfine --shell zsh 'rm -rf ~/.tsh; tsh login --proxy=teleport.example.com:443 --user=alice'
  
  Benchmark 1: rm -rf ~/.tsh; tsh login --proxy=teleport.example.com:443 --user=alice
    Time (mean ± σ):      2.303 s ±  0.073 s    [User: 0.287 s, System: 0.064 s]
    Range (min … max):    2.226 s …  2.448 s    10 runs
  

  1x Remote Connect To Cluster Call:
  

[smallinsky]

@rosstimothy Let me know what do you think about this solution where a proxyClient is initialised in top flow and then propagated by WithProxyClient func so we can fully levrage the proxy.CurrentCluster() caching.

[rosstimothy]

@rosstimothy Let me know what do you think about this solution where a proxyClient is initialised in top flow and then propagated by WithProxyClient func so we can fully levrage the proxy.CurrentCluster() caching.

Nice! What do you think about making the clients explicit arguments in functions that require connections to a cluster instead of using functional options? Processing the options is a bit verbose, although I suppose a helper method to get the client from the options might eliminate that.

[smallinsky]

What do you think about making the clients explicit arguments in functions that require connections to a cluster instead of using functional options?

I'm not big fan of this approach without doing complete alignment because we will create a lot of vague functions with nil parameter like DoSomthing(ctx, tc, nil)

I suppose a helper method to get the client from the options might eliminate that.

yea, I have updated the code applying this approach in: d3c0f69

[rosstimothy]

What do you think about making the clients explicit arguments in functions that require connections to a cluster instead of using functional options?

I'm not big fan of this approach without doing complete alignment because we will create a lot of vague functions with nil parameter like DoSomthing(ctx, tc, nil)

Yeah I think this only makes sense if we convert all usages so that passing in nil is not allowed. The fact that tc.ConnectToProxy is hidden behind all of these helper methods is part of the reason several commands have latency issues. By explicitly requiring a client to be passed in then there is a contract that enforces callers to connect once and reuse the client.

[rosstimothy]

@smallinsky I took the alternate approach of explicitly requiring an auth.ClientI in #27029 - what do you think?

[smallinsky]

I took the alternate approach of explicitly requiring an auth.ClientI in #27029 - what do you think?

@rosstimothy
I have thought that this approach will require more alignment but looking at diff changes it mostly LGTM. I closing this PR in favour of #27029