Skip to content

Require dns+nft egress mode for OpenSandbox network policies - #74504

Merged
kaxil merged 2 commits into
apache:mainfrom
zozo123:docs/opensandbox-egress-nft
Oct 13, 2026
Merged

kaxil merged 2 commits into
apache:mainfrom
zozo123:docs/opensandbox-egress-nft

Conversation

@zozo123

@zozo123 zozo123 commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Behaviour change. On an OpenSandbox server with the default [egress] mode = "dns", OpenSandboxBackend now refuses a deny-all or allowlist SandboxSpec at create, including the default SandboxSpec(). The error names the fix: set [egress] mode = "dns+nft" in the server config, or pass SandboxSpec(block_network=False) for a sandbox that may reach the network. The backend is Experimental, so the stability policy allows this in a minor release. The changelog note under the Changelog header announces it, as stability.rst requires.

Why. dns is the server's default (EgressConfig.mode) and is what every example config ships. In that mode the egress sidecar filters name resolution and nothing else, so a "no network" sandbox can still connect to any numeric address. _verify_network_policy passed anyway, because SandboxSync.get_egress_policy() returns the same rules in dns and dns+nft mode. That breaks the SandboxSpec contract that a backend which cannot enforce a field must raise rather than silently ignore it.

Release context. apache-airflow-providers-common-ai 0.11.0 (RC vote #74459, ends 2026-10-11 14:47 UTC) ships the backend without this check. The caveat is on the RC thread: #74459 (comment). This lands in the next release.

Two questions a reviewer will ask:

  • Why use the SDK's generated opensandbox.api.egress client? The sidecar's GET /policy response carries enforcementMode, but the high-level get_egress_policy() keeps only the rules and drops that field.
  • Why a separate client? It uses a separately owned SDK transport with the connection's retry policy, so proxy environment variables do not redirect read-back and transient 502/503 responses are retried. The client closes only its own transport. OpenSandbox 1.1.0 has no new_owned_transport(), so this uses config.model_copy(update={"transport": None}).with_transport_if_missing().transport. Non-200 responses go through the SDK's own handle_api_error and require_parsed, so an HTTP error is reported as one. A missing or unreadable mode also fails closed.

allow_egress_to_cidrs is still refused. Its message no longer claims that the mode cannot be read. Supporting CIDRs is possible now, but it needs Modal-style entry validation, so it is left for a follow-up.

Measured against OpenSandbox server 0.2.3 with egress sidecar v1.1.7, running on Docker (colima), with opensandbox==1.1.0. Each sandbox was created through OpenSandboxBackend.create().

What a sandbox can reach with the backend as it is on main, which is byte-identical to 0.11.0rc1:

From inside the sandbox dns, deny-all dns, allow pypi.org dns+nft, deny-all dns+nft, allow pypi.org
TCP 1.1.1.1:443, 8.8.8.8:443 connects connects times out times out
HTTP GET 1.1.1.1:80 301 from Cloudflare 301 from Cloudflare times out times out
HTTPS HEAD pypi.org/simple/ fails (no DNS) 200 fails (no DNS) 200

In another dns-mode deny-all sandbox created the same way, the system resolver could not look up example.com. A DNS-over-HTTPS query to https://1.1.1.1/dns-query returned its address, and GET https://example.com/ over that address returned 200 OK with a verified certificate. So the DNS-only filter is easy to get around.

With this PR:

  • dns+nft server: deny-all and allow_egress_to=["pypi.org"] are accepted, with the same reachability as the table above. The system test example_sandbox_toolset_opensandbox.py succeeds in 7.30 s. On 2026-10-08, against dns+nft with 0.11.0rc1, the timeout, kill and cleanup edge cases also passed.
  • dns server: deny-all is refused after 2.6 s and the allowlist after 2.0 s, each with the error above. No sandbox container is left behind.

Validation after review: all 85 OpenSandbox tests pass under Python 3.12 with opensandbox==1.1.0; targeted mypy passes on source and tests; fast prek checks pass. The fixture now patches HTTPTransport.handle_request, exercising transport, retries, generated client and parsing. New cases cover proxy environment variables, transient 502/503 responses and closing the owned transport without closing the sandbox's shared transport. All three new cases fail when the transport argument is removed.

A missing enforcement mode now advises upgrading the egress sidecar to v1.0.1 or later. Configuration docs describe creation followed by destruction and task failure. The changelog conflict is resolved against current main.

Full manual prek and documentation builds were attempted but stopped during a fresh Python 3.11 CI-image build; they remain unverified locally. Targeted tests and type checks used the existing Python 3.12 CI image.

related: #71676, #74459


Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

Generated-by: Claude Code (Opus 5.5) and Codex (GPT-6) following the guidelines


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

Comment thread providers/common/ai/src/airflow/providers/common/ai/sandbox/opensandbox.py Outdated
Comment thread providers/common/ai/docs/sandbox/configuration.rst Outdated
zozo123 and others added 2 commits October 12, 2026 16:30
The OpenSandbox server runs its egress sidecar in "dns" mode by default
(EgressConfig.mode, and every example configuration), where it filters
name resolution and nothing else. A sandbox created from a default
SandboxSpec() could therefore still connect to any numeric address,
including a DNS-over-HTTPS resolver, while the backend's read-back passed,
because get_egress_policy() returns the same rules in "dns" and "dns+nft"
mode. The sidecar's own /policy response names the mode, so the backend
can keep the deny-by-default contract SandboxSpec promises instead of
leaving it to a server setting nobody is told about.
@zozo123
zozo123 force-pushed the docs/opensandbox-egress-nft branch from b6cb55f to 1c2d5fb Compare October 12, 2026 13:38
@kaxil
kaxil merged commit 304607f into apache:main Oct 13, 2026
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants