Skip to content

fix(CubeMaster): preserve template network rules and resource defaults #581

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
zyl1121 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix(CubeMaster): preserve template network rules and resource defaults #581

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
51 changes: 32 additions & 19 deletions CubeMaster/pkg/service/httpservice/cube/cubeboxutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ func mergeCubeNetworkConfigs(templateCfg *types.CubeNetworkConfig, requestCfg *t
return cloneCubeNetworkConfig(templateCfg)
}

out := cloneCubeNetworkConfig(templateCfg)
out := cloneCubeNetworkConfigBase(templateCfg)
if requestCfg.AllowInternetAccess != nil {
allowInternetAccess := *requestCfg.AllowInternetAccess
out.AllowInternetAccess = &allowInternetAccess
Expand All @@ -285,19 +285,29 @@ func mergeCubeNetworkConfigs(templateCfg *types.CubeNetworkConfig, requestCfg *t
out.DenyOut = appendUniqueCIDRs(out.DenyOut, requestCfg.DenyOut)
}
if len(requestCfg.Rules) > 0 {
out.Rules = mergeEgressRules(out.Rules, requestCfg.Rules)
out.Rules = mergeEgressRules(templateCfg.Rules, requestCfg.Rules)
} else {
out.Rules = cloneEgressRules(templateCfg.Rules)
}
return out
}

func cloneCubeNetworkConfig(in *types.CubeNetworkConfig) *types.CubeNetworkConfig {
out := cloneCubeNetworkConfigBase(in)
if out == nil {
return nil
}
out.Rules = cloneEgressRules(in.Rules)
return out
}

