Skip to content

cubemastercli: Refactor CubeVS params parsing logic #125

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chenhengqi wants to merge 2 commits into
base: master
Choose a base branch
from
Open

cubemastercli: Refactor CubeVS params parsing logic #125

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d00668c
cubemastercli: Refactor CubeVS params parsing logic
chenhengqi Apr 29, 2026
43642aa
Merge remote-tracking branch 'origin/master' into fix-tpl-net-params
fslongjin May 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions CubeMaster/cmd/cubemastercli/commands/cubebox/net.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package cubebox

import (
"net"

"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/service/sandbox/types"
"github.com/urfave/cli"
)

const (
keyAllowInternetAccess = "allow-internet-access"
keyAllowOutCIDR = "allow-out-cidr"
keyDenyOutCIDR = "deny-out-cidr"
)

func parseCubeVSParams(c *cli.Context) (*types.CubeVSContext, error) {
if !c.IsSet(keyAllowInternetAccess) &&
len(c.StringSlice(keyAllowOutCIDR)) == 0 &&
len(c.StringSlice(keyDenyOutCIDR)) == 0 {
return nil, nil
}

params := &types.CubeVSContext{}
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

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)
}

return params, nil
}

func mergeCubeVSParams(c *cli.Context, base *types.CubeVSContext) (*types.CubeVSContext, error) {
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

if !c.IsSet(keyAllowInternetAccess) &&
len(c.StringSlice(keyAllowOutCIDR)) == 0 &&
len(c.StringSlice(keyDenyOutCIDR)) == 0 {
return base, nil
}

if base == nil {
base = &types.CubeVSContext{}
}

if c.IsSet(keyAllowInternetAccess) {
val := c.Bool(keyAllowInternetAccess)
base.AllowInternetAccess = &val
}
for _, v := range c.StringSlice(keyAllowOutCIDR) {
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_, _, err := net.ParseCIDR(v)
if err != nil {
return nil, err
}
base.AllowOut = append(base.AllowOut, v)
}
for _, v := range c.StringSlice(keyDenyOutCIDR) {
_, _, err := net.ParseCIDR(v)
if err != nil {
return nil, err
}
base.DenyOut = append(base.DenyOut, v)
}

return base, nil
}
183 changes: 9 additions & 174 deletions CubeMaster/cmd/cubemastercli/commands/cubebox/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,177 +112,6 @@ type templateDeleteRequest struct {
Sync bool `json:"sync,omitempty"`
}

func mergeCubeVSContextFlags(c *cli.Context, existing *types.CubeVSContext) *types.CubeVSContext {
hasAllowInternetAccess := c.IsSet("allow-internet-access")
allowOut := dedupeCIDRs(c.StringSlice("allow-out-cidr"))
denyOut := dedupeCIDRs(c.StringSlice("deny-out-cidr"))
return mergeCubeVSContextValues(existing, hasAllowInternetAccess, c.Bool("allow-internet-access"), allowOut, denyOut)
}

func mergeCubeVSContextValues(existing *types.CubeVSContext, hasAllowInternetAccess bool, allowInternetAccess bool, allowOut []string, denyOut []string) *types.CubeVSContext {
if !hasAllowInternetAccess && len(allowOut) == 0 && len(denyOut) == 0 {
return existing
}

out := cloneCubeVSContext(existing)
if out == nil {
out = &types.CubeVSContext{}
}
if hasAllowInternetAccess {
out.AllowInternetAccess = &allowInternetAccess
}
if len(allowOut) > 0 {
out.AllowOut = appendUniqueCIDRs(out.AllowOut, allowOut)
}
if len(denyOut) > 0 {
out.DenyOut = appendUniqueCIDRs(out.DenyOut, denyOut)
}
return out
}

type createFromImageExtraCubeVSFlags struct {
hasAllowInternetAccess bool
allowInternetAccess bool
allowOut []string
denyOut []string
}

func mergeCreateFromImageCubeVSContextFlags(c *cli.Context, existing *types.CubeVSContext) (*types.CubeVSContext, error) {
extra, err := parseCreateFromImageExtraCubeVSFlags(c)
if err != nil {
return nil, err
}
hasAllowInternetAccess := c.IsSet("allow-internet-access") || extra.hasAllowInternetAccess
allowInternetAccess := c.Bool("allow-internet-access")
if extra.hasAllowInternetAccess {
allowInternetAccess = extra.allowInternetAccess
}
allowOut := appendUniqueCIDRs(dedupeCIDRs(c.StringSlice("allow-out-cidr")), extra.allowOut)
denyOut := appendUniqueCIDRs(dedupeCIDRs(c.StringSlice("deny-out-cidr")), extra.denyOut)
return mergeCubeVSContextValues(existing, hasAllowInternetAccess, allowInternetAccess, allowOut, denyOut), nil
}

func parseCreateFromImageExtraCubeVSFlags(c *cli.Context) (*createFromImageExtraCubeVSFlags, error) {
extraArgs := make([]string, 0, c.NArg())
for i := 0; i < c.NArg(); i++ {
extraArgs = append(extraArgs, c.Args().Get(i))
}
if len(extraArgs) == 0 {
return &createFromImageExtraCubeVSFlags{}, nil
}
extra := &createFromImageExtraCubeVSFlags{}
idx := 0

if c.IsSet("allow-internet-access") {
if value, ok := parseBoolToken(extraArgs[idx]); ok {
extra.hasAllowInternetAccess = true
extra.allowInternetAccess = value
idx++
}
}

for idx < len(extraArgs) {
arg := extraArgs[idx]
switch {
case arg == "--allow-out-cidr":
idx++
if idx >= len(extraArgs) {
return nil, errors.New("--allow-out-cidr requires a value")
}
extra.allowOut = append(extra.allowOut, extraArgs[idx])
idx++
case strings.HasPrefix(arg, "--allow-out-cidr="):
extra.allowOut = append(extra.allowOut, strings.TrimPrefix(arg, "--allow-out-cidr="))
idx++
case arg == "--deny-out-cidr":
idx++
if idx >= len(extraArgs) {
return nil, errors.New("--deny-out-cidr requires a value")
}
extra.denyOut = append(extra.denyOut, extraArgs[idx])
idx++
case strings.HasPrefix(arg, "--deny-out-cidr="):
extra.denyOut = append(extra.denyOut, strings.TrimPrefix(arg, "--deny-out-cidr="))
idx++
case arg == "--allow-internet-access":
idx++
if idx >= len(extraArgs) {
return nil, errors.New("--allow-internet-access requires true or false when passed as a trailing argument")
}
value, ok := parseBoolToken(extraArgs[idx])
if !ok {
return nil, fmt.Errorf("invalid --allow-internet-access value %q: want true or false", extraArgs[idx])
}
extra.hasAllowInternetAccess = true
extra.allowInternetAccess = value
idx++
case strings.HasPrefix(arg, "--allow-internet-access="):
value, ok := parseBoolToken(strings.TrimPrefix(arg, "--allow-internet-access="))
if !ok {
return nil, fmt.Errorf("invalid --allow-internet-access value %q: want true or false", strings.TrimPrefix(arg, "--allow-internet-access="))
}
extra.hasAllowInternetAccess = true
extra.allowInternetAccess = value
idx++
default:
return nil, fmt.Errorf("unexpected positional or trailing argument %q; use --allow-internet-access=false or place bool values at the end only when explicitly supported", arg)
}
}

extra.allowOut = dedupeCIDRs(extra.allowOut)
extra.denyOut = dedupeCIDRs(extra.denyOut)
return extra, nil
}

func parseBoolToken(value string) (bool, bool) {
switch strings.ToLower(strings.TrimSpace(value)) {
case "true":
return true, true
case "false":
return false, true
default:
return false, false
}
}

func cloneCubeVSContext(in *types.CubeVSContext) *types.CubeVSContext {
if in == nil {
return nil
}
out := &types.CubeVSContext{
AllowOut: append([]string(nil), in.AllowOut...),
DenyOut: append([]string(nil), in.DenyOut...),
}
if in.AllowInternetAccess != nil {
allowInternetAccess := *in.AllowInternetAccess
out.AllowInternetAccess = &allowInternetAccess
}
return out
}

func dedupeCIDRs(values []string) []string {
return appendUniqueCIDRs(nil, values)
}

func appendUniqueCIDRs(base []string, extra []string) []string {
seen := make(map[string]struct{}, len(base)+len(extra))
out := append([]string(nil), base...)
for _, cidr := range base {
seen[cidr] = struct{}{}
}
for _, cidr := range extra {
if cidr == "" {
continue
}
if _, ok := seen[cidr]; ok {
continue
}
seen[cidr] = struct{}{}
out = append(out, cidr)
}
return out
}

func formatCubeVSContext(ctx *types.CubeVSContext) string {
if ctx == nil {
return "allow_internet_access=default(true) allow_out=[] deny_out=[]"
Expand Down Expand Up @@ -416,7 +245,10 @@ var TemplateCreateCommand = cli.Command{
if scope := c.StringSlice("node"); len(scope) > 0 {
req.DistributionScope = scope
}
req.CubeVSContext = mergeCubeVSContextFlags(c, req.CubeVSContext)
req.CubeVSContext, err = mergeCubeVSParams(c, req.CubeVSContext)
if err != nil {
return err
}

serverList = getServerAddrs(c)
if len(serverList) == 0 {
Expand Down Expand Up @@ -702,7 +534,10 @@ var TemplateCommitCommand = cli.Command{
createReq.Annotations = map[string]string{}
}
createReq.Annotations[constants.CubeAnnotationAppSnapshotTemplateID] = templateID
createReq.CubeVSContext = mergeCubeVSContextFlags(c, createReq.CubeVSContext)
createReq.CubeVSContext, err = mergeCubeVSParams(c, createReq.CubeVSContext)
if err != nil {
return err
}

req := &templateCommitRequest{
RequestID: requestID,
Expand Down Expand Up @@ -806,7 +641,7 @@ var TemplateCreateFromImageCommand = cli.Command{
RegistryPassword: c.String("registry-password"),
ContainerOverrides: containerOverrides,
}
req.CubeVSContext, err = mergeCreateFromImageCubeVSContextFlags(c, req.CubeVSContext)
req.CubeVSContext, err = parseCubeVSParams(c)
if err != nil {
return err
}
Expand Down
67 changes: 0 additions & 67 deletions CubeMaster/cmd/cubemastercli/commands/cubebox/template_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import (
"strings"
"testing"

"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/service/sandbox/types"
"github.com/urfave/cli"
)

Expand Down Expand Up @@ -71,46 +70,6 @@ func TestCreateCommandParsesNodeScope(t *testing.T) {
}
Copy link
Copy Markdown

@github-actions github-actions Bot Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

func TestMergeCreateFromImageCubeVSContextFlagsEqualsSyntax(t *testing.T) {
ctx := newCreateFromImageContext(t, []string{
"--allow-internet-access=false",
"--allow-out-cidr", "172.67.0.0/16",
"--deny-out-cidr", "10.0.0.0/8",
})

got, err := mergeCreateFromImageCubeVSContextFlags(ctx, nil)
if err != nil {
t.Fatalf("mergeCreateFromImageCubeVSContextFlags error=%v", err)
}
if got == nil || got.AllowInternetAccess == nil || *got.AllowInternetAccess {
t.Fatalf("AllowInternetAccess=%v, want false", got)
}
if len(got.AllowOut) != 1 || got.AllowOut[0] != "172.67.0.0/16" {
t.Fatalf("AllowOut=%v, want [172.67.0.0/16]", got.AllowOut)
}
if len(got.DenyOut) != 1 || got.DenyOut[0] != "10.0.0.0/8" {
t.Fatalf("DenyOut=%v, want [10.0.0.0/8]", got.DenyOut)
}
}

func TestMergeCreateFromImageCubeVSContextFlagsSupportsTrailingFalse(t *testing.T) {
ctx := newCreateFromImageContext(t, []string{
"--allow-internet-access", "false",
"--allow-out-cidr", "172.67.0.0/16",
})

got, err := mergeCreateFromImageCubeVSContextFlags(ctx, nil)
if err != nil {
t.Fatalf("mergeCreateFromImageCubeVSContextFlags error=%v", err)
}
if got == nil || got.AllowInternetAccess == nil || *got.AllowInternetAccess {
t.Fatalf("AllowInternetAccess=%v, want false", got)
}
if len(got.AllowOut) != 1 || got.AllowOut[0] != "172.67.0.0/16" {
t.Fatalf("AllowOut=%v, want [172.67.0.0/16]", got.AllowOut)
}
}

func TestCreateFromImageCommandParsesNodeScope(t *testing.T) {
ctx := newCreateFromImageContext(t, []string{
"--node", "node-a",
Expand All @@ -121,32 +80,6 @@ func TestCreateFromImageCommandParsesNodeScope(t *testing.T) {
}
}

func TestMergeCreateFromImageCubeVSContextFlagsRejectsUnexpectedArgs(t *testing.T) {
ctx := newCreateFromImageContext(t, []string{
"--allow-internet-access", "false",
"unexpected",
})

_, err := mergeCreateFromImageCubeVSContextFlags(ctx, nil)
if err == nil {
t.Fatal("expected error for unexpected trailing argument")
}
}

func TestMergeCubeVSContextValuesPreservesExistingCIDRs(t *testing.T) {
existing := &types.CubeVSContext{
AllowOut: []string{"192.168.0.0/16"},
}

got := mergeCubeVSContextValues(existing, true, false, []string{"172.67.0.0/16"}, nil)
if got == nil || got.AllowInternetAccess == nil || *got.AllowInternetAccess {
t.Fatalf("AllowInternetAccess=%v, want false", got)
}
if len(got.AllowOut) != 2 || got.AllowOut[0] != "192.168.0.0/16" || got.AllowOut[1] != "172.67.0.0/16" {
t.Fatalf("AllowOut=%v, want merged CIDRs", got.AllowOut)
}
}

func TestRedoCommandParsesNodeScope(t *testing.T) {
ctx := newRedoContext(t, []string{
"--template-id", "tpl-1",
Expand Down
Loading