Move pure ssh client and server logic to proxy package and test it#3569
Move pure ssh client and server logic to proxy package and test it#3569
Conversation
298 failing tests:
|
3c980ed to
ac1e3ed
Compare
ac1e3ed to
984311b
Compare
bf3373e to
2222cb9
Compare
984311b to
e1d5c03
Compare
e1d5c03 to
c8a8bfa
Compare
c8a8bfa to
6256801
Compare
2222cb9 to
94c2ae2
Compare
6256801 to
d8985aa
Compare
d8985aa to
8769376
Compare
94c2ae2 to
8f69376
Compare
8769376 to
8353484
Compare
8353484 to
6cb8f6a
Compare
6cb8f6a to
42a8a3e
Compare
09dfb3e to
2f6dc5a
Compare
2f6dc5a to
5c89e05
Compare
5c89e05 to
772db5e
Compare
772db5e to
f351e0e
Compare
| } | ||
| } | ||
|
|
||
| func TestHandover(t *testing.T) { |
There was a problem hiding this comment.
This is a potentially flaky test, and testing/synctest can't be used, as we rely on external IO (real http server, websockets, and cat command).
I've improved the logic itself to rely less on timeouts - only use them to reduce the amount of messages the client is sending, to avoid test buffer overflows. The asserts are done by waiting on the client.onWrite channel, so there shouldn't be timing issues.
Tested it with a few go test -count=1000 -parallel=500 + with the -race flag.
| f"--cluster={ctx.clusterId}", | ||
| f"--secrets-scope-name={secrets_scope}", | ||
| f"--client-key-name={public_key_secret_name}", | ||
| f"--keys-secret-scope-name={secrets_scope}", |
There was a problem hiding this comment.
This rename seems to be a big part of this PR but it's not mentioned in title and description. Is it required here, what prompted it? Perhaps it deserves its own PR?
There was a problem hiding this comment.
That's based on the @pietern comments in the previous PR: #3544 (comment)
The main goal is to be more explicit with the argument names to avoid any possible confusion (even though the server command is hidden and not supposed to be used by anyone except us)
There was a problem hiding this comment.
The current ones are not yet optimal, IMO.
E.g. mix of authorized key name and public key secret name. Is it an authorized key or public key? If we use a single secret scope for everything (do we have more than keys?), we can call it --secret-scope-name. If we need a public key for the client, use --client-public-key-secret-name. It's OK to be verbose because the args are only used internally.
pietern
left a comment
There was a problem hiding this comment.
Please take a look at the remaining comments.
No need to block on the flag names.
| f"--cluster={ctx.clusterId}", | ||
| f"--secrets-scope-name={secrets_scope}", | ||
| f"--client-key-name={public_key_secret_name}", | ||
| f"--keys-secret-scope-name={secrets_scope}", |
There was a problem hiding this comment.
The current ones are not yet optimal, IMO.
E.g. mix of authorized key name and public key secret name. Is it an authorized key or public key? If we use a single secret scope for everything (do we have more than keys?), we can call it --secret-scope-name. If we need a public key for the client, use --client-public-key-secret-name. It's OK to be verbose because the args are only used internally.
## Changes - Add a cluster selector UI to the `ssh setup` command. We don't filter or check the clusters during that stage, as it's done later by the `validateClusterAccess`. Cluster option is no longer required for the `setup` command. A few customers asked for this already. - Add an `--auto-start-cluster` flag to both `ssh setup` and `ssh connect` commands, which defaults to true. Useful for PyCharm users, where the IDE checks every host in the config to see the if the connection can be established, starting all the clusters. - Change a few `client.go` functions to accept ClientOptions instead of a long list of separate arguments Based on #3569 ## Tests No unit tests, integration tests will be in the follow ups.
Changes
Based on #3544
Move pure client and server logic to the proxy package and test it.
Now
proxyConnectionis completely private, andproxypackage exposes only 2 public functions:RunClientProxyfunctionNewProxyServerconstructorMeanwhile the internal logic doesn't depend on any ssh stuff and can be unit tested.
internal/clientandinternal/serverpackages are now much smaller and contain mainly ssh/jobs/cluster glue.Also adding
bench.shuntil we have proper e2e tests.Why
Tests
Tests
Unit and manual tests