Skip to content
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

Fix Basic RPC Tunnel with Tramp, and Enhance Tunnelling for Multihop Usage #357

Merged
merged 8 commits into from
Jun 12, 2019

Conversation

falloutphil
Copy link
Contributor

Fixes #335 and provides further enhancements as per requested in the issue comments

@proofit404
Copy link
Contributor

Hi! Thank you for all the time you invested in the resolution of this issue!

Unfortunately, I'm unable to review this PR due to the lack of understanding of how it works.

Can you explain the actual situation, the desired behavior and how this patch implements it?

I know there is a long conversation in the related issue. Unfortunately, I'm short on time so I can't read it from cover to cover and restore the whole picture from it chronologically.

Thanks for your time and your patience!

Regards,
Artem.

@falloutphil
Copy link
Contributor Author

falloutphil commented Jun 6, 2019

Hi Artem,

Happy to help - I'm a big fan of the package!

Summary below:

  • The current code ssh tunnel doesn't work when sending RPC calls to files opened with TRAMP (although by chance it does work when no firewall exists, by incorrectly using a direct RPC connection - but the SSH tunnel is never created or used).
  • The problem is 2 fold - the tunnel setup process was incorrect/failing, and RPC calls were not being sent via the tunnel in any case.
  • The original fix was to simply address these issues by setting-up and using local port forwarding correctly.
  • Users then correctly commented that whilst the fix worked, it didn't work for a more complex scenario when a python file existed on a third server and was accessed via multihop TRAMP syntax through a proxy (eg /ssh:user@server1|ssh:user@server2|:/path/on/server2/to/file.py)
  • So I wrote function anaconda-jump-proxy-string to parse a TRAMP multihop connection string into an SSH client ProxyJump option string, which was fairly straightforward to add support to the current design.

My only concern is that the Docker use (rather than TRAMP/SSH use) remains compatible with the changes. I've highlighted as a PR code-review comment the one place that might need to handle Docker differently to TRAMP. When sending RPC requests via our local port forward - I've updated the http address to use the locally created tunnel - I suspect a predicate may be required to use the original http address with Docker. This is easy to fix if needs be... Alas I have no way of testing Docker use - so was hoping someone could confirm the RPC address in the Docker case?

@proofit404
Copy link
Contributor

Ok, cool!

If someone can test docker in this case and provides some logs here I'll be happy to merge PR.

Thanks for your time!

Regards,
Artem.

@AlexLewandowski
Copy link

I don't use docker much, but I was able to get completion working from the Tensorflow docker image after installing socat. Not sure what else to report, but let me know if you need any details.

@proofit404
Copy link
Contributor

Hi @AlexLewandowski

Do you install anaconda-mode from the git branch of this PR?

Just to be clear.

Regards, Artem.

@AlexLewandowski
Copy link

Yes, I am using falloutphil:master to multi-hop behind a university server and also to test the docker case. In both cases, everything works well.

Couple notes on this pull request. The message "Anaconda Jump Proxy: nil" is printed. Is this supposed to be more informative?

As stated in #335, the multihop connections must be added (via ssh -J p1,p2 user@remote) to known_hosts. However, make sure that the connections were not previously added to known_hosts with aliases. This initially caused me some trouble. For those who have many servers that they access in different ways, you may want to save the hassle and delete your known_hosts.

@proofit404 proofit404 merged commit 6574d8c into pythonic-emacs:master Jun 12, 2019
@proofit404
Copy link
Contributor

Thank everyone involved in the solution to this issue!

@proofit404
Copy link
Contributor

Unfortunately, I was forced to revert it.

See #358

CeleritasCelery added a commit that referenced this pull request Dec 4, 2019
…r Multihop Usage (#357)"

This reverts commit 24aa81b.
This was initially reverted due to #358, but it appears that this
commit was actually not the source of the problem. Reinstating this
commit unless problems start to appear.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

anaconda-mode with TRAMP not working?
3 participants