Repository navigation
fix: Novita ARG prefix collision and example sandbox leak #1235
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,10 @@ def _resolve_from_references(content: str) -> str: | |
| - ``FROM --platform=linux/amd64 python:3.10-slim`` likewise keeps the flag | ||
| as part of the name. Novita builds for its own platform, so the flag is | ||
| dropped rather than propagated. | ||
|
|
||
| Substitutions are applied longest-name-first so a short unbraced prefix | ||
| cannot corrupt a longer sibling (e.g. ``$BASE`` must not rewrite | ||
| ``$BASE_IMAGE`` / ``${BASE_IMAGE}`` into ``python:3.12_IMAGE``). | ||
| """ | ||
| arg_defaults: Dict[str, str] = {} | ||
| for line in content.split("\n"): | ||
|
|
@@ -138,6 +142,11 @@ def _resolve_from_references(content: str) -> str: | |
| if match: | ||
| arg_defaults[match.group("name")] = match.group("value") | ||
|
|
||
| # Longest names first: `$BASE` must not rewrite `$BASE_IMAGE` mid-token. | ||
| arg_items = sorted( | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct fix. Sorting ARG names longest-first guarantees Non-blocking / out of scope: this disambiguates only among declared ARGs. Since the substitution is still a blunt |
||
| arg_defaults.items(), key=lambda item: len(item[0]), reverse=True | ||
| ) | ||
|
|
||
| out: List[str] = [] | ||
| for line in content.split("\n"): | ||
| match = _FROM_RE.match(line) | ||
|
|
@@ -147,7 +156,7 @@ def _resolve_from_references(content: str) -> str: | |
|
|
||
| reference = match.group("rest").strip() | ||
| reference = re.sub(r"^(--platform=\S+\s*)+", "", reference).strip() | ||
| for name, value in arg_defaults.items(): | ||
| for name, value in arg_items: | ||
| reference = reference.replace(f"${{{name}}}", value) | ||
| reference = reference.replace(f"${name}", value) | ||
| out.append(f"FROM {reference}") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| """Every Novita example must release its sandbox if readiness never arrives.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import ast | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
| EXAMPLES_DIR = Path(__file__).resolve().parents[2] / "examples" | ||
| NOVITA_EXAMPLES = sorted(EXAMPLES_DIR.glob("novita_*.py")) | ||
|
|
||
|
|
||
| def _calls(node: ast.AST, name: str) -> bool: | ||
| return any( | ||
| isinstance(child, ast.Attribute) and child.attr == name | ||
| for child in ast.walk(node) | ||
| ) | ||
|
|
||
|
|
||
| def _guarded_by_stop_container(tree: ast.AST) -> list[ast.Try]: | ||
| return [ | ||
| node | ||
| for node in ast.walk(tree) | ||
| if isinstance(node, ast.Try) | ||
| and any(_calls(handler, "stop_container") for handler in node.finalbody) | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("path", NOVITA_EXAMPLES, ids=lambda path: path.name) | ||
| def test_wait_for_ready_runs_under_the_stop_container_guard(path: Path): | ||
| tree = ast.parse(path.read_text()) | ||
|
|
||
| if not _calls(tree, "wait_for_ready"): | ||
| pytest.skip(f"{path.name} does not wait for readiness") | ||
|
|
||
| guards = _guarded_by_stop_container(tree) | ||
| assert guards, f"{path.name} calls wait_for_ready without a stop_container finally" | ||
| assert any( | ||
| _calls(statement, "wait_for_ready") | ||
| for guard in guards | ||
| for statement in guard.body | ||
| ), ( | ||
| f"{path.name} calls wait_for_ready outside the try that stops the sandbox; " | ||
| "a readiness timeout would leak a paid sandbox" | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -862,6 +862,37 @@ def test_resolve_arg_in_from(self): | |
| assert "FROM python:3.11" in out | ||
| assert "${BASE_IMAGE}" not in out.split("\n")[1] | ||
|
|
||
| def test_resolve_prefix_colliding_unbraced_args(self): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. High-signal regression test: it fails on the old code (unbraced Minor: the braced |
||
| """Shorter ARG names must not corrupt longer sibling references. | ||
|
|
||
| Declaring ``ARG BASE`` before ``ARG BASE_IMAGE`` used to rewrite | ||
| ``FROM $BASE_IMAGE`` / ``FROM ${BASE_IMAGE}`` into | ||
| ``FROM python:3.12_IMAGE`` because ``$BASE`` is a prefix of | ||
| ``$BASE_IMAGE``. | ||
| """ | ||
| from openenv.core.containers.runtime.novita_provider import ( | ||
| _resolve_from_references, | ||
| ) | ||
|
|
||
| dockerfile = ( | ||
| "ARG BASE=python:3.12\n" | ||
| "ARG BASE_IMAGE=python:3.12-slim\n" | ||
| "FROM $BASE_IMAGE\n" | ||
| "RUN echo hi\n" | ||
| ) | ||
| out = _resolve_from_references(dockerfile) | ||
| assert "FROM python:3.12-slim" in out | ||
| assert "python:3.12_IMAGE" not in out | ||
|
|
||
| braced = ( | ||
| "ARG BASE=python:3.12\n" | ||
| "ARG BASE_IMAGE=python:3.12-slim\n" | ||
| "FROM ${BASE_IMAGE}\n" | ||
| ) | ||
| out_braced = _resolve_from_references(braced) | ||
| assert "FROM python:3.12-slim" in out_braced | ||
| assert "python:3.12_IMAGE" not in out_braced | ||
|
|
||
| def test_arg_after_first_from_not_in_scope(self): | ||
| from openenv.core.containers.runtime.novita_provider import ( | ||
| _resolve_from_references, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch on the leak. Moving
wait_for_readyinside thetrymeans a readiness timeout now reachesfinally: provider.stop_container()instead of leaving the sandbox alive until Novita's hard lifetime.start_containercorrectly stays outside thetry(nothing to stop if it fails), andstop_container()is idempotent (no-op when_sandbox is None), so the pre-existing second teardown via theTbench2Envcontext-manager exit remains harmless. This reinforces RFC 002's explicit-cleanup / S6 bounded-blast-radius invariants.