fix(dns): probe iptables at /sbin, /usr/sbin when not on PATH (#557)#1168
Open
yimoj wants to merge 1 commit intoNVIDIA:mainfrom
Open
fix(dns): probe iptables at /sbin, /usr/sbin when not on PATH (#557)#1168yimoj wants to merge 1 commit intoNVIDIA:mainfrom
yimoj wants to merge 1 commit intoNVIDIA:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughThe DNS proxy setup script now probes for an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/dns-proxy.test.js (1)
84-114: Add one behavioral test for the missing-iptablesbranch.These cases still only
readFileSync()the script and assert substrings, so a broken probe command or a bad no-iptablescontrol path would still pass. Please add at least onespawnSync()-style test with stubbeddocker/kubectlwrappers that actually runsscripts/setup-dns-proxy.shand asserts the warning plus the no-/etc/resolv.conf-rewrite behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/dns-proxy.test.js` around lines 84 - 114, The current tests only inspect the script text; add a behavioral test in test/dns-proxy.test.js that actually executes scripts/setup-dns-proxy.sh via child_process.spawnSync (or a test helper that wraps spawnSync) with environment stubs for docker/kubectl so the probe path returns "not found", then assert the subprocess stdout/stderr contains the iptables-missing warning ("iptables not found in pod" / "Cannot add UDP DNS exception") and verify that the script did not attempt to rewrite /etc/resolv.conf (e.g., by checking no writes or that the script exits before writing that file). Stub the docker/kubectl wrappers used by the script (mock the commands on PATH or set PATH to point to test shims) so the iptables probe branch is exercised rather than just reading SETUP_DNS_PROXY content.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/setup-dns-proxy.sh`:
- Around line 200-213: Persist the sandbox's original /etc/resolv.conf before
the first rewrite (e.g., when you run the kctl exec ... sh -c "printf
'nameserver ${VETH_GW}...' > /etc/resolv.conf" in the Step 5 block) by saving
its contents to a safe file in the sandbox namespace (use SANDBOX_NS and POD
context, e.g., /tmp/original_resolv.conf) and then, in the else branch where
iptables is not found, restore that saved file back to /etc/resolv.conf via kctl
exec (using the same POD and SANDBOX_NS) so the original resolver is
re-established when the UDP:53 iptables exception cannot be installed; ensure
the save happens only once (detect existing backup before overwriting) and use
the same kctl/ip netns invocation pattern as the existing rewrite to locate the
correct namespace and file.
---
Nitpick comments:
In `@test/dns-proxy.test.js`:
- Around line 84-114: The current tests only inspect the script text; add a
behavioral test in test/dns-proxy.test.js that actually executes
scripts/setup-dns-proxy.sh via child_process.spawnSync (or a test helper that
wraps spawnSync) with environment stubs for docker/kubectl so the probe path
returns "not found", then assert the subprocess stdout/stderr contains the
iptables-missing warning ("iptables not found in pod" / "Cannot add UDP DNS
exception") and verify that the script did not attempt to rewrite
/etc/resolv.conf (e.g., by checking no writes or that the script exits before
writing that file). Stub the docker/kubectl wrappers used by the script (mock
the commands on PATH or set PATH to point to test shims) so the iptables probe
branch is exercised rather than just reading SETUP_DNS_PROXY content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: baaa7c8f-afa3-4af6-85bc-d5b2d6b721cf
📒 Files selected for processing (2)
scripts/setup-dns-proxy.shtest/dns-proxy.test.js
…#557) DNS setup failed silently on sandbox images where iptables was installed at /sbin/iptables but not on PATH. The script now probes well-known locations before giving up, and skips the resolv.conf rewrite when no iptables binary is found to preserve fail-fast behavior.
yimoj
force-pushed
the
fix/dns-proxy-iptables-path-557
branch
from
da482b1 to
cea448f
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/setup-dns-proxy.sh (1)
185-190: Consider simplifying the absolute-path probes.The current logic using
command -vwith a fallback works correctly, but for absolute paths like/sbin/iptables, a directtest -xis clearer since the path is already fully qualified.♻️ Suggested simplification
IPTABLES_BIN="" for candidate in iptables /sbin/iptables /usr/sbin/iptables; do - if kctl exec -n openshell "$POD" -- sh -c "test -x \"\$(command -v $candidate 2>/dev/null || echo $candidate)\"" 2>/dev/null; then + if kctl exec -n openshell "$POD" -- sh -c "command -v $candidate >/dev/null 2>&1 || test -x $candidate" 2>/dev/null; then IPTABLES_BIN="$candidate" break fi doneThis separates concerns:
command -vfor PATH lookup (bareiptables),test -xfor absolute paths — making the intent clearer while preserving the same behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-dns-proxy.sh` around lines 185 - 190, Loop that sets IPTABLES_BIN currently calls command -v even for absolute paths; change the probe to call command -v only for bare candidates (e.g., "iptables") and use a direct test -x for absolute candidates (candidates starting with "/") inside the kctl exec check, preserving the kctl exec -n openshell "$POD" -- sh -c wrapper and the assignment to IPTABLES_BIN when the check succeeds.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/setup-dns-proxy.sh`:
- Around line 185-190: Loop that sets IPTABLES_BIN currently calls command -v
even for absolute paths; change the probe to call command -v only for bare
candidates (e.g., "iptables") and use a direct test -x for absolute candidates
(candidates starting with "/") inside the kctl exec check, preserving the kctl
exec -n openshell "$POD" -- sh -c wrapper and the assignment to IPTABLES_BIN
when the check succeeds.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5c80259d-2143-4d28-86df-ce69380c7323
📒 Files selected for processing (2)
scripts/setup-dns-proxy.shtest/dns-proxy.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/dns-proxy.test.js
2 tasks
wscurran
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #557 — DNS resolution fails inside sandboxes because
iptablesis not on the sandbox image'sPATH(/sandbox/.venv/bin:/usr/local/bin:/usr/bin:/bin), even though it exists at/sbin/iptables.Problem
The
setup-dns-proxy.shscript runs bareiptablesviaip netns exec, which searches PATH and fails silently. Without the iptables UDP:53 exception rule, DNS queries from the sandbox never reach the pod-side forwarder, causingEAI_AGAINfor allgetaddrinfocalls — including WebSocket connections (Slack Socket Mode, Discord gateway) that bypass the HTTP proxy.Fix
iptables,/sbin/iptables,/usr/sbin/iptables) to find the binary before using itresolv.confrewrite when no iptables binary is found, preserving fail-fast behavior instead of silently timing outReproduction
Verified on macOS (Apple Silicon) with OpenShell 0.0.16 + Colima +
ghcr.io/nvidia/openshell-community/sandboxes/base:latest:Before:
socket.gaierror: [Errno -3] Temporary failure in name resolution(EAI_AGAIN)After: All 4 verification checks pass,
wss-primary.slack.comresolves successfullyTest plan
test/dns-proxy.test.jscovering path probing, discovered binary usage, and missing-iptables warningSummary by CodeRabbit
Bug Fixes
Tests