Report ssh command failures to the surface (#1959)
The ssh debug commands told the user when something went wrong but
never told the surface that ran them, so `ssh host cmd` always exited 0
and a few failures were not reported at all. A failed command now
returns its error, a failure the user has already heard about is a
commands.Failure, and the ssh exec path exits 1 for either. The command
bodies also lose their ssh prefixes and the duplication they carried
over from ssh.go; both goldens pin that nothing else changed.
- Dispatch returns the callback's error, or shlex's; the ssh session
logs it at debug, and not at all for a Failure the user already saw
- commands.Fail writes a message and returns it as a Failure in one step;
an unknown command, `help` for a command that is not there, a failed
reload and an unbalanced quote are failures
- ssh exec exit-status is 1 for a command, flag or failure error, 0
otherwise; the e2e golden records the exit status for every case
- Clone rebinds help to the copy, so an ssh session's help lists logout
and `help logout` and `logout -h` answer; sshd.LogoutCommand is the
session's logout, exported for tests
- "did not understand" names the first word only, the one looked up;
DispatchArgs takes just the tokens
- log-format writes its refusal to the user, as log-level does
- print-relays emits the relay error as a string and omits the key when
there is none, where it printed {} or "Error":null
- list-hostmap and list-lighthouse-addrmap -json return the encode
error; print-cert reports a certificate that will not marshal
- a flags type that is not the one registered is an internal error, not
a silent success
- command functions and flag types are <name>Command and <name>Flags;
parseVpnAddr and listHostMapFlagSet replace the repeated parse block
and flag set; close-tunnel sends through sendCloseTunnel
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Migrate commands from ssh into a transport neutral package
- add golden tests validating that output remains unchanged
- move commands to commands.go
Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
These new helpers make the code a lot cleaner. I confirmed that the
simple helpers like `atomic.Int64` don't add any extra overhead as they
get inlined by the compiler. `atomic.Pointer` adds an extra method call
as it no longer gets inlined, but we aren't using these on the hot path
so it is probably okay.
This change fixes all of the known data races that `make smoke-docker-race` finds, except for one.
Most of these races are around the handshake phase for a hostinfo, so we add a RWLock to the hostinfo and Lock during each of the handshake stages.
Some of the other races are around consistently using `atomic` around the `messageCounter` field. To make this harder to mess up, I have renamed the field to `atomicMessageCounter` (I also removed the unnecessary extra pointer deference as we can just point directly to the struct field).
The last remaining data race is around reading `ConnectionInfo.ready`, which is a boolean that is only written to once when the handshake has finished. Due to it being in the hot path for packets and the rare case that this could actually be an issue, holding off on fixing that one for now.
here is the results of `make smoke-docker-race`:
before:
lighthouse1: Found 2 data race(s)
host2: Found 36 data race(s)
host3: Found 17 data race(s)
host4: Found 31 data race(s)
after:
host2: Found 1 data race(s)
host4: Found 1 data race(s)
Fixes: #147Fixes: #226Fixes: #283Fixes: #316
