Skip to content

Commit 63393b4

Browse files
chenhengqichenhengqi
committed
cubemastercli: Refactor CubeVS params parsing logic
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 Signed-off-by: Hengqi Chen <hengqi.chen@gmail.com>
1 parent ec37401 commit 63393b4

3 files changed

Lines changed: 68 additions & 241 deletions

File tree

‎CubeMaster/cmd/cubemastercli/commands/cubebox/net.go‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
package cubebox
2+
3+
import (
4+
"net"
5+
6+
"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/service/sandbox/types"
7+
"github.com/urfave/cli"
8+
)
9+
10+
const (
11+
keyAllowInternetAccess = "allow-internet-access"
12+
keyAllowOutCIDR = "allow-out-cidr"
13+
keyDenyOutCIDR = "deny-out-cidr"
14+
)
15+
16+
func parseCubeVSParams(c *cli.Context) (*types.CubeVSContext, error) {
17+
params := &types.CubeVSContext{}
18+
if c.IsSet(keyAllowInternetAccess) {
19+
val := c.Bool(keyAllowInternetAccess)
20+
params.AllowInternetAccess = &val
21+
}
22+
for _, v := range c.StringSlice(keyAllowOutCIDR) {
23+
_, _, err := net.ParseCIDR(v)
24+
if err != nil {
25+
return nil, err
26+
}
27+
params.AllowOut = append(params.AllowOut, v)
28+
}
29+
for _, v := range c.StringSlice(keyDenyOutCIDR) {
30+
_, _, err := net.ParseCIDR(v)
31+
if err != nil {
32+
return nil, err
33+
}
34+
params.DenyOut = append(params.DenyOut, v)
35+
}
36+
return params, nil
37+
}
38+
39+
func mergeCubeVSParams(c *cli.Context, base *types.CubeVSContext) (*types.CubeVSContext, error) {
40+
if c.IsSet(keyAllowInternetAccess) {
41+
val := c.Bool(keyAllowInternetAccess)
42+
base.AllowInternetAccess = &val
43+
}
44+
for _, v := range c.StringSlice(keyAllowOutCIDR) {
45+
_, _, err := net.ParseCIDR(v)
46+
if err != nil {
47+
return nil, err
48+
}
49+
base.AllowOut = append(base.AllowOut, v)
50+
}
51+
for _, v := range c.StringSlice(keyDenyOutCIDR) {
52+
_, _, err := net.ParseCIDR(v)
53+
if err != nil {
54+
return nil, err
55+
}
56+
base.DenyOut = append(base.DenyOut, v)
57+
}
58+
return base, nil
59+
}

‎CubeMaster/cmd/cubemastercli/commands/cubebox/template.go‎

Lines changed: 9 additions & 174 deletions
Original file line numberDiff line numberDiff line change
@@ -112,177 +112,6 @@ type templateDeleteRequest struct {
112112
Sync bool `json:"sync,omitempty"`
113113
}
114114

