From 758c2b02a165c6b9dedffb83e8bfb4ca16a5568c Mon Sep 17 00:00:00 2001 From: Kyle McCormick Date: Thu, 29 Oct 2020 15:21:47 -0400 Subject: [PATCH] Pass course-run-specific resource limits to codejail The limits can be defined in settings.CODE_JAIL['limits_overrides'], which is a dict mapping context ids (generally, course run keys) to overrides to be applied to settings.CODE_JAIL['limits']. This will allow us to temporarily alter the codejail limits for certain contexts in order to enable, for example, a certain course run's tasks to run longer while a large exam's grades are recomputed. TNL-7649 --- cms/envs/common.py | 5 ++ common/lib/capa/capa/capa_problem.py | 5 +- common/lib/capa/capa/responsetypes.py | 4 + common/lib/capa/capa/safe_exec/safe_exec.py | 16 +++- .../capa/safe_exec/tests/test_safe_exec.py | 83 ++++++++++++++++++- common/lib/capa/capa/util.py | 31 +++++++ lms/djangoapps/debug/views.py | 11 ++- lms/envs/common.py | 6 ++ 8 files changed, 153 insertions(+), 8 deletions(-) diff --git a/cms/envs/common.py b/cms/envs/common.py index c67841df4b..ea9172e20f 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -911,6 +911,11 @@ CODE_JAIL = { # Needs to be non-zero so that jailed code can use it as their temp directory.(1MiB in bytes) 'FSIZE': 1048576, }, + + # Overrides to default configurable 'limits' (above). + # Keys should be course run ids. + # Values should be dictionaries that look like 'limits'. + "limit_overrides": {}, } # Some courses are allowed to run unsafe code. This is a list of regexes, one diff --git a/common/lib/capa/capa/capa_problem.py b/common/lib/capa/capa/capa_problem.py index bce772e573..713222d322 100644 --- a/common/lib/capa/capa/capa_problem.py +++ b/common/lib/capa/capa/capa_problem.py @@ -33,7 +33,7 @@ import capa.responsetypes as responsetypes import capa.xqueue_interface as xqueue_interface from capa.correctmap import CorrectMap from capa.safe_exec import safe_exec -from capa.util import contextualize_text, convert_files_to_filenames +from capa.util import contextualize_text, convert_files_to_filenames, get_course_id_from_capa_module from openedx.core.djangolib.markup import HTML, Text from openedx.core.lib.edx_six import get_gettext from xmodule.stringify import stringify_children @@ -930,6 +930,9 @@ class LoncapaProblem(object): python_path=python_path, extra_files=extra_files, cache=self.capa_system.cache, + limit_overrides_context=get_course_id_from_capa_module( + self.capa_module + ), slug=self.problem_id, unsafely=self.capa_system.can_execute_unsafe_code(), ) diff --git a/common/lib/capa/capa/responsetypes.py b/common/lib/capa/capa/responsetypes.py index 7ca8e837d0..4d293599d5 100644 --- a/common/lib/capa/capa/responsetypes.py +++ b/common/lib/capa/capa/responsetypes.py @@ -57,6 +57,7 @@ from .util import ( convert_files_to_filenames, default_tolerance, find_with_default, + get_course_id_from_capa_module, get_inner_html_from_xpath, is_list_of_files ) @@ -487,6 +488,9 @@ class LoncapaResponse(six.with_metaclass(abc.ABCMeta, object)): globals_dict, python_path=self.context['python_path'], extra_files=self.context['extra_files'], + limit_overrides_context=get_course_id_from_capa_module( + self.capa_module + ), slug=self.id, random_seed=self.context['seed'], unsafely=self.capa_system.can_execute_unsafe_code(), diff --git a/common/lib/capa/capa/safe_exec/safe_exec.py b/common/lib/capa/capa/safe_exec/safe_exec.py index d5f5651f6f..80508f61c9 100644 --- a/common/lib/capa/capa/safe_exec/safe_exec.py +++ b/common/lib/capa/capa/safe_exec/safe_exec.py @@ -86,6 +86,7 @@ def safe_exec( python_path=None, extra_files=None, cache=None, + limit_overrides_context=None, slug=None, unsafely=False, ): @@ -109,11 +110,16 @@ def safe_exec( to cache the execution, taking into account the code, the values of the globals, and the random seed. + `limit_overrides_context` is an optional string to be used as a key on + the `settings.CODE_JAIL['limit_overrides']` dictionary in order to apply + context-specific overrides to the codejail execution limits. + If `limit_overrides_context` is omitted or not present in limit_overrides, + then use the default limits specified insettings.CODE_JAIL['limits']. + `slug` is an arbitrary string, a description that's meaningful to the caller, that will be used in log messages. If `unsafely` is true, then the code will actually be executed without sandboxing. - """ # Check the cache for a previous result. if cache: @@ -144,8 +150,12 @@ def safe_exec( # Run the code! Results are side effects in globals_dict. try: exec_fn( - code_prolog + LAZY_IMPORTS + code, globals_dict, - python_path=python_path, extra_files=extra_files, slug=slug, + code_prolog + LAZY_IMPORTS + code, + globals_dict, + python_path=python_path, + extra_files=extra_files, + limit_overrides_context=limit_overrides_context, + slug=slug, ) except SafeExecException as e: # Saving SafeExecException e in exception to be used later. diff --git a/common/lib/capa/capa/safe_exec/tests/test_safe_exec.py b/common/lib/capa/capa/safe_exec/tests/test_safe_exec.py index 15e37e869b..9f6952cbd3 100644 --- a/common/lib/capa/capa/safe_exec/tests/test_safe_exec.py +++ b/common/lib/capa/capa/safe_exec/tests/test_safe_exec.py @@ -10,8 +10,12 @@ import unittest import pytest import random2 as random import six -from codejail.jail_code import is_configured +from codejail import jail_code +from codejail.django_integration import ConfigureCodeJailMiddleware from codejail.safe_exec import SafeExecException +from django.conf import settings +from django.core.exceptions import MiddlewareNotUsed +from django.test import override_settings from six import text_type, unichr from six.moves import range @@ -79,7 +83,7 @@ class TestSafeExec(unittest.TestCase): class TestSafeOrNot(unittest.TestCase): def test_cant_do_something_forbidden(self): # Can't test for forbiddenness if CodeJail isn't configured for python. - if not is_configured("python"): + if not jail_code.is_configured("python"): pytest.skip() g = {} @@ -94,6 +98,81 @@ class TestSafeOrNot(unittest.TestCase): self.assertEqual(g['files'], os.listdir('/')) +class TestLimitConfiguration(unittest.TestCase): + """ + Test that resource limits can be configured and overriden via Django settings. + + We just test that the limits passed to `codejail` as we expect them to be. + Actual resource limiting tests are within the `codejail` package itself. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + + # Make a copy of codejail settings just for this test class. + # Set a global REALTIME limit of 100. + # Set a REALTIME limit override of 200 for a special course. + cls.test_codejail_settings = (getattr(settings, 'CODE_JAIL', None) or {}).copy() + cls.test_codejail_settings['limits'] = { + 'REALTIME': 100, + } + cls.test_codejail_settings['limit_overrides'] = { + 'course-v1:my+special+course': {'REALTIME': 200, 'NPROC': 30}, + } + cls.configure_codejail(cls.test_codejail_settings) + + @classmethod + def tearDownClass(cls): + super().tearDownClass() + + # Re-apply original configuration. + cls.configure_codejail(getattr(settings, 'CODE_JAIL', None) or {}) + + @staticmethod + def configure_codejail(codejail_settings): + """ + Given a `settings.CODE_JAIL` dictionary, apply it to the codejail package. + + We use the `ConfigureCodeJailMiddleware` that comes with codejail. + """ + with override_settings(CODE_JAIL=codejail_settings): + # To apply `settings.CODE_JAIL`, we just intialize an instance of the + # middleware class. We expect it to apply to changes, and then raise + # "MiddlewareNotUsed" to indicate that its work is done. + # This is exactly how the settings are applied in production (except the + # middleware is automatically initialized because it's an element of + # `settings.MIDDLEWARE`). + try: + ConfigureCodeJailMiddleware() + except MiddlewareNotUsed: + pass + + def test_effective_limits_reflect_configuration(self): + """ + Test that `get_effective_limits` returns configured limits with overrides + applied correctly. + """ + # REALTIME has been configured with a global limit. + # Check it with no overrides context. + assert jail_code.get_effective_limits()['REALTIME'] == 100 + + # Now check REALTIME with an overrides context that we haven't configured. + # Should be the same. + assert jail_code.get_effective_limits('random-context-name')['REALTIME'] == 100 + + # Now check REALTIME limit for a special course. + # It should be overriden. + assert jail_code.get_effective_limits('course-v1:my+special+course')['REALTIME'] == 200 + + # We haven't configured a limit for NPROC. + # It should use the codejail default. + assert jail_code.get_effective_limits()['NPROC'] == 15 + + # But we have configured an NPROC limit override for a special course. + assert jail_code.get_effective_limits('course-v1:my+special+course')['NPROC'] == 30 + + class DictCache(object): """A cache implementation over a simple dict, for testing.""" diff --git a/common/lib/capa/capa/util.py b/common/lib/capa/capa/util.py index a352ad764a..f7c9ee2f03 100644 --- a/common/lib/capa/capa/util.py +++ b/common/lib/capa/capa/util.py @@ -221,3 +221,34 @@ def remove_markup(html): u'Rock & Roll' """ return HTML(bleach.clean(html, tags=[], strip=True)) + + +def get_course_id_from_capa_module(capa_module): + """ + Extract a stringified course run key from a CAPA module (aka ProblemBlock). + + This is a bit of a hack. Its intended use is to allow us to pass the course id + (if available) to `safe_exec`, enabling course-run-specific resource limits + in the safe execution environment (codejail). + + Arguments: + capa_module (ProblemBlock|None) + + Returns: str|None + The stringified course run key of the module. + If not available, fall back to None. + """ + if not capa_module: + return None + try: + return str(capa_module.scope_ids.usage_id.course_key) + except (AttributeError, TypeError): + # AttributeError: + # If the capa module lacks scope ids or has unexpected scope ids, we + # would rather fall back to `None` than let an AttributeError be raised + # here. + # TypeError: + # Old Mongo usage keys lack a 'run' specifier, and may + # raise a type error when we try to serialize them into a course + # run key. This is tolerable because such course runs are deprecated. + return None diff --git a/lms/djangoapps/debug/views.py b/lms/djangoapps/debug/views.py index fb4fc8a31d..8a772128f6 100644 --- a/lms/djangoapps/debug/views.py +++ b/lms/djangoapps/debug/views.py @@ -17,7 +17,14 @@ from openedx.core.djangolib.markup import HTML @login_required @ensure_csrf_cookie def run_python(request): - """A page to allow testing the Python sandbox on a production server.""" + """ + A page to allow testing the Python sandbox on a production server. + + Runs in the override context "debug_run_python", so resource limits with come first from: + CODE_JAIL['limit_overrides']['debug_run_python'] + and then from: + CODE_JAIL['limits'] + """ if not request.user.is_staff: raise Http404 c = {} @@ -27,7 +34,7 @@ def run_python(request): py_code = c['code'] = request.POST.get('code') g = {} try: - safe_exec(py_code, g) + safe_exec(py_code, g, limit_overrides_context="debug_run_python") except Exception: # pylint: disable=broad-except c['results'] = traceback.format_exc() else: diff --git a/lms/envs/common.py b/lms/envs/common.py index b0cc63d4c1..397c886b68 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -1282,6 +1282,12 @@ CODE_JAIL = { 'REALTIME': 3, 'PROXY': 0, }, + + # Overrides to default configurable 'limits' (above). + # Keys should be course run ids (or, in the special case of code running + # on the /debug/run_python page, the key is 'debug_run_python'). + # Values should be dictionaries that look like 'limits'. + "limit_overrides": {}, } # Some courses are allowed to run unsafe code. This is a list of regexes, one