fix: harden install cleanup and agent startup#40
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughRefactors VM network addressing from 172.16.{slot}.0/30 to 198.19.{slot}.0/30 subnet. Introduces new network-address TypeScript module with IP utility functions. Updates NetworkManager, install script, and related components to use centralized helpers instead of hardcoded addresses. Improves error handling in rootfs preparation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This reverts commit 38ea3bf.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
scripts/publish-runtimes-r2.mjs (1)
65-70: Add explicitbreakto satisfy the linter.While
process.exit(0)prevents actual fallthrough, Biome flags this as suspicious. Adding abreakstatement (which will never execute) silences the warning and makes the intent clearer.♻️ Proposed fix
case "--help": case "-h": printHelp(); process.exit(0); + break; default:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/publish-runtimes-r2.mjs` around lines 65 - 70, Add an explicit unreachable break after the process.exit(0) call in the switch branch that handles "--help" and "-h" to satisfy the linter; locate the switch cases for "--help" / "-h" that call printHelp() and process.exit(0) and insert a break statement immediately after process.exit(0) so Biome/ESLint no longer flags the fallthrough while preserving existing behavior.install.sh (1)
1050-1060: Consider deduplicating the runtime build blocks.Both conditional branches execute identical
build_runtimecalls. The logic could be simplified by combining the conditions.♻️ Proposed simplification
RUNTIME_VERSION="$(runtime_current_version)" -if command -v docker >/dev/null 2>&1; then - build_runtime "node22" "$NODE22_BASE_IMAGE" "$RUNTIME_VERSION" - build_runtime "node24" "$NODE24_BASE_IMAGE" "$RUNTIME_VERSION" - build_runtime "python3.13" "$PYTHON313_BASE_IMAGE" "$RUNTIME_VERSION" -elif [ -n "$RUNTIME_ARTIFACT_BASE_URL" ]; then +if command -v docker >/dev/null 2>&1 || [ -n "$RUNTIME_ARTIFACT_BASE_URL" ]; then build_runtime "node22" "$NODE22_BASE_IMAGE" "$RUNTIME_VERSION" build_runtime "node24" "$NODE24_BASE_IMAGE" "$RUNTIME_VERSION" build_runtime "python3.13" "$PYTHON313_BASE_IMAGE" "$RUNTIME_VERSION" else warn "Docker not found — skipping runtime image builds. Install Docker and re-run to build runtime images." fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 1050 - 1060, The two branches that call build_runtime("node22", "node24", "python3.13") are duplicated; simplify by consolidating the condition so you only call build_runtime once when either Docker exists (command -v docker) or RUNTIME_ARTIFACT_BASE_URL is set, and call warn only in the else branch. Locate the conditional that checks command -v docker and the elif testing RUNTIME_ARTIFACT_BASE_URL and replace the duplicated build_runtime calls with a single block guarded by a combined condition (docker present OR RUNTIME_ARTIFACT_BASE_URL non-empty), keeping the warn "Docker not found — skipping runtime image builds..." in the else.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 131-142: The code currently constructs deletion commands using
eval on the captured iptables rules (see the blocks referencing tap_device, slot
and veth_host where iptables -S output is piped into while read -r rule; do eval
"iptables $(echo "$rule" | sed 's/^-A/-D/')" ... ), which is unsafe; change it
to parse each rule into an argument array (e.g., read into rule_parts or use set
-- to split the rule string), replace the leading "-A" token with "-D" in that
array, and then invoke iptables directly with the positional/array arguments (no
eval or shell interpolation) so iptables receives each token as a separate
argument and avoids command injection risk.
In `@scripts/publish-runtimes-r2.mjs`:
- Around line 158-168: The readExistingManifest function currently calls
JSON.parse(output) directly which will throw on malformed JSON; wrap the
JSON.parse call in a try-catch inside readExistingManifest (after getting output
from maybeExec) and handle parse errors gracefully (e.g., log the parse error or
use processLogger if available, and return null) so a corrupted S3 object
doesn’t crash the process; keep the behavior of returning null when output is
falsy and ensure the catch references the output variable and the function name
(readExistingManifest) for clear context.
In `@src/lib/network-address.ts`:
- Around line 13-21: The parseIpv4 helper is coercing malformed octets via
Number(part) (e.g., "1e2", ""), so change parseIpv4 to first validate each part
is a base-10 unsigned integer string (e.g., /^\d+$/) before converting; then use
parseInt(part, 10) and enforce 0<=octet<=255 and exactly four parts, throwing on
any failure. Apply the same strict-string-check-and-parse approach to the
sibling helper in the same file (the function covering lines 34-40) so malformed
persisted state cannot be silently mapped to another VM/network.
In `@src/lib/network.ts`:
- Around line 267-268: Current code only uses vmAddressBlock =
vmAddressBlockCidrFromIp(hostIp) and installs/removes DROP rules for that single
prefix; change it to iterate over the full set of supported vmsan VM blocks and
apply the same DROP rule actions for each block instead of just vmAddressBlock.
Add or use a constant/list of supported VM blocks (e.g., SUPPORTED_VM_BLOCKS or
vmSanBlocks) and replace all single-use occurrences of vmAddressBlock (and calls
that derive it like vmAddressBlockCidrFromIp) with a loop that installs/removes
the DROP rules for every entry in that list; update the four other occurrences
called out (the other spots currently using vmAddressBlock) so they all iterate
the same list. Ensure the rule installer/remover functions are called for each
block in the list.
In `@src/lib/vm-state.ts`:
- Around line 59-60: The loop in vm-state.ts currently skips any state that is
not "running" or "creating", which frees slots for "stopped" VMs and allows
network conflicts on restart; update the filter so "stopped" is treated like
"running"/"creating" and its slot remains reserved (i.e., in the loop condition
that checks state.status, add "stopped" to the allowed statuses), ensuring
restarted VMs in src/services/vm.ts reuse their persisted network config and
avoid src/lib/network.ts logic that deletes veth-h-{slot}/fhvm{slot} as stale.
---
Nitpick comments:
In `@install.sh`:
- Around line 1050-1060: The two branches that call build_runtime("node22",
"node24", "python3.13") are duplicated; simplify by consolidating the condition
so you only call build_runtime once when either Docker exists (command -v
docker) or RUNTIME_ARTIFACT_BASE_URL is set, and call warn only in the else
branch. Locate the conditional that checks command -v docker and the elif
testing RUNTIME_ARTIFACT_BASE_URL and replace the duplicated build_runtime calls
with a single block guarded by a combined condition (docker present OR
RUNTIME_ARTIFACT_BASE_URL non-empty), keeping the warn "Docker not found —
skipping runtime image builds..." in the else.
In `@scripts/publish-runtimes-r2.mjs`:
- Around line 65-70: Add an explicit unreachable break after the process.exit(0)
call in the switch branch that handles "--help" and "-h" to satisfy the linter;
locate the switch cases for "--help" / "-h" that call printHelp() and
process.exit(0) and insert a break statement immediately after process.exit(0)
so Biome/ESLint no longer flags the fallthrough while preserving existing
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c53c1cba-ae34-4b69-9288-34ff0944ccac
📒 Files selected for processing (12)
README.mddocs/content/2.guide/2.networking.mddocs/content/index.mdinstall.shpackage.jsonscripts/publish-runtimes-r2.mjssrc/lib/jailer.tssrc/lib/network-address.tssrc/lib/network.tssrc/lib/vm-state.tssrc/services/vm.tstests/unit/network-address.test.ts
Comment on lines
+158
to
+168
| function readExistingManifest(bucket, endpoint, key) { | ||
| const output = maybeExec("aws", [ | ||
| ...awsBaseArgs(endpoint), | ||
| "s3", | ||
| "cp", | ||
| `s3://${bucket}/${key}`, | ||
| "-", | ||
| ]); | ||
| if (!output) return null; | ||
| return JSON.parse(output); | ||
| } |
There was a problem hiding this comment.
Consider wrapping JSON.parse in try-catch for robustness.
If the S3 file exists but contains malformed JSON, JSON.parse(output) will throw an uncaught exception. This edge case is unlikely but could cause confusing failures.
🛡️ Proposed fix
function readExistingManifest(bucket, endpoint, key) {
const output = maybeExec("aws", [
...awsBaseArgs(endpoint),
"s3",
"cp",
`s3://${bucket}/${key}`,
"-",
]);
if (!output) return null;
- return JSON.parse(output);
+ try {
+ return JSON.parse(output);
+ } catch {
+ return null;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function readExistingManifest(bucket, endpoint, key) { | |
| const output = maybeExec("aws", [ | |
| ...awsBaseArgs(endpoint), | |
| "s3", | |
| "cp", | |
| `s3://${bucket}/${key}`, | |
| "-", | |
| ]); | |
| if (!output) return null; | |
| return JSON.parse(output); | |
| } | |
| function readExistingManifest(bucket, endpoint, key) { | |
| const output = maybeExec("aws", [ | |
| ...awsBaseArgs(endpoint), | |
| "s3", | |
| "cp", | |
| `s3://${bucket}/${key}`, | |
| "-", | |
| ]); | |
| if (!output) return null; | |
| try { | |
| return JSON.parse(output); | |
| } catch { | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/publish-runtimes-r2.mjs` around lines 158 - 168, The
readExistingManifest function currently calls JSON.parse(output) directly which
will throw on malformed JSON; wrap the JSON.parse call in a try-catch inside
readExistingManifest (after getting output from maybeExec) and handle parse
errors gracefully (e.g., log the parse error or use processLogger if available,
and return null) so a corrupted S3 object doesn’t crash the process; keep the
behavior of returning null when output is falsy and ensure the catch references
the output variable and the function name (readExistingManifest) for clear
context.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/network.ts (1)
135-140: Consider chaining the original error for better debugging.The catch block discards the original error's stack trace. For easier debugging, consider using error chaining:
♻️ Suggested improvement
let slot: number; try { slot = slotFromVmHostIp(network.hostIp); - } catch { - throw new Error(`invalid network slot derived from hostIp: ${network.hostIp}`); + } catch (err) { + throw new Error(`invalid network slot derived from hostIp: ${network.hostIp}`, { cause: err }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/network.ts` around lines 135 - 140, The catch block around slotFromVmHostIp(network.hostIp) discards the original error; update the catch to capture the thrown error (e.g., catch (err)) and rethrow a new Error that includes the original error as a cause or embeds its message/stack so debugging retains context—target the try/catch that assigns slot and use slotFromVmHostIp and network.hostIp references when creating the new error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/network.ts`:
- Around line 135-140: The catch block around slotFromVmHostIp(network.hostIp)
discards the original error; update the catch to capture the thrown error (e.g.,
catch (err)) and rethrow a new Error that includes the original error as a cause
or embeds its message/stack so debugging retains context—target the try/catch
that assigns slot and use slotFromVmHostIp and network.hostIp references when
creating the new error.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
install.sh (1)
128-131:⚠️ Potential issue | 🟡 MinorRemove
evalfrom iptables rule replay.These loops still build deletion commands through
eval. Even though the source is localiptables -Soutput, invokingiptablesdirectly with parsed arguments is safer and easier to reason about.Also applies to: 136-138
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 128 - 131, The loop that replays iptables rules uses eval to run constructed deletion commands; remove eval and instead parse each rule into arguments and invoke iptables directly (e.g., read the line from iptables -S output in the while loop, convert the leading "-A" to "-D", split the rule into shell-safe tokens and call iptables with those tokens) so you avoid executing via eval; update both occurrences that process iptables -S output for tap_device (the while-read loop that currently uses eval "iptables $(echo ... | sed 's/^-A/-D/')") and the similar loop later (lines referenced in the comment) to build and call iptables with an argument array (or use set -- to populate $@) instead of eval.src/lib/network.ts (1)
267-268:⚠️ Potential issue | 🔴 CriticalBlock every supported vmsan VM CIDR, not just the current family.
This still derives a single
vmAddressBlockfrom the VM’s ownhostIp, then installs/removes DROP rules only for that one prefix. During the 172.16 → 198.19 migration, a legacy VM and a new VM can still talk to each other because each side only blocks its own family. Please apply the same DROP rule loop to every supported vmsan block in both setup and teardown.Also applies to: 483-483, 612-612, 721-722, 921-921
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/network.ts` around lines 267 - 268, The code only computes a single vmAddressBlock via vmAddressBlockCidrFromIp(hostIp) and applies DROP rules for that one prefix; instead, iterate over the full set of supported vmsan VM CIDR blocks (the project constant/array that lists all vmsan prefixes) and apply the same iptables/ip rule install/remove for each block during both setup and teardown; update the logic that currently builds vethGuest (this.config.netnsName ? `veth-g-${this.config.slot}` : undefined) to reuse that veth name inside the new loop so each supported block receives the DROP rule, and make the same change at the other affected sites (the other occurrences referenced in the comment) to ensure all supported vmsan blocks are blocked, not just the VM’s own family.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 64-66: The default_iface() function assumes the interface is field
5 from `ip route show default`, which fails when the output uses the "dev
<iface>" form; update default_iface to parse the token "dev" and return the next
field (and fall back to the existing awk pattern if necessary) so the uninstall
logic that removes MASQUERADE/DNAT rules uses the correct interface name; locate
the default_iface function and change its awk/shell parsing to search for "dev"
and emit the following token as the interface, preserving the original behavior
as a fallback.
- Around line 107-119: The cleanup currently requires both $default_iface_name
and $guest_cidr before removing guest-specific ACCEPT rules, which leaves the
OUTPUT/INPUT rules for $guest_ip behind if default-route discovery fails; change
the logic in the teardown so that iptables_delete_rule -D OUTPUT -d "$guest_ip"
-j ACCEPT and iptables_delete_rule -D INPUT -s "$guest_ip" -j ACCEPT are
executed unconditionally (or at least independent of
$default_iface_name/$guest_cidr), while keeping the NAT/PREROUTING and FORWARD
port cleanup (the loop reading json_ports_field "$state_file" and the
iptables_delete_rule -t nat ... PREROUTING / -D FORWARD ... rules) gated by the
existing $default_iface_name/$guest_cidr check and the skip_dnat flag; locate
these calls around the iptables_delete_rule invocations and adjust their
placement so guest_ip ACCEPT rule deletions run even when default_iface_name or
guest_cidr are empty.
In `@src/lib/network.ts`:
- Around line 272-294: The early return inside the policy === "deny-all" branch
prevents installing host-side control-path accept rules (the INPUT/OUTPUT
accepts added via fwd) so host↔guest agent/PTy/tunnel traffic may be blocked;
move or duplicate the host-side accept-rule calls so they run for "deny-all" as
well (e.g., ensure the fwd calls that create the INPUT and OUTPUT accepts for
the control path are executed even when policy === "deny-all" by placing them
before the return or by adding them inside the deny-all branch), referencing the
existing variables and helpers (policy, vethGuest, vethGuest-related FORWARD
accepts, tapDevice, guestIp, and the fwd(...) helper) to locate where to add the
ACCEPT rules.
---
Duplicate comments:
In `@install.sh`:
- Around line 128-131: The loop that replays iptables rules uses eval to run
constructed deletion commands; remove eval and instead parse each rule into
arguments and invoke iptables directly (e.g., read the line from iptables -S
output in the while loop, convert the leading "-A" to "-D", split the rule into
shell-safe tokens and call iptables with those tokens) so you avoid executing
via eval; update both occurrences that process iptables -S output for tap_device
(the while-read loop that currently uses eval "iptables $(echo ... | sed
's/^-A/-D/')") and the similar loop later (lines referenced in the comment) to
build and call iptables with an argument array (or use set -- to populate $@)
instead of eval.
In `@src/lib/network.ts`:
- Around line 267-268: The code only computes a single vmAddressBlock via
vmAddressBlockCidrFromIp(hostIp) and applies DROP rules for that one prefix;
instead, iterate over the full set of supported vmsan VM CIDR blocks (the
project constant/array that lists all vmsan prefixes) and apply the same
iptables/ip rule install/remove for each block during both setup and teardown;
update the logic that currently builds vethGuest (this.config.netnsName ?
`veth-g-${this.config.slot}` : undefined) to reuse that veth name inside the new
loop so each supported block receives the DROP rule, and make the same change at
the other affected sites (the other occurrences referenced in the comment) to
ensure all supported vmsan blocks are blocked, not just the VM’s own family.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3591e7e-de6c-43b2-abcf-b7b4a635c815
📒 Files selected for processing (3)
README.mdinstall.shsrc/lib/network.ts
Comment on lines
+64
to
+66
| default_iface() { | ||
| ip route show default 2>/dev/null | awk '/default/ {print $5; exit}' | ||
| } |
There was a problem hiding this comment.
Parse the dev token instead of assuming field 5.
ip route show default can also render as default dev <iface> .... In that form, $5 is empty and the uninstall path silently skips MASQUERADE/DNAT cleanup.
💡 Suggested change
default_iface() {
- ip route show default 2>/dev/null | awk '/default/ {print $5; exit}'
+ ip route show default 2>/dev/null | awk '{
+ for (i = 1; i < NF; i++) {
+ if ($i == "dev") {
+ print $(i + 1)
+ exit
+ }
+ }
+ }'
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| default_iface() { | |
| ip route show default 2>/dev/null | awk '/default/ {print $5; exit}' | |
| } | |
| default_iface() { | |
| ip route show default 2>/dev/null | awk '{ | |
| for (i = 1; i < NF; i++) { | |
| if ($i == "dev") { | |
| print $(i + 1) | |
| exit | |
| } | |
| } | |
| }' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 64 - 66, The default_iface() function assumes the
interface is field 5 from `ip route show default`, which fails when the output
uses the "dev <iface>" form; update default_iface to parse the token "dev" and
return the next field (and fall back to the existing awk pattern if necessary)
so the uninstall logic that removes MASQUERADE/DNAT rules uses the correct
interface name; locate the default_iface function and change its awk/shell
parsing to search for "dev" and emit the following token as the interface,
preserving the original behavior as a fallback.
Comment on lines
+107
to
+119
| if [ -n "$default_iface_name" ] && [ -n "$guest_cidr" ]; then | ||
| iptables_delete_rule -t nat -D POSTROUTING -s "$guest_cidr" -o "$default_iface_name" -j MASQUERADE | ||
| iptables_delete_rule -D OUTPUT -d "$guest_ip" -j ACCEPT | ||
| iptables_delete_rule -D INPUT -s "$guest_ip" -j ACCEPT | ||
|
|
||
| if [ "$skip_dnat" != "true" ]; then | ||
| while IFS= read -r port; do | ||
| [ -n "$port" ] || continue | ||
| iptables_delete_rule -t nat -D PREROUTING -i "$default_iface_name" -p tcp --dport "$port" -j DNAT --to-destination "${guest_ip}:${port}" | ||
| iptables_delete_rule -D FORWARD -p tcp -d "$guest_ip" --dport "$port" -j ACCEPT | ||
| done < <(json_ports_field "$state_file") | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Don’t gate OUTPUT/INPUT cleanup on default-route discovery.
The guest-specific -D OUTPUT -d "$guest_ip" and -D INPUT -s "$guest_ip" rules do not depend on $default_iface_name or $guest_cidr. If default-interface lookup fails, uninstall currently leaves those vmsan-owned ACCEPT rules behind.
💡 Suggested change
+ iptables_delete_rule -D OUTPUT -d "$guest_ip" -j ACCEPT
+ iptables_delete_rule -D INPUT -s "$guest_ip" -j ACCEPT
+
if [ -n "$default_iface_name" ] && [ -n "$guest_cidr" ]; then
iptables_delete_rule -t nat -D POSTROUTING -s "$guest_cidr" -o "$default_iface_name" -j MASQUERADE
- iptables_delete_rule -D OUTPUT -d "$guest_ip" -j ACCEPT
- iptables_delete_rule -D INPUT -s "$guest_ip" -j ACCEPT
if [ "$skip_dnat" != "true" ]; then
while IFS= read -r port; do📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ -n "$default_iface_name" ] && [ -n "$guest_cidr" ]; then | |
| iptables_delete_rule -t nat -D POSTROUTING -s "$guest_cidr" -o "$default_iface_name" -j MASQUERADE | |
| iptables_delete_rule -D OUTPUT -d "$guest_ip" -j ACCEPT | |
| iptables_delete_rule -D INPUT -s "$guest_ip" -j ACCEPT | |
| if [ "$skip_dnat" != "true" ]; then | |
| while IFS= read -r port; do | |
| [ -n "$port" ] || continue | |
| iptables_delete_rule -t nat -D PREROUTING -i "$default_iface_name" -p tcp --dport "$port" -j DNAT --to-destination "${guest_ip}:${port}" | |
| iptables_delete_rule -D FORWARD -p tcp -d "$guest_ip" --dport "$port" -j ACCEPT | |
| done < <(json_ports_field "$state_file") | |
| fi | |
| fi | |
| iptables_delete_rule -D OUTPUT -d "$guest_ip" -j ACCEPT | |
| iptables_delete_rule -D INPUT -s "$guest_ip" -j ACCEPT | |
| if [ -n "$default_iface_name" ] && [ -n "$guest_cidr" ]; then | |
| iptables_delete_rule -t nat -D POSTROUTING -s "$guest_cidr" -o "$default_iface_name" -j MASQUERADE | |
| if [ "$skip_dnat" != "true" ]; then | |
| while IFS= read -r port; do | |
| [ -n "$port" ] || continue | |
| iptables_delete_rule -t nat -D PREROUTING -i "$default_iface_name" -p tcp --dport "$port" -j DNAT --to-destination "${guest_ip}:${port}" | |
| iptables_delete_rule -D FORWARD -p tcp -d "$guest_ip" --dport "$port" -j ACCEPT | |
| done < <(json_ports_field "$state_file") | |
| fi | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 107 - 119, The cleanup currently requires both
$default_iface_name and $guest_cidr before removing guest-specific ACCEPT rules,
which leaves the OUTPUT/INPUT rules for $guest_ip behind if default-route
discovery fails; change the logic in the teardown so that iptables_delete_rule
-D OUTPUT -d "$guest_ip" -j ACCEPT and iptables_delete_rule -D INPUT -s
"$guest_ip" -j ACCEPT are executed unconditionally (or at least independent of
$default_iface_name/$guest_cidr), while keeping the NAT/PREROUTING and FORWARD
port cleanup (the loop reading json_ports_field "$state_file" and the
iptables_delete_rule -t nat ... PREROUTING / -D FORWARD ... rules) gated by the
existing $default_iface_name/$guest_cidr check and the skip_dnat flag; locate
these calls around the iptables_delete_rule invocations and adjust their
placement so guest_ip ACCEPT rule deletions run even when default_iface_name or
guest_cidr are empty.
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
1 participant
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
Validation
Summary by CodeRabbit
Documentation
Bug Fixes
Chores