Repository navigation
feat!: remove support for local CodeJail - #222
MoisesGSalas wants to merge 5 commits into
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @MoisesGSalas! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
47fb1c9 to
025acae
Compare
|
Hi @feanil, i would like a little bit of advice and what would be the best way to handle this: A lot of the tests in here rely on running codejail locally to pass (51/814). If I configure an instance of codejail-service that listens to localhost:8550 and set Should we add an skip those fifty tests? Delete them? |
|
I haven't looked at the tests (the workflow run has expired) but I'm guessing that a lot of them exercise the codejail sandboxing functionality. It's probably worth removing a lot of those tests and relying on codejail-service's own test suite for that. (Maybe there are some tests we want to add on that side where coverage is missing.) In general, I would support doing a lot of mocking here with |
025acae to
65108b2
Compare
I rebased the branch so a fresh run should be available. From what I saw, lot of failures are using the unsafe decorator to test the xblock behavior they need to run it via codejail. I'm thinking of mocking |
db89221 to
1a03132
Compare
|
Hi @openedx/axim-engineering! Could someone take a look at this for us? @MoisesGSalas there are branch conflicts that have popped up. |
As part of openedx/openedx-platform#36639, CodeJail won't be able to be invoked directly by openedx-platform and instead the remote CodeJail REST service will be the only interface available. A large portion of the tests rely on executing the actual problem code so a small mock implementation of the remote CodeJail service is introduced. The mock is a wrapper around CodeJail's `not_safe_exec` and disregards any kind of sandboxing. Tests that validate sandboxing capabilities were removed considering the remote service has a more comprehensive testing suite.
a524e24 to
89093b8
Compare
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
A quick AI assisted review, let me know if I misunderstood anything, generally this looks good. What is the roll out plan for this? Since tutor doesn't ship with codejail, this change won't have a major impact on tutor deployments right?
@robrap I assume this change won't have a major impact for you either since you're already using the service?
Seems like the biggest impact would be on operators running outside of tutor who would need to configure the remote codejail service before they could use a bunch of basic problem block features? If that's the case, I want to make sure we document that very clearly and include it and guidance in the release notes.
| if mod_name not in original_modules: | ||
| del sys.modules[mod_name] | ||
| for archive_path in python_path: | ||
| if importer := sys.path_importer_cache[archive_path]: |
There was a problem hiding this comment.
sys.path_importer_cache[archive_path] raises KeyError whenever python_path is set and the executed code never imports from the archive. Python only creates that cache entry when an import actually walks the path entry. Reproduced on 89093b8d:
safe_exec("a = 1", {}, python_path=["python_lib.zip"],
extra_files=[("python_lib.zip", zip_bytes)])
KeyError: 'python_lib.zip'
That is a course with a python_lib.zip asset whose problem script does not import from it. Use sys.path_importer_cache.get(archive_path); the full suite still passes with it.
Keep the loop. It looks redundant next to the clear() below, but zipimporter.invalidate_caches() refreshes zipimport._zip_directory_cache, which the clear does not touch. Dropping the loop fails test_responsetypes.py::CustomResponseTest::test_python_lib_zip_is_available.
| Arguments: | ||
| data (dict): Payload with the same shape as the remote service request: | ||
| code, globals_dict, python_path, extra_files, | ||
| limit_overrides_context, slug, unsafely. |
There was a problem hiding this comment.
The docstring lists unsafely in the payload but the function never reads it; it always runs not_safe_exec. Drop it from the list, or honour it. Worth deciding rather than leaving implicit, because test_can_do_something_forbidden_if_run_unsafely was the only coverage of that flag and it goes away here.
There was a problem hiding this comment.
unsafely is not used in the utility function, but can be used in safe_exec which this is mocking.
There was a problem hiding this comment.
It can't be used in safe_exec any more either. codejail-service returns a 400 for
unsafely=true (codejail_service/apps/api/v0/views.py:203-205), and the mock here runs
not_safe_exec unconditionally, so a test that sets the flag passes here and would fail
against the real service. Leaving the docstring as a description of the payload is fine with
me; see my comment on the "unsafely": unsafely, line for the part that isn't.
| "extra_files": extra_files, | ||
| } | ||
| if not is_codejail_rest_service_enabled(): | ||
| raise ImproperlyConfigured("To make use of this feature configure a remote Codejail service.") |
There was a problem hiding this comment.
The annotation on ENABLE_CODEJAIL_REST_SERVICE (remote_exec.py:21-28) still describes it as an opt-in to run codejail "using a separate VM or container", which was true when the alternative was running it in process. There is no alternative now, so rewrite it to say what the toggle means today: off means capa Python execution raises. Name CODE_JAIL_REST_SERVICE_HOST in the message here too, so an operator who hits this knows what to set.
Separately, is the toggle worth keeping at all now that it only reports whether the service was configured? Not a change to make here if you keep it: SettingToggle.is_enabled() is bool(getattr(settings, self.name, self.default)) and openedx-platform assigns the setting explicitly at openedx/envs/common.py:2281, so removing it needs a platform PR in the same window. That PR also wants :2288, where CODE_JAIL_REST_SERVICE_REMOTE_EXEC still defaults to the xmodule.capa.safe_exec.remote_exec deprecation stub.
There was a problem hiding this comment.
I'm not quite sure what to do with that toggle. I added this exception to provide a clearer message when the service was not configured:
The previous message was more obscure, something like: [Errno 2] No such file or directory: 'TMPDIR=tmp'.
I would like to signal people that they must configure a remote service. Maybe we can set CODE_JAIL_REST_SERVICE_HOST to None by default and use that instead of the redundant toggle.
I can later open the PR to change the default for CODE_JAIL_REST_SERVICE_REMOTE_EXEC.
There was a problem hiding this comment.
Yea, using CODE_JAIL_REST_SERVICE_HOST as the switch works for me, and the exception message is a clear improvement over the TMPDIR=tmp one. Drop ENABLE_CODEJAIL_REST_SERVICE and is_codejail_rest_service_enabled() here and raise when the host is unset, with the message naming CODE_JAIL_REST_SERVICE_HOST.
The platform default at openedx/envs/common.py:2292 is http://127.0.0.1:8550, so until it is set to None the check never trips and operators get CodejailServiceUnavailable instead. If you open the platform PR for the CODE_JAIL_REST_SERVICE_REMOTE_EXEC default, it can cover this one too, and it should land with the xblocks-contrib pin bump.
|
@feanil: Confirming that we are on the new service. Thanks. Also, we are still on the non-extracted xblock. :) |
Delete unused constant test file
Delete unused UseUnsafeCodejail util decorator
Avoid failing when retrieving the importer cache
Remove unused cacheable variable
|
@feanil, i made a few fixes as requested.
Yes, this won't affect anyone running Tutor.
Yes, only people running the platform without Tutor and with local codejail will be affected. I would like a hand on documenting that process, mostly because as an operator I never performed the migration using the darklaunch feature 2U used. I've also only deployed openedx/codejail-service via Tutor, so i don't know well documented is the process for other environments. |
|
@robrap do you know if there is some good docs on the darklaunch feature or other cut-over docs that it would be useful to share with @MoisesGSalas |
feanil
left a comment
feanil
left a comment
There was a problem hiding this comment.
xblocks_contrib/problem/capa/safe_exec/README.rst is still unchanged. It still walks an operator through configuring local CodeJail, still tells them "you don't have to do anything to configure sandboxing if you don't want to, and everything will operate properly", and still documents CODE_JAIL['limits'], which this package no longer reads. Rewrite it to be accurate and where possible it can point to the codejail-service docs.
| @@ -1,31 +0,0 @@ | |||
| """Codejail controls for tests that execute capa problem code.""" | |||
There was a problem hiding this comment.
Sorry, I asked for this deletion without checking who imports it. openedx-platform master
(f7dc6ff8) imports it in three places:
xmodule/tests/test_capa_block.py:32lms/djangoapps/instructor_task/tests/test_integration.py:23lms/djangoapps/courseware/tests/test_submitting_problems.py:24
and xblocks_contrib/problem/capa/testing/__init__.py says this package ships so that
downstream suites can import from it. UseUnsafeCodejail itself can go, since it sets a codejail flag nothing reads any more. The mock is the part they need, and it's in safe_exec/tests/test_utils.py, which doesn't ship (pyproject.toml excludes *tests*).
Move the mock into this file as send_safe_exec_request_locally since this file ships with the package.
Then we can point CODE_JAIL_REST_SERVICE_REMOTE_EXEC in test_settings.py at xblocks_contrib.problem.capa.testing.codejail.send_safe_exec_request_locally. Platform can replace the @UseUnsafeCodejail() decorator on those three classes with override_settings(ENABLE_CODEJAIL_REST_SERVICE=True, CODE_JAIL_REST_SERVICE_REMOTE_EXEC=...) using the same path, in the same PR as the xblocks-contrib pin bump.
| "python_path": python_path, | ||
| "limit_overrides_context": limit_overrides_context, | ||
| "slug": slug, | ||
| "unsafely": unsafely, |
There was a problem hiding this comment.
unsafely can only fail now. codejail-service answers it with a 400 (codejail_service/apps/api/v0/views.py:203-205), which reaches the course as Codejail API Service invalid response., and the docstring at safe_exec.py:139 still promises unsandboxed execution.
It is also the one capa Python surface that works on stock Tutor today and doesn't after this change. Under Tutor's common_all.py config with ENABLE_CODEJAIL_REST_SERVICE = False:
origin/main unsafely=True -> OK a=2
unsafely=False -> FileNotFoundError: [Errno 2] No such file or directory: 'TMPDIR=tmp'
this branch both -> ImproperlyConfigured: To make use of this feature configure a remote Codejail service.
Operators with a non-empty COURSES_WITH_UNSAFE_CODE lose problem Python for those courses, so keep the parameter in this release and raise before the request when it is true. The message should say unsandboxed execution is gone, name COURSES_WITH_UNSAFE_CODE, and point at #36639 where the Replacement section covers per-course limits on the service. We can drop the parameter in a later release once operators have had time to fix their config. Fix the docstring, and put the removal in a BREAKING CHANGE: footer in the commit body.
| "code": code_prolog + LAZY_IMPORTS + code, | ||
| "globals_dict": globals_dict, | ||
| "python_path": python_path, | ||
| "limit_overrides_context": limit_overrides_context, |
There was a problem hiding this comment.
The docstring at safe_exec.py:130-134 still sends the reader to
settings.CODE_JAIL['limit_overrides'] and settings.CODE_JAIL['limits']. Nothing in this
package reads settings.CODE_JAIL any more; those limits are configured on codejail-service.
Point the docstring there.
| Arguments: | ||
| data (dict): Payload with the same shape as the remote service request: | ||
| code, globals_dict, python_path, extra_files, | ||
| limit_overrides_context, slug, unsafely. |
There was a problem hiding this comment.
It can't be used in safe_exec any more either. codejail-service returns a 400 for
unsafely=true (codejail_service/apps/api/v0/views.py:203-205), and the mock here runs
not_safe_exec unconditionally, so a test that sets the flag passes here and would fail
against the real service. Leaving the docstring as a description of the payload is fine with
me; see my comment on the "unsafely": unsafely, line for the part that isn't.
| "extra_files": extra_files, | ||
| } | ||
| if not is_codejail_rest_service_enabled(): | ||
| raise ImproperlyConfigured("To make use of this feature configure a remote Codejail service.") |
There was a problem hiding this comment.
Yea, using CODE_JAIL_REST_SERVICE_HOST as the switch works for me, and the exception message is a clear improvement over the TMPDIR=tmp one. Drop ENABLE_CODEJAIL_REST_SERVICE and is_codejail_rest_service_enabled() here and raise when the host is unset, with the message naming CODE_JAIL_REST_SERVICE_HOST.
The platform default at openedx/envs/common.py:2292 is http://127.0.0.1:8550, so until it is set to None the check never trips and operators get CodejailServiceUnavailable instead. If you open the platform PR for the CODE_JAIL_REST_SERVICE_REMOTE_EXEC default, it can cover this one too, and it should land with the xblocks-contrib pin bump.
6 participants
As part of openedx/openedx-platform#36639, CodeJail won't be able to be invoked directly by openedx-platform and instead the remote CodeJail REST service will be the only interface available.
A large portion of the tests rely on executing the actual problem code so a small mock implementation of the remote CodeJail service is introduced. The mock is a wrapper around CodeJail's
not_safe_execand disregards any kind of sandboxing. Tests that validate sandboxing capabilities were removed considering the remote service has a more comprehensive testing suite.Merge checklist:
Check off if complete or not applicable: