Fixes to work on Google App Engine #334

Closed
wants to merge 1 commit into
from

Projects

None yet

5 participants

@tokibito
Contributor

This PR resolves some problems when using GoogleAppEngine Sockets API.

1: Subprocess module doesn't work on Google App Engine.

Traceback (most recent call last):
  File "/home/tokibito/src/google_appengine/google/appengine/runtime/wsgi.py", line 239, in Handle
    handler = _config_handle.add_wsgi_middleware(self._LoadHandler())
  File "/home/tokibito/src/google_appengine/google/appengine/runtime/wsgi.py", line 298, in _LoadHandler
    handler, path, err = LoadObject(self._handler)
  File "/home/tokibito/src/google_appengine/google/appengine/runtime/wsgi.py", line 84, in LoadObject
    obj = __import__(path[0])
  File "/home/tokibito/sandbox/gae-paramiko/main.py", line 2, in <module>
    import paramiko
  File "/home/tokibito/sandbox/gae-paramiko/lib/paramiko/__init__.py", line 56, in <module>
    from paramiko.proxy import ProxyCommand
  File "/home/tokibito/sandbox/gae-paramiko/lib/paramiko/proxy.py", line 24, in <module>
    from subprocess import Popen, PIPE

2: It is raises the socket error if the active check timeout is too short.

Traceback (most recent call last):
  File "/base/data/home/runtimes/python27/python27_lib/versions/1/google/appengine/runtime/wsgi.py", line 266, in Handle
    result = handler(dict(self._environ), self._StartResponse)
  File "/base/data/home/apps/s~nullpobug-sandbox-hrd/ssh.376051288567321218/main.py", line 9, in app
    client.connect('test.nullpobug.com', username='spam', key_filename=os.path.join(base_dir, 'secret.key'))
  File "/base/data/home/apps/s~nullpobug-sandbox-hrd/ssh.376051288567321218/lib/paramiko/client.py", line 242, in connect
    t.start_client()
  File "/base/data/home/apps/s~nullpobug-sandbox-hrd/ssh.376051288567321218/lib/paramiko/transport.py", line 346, in start_client
    raise e
SSHException: Error reading SSH protocol banner[Errno 11] Resource temporarily unavailable

Such code works on Google App Engine with this patch.

import os
import paramiko
import paramiko.transport

def app(environ, start_response):
    base_dir = os.path.dirname(os.path.abspath(__file__))
    paramiko.transport.Transport._active_check_timeout = 5
    client = paramiko.SSHClient()
    client.set_missing_host_key_policy(paramiko.AutoAddPolicy())
    client.connect('test.nullpobug.com', username='spam', key_filename=os.path.join(base_dir, 'secret.key'))
    stdin, stdout, stderr = client.exec_command('uname -a')

    start_response('200 OK', [('Content-type', 'text/plain')])
    return stdout.read()
@coveralls

Coverage Status

Coverage decreased (-0.0%) when pulling 12cdf51 on tokibito:master into 951faed on paramiko:master.

@bitprophet bitprophet commented on the diff Aug 8, 2014
paramiko/proxy.py
@@ -21,7 +21,11 @@
import os
from shlex import split as shlsplit
import signal
-from subprocess import Popen, PIPE
+try:
+ from subprocess import Popen, PIPE
+except ImportError:
+ # Subprocess module doesn't work on Google App Engine.
+ Popen = PIPE = None
@bitprophet
bitprophet Aug 8, 2014 Member

Setting Popen to None seems like an odd choice here - if one were ever to try generating a ProxyCommand class they'd get a NoneType is not callable error. I think logging a warning level message would be the best approach.

Raising some other clearer, customized error (eg RuntimeError("You don't seem to have a working subprocess module, why are you trying to use ProxyCommand?") in ProxyCommand.__init__ might also be a good idea. I need to revisit in detail later, but am open to suggestions.

@wnojopra wnojopra added a commit to secretlair/paramiko that referenced this pull request May 6, 2015
@wnojopra wnojopra Added reference to paramiko#334. 0653822
@wnojopra wnojopra added a commit to secretlair/paramiko that referenced this pull request Oct 30, 2015
@wnojopra @wnojopra wnojopra + wnojopra Added reference to paramiko#334. d237650
@jeremydw
jeremydw commented Jun 21, 2016 edited

Hi folks -- https://github.com/grow/grow is looking to use (or at least import) paramiko on GAE. Any chance this can be merged or updated and merged? Thanks!

@bitprophet
Member
bitprophet commented Jun 21, 2016 edited

@jeremydw Couple questions!

  • Have you actually used this patch? I assume so but you don't actually say. Extra "used it, works great, +1" always valuable.
  • Any feedback on the line note I apparently left in the past re: exact behavior around neutering subprocess.Popen? What's best for GAE users in this situation exactly?
    • I assume neither you nor the OP are using ProxyCommand and friends (I'd assume that's straight up impossible to use on GAE), but are simply seeking to work around the import-time error. In that case, I assume a warning message instead of setting the objects to None would work equally well, yes?
@jeremydw

Hi! Nope, I haven't done due diligence to test this, but reviewing the change would fix the issue we were experiencing (simply importing a code path that imports paramiko causes a problem due to the unavailability of the subprocess module on GAE). Our desired behavior is to be able to import the module, and if there's a feature/code path that is unavailable due to the runtime or environment, for an error to be raised at that moment.

A customized RuntimeError would be preferrable over some sort of a "AttributeError: 'NoneType' object has no attribute 'Popen' error, so I 👍 -ed that.

@mburgs
mburgs commented Jun 27, 2016

To add here I've used it on GAE and it works great, +1 (without ProxyCommand)

@bitprophet bitprophet changed the base branch to paramiko:1.17 from paramiko:master Dec 6, 2016
@bitprophet
Member

Grump, forgot changing target branch doesn't do any magic around source branch. Please remember to rebase on bugfix releases if possible folks :( /me cherry-picks...

@bitprophet bitprophet added a commit that referenced this pull request Dec 6, 2016
@bitprophet bitprophet Tweak subprocess importing so it still ImportErrors, just lazily.
This feels better than raising our own custom error of whatever class when popen is None.

Only obvious downside is it's 'bad style' but I defer to Zen of Python number 9

Re #334
e2605f4
@bitprophet bitprophet added a commit that referenced this pull request Dec 6, 2016
@bitprophet bitprophet Changelog re #334, close #334 a29d22d
@bitprophet bitprophet added a commit that closed this pull request Dec 6, 2016
@bitprophet bitprophet Changelog re #334, close #334 a29d22d
@bitprophet bitprophet closed this in a29d22d Dec 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment