cubemastercli: Refactor CubeVS params parsing logic

[chenhengqi]

Let's rely on urfave/cli's functionalities instead of reinvent custom parsing logic. With this change, we allow users to specify the following options:

--allow-internet-access
--allow-internet-access=false
--allow-internet-access=true
--allow-out-cidr "10.0.0.0/8"
--deny-out-cidr "10.0.0.0/8"

But not:

--allow-internet-access false
--allow-internet-access true

[github-actions]

Claude finished @chenhengqi's task in 2m 36s —— View job

---

PR Review: cubemastercli: Refactor CubeVS params parsing logic

• Gather context: read PR diff and changed files
• Check DCO (Signed-off-by) on all commits
• Review code quality, bugs, security, and performance
• Post inline review comments
• Final summary

DCO Check

PASS — Commit 63393b4 contains Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>.

Summary

This is a solid refactoring that replaces ~170 lines of custom CLI argument parsing with ~60 lines that leverage urfave/cli's built-in flag handling. The overall direction is good — delegating to the framework is the right call.

However, I found 4 issues that should be addressed before merging:

#	Severity	Issue	Location
1	Bug	parseCubeVSParams always returns non-nil *types.CubeVSContext, even when no flags are set. This changes the JSON payload for create-from-image from omitting cubevs_context to always including "cubevs_context":{}.	net.go:16-37
2	Behavioral	mergeCubeVSParams mutates base in-place, whereas the old code cloned first. Safe for current call sites but a contract change.	net.go:39-59
3	Behavioral	CIDR deduplication was removed. Duplicate --allow-out-cidr / --deny-out-cidr values will now appear multiple times.	net.go:44-56
4	Coverage	4 tests removed, 0 added for the new parseCubeVSParams and mergeCubeVSParams functions.	template_test.go

The most critical issue is #1 — it could cause the create-from-image endpoint to receive a different JSON payload when no network flags are provided, potentially changing server-side behavior if it distinguishes between absent and empty CubeVSContext.

See inline comments for details and suggested fixes.

[github-actions]

Bug: parseCubeVSParams always returns a non-nil *types.CubeVSContext

When no network flags are set, this function returns &types.CubeVSContext{} instead of nil. The create-from-image command calls parseCubeVSParams (not mergeCubeVSParams), so req.CubeVSContext will always be set to a non-nil empty struct even when the user provides no network flags.

The old code returned nil when no flags were set, which means the CubeVSContext field would be omitted from the JSON payload (due to omitempty). With this change, it will always be serialized as "cubevs_context":{}.

Suggested fix — return nil when no flags are set:

func parseCubeVSParams(c *cli.Context) (*types.CubeVSContext, error) {
	params := &types.CubeVSContext{}
	if c.IsSet(keyAllowInternetAccess) {
		val := c.Bool(keyAllowInternetAccess)
		params.AllowInternetAccess = &val
	}
	for _, v := range c.StringSlice(keyAllowOutCIDR) {
		_, _, err := net.ParseCIDR(v)
		if err != nil {
			return nil, err
		}
		params.AllowOut = append(params.AllowOut, v)
	}
	for _, v := range c.StringSlice(keyDenyOutCIDR) {
		_, _, err := net.ParseCIDR(v)
		if err != nil {
			return nil, err
		}
		params.DenyOut = append(params.DenyOut, v)
	}
	if params.AllowInternetAccess == nil && len(params.AllowOut) == 0 && len(params.DenyOut) == 0 {
		return nil, nil
	}
	return params, nil
}

[github-actions]

Behavioral change: mergeCubeVSParams mutates base in-place

The old code used cloneCubeVSContext to clone the existing context before modifying it. This new implementation directly mutates base by:

1. Setting base.AllowInternetAccess = &val
2. Appending to base.AllowOut and base.DenyOut

While the current call sites (in TemplateCreateCommand and TemplateCommitCommand) pass freshly-deserialized objects, this is a subtle contract change that could introduce bugs if future callers pass shared objects. Consider either:

• Documenting that base is mutated in-place, or
• Cloning base before modifying it (as the old code did)

[github-actions]

Behavioral regression: CIDR deduplication removed

The old code used dedupeCIDRs and appendUniqueCIDRs to prevent duplicate CIDRs from being added. This new code simply appends without checking for duplicates. If a user specifies --allow-out-cidr 10.0.0.0/8 --allow-out-cidr 10.0.0.0/8, the same CIDR will appear twice in the result.

This was intentional in the old implementation — is the removal deliberate? If so, it should be noted in the PR description.

[github-actions]

Missing tests for new functions

Four tests were removed:

• TestMergeCreateFromImageCubeVSContextFlagsEqualsSyntax
• TestMergeCreateFromImageCubeVSContextFlagsSupportsTrailingFalse
• TestMergeCreateFromImageCubeVSContextFlagsRejectsUnexpectedArgs
• TestMergeCubeVSContextValuesPreservesExistingCIDRs

But no new tests were added for parseCubeVSParams or mergeCubeVSParams. At minimum, tests should cover:

1. parseCubeVSParams returns nil when no flags are set
2. parseCubeVSParams with --allow-internet-access=false sets AllowInternetAccess to &false
3. parseCubeVSParams validates CIDR format
4. mergeCubeVSParams preserves existing CIDRs when merging
5. --allow-internet-access=true and --allow-internet-access work as expected

[chenhengqi]

/cubebot review latest change.

[github-actions]

Claude finished @chenhengqi's task in 3m 13s —— View job

---

Review: Latest Changes on fix-tpl-net-params

• Gather context: read PR diff and changed files
• Review code quality, bugs, security, and performance
• Post review feedback
• Correct review errors

---

The latest commit (d00668c) addresses the most critical issue from the prior review — parseCubeVSParams now correctly returns nil when no flags are set (net.go:17-21). That was the bug that would have changed the JSON payload from omitting cubevs_context to always including "cubevs_context":{}.

Resolved from Prior Review

#	Issue	Status
1	parseCubeVSParams always returns non-nil	Fixed — early return at net.go:17-21

Still Open from Prior Review

#	Severity	Issue	Location
2	Low	mergeCubeVSParams mutates base in-place (old code cloned first). Safe for current call sites but a contract change.	net.go:46-77
3	Low	CIDR deduplication removed — duplicate --allow-out-cidr values will appear multiple times	net.go:61-74
4	Medium	4 tests removed, 0 added for parseCubeVSParams and mergeCubeVSParams	template_test.go

New Observations

5. net.ParseCIDR return values (net.go:29, net.go:36)

The _, _, err := net.ParseCIDR(v) pattern is correct — net.ParseCIDR returns (IP, *IPNet, error), three values. I initially flagged this incorrectly; no issue here.

6. cli.BoolFlag and --flag=false behavior (template.go:186, 492, 594)

The project uses urfave/cli v1.22.14, which delegates to the standard flag package. The standard flag package supports --flag=false syntax for bool flags, so the behavior described in the PR body is accurate:

• --allow-internet-access → true ✓
• --allow-internet-access=true → true ✓
• --allow-internet-access=false → false ✓
• --allow-internet-access false → flag is true, false is a positional arg ✗ (rejected as intended)

I initially raised a false concern about v1 BoolFlag not supporting =false; this is incorrect.

Updated Summary

Issue #1 from the prior review is fixed. The in-place mutation (#2) and removed dedup (#3) are low severity for current call sites. The missing tests (#4) should ideally be addressed — at minimum, tests for:

1. parseCubeVSParams returns nil when no flags are set
2. parseCubeVSParams with --allow-internet-access=false sets AllowInternetAccess to &false
3. parseCubeVSParams validates CIDR format
4. mergeCubeVSParams preserves existing CIDRs when merging

No blocking issues remain. The refactoring achieves its stated goal of replacing custom parsing with urfave/cli's built-in flag handling.

[fslongjin]

I found one blocking issue that should be addressed before merge.

Blocking

tpl create-from-image regresses the previously supported --allow-internet-access false usage, and this also causes the following CIDR flags to stop being parsed.

• Relevant code: CubeMaster/cmd/cubemastercli/commands/cubebox/net.go:16, CubeMaster/cmd/cubemastercli/commands/cubebox/template.go:644
• This PR replaces the dedicated mergeCreateFromImageCubeVSContextFlags(...) path with the shared parseCubeVSParams(...) path for create-from-image
• However, this command still uses urfave/cli v1 BoolFlag. In that library, --allow-internet-access false does not parse false as the flag value; false becomes a positional argument instead
• As a result:
  • c.Bool("allow-internet-access") becomes true, which is the opposite of what the user explicitly requested
  • Once flag.FlagSet hits that positional argument, it stops parsing the remaining flags, so --allow-out-cidr / --deny-out-cidr are also dropped from the final request

I verified this locally with a minimal reproduction:

--allow-internet-access false
=> IsSet=true, Bool=true, remaining arg0="false"

--allow-internet-access false --allow-out-cidr 172.16.0.0/16
=> Bool=true, allowOut=[]

This is a behavior regression. The previous implementation had parseCreateFromImageExtraCubeVSFlags(...) specifically to handle this case, and this PR removes that compatibility path. The related tests were removed at the same time, so the current test pass does not cover this regression.

Suggested fix:

1. Restore compatibility for create-from-image with --allow-internet-access false, or at minimum fail explicitly in the CLI instead of silently interpreting false as true
2. Add regression tests for this case, especially when combined with --allow-out-cidr / --deny-out-cidr

I do not recommend merging this as-is.

[cubesandboxbot]

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

---

Review: PR #125 — cubemastercli: Refactor CubeVS params parsing logic

• Analyze current PR state after synchronize event
• Review the blocking issue from fslongjin
• Provide updated review feedback

---

Current State

The PR was synchronized by merging origin/master into the branch (commit 43642aa). No code changes were made to address the feedback from prior reviews.

Blocking Issue (Unresolved)

create-from-image regresses --allow-internet-access false usage — identified by @fslongjin.

The create-from-image command at template.go:644 now uses parseCubeVSParams(c) instead of the old mergeCreateFromImageCubeVSContextFlags(c, ...). The new function relies on urfave/cli v1 BoolFlag, which does not support --flag false syntax — false becomes a positional argument, c.Bool("allow-internet-access") returns true (the opposite of what the user requested), and the flag.FlagSet stops parsing subsequent flags like --allow-out-cidr / --deny-out-cidr.

The old code had parseCreateFromImageExtraCubeVSFlags(...) (removed in this PR) that specifically handled this case by scanning positional arguments for --allow-internet-access false and --allow-out-cidr / --deny-out-cidr values that fell through as positional args.

The related tests (TestMergeCreateFromImageCubeVSContextFlagsSupportsTrailingFalse, TestMergeCreateFromImageCubeVSContextFlagsEqualsSyntax, TestMergeCreateFromImageCubeVSContextFlagsRejectsUnexpectedArgs) were also removed, so the test suite no longer catches this regression.

Other Open Items from Prior Reviews

#	Severity	Issue	Location
2	Low	mergeCubeVSParams mutates base in-place (old code cloned first)	net.go:46-77
3	Low	CIDR deduplication removed — duplicate --allow-out-cidr values appear multiple times	net.go:61-74
4	Medium	4 tests removed, 0 added for parseCubeVSParams / mergeCubeVSParams	template_test.go

Suggested Approach

To resolve the blocking issue, the create-from-image command needs a path that handles --allow-internet-access false correctly. Options:

1. Restore the parseCreateFromImageExtraCubeVSFlags path for create-from-image only, keeping parseCubeVSParams for create and commit (which use mergeCubeVSParams and don't have this issue since they read from a JSON file as the primary input).

2. Fail explicitly when --allow-internet-access is followed by a positional false/true argument, rather than silently misinterpreting it.

3. Upgrade to cli.BoolFlag with --flag=value enforcement — the = syntax (--allow-internet-access=false) works correctly with urfave/cli v1's underlying flag package.

I do not recommend merging this PR as-is until the blocking regression is addressed.

---