feat(one-click): support cubevs CIDR via env var with conflict detection

[fslongjin]

Allow users to specify CUBE_SANDBOX_NETWORK_CIDR at install time to avoid conflicts with existing host network subnets.

Key changes:

• New env var CUBE_SANDBOX_NETWORK_CIDR (IPv4 CIDR, mask /8-/30)
• New helpers: ip_to_int, ip_int_to_dot, check_cidr_preflight, _check_cidr_conflict in lib/common.sh
• Preflight validation in install.sh: format check, network alignment, host interface/route overlap detection with fail-fast
• Config patching: sed with '|' delimiter to handle '/' in CIDR values
• Symlink rejection guard for sed -i safety
• Bypass flag: CUBE_SANDBOX_NETWORK_CIDR_SKIP_CONFLICT_CHECK
• Persistence to .one-click.env after successful config patch
• Fixed upsert_env_kv atomicity: same-directory temp file

docs: add CIDR env var entries to env.example and configuration reference tables in self-build-deploy.md and multi-node-deploy.md

fix(systemd): use explicit /usr/bin/bash for ExecStart

OpenCloudOS 9.4 systemd 255 uses systemd-executor which does not parse script shebangs, causing 203/EXEC errors. Prefix all ExecStart/ExecStartPre/ExecStartPost with /usr/bin/bash.

[cubesandboxbot]

Claude finished @fslongjin's task in 2m 19s —— View job

---

Code Review: feat(one-click): support cubevs CIDR via env var with conflict detection

Reviewing PR #451...

---

Summary

This PR introduces three major changes:

1. CIDR env var support (CUBE_SANDBOX_NETWORK_CIDR) with format validation, host-network conflict detection, and config patching
2. systemd 255 compatibility fix — prefixing ExecStart* lines with /usr/bin/bash
3. DNS deadlock root cause analysis doc in docs/plan/

I've reviewed all changed files. Overall the code is well-structured with good security consciousness (format validation before bypass, symlink guard, 10# prefix for octal safety). Below are my findings.

---

[A] common.sh — Network helper functions

ip_to_int (L391–404) and ip_int_to_dot (L408–410): ✅ Correct and safe. The 10# prefix properly prevents octal interpretation of leading zeros.

_check_cidr_conflict (L415–508):

• ✅ Overlap test start_A <= end_B && end_A >= start_B is mathematically correct.
• ✅ scope global filter on L459 correctly excludes loopback interfaces.
• ✅ Skipping non-conflicting route ranges (169.254.x.x, 224.x.x, 127.x.x, 0.0.0.0/0) is appropriate.
• ✅ Helpful error message with suggested private ranges and bypass flag.
• 🟡 || true on L459 is ineffective for the ip failure case — in a process substitution (<(...)), pipeline errors inside the subshell don't propagate to the parent regardless of || true. This is harmless since require_cmd ip is called at the top, but the || true is misleading.

check_cidr_preflight (L517–599):

• ✅ Excellent security design: Format validation runs before the SKIP_CONFLICT_CHECK bypass, preventing injection regardless of the bypass flag.
• ✅ Octet length check (≤3 digits, L549) prevents bash arithmetic overflow attacks.
• ✅ Mask range [8, 30] and network alignment check with suggested fix (L570–575) are user-friendly.
• ✅ The 10# prefix is used consistently for mask and octet arithmetic — no octal pitfalls.

---

[B] install.sh — CIDR persistence and patching

CIDR preflight call (L446–450): ✅ Clean integration. Validates before install_required_dependencies so we fail fast.

Symlink guard (L579–581): ✅ Important defense — refuses to patch a symlink target to prevent TOCTOU/symlink-follow attacks via sed -i.

sed delimiter choice (L586): ✅ Using '|' as delimiter correctly avoids conflict with '/' in CIDR values. The format validation (L536) guarantees the CIDR only contains [0-9./], so no injection vector here.

Persistence after patch (L598–607): ✅ Defense-in-depth — env file is only written after sed succeeds, keeping config.toml and .one-click.env in sync.

🟡 Redundant inner check on L598: The block is already inside if [[ -n "${CUBE_SANDBOX_NETWORK_CIDR:-}" ]] (L573), so the inner if [[ -n "${CUBE_SANDBOX_NETWORK_CIDR:-}" ]] on L598 is guaranteed true. Not a bug, but could be simplified.

---

[C] systemd ExecStart fixes

✅ All ExecStart, ExecStartPre, and ExecStartPost lines across 9 service files are correctly prefixed with /usr/bin/bash.

🔴 ExecStop lines are NOT prefixed — e.g. in cube-sandbox-cubemaster.service, cube-sandbox-cubelet.service, cube-sandbox-mysql.service, etc.:

ExecStop=/usr/local/services/cubetoolbox/scripts/systemd/cubelet-stop.sh

If the ExecStop scripts also use shebangs, systemd 255's systemd-executor will fail with 203/EXEC on stop as well. This is less critical than the start path (no deadlock risk), but stop operations will silently fail on affected systems. Recommend prefixing all ExecStop paths too.

---

[D] upsert_env_kv fix

✅ The temp-file-in-same-directory pattern (mktemp "${env_file}.XXXXXX") is the correct fix for cross-filesystem atomic rename. Previously, a temp file in /tmp (tmpfs) could not atomically replace a target on /usr/local/... (ext4/xfs). Good defensive fix.

---

[E] Documentation

✅ All four documentation files (self-build-deploy.md, multi-node-deploy.md, zh versions) include clear entries for both CUBE_SANDBOX_NETWORK_CIDR and CUBE_SANDBOX_NETWORK_CIDR_SKIP_CONFLICT_CHECK.

✅ env.example has well-documented entries with the mask range constraint and conflict check bypass flag.

✅ The DNS deadlock root cause analysis doc is thorough and well-structured.

---

[F] Minor observations

1. L459 unnnecessary || true: ip -4 addr show scope global 2>/dev/null | awk '/inet / {print $2, $NF}' || true — in a process substitution context, this exit code is never read by the parent shell. Consider removing for clarity, or keep it as documentation of intent (no functional harm either way).

2. ExecStop omission: As noted in section [C], ExecStop lines remain unprefixed in all systemd units. While these scripts rarely run through systemd-executor hot paths, they can still fail on stop. Worth addressing as a follow-up.

---

Verdict

Solid implementation. The CIDR validation is thorough, security-conscious, and user-friendly. The systemd fix addresses a real production issue on OpenCloudOS 9.4. Two items worth addressing before merging:

• Fix ExecStop unprefixed systemd paths →
• Remove redundant inner guard in install.sh →

---

[cubesandboxbot]

Claude finished @fslongjin's task in 1m 22s —— View job

---

Code Review: feat(one-click): support cubevs CIDR via env var with conflict detection

Reviewing PR #451...

---

Summary

This PR introduces three changes:

1. CIDR env var (CUBE_SANDBOX_NETWORK_CIDR) with format validation, host-network conflict detection, and config patching
2. systemd 255 compatibility fix — prefixing ExecStart* lines with /usr/bin/bash for OpenCloudOS 9.4
3. upsert_env_kv atomicity fix — same-directory temp file for cross-filesystem rename safety

I've reviewed all 17 changed files. The code is well-structured with thoughtful security considerations. Below are my findings.

---

[A] common.sh — Network helper functions (L388–599)

ip_to_int / ip_int_to_dot (L391–410): ✅ Correct and safe. The 10# prefix properly prevents octal interpretation of leading zeros.

_check_cidr_conflict (L415–508):

• ✅ Overlap test start_A <= end_B && end_A >= start_B at L456 is mathematically correct.
• ✅ scope global filter at L459 correctly excludes loopback interfaces.
• ✅ Skipping well-known non-conflicting route ranges (169.254.x.x, 224.x.x, 127.x.x, 0.0.0.0/0) is appropriate.
• ✅ Helpful error message with suggested private ranges and bypass flag (L496–507).
• 🟡 || true on L459 is ineffective in process substitution context — In a process substitution (<(...)), pipeline exit codes from the subshell do not propagate to the parent shell regardless of || true. This is harmless since require_cmd ip is called at L417, but the || true creates a misleading impression of defense. Consider removing for clarity.

check_cidr_preflight (L517–599):

• ✅ Excellent security design: Format validation runs before the SKIP_CONFLICT_CHECK bypass (L534 comment), preventing injection regardless of bypass flag.
• ✅ Octet length check (≤3 digits, L549) prevents bash arithmetic overflow attacks.
• ✅ Mask range [8, 30] and network alignment check with suggested fix (L570–575) is user-friendly.
• ✅ Consistent 10# prefix throughout — no octal pitfalls.

---

[B] install.sh — CIDR persistence and patching (L445–613)

CIDR preflight call (L446–450): ✅ Clean integration. Validation runs before install_required_dependencies at L452 for fast failure.

Symlink guard (L579–581): ✅ Important TOCTOU defense — sed -i follows symlinks, so this prevents an attacker with write access to INSTALL_PREFIX from targeting arbitrary files.

sed delimiter choice (L586): ✅ Using '|' correctly avoids conflict with '/' in CIDR values. The format validation at L536 guarantees the CIDR only contains [0-9./], so no injection vector remains.

Persistence after patch (L595–607): ✅ Defense-in-depth — env file is only written after sed succeeds, keeping config.toml and .one-click.env in sync. This prevents a crash-before-persist scenario from leaving an inconsistent state.

🟡 Redundant inner guard at L598: The block is already inside if [[ -n "${CUBE_SANDBOX_NETWORK_CIDR:-}" ]] (L573), so the inner if [[ -n "${CUBE_SANDBOX_NETWORK_CIDR:-}" ]] on L598 is guaranteed true. Not a bug, but could be simplified. Fix this →

---

[C] Systemd ExecStop compatibility (9 service files)

✅ All ExecStart, ExecStartPre, and ExecStartPost lines are correctly prefixed with /usr/bin/bash.

🔴 ExecStop lines remain unprefixed in 7 service files:

File	ExecStop
cube-sandbox-coredns.service	/usr/local/services/cubetoolbox/scripts/systemd/coredns-stop.sh
cube-sandbox-cubelet.service	/usr/local/services/cubetoolbox/scripts/systemd/cubelet-stop.sh
cube-sandbox-cubemaster.service	(no ExecStop defined, but cube-sandbox-cubelet.service has one)
cube-sandbox-mysql.service	/usr/local/services/cubetoolbox/scripts/systemd/mysql-stop.sh
cube-sandbox-redis.service	/usr/local/services/cubetoolbox/scripts/systemd/redis-stop.sh
cube-sandbox-webui.service	/usr/local/services/cubetoolbox/scripts/systemd/webui-stop.sh
cube-sandbox-cube-proxy.service	/usr/local/services/cubetoolbox/scripts/systemd/cube-proxy-stop.sh
cube-sandbox-dns.service	/usr/local/services/cubetoolbox/scripts/systemd/dns-host-route-down.sh

If the stop scripts use shebangs (they likely do, following the same pattern as start scripts), systemd 255's systemd-executor will fail with 203/EXEC on stop as well. While the stop path is less critical (no deadlock risk), stop operations will silently fail on OpenCloudOS 9.4. Recommend prefixing all ExecStop paths with /usr/bin/bash for consistency. Fix all ExecStop paths →

---

[D] upsert_env_kv fix

✅ Using mktemp "${env_file}.XXXXXX" (same-directory temp file) is the correct fix for cross-filesystem atomic rename. A temp file in /tmp (tmpfs) could not atomically replace a target on an ext4/x4 filesystem. Good defensive fix.

