Skip to content

Commit a0567ea

Browse files
author
Matthew Fisher
authored
fix(api): remove command escaping from v1 (#822)
This code used to be necessary to prevent systemd abuse. Now that kubernetes handles the escaping for us, there is no need to be super defensive in our own code.
1 parent 4f3117d commit a0567ea

4 files changed

Lines changed: 123 additions & 98 deletions

File tree

‎rootfs/api/models/app.py‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -133,37 +133,45 @@ def _get_job_id(self, container_type):
133133
return "{app}-{version}-{container_type}".format(**locals())
134134

135135
def _get_command(self, container_type):
136+
"""
137+
Return the kubernetes "container arguments" to be sent off to the scheduler.
138+
139+
In reality this is the command that the user it attempting to run.
140+
"""
136141
try:
137-
# if this is not procfile-based app, ensure they cannot break out
138-
# and run arbitrary commands on the host
139142
# FIXME: remove slugrunner's hardcoded entrypoint
140143
release = self.release_set.latest()
141144
if release.build.dockerfile or not release.build.sha:
142-
return "bash -c '{}'".format(release.build.procfile[container_type])
145+
return [release.build.procfile[container_type]]
143146

144-
return 'start {}'.format(container_type)
147+
return ['start', container_type]
145148
# if the key is not present or if a parent attribute is None
146149
except (KeyError, TypeError, AttributeError):
147150
# handle special case for Dockerfile deployments
148-
return '' if container_type == 'cmd' else 'start {}'.format(container_type)
151+
return [] if container_type == 'cmd' else ['start', container_type]
149152

150-
def _get_command_run(self, command):
151-
# SECURITY: shell-escape user input
152-
command = command.replace("'", "'\\''")
153+
def _get_entrypoint(self, container_type):
154+
"""
155+
Return the kubernetes "container command" to be sent off to the scheduler.
156+
157+
In this case, it is the entrypoint for the docker image. Because of Heroku compatibility,
158+
Any containers that are not from a buildpack are run under /bin/bash.
159+
"""
160+
# handle special case for Dockerfile deployments
161+
if container_type == 'cmd':
162+
return []
153163

154164
# if this is a procfile-based app, switch the entrypoint to slugrunner's default
155165
# FIXME: remove slugrunner's hardcoded entrypoint
156166
release = self.release_set.latest()
157167
if release.build.procfile and \
158168
release.build.sha and not \
159169
release.build.dockerfile:
160-
entrypoint = '/runner/init'
161-
command = "'{}'".format(command)
170+
entrypoint = ['/runner/init']
162171
else:
163-
entrypoint = '/bin/bash'
164-
command = "-c '{}'".format(command)
172+
entrypoint = ['/bin/bash', '-c']
165173

166-
return entrypoint, command
174+
return entrypoint
167175

168176
def log(self, message, level=logging.INFO):
169177
"""Logs a message in the context of this application.
@@ -436,13 +444,13 @@ def _scale_pods(self, scale_types):
436444
'deploy_timeout': deploy_timeout,
437445
}
438446

439-
command = self._get_command(scale_type)
440447
try:
441448
self._scheduler.scale(
442449
namespace=self.id,
443450
name=self._get_job_id(scale_type),
444451
image=release.image,
445-
command=command,
452+
entrypoint=self._get_entrypoint(scale_type),
453+
command=self._get_command(scale_type),
446454
**kwargs
447455
)
448456
except Exception as e:
@@ -523,6 +531,7 @@ def deploy(self, release, force_deploy=False):
523531
namespace=self.id,
524532
name=self._get_job_id(scale_type),
525533
image=release.image,
534+
entrypoint=self._get_entrypoint(scale_type),
526535
command=self._get_command(scale_type),
527536
**kwargs
528537
)
@@ -674,17 +683,15 @@ def pod_name(size=5, chars=string.ascii_lowercase + string.digits):
674683
return ''.join(random.choice(chars) for _ in range(size))
675684

676685
"""Run a one-off command in an ephemeral app container."""
686+
scale_type = 'run'
677687
release = self.release_set.latest()
678688
if release.build is None:
679689
raise DeisException('No build associated with this release to run this command')
680690

681691
# see if the app config has deploy timeout preference, otherwise use global
682692
deploy_timeout = release.config.values.get('DEIS_DEPLOY_TIMEOUT', settings.DEIS_DEPLOY_TIMEOUT) # noqa
683693

684-
# TODO: add support for interactive shell
685-
entrypoint, command = self._get_command_run(command)
686-
687-
name = self._get_job_id('run') + '-' + pod_name()
694+
name = self._get_job_id(scale_type) + '-' + pod_name()
688695
self.log("{} on {} runs '{}'".format(user.username, name, command))
689696

690697
kwargs = {
@@ -703,8 +710,8 @@ def pod_name(size=5, chars=string.ascii_lowercase + string.digits):
703710
self.id,
704711
name,
705712
release.image,
706-
entrypoint,
707-
command,
713+
self._get_entrypoint(scale_type),
714+
[command],
708715
**kwargs
709716
)
710717

‎rootfs/api/tests/deployments/test_pods.py‎

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -526,31 +526,31 @@ def test_command_good(self, mock_requests):
526526
)
527527

528528
# use `start web` for backwards compatibility with slugrunner
529-
self.assertEqual(app._get_command('web'), 'start web')
530-
self.assertEqual(app._get_command('worker'), 'start worker')
529+
self.assertEqual(app._get_command('web'), ['start', 'web'])
530+
self.assertEqual(app._get_command('worker'), ['start', 'worker'])
531531

532532
# switch to docker image app
533533
build.sha = ''
534534
build.save()
535-
self.assertEqual(app._get_command('web'), "bash -c 'node server.js'")
535+
self.assertEqual(app._get_command('web'), ["node server.js"])
536536

537537
# switch to dockerfile app
538538
build.sha = 'european-swallow'
539539
build.dockerfile = 'dockerdockerdocker'
540540
build.save()
541-
self.assertEqual(app._get_command('web'), "bash -c 'node server.js'")
542-
self.assertEqual(app._get_command('cmd'), '')
541+
self.assertEqual(app._get_command('web'), ["node server.js"])
542+
self.assertEqual(app._get_command('cmd'), [])
543543

544544
# ensure we can override the cmd process type in a Procfile
545545
build.procfile['cmd'] = 'node server.js'
546546
build.save()
547-
self.assertEqual(app._get_command('cmd'), "bash -c 'node server.js'")
548-
self.assertEqual(app._get_command('worker'), "bash -c 'node worker.js'")
547+
self.assertEqual(app._get_command('cmd'), ["node server.js"])
548+
self.assertEqual(app._get_command('worker'), ["node worker.js"])
549549

550550
# for backwards compatibility if no Procfile is supplied
551551
build.procfile = {}
552552
build.save()
553-
self.assertEqual(app._get_command('worker'), 'start worker')
553+
self.assertEqual(app._get_command('worker'), ['start', 'worker'])
554554

555555
def test_run_command_good(self, mock_requests):
556556
"""Test the run command for each container workflow"""
@@ -587,9 +587,8 @@ def test_run_command_good(self, mock_requests):
587587
body = {'command': 'echo hi'}
588588
response = self.client.post(url, body)
589589
self.assertEqual(response.status_code, 200, response.data)
590-
data = App.objects.get(id=app_id)
591-
entrypoint, _ = data._get_command_run('echo hi')
592-
self.assertEqual(entrypoint, '/bin/bash')
590+
app = App.objects.get(id=app_id)
591+
self.assertEqual(app._get_entrypoint('web'), ['/bin/bash', '-c'])
593592

594593
# docker image workflow
595594
build.dockerfile = ''
@@ -599,9 +598,8 @@ def test_run_command_good(self, mock_requests):
599598
body = {'command': 'echo hi'}
600599
response = self.client.post(url, body)
601600
self.assertEqual(response.status_code, 200, response.data)
602-
data = App.objects.get(id=app_id)
603-
entrypoint, _ = data._get_command_run('echo hi')
604-
self.assertEqual(entrypoint, '/bin/bash')
601+
app = App.objects.get(id=app_id)
602+
self.assertEqual(app._get_entrypoint('cmd'), [])
605603

606604
# procfile workflow
607605
build.sha = 'somereallylongsha'
@@ -610,9 +608,8 @@ def test_run_command_good(self, mock_requests):
610608
body = {'command': 'echo hi'}
611609
response = self.client.post(url, body)
612610
self.assertEqual(response.status_code, 200, response.data)
613-
data = App.objects.get(id=app_id)
614-
entrypoint, _ = data._get_command_run('echo hi')
615-
self.assertEqual(entrypoint, '/runner/init')
611+
app = App.objects.get(id=app_id)
612+
self.assertEqual(app._get_entrypoint('web'), ['/runner/init'])
616613

617614
def test_run_not_fail_on_debug(self, mock_requests):
618615
"""
@@ -654,9 +651,8 @@ def test_run_not_fail_on_debug(self, mock_requests):
654651
body = {'command': 'echo hi'}
655652
response = self.client.post(url, body)
656653
self.assertEqual(response.status_code, 200, response.data)
657-
data = App.objects.get(id=app_id)
658-
entrypoint, _ = data._get_command_run('echo hi')
659-
self.assertEqual(entrypoint, '/bin/bash')
654+
app = App.objects.get(id=app_id)
655+
self.assertEqual(app._get_entrypoint('web'), ['/bin/bash', '-c'])
660656

661657
def test_scaling_does_not_add_run_proctypes_to_structure(self, mock_requests):
662658
"""Test that app info doesn't show transient "run" proctypes."""

‎rootfs/api/tests/test_pods.py‎

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -524,31 +524,31 @@ def test_command_good(self, mock_requests):
524524
)
525525

526526
# use `start web` for backwards compatibility with slugrunner
527-
self.assertEqual(app._get_command('web'), 'start web')
528-
self.assertEqual(app._get_command('worker'), 'start worker')
527+
self.assertEqual(app._get_command('web'), ['start', 'web'])
528+
self.assertEqual(app._get_command('worker'), ['start', 'worker'])
529529

530530
# switch to docker image app
531531
build.sha = ''
532532
build.save()
533-
self.assertEqual(app._get_command('web'), "bash -c 'node server.js'")
533+
self.assertEqual(app._get_command('web'), ["node server.js"])
534534

535535
# switch to dockerfile app
536536
build.sha = 'european-swallow'
537537
build.dockerfile = 'dockerdockerdocker'
538538
build.save()
539-
self.assertEqual(app._get_command('web'), "bash -c 'node server.js'")
540-
self.assertEqual(app._get_command('cmd'), '')
539+
self.assertEqual(app._get_command('web'), ["node server.js"])
540+
self.assertEqual(app._get_command('cmd'), [])
541541

542542
# ensure we can override the cmd process type in a Procfile
543543
build.procfile['cmd'] = 'node server.js'
544544
build.save()
545-
self.assertEqual(app._get_command('cmd'), "bash -c 'node server.js'")
546-
self.assertEqual(app._get_command('worker'), "bash -c 'node worker.js'")
545+
self.assertEqual(app._get_command('cmd'), ["node server.js"])
546+
self.assertEqual(app._get_command('worker'), ["node worker.js"])
547547

