feat(egress): add patch policy updates and smoke coverage (opensandbox-group#392)

* feat(egress): add patch policy updates and smoke coverage

* feat(egress): add patch override ordering and docs

Author: Pangjiping

components/egress/README.md

@@ -2,9 +2,6 @@
 
 The **Egress Sidecar** is a core component of OpenSandbox that provides **FQDN-based egress control**. It runs alongside the sandbox application container (sharing the same network namespace) and enforces declared network policies.
 
-> **Status**: Implementing. Currently supports Layer 1 (DNS Proxy). Layer 2 (Network Filter) is on the roadmap.
-> See [OSEP-0001: FQDN-based Egress Control](../../oseps/0001-fqdn-based-egress-control.md) for the detailed design.
-
 ## Features
 
 - **FQDN-based Allowlist**: Control outbound traffic by domain name (e.g., `api.github.com`).
@@ -59,8 +56,9 @@ The egress control is implemented as a **Sidecar** that shares the network names
 
 - Default listen address: `:18080` (override with `OPENSANDBOX_EGRESS_HTTP_ADDR`).
 - Endpoints:
-  - `GET /policy` — returns the current policy.
-  - `POST /policy` — replaces the policy. Empty/whitespace/`{}`/`null` resets to default deny-all.
+- `GET /policy` — returns the current policy.
+- `POST /policy` — replaces the policy. Empty/whitespace/`{}`/`null` resets to default deny-all.
+  - `PATCH /policy` — merge/append rules at runtime. Body **must** be a JSON array of egress rules (not wrapped in an object). New rules are placed before existing ones (same target overrides), so a later PATCH can override prior wildcard denies with a more specific allow, and vice versa.
 
 Examples:
 
@@ -84,6 +82,16 @@ Examples:
   curl -XPOST http://127.0.0.1:18080/policy \
     -d '{"defaultAction":"deny","egress":[{"action":"allow","target":"*.example.com"},{"action":"allow","target":"203.0.113.0/24"},{"action":"deny","target":"*.bad.com"}]}'
   ```
+- Merge-only PATCH (override wildcard deny with a specific allow):
+  ```bash
+  # baseline: deny *.cloudflare.com
+  curl -XPOST http://127.0.0.1:18080/policy \
+    -d '{"defaultAction":"allow","egress":[{"action":"deny","target":"*.cloudflare.com"}]}'
+
+  # allow a specific host; PATCH rules are prepended, so this wins
+  curl -XPATCH http://127.0.0.1:18080/policy \
+    -d '[{"action":"allow","target":"www.cloudflare.com"}]'
+  ```
 
 ## Build & Run
 

components/egress/policy_server.go

@@ -24,6 +24,7 @@ import (
 	"net/http"
 	"net/netip"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/alibaba/opensandbox/egress/pkg/constants"
@@ -104,6 +105,15 @@ type policyServer struct {
 	token           string
 	enforcementMode string
 	nameserverIPs   []netip.Addr
+	mu              sync.Mutex // serializes read-merge-apply to avoid lost updates across POST/PATCH
+}
+
+type policyStatusResponse struct {
+	Status          string `json:"status,omitempty"`
+	Mode            string `json:"mode,omitempty"`
+	EnforcementMode string `json:"enforcementMode,omitempty"`
+	Reason          string `json:"reason,omitempty"`
+	Policy          any    `json:"policy,omitempty"`
 }
 
 func (s *policyServer) handlePolicy(w http.ResponseWriter, r *http.Request) {
@@ -116,24 +126,29 @@ func (s *policyServer) handlePolicy(w http.ResponseWriter, r *http.Request) {
 		s.handleGet(w)
 	case http.MethodPost, http.MethodPut:
 		s.handlePost(w, r)
+	case http.MethodPatch:
+		s.handlePatch(w, r)
 	default:
-		w.Header().Set("Allow", "GET, POST, PUT")
+		w.Header().Set("Allow", "GET, POST, PUT, PATCH")
 		http.Error(w, "method not allowed", http.StatusMethodNotAllowed)
 	}
 }
 
 func (s *policyServer) handleGet(w http.ResponseWriter) {
 	current := s.proxy.CurrentPolicy()
 	mode := modeFromPolicy(current)
-	writeJSON(w, http.StatusOK, map[string]any{
-		"mode":            mode,
-		"enforcementMode": s.enforcementMode,
-		"policy":          current,
+	writeJSON(w, http.StatusOK, policyStatusResponse{
+		Status:          "ok",
+		Mode:            mode,
+		EnforcementMode: s.enforcementMode,
+		Policy:          current,
 	})
 }
 
 func (s *policyServer) handlePost(w http.ResponseWriter, r *http.Request) {
 	defer r.Body.Close()
+	s.mu.Lock()
+	defer s.mu.Unlock()
 
 	body, err := io.ReadAll(io.LimitReader(r.Body, 1<<20)) // 1MB limit
 	if err != nil {
@@ -154,10 +169,10 @@ func (s *policyServer) handlePost(w http.ResponseWriter, r *http.Request) {
 		}
 		s.proxy.UpdatePolicy(def)
 		log.Infof("policy API: proxy and nftables updated to deny_all")
-		writeJSON(w, http.StatusOK, map[string]any{
-			"status": "ok",
-			"mode":   "deny_all",
-			"reason": "policy reset to default deny-all",
+		writeJSON(w, http.StatusOK, policyStatusResponse{
+			Status: "ok",
+			Mode:   "deny_all",
+			Reason: "policy reset to default deny-all",
 		})
 		return
 	}
@@ -179,10 +194,78 @@ func (s *policyServer) handlePost(w http.ResponseWriter, r *http.Request) {
 	}
 	s.proxy.UpdatePolicy(pol)
 	log.Infof("policy API: proxy and nftables updated successfully")
-	writeJSON(w, http.StatusOK, map[string]any{
-		"status":          "ok",
-		"mode":            mode,
-		"enforcementMode": s.enforcementMode,
+	writeJSON(w, http.StatusOK, policyStatusResponse{
+		Status:          "ok",
+		Mode:            mode,
+		EnforcementMode: s.enforcementMode,
+	})
+}
+
+// handlePatch adds or replaces egress rules by merging with the current policy.
+// It is a convenience wrapper over the full replace flow: we still read -> merge -> apply.
+// Request body supports {"egress":[{"action":"allow","target":"example.com"}, ...]}.
+func (s *policyServer) handlePatch(w http.ResponseWriter, r *http.Request) {
+	defer r.Body.Close()
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	body, err := io.ReadAll(io.LimitReader(r.Body, 1<<20)) // 1MB limit
+	if err != nil {
+		http.Error(w, fmt.Sprintf("failed to read body: %v", err), http.StatusBadRequest)
+		return
+	}
+	raw := strings.TrimSpace(string(body))
+	if raw == "" {
+		http.Error(w, "patch body cannot be empty", http.StatusBadRequest)
+		return
+	}
+
+	var patchRules []policy.EgressRule
+	if err = json.Unmarshal([]byte(raw), &patchRules); err != nil {
+		http.Error(w, fmt.Sprintf("invalid patch rules: %v", err), http.StatusBadRequest)
+		return
+	}
+	if len(patchRules) == 0 {
+		http.Error(w, "patch must include at least one egress rule", http.StatusBadRequest)
+		return
+	}
+
+	base := s.proxy.CurrentPolicy()
+	if base == nil {
+		base = policy.DefaultDenyPolicy()
+	}
+	baseCopy := *base
+	baseCopy.Egress = append([]policy.EgressRule(nil), base.Egress...)
+
+	merged := mergeEgressRules(baseCopy.Egress, patchRules)
+
+	// Reuse parser to normalize targets/actions.
+	rawMerged, _ := json.Marshal(policy.NetworkPolicy{
+		DefaultAction: baseCopy.DefaultAction,
+		Egress:        merged,
+	})
+	newPolicy, err := policy.ParsePolicy(string(rawMerged))
+	if err != nil {
+		http.Error(w, fmt.Sprintf("invalid merged policy: %v", err), http.StatusBadRequest)
+		return
+	}
+
+	mode := modeFromPolicy(newPolicy)
+	log.Infof("policy API: patching policy with %d new rule(s), mode=%s, enforcement=%s", len(patchRules), mode, s.enforcementMode)
+	if s.nft != nil {
+		polWithNS := newPolicy.WithExtraAllowIPs(s.nameserverIPs)
+		if err := s.nft.ApplyStatic(r.Context(), polWithNS); err != nil {
+			log.Errorf("policy API: nftables apply failed on patch: %v", err)
+			http.Error(w, fmt.Sprintf("failed to apply nftables policy: %v", err), http.StatusInternalServerError)
+			return
+		}
+	}
+	s.proxy.UpdatePolicy(newPolicy)
+	log.Infof("policy API: patch applied successfully")
+	writeJSON(w, http.StatusOK, policyStatusResponse{
+		Status:          "ok",
+		Mode:            mode,
+		EnforcementMode: s.enforcementMode,
 	})
 }
 
@@ -218,3 +301,40 @@ func modeFromPolicy(p *policy.NetworkPolicy) string {
 
 	return "enforcing"
 }
+
+// mergeEgressRules joins base rules and additions, deduping by target (last writer wins).
+func mergeEgressRules(base, additions []policy.EgressRule) []policy.EgressRule {
+	if len(additions) == 0 {
+		return base
+	}
+	out := make([]policy.EgressRule, 0, len(base)+len(additions))
+	seen := make(map[string]struct{})
+
+	// Priority: additions first; base rules only if target not overridden.
+	for _, r := range additions {
+		key := mergeKey(r)
+		if _, ok := seen[key]; ok {
+			continue
+		}
+		seen[key] = struct{}{}
+		out = append(out, r)
+	}
+	for _, r := range base {
+		key := mergeKey(r)
+		if _, ok := seen[key]; ok {
+			continue
+		}
+		seen[key] = struct{}{}
+		out = append(out, r)
+	}
+	return out
+}
+
+// mergeKey normalizes domain targets to lowercase for dedupe;
+// IP/CIDR targets are kept as-is.
+func mergeKey(r policy.EgressRule) string {
+	if r.Target == "" {
+		return r.Target
+	}
+	return strings.ToLower(r.Target)
+}

components/egress/policy_server_test.go

@@ -125,3 +125,80 @@ func TestHandleGet_ReturnsEnforcementMode(t *testing.T) {
 		t.Fatalf("expected enforcementMode dns in response, got: %s", string(body))
 	}
 }
+
+func TestHandlePatch_MergesAndApplies(t *testing.T) {
+	initial := &policy.NetworkPolicy{
+		DefaultAction: policy.ActionDeny,
+		Egress: []policy.EgressRule{
+			{Action: policy.ActionAllow, Target: "example.com"},
+			{Action: policy.ActionDeny, Target: "*.example.com"},
+		},
+	}
+	proxy := &stubProxy{updated: initial}
+	nft := &stubNft{}
+	srv := &policyServer{proxy: proxy, nft: nft, enforcementMode: "dns+nft"}
+
+	body := `[{"action":"deny","target":"blocked.com"},{"action":"allow","target":"example.com"}]`
+	req := httptest.NewRequest(http.MethodPatch, "/policy", strings.NewReader(body))
+	w := httptest.NewRecorder()
+
+	srv.handlePolicy(w, req)
+
+	resp := w.Result()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("expected 200, got %d", resp.StatusCode)
+	}
+	if nft.calls != 1 {
+		t.Fatalf("expected nft ApplyStatic called once, got %d", nft.calls)
+	}
+	if proxy.updated == nil {
+		t.Fatalf("expected proxy policy to be updated")
+	}
+	if proxy.updated.DefaultAction != policy.ActionDeny {
+		t.Fatalf("default action should be preserved, got %s", proxy.updated.DefaultAction)
+	}
+	if len(proxy.updated.Egress) != 3 {
+		t.Fatalf("expected 3 egress rules, got %d", len(proxy.updated.Egress))
+	}
+	if proxy.updated.Egress[0].Target != "blocked.com" || proxy.updated.Egress[0].Action != policy.ActionDeny {
+		t.Fatalf("expected first rule to be patched blocked.com deny, got %+v", proxy.updated.Egress[0])
+	}
+	if proxy.updated.Egress[1].Target != "example.com" || proxy.updated.Egress[1].Action != policy.ActionAllow {
+		t.Fatalf("expected second rule to be patched example.com allow, got %+v", proxy.updated.Egress[1])
+	}
+	if proxy.updated.Egress[2].Target != "*.example.com" || proxy.updated.Egress[2].Action != policy.ActionDeny {
+		t.Fatalf("expected base wildcard rule to remain last, got %+v", proxy.updated.Egress[2])
+	}
+}
+
+func TestHandlePatch_DomainCaseOverride(t *testing.T) {
+	initial := &policy.NetworkPolicy{
+		DefaultAction: policy.ActionDeny,
+		Egress: []policy.EgressRule{
+			{Action: policy.ActionDeny, Target: "Example.COM"},
+		},
+	}
+	proxy := &stubProxy{updated: initial}
+	nft := &stubNft{}
+	srv := &policyServer{proxy: proxy, nft: nft, enforcementMode: "dns+nft"}
+
+	body := `[{"action":"allow","target":"example.com"}]`
+	req := httptest.NewRequest(http.MethodPatch, "/policy", strings.NewReader(body))
+	w := httptest.NewRecorder()
+
+	srv.handlePolicy(w, req)
+
+	resp := w.Result()
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("expected 200, got %d", resp.StatusCode)
+	}
+	if proxy.updated == nil {
+		t.Fatalf("expected proxy policy to be updated")
+	}
+	if len(proxy.updated.Egress) != 1 {
+		t.Fatalf("expected deduped rule count 1, got %d", len(proxy.updated.Egress))
+	}
+	if proxy.updated.Egress[0].Action != policy.ActionAllow || proxy.updated.Egress[0].Target != "example.com" {
+		t.Fatalf("expected allow example.com to override, got %+v", proxy.updated.Egress[0])
+	}
+}

components/egress/tests/smoke-nft.sh

@@ -94,4 +94,42 @@ else
   pass "DoT 853 blocked"
 fi
 
+info "Rules update: wildcard deny -> patch allow specific (dns+nft)"
+curl -sSf -XPOST "http://127.0.0.1:${POLICY_PORT}/policy" \
+  -d '{"defaultAction":"allow","egress":[{"action":"deny","target":"*.cloudflare.com"}]}'
+
+info "Test: www.cloudflare.com should be blocked initially (deny via wildcard)"
+if run_in_app -I https://www.cloudflare.com --max-time 8 >/dev/null 2>&1; then
+  fail "www.cloudflare.com should be blocked before patch"
+else
+  pass "www.cloudflare.com blocked before patch"
+fi
+
+info "Patching allow for www.cloudflare.com (specific should override earlier deny)"
+curl -sSf -XPATCH "http://127.0.0.1:${POLICY_PORT}/policy" \
+  -d '[{"action":"allow","target":"www.cloudflare.com"}]'
+
+info "Test: www.cloudflare.com should be allowed after patch"
+run_in_app -I https://www.cloudflare.com --max-time 10 >/dev/null 2>&1 || fail "www.cloudflare.com should succeed after patch"
+pass "www.cloudflare.com allowed after patch"
+
+info "Rules update: wildcard allow -> patch deny specific (dns+nft)"
+curl -sSf -XPOST "http://127.0.0.1:${POLICY_PORT}/policy" \
+  -d '{"defaultAction":"deny","egress":[{"action":"allow","target":"*.mozilla.org"}]}'
+
+info "Test: www.mozilla.org should be allowed initially (allow via wildcard)"
+run_in_app -I https://www.mozilla.org --max-time 10 >/dev/null 2>&1 || fail "www.mozilla.org should succeed before patch"
+pass "www.mozilla.org allowed before patch"
+
+info "Patching deny for www.mozilla.org (specific should override earlier allow)"
+curl -sSf -XPATCH "http://127.0.0.1:${POLICY_PORT}/policy" \
+  -d '[{"action":"deny","target":"www.mozilla.org"}]'
+
+info "Test: www.mozilla.org should be blocked after patch"
+if run_in_app -I https://www.mozilla.org --max-time 8 >/dev/null 2>&1; then
+  fail "www.mozilla.org should be blocked after patch"
+else
+  pass "www.mozilla.org blocked after patch"
+fi
+
 info "All smoke tests passed."