Standardize mode_xxx, sudo, run, local and make it thread safe

[schacki]

There are currently 4 mode functions(=mode_local, mode_remote, mode_sudo, mode_user)  and 4 global mode variables (=MODE_USER,MODE_LOCAL, MODE_SUDO, MODE) , 3 cuisine execution functions (run, sudo, run_local) and the 3 native fabric execution functions (run, sudo, local).

Some of these have different APIs, but similar functionality. E.g. model_local and mode_sudo both influence the very basic behaviour of the run function. But mode_sudo uses/implements the "with" API and manipulates a global variable to control the behavior, while mode_local manipulates the module itself.

Some of these just overwrite and wrap the fabric native functions without changing the functionality like sudo, while others change the functionality significantly like run.

One global variable is boolean (mode_local), while others are strings(mode_sudo).

At the moment I do not get my head around it, why these complexities are necessary and why we cannot standardize all that and make it more simple. I am aware that I currently might not fully understand this complete beast and that whatever kind of changes we possibly apply here, this has a major effect.

So let me sketch some preliminary ideas here, to make my thoughts clearer, but also hopefully as a basis for further discussions around this topic:

1. Standardize on exactly 1 execution function
   Implement ONE cuisine run function. This run function checks fabric.api.env for certain variables. Based on the results of fabric.api.env, a certain excecution behavior will be enforced (e.g. run local with sudo, or run remote as current user, etc). And this is the only place where this complete set of behaviors is controlled.
2. Control User/Sudo
   a) 1 boolean env variable env.MODE_SUDO=True|False will determine, if sudo is used in the run function or not.
   b) The current mode_sudo and mode_user classes are used to controll env.MODE_SUDO
3. Controll Local/Remote
   a) 1 boolean env variable env.MODE_LOCAL=True|False will determine, if statements are executed on the local ore remote machine.
   b) 2 new classes mode_local and model_remote including the enter/exit API are used to controll env.MODE_LOCAL
4. I personally would set env.MODE_LOCAL=False and env.MODE_SUDO=False as defaults.

Looking forward to your feedback. I am happy to contribute any code along those lines.
Juergen

[kaharlichenko]

I think it is better to suggest such a change to fabric instead of cuisine. See fabric#538 and fabric#98 for more discussion.

[schacki]

Thinking about it, it absolutely makes sense to implement this kind of feature in fabric. But following above mentioned issues, it is not clear to me, when and if this will come.

But in the meantime, we still have to live with what we have: and this is a working, but slightly confusing/inconsistent solution - that I wanted to streamline with my proposal.

These are our options:
a) we fix it in cuisine, till it is eventually ressolved in fabric.
b) leave cuisine unchanged, till is eventually ressolved in fabric.

And anyway, if we want to approach the fabric guys, it might be helpful to come with a proper proposal. And at the moment, we just have a brainstorming with myself.

[sebastien]

This is in an interesting discussion, however, the main reason why I introduced this notion of "modes" is the to avoid having to re-write functions that use a specific mode.

In particular, most basic functions should only be implemented using run (ie. file ops, dir ops, command ops), but some others need to be implemented using sudo (packages, users, groups) as they are typical admin tasks.

However, you don't want to have file_write and file_write_sudo, which is why I introduced mode_sudo to override the status of run.

Sames goes for mode_local, as it allows to using any cuisine function on the local machine.

So in essence:

• run() is necessary, and should be what is used in most core cuisine functions
• sudo() is optional (if you have mode_sudo()), but core cuisine functions that are typically restricted to admins should then wrap the runs in mode_sudo()
• mode_local() and mode_sudo() are also necessary to allow to use existing cuisine core functions on the local system and using sudo priviledges without rewriting them

I agree with @schacki's proposals:

1. mode_local should support enter/exit such as mode_sudo
2. MODE_SUDO and MODE_LOCAL should be False by default
3. The MODE_* should be set in fabric.api.env, not in the globals.

The reason why I think we should keep sudo() is because I often sprinkle a couple of admin commands within my cuisine scripts. And I would hate to have to do:

with mode_sudo(): run("some admin-related command")

instead of

sudo("some admin-related command")

but I would agree with using mode_sudo() instead of run in cuisine's core functions.

Thoughts?

[schacki]

@sebastien

on the one hand, you prefer "sudo("some admin-related command")" over "with mode_sudo(): run("some admin-related command")", but at the same time you prefere "mode_sudo() instead of run in cuisine's core functions". I do not bring that together in my mind, please help :-).

i do not fully agree with: "but some others need to be implemented using sudo (packages, users, groups) as they are typical admin tasks". actually, this complete idea was triggered by an issue I had on webfaction: i do not have sudo rights there, but I do have read access to /etc/passwd and /etc/shadow. when i try to run check_user, i get an error, since you are using sudo:
d = sudo("cat /etc/passwd | egrep '^%s:' ; true" % (name))
s = sudo("cat /etc/shadow | egrep '^%s:' | awk -F':' '{print $2}'" % (name))
so i had to write my own check_user function without sudo to get it working.

[sebastien]

In that case reading the password should not use sudo, as it's not necessary. I didn't say I would like to use run() only in function core, only that I was not opposed to using with mode_sudo and run() instead ;)

[schacki]

great discussion so far, so what is the bottom line? and i am not sarcarstic but really not sure :-)
i think we agree on the issue, but not sure on the solution.

first of all: do we want to align/push the fabric team, as madkinder proposed, or do we just want to move forward and adapt later on, once the fabric team has come up with a better solution?

if we want to move forward, what kind of API do we want to use for what I called the execution function(s)? please find my ideas here:

a) run (cuisine)
the core cuisine execution function, which respects all our "mode" definitions. should we keep the name "run" or should we make the different functionality more prominent by giving it a different name like "do"?

b) run (fabric)
will be used where appropriate as part of the run(cusinse) function. otherwise not recommended to be used since it will never respect the mode definitions

c) run_local(cuisine) and local (fabric)
why do we need run_local? from my understanding it is the same as local(fabric). if it has any additional/different feature, let's keep it, otherwise bin it. whatever of the 2 we go for: it should then be used where appropriate as part of the run(cusinse) function. and it will be the recommended way for users, if they want to enforce the local execution (and only recommended for that).

d) sudo (cuisine) and sudo (fabric)
same as c): why do we need both? should we keep the cuisine version or bin it? whatever of the 2 we go for: it should then be used where appropriate as part of the run(cusinse) function. and it will be the recommended way for users, if they want to enforce the sudo execution (and only recommended for that).

[kaharlichenko]

I have had a talk on #fabric and figured out that the devs aren't happy about the idea of having any kind of local_sudo implementation in the core. So I guess the best way to persuade them (if it is possible at all) is to implement it in cuisine, then show them some use cases that demonstrate reusability of the code and how awesome it is. To cut the long story short: I guess we should go cuisine-only way. At least for now.

Here are some other random ideas.

• group_* and user_* functions shouldn't access /etc/{passwd,group} directly, instead they should use getent, groups, etc tools.

  This way we will honor /etc/nsswitch.conf picking users/groups from ldap and/or other backends configured there without even knowing about it.

• it would be nice to have an optional user argument to mode_sudo() defaulting to 'root'

  with mode_sudo(user='postgres'):
      run('createuser ...')
      run('createdb ...')

• I guess it is better to store a stack of modes rather then storing it in _old_mode. And yes, storing it in fabric.api.env is definitely the way to go.

• I haven't checked myself but they say fabric has some issues with quoting inconsistency with local/remote invocation. So run('echo "Hello, world!"') may behave differently in different local/remote/sudo modes.

I totally agree with sebastien that all the cuisine core functions are supposed to use the generic run() (the one that honors mode_*()). On the other hand I also agree with schacki that core functions should not impose sudo-ing in any way. Personally I prefer to declare the permissions explicitly:

with mode_sudo(user='superpower'):
    package_ensure('python-django')
    package_ensure('postgresql-9.1')

[schacki]

@madkinder:

