Skip to content

Commit

Permalink
Fix command quoting issues
Browse files Browse the repository at this point in the history
The command the container is supposed to run is being passed to
pipes.quote() once in get_docker_run_cmd() and once again in
run_docker_container().

If the string contains anything that pipes.quote thinks should be
quoted then we end up with some very weird results. For example
"echo 'Hello' && sleep 60" turns to "echo foo '"'"'&&'"'"' sleep 60".

This gets weirder when the command passed to us is already in the form
'''/bin/sh -c '/bin/echo "docker docker docker"' ''' because then we
prepend /bin/sh in front of it and quoting gets *very* ugly.
  • Loading branch information
Kyriakos Oikonomakos committed Jun 15, 2016
1 parent 1beac8a commit b9ef45f
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 8 deletions.
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ dist/
.pip/
*.py?
.*.sw?
.idea
paasta_tools.egg-info/
docs/man/
tags
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,10 @@ chronos_job:
mem: 100
disk: 300
schedule: 'R/2015-08-14T10:00:00+00:00/PT10M'

chronos_job_with_cmd:
cpus: .1
mem: 100
disk: 300
schedule: 'R/2015-08-14T10:00:00+00:00/PT10M'
cmd: 'echo hello && sleep 5 && exit 42'
6 changes: 6 additions & 0 deletions general_itests/local_run.feature
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,9 @@ Feature: paasta local-run can be used
And a simple service to test
When we run paasta local-run in non-interactive mode on a chronos job
Then we should see the expected return code

Scenario: Running paasta local-run in non-interactive mode on a Chronos job with a complex command
Given Docker is available
And a simple service to test
When we run paasta local-run in non-interactive mode on a chronos job with cmd set to 'echo hello && sleep 5'
Then we should see the expected return code
14 changes: 13 additions & 1 deletion general_itests/steps/local_run_steps.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def non_interactive_local_run(context, var, val):
"--service fake_simple_service "
"--cluster test-cluster "
"--build "
"--cmd '/bin/sh -c \"echo \"%s=$%s\" && sleep 2s && exit 42\"'" % (var, val))
'''--cmd '/bin/sh -c "echo \\"%s=$%s\\" && sleep 2s && exit 42"' ''' % (var, val))
context.local_run_return_code, context.local_run_output = _run(command=localrun_cmd, timeout=30)


Expand Down Expand Up @@ -76,3 +76,15 @@ def local_run_on_chronos_job(context):
"--build "
"--cmd '/bin/sh -c \"sleep 2s && exit 42\"'")
context.local_run_return_code, context.local_run_output = _run(command=local_run_cmd, timeout=30)


@when(u'we run paasta local-run in non-interactive mode on a chronos job with cmd set to \'echo hello && sleep 5\'')
def local_run_on_chronos_job_with_cmd(context):
with Path("fake_simple_service"):
local_run_cmd = ("paasta local-run "
"--yelpsoa-config-root ../fake_soa_configs_local_run/ "
"--service fake_simple_service "
"--cluster test-cluster "
"--instance chronos_job_with_cmd "
"--build ")
context.local_run_return_code, context.local_run_output = _run(command=local_run_cmd, timeout=30)
11 changes: 5 additions & 6 deletions paasta_tools/cli/cmds/local_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -406,9 +406,9 @@ def get_docker_run_cmd(memory, random_port, container_name, volumes, env, intera
cmd.append('%s' % docker_hash)
if command:
cmd.extend((
'sh', '-c',
' '.join(pipes.quote(part) for part in command)
'sh', '-c'
))
cmd.append(pipes.quote(' '.join(command)))
return cmd


Expand Down Expand Up @@ -517,8 +517,7 @@ def run_docker_container(
net=net,
docker_params=docker_params,
)
# http://stackoverflow.com/questions/4748344/whats-the-reverse-of-shlex-split
joined_docker_run_cmd = ' '.join(pipes.quote(word) for word in docker_run_cmd)
joined_docker_run_cmd = ' '.join(docker_run_cmd)
healthcheck_mode, healthcheck_data = get_healthcheck_for_instance(
service, instance, instance_config, random_port, soa_dir=soa_dir)

Expand Down Expand Up @@ -689,12 +688,12 @@ def configure_and_run_docker_container(
if args.interactive is True and args.cmd is None:
command = ['bash']
elif args.cmd:
command = shlex.split(args.cmd)
command = shlex.split(args.cmd, posix=False)
else:
command_from_config = instance_config.get_cmd()
if command_from_config:
command_modifier = command_function_for_framework(instance_type)
command = shlex.split(command_modifier(command_from_config))
command = shlex.split(command_modifier(command_from_config), posix=False)
else:
command = instance_config.get_args()

Expand Down
2 changes: 1 addition & 1 deletion tests/cli/test_cmds_local_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -543,7 +543,7 @@ def test_get_docker_run_cmd_interactive_false():
assert '--interactive=true' not in actual
assert '--tty=true' not in actual
assert docker_hash in actual
assert ' '.join(pipes.quote(part) for part in command) in actual
assert ' '.join(pipes.quote(part) for part in command) in ' '.join(actual)


def test_get_docker_run_cmd_interactive_true():
Expand Down

0 comments on commit b9ef45f

Please sign in to comment.