548548
# for backwards compatibility if no Procfile is supplied
549549
build.procfile = {}
550550
build.save()
551-
self.assertEqual(app._get_command('worker'), 'start worker')
551+
self.assertEqual(app._get_command('worker'), ['start', 'worker'])
552552

553553
def test_run_command_good(self, mock_requests):
554554
"""Test the run command for each container workflow"""
@@ -585,9 +585,8 @@ def test_run_command_good(self, mock_requests):
585585
body = {'command': 'echo hi'}
586586
response = self.client.post(url, body)
587587
self.assertEqual(response.status_code, 200, response.data)
588-
data = App.objects.get(id=app_id)
589-
entrypoint, _ = data._get_command_run('echo hi')
590-
self.assertEqual(entrypoint, '/bin/bash')
588+
app = App.objects.get(id=app_id)
589+
self.assertEqual(app._get_entrypoint('web'), ['/bin/bash', '-c'])
591590

592591
# docker image workflow
593592
build.dockerfile = ''
@@ -597,9 +596,8 @@ def test_run_command_good(self, mock_requests):
597596
body = {'command': 'echo hi'}
598597
response = self.client.post(url, body)
599598
self.assertEqual(response.status_code, 200, response.data)
600-
data = App.objects.get(id=app_id)
601-
entrypoint, _ = data._get_command_run('echo hi')
602-
self.assertEqual(entrypoint, '/bin/bash')
599+
app = App.objects.get(id=app_id)
600+
self.assertEqual(app._get_entrypoint('cmd'), [])
603601

604602
# procfile workflow
605603
build.sha = 'somereallylongsha'
@@ -608,9 +606,8 @@ def test_run_command_good(self, mock_requests):
608606
body = {'command': 'echo hi'}
609607
response = self.client.post(url, body)
610608
self.assertEqual(response.status_code, 200, response.data)
611-
data = App.objects.get(id=app_id)
612-
entrypoint, _ = data._get_command_run('echo hi')
613-
self.assertEqual(entrypoint, '/runner/init')
609+
app = App.objects.get(id=app_id)
610+
self.assertEqual(app._get_entrypoint('web'), ['/runner/init'])
614611

615612
def test_run_not_fail_on_debug(self, mock_requests):
616613
"""
@@ -652,9 +649,8 @@ def test_run_not_fail_on_debug(self, mock_requests):
652649
body = {'command': 'echo hi'}
653650
response = self.client.post(url, body)
654651
self.assertEqual(response.status_code, 200, response.data)
655-
data = App.objects.get(id=app_id)
656-
entrypoint, _ = data._get_command_run('echo hi')
657-
self.assertEqual(entrypoint, '/bin/bash')
652+
app = App.objects.get(id=app_id)
653+
self.assertEqual(app._get_entrypoint('web'), ['/bin/bash', '-c'])
658654

659655
def test_scaling_does_not_add_run_proctypes_to_structure(self, mock_requests):
660656
"""Test that app info doesn't show transient "run" proctypes."""

0 commit comments

Comments
 (0)