Skip to content

[Remote SSH] Add a parameter allow_agent #4186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
gcoter opened this issue Jul 9, 2020 · 11 comments · Fixed by #4623
Closed

[Remote SSH] Add a parameter allow_agent #4186

gcoter opened this issue Jul 9, 2020 · 11 comments · Fixed by #4623
Labels
feature request Requesting a new feature help wanted p3-nice-to-have It should be done this or next sprint

Comments

@gcoter
Copy link
Contributor

gcoter commented Jul 9, 2020

Hello,

For some reason, when I use dvc pull or dvc push with an SSH remote, sometimes I get an error like this:

ERROR: unexpected error - No existing session

This is only on one specific machine that I use. On other machines, I don't have this problem.

To fix it, I have to give the parameter allow_agent=False to paramiko. If I edit manually the line https://github.com/iterative/dvc/blob/master/dvc/remote/ssh/__init__.py#L148 to add it, it works.

I was thinking that DVC could pass it through its config file, similarly to port or ask_password. For instance, I could add the line allow_agent = false in .dvc/config.local. I wanted to make a PR for that but I am too busy currently to do this myself :(

What do you think?

@ghost ghost added the triage Needs to be triaged label Jul 9, 2020
@efiop
Copy link
Contributor

efiop commented Jul 9, 2020

@gcoter Sounds good! Let's do that!

Unfortunately, we don't have the capacity for this right now as well 🙁 So it might have to wait until someone has time for it.

@efiop efiop added feature request Requesting a new feature help wanted p3-nice-to-have It should be done this or next sprint labels Jul 9, 2020
@ghost ghost removed triage Needs to be triaged labels Jul 9, 2020
@efiop
Copy link
Contributor

efiop commented Jul 9, 2020

@gcoter Btw, i think you can already do that using your ssh config:

Host example.com
  ForwardAgent no

@gcoter
Copy link
Contributor Author

gcoter commented Jul 12, 2020

Hi @efiop, thanks for your answer! I tried to modify ForwardAgent but it doesn't seem to work in my case...

@gcoter
Copy link
Contributor Author

gcoter commented Jul 12, 2020

I will try to make the PR myself

@efiop
Copy link
Contributor

efiop commented Jul 12, 2020

Hi @efiop, thanks for your answer! I tried to modify ForwardAgent but it doesn't seem to work in my case...

Might be that paramiko doesn't read it by itself from ssh config and we need to extract it ourselves (luckily there is user_ssh_config logic already in the code). Both ways would be a good solution, but probably I would first try to use user_ssh_config and then consider introducing an in-dvc config option, just because it is a much better known mechanism that people might already use. But we would be happy to accept a patch using any approach, whichever suits you best 🙂 Thanks again for looking into it! 🙏

@gcoter
Copy link
Contributor Author

gcoter commented Jul 12, 2020

@efiop Actually, I managed to add a parameter allow_agent and I could make a PR (it was easier than I thought) 🙂 But I understand that using the user's ssh config is a good approach as well. Would it be easy to implement your idea? What do you prefer, should I make a PR with what I have?

@efiop
Copy link
Contributor

efiop commented Jul 12, 2020

@gcoter It is actually extremely easy to implement. Here we have ProxyCommand support

proxy_command = user_ssh_config.get("proxycommand", False)

so all you need is just

self.allow_agent = user_ssh_config.get("forwardagent", False)

and just pass it to the connection.

Would it be easy to implement your idea? What do you prefer, should I make a PR with what I have?

We would be happy with any approach that you actually will use 🙂 But I feel like the proper way is to indeed start with ForwardAgent, just because it is not paramiko-specific and people will likely have it already. So really this is your choice here, we will be happy with one or both, as long as they are tested and used by you or other users 🙂

@gcoter
Copy link
Contributor Author

gcoter commented Jul 31, 2020

@efiop Sorry for my late response, I will try to implement your idea when I have time 🙂

@efiop
Copy link
Contributor

efiop commented Aug 1, 2020

@gcoter No worries :) Thank you so much! 🙏

@gcoter
Copy link
Contributor Author

gcoter commented Aug 30, 2020

Hi @efiop, I have worked on the two approaches but it seems to me that having an option allow_agent in DVC config would be better for my particular use case. With your idea, I need to create a new host in my SSH config just for DVC (all other applications in my VM work with the default config) and set ForwardAgent to no. Since this issue is really specific to DVC, it makes sense to set this option at the DVC level directly. And it is easier to setup for my colleagues as well (they use the same VM).
However, my use case might not be the general use case and I don't want to impose anything which would be specific to me. Or we could combine both ideas. What do you think?

@efiop
Copy link
Contributor

efiop commented Aug 30, 2020

all other applications in my VM work with the default config

Oh, I didn't realise that before. Makes me think if there is some other problem lurking in there. Do you have a verbose log by any chance?

If allow_agent in dvc config would solve it for you, we'll be happy to accept a PR just for it. 🙏 If we'll need to support ssh config in the future, your change won't harm it at all, we'll just merge the config options together like we do in other places.

gcoter added a commit to gcoter/dvc that referenced this issue Sep 26, 2020
gcoter added a commit to gcoter/dvc.org that referenced this issue Sep 26, 2020
shcheklein added a commit to iterative/dvc.org that referenced this issue Sep 30, 2020
* remote modify [ssh]: Add doc about the parameter 'allow_agent'

This parameter was created to fix iterative/dvc#4186

* Update content/docs/command-reference/remote/modify.md

* minor: apply inline code

* Update content/docs/command-reference/remote/modify.md

Co-authored-by: Ruslan Kuprieiev <[email protected]>
Co-authored-by: Ivan Shcheklein <[email protected]>
Co-authored-by: Jorge Orpinel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requesting a new feature help wanted p3-nice-to-have It should be done this or next sprint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants