Skip to content

ci: allow Brev launchable bridge access#4492

Open
ericksoa wants to merge 3 commits into
mainfrom
ci/brev-bridge-firewall
Open

ci: allow Brev launchable bridge access#4492
ericksoa wants to merge 3 commits into
mainfrom
ci/brev-bridge-firewall

Conversation

@ericksoa
Copy link
Copy Markdown
Contributor

@ericksoa ericksoa commented May 29, 2026

Summary

  • add Docker bridge UFW allow rules to the CI Brev launchable startup script for OpenShell gateway (8080) and auth proxy (11435)
  • add a static workflow contract test so the bridge rules stay wired into the launchable path

Why

This mirrors the firewall fix proposed in brevdev/nemoclaw-image so we can validate the same Brev host-network shape from NemoClaw's trusted Brev E2E runner. The failure we are chasing is sandbox containers being unable to reach host.openshell.internal:8080 from the Docker bridge.

Validation

  • bash -n scripts/brev-launchable-ci-cpu.sh
  • shellcheck scripts/brev-launchable-ci-cpu.sh
  • npx vitest run --project cli test/brev-nightly-workflow.test.ts
  • git diff --check
  • npx @biomejs/biome check test/brev-nightly-workflow.test.ts (ignored by repo config; no files processed)

Signed-off-by: Aaron Erickson aerickson@nvidia.com

Summary by CodeRabbit

  • Chores

    • Added CI firewall configuration to expose the service gateway and auth proxy via the Docker bridge, applying network rules early during VM provisioning.
  • Tests

    • Added test coverage to verify the CI launch workflow establishes the intended Docker-bridge firewall rules and records the corresponding launch log messages.

Review Change Stack

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

E2E Advisor Recommendation

Required E2E: launchable-smoke-e2e, e2e-branch-validation-full
Optional E2E: messaging-providers, gpu-e2e

Dispatch hint: launchable-smoke-e2e

Auto-dispatched E2E: launchable-smoke-e2e via nightly-e2e.yaml at e6e127c055161ce43869635a41edac505cc3767enightly run

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • launchable-smoke-e2e (medium): Runs the launchable install-flow smoke path against scripts/brev-launchable-ci-cpu.sh, then onboards a sandbox, verifies gateway/sandbox health, and exercises live inference through inference.local. This is the closest existing merge-blocking coverage for the changed launchable bootstrap script and Docker bridge firewall behavior.
  • e2e-branch-validation-full (medium-high): Validates the branch on an ephemeral Brev CPU instance using the launchable setup path, then runs the full install/onboard/sandbox/inference suite. This is important because the changed script is specifically the Brev launchable CI startup script and firewall behavior may differ on real Brev VMs.

Optional E2E

  • messaging-providers (medium-high): Optional Brev branch-validation suite for provider/L7 proxy flows if maintainers want extra confidence that host-service firewall changes do not affect messaging credential/provider network reachability.
  • gpu-e2e (high): Optional only if the 11435 auth-proxy rule is intended to cover local Ollama/GPU-style inference paths; the changed file is CPU launchable, so this is useful adjacency coverage but not merge-blocking.

New E2E recommendations

  • Brev launchable firewall regression (medium): Existing launchable smoke validates end-to-end inference but does not explicitly enable/default-deny UFW and assert that a sandbox/container can reach both host service ports opened by the new rules, especially the Ollama auth-proxy port 11435.
    • Suggested test: Add a focused Brev launchable firewall E2E that enables UFW/default deny on the launchable VM, runs the startup script, then verifies Docker bridge/sandbox connectivity to the OpenShell gateway port 8080 and auth-proxy port 11435.

Dispatch hint

  • Workflow: nightly-e2e.yaml
  • jobs input: launchable-smoke-e2e

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

E2E Scenario Advisor Recommendation

Required scenario E2E: None
Optional scenario E2E: None

Workflow run

Full scenario advisor summary

E2E Scenario Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required scenario E2E

  • None. No scenario workflow, scenario metadata, scenario runtime, or validation-suite files changed.

Optional scenario E2E

  • None.

Relevant changed files

  • None.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 29, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 4c10ce5f-bfef-4495-9b82-6b64c4ddb6bd

📥 Commits

Reviewing files that changed from the base of the PR and between 91ab0b3 and e6e127c.

📒 Files selected for processing (1)
  • test/brev-nightly-workflow.test.ts

📝 Walkthrough

Walkthrough

Adds Docker bridge CIDR and host port constants and UFW helper functions to the Brev CI launch script, invokes firewall configuration early during Docker enablement, and adds a test that runs the helper with stubbed sudo/ufw/getent and asserts the emitted commands and log messages.

Changes

Docker bridge firewall configuration

Layer / File(s) Summary
Firewall constants and helpers
scripts/brev-launchable-ci-cpu.sh
Adds an environment overrides comment and defines DOCKER_BRIDGE_POOL_CIDR, OPENSHELL_GATEWAY_PORT, OLLAMA_AUTH_PROXY_PORT, implements allow_bridge_port and configure_openshell_bridge_firewall UFW helper logic.
Invoke firewall during Docker enablement
scripts/brev-launchable-ci-cpu.sh
Calls configure_openshell_bridge_firewall after making /var/run/docker.sock usable and logging Docker readiness.
Firewall execution and verification tests
test/brev-nightly-workflow.test.ts
Imports readFileSync, adds a test that stubs sudo, ufw, and getent, invokes configure_openshell_bridge_firewall, and asserts expected ufw allow commands and launch-log messages; includes writeExecutable helper update.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#3960: Both PRs add/verify ufw TCP allow rules from the Docker bridge pool to OPENSHELL_GATEWAY_PORT and OLLAMA_AUTH_PROXY_PORT.

Suggested labels

Platform: Brev, CI/CD, Docker, OpenShell, Networking, Sandbox, fix, status: rfr

Suggested reviewers

  • jyaunches
  • cv

Poem

🐰 I hopped through scripts at break of dawn,
adding bridges where the packets fawn.
UFW gates swing open, ports kept bright,
tests catch the logs and blink with delight.
A small rabbit cheers: the CI sleeps tight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The pull request title accurately describes the main change: adding UFW firewall rules to permit Docker bridge access to Brev launchable services (OpenShell gateway and Ollama auth proxy).
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/brev-bridge-firewall

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 29, 2026

PR Review Advisor

Findings: 0 needs attention, 6 worth checking, 0 nice ideas
Since last review: 0 prior items resolved, 4 still apply, 0 new items found

Review findings

🛠️ Needs attention

  • None.

🔎 Worth checking

  • Source-of-truth review needed: CPU Brev launchable Docker bridge firewall workaround: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: `configure_openshell_bridge_firewall` adds broad UFW rules after Docker startup. Repository docs recommend exact-subnet remediation via `docker network inspect openshell-docker`, but the script uses `172.16.0.0/12` and does not explain ownership or lifecycle of the workaround.
  • Constrain or document broad Docker bridge firewall access (scripts/brev-launchable-ci-cpu.sh:41): The launchable allows every source in Docker's default private bridge pool, 172.16.0.0/12, to reach host ports 8080 and 11435. Nearby repository docs recommend allowing only the exact OpenShell Docker subnet. In an ephemeral CI VM this is lower risk, but it still expands the network authorization boundary for the OpenShell gateway and Ollama auth proxy if other containers, routed peers, or attached networks exist in that private range.
    • Recommendation: Prefer deriving and allowing the exact OpenShell Docker network subnet after it exists, or constrain the rule by interface. If the broad pre-network rule is intentional, document the Brev threat model, why exact subnet selection cannot be used here, and when the broad rule should be removed.
    • Evidence: scripts/brev-launchable-ci-cpu.sh defines DOCKER_BRIDGE_POOL_CIDR="172.16.0.0/12" and runs `sudo -n ufw allow from "$DOCKER_BRIDGE_POOL_CIDR" to any port "$port" proto tcp` for OPENSHELL_GATEWAY_PORT="8080" and OLLAMA_AUTH_PROXY_PORT="11435". docs/reference/troubleshooting.mdx documents deriving `SUBNET=$(docker network inspect openshell-docker --format '{{(index .IPAM.Config 0).Subnet}}')` and allowing from that subnet.
  • Launchable can become ready without the bridge firewall rule (scripts/brev-launchable-ci-cpu.sh:92): Firewall setup is best-effort: if `ufw` is missing or `sudo -n ufw` fails, the helper logs a warning and returns. The script later writes the readiness sentinel unconditionally, so CI can receive a ready VM even though sandbox containers may still be unable to reach `host.openshell.internal:8080` or `:11435`.
    • Recommendation: Decide whether these firewall rules are required for this CI launchable. If required, fail before writing the sentinel when rule creation fails. If best-effort is intentional, make that contract explicit and add a later readiness or reachability probe that prevents false-ready launchables.
    • Evidence: `allow_bridge_port` returns after `warn "ufw missing; skip $label"` or `warn "Could not add UFW rule for $label:$port"`; line 326 later executes `sudo touch "$SENTINEL"`, followed by writing `=== Ready ===`.
  • Firewall coverage does not validate failure or runtime reachability (test/brev-nightly-workflow.test.ts:58): The new test is stronger than the previous static snippet checks because it sources the shell helper and verifies mocked `sudo`/`ufw` commands. However, it only covers the success-path command construction. It does not test missing or failing `ufw`, readiness behavior after firewall failure, full-script ordering relative to the sentinel, or actual sandbox reachability to the gateway and auth proxy.
    • Recommendation: Add a negative shell-level contract test with mocked `ufw`/`sudo` failures that verifies the intended readiness behavior. Also add or identify targeted runtime validation for the CPU Brev launchable path, such as a sandbox probe for `host.openshell.internal:8080` and `host.openshell.internal:11435`.
    • Evidence: The test invokes `configure_openshell_bridge_firewall` from a sliced source prefix and asserts the two success-path UFW command lines. It does not exercise the `ufw missing`, failed `sudo -n ufw`, sentinel-writing, or real Docker/OpenShell network paths.
  • Source-of-truth explanation is missing for the firewall workaround (scripts/brev-launchable-ci-cpu.sh:102): This is localized workaround behavior for an infrastructure/network invalid state, but the code does not state where that invalid state is created, why the source cannot be fixed in this PR, what regression proves the source boundary cannot regress, or when the workaround can be removed.
    • Recommendation: Document the invalid state and source boundary in the script or adjacent launchable documentation. Prefer making the invalid state impossible at its source; if that is not feasible here, record the source-fix constraint, add behavioral regression coverage, and include a removal condition for the broad rule.
    • Evidence: `configure_openshell_bridge_firewall` applies UFW rules immediately after Docker starts. Existing docs describe exact-subnet user remediation for firewall-blocked sandbox paths, but the changed script does not explain source ownership, source-fix constraints, regression protection, or removal criteria.
  • Privileged firewall helper does not validate its port argument (scripts/brev-launchable-ci-cpu.sh:89): `allow_bridge_port` interpolates its `port` argument into a privileged `sudo -n ufw allow` command. Current call sites pass constants, so no direct injection path is evident in this patch, but the helper is now a reusable privileged boundary.
    • Recommendation: Validate that `port` is a decimal TCP port in the range 1-65535 before invoking `sudo -n ufw`, or keep the helper private enough that future call sites cannot pass untrusted values.
    • Evidence: `allow_bridge_port() { local port="$1" label="$2" ... sudo -n ufw allow from "$DOCKER_BRIDGE_POOL_CIDR" to any port "$port" proto tcp ... }`; current callers pass `OPENSHELL_GATEWAY_PORT` and `OLLAMA_AUTH_PROXY_PORT` constants.

🌱 Nice ideas

  • None.
Since last review details

Current findings:

  • Source-of-truth review needed: CPU Brev launchable Docker bridge firewall workaround: The advisor marked localized patch analysis as needs_followup.
    • Recommendation: Identify the invalid state, source boundary, source-fix constraint, regression test, and removal condition before merging the localized behavior.
    • Evidence: `configure_openshell_bridge_firewall` adds broad UFW rules after Docker startup. Repository docs recommend exact-subnet remediation via `docker network inspect openshell-docker`, but the script uses `172.16.0.0/12` and does not explain ownership or lifecycle of the workaround.
  • Constrain or document broad Docker bridge firewall access (scripts/brev-launchable-ci-cpu.sh:41): The launchable allows every source in Docker's default private bridge pool, 172.16.0.0/12, to reach host ports 8080 and 11435. Nearby repository docs recommend allowing only the exact OpenShell Docker subnet. In an ephemeral CI VM this is lower risk, but it still expands the network authorization boundary for the OpenShell gateway and Ollama auth proxy if other containers, routed peers, or attached networks exist in that private range.
    • Recommendation: Prefer deriving and allowing the exact OpenShell Docker network subnet after it exists, or constrain the rule by interface. If the broad pre-network rule is intentional, document the Brev threat model, why exact subnet selection cannot be used here, and when the broad rule should be removed.
    • Evidence: scripts/brev-launchable-ci-cpu.sh defines DOCKER_BRIDGE_POOL_CIDR="172.16.0.0/12" and runs `sudo -n ufw allow from "$DOCKER_BRIDGE_POOL_CIDR" to any port "$port" proto tcp` for OPENSHELL_GATEWAY_PORT="8080" and OLLAMA_AUTH_PROXY_PORT="11435". docs/reference/troubleshooting.mdx documents deriving `SUBNET=$(docker network inspect openshell-docker --format '{{(index .IPAM.Config 0).Subnet}}')` and allowing from that subnet.
  • Launchable can become ready without the bridge firewall rule (scripts/brev-launchable-ci-cpu.sh:92): Firewall setup is best-effort: if `ufw` is missing or `sudo -n ufw` fails, the helper logs a warning and returns. The script later writes the readiness sentinel unconditionally, so CI can receive a ready VM even though sandbox containers may still be unable to reach `host.openshell.internal:8080` or `:11435`.
    • Recommendation: Decide whether these firewall rules are required for this CI launchable. If required, fail before writing the sentinel when rule creation fails. If best-effort is intentional, make that contract explicit and add a later readiness or reachability probe that prevents false-ready launchables.
    • Evidence: `allow_bridge_port` returns after `warn "ufw missing; skip $label"` or `warn "Could not add UFW rule for $label:$port"`; line 326 later executes `sudo touch "$SENTINEL"`, followed by writing `=== Ready ===`.
  • Firewall coverage does not validate failure or runtime reachability (test/brev-nightly-workflow.test.ts:58): The new test is stronger than the previous static snippet checks because it sources the shell helper and verifies mocked `sudo`/`ufw` commands. However, it only covers the success-path command construction. It does not test missing or failing `ufw`, readiness behavior after firewall failure, full-script ordering relative to the sentinel, or actual sandbox reachability to the gateway and auth proxy.
    • Recommendation: Add a negative shell-level contract test with mocked `ufw`/`sudo` failures that verifies the intended readiness behavior. Also add or identify targeted runtime validation for the CPU Brev launchable path, such as a sandbox probe for `host.openshell.internal:8080` and `host.openshell.internal:11435`.
    • Evidence: The test invokes `configure_openshell_bridge_firewall` from a sliced source prefix and asserts the two success-path UFW command lines. It does not exercise the `ufw missing`, failed `sudo -n ufw`, sentinel-writing, or real Docker/OpenShell network paths.
  • Source-of-truth explanation is missing for the firewall workaround (scripts/brev-launchable-ci-cpu.sh:102): This is localized workaround behavior for an infrastructure/network invalid state, but the code does not state where that invalid state is created, why the source cannot be fixed in this PR, what regression proves the source boundary cannot regress, or when the workaround can be removed.
    • Recommendation: Document the invalid state and source boundary in the script or adjacent launchable documentation. Prefer making the invalid state impossible at its source; if that is not feasible here, record the source-fix constraint, add behavioral regression coverage, and include a removal condition for the broad rule.
    • Evidence: `configure_openshell_bridge_firewall` applies UFW rules immediately after Docker starts. Existing docs describe exact-subnet user remediation for firewall-blocked sandbox paths, but the changed script does not explain source ownership, source-fix constraints, regression protection, or removal criteria.
  • Privileged firewall helper does not validate its port argument (scripts/brev-launchable-ci-cpu.sh:89): `allow_bridge_port` interpolates its `port` argument into a privileged `sudo -n ufw allow` command. Current call sites pass constants, so no direct injection path is evident in this patch, but the helper is now a reusable privileged boundary.
    • Recommendation: Validate that `port` is a decimal TCP port in the range 1-65535 before invoking `sudo -n ufw`, or keep the helper private enough that future call sites cannot pass untrusted values.
    • Evidence: `allow_bridge_port() { local port="$1" label="$2" ... sudo -n ufw allow from "$DOCKER_BRIDGE_POOL_CIDR" to any port "$port" proto tcp ... }`; current callers pass `OPENSHELL_GATEWAY_PORT` and `OLLAMA_AUTH_PROXY_PORT` constants.

Workflow run details

This is an automated advisory review. A human maintainer must make the final merge decision.

@github-actions
Copy link
Copy Markdown
Contributor

Brev E2E (full): FAILED on branch ci/brev-bridge-firewallSee logs

Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 26618428424
Target ref: bf9a4928993c0a6acbcebb06dc8d9e6bf0cece37
Workflow ref: main
Requested jobs: launchable-smoke-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job Result
launchable-smoke-e2e ✅ success

@github-actions
Copy link
Copy Markdown
Contributor

Brev E2E (full): PASSED on branch ci/brev-bridge-firewallSee logs

@github-actions
Copy link
Copy Markdown
Contributor

Selective E2E Results — ✅ All requested jobs passed

Run: 26619286330
Target ref: e6e127c055161ce43869635a41edac505cc3767e
Workflow ref: main
Requested jobs: launchable-smoke-e2e
Summary: 1 passed, 0 failed, 0 skipped

Job Result
launchable-smoke-e2e ✅ success

@github-actions
Copy link
Copy Markdown
Contributor

Brev E2E (full): PASSED on branch ci/brev-bridge-firewallSee logs

@wscurran wscurran added CI/CD Use this label to identify issues with NemoClaw CI/CD pipeline or GitHub Actions. Docker Support for Docker containerization fix labels May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD Use this label to identify issues with NemoClaw CI/CD pipeline or GitHub Actions. Docker Support for Docker containerization fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants