Skip to content

fix(onboard): tighten UFW reachability remediation#3533

Merged
cv merged 1 commit into
NVIDIA:mainfrom
stevenrick:fix/tighten-ufw-reachability-remediation
May 15, 2026
Merged

fix(onboard): tighten UFW reachability remediation#3533
cv merged 1 commit into
NVIDIA:mainfrom
stevenrick:fix/tighten-ufw-reachability-remediation

Conversation

@stevenrick

@stevenrick stevenrick commented May 14, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Member

Summary

Narrows the UFW remediation shown for sandbox reachability failures when NemoClaw knows both the Docker bridge subnet and gateway IP.
The existing broader subnet-only fallback remains in place when the gateway IP is unavailable.

Related Issue

Related to #3456.

Changes

  • Use sudo ufw allow from <subnet> to <gateway-ip> port <port> proto tcp for gateway and Ollama proxy reachability messages when the probed Docker gateway IP is known.
  • Preserve the current to any port remediation when only the subnet is known or must be looked up dynamically.
  • Update formatter tests to cover the gateway-specific command and subnet-only fallback.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Verification

Focused checks passed locally under Node 22.22.3:

  • npm run typecheck:cli
  • npx @biomejs/biome check src/lib/onboard/gateway-sandbox-reachability.ts src/lib/onboard/gateway-sandbox-reachability.test.ts src/lib/onboard/ollama-proxy-reachability.ts src/lib/onboard/ollama-proxy-reachability.test.ts
  • npm test -- src/lib/onboard/gateway-sandbox-reachability.test.ts src/lib/onboard/ollama-proxy-reachability.test.ts

Attempted full local gates. npm test, make check, and npx prek run --from-ref origin/main --to-ref HEAD did not pass cleanly on this macOS workstation due failures outside the touched reachability files, mostly coverage-run per-test timeouts. The touched reachability tests passed in focused runs and during larger CLI coverage runs.

  • npx prek run --all-files passes
  • npm test passes
  • Tests added or updated for new or changed behavior
  • No secrets, API keys, or credentials committed
  • Docs updated for user-facing behavior changes
  • make docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: stevenrick srick@nvidia.com

Summary by CodeRabbit

  • Bug Fixes

    • Firewall command generation tightened: when gateway IP is available, suggested rules target that specific gateway; otherwise they fall back to subnet-to-any or dynamic subnet resolution.

  • Tests

    • Unit tests expanded to cover both gateway-present and gateway-missing scenarios, validating specific and fallback firewall rule outputs.

Review Change Stack

@copy-pr-bot

copy-pr-bot Bot commented May 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented May 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Two onboarding reachability formatters now emit a narrower UFW allow command when both subnet and gatewayIp are known (ufw allow from <subnet> to <gatewayIp> port <port>); when gatewayIp is missing they fall back to subnet→any or a docker-derived subnet as before. Tests were added/updated for both cases.

Changes

UFW Rule Specificity for Gateway Reachability

Layer / File(s) Summary
Gateway reachability formatter + tests
src/lib/onboard/gateway-sandbox-reachability.ts, src/lib/onboard/gateway-sandbox-reachability.test.ts		 formatSandboxBridgeUnreachableMessage() now emits ufw allow from <subnet> to <gatewayIp> port 8080 when result.subnet and result.gatewayIp exist; otherwise it falls back to from <subnet> to any or uses docker network inspect to derive a subnet. Tests updated to assert both gateway-specific and subnet-only fallback messages.
Ollama proxy reachability formatter + tests
src/lib/onboard/ollama-proxy-reachability.ts, src/lib/onboard/ollama-proxy-reachability.test.ts		 formatOllamaProxyUnreachableMessage() updated similarly to emit ufw allow from <subnet> to <gatewayIp> port <port> when both subnet and gatewayIp are present; otherwise preserves subnet→any or docker-derived subnet behavior. Tests added/changed to validate both cases.
Import/destructuring reorder (non-functional)
src/lib/onboard/ollama-proxy-reachability.ts, src/lib/onboard/gateway-sandbox-reachability.test.ts		 Minor reordering of destructured imports in modules/tests; no runtime behavioral change.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • NVIDIA/NemoClaw#3441: Modifies gateway-sandbox-reachability and related UFW remediation formatting.
  • NVIDIA/NemoClaw#3459: Introduced related sandbox reachability probing/formatting referenced by these changes.
  • NVIDIA/NemoClaw#3472: Similar updates to formatOllamaProxyUnreachableMessage UFW rule generation.

Suggested labels

fix, Docker, Sandbox, NemoClaw CLI, bug

Suggested reviewers

  • ericksoa

Poem

🐰 I hop where subnets meet the gate,
I whisper rules to make things straight.
If gateway’s known, I point the lane,
Else "to any" keeps the nets the same.
Tests stamp paws — connectivity's sane!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.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 title directly reflects the main change: tightening UFW remediation commands when gateway IP is available, making them more specific rather than allowing from subnet to 'any'.
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 unit tests (beta)
  • Create PR with unit tests

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.

@stevenrick stevenrick force-pushed the fix/tighten-ufw-reachability-remediation branch 4 times, most recently from 7d7dde9 to f7b4100 Compare May 14, 2026 21:48
@stevenrick stevenrick force-pushed the fix/tighten-ufw-reachability-remediation branch 2 times, most recently from daa2e19 to f7fae18 Compare May 14, 2026 23:37
@stevenrick

stevenrick commented May 15, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor
✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@stevenrick stevenrick force-pushed the fix/tighten-ufw-reachability-remediation branch from f7fae18 to 9b5262a Compare May 15, 2026 13:57
@stevenrick
fix(onboard): tighten UFW reachability remediation
b94cb32 
Signed-off-by: stevenrick <srick@nvidia.com>
@stevenrick stevenrick force-pushed the fix/tighten-ufw-reachability-remediation branch from 9b5262a to b94cb32 Compare May 15, 2026 14:00
@cv cv merged commit 73d30c9 into NVIDIA:main May 15, 2026
17 checks passed
@stevenrick stevenrick deleted the fix/tighten-ufw-reachability-remediation branch May 15, 2026 14:09
@nvshaxie nvshaxie mentioned this pull request May 18, 2026
Closed
@wscurran wscurran added the bug-fix PR fixes a bug or regression label Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cv cv cv approved these changes

Assignees

No one assigned

Labels

bug-fix PR fixes a bug or regression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@stevenrick @cv @wscurran