115-
func mergeCubeVSContextFlags(c *cli.Context, existing *types.CubeVSContext) *types.CubeVSContext {
116-
hasAllowInternetAccess := c.IsSet("allow-internet-access")
117-
allowOut := dedupeCIDRs(c.StringSlice("allow-out-cidr"))
118-
denyOut := dedupeCIDRs(c.StringSlice("deny-out-cidr"))
119-
return mergeCubeVSContextValues(existing, hasAllowInternetAccess, c.Bool("allow-internet-access"), allowOut, denyOut)
120-
}
121-
122-
func mergeCubeVSContextValues(existing *types.CubeVSContext, hasAllowInternetAccess bool, allowInternetAccess bool, allowOut []string, denyOut []string) *types.CubeVSContext {
123-
if !hasAllowInternetAccess && len(allowOut) == 0 && len(denyOut) == 0 {
124-
return existing
125-
}
126-
127-
out := cloneCubeVSContext(existing)
128-
if out == nil {
129-
out = &types.CubeVSContext{}
130-
}
131-
if hasAllowInternetAccess {
132-
out.AllowInternetAccess = &allowInternetAccess
133-
}
134-
if len(allowOut) > 0 {
135-
out.AllowOut = appendUniqueCIDRs(out.AllowOut, allowOut)
136-
}
137-
if len(denyOut) > 0 {
138-
out.DenyOut = appendUniqueCIDRs(out.DenyOut, denyOut)
139-
}
140-
return out
141-
}
142-
143-
type createFromImageExtraCubeVSFlags struct {
144-
hasAllowInternetAccess bool
145-
allowInternetAccess bool
146-
allowOut []string
147-
denyOut []string
148-
}
149-
150-
func mergeCreateFromImageCubeVSContextFlags(c *cli.Context, existing *types.CubeVSContext) (*types.CubeVSContext, error) {
151-
extra, err := parseCreateFromImageExtraCubeVSFlags(c)
152-
if err != nil {
153-
return nil, err
154-
}
155-
hasAllowInternetAccess := c.IsSet("allow-internet-access") || extra.hasAllowInternetAccess
156-
allowInternetAccess := c.Bool("allow-internet-access")
157-
if extra.hasAllowInternetAccess {
158-
allowInternetAccess = extra.allowInternetAccess
159-
}
160-
allowOut := appendUniqueCIDRs(dedupeCIDRs(c.StringSlice("allow-out-cidr")), extra.allowOut)
161-
denyOut := appendUniqueCIDRs(dedupeCIDRs(c.StringSlice("deny-out-cidr")), extra.denyOut)
162-
return mergeCubeVSContextValues(existing, hasAllowInternetAccess, allowInternetAccess, allowOut, denyOut), nil
163-
}
164-
165-
func parseCreateFromImageExtraCubeVSFlags(c *cli.Context) (*createFromImageExtraCubeVSFlags, error) {
166-
extraArgs := make([]string, 0, c.NArg())
167-
for i := 0; i < c.NArg(); i++ {
168-
extraArgs = append(extraArgs, c.Args().Get(i))
169-
}
170-
if len(extraArgs) == 0 {
171-
return &createFromImageExtraCubeVSFlags{}, nil
172-
}
173-
extra := &createFromImageExtraCubeVSFlags{}
174-
idx := 0
175-
176-
if c.IsSet("allow-internet-access") {
177-
if value, ok := parseBoolToken(extraArgs[idx]); ok {
178-
extra.hasAllowInternetAccess = true
179-
extra.allowInternetAccess = value
180-
idx++
181-
}
182-
}
183-
184-
for idx < len(extraArgs) {
185-
arg := extraArgs[idx]
186-
switch {
187-
case arg == "--allow-out-cidr":
188-
idx++
189-
if idx >= len(extraArgs) {
190-
return nil, errors.New("--allow-out-cidr requires a value")
191-
}
192-
extra.allowOut = append(extra.allowOut, extraArgs[idx])
193-
idx++
194-
case strings.HasPrefix(arg, "--allow-out-cidr="):
195-
extra.allowOut = append(extra.allowOut, strings.TrimPrefix(arg, "--allow-out-cidr="))
196-
idx++
197-
case arg == "--deny-out-cidr":
198-
idx++
199-
if idx >= len(extraArgs) {
200-
return nil, errors.New("--deny-out-cidr requires a value")
201-
}
202-
extra.denyOut = append(extra.denyOut, extraArgs[idx])
203-
idx++
204-
case strings.HasPrefix(arg, "--deny-out-cidr="):
205-
extra.denyOut = append(extra.denyOut, strings.TrimPrefix(arg, "--deny-out-cidr="))
206-
idx++
207-
case arg == "--allow-internet-access":
208-
idx++
209-
if idx >= len(extraArgs) {
210-
return nil, errors.New("--allow-internet-access requires true or false when passed as a trailing argument")
211-
}
212-
value, ok := parseBoolToken(extraArgs[idx])
213-
if !ok {
214-
return nil, fmt.Errorf("invalid --allow-internet-access value %q: want true or false", extraArgs[idx])
215-
}
216-
extra.hasAllowInternetAccess = true
217-
extra.allowInternetAccess = value
218-
idx++
219-
case strings.HasPrefix(arg, "--allow-internet-access="):
220-
value, ok := parseBoolToken(strings.TrimPrefix(arg, "--allow-internet-access="))
221-
if !ok {
222-
return nil, fmt.Errorf("invalid --allow-internet-access value %q: want true or false", strings.TrimPrefix(arg, "--allow-internet-access="))
223-
}
224-
extra.hasAllowInternetAccess = true
225-
extra.allowInternetAccess = value
226-
idx++
227-
default:
228-
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)
229-
}
230-
}
231-
232-
extra.allowOut = dedupeCIDRs(extra.allowOut)
233-
extra.denyOut = dedupeCIDRs(extra.denyOut)
234-
return extra, nil
235-
}
236-
237-
func parseBoolToken(value string) (bool, bool) {
238-
switch strings.ToLower(strings.TrimSpace(value)) {
239-
case "true":
240-
return true, true
241-
case "false":
242-
return false, true
243-
default:
244-
return false, false
245-
}
246-
}
247-
248-
func cloneCubeVSContext(in *types.CubeVSContext) *types.CubeVSContext {
249-
if in == nil {
250-
return nil
251-
}
252-
out := &types.CubeVSContext{
253-
AllowOut: append([]string(nil), in.AllowOut...),
254-
DenyOut: append([]string(nil), in.DenyOut...),
255-
}
256-
if in.AllowInternetAccess != nil {
257-
allowInternetAccess := *in.AllowInternetAccess
258-
out.AllowInternetAccess = &allowInternetAccess
259-
}
260-
return out
261-
}
262-
263-
func dedupeCIDRs(values []string) []string {
264-
return appendUniqueCIDRs(nil, values)
265-
}
266-
267-
func appendUniqueCIDRs(base []string, extra []string) []string {
268-
seen := make(map[string]struct{}, len(base)+len(extra))
269-
out := append([]string(nil), base...)
270-
for _, cidr := range base {
271-
seen[cidr] = struct{}{}
272-
}
273-
for _, cidr := range extra {
274-
if cidr == "" {
275-
continue
276-
}
277-
if _, ok := seen[cidr]; ok {
278-
continue
279-
}
280-
seen[cidr] = struct{}{}
281-
out = append(out, cidr)
282-
}
283-
return out
284-
}
285-
286115
func formatCubeVSContext(ctx *types.CubeVSContext) string {
287116
if ctx == nil {
288117
return "allow_internet_access=default(true) allow_out=[] deny_out=[]"
@@ -416,7 +245,10 @@ var TemplateCreateCommand = cli.Command{
416245
if scope := c.StringSlice("node"); len(scope) > 0 {
417246
req.DistributionScope = scope
418247
}
419-
req.CubeVSContext = mergeCubeVSContextFlags(c, req.CubeVSContext)
248+
req.CubeVSContext, err = mergeCubeVSParams(c, req.CubeVSContext)
249+
if err != nil {
250+
return err
251+
}
420252

421253
serverList = getServerAddrs(c)
422254
if len(serverList) == 0 {
@@ -702,7 +534,10 @@ var TemplateCommitCommand = cli.Command{
702534
createReq.Annotations = map[string]string{}
703535
}
704536
createReq.Annotations[constants.CubeAnnotationAppSnapshotTemplateID] = templateID
705-
createReq.CubeVSContext = mergeCubeVSContextFlags(c, createReq.CubeVSContext)
537+
createReq.CubeVSContext, err = mergeCubeVSParams(c, createReq.CubeVSContext)
538+
if err != nil {
539+
return err
540+
}
706541

707542
req := &templateCommitRequest{
708543
RequestID: requestID,
@@ -806,7 +641,7 @@ var TemplateCreateFromImageCommand = cli.Command{
806641
RegistryPassword: c.String("registry-password"),
807642
ContainerOverrides: containerOverrides,
808643
}
809-
req.CubeVSContext, err = mergeCreateFromImageCubeVSContextFlags(c, req.CubeVSContext)
644+
req.CubeVSContext, err = parseCubeVSParams(c)
810645
if err != nil {
811646
return err
812647
}

‎CubeMaster/cmd/cubemastercli/commands/cubebox/template_test.go‎

Lines changed: 0 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"flag"
99
"testing"
1010

11-
"github.com/tencentcloud/CubeSandbox/CubeMaster/pkg/service/sandbox/types"
1211
"github.com/urfave/cli"
1312
)
1413

@@ -70,46 +69,6 @@ func TestCreateCommandParsesNodeScope(t *testing.T) {
7069
}
7170
}
7271

73-
func TestMergeCreateFromImageCubeVSContextFlagsEqualsSyntax(t *testing.T) {
74-
ctx := newCreateFromImageContext(t, []string{
75-
"--allow-internet-access=false",
76-
"--allow-out-cidr", "172.67.0.0/16",
77-
"--deny-out-cidr", "10.0.0.0/8",
78-
})
79-
80-
got, err := mergeCreateFromImageCubeVSContextFlags(ctx, nil)
81-
if err != nil {
82-
t.Fatalf("mergeCreateFromImageCubeVSContextFlags error=%v", err)
83-
}
84-
if got == nil || got.AllowInternetAccess == nil || *got.AllowInternetAccess {
85-
t.Fatalf("AllowInternetAccess=%v, want false", got)
86-
}
87-
if len(got.AllowOut) != 1 || got.AllowOut[0] != "172.67.0.0/16" {
88-
t.Fatalf("AllowOut=%v, want [172.67.0.0/16]", got.AllowOut)
89-
}
90-
if len(got.DenyOut) != 1 || got.DenyOut[0] != "10.0.0.0/8" {
91-
t.Fatalf("DenyOut=%v, want [10.0.0.0/8]", got.DenyOut)
92-
}
93-
}
94-
95-
func TestMergeCreateFromImageCubeVSContextFlagsSupportsTrailingFalse(t *testing.T) {
96-
ctx := newCreateFromImageContext(t, []string{
97-
"--allow-internet-access", "false",
98-
"--allow-out-cidr", "172.67.0.0/16",
99-
})
100-
101-
got, err := mergeCreateFromImageCubeVSContextFlags(ctx, nil)
102-
if err != nil {
103-
t.Fatalf("mergeCreateFromImageCubeVSContextFlags error=%v", err)
104-
}
105-
if got == nil || got.AllowInternetAccess == nil || *got.AllowInternetAccess {
106-
t.Fatalf("AllowInternetAccess=%v, want false", got)
107-
}
108-
if len(got.AllowOut) != 1 || got.AllowOut[0] != "172.67.0.0/16" {
109-
t.Fatalf("AllowOut=%v, want [172.67.0.0/16]", got.AllowOut)
110-
}
111-
}
112-
11372
func TestCreateFromImageCommandParsesNodeScope(t *testing.T) {
11473
ctx := newCreateFromImageContext(t, []string{
11574
"--node", "node-a",
@@ -120,32 +79,6 @@ func TestCreateFromImageCommandParsesNodeScope(t *testing.T) {
12079
}
12180
}
12281

123-
func TestMergeCreateFromImageCubeVSContextFlagsRejectsUnexpectedArgs(t *testing.T) {
124-
ctx := newCreateFromImageContext(t, []string{
125-
"--allow-internet-access", "false",
126-
"unexpected",
127-
})
128-
129-
_, err := mergeCreateFromImageCubeVSContextFlags(ctx, nil)
130-
if err == nil {
131-
t.Fatal("expected error for unexpected trailing argument")
132-
}
133-
}
134-
135-
func TestMergeCubeVSContextValuesPreservesExistingCIDRs(t *testing.T) {
136-
existing := &types.CubeVSContext{
137-
AllowOut: []string{"192.168.0.0/16"},
138-
}
139-
140-
got := mergeCubeVSContextValues(existing, true, false, []string{"172.67.0.0/16"}, nil)
141-
if got == nil || got.AllowInternetAccess == nil || *got.AllowInternetAccess {
142-
t.Fatalf("AllowInternetAccess=%v, want false", got)
143-
}
144-
if len(got.AllowOut) != 2 || got.AllowOut[0] != "192.168.0.0/16" || got.AllowOut[1] != "172.67.0.0/16" {
145-
t.Fatalf("AllowOut=%v, want merged CIDRs", got.AllowOut)
146-
}
147-
}
148-
14982
func TestRedoCommandParsesNodeScope(t *testing.T) {
15083
ctx := newRedoContext(t, []string{
15184
"--template-id", "tpl-1",

0 commit comments

Comments
 (0)