feat(deploy): support CIDR and tap_init_num configuration via environment variables

[woshihoujinxin]

Problem

Operators had no way to override CIDR and tap pool size in one-click deployment without manually editing config files.

Changes

• Add CUBE_CIDR and CUBE_TAP_INIT_NUM to env.example
• Propagate env vars through install.sh, common.sh, up.sh
• Cleanup TAP devices in down-local.sh to prevent restart race condition
• Add test-cidr-replace.sh for CIDR swap validation
• Update Cubelet/config/config.toml and cubemaster.yaml defaults

Testing

• test-cidr-replace.sh validates CIDR swap scenario end-to-end

[cubesandboxbot]

Claude finished @woshihoujinxin's task in 1m 9s —— View job

---

Code Review for PR #379: CIDR and tap_init_num configuration via environment variables

I've reviewed all 8 changed files. Overall this is a well-structured change with good separation between install-time and runtime configuration handling. A few observations below.

---

Structurally sound

Separation of concerns is clean — install.sh handles one-time placeholder replacement during package extraction (generate_cubelet_config_cidr, generate_cubemaster_config_cidr), while common.sh:296-332 (render_config_placeholders) handles runtime re-rendering each time up.sh is invoked. The two paths don't conflict because both guard against already-replaced placeholders.

Sed delimiter choice is correct — Using | instead of / avoids conflicts with CIDR notation (e.g., 192.168.0.0/18).

TAP cleanup in down-local.sh:21-35 is a solid reliability fix. Without it, a rapid restart cycle could leave stale TAP devices that collide with fresh allocations.

---

Findings

1. Runtime re-render is not idempotent for cubemaster.yaml

common.sh:322 guards the cubemaster sed with:

if grep -q "__CUBE_SANDBOX_" "${cubemaster_config}"; then

After the first up.sh run, all __CUBE_SANDBOX_DENY_OUT_* placeholders are replaced with resolved values. On subsequent up.sh runs (even with updated env vars), the guard skips the sed entirely. This means if an operator changes CUBE_SANDBOX_CIDR in .env and runs up.sh (without re-running install.sh), the change is silently ignored.

Compare with the Cubelet config handling at install.sh:144-150 which uses a specific placeholder (__CUBE_SANDBOX_CIDR__), making the intent clearer and the guard more precise.

Suggestion: Either document that CIDR changes require re-running install.sh (not just up.sh), or make the runtime render use per-variable guards (checking for each specific placeholder) and re-render individually.

2. tap_init_num regex uses * instead of \+

common.sh:317:

sed -i "s/tap_init_num = [0-9]*/tap_init_num = ${tap_init_num}/" "${cubelet_config}"

In BRE mode (sed default), [0-9]* matches zero or more digits, so it would match tap_init_num = (empty value). While the template always has tap_init_num = 500 so this isn't a practical problem, [0-9]\+ would be more precise.

3. Test coverage gap — tap_init_num not tested

test-cidr-replace.sh thoroughly validates CIDR placeholder replacement (default, custom, all-custom) but doesn't test the tap_init_num sed logic at all. Consider adding a test case for that.

4. TAP cleanup regex could be overly broad

down-local.sh:26:

grep -oE 'z[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+'

This could match non-TAP interfaces. In practice, the z prefix convention is specific to CubeSandbox's network-agent, so false positives are unlikely. Consider adding a comment explaining that z prefix convention is enforced by network-agent, which is already partially documented on line 19. The existing comment covers it well — this is just a note.

---

Minor / Non-blocking

• The duplicate CIDR definition in env.example (CUBE_SANDBOX_CIDR vs CUBE_SANDBOX_DENY_OUT_SANDBOX_CIDR both default to 192.168.0.0/18) could be clarified with a cross-reference in the comments.
• test-cidr-replace.sh duplicates the sed logic inline rather than sourcing the actual functions from the scripts. This is a pragmatic choice (the functions need root / installed state), but worth noting that the tests test the pattern of replacement rather than the actual functions.

---

Verdict: Clean change, functionally correct. The main actionable item is #1 (re-render idempotency gap) — it's worth addressing or documenting before merge. Everything else is minor polish.

[cubesandboxbot]

Claude finished @woshihoujinxin's task in 1m 21s —— View job

---

Code Review for PR #379: CIDR and tap_init_num configuration via environment variables

I've reviewed all 8 changed files. This is a well-organized change that properly separates install-time placeholder replacement from runtime config re-rendering. Below are my findings.

---

Architecture

Two-layer approach:

• Install time (install.sh:126-150): New functions generate_cubemaster_config_cidr() and generate_cubelet_config_cidr() replace placeholders on the freshly extracted package before copying to the install prefix.
• Runtime (common.sh:296-332): render_config_placeholders() runs on every up.sh invocation to ensure configs are always in sync with the env file.

This is a clean separation and the previous reviewer's observation about the two-layer approach is correct — they don't conflict.

---

Findings

1. Install-time Cubelet CIDR replacement operates on PKG_ROOT after cp -a to INSTALL_PREFIX

In install.sh:518-523:

  generate_cubemaster_config_ports          # modifies PKG_ROOT
  generate_cubemaster_config_cidr           # modifies PKG_ROOT
  cp -a "${PKG_ROOT}/." "${INSTALL_PREFIX}/"  # copies to install prefix
fi

generate_cubelet_config_cidr                # modifies PKG_ROOT (after copy!)

generate_cubelet_config_cidr() modifies ${PKG_ROOT}/Cubelet/config/config.toml, but the files were already copied to ${INSTALL_PREFIX} on line 520. The temp PKG_ROOT is cleaned up on EXIT, so the install-time Cubelet CIDR replacement has no effect on the installed files. The installed config.toml retains the __CUBE_SANDBOX_CIDR__ placeholder and relies entirely on the runtime render_config_placeholders() to resolve it.

Impact: The install-time function is effectively dead code for the Cubelet config path. The cubemaster path doesn't have this issue because generate_cubemaster_config_cidr() runs before cp -a.

Suggestion: Move generate_cubelet_config_cidr before the cp -a call, or remove it if the runtime render is sufficient.

2. Cubemaster runtime re-render guard skips all replacements after first install

common.sh:322:

if [[ -f "${cubemaster_config}" ]] && grep -q "__CUBE_SANDBOX_" "${cubemaster_config}"; then

After install, generate_cubemaster_config_ports() replaces __CUBE_SANDBOX_MYSQL_PORT__ and __CUBE_SANDBOX_REDIS_PORT__, and generate_cubemaster_config_cidr() replaces all __CUBE_SANDBOX_DENY_OUT_* placeholders in cubemaster.yaml. After that, no __CUBE_SANDBOX_ pattern remains. On subsequent up.sh runs, this guard fails and render_config_placeholders() skips cubemaster.yaml entirely.

This means if an operator changes CUBE_SANDBOX_DENY_OUT_SANDBOX_CIDR in .env and runs up.sh (without re-running install.sh), the change is silently ignored.

The Cubelet path uses a per-variable guard (grep -q "__CUBE_SANDBOX_CIDR__"), which is more precise. The cubemaster guard should use a similar approach — checking for each specific placeholder individually — or the cubemaster runtime render should re-run unconditionally since the seds are idempotent when no match is found.

3. tap_init_num regex uses * instead of \+

common.sh:317:

sed -i "s/tap_init_num = [0-9]*/tap_init_num = ${tap_init_num}/" "${cubelet_config}"

In BRE mode (sed default), [0-9]* matches zero or more digits. This would match tap_init_num = (empty value). The template always has tap_init_num = 500 so there's no practical problem, but [0-9]\+ (with \+ for BRE "one or more") or [0-9][0-9]* would be more precise.

4. Test coverage gap — tap_init_num not tested

test-cidr-replace.sh thoroughly validates CIDR placeholder replacement (default, custom, all-custom) but doesn't test the tap_init_num sed logic. Consider adding a test case.

5. Test script tests patterns, not functions

The test duplicates the sed logic inline rather than sourcing common.sh or install.sh. This means tests validate the shape of the replacement rather than the actual functions. This is a pragmatic choice (the functions need root / installed state), but worth noting.

---

Minor / Non-blocking

• env.example has CUBE_SANDBOX_CIDR (line 133) and CUBE_SANDBOX_DENY_OUT_SANDBOX_CIDR (line 141) both defaulting to 192.168.0.0/18. The comments explain the distinction, but a brief cross-reference comment like "Must match CUBE_SANDBOX_CIDR unless sandbox-to-sandbox traffic is intentionally allowed" on line 141 would clarify the relationship.
• The TAP cleanup regex in down-local.sh:26 (z[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+) could theoretically match non-TAP interfaces, but the z prefix convention is specific to CubeSandbox's network-agent, so false positives are unlikely. The existing comments adequately cover this.
• down-local.sh mode changed from 100755 to 100644 (old mode 100755, new mode 100644). This may be unintentional — verify the file is still executable.

---

Verdict

The PR is functionally correct and the architectural separation between install-time and runtime is well-designed. The two main items to address are:

1. Dead code in install.sh — generate_cubelet_config_cidr() modifies PKG_ROOT after files are copied to the install prefix
2. Cubemaster runtime re-render not idempotent — the catch-all grep guard skips re-rendering after initial install

The previous reviewer's code review already identified issue #2 and the regex precision point (#3). These remain the most actionable items.

[fslongjin]

I have a different perspective on this PR's approach and would like to discuss it.

Core Issue

This PR splits the 4 CIDRs in denyOut into 4 separate environment variables, each with a semantically-named variable (CUBE_SANDBOX_DENY_OUT_PRIVATE_A, CUBE_SANDBOX_DENY_OUT_CGN, CUBE_SANDBOX_DENY_OUT_PRIVATE_B, CUBE_SANDBOX_DENY_OUT_SANDBOX_CIDR), and embeds 4 individual placeholders in the JSON template inside cubemaster.yaml.

This has several problems:

1. Mismatch with the data model: DenyOut is []string in Go (see cubebox.pb.go:2960, types/types.go:109) — it's a variable-length list. But this PR turns it into a fixed 4-element structure, losing the list semantics entirely.

2. Poor extensibility: If an operator wants to add a 5th deny CIDR (e.g., 198.18.0.0/15 for network benchmarking), they need to modify the code — add a new placeholder, a new sed expression, and a new env var. If they want to reduce to 3 CIDRs, it's even more awkward because the JSON array structure can't have empty elements.

3. Business semantics leaked into the deployment layer: denyOut is a generic network policy list. Which specific CIDRs it contains is a configuration concern. But by naming each variable semantically (PRIVATE_A = RFC1918 10.0.0.0/8, CGN = RFC6598 100.64.0.0/10), this PR bakes those semantics into the deployment scripts — creating an implicit coupling that shouldn't exist.

Suggested Approach

A better approach would be to preserve the list as a whole: use a single placeholder for the entire denyOut array in the template, and a single env var to carry the whole list.

Recommended solution

cubemaster.yaml template:

"denyOut": __CUBE_SANDBOX_DENY_OUT__

env.example (a single env var using JSON array format):

# Sandbox deny-out CIDRs, as a JSON array.
# These CIDRs will be blocked for outbound traffic from sandboxes.
# Default: deny RFC1918 private networks, CGN space, and the sandbox CIDR itself.
CUBE_SANDBOX_DENY_OUT=["10.0.0.0/8","100.64.0.0/10","172.16.0.0/12","192.168.0.0/18"]

common.sh rendering logic (simplified to a single sed):

sed -i "s|__CUBE_SANDBOX_DENY_OUT__|${CUBE_SANDBOX_DENY_OUT}|g" "${cfg}"

This way:

• denyOut remains a variable-length list — operators can freely add or remove CIDRs
• Adding a CIDR requires only changing the env var value, no code changes
• The data model and deployment config semantics are aligned

Optionally, you could add validation (e.g., via jq) to verify the env var is a valid JSON array of strings.

Alternative

If JSON-in-an-env-var feels awkward, a comma-separated list works too, with sed assembling the JSON array:

CUBE_SANDBOX_DENY_OUT_CIDRS=10.0.0.0/8,100.64.0.0/10,172.16.0.0/12,192.168.0.0/18

Then construct the JSON array during replacement using jq or printf to format the CIDR list into a JSON array before inserting.

---

@woshihoujinxin Interested in your thoughts on this.

[chenhengqi]

I commented on your last PR. No need to change these CIDRs here.

[cubesandboxbot]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[cubesandboxbot]

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

---

Review of PR #379 (Updated Commit 8b8d7e3)

The latest commit 8b8d7e3 addresses all findings from both previous code reviews:

---

✅ Issues Resolved

Previous Finding	Resolution
4 separate denyOut env vars → single JSON array (raised by @fslongjin)	Replaced 4 separate CUBE_SANDBOX_DENY_OUT_* variables with a single CUBE_SANDBOX_DENY_OUT='["...","..."]' JSON array. The cubemaster.yaml template now uses a single __CUBE_SANDBOX_DENY_OUT__ placeholder instead of 4 embedded placeholders in the JSON array. This preserves denyOut's variable-length list semantics ([]string in Go).
install.sh: generate_cubelet_config_cidr after cp -a	Moved to line 491 — now runs before copy_dir_contents / cp -a, so both compute and control roles pick up the substitution in PKG_ROOT.
common.sh: grep guard preventing cubemaster re-render	Removed the grep -q "__CUBE_SANDBOX_" guard entirely. The seds now run unconditionally; since they match specific placeholders, they're idempotent when no match is found.
tap_init_num regex * → \+	Fixed in common.sh:324 — now uses [0-9]\+ (BRE "one or more").
Test coverage for tap_init_num	Added to all 3 test cases (default, custom CIDR, all-custom).
down-local.sh file mode	Already 755 (-rwxr-xr-x).

---

Architecture Summary

The change now uses a clean two-layer approach:

1. Install-time (install.sh): Replaces __CUBE_SANDBOX_CIDR__ in config.toml and __CUBE_SANDBOX_DENY_OUT__ in conf.yaml in PKG_ROOT before cp -a to INSTALL_PREFIX.

2. Runtime (common.sh:render_config_placeholders): Re-renders placeholders on every up.sh invocation via unconditional seds. The tap_init_num value is always refreshed (it uses a direct pattern match on the value, not a placeholder).

---

Minor Observations (Non-blocking)

• Runtime config drift: If an operator changes CUBE_SANDBOX_CIDR or CUBE_SANDBOX_DENY_OUT in .one-click.env and re-runs up.sh, the placeholders are already replaced so the change is silently ignored. tap_init_num does not have this limitation because it uses a direct value match. This is consistent with how CUBE_SANDBOX_MYSQL_PORT / CUBE_SANDBOX_REDIS_PORT already behave. The canonical reconfiguration path is to re-run install.sh.
• The denyOut default in env.example (192.168.0.0/18 included) and CUBE_SANDBOX_CIDR default overlap deliberately — this prevents sandbox-to-sandbox traffic. Operators changing CUBE_SANDBOX_CIDR should also update CUBE_SANDBOX_DENY_OUT to include the new CIDR.

---

Verdict

Looks good. The PR is functionally correct and all prior review feedback has been addressed. The architectural separation between install-time and runtime is clean, and the single-placeholder denyOut approach aligns with the Go data model.

[cubesandboxbot]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[cubesandboxbot]

Claude finished @woshihoujinxin's task in 1m 13s —— View job

---

I'll analyze this and get back to you.