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: openssh_tunnel did not parse port in server #1567

Merged
merged 1 commit into from Apr 11, 2012

Conversation

tkf
Copy link
Contributor

@tkf tkf commented Apr 11, 2012

Despite what the docstring of openssh_tunnel mentioned, it did not parse
username in port in the server argument. This patch fixes the problem
so that ipython client can connect to the server over port-forwarded ssh
connection. For example, you can connect the client by the following
code now::

  c = Client('/path/to/ipcontroller-client.json',
             sshserver='me@localhost:1234')

Despite what the docstring of openssh_tunnel mentioned, it did not parse
username in port in the `server` argument.  This patch fixes the problem
so that ipython client can connect to the server over port-forwarded ssh
connection.  For example, you can connect the client by the following
code now::

  c = Client('/path/to/ipcontroller-client.json',
             sshserver='me@localhost:1234')
@minrk
Copy link
Member

minrk commented Apr 11, 2012

Thanks!

minrk added a commit that referenced this pull request Apr 11, 2012
Fix: openssh_tunnel did not parse port in `server`
@minrk minrk merged commit b8d103d into ipython:master Apr 11, 2012
@minrk
Copy link
Member

minrk commented Apr 11, 2012

Crap, this actually broke another case (implicit username from .ssh/config). I'll push a fix shortly.

minrk added a commit to minrk/ipython that referenced this pull request Apr 11, 2012
PR ipython#1567 fixed an issue where ssh server would not recognize custom
ssh server ports.  However, it broke another case by forcing
local username if it is unspecified.

ssh config should be trusted with the default username.
@tkf
Copy link
Contributor Author

tkf commented Apr 11, 2012

Thanks for the super fast pull!

I was a little bit uneasy for adding username and port automatically. Maybe it is possible to set port in .ssh/config also? I was unaware of the implicit case.

@minrk minrk mentioned this pull request Apr 11, 2012
@minrk
Copy link
Member

minrk commented Apr 11, 2012

Can you check if your case still works after #1568?

@minrk
Copy link
Member

minrk commented Apr 11, 2012

Yes, the code would have worked before if you had simply configured .ssh/config with your username and port. Now, with PR #1568, it properly only specifies the parts you are specifying. For instance, I have various user/server/port combos set up in my sshconfig, and then I can just pass their ssh alias to the Client constructor.

@tkf
Copy link
Contributor Author

tkf commented Apr 11, 2012

Yes your fix worked in my setup.

BTW, do you know if some incompatibility from 0.12 introduced in the master branch? I am thinking of using the master branch in my laptop and the stable one in my clusters.

@minrk
Copy link
Member

minrk commented Apr 11, 2012

I think they should work together, but I'm not 100% sure. You should be able to test locally with a couple of virtualenvs.

I will do some cleanup of serialization and connection files issues that have been bugging me, and these will break compatibility with older versions, but they will not go into 0.13 (unless it is significantly delayed).

minrk added a commit that referenced this pull request Apr 11, 2012
fix PR #1567

PR #1567 fixed an issue where ssh server would not recognize custom ssh server ports.

However, it broke another case by forcing local username if it is unspecified.

ssh config should be trusted with the default username.
@tkf
Copy link
Contributor Author

tkf commented Apr 11, 2012

Yea I know, I won't blame you even it brakes my laptop. I will be aware of your connection branch. Thank you very much!

@minrk
Copy link
Member

minrk commented Apr 11, 2012

It will only break for users trying to use multiple versions of IPython together, in the same cluster. Everything else should be fine across the upgrade.

Just as a datapoint, what launchers (SGE/MPI/SSH) are you using on your cluster?

@tkf
Copy link
Contributor Author

tkf commented Apr 11, 2012

I need to use same version in the machine where I run ipcontroller and the machine where I run the client because they pass around the parameter via ipcontroller-client.json, right?

I just started using via ssh and haven't setup anything advanced yet. The controller and engines are at the same remote machine.

@minrk
Copy link
Member

minrk commented Apr 11, 2012

The connection files don't change from 0.11-0.13, so you should be fine. But there will be a change between 0.13/0.14. But it's always a good idea for things to match if you can help it.

@tkf
Copy link
Contributor Author

tkf commented Apr 11, 2012

Right, you said that your connection branch won't be in 0.13. So I suppose the connection file format won't change as long as I am on the master.

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Fix: openssh_tunnel did not parse port in `server`
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
PR ipython#1567 fixed an issue where ssh server would not recognize custom
ssh server ports.  However, it broke another case by forcing
local username if it is unspecified.

ssh config should be trusted with the default username.
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
fix PR ipython#1567

PR ipython#1567 fixed an issue where ssh server would not recognize custom ssh server ports.

However, it broke another case by forcing local username if it is unspecified.

ssh config should be trusted with the default username.
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.

None yet

2 participants