func cloneCubeNetworkConfigBase(in *types.CubeNetworkConfig) *types.CubeNetworkConfig {
if in == nil {
return nil
}
out := &types.CubeNetworkConfig{
AllowOut: append([]string(nil), in.AllowOut...),
DenyOut: append([]string(nil), in.DenyOut...),
Rules: cloneEgressRules(in.Rules),
}
if in.AllowInternetAccess != nil {
allowInternetAccess := *in.AllowInternetAccess
Expand All @@ -310,32 +320,35 @@ func cloneCubeNetworkConfig(in *types.CubeNetworkConfig) *types.CubeNetworkConfi
return out
}

// mergeEgressRules combines template + request rules. Rules sharing the same
// Name are overridden by the request side; otherwise request rules are
// appended after template rules to preserve first-match-wins ordering with
// the template's policy taking precedence on overlap.
// mergeEgressRules combines template + request rules. Because egress rules are
// first-match-wins, per-sandbox/request rules must come before template rules.
// Template rules whose Name is overridden by the request side are skipped.
func mergeEgressRules(base []*types.EgressRule, extra []*types.EgressRule) []*types.EgressRule {

@cubesandboxbot cubesandboxbot Bot Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Design consideration: name-based dedup removes template rules entirely, not just deprioritizes them

Request rules are placed before template rules, which is correct for first-match-wins precedence. However, the additional name-based dedup (overridden map, line ~341) removes template rules from the output entirely when their name matches a request rule, rather than just letting them be deprioritized by position.

This means a sandbox user who knows a template rule's name (e.g., "block-metadata") can craft a request rule with the same name and a permissive action to bypass that template security rule. The template rule would be gone from the evaluated set — not just out-prioritized.

The old code (in-place overwrite in the base list) had the same concern, and at minimum the behavior should be documented as a known trait. If template rules are meant to be mandatory security boundaries, consider using position-only precedence and removing the name-based dedup.

// Option: position-only precedence, no name-based removal
out := make([]*types.EgressRule, 0, len(extra)+len(base))
for _, r := range extra {
    out = append(out, cloneEgressRule(r))
}
for _, r := range base {
    out = append(out, cloneEgressRule(r))
}

if len(extra) == 0 {

@cubesandboxbot cubesandboxbot Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: mergeEgressRules now always clones (defensive, correct)

The new code on both early-return paths (len(extra)==0 → returns cloneEgressRules(base), and len(base)==0 → returns cloneEgressRules(extra)) always returns a deep copy. The old code returned base directly when no extra rules were provided.

This pairs with mergeCubeNetworkConfigs (line 267+279): out is already a clone of templateCfg, but mergeEgressRules re-clones it. When both sides have rules, template rules are cloned twice — once in cloneCubeNetworkConfig and again inside mergeEgressRules. Consider passing templateCfg.Rules directly to mergeEgressRules to avoid the redundant clone.

@cubesandboxbot cubesandboxbot Bot Jun 17, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

This PR fixes three data-loss bugs in the template/sandbox request materialization path. The changes are well-scoped and the reasoning is clearly articulated. Below are the findings I consider noteworthy.

Pre-existing bug: parse errors silently dropped in getReqResource (Not PR-introduced)

File: CubeMaster/pkg/service/sandbox/util.go:90-98

The resource.ParseQuantity calls use := which declares a new for-loop-scoped err, shadowing the named return err from the function signature. After break, the for-loop-scoped err goes out of scope and line 104 reads the named return err which is still nil.

Suggestion: Use = instead of := to write to the named return err.

Docs describe wrong rule merge ordering

Files:

  • docs/guide/egress-network-policy.md (English, line 197)
  • docs/zh/guide/egress-network-policy.md (Chinese, line 197)

Both state that request rules with new names are appended after template rules. The mergeEgressRules implementation puts all request rules first, then template rules minus overridden ones. The code is correct; the docs should match.

Minor: redundant clone when both sides have egress rules

When both templateCfg and requestCfg contain rules (cubeboxutil.go:267,279), template rules are deep-cloned twice — once in cloneCubeNetworkConfig and again in mergeEgressRules.

Positive observations

  • The getReqResource early-return fix is correct and unambiguous.
  • Test coverage verifies pointer independence at all nesting levels.
  • Nil entry handling in mergeEgressRules is robust.

return base
return cloneEgressRules(base)
}
indexByName := make(map[string]int, len(base))
out := cloneEgressRules(base)
for i, r := range out {
if r != nil {
indexByName[r.Name] = i
}
if len(base) == 0 {
return cloneEgressRules(extra)
}

out := make([]*types.EgressRule, 0, len(extra)+len(base))
overridden := make(map[string]struct{}, len(extra))
for _, r := range extra {
if r == nil {
continue
}
cloned := cloneEgressRule(r)
if idx, ok := indexByName[r.Name]; ok {
out[idx] = cloned
out = append(out, cloneEgressRule(r))
overridden[r.Name] = struct{}{}
}

for _, r := range base {
if r == nil {
continue
}
indexByName[r.Name] = len(out)
out = append(out, cloned)
if _, ok := overridden[r.Name]; ok {
continue
}
out = append(out, cloneEgressRule(r))
}
return out
}
Expand Down
203 changes: 203 additions & 0 deletions CubeMaster/pkg/service/httpservice/cube/cubeboxutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,203 @@
// Copyright (c) 2026 Tencent Inc.
// SPDX-License-Identifier: Apache-2.0
//

package cube

import (
"testing"

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

func TestMergeEgressRulesPrependsRequestRulesAndOverridesTemplateByName(t *testing.T) {
templateRules := []*types.EgressRule{
{
Name: "shared",
Match: &types.EgressRuleMatch{
Host: strPtr("template.example.com"),
},
Action: &types.EgressRuleAction{Allow: false},
},
{
Name: "template-only",
Match: &types.EgressRuleMatch{
Host: strPtr("template-only.example.com"),
},
Action: &types.EgressRuleAction{Allow: true},
},
}
requestRules := []*types.EgressRule{
{
Name: "shared",
Match: &types.EgressRuleMatch{
Host: strPtr("request.example.com"),
},
Action: &types.EgressRuleAction{Allow: true},
},
{
Name: "request-only",
Match: &types.EgressRuleMatch{
Host: strPtr("request-only.example.com"),
},
Action: &types.EgressRuleAction{Allow: false},
},
}

got := mergeEgressRules(templateRules, requestRules)
if len(got) != 3 {
t.Fatalf("expected 3 merged rules, got %d", len(got))
}
if got[0].Name != "shared" || got[1].Name != "request-only" || got[2].Name != "template-only" {
t.Fatalf("unexpected merged rule order: %#v", []string{got[0].Name, got[1].Name, got[2].Name})
}
if got[0] == requestRules[0] || got[1] == requestRules[1] || got[2] == templateRules[1] {
t.Fatal("expected merged rules to be cloned, got shared pointers")
}
if got[0].Match == nil || got[0].Match.Host == nil || *got[0].Match.Host != "request.example.com" {
t.Fatalf("expected request override to win for shared rule, got %+v", got[0])
}
}

func TestMergeEgressRulesPrependsRequestRulesWithoutNameConflict(t *testing.T) {
templateRules := []*types.EgressRule{
{Name: "template-a", Action: &types.EgressRuleAction{Allow: true}},
{Name: "template-b", Action: &types.EgressRuleAction{Allow: false}},
}
requestRules := []*types.EgressRule{
{Name: "request-a", Action: &types.EgressRuleAction{Allow: true}},
{Name: "request-b", Action: &types.EgressRuleAction{Allow: false}},
}

got := mergeEgressRules(templateRules, requestRules)
if len(got) != 4 {
t.Fatalf("expected 4 merged rules, got %d", len(got))
}
gotNames := []string{got[0].Name, got[1].Name, got[2].Name, got[3].Name}
wantNames := []string{"request-a", "request-b", "template-a", "template-b"}
for i := range wantNames {
if gotNames[i] != wantNames[i] {
t.Fatalf("unexpected merged rule order: got=%v want=%v", gotNames, wantNames)
}
}
if got[0] == requestRules[0] || got[1] == requestRules[1] || got[2] == templateRules[0] || got[3] == templateRules[1] {
t.Fatal("expected merged rules to be cloned, got shared pointers")
}
}

func TestMergeEgressRulesHandlesEmptySides(t *testing.T) {
templateRules := []*types.EgressRule{
{Name: "template-a", Match: &types.EgressRuleMatch{Host: strPtr("template.example.com")}},
}
requestRules := []*types.EgressRule{
{Name: "request-a", Match: &types.EgressRuleMatch{Host: strPtr("request.example.com")}},
}

gotTemplateOnly := mergeEgressRules(templateRules, nil)
if len(gotTemplateOnly) != 1 || gotTemplateOnly[0].Name != "template-a" {
t.Fatalf("unexpected template-only merge result: %+v", gotTemplateOnly)
}
if gotTemplateOnly[0] == templateRules[0] {
t.Fatal("expected template-only merge to clone rules")
}

gotRequestOnly := mergeEgressRules(nil, requestRules)
if len(gotRequestOnly) != 1 || gotRequestOnly[0].Name != "request-a" {
t.Fatalf("unexpected request-only merge result: %+v", gotRequestOnly)
}
if gotRequestOnly[0] == requestRules[0] {
t.Fatal("expected request-only merge to clone rules")
}
}

func TestMergeEgressRulesSkipsNilEntries(t *testing.T) {
templateRules := []*types.EgressRule{
nil,
{Name: "template-a", Action: &types.EgressRuleAction{Allow: true}},
}
requestRules := []*types.EgressRule{
nil,
{Name: "request-a", Action: &types.EgressRuleAction{Allow: false}},
}

got := mergeEgressRules(templateRules, requestRules)
if len(got) != 2 {
t.Fatalf("expected nil entries to be skipped, got %d merged rules", len(got))
}
if got[0].Name != "request-a" || got[1].Name != "template-a" {
t.Fatalf("unexpected merged rule order with nil entries: %#v", []string{got[0].Name, got[1].Name})
}
}

func TestMergeCubeNetworkConfigsMergesRulesWithoutAliasingTemplate(t *testing.T) {
templateAllow := false
requestAllow := true
templateCfg := &types.CubeNetworkConfig{
AllowInternetAccess: &templateAllow,
AllowOut: []string{"10.0.0.0/8"},
DenyOut: []string{"192.168.0.0/16"},
Rules: []*types.EgressRule{
{Name: "template-only", Match: &types.EgressRuleMatch{Host: strPtr("template.example.com")}},
},
}
requestCfg := &types.CubeNetworkConfig{
AllowInternetAccess: &requestAllow,
AllowOut: []string{"172.16.0.0/12"},
DenyOut: []string{"100.64.0.0/10"},
Rules: []*types.EgressRule{
{Name: "request-only", Match: &types.EgressRuleMatch{Host: strPtr("request.example.com")}},
},
}

got := mergeCubeNetworkConfigs(templateCfg, requestCfg)
if got == nil {
t.Fatal("expected merged config, got nil")
}
if got.AllowInternetAccess == nil || !*got.AllowInternetAccess {
t.Fatalf("expected request allowInternetAccess override, got %+v", got.AllowInternetAccess)
}
if len(got.AllowOut) != 2 || got.AllowOut[0] != "10.0.0.0/8" || got.AllowOut[1] != "172.16.0.0/12" {
t.Fatalf("unexpected merged allowOut: %#v", got.AllowOut)
}
if len(got.DenyOut) != 2 || got.DenyOut[0] != "192.168.0.0/16" || got.DenyOut[1] != "100.64.0.0/10" {
t.Fatalf("unexpected merged denyOut: %#v", got.DenyOut)
}
if len(got.Rules) != 2 || got.Rules[0].Name != "request-only" || got.Rules[1].Name != "template-only" {
t.Fatalf("unexpected merged rules: %#v", []string{got.Rules[0].Name, got.Rules[1].Name})
}
if got.Rules[0] == requestCfg.Rules[0] || got.Rules[1] == templateCfg.Rules[0] {
t.Fatal("expected merged rules to be cloned, got shared pointers")
}
}

func TestMergeCubeNetworkConfigsClonesTemplateRulesWhenRequestRulesEmpty(t *testing.T) {
templateAllow := false
requestAllow := true
templateCfg := &types.CubeNetworkConfig{
AllowInternetAccess: &templateAllow,
Rules: []*types.EgressRule{
{Name: "template-only", Match: &types.EgressRuleMatch{Host: strPtr("template.example.com")}},
},
}
requestCfg := &types.CubeNetworkConfig{
AllowInternetAccess: &requestAllow,
}

got := mergeCubeNetworkConfigs(templateCfg, requestCfg)
if got == nil {
t.Fatal("expected merged config, got nil")
}
if got.AllowInternetAccess == nil || !*got.AllowInternetAccess {
t.Fatalf("expected request allowInternetAccess override, got %+v", got.AllowInternetAccess)
}
if len(got.Rules) != 1 || got.Rules[0].Name != "template-only" {
t.Fatalf("unexpected merged rules: %#v", got.Rules)
}
if got.Rules[0] == templateCfg.Rules[0] {
t.Fatal("expected template rules to be cloned when request has no rules")
}
}

func strPtr(s string) *string {
return &s
}
4 changes: 2 additions & 2 deletions CubeMaster/pkg/service/sandbox/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,11 @@ func getReqResource(req *types.CreateCubeSandboxReq) (cpu, mem resource.Quantity

if config.GetConfig().Scheduler != nil {
if cpu.Cmp(config.GetConfig().Scheduler.MaxMvmCPURes()) >= 0 {
err = ret.Errorf(errorcode.ErrorCode_MasterParamsError, "request Resources cpu[%dm] is invalid",
return cpu, mem, ret.Errorf(errorcode.ErrorCode_MasterParamsError, "request Resources cpu[%dm] is invalid",
cpu.MilliValue())
}
if mem.Cmp(config.GetConfig().Scheduler.MaxMvmMemoryRes()) >= 0 {
err = ret.Errorf(errorcode.ErrorCode_MasterParamsError, "request Resources mem[%dKB] is invalid",
return cpu, mem, ret.Errorf(errorcode.ErrorCode_MasterParamsError, "request Resources mem[%dKB] is invalid",

@cubesandboxbot cubesandboxbot Bot Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Minor: double space in error message

"request Resources mem[%dKB] is invalid" has an extra space between "Resources" and "mem". The CPU message above (line 110) uses a single space: "request Resources cpu[%dm] is invalid".

mem.Value()/1024)
}
}
Expand Down
Loading
Loading