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

Consider a guardrail for the fact that -f changed to load config, not fabfile #1823

Closed
pcdinh opened this Issue Jul 13, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@pcdinh

pcdinh commented Jul 13, 2018

Hi,

The following code (fabfile.py) can cause an error by specifying an "import os"

from invoke import task, Exit, Collection, Responder
from fabric2 import Connection
from fabric2 import Config
from fabric2.transfer import Transfer
from patchwork.files import append
import os

@task
def deploy(context, host_name):
    pass

Run the above code:

fab2 -f ./fabfile.py -i C:/Users/fox/.ssh/id_rsa --ssh-config=C:/Users/fox/.ssh/config -H mango deploy dev1

on Windows

or

fab2 -f ./fabfile.py -i ~/.ssh/id_rsa --ssh-config=~/.ssh/config -H mango deploy dev1

on Ubuntu 18

will yield the following exception

Traceback (most recent call last):
  File "c:\python36\lib\runpy.py", line 193, in _run_module_as_main
    "__main__", mod_spec)
  File "c:\python36\lib\runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "C:\Python36\Scripts\fab2.exe\__main__.py", line 9, in <module>
  File "c:\python36\lib\site-packages\invoke\program.py", line 329, in run
    self.update_config()
  File "c:\python36\lib\site-packages\fabric2\main.py", line 130, in update_config
    self.config.merge()
  File "c:\python36\lib\site-packages\invoke\config.py", line 931, in merge
    self._merge_file('runtime', "Runtime")
  File "c:\python36\lib\site-packages\invoke\config.py", line 951, in _merge_file
    merge_dicts(self._config, data)
  File "c:\python36\lib\site-packages\invoke\config.py", line 1200, in merge_dicts
    base[key] = copy.copy(value)
  File "c:\python36\lib\copy.py", line 96, in copy
    rv = reductor(4)
TypeError: can't pickle module objects

The exception disappears when I remove the line import os

Environment

Windows 10, Ubuntu 18
Python 3.6.4
Fabric 2.1.3

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 13, 2018

I think the problem is you're using -f, which unfortunately changed between versions - in v2 it's now used for runtime config files, not task/fabfile files. The flag for specifying task collection to load is -c.

That's the proximate cause, the root cause is I think an actual bug where we must not be stripping out module objects from Python-style config files. I thought this got fixed but it must be an outstanding PR or something...hrm, can't find. Say hi to pyinvoke/invoke#556 ...


That said, unless you modified your example from the real problem case: you shouldn't need -f ./fabfile.py at all (in either version); fab should be automatically seeking a fabfile Python module with your CWD added to sys.path, meaning that ./fabfile.py ought to be the first thing it tries. You may want to look at whatever prompted you to do this in the first place - might be another unrelated bug?


This (getting tripped up by the change in what fab -f means) seems like it'll be a common footgun so I am wondering if it's worth trying to throw a warning or error when someone points -f at fabfile or tasks...may leave this open for that.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 13, 2018

Also I just double checked and we do cover this in the upgrading doc - http://www.fabfile.org/upgrading.html#id7 - so I'm covered there at least 😅

@bitprophet bitprophet changed the title from Got "TypeError: can't pickle module objects" when "import os" is available to Consider a guardrail for the fact that -f changed to load config, not fabfile Jul 13, 2018

@bitprophet bitprophet added this to the 2.0.x milestone Jul 13, 2018

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 31, 2018

Hrm, I don't see a foolproof way to guard against the highest level problem (loading a fabfile as a config file) because it's technically plausible a user could do anything short of literally including a module object, in a config file. E.g. random stuff like:

from requests import get

some_key = get("http://my/api/server").json()

will work because copy.copy has no issue pickling functions, just modules. (No idea why, but, hey.) It's also somewhat plausible that users could even import common things like @task into their config file, because they want to do silly hairy things like stuff some tasks into the config, or (perhaps more likely) set a task subclass setting, or whatever.

What this means is, it's not possible to really detect the "oops, gave a fabfile to a config file" scenario. I think the best we can possibly do is just catch this particular wrinkle (user put a module in there, which will never ever work) and warn about that itself; maybe putting language in just in case it is this problem.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jul 31, 2018

Anyway, that means pyinvoke/invoke#556 is "it", going to have to close this one as unfixable for now :( Please follow that issue, it will be out soon!

@bitprophet bitprophet closed this Jul 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment