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

No way to specify remote environment variables #1826

Closed
max-arnold opened this Issue Jul 16, 2018 · 13 comments

Comments

Projects
None yet
3 participants
@max-arnold

max-arnold commented Jul 16, 2018

from fabric import task

cmd = '''python3 -c "import os; print(os.environ.get('ENV'))"'''

@task
def test_env(ctx):
    ctx.run(cmd, replace_env=False, echo=True, env={'ENV': 'production'})

@task
def test_prefix(ctx):
    with ctx.prefix('ENV=production'):
        ctx.run(cmd, replace_env=False, echo=True)

First task prints None because the default sshd install has AcceptEnv LANG LC_* and nothing else (see #1744 (comment)):

% fab -H example.com test-env

python3 -c "import os; print(os.environ.get('ENV'))"
None

Second one fails due to && (although it probably used to work, see #1744 (comment)):

% fab -H example.com test-prefix

ENV=production && python3 -c "import os; print(os.environ.get('ENV'))"
None

The value of replace_env flag does not matter.

@ciarancourtney

This comment has been minimized.

Show comment
Hide comment
@ciarancourtney

ciarancourtney Aug 3, 2018

One workaround is to use export in the prefix case:

ENV=production && python3 -c "import os; print(os.environ.get('ENV'))"
None

export ENV=production && python3 -c "import os; print(os.environ.get('ENV'))"
production

ciarancourtney commented Aug 3, 2018

One workaround is to use export in the prefix case:

ENV=production && python3 -c "import os; print(os.environ.get('ENV'))"
None

export ENV=production && python3 -c "import os; print(os.environ.get('ENV'))"
production
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 7, 2018

Member

Think this needs to roll into #1744, please follow along there for comments.

Member

bitprophet commented Aug 7, 2018

Think this needs to roll into #1744, please follow along there for comments.

@bitprophet bitprophet closed this Aug 7, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 7, 2018

Member

Sorry, #1752. Ugh. So many tickets. No wonder I never get anything done, I'm spending all my time closing duplicates 😢

Member

bitprophet commented Aug 7, 2018

Sorry, #1752. Ugh. So many tickets. No wonder I never get anything done, I'm spending all my time closing duplicates 😢

@bitprophet bitprophet reopened this Aug 8, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 8, 2018

Member

Still suffering ticket confusion. Insofar as this issue is about "most servers only have AcceptEnv LANG LC_* set by default", it needs to stay open.


Offhand, while I don't like the "turn the requested env into a bunch of INLINE=settings LIKE=this BEFORE=the command" approach, I don't see anything that will work better.

Main question is thus how to add this to the API; the released-as-2.0 behavior is to use "proper" env manipulation, so we cannot simply change it, or we'll break for anyone who's happily trucking along with a more permissive AcceptEnv (as uncommon as that probably is).

I'm thinking most obvious is to add a configuration setting (maybe also a keyword argument, but this feels a lot like it really wants to be config-only; it's effectively a property of the remote host or connection) like, say, ssh_env_inline (a bit mouthy but anything I can come up with is gonna be at least as bad).

Member

bitprophet commented Aug 8, 2018

Still suffering ticket confusion. Insofar as this issue is about "most servers only have AcceptEnv LANG LC_* set by default", it needs to stay open.


Offhand, while I don't like the "turn the requested env into a bunch of INLINE=settings LIKE=this BEFORE=the command" approach, I don't see anything that will work better.

Main question is thus how to add this to the API; the released-as-2.0 behavior is to use "proper" env manipulation, so we cannot simply change it, or we'll break for anyone who's happily trucking along with a more permissive AcceptEnv (as uncommon as that probably is).

I'm thinking most obvious is to add a configuration setting (maybe also a keyword argument, but this feels a lot like it really wants to be config-only; it's effectively a property of the remote host or connection) like, say, ssh_env_inline (a bit mouthy but anything I can come up with is gonna be at least as bad).

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 9, 2018

