From 5703054b6027241ac3df8f6f18f666e3b74f0088 Mon Sep 17 00:00:00 2001 From: Abhishek Deepak Date: Mon, 5 Oct 2026 20:52:35 +0530 Subject: [PATCH] fix(webhook): refuse NetworkRule backends whose port differs from spec.port The datapath never rewrites ports (DSR), so a backend port other than spec.port was admitted and then ignored. Reject at admission with a message that says DSR cannot remap ports, document the constraint on the gateway architecture page, and cover it with a validator test. Fixes #737 --- docs/agents/ARCHITECTURE-GATEWAY.md | 2 +- internal/webhook/networkrule_webhook.go | 20 ++++++++++ internal/webhook/networkrule_webhook_test.go | 39 ++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/docs/agents/ARCHITECTURE-GATEWAY.md b/docs/agents/ARCHITECTURE-GATEWAY.md index 2625ba44..42fed3d9 100644 --- a/docs/agents/ARCHITECTURE-GATEWAY.md +++ b/docs/agents/ARCHITECTURE-GATEWAY.md @@ -109,7 +109,7 @@ per-VPC gateway design to colocate with the workload's own VRF. | CRD | Scope | Written by | Purpose | | ---------------- | --------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | `NetworkGateway` | Namespaced, one per gateway node (`spec.targetRef.name` = node name) | Operator, once per gateway node (see [worked example](#worked-containerlab-example) below) | Node-scoped root object, mirroring `BGPRouter`'s pattern. Identifies which nodes participate in the anycast mesh and surfaces each node's engine health via `status.conditions`. Carries **no** self-address field — DSR rewrites nothing, so there is no SNAT source to publish. | -| `NetworkRule` | Namespaced, tenant-writable | Tenant, via an admission webhook that verifies VPC/VPCAttachment ownership (**webhook not yet deployed in this repo** — see Known Constraints) | Ingress load-balancing spec: `vpcRef`/`vpcAttachmentRef` (opaque tenant identifiers), `vipAddresses` (1–8, IPv4/IPv6), `protocol` (`tcp`/`udp`), `port`, `backends` (1–64 `address:port` pairs). Served by every `NetworkGateway` in the namespace identically — there is **no** `status.primaryNode` field. | +| `NetworkRule` | Namespaced, tenant-writable | Tenant, via an admission webhook that verifies VPC/VPCAttachment ownership (**webhook not yet deployed in this repo** — see Known Constraints) | Ingress load-balancing spec: `vpcRef`/`vpcAttachmentRef` (opaque tenant identifiers), `vipAddresses` (1–8, IPv4/IPv6), `protocol` (`tcp`/`udp`), `port`, `backends` (1–64 `address:port` pairs; each backend `port` must equal the rule `port` — DSR cannot remap ports). Served by every `NetworkGateway` in the namespace identically — there is **no** `status.primaryNode` field. | Both are defined in `go.datum.net/network`'s `api/v1alpha1` package (`gateway_types.go`, `rule_types.go`) — the same external CRD module the diff --git a/internal/webhook/networkrule_webhook.go b/internal/webhook/networkrule_webhook.go index 8f626d8a..8e53bf86 100644 --- a/internal/webhook/networkrule_webhook.go +++ b/internal/webhook/networkrule_webhook.go @@ -60,6 +60,10 @@ func (v *NetworkRuleValidator) ValidateDelete( // object has passed admission and been persisted: a validating webhook cannot // write to the status of a request it has not yet admitted. func (v *NetworkRuleValidator) authorize(ctx context.Context, rule *networkv1alpha1.NetworkRule) error { + if err := validateBackendPorts(rule); err != nil { + return err + } + req, err := admission.RequestFromContext(ctx) if err != nil { return fmt.Errorf("resolve admission request: %w", err) @@ -93,3 +97,19 @@ func (v *NetworkRuleValidator) SetupWebhookWithManager(mgr ctrl.Manager) error { // Production registration additionally requires a webhook configuration and // service manifest, plus TLS provisioning, neither of which is wired up here. + +// validateBackendPorts refuses a NetworkRule whose backends declare a port other +// than spec.port. The gateway datapath never rewrites ports (Direct Server +// Return); a mismatched backend port is accepted today and then ignored, so the +// backend never sees traffic on the port it was configured with (#737). +func validateBackendPorts(rule *networkv1alpha1.NetworkRule) error { + for i, b := range rule.Spec.Backends { + if b.Port != rule.Spec.Port { + return fmt.Errorf( + "backends[%d].port (%d) must match spec.port (%d): Direct Server Return cannot remap ports", + i, b.Port, rule.Spec.Port, + ) + } + } + return nil +} diff --git a/internal/webhook/networkrule_webhook_test.go b/internal/webhook/networkrule_webhook_test.go index cf5111d8..8a19486c 100644 --- a/internal/webhook/networkrule_webhook_test.go +++ b/internal/webhook/networkrule_webhook_test.go @@ -103,3 +103,42 @@ func TestAllowAllAuthorizer_AlwaysAllows(t *testing.T) { t.Fatal("AllowAllAuthorizer must always allow") } } + +func TestNetworkRuleValidator_BackendPortMustMatchRulePort(t *testing.T) { + v := &NetworkRuleValidator{Authorizer: fakeAuthorizer{allow: true}} + + mismatch := testRule() + mismatch.Spec.Port = 443 + mismatch.Spec.Backends = []networkv1alpha1.NetworkRuleBackend{ + {Address: "10.0.0.1", Port: 8443}, + } + _, err := v.ValidateCreate(contextWithRequester("alice"), mismatch) + if err == nil { + t.Fatal("expected refusal when backend port differs from spec.port") + } + if want := "Direct Server Return cannot remap ports"; !containsStr(err.Error(), want) { + t.Errorf("error %q does not mention %q", err.Error(), want) + } + + match := testRule() + match.Spec.Port = 443 + match.Spec.Backends = []networkv1alpha1.NetworkRuleBackend{ + {Address: "10.0.0.1", Port: 443}, + } + if _, err := v.ValidateCreate(contextWithRequester("alice"), match); err != nil { + t.Fatalf("matching ports should be admitted: %v", err) + } +} + +func containsStr(s, sub string) bool { + return len(sub) == 0 || (len(s) >= len(sub) && (s == sub || len(s) > 0 && containsAt(s, sub))) +} + +func containsAt(s, sub string) bool { + for i := 0; i+len(sub) <= len(s); i++ { + if s[i:i+len(sub)] == sub { + return true + } + } + return false +}