• getent, groups ==> maybe please put that into a separate thread
• optional user argument to mode_sudo() defaulting to 'root' ==> great idea, and the same for mode_user() :-)
• fabric has some issues with quoting inconsistency ==> i think we just have to live with it for the moment :-(

[kaharlichenko]

@schacki
I don't think mode_user() can accept a user argument by design. sudo is supposed to change the user running the command, how do you imagine non-sudo mode to do that?

What do you think about having a stack of modes rather then storing them in _old_mode?

See #48 for getent stuff.

[schacki]

following this: http://docs.fabfile.org/en/1.3.4/usage/env.html#user, we could utilize for user argument of mode_user()

could you explain the stack of modes in more detail please, and what would be the advantage?

[schacki]

to be consistent with fabric approach, should the mode_xxx() not wrap the fabric settings decorator. from my understanding, this is EXACTLY what we want.

[sebastien]

@schacki yes indeed, and I think @madkinder made a pretty good summary of what we should do next. I've released an update today, but would love to have some contributions for you guys, esp. for the proper user_* and group_* implementation. I will take care of the mode_* rewrite using fabric's env.

So who is taking what?

[schacki]

regarding " proper user* and group* implementation", I would not recommend myself, my command line knowledge is limited (thats why I loave cuisine :-) ).
I am happy to remove sudo from where it is not needed (being aware that this is not a huge contribution), if you need any other help that is not too close to command line, let me know.

[sebastien]

Alright, then, thanks!

[davehughes]

I think there are a lot of good ideas in this thread, and I've been looking into possible implementations. @sebastien, does the env-based implementation seem to make sense, and is it coming along?

I put together a proof-of-concept implementation today, but it involves monkey-patching fabric, which some people find unacceptable. It turns out that fabric's internal _run_command function controls almost everything you'd want to change in the modes, including specifying a user to sudo as. I've patched that function to override variables based on env settings and to run local execution through subprocess (similar to a pull request I made earlier, but cleaner, since fabric handles output, aborting during errors, etc.).

Before I start working on a clean pull request, does this sound like a workable approach? I like to avoid monkey-patching whenever possible, but in this case it just seems like a much cleaner way to solve the problem.

[sebastien]

@davehughes I was planning to rework the run function in Cuisine to support sudo/user and local/remote all based on fabric.api.env. Maybe it would be best to wait for me to do the update (which will be probably done today, actually), and then see if you find things to be improved -- if we can avoid monkey-patching, we should try first.

Feel free to share a gist though, I'm curious to see what it's like!

[davehughes]

Sounds good, I'll look forward to the update. In the meantime, my stab at the implementation is summarized in this gist.

[sebastien]

Ha, it's interesting, we took at similar approach for modes -- well done! Here's the update 96e7300 -- I've added some test cases for modes as well.

[davehughes]

The update looks good - in hindsight, I guess it isn't necessary to have a stack for modes, since each context manager just has to hold on to the oldMode to be able to roll back to the previous state.

There are a few possible improvements that I see, mostly having to do with the local mode. Right now, there are a few things that fabric's remote run methods do that run_local() doesn't:

• Prefixing commands based on env.command_prefixes, env.path, and env.sudo_prefix (a property that doesn't seem to be documented on the fabric site)
• Some general behavior, such as output and conditionally aborting on errors based on env.warn_only.

I think I'm going to try and rework run_local so it works more like the _run_command function I was trying to monkey-patch. It will involve a little bit of duplication, but I think it's worth it to have behavior that is more in line with fabric's handling.

[sebastien]

Yes, if cuisine.run_local can behave as close as possible to fabric.run it would be ideal -- but let's try to keep things simple as well and not mimic obscure/undocumented featured of Fabric.

[davehughes]

My bad, env.sudo_prefix did get documented recently; I was browsing some outdated doc.

http://docs.fabfile.org/en/1.4.3/usage/env.html#sudo-prefix

This seems like it could be a replacement for the SUDO_PASSWORD setting. The main question in my mind is whether cuisine.sudo_password() is intended to only set the sudo password for mode_local (this seems to be the case, but I can't tell for sure).

[sebastien]

I actually didn't add sudo_password but saw it in the code today. I think it's safe to remove it, especially if it's an option already supported by fabric.