Machine-specific persistent option to specify SSH config #3410

brettdh · 2016-05-05T12:53:41Z

This adds two flags to docker-machine create:

--generic-ssh-config-file <path/to/config>

    Use the provided config file with the external SSH client (-F)

--generic-use-user-ssh-config

    Use ~/.ssh/config as the config file

(The flags are mutually exclusive in order to avoid ambiguity.)

Tried this out in my environment, which requires a ProxyCommand to
access a remote site via SSH. Before, the issue described in #3310
occurred, preventing me from using docker-machine to configure this
host. With this, the --generic-use-user-ssh-config option adds
the config file in place of the previous-default /dev/null and all
seems well.

Also added a couple unit test cases.

Depends on an updated version of #3209.
Note that this PR includes brettdh/machine@d39de4f
(which is just #3209 rebased on master).

The merge will be a bit messy, since this will get rebased on whatever #3209
becomes, but at least now it can be reviewed in relative isolation.
I considered setting the base for this PR on brettdh/machine@d39de4f,
but I'm pretty sure that would make a PR on my fork, rather than on docker/machine.

GordonTheTurtle · 2016-05-09T12:16:47Z

Please sign your commits following these rules:
https://github.com/docker/docker/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "use-user-ssh-config" git@github.com:brettdh/machine.git somewhere
$ cd somewhere
$ git rebase -i HEAD~7
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Ammending updates the existing PR. You DO NOT need to open a new one.

brettdh · 2016-05-09T12:28:48Z

Gordon is a little excitable about a missing sign-off, it seems.

brettdh · 2016-06-06T12:55:05Z

@nathanleclaire any further thoughts on this and/or #3209?

bamarni · 2016-06-06T16:59:14Z

Is --generic-use-user-ssh-config really needed? As it seems equivalent to --generic-ssh-config-file ~/.ssh/config.

brettdh · 2016-06-06T18:46:25Z

@bamarni yep, that's correct. Just a shortcut (barely).

lox · 2016-11-03T01:09:40Z

Any progress on this? This will block anyone with a slightly complicated SSH setup.

lox · 2016-11-03T01:12:52Z

@brettdh I'd happily take this over if you aren't inclined.

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>

This adds two flags to the `generic` driver: --generic-ssh-config-file <path/to/config> Use the provided config file with the external SSH client (-F) --generic-use-user-ssh-config Use ~/.ssh/config as the config file The flags are mutually exclusive in order to avoid ambiguity. This also adds the SSHConfigFile parameter to BaseDriver and a GetSSHConfigFile method to all drivers. It seems like there should be a way to reduce this duplication, since most of the implementations are identical (return "/dev/null"). I'm a golang noob, though, so if you know of a better way to accomplish this, please let me know. Tried this out in my environment, which requires a `ProxyCommand` to access a remote site via SSH. Before, the issue described in docker-archive-public#3310 occurred, preventing me from using `docker-machine` to configure this host. With this, the `--generic-use-user-ssh-config` option adds the config file in place of the previous-default `/dev/null` and all seems well. Also added a couple unit test cases. Fixes docker-archive-public#3318. Signed-off-by: Brett Higgins <brhiggins@arbor.net>

The listening server speaks HTTP, so doing the validation check should take HTTP proxy settings from env vars into account. Signed-off-by: Brett Higgins <brhiggins@arbor.net>

See docker-archive-public#3209 Signed-off-by: Brett Higgins <brhiggins@arbor.net>

Signed-off-by: Brett Higgins <brhiggins@arbor.net>

To hopefully avoid breaking that test. Signed-off-by: Brett Higgins <brhiggins@arbor.net>

Signed-off-by: Brett Higgins <brhiggins@arbor.net>

brettdh · 2016-11-03T21:56:13Z

@lox rebased and resolved conflicts. I'd say this is still in @nathanleclaire 's court, though - I don't know what he wanted to do about #3209, so that conversation was blocking this, IIRC.

lox · 2016-11-03T22:32:31Z

Yup fair enough. Merging this would at least let the majority of users who need ssh proxying get on with things though, as it stands machine just doesn't work for us.

brettdh · 2016-11-03T23:17:16Z

Good news is that docker-machine is tremendously easy to build, so if you
need this today you can always go that route, as I've done at my company.
On Thu, Nov 3, 2016 at 6:32 PM Lachlan Donald notifications@github.com
wrote:

Yup fair enough. Merging this would at least let the majority of users who
need ssh proxying get on with things though, as it stands machine just
doesn't work for us.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3410 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFlnrQb6yGb9qLcJwWegu7rV7HGeK5Dks5q6mEIgaJpZM4IX_Rb
.

lox · 2016-11-03T23:18:57Z

I'm fairly allergic to distributing custom builds of docker things internally, but yup, fair point.

nathanleclaire · 2016-11-03T23:41:17Z

Here's some thinking on this. Sorry it's been slow going but it's a bit riskier change than most.

it's good feature and i'd like to support it if possible, additionally fixing the long-standing propagating of options bug would be nice
we could probably avoid bumping the API version and just print a warning like we've done here, since we are just adding methods, not changing or removing existing ones, but this should be tested to make sure it behaves as expected. maybe we could treat this as experiment since api version bump / modification hasn't really been done before
@brettdh you can't edit vendored code directly but it seems you could just set HTTPClient.Proxy on the returned client where we generate the code.
what's the utility of GetMachineOptions in general? I would expect to just call SetMachineOptions for each server driver... It's a one-way operation dictating what those options should be from the main binary/flags, right?
isn't this a bit risky? have you tested invoking this code on hosts originally created with 0.8.2? i think it will need either a proper host migration or some other way to assure safety, the nil checking seems a bit off to me... i guess it's so you don't have to specify the ssh config file each invocation? (i agree our lack of a default config generally, which would help to avoid having to set the flags each time, is annoying).
why --use-user-ssh-config when they could just specify that themselves with the other new flag?

thanks for your persistence and interest in this.

lox · 2016-11-03T23:42:17Z

Agree with dropping --use-user-ssh-config, seems like unneeded syntactic sugar.

brettdh · 2016-11-04T13:37:38Z

@brettdh you can't edit vendored code directly but it seems you could just set HTTPClient.Proxy on the returned client where we generate the code.

I'm taking a crack at this, but my golang is rusty, and I feel like I'm doing something rather dumb - or just not fully understanding the details of http.Transport and related bits.

what's the utility of GetMachineOptions in general? I would expect to just call SetMachineOptions for each server driver... It's a one-way operation dictating what those options should be from the main binary/flags, right?

I think you answered your own question in the next bit; we need to be able to retrieve the saved options because the user isn't providing this option on every invocation.

...at least, that would have been my answer a few minutes ago. Looking more carefully, it looks like GetMachineOptions is only called during create, and it only ever returns the defaultOptions from libmachine/opt/options.go. So that's pretty silly. I'll rip it out and just call mcnopt.Opts() directly.

isn't this a bit risky? have you tested invoking this code on hosts originally created with 0.8.2? i think it will need either a proper host migration or some other way to assure safety, the nil checking seems a bit off to me... i guess it's so you don't have to specify the ssh config file each invocation? (i agree our lack of a default config generally, which would help to avoid having to set the flags each time, is annoying).

Maybe? It's been a while since I tested it, but I thought that the missing value would end up with a value of nil, since it's just saved as JSON.

Giving this a quick test: with a machine created with 0.8.1, and using this PR's build to talk to it, docker-machine inspect shows SSHOptions: null, and docker-machine ssh works fine. (Interestingly, using 0.8.1 to docker-machine inspect a machine created with this PR's build also works fine; it just doesn't show SSHOptions.)

why --use-user-ssh-config when they could just specify that themselves with the other new flag?

This has now been brought up by three different people, so I give. 😅
I might like to try just having --generic-ssh-config-file having a default value of $HOME/.ssh/config if it's given without a value, but it appears that's tricky, because (I think) you get "" for its value whether it's provided without a value or not provided. I'll take a quick look, but probably won't spend much time on it.

- Remove --use-user-ssh-config - Remove GetMachineOptions - Remove modification to vendored code - Instead set proxy in mcndockerclient/docker_client.go Signed-off-by: Brett Higgins <brhiggins@arbor.net>

brettdh · 2016-11-04T18:10:19Z

libmachine/mcndockerclient/docker_client.go

+	client, err := dockerclient.NewDockerClient(url, tlsConfig)
+	if err != nil {
+		transport := (client.HTTPClient.Transport).(*http.Transport)
+		transport.Proxy = http.ProxyFromEnvironment


This is the part I'm uncertain about - whether it is sane to update the proxy setting in this way.

...I hope, anyway - by not trying to SetMachineOptions on the remote driver if this is the ci-test driver. Have to push to test, as this doesn't seem to fail on my laptop. Signed-off-by: Brett Higgins <brhiggins@arbor.net>

brettdh · 2016-11-16T13:33:52Z

@nathanleclaire any further thoughts on this? I think I've addressed all your concerns stated above, except perhaps the API version non-bump testing that you mentioned, as I'm not sure how to go about that.

brettdh · 2016-12-05T13:43:51Z

@nathanleclaire had another coworker get bit by the lack of this feature today. Any chance we could take another look at this soon?

bilts · 2017-10-25T14:06:43Z

Any status on this? I can't use docker-machine because of it.

nathanleclaire · 2017-10-25T18:06:54Z

I am not maintainer any more -- ping @shin-

brettdh · 2017-10-25T18:30:38Z

And I'm not using docker-machine these days, so @lox if you still want to take it over, be my guest :)

davidvasandani · 2018-11-09T05:57:38Z

@shin- Any thoughts on this? Thanks!

EvgeniiProkofevItomych · 2019-02-21T14:41:16Z

@shin this makes impossible to use GitLab CI autoscale approach within isolated network

GuSuku · 2019-05-01T22:19:47Z

What is the status of this? This is currently a big impediment for connecting to remotes beyond bastions.

letalvoj · 2019-11-04T10:03:20Z

Could please any maintainer summarize the changes needed to get this to master? Thanks 👍

josephwibowo · 2020-01-03T21:02:11Z

Is there an update on this?
How can I use docker-machine if I'm using Bastion and not able to change the ssh config?

GordonTheTurtle added status/0-triage labels May 5, 2016

brettdh mentioned this pull request May 5, 2016

Generic driver: flags to provide SSH config file #3319

Closed

GordonTheTurtle added dco/no labels May 9, 2016

brettdh force-pushed the use-user-ssh-config branch from 5727275 to d322b8d Compare May 9, 2016 12:16

GordonTheTurtle added the dco/no label May 9, 2016

brettdh force-pushed the use-user-ssh-config branch from d322b8d to 5a52439 Compare May 9, 2016 12:26

GordonTheTurtle removed dco/no labels May 9, 2016

bamarni mentioned this pull request Jun 9, 2016

docker-machine create -d generic fails with multiple known ssh-keys #3487

Open

bamarni mentioned this pull request Jul 2, 2016

Need to SSH through a gateway with a custom driver #3551

Open

tlefebvr mentioned this pull request Jul 13, 2016

Fork Spirals-Team/docker-machine-driver-g5k#1

Merged

gitbisect mentioned this pull request Sep 29, 2016

I need docker-machine to use my ssh config file #3794

Open

nathanleclaire mentioned this pull request Nov 3, 2016

Unable to create via generic driver over SSH Proxy #3857

Closed

nathanleclaire and others added 7 commits November 3, 2016 17:27

Propogate given configuration to plugin servers

610a169

Signed-off-by: Nathan LeClaire <nathan.leclaire@gmail.com>

Use HTTPS client for ValidateCertificate

e1f039c

The listening server speaks HTTP, so doing the validation check should take HTTP proxy settings from env vars into account. Signed-off-by: Brett Higgins <brhiggins@arbor.net>

Revert driver API changes; build on generic opts

76be0d3

See docker-archive-public#3209 Signed-off-by: Brett Higgins <brhiggins@arbor.net>

Got it working atop the generic machine opts PR

cc39c0e

Signed-off-by: Brett Higgins <brhiggins@arbor.net>

Suppress machine opts warning for ci-test driver

ca2da34

To hopefully avoid breaking that test. Signed-off-by: Brett Higgins <brhiggins@arbor.net>

Golint fixes

38ce22f

Signed-off-by: Brett Higgins <brhiggins@arbor.net>

brettdh force-pushed the use-user-ssh-config branch from 5a52439 to 38ce22f Compare November 3, 2016 21:52

GordonTheTurtle added dco/no labels Nov 4, 2016

Address some review feedback

e1a85b6

- Remove --use-user-ssh-config - Remove GetMachineOptions - Remove modification to vendored code - Instead set proxy in mcndockerclient/docker_client.go Signed-off-by: Brett Higgins <brhiggins@arbor.net>

brettdh force-pushed the use-user-ssh-config branch from 2dfbeca to e1a85b6 Compare November 4, 2016 18:07

GordonTheTurtle added the dco/no label Nov 4, 2016

brettdh commented Nov 4, 2016

View reviewed changes

Suppress the ci-test driver failure

d1a9738

...I hope, anyway - by not trying to SetMachineOptions on the remote driver if this is the ci-test driver. Have to push to test, as this doesn't seem to fail on my laptop. Signed-off-by: Brett Higgins <brhiggins@arbor.net>

GordonTheTurtle removed the dco/no label Nov 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Machine-specific persistent option to specify SSH config #3410

Machine-specific persistent option to specify SSH config #3410

Uh oh!

brettdh commented May 5, 2016

GordonTheTurtle commented May 9, 2016

brettdh commented May 9, 2016

brettdh commented Jun 6, 2016

bamarni commented Jun 6, 2016 •

edited

Loading

brettdh commented Jun 6, 2016

lox commented Nov 3, 2016

lox commented Nov 3, 2016

brettdh commented Nov 3, 2016

lox commented Nov 3, 2016

brettdh commented Nov 3, 2016

lox commented Nov 3, 2016

nathanleclaire commented Nov 3, 2016

lox commented Nov 3, 2016

brettdh commented Nov 4, 2016 •

edited

Loading

brettdh Nov 4, 2016

brettdh commented Nov 16, 2016

brettdh commented Dec 5, 2016

bilts commented Oct 25, 2017

nathanleclaire commented Oct 25, 2017

brettdh commented Oct 25, 2017

davidvasandani commented Nov 9, 2018

EvgeniiProkofevItomych commented Feb 21, 2019

GuSuku commented May 1, 2019

letalvoj commented Nov 4, 2019

josephwibowo commented Jan 3, 2020

Machine-specific persistent option to specify SSH config #3410

Are you sure you want to change the base?

Machine-specific persistent option to specify SSH config #3410

Uh oh!

Conversation

brettdh commented May 5, 2016

GordonTheTurtle commented May 9, 2016

brettdh commented May 9, 2016

brettdh commented Jun 6, 2016

bamarni commented Jun 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

brettdh commented Jun 6, 2016

lox commented Nov 3, 2016

lox commented Nov 3, 2016

brettdh commented Nov 3, 2016

lox commented Nov 3, 2016

brettdh commented Nov 3, 2016

lox commented Nov 3, 2016

nathanleclaire commented Nov 3, 2016

lox commented Nov 3, 2016

brettdh commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

brettdh Nov 4, 2016

Choose a reason for hiding this comment

brettdh commented Nov 16, 2016

brettdh commented Dec 5, 2016

bilts commented Oct 25, 2017

nathanleclaire commented Oct 25, 2017

brettdh commented Oct 25, 2017

davidvasandani commented Nov 9, 2018

EvgeniiProkofevItomych commented Feb 21, 2019

GuSuku commented May 1, 2019

letalvoj commented Nov 4, 2019

josephwibowo commented Jan 3, 2020

bamarni commented Jun 6, 2016 •

edited

Loading

brettdh commented Nov 4, 2016 •

edited

Loading