fix improper remote-exec timeout and communicator error handling

[jbardin]

The remote.Cmd struct could not convey any transport related error to
the caller, meaning that interrupted commands would show that they
succeeded.

Change Cmd.SetExited to accept an exit status, as well as an error to
store for the caller. Make the status and error fields internal,
require serialized access through the getter methods.

Users of remote.Cmd should not check both Cmd.Err() and Cmd.ExitStatus()
until after Wait() returns.

Require communicators to call Cmd.Init before executing the command.
This will indicate incorrect usage of the remote.Cmd by causing a panic
in SetExitStatus.

The timeout for the remote command was taken from the wrong config
field, and the connection timeout was being used which is 5 min. Any
remote command taking more than 5 min would be terminated by
disconnecting the communicator. Remove the timeout from the context, and
rely on the global timeout provided by terraform.

There was no way to get the error from the communicator previously, so
the broken connection was silently ignored and the provisioner returned
successfully. Now we can use the new cmd.Err() method to retrieve any
errors encountered during execution.

[apparentlymart]

(Can save this for another day since subjective design discussion shouldn't block bug fixes)

Could we make the interface here instead be that cmd.Wait() returns (int, error) to make it clearer that there are some return values to deal with here? The pattern of mutating the object and getting the result via accessors -- particularly now that there are two of them -- feels less obvious and easier to do incorrectly.

I'd also consider following the lead of os/exec here by making a non-zero exit status be a special type of error that implements String to return the error message you used below, thus making it simpler to handle in cases like this where the caller doesn't really care to distinguish errors returned by the program or operational errors along the way:

return cmd.Wait() // returns an error which might be a typed error wrapping an exit status

os/exec wraps the entire process state, which is not possible here (due to the remoteness) and also doesn't seem necessary. A wrapper around just the int feels sufficient.

It looks like there a few cases below where the caller does care to recognize the exit status, which we could make more ergonomic by a helper method remote.GetExitStatus(err) that returns the exit status if it's that sort of error or zero otherwise:

if exitStatus := remote.GetExitStatus(err); exitStatus != 0 {
    // ...
}

What do you think? (Again, we can totally ignore this for now just to get the bug fixed, if you prefer.)

[jbardin]

Yes, I was going to do the same thing, but then decided to try and keep the API changes minimal, while getting rid of the races and adding an error value. It turns out that I probably would have had the same amount of changes to handle had I added an error value to Wait, with a specific ExitError error type for the return code. 🤷‍♂️

I think we can make that a separate patch, since it's more cosmetic than anything at this point.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.