---

[E] Documentation

✅ env.example (L79–85): Well-documented entries for both CUBE_SANDBOX_NETWORK_CIDR and CUBE_SANDBOX_NETWORK_CIDR_SKIP_CONFLICT_CHECK with mask range constraint and clear comments.

✅ All four docs files (self-build-deploy.md, multi-node-deploy.md, zh versions) include the CIDR env var entries.

---

[F] Minor observations

1. 🟡 check_cidr_preflight is tested with CUBE_SANDBOX_NETWORK_CIDR= set but call at install.sh L448 passes "${CUBE_SANDBOX_NETWORK_CIDR}" (with the :-} default at L446). The function itself also checks if [[ -z "${cidr}" ]]; then return 0 at L521. When the env var is empty, check_cidr_preflight "" is called, which returns immediately — correct behavior, but slightly redundant. Consider guarding the call with [[ -n ... ]] instead of relying on the function-level check, for consistency with the outer guard pattern used elsewhere.

2. 🟡 Line 459 || true: As noted in [A], this is misleading in a process substitution context. Recommend removing it for clarity.

---

Verdict

Solid implementation. The CIDR validation is thorough, security-conscious, and user-friendly. The systemd fix addresses a real production issue. Two items worth addressing before merging:

1. 🔴 Prefix ExecStop paths with /usr/bin/bash in all systemd service files for OpenCloudOS 9.4 compatibility
2. 🟡 Remove the redundant inner guard in install.sh at L598
   — Branch: jinlong-feat-cubevs-cidr-env-var

[cubesandboxbot]

Claude finished @fslongjin's task in 1m 12s —— View job

---

Code Review: feat(one-click): support cubevs CIDR via env var with conflict detection

Progress Update

Reviewing PR #451...

---

Summary

This PR introduces three changes:

1. CIDR env var (CUBE_SANDBOX_NETWORK_CIDR) with format validation, host-network conflict detection, and config patching
2. systemd 255 compatibility fix — prefixing ExecStart* lines with /usr/bin/bash for OpenCloudOS 9.4
3. upsert_env_kv atomicity fix — same-directory temp file for cross-filesystem rename safety

The latest commit (3d70d53) addresses the 🔴 ExecStop issue identified in the previous review. All 7 ExecStop paths are now correctly prefixed with /usr/bin/bash.

I've reviewed all 17 changed files. The code is well-structured with thoughtful security considerations. Below are my findings.

---

[A] common.sh — Network helper functions (L388–599)

ip_to_int / ip_int_to_dot (L391–410): ✅ Correct and safe. The 10# prefix properly prevents octal interpretation of leading zeros.

_check_cidr_conflict (L415–508):

• ✅ Overlap test start_A <= end_B && end_A >= start_B at L456 is mathematically correct.
• ✅ scope global filter at L459 correctly excludes loopback interfaces.
• ✅ Skipping well-known non-conflicting route ranges (169.254.x.x, 224.x.x, 127.x.x, 0.0.0.0/0) is appropriate.
• ✅ require_cmd ip at L417 ensures the ip command is available before use.
• ✅ Helpful error message with suggested private ranges and bypass flag (L496–507).
• 🟡 || true on L459 is ineffective in process substitution context — In done < <(ip ... || true), the pipeline exit code from the subshell does not propagate to the parent shell regardless of || true. Harmless (the require_cmd ip at L417 already guards against missing ip), but the || true creates a misleading impression of defense. Consider removing for clarity.

check_cidr_preflight (L517–599):

• ✅ Excellent security design: Format validation runs before the SKIP_CONFLICT_CHECK bypass (L534 comment), preventing injection regardless of bypass flag.
• ✅ Octet length check (≤3 digits, L549) prevents bash arithmetic overflow attacks.
• ✅ Mask range [8, 30] and network alignment check with suggested fix (L570–575) is user-friendly.
• ✅ Consistent 10# prefix throughout — no octal pitfalls.

---

[B] install.sh — CIDR persistence and patching (L445–613)

CIDR preflight call (L446–450): ✅ Clean integration. Validation runs before install_required_dependencies at L452 for fast failure.

Symlink guard (L579–581): ✅ Important TOCTOU defense — sed -i follows symlinks, so this prevents an attacker with write access to INSTALL_PREFIX from targeting arbitrary files.

sed delimiter choice (L586): ✅ Using '|' correctly avoids conflict with '/' in CIDR values. The format validation at L536 guarantees the CIDR only contains [0-9./], so no injection vector remains.

Persistence after patch (L595–607): ✅ Defense-in-depth — env file is only written after sed succeeds, keeping config.toml and .one-click.env in sync. This prevents a crash-before-persist scenario from leaving an inconsistent state.

🟡 Redundant inner guard at L598: The block is already inside if [[ -n "${CUBE_SANDBOX_NETWORK_CIDR:-}" ]] (L573), so the inner if [[ -n "${CUBE_SANDBOX_NETWORK_CIDR:-}" ]] on L598 is guaranteed true. Not a bug, but could be simplified by removing the inner guard and directly calling upsert_env_kv. Fix this →

---

[C] Systemd ExecStop — FIXED ✅

The previous review flagged that ExecStop paths were not prefixed with /usr/bin/bash. The new commit 3d70d53 addresses this:

File	Before	After
cube-sandbox-coredns.service	ExecStop=...coredns-stop.sh	ExecStop=/usr/bin/bash ...coredns-stop.sh
cube-sandbox-cube-proxy.service	ExecStop=...cube-proxy-stop.sh	ExecStop=/usr/bin/bash ...cube-proxy-stop.sh
cube-sandbox-cubelet.service	ExecStop=...cubelet-stop.sh	ExecStop=/usr/bin/bash ...cubelet-stop.sh
cube-sandbox-dns.service	ExecStop=...dns-host-route-down.sh	ExecStop=/usr/bin/bash ...dns-host-route-down.sh
cube-sandbox-mysql.service	ExecStop=...mysql-stop.sh	ExecStop=/usr/bin/bash ...mysql-stop.sh
cube-sandbox-redis.service	ExecStop=...redis-stop.sh	ExecStop=/usr/bin/bash ...redis-stop.sh
cube-sandbox-webui.service	ExecStop=...webui-stop.sh	ExecStop=/usr/bin/bash ...webui-stop.sh

All 7 ExecStop paths are now correctly prefixed. This ensures systemd 255 compatibility on OpenCloudOS 9.4 for both start and stop paths.

---

[D] upsert_env_kv fix

✅ Using mktemp "${env_file}.XXXXXX" (same-directory temp file) is the correct fix for cross-filesystem atomic rename. A temp file in /tmp (tmpfs) could not atomically replace a target on an ext4/xfs filesystem. Good defensive fix.

---

[E] Documentation

✅ env.example (L79–85): Well-documented entries for both CUBE_SANDBOX_NETWORK_CIDR and CUBE_SANDBOX_NETWORK_CIDR_SKIP_CONFLICT_CHECK with mask range constraint and clear comments.

✅ All four docs files (self-build-deploy.md, multi-node-deploy.md, zh versions) include the CIDR env var entries.

---

[F] Minor observations (unchanged from prior review)

1. 🟡 Line 459 || true: In a process substitution context, the || true is not needed and could be misleading — ip failure here is already guarded by require_cmd ip at L417. Consider removing for clarity.
2. 🟡 Redundant inner guard at L598: As noted in section [B], this could be simplified.

---

Verdict

Solid implementation. The CIDR validation is thorough, security-conscious, and user-friendly. The ExecStop issue from the previous review has been fully resolved in commit 3d70d53.

Remaining minor suggestions (optional, 🟡 level):

• Remove the redundant inner guard at install.sh L598
• Remove the misleading || true at common.sh L459