Member

Have this working, as inline_ssh_env (yes, opposite of above) with one caveat: kicking the can regarding any escaping of env var values, even ones with spaces or other shell metachars in them. Nothing is quoted, backslashed, or etc. This has the following properties:

  • Not doing it half-assed means not having to support that half-assed attempt forever. Cough, fabric 1's general approach to commands at large, cough.
  • We retain the right to find some bulletproof shell escaping related library function and apply it later (with, as always, some opt-in setting to activate it). It's easier to do this than to undo a naive, only-partly-works implementation (see first line item).
  • Users can insert their own quoting, escaping, etc in the env dict's values, and it will arrive untouched at the remote end. They may still have issues depending on the details and their remote shell, but we will not be getting in their way at all.
    • Again, we have a lot of experience where a half-baked auto-escape system actually makes life worse for any use cases it fails to cover.
Member

bitprophet commented Aug 9, 2018

Have this working, as inline_ssh_env (yes, opposite of above) with one caveat: kicking the can regarding any escaping of env var values, even ones with spaces or other shell metachars in them. Nothing is quoted, backslashed, or etc. This has the following properties:

  • Not doing it half-assed means not having to support that half-assed attempt forever. Cough, fabric 1's general approach to commands at large, cough.
  • We retain the right to find some bulletproof shell escaping related library function and apply it later (with, as always, some opt-in setting to activate it). It's easier to do this than to undo a naive, only-partly-works implementation (see first line item).
  • Users can insert their own quoting, escaping, etc in the env dict's values, and it will arrive untouched at the remote end. They may still have issues depending on the details and their remote shell, but we will not be getting in their way at all.
    • Again, we have a lot of experience where a half-baked auto-escape system actually makes life worse for any use cases it fails to cover.

bitprophet added a commit that referenced this issue Aug 9, 2018

bitprophet added a commit that referenced this issue Aug 9, 2018

Failing test proving #1826
And a skipped one around escaping. ugh.

bitprophet added a commit that referenced this issue Aug 9, 2018

Implement #1826
Final piece, when setting active, prefixes instead of using channel method

Again, explicitly NOT escaping for now, users can try putting their own in if they wanna
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 9, 2018

Member

Enough of the Travis run is green that I am declaring this Done For Now. It'll be out in 2.3.

Member

bitprophet commented Aug 9, 2018

Enough of the Travis run is green that I am declaring this Done For Now. It'll be out in 2.3.

@bitprophet bitprophet closed this Aug 9, 2018

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 9, 2018

Member

Grump, just realized that this isn't the absolute best way to do this; it breaks down for nontrivial shell commands, eg FOO=bar command1 && command2 only applies FOO to command1.

Going to VERY quickly address this as if it were a bug and pop out another release.

Member

bitprophet commented Aug 9, 2018

Grump, just realized that this isn't the absolute best way to do this; it breaks down for nontrivial shell commands, eg FOO=bar command1 && command2 only applies FOO to command1.

Going to VERY quickly address this as if it were a bug and pop out another release.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 9, 2018

Member

2.3.1 now out with that fix. Hopefully that's the only big embarrassing mistake with this 😝 I skimmed the rest of v1's shell_env() contextmanager and the rest of what it does isn't very applicable (extra logic around $PATH that we don't currently offer, that sort of thing).

Member

bitprophet commented Aug 9, 2018

2.3.1 now out with that fix. Hopefully that's the only big embarrassing mistake with this 😝 I skimmed the rest of v1's shell_env() contextmanager and the rest of what it does isn't very applicable (extra logic around $PATH that we don't currently offer, that sort of thing).

@max-arnold

This comment has been minimized.

Show comment
Hide comment
@max-arnold

max-arnold Aug 17, 2018

Thanks! Confirm that it works for me, but only with replace_env=True.

