cli/connhelper: support overriding the docker binary over SSH #5627
Conversation
mattmacleod
force-pushed
the
5626-allow-binary-override-over-ssh
branch
from
951dcee
to
1fdc2b8
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5627 +/- ##
=======================================
Coverage 59.50% 59.51%
=======================================
Files 346 346
Lines 29365 29372 +7
=======================================
+ Hits 17474 17480 +6
- Misses 10916 10917 +1
Partials 975 975 |
cli/connhelper/connhelper.go
Outdated
| @@ -49,7 +56,7 @@ func getConnectionHelper(daemonURL string, sshFlags []string) (*ConnectionHelper | |||
| sshFlags = disablePseudoTerminalAllocation(sshFlags) | |||
| return &ConnectionHelper{ | |||
| Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) { | |||
| args := []string{"docker"} | |||
| args := []string{dockerSSHRemoteBinary()} | |||
There was a problem hiding this comment.
Having this inside of the Dialer means that it'll possibly get evaluated multiple times during a single CLI invocation – so, if something unexpected happens causing the environment variable to get unset while the process is running and Dialer gets called again a different value would get returned.
I think it'd be safer to move this evaluation up to L56 to make sure it only happens once.
There was a problem hiding this comment.
something like
remoteDockerBinary := dockerSSHRemoteBinary()
return &ConnectionHelper{
Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) {
args := []string{remoteDockerBinary}
...There was a problem hiding this comment.
Great point – thanks @laurazard! I've updated as suggested.
mattmacleod
force-pushed
the
5626-allow-binary-override-over-ssh
branch
2 times, most recently
from
82352ff
to
43dd6ea
Compare
cli/connhelper/connhelper_test.go
Outdated
|
|
||
| func TestDockerSSHBinaryOverride(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| env string | ||
| expected string | ||
| }{ | ||
| { | ||
| name: "Default", | ||
| env: "", | ||
| expected: "docker", | ||
| }, | ||
| { | ||
| name: "Override", | ||
| env: "other-binary", | ||
| expected: "other-binary", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| t.Setenv(DockerSSHRemoteBinaryEnv, tc.env) | ||
| result := dockerSSHRemoteBinary() | ||
| if result != tc.expected { | ||
| t.Errorf("expected %q, got %q", tc.expected, result) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
To be honest, the function tested here is so trivial, that it's in fact much simpler than the test code.
I don't think we really need it, but since it's already there, I guess it doesn't hurt either 🤷🏻
| // commands over SSH. It defaults to "docker" if the DOCKER_SSH_REMOTE_BINARY | ||
| // environment variable is not set. | ||
| func dockerSSHRemoteBinary() string { | ||
| value := os.Getenv(DockerSSHRemoteBinaryEnv) |
There was a problem hiding this comment.
It would probably be better if this was a per-context setting rather than a global env-variable.
However, it's a niche use-case, so I guess it's okay as an escape hatch and not really a blocker.
@thaJeztah @laurazard WDYT?
|
I'm honestly a bit on the fence on this change, but curious what exact issues this is resolving; from the ticket;
Happy to hear if there's more details to this requirement. |
|
Hey @thaJeztah! I think this is honestly probably pretty limited in terms of applicability, and I've only raised it due to a pretty specific set of circumstances – to support remote ops to machines running BalenaOS. These devices are running balenaEngine rather than Docker – it's a Moby-derived container engine. The upshot of all this is that the root filesystem is read-only, and the container engine CLI command is named Since the root filesystem is readonly by design, I've got limited options in terms of things like aliases or changes that would otherwise allow this to be implemented. We've obviously in my team got the option of building our own CLI release instead to support this pretty narrow use-case, but I figured it was generally a light-touch change that might benefit some other people too. |
This change adds the ability to override the docker binary used when executing commands over SSH. This is useful when the docker binary is not in the PATH of the SSH user, or when the binary is not named `docker`. If `DOCKER_SSH_REMOTE_BINARY` is set in the environment, the value will be used as the docker binary when executing commands over SSH. If the environment variable is not set, the default value of `docker` will be used. Signed-off-by: Matthew MacLeod <[email protected]>
Signed-off-by: Matthew MacLeod <[email protected]>
Simplify test assertion Co-authored-by: Paweł Gronowski <[email protected]> Signed-off-by: Matthew MacLeod <[email protected]>
mattmacleod
force-pushed
the
5626-allow-binary-override-over-ssh
branch
from
67dee0e
to
24fb803
Compare
This change adds the ability to override the docker binary used when executing commands over SSH. This is useful when the docker binary is not in the PATH of the SSH user, or when the binary is not named
docker.If
DOCKER_SSH_REMOTE_BINARYis set in the environment, the value will be used as the docker binary when executing commands over SSH. If the environment variable is not set, the default value ofdockerwill be used.Closes #5626
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)