Skip to content

Reconcile network isolation with network mode#5794

Open
JAORMX wants to merge 2 commits into
mainfrom
fix-host-network-isolation
Open

Reconcile network isolation with network mode#5794
JAORMX wants to merge 2 commits into
mainfrom
fix-host-network-isolation

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator

Summary

Network isolation is a bridge-topology construct: --isolate-network builds a
per-workload internal-only network plus DNS (dnsmasq) and egress/ingress
(Squid) sidecars, and only works when the MCP container lives on a
Docker-managed bridge. Since isolation became the default in v0.30.1 (#5583),
running a server with --network host silently built those sidecars attached
only to the internal (no-route-out) network, so all DNS and outbound traffic
broke on the next re-create
— with no error at deploy time. --network none
hit the same path. A host-networked container also can't be forced through the
sidecars at all (it shares the host netns), so the isolation was both broken
and illusory.

Because isolation cannot be enforced for host/none/custom modes, this
drops it there instead of building broken sidecars — and never reports a
workload as successfully started with non-functional isolation.

  • Drop isolation for non-bridge modes, deciding how loudly by provenance:
    • host/custom, isolation on only by default → drop + warn (restores the
      pre-v0.30.1 behaviour host-mode servers relied on).
    • host/custom with an explicit --isolate-network=truefail fast with
      an error naming both exits (a genuinely contradictory request).
    • none → drop silently (debug); it is already maximally confined, so
      isolation is merely redundant, not a security reduction.
  • Enforced at three layers so no code path can slip through: the docker
    client (DeployWorkload, structural backstop for every caller including
    restart), the config builder (owns the warn-vs-error decision — the only
    place that knows whether the flag was explicit), and config load (ReadJSON,
    self-heals legacy persisted configs so thv list/inspect stop reporting
    isolation that isn't enforced).
  • Shared networking.IsBridgeMode predicate + shared log-message constants so
    the rule lives in one place.
  • Docs: new interaction section in docs/arch/05-runconfig-and-permissions.md
    and disclosure on the --network / --isolate-network flag help.

Scope is Docker/Podman only — the Kubernetes runtime ignores both fields, so
the operator is unaffected.

Closes #5775

Type of change

  • Bug fix

Test plan

  • Unit tests (task test)
  • Linting (task lint-fix)

New coverage: flag→builder explicitness wiring (cmd/thv/app/run_flags_test.go),
docker-layer assertion that no DNS/egress/ingress sidecars and no HTTP_PROXY
are created for host/none (pkg/container/docker/client_deploy_test.go),
builder reconciliation matrix and ReadJSON degrade (pkg/runner/*_test.go),
and the upgrade-path degrade (pkg/workloads/upgrade/applier_test.go).

Changes

File Change
pkg/container/docker/client.go Gate sidecars on isolate && IsBridgeMode; effective-isolation used for MCP/ingress/label
pkg/runner/config_builder.go reconcileNetworkIsolation (fail-fast vs degrade) + WithNetworkIsolationExplicit option
pkg/runner/config.go ReadJSON degrade for legacy persisted configs
pkg/networking/utilities.go Shared IsBridgeMode predicate + log-message constants
cmd/thv/app/run_flags.go Thread cmd.Flags().Changed("isolate-network"); --network help disclosure
pkg/workloads/upgrade/applier.go Comment: upgrade intentionally degrades (no fail-fast)
docs/arch/05-runconfig-and-permissions.md, docs/cli/thv_run.md Document the interaction

Does this introduce a user-facing change?

Yes. thv run --network host no longer silently loses outbound connectivity:
network isolation (on by default) is now dropped for host/none networking with
a warning, restoring the pre-v0.30.1 behaviour. Explicitly combining
--isolate-network=true with --network host now fails fast with an
explanatory error instead of starting a broken workload.

Implementation plan

Approved implementation plan

Reasoned from mechanics rather than patching the symptom:

  • Isolation is a bridge-topology construct — internal-only netns + interposed
    DNS/Squid sidecars; every enforcement mechanism requires the container on a
    Docker-managed bridge.
  • Network mode is the container's netns choicehost shares the host netns
    (bypasses all Docker network controls), none is loopback-only.
  • Choosing --network host doesn't mean the user waived isolation — it means
    isolation can't be enforced.
    So the default is really a bridge-mode
    default; for non-bridge modes it must be dropped, loudly only when dropping it
    reduces security.
mode --isolate-network source outcome
bridge/""/default any isolation as designed
host/custom default drop + warn
host/custom explicit true fail fast (error)
none any drop silently (debug)

Three defense-in-depth layers: docker-client structural gate (unbypassable,
covers restart) + creation-time policy (owns error-vs-warn via the explicit
bit) + load-time degrade (legacy persisted configs / display honesty).

Reviewed by a spec/standards/security/architecture/UX/DevEx/duplication/
library-reuse/QA panel; findings folded in (predicate + message consolidation,
message precision, flag-help disclosure, doc cross-refs, and the added tests).

Special notes for reviewers

🤖 Generated with Claude Code

Network isolation is a bridge-topology construct: it builds a
per-workload internal-only network plus DNS and egress/ingress Squid
sidecars, and only works when the container lives on a Docker-managed
bridge. Since isolation became the default (v0.30.1), running a server
with --network host silently built those sidecars attached only to the
internal network with no route out, so all DNS and outbound traffic
broke on the next re-create. --network none hit the same path.

Isolation cannot be enforced for host/none/custom modes, so drop it
there instead of building broken sidecars. How loudly depends on
provenance: warn when isolation was on only by default (restores the
pre-v0.30.1 behaviour host-mode servers relied on), fail fast when the
user explicitly passed --isolate-network=true (a genuinely
contradictory request). "none" is already maximally confined, so its
drop is silent. The rule is enforced at three layers so no path can
report a workload started with non-functional isolation: the docker
client (structural backstop for every caller), the config builder
(owns the warn-vs-error decision, the only place that knows whether the
flag was explicit), and config load (self-heals legacy persisted
configs on restart, which bypass the builder).

Docker/Podman only; the Kubernetes runtime ignores both fields.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/M Medium PR: 300-599 lines changed label Jul 14, 2026
@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.14815% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 70.85%. Comparing base (75b3ed7) to head (68021e2).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
pkg/container/docker/client.go 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5794      +/-   ##
==========================================
+ Coverage   70.76%   70.85%   +0.08%     
==========================================
  Files         685      685              
  Lines       69435    69484      +49     
==========================================
+ Hits        49139    49234      +95     
+ Misses      16701    16660      -41     
+ Partials     3595     3590       -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aponcedeleonch
aponcedeleonch previously approved these changes Jul 14, 2026
Copilot AI requested a review from jerm-dro as a code owner July 14, 2026 10:48
@github-actions github-actions Bot added size/M Medium PR: 300-599 lines changed and removed size/M Medium PR: 300-599 lines changed labels Jul 14, 2026
Copilot AI requested a review from aponcedeleonch July 14, 2026 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Medium PR: 300-599 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Default isolation breaks outbound traffic for --network host servers

3 participants