With False it fails with bash syntax error (the list of my environment variables is reduced for clarity):

bash: -c: line 0: syntax error near unexpected token `('
bash: -c: line 0: `export PS1=(venv) %B%F{green}%n@%m%k %B%F{blue}%~%(?. .%B%F{red}!%F{blue})%# %b%f%k WORDCHARS=*?-.[]~=&;!#$%^(){}<> && cd /var/www/example.com && /var/www/example.com/venv/bin/python3 src/manage.py migrate --noinput'

max-arnold commented Aug 17, 2018

Thanks! Confirm that it works for me, but only with replace_env=True.

With False it fails with bash syntax error (the list of my environment variables is reduced for clarity):

bash: -c: line 0: syntax error near unexpected token `('
bash: -c: line 0: `export PS1=(venv) %B%F{green}%n@%m%k %B%F{blue}%~%(?. .%B%F{red}!%F{blue})%# %b%f%k WORDCHARS=*?-.[]~=&;!#$%^(){}<> && cd /var/www/example.com && /var/www/example.com/venv/bin/python3 src/manage.py migrate --noinput'
@max-arnold

This comment has been minimized.

Show comment
Hide comment
@max-arnold

max-arnold Aug 17, 2018

Also, with replace_env=True it fails to find my local python executable from virtualenv because PATH is not inherited.

max-arnold commented Aug 17, 2018

Also, with replace_env=True it fails to find my local python executable from virtualenv because PATH is not inherited.

@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 17, 2018

Member

That is probably because you have lots of nontrivial data in your local shell env and it needs escaping on the remote end (because it's being shoehorned into the command line string instead of set via the server's "real" env setting functionality, as outlined above).

To be fair, using replace_env=False is a security risk as well as being problematic for those escaping reasons, and also I don't see why your local PATH would be useful on the remote end anyways...? (Unless your local/remote systems resemble each other closely, of course.)

Is there a reason not to put PATH updates into either the remote e.g. ~/.bash_profile, or into the Fabric code/config (alongside your existing, explicit env={'ENV': 'prod'} bit)?

(Yes, these concerns should have been raised earlier as they are technically orthogonal to the actual functionality fix... apologies for not doing so 🙃 )

Member

bitprophet commented Aug 17, 2018

That is probably because you have lots of nontrivial data in your local shell env and it needs escaping on the remote end (because it's being shoehorned into the command line string instead of set via the server's "real" env setting functionality, as outlined above).

To be fair, using replace_env=False is a security risk as well as being problematic for those escaping reasons, and also I don't see why your local PATH would be useful on the remote end anyways...? (Unless your local/remote systems resemble each other closely, of course.)

Is there a reason not to put PATH updates into either the remote e.g. ~/.bash_profile, or into the Fabric code/config (alongside your existing, explicit env={'ENV': 'prod'} bit)?

(Yes, these concerns should have been raised earlier as they are technically orthogonal to the actual functionality fix... apologies for not doing so 🙃 )

@max-arnold

This comment has been minimized.

Show comment
Hide comment
@max-arnold

max-arnold Aug 18, 2018

Keeping PATH intact is important for me, because the task is designed to be invoked both remotely and locally (i.e. fab -H host migrate or fab migrate). Below is my current solution (quite verbose, you may find it useful for improving Fabric API):

@contextlib.contextmanager
def chdir(directory):
    """Context manager to change current directory."""
    curdir = os.getcwd()
    try:
        os.chdir(directory)
        yield
    finally:
        os.chdir(curdir)

def run_django_command(ctx, cmd, opts=None, hide=None):
    """Run arbitrary Django command."""
    opts = opts or []
    if hasattr(ctx, 'local'):
        python = os.path.join(DST_PATH, 'venv', 'bin', 'python3')
        cd = ctx.cd
        path = DST_PATH
        replace_env = True
    else:
        python = 'python3'
        cd = chdir
        path = get_proj_path()
        replace_env = False

    with cd(path):
        return ctx.run(
            ' '.join([python, 'src/manage.py', cmd] + opts),
            hide=hide,
            replace_env=replace_env,
            env={'DJANGO_ENV': 'production'},
        )

@task
def migrate(ctx, display=False, no_initial_data=False,
            fake=False, fake_initial=False):
    """Migrate all installed Django apps."""
    opts = ['--noinput']
    if display:
        opts.append('--list')
    else:
        # All other options should be parsed here
        # (do not combine them with --list)
        if no_initial_data:
            opts.append('--no-initial-data')
        if fake:
            opts.append('--fake')
        if fake_initial:
            opts.append('--fake-initial')

    run_django_command(ctx, 'migrate', opts=opts)

max-arnold commented Aug 18, 2018

Keeping PATH intact is important for me, because the task is designed to be invoked both remotely and locally (i.e. fab -H host migrate or fab migrate). Below is my current solution (quite verbose, you may find it useful for improving Fabric API):

@contextlib.contextmanager
def chdir(directory):
    """Context manager to change current directory."""
    curdir = os.getcwd()
    try:
        os.chdir(directory)
        yield
    finally:
        os.chdir(curdir)

def run_django_command(ctx, cmd, opts=None, hide=None):
    """Run arbitrary Django command."""
    opts = opts or []
    if hasattr(ctx, 'local'):
        python = os.path.join(DST_PATH, 'venv', 'bin', 'python3')
        cd = ctx.cd
        path = DST_PATH
        replace_env = True
    else:
        python = 'python3'
        cd = chdir
        path = get_proj_path()
        replace_env = False

    with cd(path):
        return ctx.run(
            ' '.join([python, 'src/manage.py', cmd] + opts),
            hide=hide,
            replace_env=replace_env,
            env={'DJANGO_ENV': 'production'},
        )

@task
def migrate(ctx, display=False, no_initial_data=False,
            fake=False, fake_initial=False):
    """Migrate all installed Django apps."""
    opts = ['--noinput']
    if display:
        opts.append('--list')
    else:
        # All other options should be parsed here
        # (do not combine them with --list)
        if no_initial_data:
            opts.append('--no-initial-data')
        if fake:
            opts.append('--fake')
        if fake_initial:
            opts.append('--fake-initial')

    run_django_command(ctx, 'migrate', opts=opts)
@bitprophet

This comment has been minimized.

Show comment
Hide comment
@bitprophet

bitprophet Aug 22, 2018

Member

To clarify, all I meant was that it might make more sense to limit your specification for the remote env to some whitelisted set of variables such as $PATH, instead of leaving replace_env=False and thus trying to use the entire local env. There's nothing inherently wrong with replicating the local PATH to the remote end, I'm just saying, do only that, explicitly. E.g. something like run(xxx, env={'PATH': os.environ['PATH']}, replace_env=True).

Not only is that more secure when hitting remote targets but it will avoid the escaping issues I think you're hitting (as PATH is less likely to have shell meta-chars in it than other things like $PS1).

This will become partly moot when we fix the local-vs-remote config issue, after which point replace_env will default to True remotely but False locally.

Member

bitprophet commented Aug 22, 2018

To clarify, all I meant was that it might make more sense to limit your specification for the remote env to some whitelisted set of variables such as $PATH, instead of leaving replace_env=False and thus trying to use the entire local env. There's nothing inherently wrong with replicating the local PATH to the remote end, I'm just saying, do only that, explicitly. E.g. something like run(xxx, env={'PATH': os.environ['PATH']}, replace_env=True).

Not only is that more secure when hitting remote targets but it will avoid the escaping issues I think you're hitting (as PATH is less likely to have shell meta-chars in it than other things like $PS1).

This will become partly moot when we fix the local-vs-remote config issue, after which point replace_env will default to True remotely but False locally.

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