From bf17225718f308d7d414a982e9a78b2994341d15 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Fri, 25 Sep 2026 16:53:35 -0500 Subject: [PATCH 1/3] fix: Serve the offline page when a backend has no endpoints A user reaching an HTTPProxy whose NetworkService has no ready endpoints gets the generic "this service is temporarily unavailable" page, which reads as a platform fault. The offline page that says nothing is running behind the address exists and is configured, but cannot be reached. Envoy Gateway collapses such a route to a bodiless 503 direct_response. That short-circuits before the router filter, so the response carries no UH flag, and UH is what the offline mapper selects on. Neither response side offers a way out: local_reply_config overrides a direct_response body, and route-level request_headers_to_add never runs. Both were measured against the edge's Envoy build rather than assumed. Pointing those routes at an endpoint-less cluster restores the UH flag and the existing mapper matches unchanged. One shared cluster is used rather than one per backend, because an endpoint-less cluster carries ~108 resident data-plane stats and a per-backend cluster would scale that by the number of idle services. Key changes: - Rewrite EG's bodiless 503 direct_response routes to forward to a single shared endpoint-less cluster, preserving match, metadata and per-filter config so a governed route keeps its WAF settings - Discriminate on status 503 plus an absent body, which EG uses only for no-ready-endpoints and which leaves connector-offline routes untouched - Gate on the branded page being configured, so behaviour is unchanged when branding is off - Count rewritten routes in nso_extension_empty_backend_routes_total Co-Authored-By: Claude Opus 5 (1M context) --- internal/extensionserver/metrics/metrics.go | 12 ++ .../extensionserver/mutate/emptybackend.go | 110 +++++++++++ .../mutate/emptybackend_test.go | 172 ++++++++++++++++++ internal/extensionserver/server/server.go | 38 ++++ 4 files changed, 332 insertions(+) create mode 100644 internal/extensionserver/mutate/emptybackend.go create mode 100644 internal/extensionserver/mutate/emptybackend_test.go diff --git a/internal/extensionserver/metrics/metrics.go b/internal/extensionserver/metrics/metrics.go index 47033245..2b5c9eff 100644 --- a/internal/extensionserver/metrics/metrics.go +++ b/internal/extensionserver/metrics/metrics.go @@ -175,6 +175,18 @@ var ( }, ) + // EmptyBackendRoutesTotal counts total routes Envoy Gateway collapsed to a + // bodiless 503 direct_response (backend Service present, no ready + // endpoints) that were rewritten to forward to the shared endpoint-less + // cluster, across all hook invocations. Each increment = one route that now + // answers with the branded offline page instead of the generic error page. + EmptyBackendRoutesTotal = promauto.NewCounter( + prometheus.CounterOpts{ + Name: "nso_extension_empty_backend_routes_total", + Help: "Total routes with no ready endpoints rewritten to the shared offline backend cluster across all hook invocations.", + }, + ) + // VPCPodSocketBindTotal counts total clusters patched with a VRF // SO_BINDTODEVICE socket option for a vpcPod HTTPProxy backend (#856), // across all hook invocations. diff --git a/internal/extensionserver/mutate/emptybackend.go b/internal/extensionserver/mutate/emptybackend.go new file mode 100644 index 00000000..086a0360 --- /dev/null +++ b/internal/extensionserver/mutate/emptybackend.go @@ -0,0 +1,110 @@ +package mutate + +import ( + "fmt" + + clusterv3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3" + routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" + "google.golang.org/protobuf/encoding/protojson" +) + +// OfflineBackendClusterName is the single endpoint-less cluster every route +// whose backend has no ready endpoints is pointed at. One shared cluster rather +// than one per backend: an endpoint-less cluster carries ~108 resident stats in +// the data plane, so a per-backend cluster would scale that by the number of +// idle services, while a shared one keeps it constant. +const OfflineBackendClusterName = "datum-offline-backend" + +// emptyBackendStatus is the status Envoy Gateway collapses a route to when the +// backend Service exists but has no ready endpoints. It is the only collapse +// case EG answers with 503 — every other one (filter error, invalid +// destination, all-zero weights) uses 500 — which is what makes the status a +// safe discriminator. See EG internal/gatewayapi/route.go, "return 503 if no +// ready endpoints exist". +const emptyBackendStatus = 503 + +// EnsureOfflineBackendCluster appends the shared endpoint-less cluster to the +// xDS cluster set if it is not already present, returning the (possibly +// extended) set and whether it added one. +// +// The cluster is STATIC with an empty endpoint list. A request routed to it +// fails at host selection, so Envoy answers 503 with the UH (no healthy +// upstream) response flag and never attempts a connection. UH is what the +// branded offline page keys on; see buildLocalReplyConfig in localreply.go. +func EnsureOfflineBackendCluster(clusters []*clusterv3.Cluster) ([]*clusterv3.Cluster, bool, error) { + for _, c := range clusters { + if c.GetName() == OfflineBackendClusterName { + return clusters, false, nil + } + } + + j := fmt.Sprintf(`{ + "name": %q, + "type": "STATIC", + "connect_timeout": "1s", + "load_assignment": { "cluster_name": %q, "endpoints": [] } +}`, OfflineBackendClusterName, OfflineBackendClusterName) + + c := &clusterv3.Cluster{} + if err := protojson.Unmarshal([]byte(j), c); err != nil { + return clusters, false, fmt.Errorf("unmarshal offline backend cluster JSON: %w", err) + } + return append(clusters, c), true, nil +} + +// RouteEmptyBackendsToOfflineCluster rewrites every route Envoy Gateway +// collapsed to a bodiless 503 direct_response so it forwards to the shared +// endpoint-less cluster instead. +// +// A direct_response short-circuits before the router filter runs, so such a +// route produces no response flag and no upstream attempt. That makes the +// offline page unreachable: local_reply_config mappers can only select on the +// request and on Envoy's own response metadata, and a direct_response supplies +// neither a UH flag nor any route-supplied header (route-level +// request_headers_to_add is applied by the router filter, which never runs). +// A direct_response body is no use either — local_reply_config overrides it. +// Forwarding to an endpoint-less cluster restores the UH flag, which is the +// only signal the mapper can actually match. +// +// Only the Action oneof is replaced, so each route's match, metadata and +// typed_per_filter_config survive — a route that already carries Coraza +// per-route config keeps it. No retry policy is attached: with no hosts there +// is nothing to retry onto, and Envoy fails fast at host selection. +// +// Idempotent: a rewritten route is no longer a direct_response, so a second +// pass does not match it. +// +// Returns the number of routes rewritten. +func RouteEmptyBackendsToOfflineCluster(rc *routev3.RouteConfiguration) int { + rewritten := 0 + for _, vh := range rc.GetVirtualHosts() { + for _, rt := range vh.GetRoutes() { + if !isEmptyBackendDirectResponse(rt) { + continue + } + rt.Action = &routev3.Route_Route{ + Route: &routev3.RouteAction{ + ClusterSpecifier: &routev3.RouteAction_Cluster{ + Cluster: OfflineBackendClusterName, + }, + }, + } + rewritten++ + } + } + return rewritten +} + +// isEmptyBackendDirectResponse reports whether a route is EG's "backend has no +// ready endpoints" collapse, as opposed to any other direct_response. +// +// The body check is what separates it from NSO's own connector-offline routes, +// which carry an explicit body, and from a Gateway API filter's direct +// response. EG sets no body on the collapse. +func isEmptyBackendDirectResponse(rt *routev3.Route) bool { + dr := rt.GetDirectResponse() + if dr == nil || dr.GetStatus() != emptyBackendStatus { + return false + } + return dr.GetBody() == nil +} diff --git a/internal/extensionserver/mutate/emptybackend_test.go b/internal/extensionserver/mutate/emptybackend_test.go new file mode 100644 index 00000000..f7cdccf8 --- /dev/null +++ b/internal/extensionserver/mutate/emptybackend_test.go @@ -0,0 +1,172 @@ +package mutate + +import ( + "testing" + + clusterv3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3" + corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" + routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" + "google.golang.org/protobuf/types/known/anypb" +) + +func directResponseRoute(name string, status uint32, body string) *routev3.Route { + dr := &routev3.DirectResponseAction{Status: status} + if body != "" { + dr.Body = &corev3.DataSource{ + Specifier: &corev3.DataSource_InlineString{InlineString: body}, + } + } + return &routev3.Route{ + Name: name, + Match: &routev3.RouteMatch{PathSpecifier: &routev3.RouteMatch_Prefix{Prefix: "/"}}, + Action: &routev3.Route_DirectResponse{DirectResponse: dr}, + } +} + +func routeConfigWith(routes ...*routev3.Route) *routev3.RouteConfiguration { + return &routev3.RouteConfiguration{ + Name: "rc", + VirtualHosts: []*routev3.VirtualHost{{Name: "vh", Routes: routes}}, + } +} + +func firstRoute(rc *routev3.RouteConfiguration) *routev3.Route { + return rc.GetVirtualHosts()[0].GetRoutes()[0] +} + +func TestRouteEmptyBackendsToOfflineCluster(t *testing.T) { + tests := []struct { + name string + route *routev3.Route + wantRewritten int + }{ + { + name: "EG collapse for no ready endpoints is rewritten", + route: directResponseRoute("empty-backend", 503, ""), + wantRewritten: 1, + }, + { + name: "connector offline route carries a body and is left alone", + route: directResponseRoute("connector-offline", 503, offlineResponseBody), + wantRewritten: 0, + }, + { + name: "EG collapse for a filter error uses 500 and is left alone", + route: directResponseRoute("filter-error", 500, ""), + wantRewritten: 0, + }, + { + name: "EG collapse for an invalid destination uses 500 and is left alone", + route: directResponseRoute("invalid-destination", 500, ""), + wantRewritten: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rc := routeConfigWith(tt.route) + + got := RouteEmptyBackendsToOfflineCluster(rc) + if got != tt.wantRewritten { + t.Fatalf("rewritten = %d, want %d", got, tt.wantRewritten) + } + + action := firstRoute(rc).GetRoute() + if tt.wantRewritten == 0 { + if action != nil { + t.Fatalf("route was rewritten to cluster %q, want direct_response preserved", action.GetCluster()) + } + return + } + if action == nil { + t.Fatal("route still has a direct_response, want a forwarding route") + } + if action.GetCluster() != OfflineBackendClusterName { + t.Fatalf("cluster = %q, want %q", action.GetCluster(), OfflineBackendClusterName) + } + if action.GetRetryPolicy() != nil { + t.Error("rewritten route carries a retry policy; there is nothing to retry onto") + } + }) + } +} + +// A rewritten route must keep everything the route carried besides its action — +// most importantly the Coraza per-route config a governed route holds, which +// lives in typed_per_filter_config. +func TestRouteEmptyBackendsPreservesRouteState(t *testing.T) { + perFilter, err := anypb.New(&routev3.FilterConfig{}) + if err != nil { + t.Fatalf("build per-filter config: %v", err) + } + + rt := directResponseRoute("empty-backend", 503, "") + rt.TypedPerFilterConfig = map[string]*anypb.Any{"coraza": perFilter} + + rc := routeConfigWith(rt) + if got := RouteEmptyBackendsToOfflineCluster(rc); got != 1 { + t.Fatalf("rewritten = %d, want 1", got) + } + + out := firstRoute(rc) + if out.GetName() != "empty-backend" { + t.Errorf("name = %q, want %q", out.GetName(), "empty-backend") + } + if out.GetMatch().GetPrefix() != "/" { + t.Errorf("match prefix = %q, want %q", out.GetMatch().GetPrefix(), "/") + } + if _, ok := out.GetTypedPerFilterConfig()["coraza"]; !ok { + t.Error("typed_per_filter_config lost; a governed route would lose its WAF config") + } +} + +func TestRouteEmptyBackendsIsIdempotent(t *testing.T) { + rc := routeConfigWith(directResponseRoute("empty-backend", 503, "")) + + if got := RouteEmptyBackendsToOfflineCluster(rc); got != 1 { + t.Fatalf("first pass rewritten = %d, want 1", got) + } + if got := RouteEmptyBackendsToOfflineCluster(rc); got != 0 { + t.Fatalf("second pass rewritten = %d, want 0", got) + } + if cl := firstRoute(rc).GetRoute().GetCluster(); cl != OfflineBackendClusterName { + t.Fatalf("cluster = %q, want %q", cl, OfflineBackendClusterName) + } +} + +func TestEnsureOfflineBackendCluster(t *testing.T) { + clusters, added, err := EnsureOfflineBackendCluster(nil) + if err != nil { + t.Fatalf("EnsureOfflineBackendCluster: %v", err) + } + if !added { + t.Fatal("added = false on an empty cluster set, want true") + } + if len(clusters) != 1 { + t.Fatalf("len(clusters) = %d, want 1", len(clusters)) + } + + c := clusters[0] + if c.GetName() != OfflineBackendClusterName { + t.Errorf("name = %q, want %q", c.GetName(), OfflineBackendClusterName) + } + if c.GetType() != clusterv3.Cluster_STATIC { + t.Errorf("type = %v, want STATIC", c.GetType()) + } + // The empty endpoint list is the whole point: it is what makes Envoy fail + // host selection and set the UH flag the offline page matches on. + if eps := c.GetLoadAssignment().GetEndpoints(); len(eps) != 0 { + t.Errorf("len(endpoints) = %d, want 0", len(eps)) + } + + again, added, err := EnsureOfflineBackendCluster(clusters) + if err != nil { + t.Fatalf("EnsureOfflineBackendCluster (second call): %v", err) + } + if added { + t.Error("added = true on a set that already has the cluster, want false") + } + if len(again) != 1 { + t.Errorf("len(clusters) = %d after second call, want 1", len(again)) + } +} diff --git a/internal/extensionserver/server/server.go b/internal/extensionserver/server/server.go index a3268d6c..156f2836 100644 --- a/internal/extensionserver/server/server.go +++ b/internal/extensionserver/server/server.go @@ -288,6 +288,42 @@ func (s *Server) PostTranslateModify( ) connRoutesSpan.End() + // --- Empty backend family (#502) --- + // EG collapses a route whose backend has no ready endpoints to a bodiless + // 503 direct_response, which short-circuits before the router filter and so + // carries no UH flag for the branded offline page to match. Point those + // routes at a shared endpoint-less cluster instead, which restores the flag. + // Runs after the connector family so connector-offline routes already carry + // their body and are therefore not mistaken for EG's collapse. + // + // Gated on the branded page being configured: with no offline page to + // serve, rewriting would only trade EG's deterministic 503 for Envoy's + // generic no_healthy_upstream and buy nothing. + var emptyBackendCount int + if !s.cfg.LocalReply.Disabled && s.cfg.LocalReply.OfflineBodyHTML != "" { + _, emptyBackendSpan := tr.Start(mctx, "emptybackend.routes") + for _, rc := range routes { + emptyBackendCount += mutate.RouteEmptyBackendsToOfflineCluster(rc) + } + if emptyBackendCount > 0 { + var ebErr error + clusters, _, ebErr = mutate.EnsureOfflineBackendCluster(clusters) + if ebErr != nil { + s.log.Error("ensure offline backend cluster", "err", ebErr) + emptyBackendSpan.RecordError(ebErr) + emptyBackendSpan.End() + mspan.RecordError(ebErr) + mspan.End() + extmetrics.PhaseDuration.WithLabelValues("mutate").Observe(time.Since(mutStart).Seconds()) + hspan.RecordError(ebErr) + outcome = outcomeError + return nil, ebErr + } + } + emptyBackendSpan.SetAttributes(attribute.Int("routes.empty_backend", emptyBackendCount)) + emptyBackendSpan.End() + } + // --- VPC pod family (#856) --- // Binds a vpcPod backend's cluster to its tenant's VRF device // (SO_BINDTODEVICE) so the shared multi-tenant Envoy fleet resolves the @@ -367,6 +403,7 @@ func (s *Server) PostTranslateModify( extmetrics.ConnectorClustersTotal.Add(float64(len(replaced))) extmetrics.ConnectorRoutesTotal.Add(float64(vhCount)) extmetrics.ConnectorOfflineRoutesTotal.Add(float64(offlineRtCount)) + extmetrics.EmptyBackendRoutesTotal.Add(float64(emptyBackendCount)) // In the test environment, record what this build changed so a test can later // confirm the proxy is running exactly that. This only reads the configuration @@ -390,6 +427,7 @@ func (s *Server) PostTranslateModify( "vhosts_connector_applied", vhCount, "connector_offline_routes", offlineRtCount, "clusters_vpcpod_bound", vpcPodCount, + "routes_empty_backend", emptyBackendCount, ) return &pb.PostTranslateModifyResponse{ From 0b9f8d404cd25505c036fbde6e05cf1274b5e729 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Fri, 25 Sep 2026 18:02:34 -0500 Subject: [PATCH 2/3] fix: Serve the offline page when a tunnel is offline A user reaching a proxy whose connector tunnel is down gets the generic "temporarily unavailable" page. The terse body the offline path writes has never reached a user, because the branded page overrides a direct_response body, so the string only ever appeared in a config dump. The cause is the same one behind the empty-backend case: a direct_response short-circuits before the router filter, so the response carries no UH flag for the offline page to select on. Sending user traffic to an endpoint-less cluster restores it. The comment this reverses argued an endpoint-less cluster would bring retry and connect noise. Measurement does not bear that out. With no hosts Envoy fails at host selection, so every retry counter stays at zero and no connection is attempted. The cost is resident stats, which one shared cluster holds constant. Key changes: - Route an offline tunnel's user traffic to a shared endpoint-less cluster when the branded page is configured, keeping the deterministic 503 when it is not - Leave the CONNECT route alone, since it answers the connector agent rather than a browser - Give the tunnel and empty-backend cases separate sinks so data-plane stats and the parity scanner can tell them apart - Teach both offline-route scanners the forwarding shape, so the parity gate keeps counting these routes Co-Authored-By: Claude Opus 5 (1M context) --- .../cell-controllers/kustomization.yaml | 20 +- config/e2e-downstream/kustomization.yaml | 2 +- config/manager/kustomization.yaml | 2 +- internal/extensionserver/metrics/metrics.go | 11 +- internal/extensionserver/mutate/connector.go | 45 ++- .../extensionserver/mutate/connector_test.go | 67 +++- .../extensionserver/mutate/emptybackend.go | 30 +- .../mutate/emptybackend_test.go | 33 +- .../extensionserver/server/programmedset.go | 2 +- .../server/programmedset_scan.go | 29 +- internal/extensionserver/server/server.go | 23 +- .../chainsaw-test.yaml | 330 ++++++++++++++++++ .../chainsaw-test.yaml | 298 ++++++++++++++++ test/parity/scan.go | 18 +- 14 files changed, 835 insertions(+), 75 deletions(-) create mode 100644 test/e2e-edge/networkservice-drained-to-zero/chainsaw-test.yaml create mode 100644 test/e2e-edge/networkservice-no-endpoints/chainsaw-test.yaml diff --git a/config/components/cell-controllers/kustomization.yaml b/config/components/cell-controllers/kustomization.yaml index 503ed273..47cebcfe 100644 --- a/config/components/cell-controllers/kustomization.yaml +++ b/config/components/cell-controllers/kustomization.yaml @@ -2,21 +2,21 @@ apiVersion: kustomize.config.k8s.io/v1alpha1 kind: Component resources: - - deployment.yaml - - metrics_service.yaml +- deployment.yaml +- metrics_service.yaml # A component's transformers apply to the parent's whole accumulation, so this # matches a name only the cell Deployment carries. Overlays pinning # ghcr.io/datum-cloud/network-services-operator still reach both Deployments: # their transformer runs after this one has resolved the name. images: - - name: network-services-operator-cell - newName: ghcr.io/datum-cloud/network-services-operator - newTag: latest +- name: network-services-operator-cell + newName: ghcr.io/datum-cloud/network-services-operator + newTag: bf172257 configMapGenerator: - - name: cell-config - files: - - config.yaml - options: - disableNameSuffixHash: true +- files: + - config.yaml + name: cell-config + options: + disableNameSuffixHash: true diff --git a/config/e2e-downstream/kustomization.yaml b/config/e2e-downstream/kustomization.yaml index 2f7f5f4f..c1c06250 100644 --- a/config/e2e-downstream/kustomization.yaml +++ b/config/e2e-downstream/kustomization.yaml @@ -22,7 +22,7 @@ resources: images: - name: ghcr.io/datum-cloud/network-services-operator newName: ghcr.io/datum-cloud/network-services-operator - newTag: 342d69f + newTag: bf172257 # The default e2e path has no Prometheus operator; drop the ServiceMonitor so # the apply doesn't fail on the missing monitoring.coreos.com CRD. (Re-add via diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 657a599a..d3213b8f 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -6,7 +6,7 @@ resources: images: - name: ghcr.io/datum-cloud/network-services-operator newName: ghcr.io/datum-cloud/network-services-operator - newTag: 342d69f + newTag: bf172257 configMapGenerator: - files: - config.yaml diff --git a/internal/extensionserver/metrics/metrics.go b/internal/extensionserver/metrics/metrics.go index 2b5c9eff..affafcaa 100644 --- a/internal/extensionserver/metrics/metrics.go +++ b/internal/extensionserver/metrics/metrics.go @@ -163,15 +163,14 @@ var ( ) // ConnectorOfflineRoutesTotal counts total user-facing forwarding routes - // rewritten to a tunnel-offline 503 direct_response across all hook - // invocations. Each increment = one route that previously targeted an - // endpoint-less offline-connector cluster and now returns a deterministic - // 503 (instead of Envoy's generic 503 no_healthy_upstream). Use rate()/sum() - // to derive per-build averages. + // moved off an offline connector's own cluster across all hook invocations. + // With the branded error page configured they are pointed at the shared + // endpoint-less cluster, so the user gets the offline page; without it they + // return a deterministic 503. Use rate()/sum() to derive per-build averages. ConnectorOfflineRoutesTotal = promauto.NewCounter( prometheus.CounterOpts{ Name: "nso_extension_connector_offline_routes_total", - Help: "Total user-facing forwarding routes rewritten to a tunnel-offline 503 direct_response across all hook invocations.", + Help: "Total user-facing forwarding routes moved off an offline connector cluster across all hook invocations.", }, ) diff --git a/internal/extensionserver/mutate/connector.go b/internal/extensionserver/mutate/connector.go index 40cf5f28..2e34d57b 100644 --- a/internal/extensionserver/mutate/connector.go +++ b/internal/extensionserver/mutate/connector.go @@ -75,15 +75,25 @@ func ReplaceConnectorClusters( // - Online (replaced) connector: prepend a CONNECT upgrade route targeting the // replaced cluster and append a unique per-connector domain to VH domains. // - Offline connector: prepend a CONNECT direct_response 503 (tunnel-control -// clients) and rewrite the user-facing forwarding routes to a 503 -// direct_response (see the offline branch for why). +// clients) and rewrite the user-facing forwarding routes (see the offline +// branch for why). // -// Returns the number of VirtualHosts mutated and the number of forwarding -// routes converted to a tunnel-offline direct_response. +// brandedOffline selects what a user reaching an offline tunnel gets. When the +// branded error page is configured, the user-facing routes forward to the +// shared endpoint-less cluster, which makes Envoy set the UH response flag the +// offline page selects on. When it is not, they keep the deterministic 503 +// direct_response, which is what they have always returned. +// +// The CONNECT route is unaffected either way: it answers the connector agent, +// not a browser, and a terse body is the right answer there. +// +// Returns the number of VirtualHosts mutated and the number of user-facing +// forwarding routes rewritten. func ApplyConnectorRoutes( rc *routev3.RouteConfiguration, idx *extcache.PolicyIndex, replaced, offline map[string]*extcache.ConnectorInfo, + brandedOffline bool, ) (mutated, converted int, err error) { for _, vh := range rc.GetVirtualHosts() { // Find any connector cluster referenced by routes in this VH. @@ -129,16 +139,31 @@ func ApplyConnectorRoutes( } vh.Routes = append([]*routev3.Route{newRoute}, vh.Routes...) - // Route user traffic to a deterministic 503 instead of the - // endpoint-less offline cluster, which would yield a generic - // no_healthy_upstream plus retry/cluster-stat noise. Replacing only - // the Action oneof preserves each route's match/metadata; idempotent - // because direct_responses carry no cluster to re-match. + // Move user traffic off the connector's own endpoint-less cluster, + // which would otherwise report retry and connect failures against a + // per-connector cluster. Replacing only the Action oneof preserves + // each route's match/metadata; idempotent because neither + // replacement carries the connector cluster to re-match. + // + // With branding on, the shared endpoint-less cluster is the target, + // because a direct_response short-circuits before the router filter + // and so never carries the UH flag the offline page selects on. The + // cluster is shared, so this adds no per-connector data-plane stats. + // With branding off, a deterministic 503 remains the better answer + // than Envoy's generic no_healthy_upstream text. for _, rt := range vh.GetRoutes() { if routeCluster(rt) != connectorCluster { continue } - if derr := setRouteDirectResponse(rt, 503, offlineResponseBody); derr != nil { + if brandedOffline { + rt.Action = &routev3.Route_Route{ + Route: &routev3.RouteAction{ + ClusterSpecifier: &routev3.RouteAction_Cluster{ + Cluster: OfflineTunnelClusterName, + }, + }, + } + } else if derr := setRouteDirectResponse(rt, 503, offlineResponseBody); derr != nil { return mutated, converted, fmt.Errorf("convert offline forward route for %q: %w", vh.GetName(), derr) } converted++ diff --git a/internal/extensionserver/mutate/connector_test.go b/internal/extensionserver/mutate/connector_test.go index b67d8dc5..0ae007a4 100644 --- a/internal/extensionserver/mutate/connector_test.go +++ b/internal/extensionserver/mutate/connector_test.go @@ -207,7 +207,7 @@ func TestApplyConnectorRoutes_Online_PrependsCONNECTRouteAndAppendsUniqueDomain( }, } - n, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline) + n, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline, false) require.NoError(t, err) assert.Equal(t, 1, n, "one VH should be mutated") assert.Equal(t, 0, converted, "online connector must not convert any forwarding routes") @@ -257,7 +257,7 @@ func TestApplyConnectorRoutes_Online_DomainAppendIsIdempotent(t *testing.T) { }, } - _, _, err := ApplyConnectorRoutes(rc, idx, replaced, offline) + _, _, err := ApplyConnectorRoutes(rc, idx, replaced, offline, false) require.NoError(t, err) // Synthetic domain must not be duplicated. @@ -295,7 +295,7 @@ func TestApplyConnectorRoutes_Online_NoDomainCollisionAcrossConnectors(t *testin }, } - _, _, err := ApplyConnectorRoutes(rc, &extcache.PolicyIndex{}, replaced, offline) + _, _, err := ApplyConnectorRoutes(rc, &extcache.PolicyIndex{}, replaced, offline, false) require.NoError(t, err) // No domain may appear more than once across the whole route config. @@ -333,7 +333,7 @@ func TestApplyConnectorRoutes_Offline_Prepends503Route_NoDomain(t *testing.T) { }, } - n, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline) + n, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline, false) require.NoError(t, err) assert.Equal(t, 1, n, "offline VH must be mutated (503 route prepended)") assert.Equal(t, 1, converted, "the user-facing forwarding route must be converted to a direct_response") @@ -404,7 +404,7 @@ func TestApplyConnectorRoutes_Offline_PreservesMatchAndUntouchedRoute(t *testing }, } - _, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline) + _, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline, false) require.NoError(t, err) assert.Equal(t, 1, converted, "only the connector forwarding route must be converted") @@ -447,12 +447,12 @@ func TestApplyConnectorRoutes_Offline_Idempotent(t *testing.T) { }, } - _, converted1, err := ApplyConnectorRoutes(rc, idx, replaced, offline) + _, converted1, err := ApplyConnectorRoutes(rc, idx, replaced, offline, false) require.NoError(t, err) assert.Equal(t, 1, converted1, "first pass converts the forwarding route") // Second pass: the cluster is gone from all routes, so nothing converts. - _, converted2, err := ApplyConnectorRoutes(rc, idx, replaced, offline) + _, converted2, err := ApplyConnectorRoutes(rc, idx, replaced, offline, false) require.NoError(t, err) assert.Equal(t, 0, converted2, "second pass must not re-convert any route") @@ -483,7 +483,7 @@ func TestApplyConnectorRoutes_NoConnector_VHUntouched(t *testing.T) { }, } - n, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline) + n, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline, false) require.NoError(t, err) assert.Equal(t, 0, n, "VH with non-connector cluster must not be mutated") assert.Equal(t, 0, converted, "no forwarding routes converted when no connector present") @@ -498,8 +498,57 @@ func TestApplyConnectorRoutes_EmptyRouteConfiguration_NoOp(t *testing.T) { rc := &routev3.RouteConfiguration{Name: "empty"} - n, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline) + n, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline, false) require.NoError(t, err) assert.Equal(t, 0, n) assert.Equal(t, 0, converted) } + +// With the branded page configured, an offline connector's user-facing routes +// must forward to the shared endpoint-less cluster rather than answer with a +// direct_response, because only the forwarded request carries the UH flag the +// offline page selects on. +func TestApplyConnectorRoutes_Offline_BrandedRoutesToTunnelCluster(t *testing.T) { + idx := connectorPolicyIndex(false) + clusterName := testClusterName() + + offlineInfo := &extcache.ConnectorInfo{Online: false, TargetHost: testTargetHost, TargetPort: testTargetPort} + replaced := map[string]*extcache.ConnectorInfo{} + offline := map[string]*extcache.ConnectorInfo{clusterName: offlineInfo} + + connRoute := routeTargeting(clusterName) + connRoute.Match = &routev3.RouteMatch{PathSpecifier: &routev3.RouteMatch_Prefix{Prefix: "/"}} + connRoute.TypedPerFilterConfig = map[string]*anypb.Any{ + "envoy.filters.http.cors": {TypeUrl: "type.googleapis.com/example.Cfg"}, + } + + rc := &routev3.RouteConfiguration{ + VirtualHosts: []*routev3.VirtualHost{ + {Name: "vh", Domains: []string{"app.local.test"}, Routes: []*routev3.Route{connRoute}}, + }, + } + + _, converted, err := ApplyConnectorRoutes(rc, idx, replaced, offline, true) + require.NoError(t, err) + assert.Equal(t, 1, converted) + + vh := rc.VirtualHosts[0] + require.Len(t, vh.Routes, 2, "connect_matcher route prepended to the original") + + // The CONNECT route answers the connector agent, not a browser, so it keeps + // its terse direct_response either way. + gotConnect := vh.Routes[0] + require.NotNil(t, gotConnect.GetDirectResponse(), "CONNECT route must stay a direct_response") + assert.Equal(t, offlineResponseBody, + gotConnect.GetDirectResponse().GetBody().GetInlineString()) + + gotUser := vh.Routes[1] + assert.Nil(t, gotUser.GetDirectResponse(), "user route must not be a direct_response") + assert.Equal(t, OfflineTunnelClusterName, routeCluster(gotUser), + "user route must forward to the shared offline-tunnel cluster") + assert.NotEqual(t, OfflineBackendClusterName, routeCluster(gotUser), + "an offline tunnel must not share the empty-backend sink") + assert.Equal(t, "/", gotUser.GetMatch().GetPrefix(), "prefix match must be preserved") + assert.Contains(t, gotUser.GetTypedPerFilterConfig(), "envoy.filters.http.cors", + "typed_per_filter_config must be preserved on the rewritten route") +} diff --git a/internal/extensionserver/mutate/emptybackend.go b/internal/extensionserver/mutate/emptybackend.go index 086a0360..fa5851a2 100644 --- a/internal/extensionserver/mutate/emptybackend.go +++ b/internal/extensionserver/mutate/emptybackend.go @@ -8,12 +8,20 @@ import ( "google.golang.org/protobuf/encoding/protojson" ) -// OfflineBackendClusterName is the single endpoint-less cluster every route -// whose backend has no ready endpoints is pointed at. One shared cluster rather -// than one per backend: an endpoint-less cluster carries ~108 resident stats in -// the data plane, so a per-backend cluster would scale that by the number of -// idle services, while a shared one keeps it constant. -const OfflineBackendClusterName = "datum-offline-backend" +// The endpoint-less clusters user traffic is pointed at when its backend cannot +// serve. One shared cluster per reason rather than one per backend or per +// connector: an endpoint-less cluster carries ~108 resident stats in the data +// plane, so per-backend clusters would scale that by the number of idle +// services, while these two keep it constant. +// +// The two reasons stay apart so the data-plane stats say which one a request +// hit, and so the parity scanner can tell the families apart. +const ( + // OfflineBackendClusterName serves a backend with no ready endpoints. + OfflineBackendClusterName = "datum-offline-backend" + // OfflineTunnelClusterName serves an offline connector tunnel. + OfflineTunnelClusterName = "datum-offline-tunnel" +) // emptyBackendStatus is the status Envoy Gateway collapses a route to when the // backend Service exists but has no ready endpoints. It is the only collapse @@ -23,7 +31,7 @@ const OfflineBackendClusterName = "datum-offline-backend" // ready endpoints exist". const emptyBackendStatus = 503 -// EnsureOfflineBackendCluster appends the shared endpoint-less cluster to the +// EnsureOfflineCluster appends the named shared endpoint-less cluster to the // xDS cluster set if it is not already present, returning the (possibly // extended) set and whether it added one. // @@ -31,9 +39,9 @@ const emptyBackendStatus = 503 // fails at host selection, so Envoy answers 503 with the UH (no healthy // upstream) response flag and never attempts a connection. UH is what the // branded offline page keys on; see buildLocalReplyConfig in localreply.go. -func EnsureOfflineBackendCluster(clusters []*clusterv3.Cluster) ([]*clusterv3.Cluster, bool, error) { +func EnsureOfflineCluster(clusters []*clusterv3.Cluster, name string) ([]*clusterv3.Cluster, bool, error) { for _, c := range clusters { - if c.GetName() == OfflineBackendClusterName { + if c.GetName() == name { return clusters, false, nil } } @@ -43,11 +51,11 @@ func EnsureOfflineBackendCluster(clusters []*clusterv3.Cluster) ([]*clusterv3.Cl "type": "STATIC", "connect_timeout": "1s", "load_assignment": { "cluster_name": %q, "endpoints": [] } -}`, OfflineBackendClusterName, OfflineBackendClusterName) +}`, name, name) c := &clusterv3.Cluster{} if err := protojson.Unmarshal([]byte(j), c); err != nil { - return clusters, false, fmt.Errorf("unmarshal offline backend cluster JSON: %w", err) + return clusters, false, fmt.Errorf("unmarshal offline cluster %q JSON: %w", name, err) } return append(clusters, c), true, nil } diff --git a/internal/extensionserver/mutate/emptybackend_test.go b/internal/extensionserver/mutate/emptybackend_test.go index f7cdccf8..f0094e1c 100644 --- a/internal/extensionserver/mutate/emptybackend_test.go +++ b/internal/extensionserver/mutate/emptybackend_test.go @@ -134,10 +134,10 @@ func TestRouteEmptyBackendsIsIdempotent(t *testing.T) { } } -func TestEnsureOfflineBackendCluster(t *testing.T) { - clusters, added, err := EnsureOfflineBackendCluster(nil) +func TestEnsureOfflineCluster(t *testing.T) { + clusters, added, err := EnsureOfflineCluster(nil, OfflineBackendClusterName) if err != nil { - t.Fatalf("EnsureOfflineBackendCluster: %v", err) + t.Fatalf("EnsureOfflineCluster: %v", err) } if !added { t.Fatal("added = false on an empty cluster set, want true") @@ -159,9 +159,9 @@ func TestEnsureOfflineBackendCluster(t *testing.T) { t.Errorf("len(endpoints) = %d, want 0", len(eps)) } - again, added, err := EnsureOfflineBackendCluster(clusters) + again, added, err := EnsureOfflineCluster(clusters, OfflineBackendClusterName) if err != nil { - t.Fatalf("EnsureOfflineBackendCluster (second call): %v", err) + t.Fatalf("EnsureOfflineCluster (second call): %v", err) } if added { t.Error("added = true on a set that already has the cluster, want false") @@ -170,3 +170,26 @@ func TestEnsureOfflineBackendCluster(t *testing.T) { t.Errorf("len(clusters) = %d after second call, want 1", len(again)) } } + +// The two sinks must stay distinct. A shared name would make the parity scanner +// count an idle backend as an offline tunnel. +func TestEnsureOfflineClusterKeepsTheTwoSinksApart(t *testing.T) { + if OfflineBackendClusterName == OfflineTunnelClusterName { + t.Fatal("the empty-backend and offline-tunnel sinks share a name") + } + + clusters, _, err := EnsureOfflineCluster(nil, OfflineBackendClusterName) + if err != nil { + t.Fatalf("EnsureOfflineCluster: %v", err) + } + clusters, added, err := EnsureOfflineCluster(clusters, OfflineTunnelClusterName) + if err != nil { + t.Fatalf("EnsureOfflineCluster: %v", err) + } + if !added { + t.Fatal("added = false for the second sink, want true") + } + if len(clusters) != 2 { + t.Fatalf("len(clusters) = %d, want 2", len(clusters)) + } +} diff --git a/internal/extensionserver/server/programmedset.go b/internal/extensionserver/server/programmedset.go index d404a4b5..2f741878 100644 --- a/internal/extensionserver/server/programmedset.go +++ b/internal/extensionserver/server/programmedset.go @@ -135,7 +135,7 @@ func buildProgrammedSet( if isConnectRoute(rt) { add(FamilyConnectorRoute, connectorRouteKey(rcName, vhName, rt.GetName())) } - if isOfflineDirectResponse(rt) { + if isOfflineRoute(rt) { add(FamilyConnectorOffline, connectorRouteKey(rcName, vhName, rt.GetName())) } } diff --git a/internal/extensionserver/server/programmedset_scan.go b/internal/extensionserver/server/programmedset_scan.go index e2906dfd..f24f37f0 100644 --- a/internal/extensionserver/server/programmedset_scan.go +++ b/internal/extensionserver/server/programmedset_scan.go @@ -80,23 +80,30 @@ func isConnectRoute(rt *routev3.Route) bool { return false } -// isOfflineDirectResponse reports whether a route directly returns the -// tunnel-offline 503 response, covering both the dedicated offline route and -// user-facing routes rewritten to it. -func isOfflineDirectResponse(rt *routev3.Route) bool { - dr := rt.GetDirectResponse() - if dr == nil { - return false +// isOfflineRoute reports whether a route is part of the connector offline path. +// That path produces two shapes, so both count: the CONNECT route answering the +// connector agent keeps a terse direct_response, while user-facing routes +// forward to the shared endpoint-less cluster when the branded error page is +// configured, so that the response carries the flag the offline page needs. +func isOfflineRoute(rt *routev3.Route) bool { + if ra := rt.GetRoute(); ra != nil { + return ra.GetCluster() == offlineTunnelCluster } - if dr.GetStatus() != 503 { + dr := rt.GetDirectResponse() + if dr == nil || dr.GetStatus() != 503 { return false } return dr.GetBody().GetInlineString() == offlineBodyMarker } -// offlineBodyMarker is the response body the connector offline path writes, -// duplicated here so the scanner needs no import dependency. -const offlineBodyMarker = "Tunnel not online" +// offlineBodyMarker is the response body the connector offline path writes on +// its CONNECT route, and offlineTunnelCluster is the shared endpoint-less +// cluster its user-facing routes point at. Both duplicated here so the scanner +// needs no import dependency. +const ( + offlineBodyMarker = "Tunnel not online" + offlineTunnelCluster = "datum-offline-tunnel" +) // isReplacedConnectorCluster reports whether a connector cluster has been // replaced with its tunnel form. A cluster that has not been replaced means the diff --git a/internal/extensionserver/server/server.go b/internal/extensionserver/server/server.go index 156f2836..ed547fd9 100644 --- a/internal/extensionserver/server/server.go +++ b/internal/extensionserver/server/server.go @@ -245,6 +245,12 @@ func (s *Server) PostTranslateModify( } s.markTPPsProgrammed(ctx, appliedTPPs) + // Whether the branded error page is configured at all. Both the connector + // offline path and the empty-backend path route user traffic to the shared + // endpoint-less cluster only when there is a page to serve; with no page, + // each keeps the answer it gave before. + brandedOffline := !s.cfg.LocalReply.Disabled && s.cfg.LocalReply.OfflineBodyHTML != "" + // --- Connector family --- // Replace clusters BEFORE adding CONNECT routes so route wiring sees the // final cluster set. Apply connector routes AFTER TPP so CONNECT routes @@ -267,7 +273,7 @@ func (s *Server) PostTranslateModify( _, connRoutesSpan := tr.Start(mctx, "connector.routes") for _, rc := range routes { - n, offlineRt, mutErr := mutate.ApplyConnectorRoutes(rc, idx, replaced, connOffline) + n, offlineRt, mutErr := mutate.ApplyConnectorRoutes(rc, idx, replaced, connOffline, brandedOffline) if mutErr != nil { s.log.Error("apply connector routes", "route_config", rc.GetName(), "err", mutErr) connRoutesSpan.RecordError(mutErr) @@ -300,14 +306,23 @@ func (s *Server) PostTranslateModify( // serve, rewriting would only trade EG's deterministic 503 for Envoy's // generic no_healthy_upstream and buy nothing. var emptyBackendCount int - if !s.cfg.LocalReply.Disabled && s.cfg.LocalReply.OfflineBodyHTML != "" { + if brandedOffline { _, emptyBackendSpan := tr.Start(mctx, "emptybackend.routes") for _, rc := range routes { emptyBackendCount += mutate.RouteEmptyBackendsToOfflineCluster(rc) } - if emptyBackendCount > 0 { + // Each family has its own shared sink, added only when something points + // at it, so the data-plane stats say which reason a request hit. + wanted := map[string]bool{ + mutate.OfflineBackendClusterName: emptyBackendCount > 0, + mutate.OfflineTunnelClusterName: offlineRtCount > 0, + } + for name, needed := range wanted { + if !needed { + continue + } var ebErr error - clusters, _, ebErr = mutate.EnsureOfflineBackendCluster(clusters) + clusters, _, ebErr = mutate.EnsureOfflineCluster(clusters, name) if ebErr != nil { s.log.Error("ensure offline backend cluster", "err", ebErr) emptyBackendSpan.RecordError(ebErr) diff --git a/test/e2e-edge/networkservice-drained-to-zero/chainsaw-test.yaml b/test/e2e-edge/networkservice-drained-to-zero/chainsaw-test.yaml new file mode 100644 index 00000000..ed860acc --- /dev/null +++ b/test/e2e-edge/networkservice-drained-to-zero/chainsaw-test.yaml @@ -0,0 +1,330 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +# +# The transition production hit in issue #502: a NetworkService that was +# serving loses its last member. +# +# A service resolves one member, a server answers at that member's allocated +# address, and a real request through the edge comes back 200. The claim is +# then released — the equivalent of the workload behind it going to zero — and +# the same request is sent again. +# +# The question this holds open is what changes when that happens. The service's +# own status does change. Whether anything on the HTTPProxy, on the downstream +# Gateway, or on the downstream HTTPRoute changes with it is the gap, and both +# responses are printed side by side so the difference is the record. +# +# The member address is bound on the downstream node and served by a +# host-network pod, exactly as in ../networkservice-endpoints — there is no +# compute in this environment, so nothing answers at an allocated address on +# its own. +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: networkservice-drained-to-zero +spec: + concurrent: false + cluster: nso-upstream + namespaceTemplate: + metadata: + labels: + meta.datumapis.com/upstream-cluster-name: cluster-project-alpha + meta.datumapis.com/upstream-namespace: default + bindings: + - name: downstreamGatewayClass + value: datum-downstream-gateway-e2e + - name: envoyNamespace + value: datum-downstream-gateway + - name: downstreamNode + value: nso-downstream-control-plane + - name: boundAddressFile + value: /tmp/nsvc-drain-bound-addresses + steps: + - name: Deliver the network's presence to this location + try: + - apply: + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: NetworkContext + metadata: + name: drain-net-us-central-1 + spec: + network: + name: drain-net + location: + name: us-central-1 + ipFamilies: + - IPv4 + mtu: 1440 + + - name: One claim binds and its interface carries what a service selects + try: + - create: + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: Network + metadata: + name: drain-net + spec: + ipam: + mode: Auto + ipFamilies: + - IPv6 + - create: + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: NetworkInterfaceClaim + metadata: + name: drain-member-1 + labels: + compute.datumapis.com/workload-name: drain + spec: + network: + name: drain-net + ipFamilies: + - IPv4 + - script: + timeout: 300s + content: | + set -eu + kubectl -n "$NAMESPACE" wait networkinterfaceclaim drain-member-1 \ + --for=condition=Bound --timeout=150s + kubectl -n "$NAMESPACE" wait networkinterfaceclaim drain-member-1 \ + --for=condition=Allocated --timeout=150s + kubectl -n "$NAMESPACE" patch networkinterface drain-member-1 \ + --subresource=status --type=merge -p \ + '{"status":{"conditions":[{"type":"HolderAvailable","status":"True","reason":"HolderAvailable","message":"Reported by the scenario in place of a workload instance","lastTransitionTime":"2026-01-01T00:00:00Z"}]}}' + + - name: The service resolves its one member + try: + - create: + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: NetworkService + metadata: + name: drain-svc + spec: + networkInterfaces: + selector: + matchLabels: + compute.datumapis.com/workload-name: drain + ports: + - name: http + port: 8080 + - assert: + timeout: 120s + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: NetworkService + metadata: + name: drain-svc + status: + summary: + locations: 1 + members: 1 + healthy: 1 + (conditions[?type == 'Ready'] | [0].status): "True" + + - name: Make the member answer at its allocated address + try: + - script: + env: + - name: NODE + value: ($downstreamNode) + - name: BOUND_FILE + value: ($boundAddressFile) + content: | + set -eu + : > "${BOUND_FILE}.${NAMESPACE}" + addr=$(kubectl -n "$NAMESPACE" get networkinterfaceclaim drain-member-1 \ + -o jsonpath='{.status.addresses[0].address}') + echo "binding ${addr} on ${NODE}" + docker exec "$NODE" ip addr add "$addr" dev lo + echo "$addr" >> "${BOUND_FILE}.${NAMESPACE}" + - apply: + cluster: nso-downstream + file: ../_fixtures/network-service-member.yaml + - script: + cluster: nso-downstream + timeout: 300s + content: | + kubectl -n default wait pod/network-service-member --for=condition=Ready --timeout=240s + cleanup: + - script: + env: + - name: NODE + value: ($downstreamNode) + - name: BOUND_FILE + value: ($boundAddressFile) + content: | + set -u + if [ -f "${BOUND_FILE}.${NAMESPACE}" ]; then + while read -r addr; do + [ -n "$addr" ] || continue + docker exec "$NODE" ip addr del "$addr" dev lo || true + done < "${BOUND_FILE}.${NAMESPACE}" + rm -f "${BOUND_FILE}.${NAMESPACE}" + fi + - script: + cluster: nso-downstream + content: | + kubectl -n default delete pod network-service-member drain-conntest \ + --ignore-not-found --force --grace-period=0 || true + + - name: The proxy over it serves a real 200 + try: + - create: + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: drain-proxy + spec: + rules: + - matches: + - path: + type: PathPrefix + value: / + backends: + - networkService: + name: drain-svc + port: http + - script: + timeout: 300s + content: | + set -eu + for i in $(seq 1 40); do + H=$(kubectl -n "$NAMESPACE" get httpproxy drain-proxy \ + -o jsonpath='{.status.canonicalHostname}' 2>/dev/null || true) + if [ -n "${H}" ]; then echo "canonical hostname: ${H}"; exit 0; fi + sleep 5 + done + echo "ERROR: HTTPProxy never published a canonicalHostname" >&2 + exit 1 + - script: + skipCommandOutput: true + skipLogOutput: true + content: | + kubectl -n "$NAMESPACE" get httpproxy drain-proxy -o jsonpath='{.status.canonicalHostname}' + outputs: + - name: proxyHostname + value: ($stdout) + - apply: + cluster: nso-downstream + resource: + apiVersion: v1 + kind: Pod + metadata: + name: drain-conntest + namespace: default + spec: + containers: + - name: curl + image: curlimages/curl:8.11.1 + command: ["sleep", "infinity"] + resources: + requests: + cpu: 50m + memory: 64Mi + limits: + cpu: 100m + memory: 128Mi + terminationGracePeriodSeconds: 0 + - script: + cluster: nso-downstream + timeout: 480s + env: + - name: HOST + value: ($proxyHostname) + - name: OWNING_GC + value: ($downstreamGatewayClass) + - name: ENVOY_NS + value: ($envoyNamespace) + content: | + set -eu + kubectl -n default wait pod/drain-conntest --for=condition=Ready --timeout=240s + IP=$(kubectl get svc -n "$ENVOY_NS" \ + -l "gateway.envoyproxy.io/owning-gatewayclass=${OWNING_GC}" \ + -o jsonpath='{.items[0].spec.clusterIP}') + ok="" + for i in $(seq 1 30); do + code=$(kubectl -n default exec drain-conntest -- \ + curl -ksS -o /dev/null -w '%{http_code}' --max-time 15 \ + --resolve "${HOST}:80:${IP}" "http://${HOST}/get" 2>/dev/null || true) + echo "attempt ${i}: ${code}" + if [ "${code}" = "200" ]; then ok="yes"; break; fi + sleep 5 + done + [ -n "$ok" ] || { echo "ERROR: the member never served"; exit 1; } + echo "BEFORE: the proxy serves 200 from its one member" + + - name: The workload goes to zero + description: | + The claim is released, which retires the interface. Nothing else is + touched: no edit to the service, the proxy, or anything downstream. + try: + - delete: + ref: + apiVersion: networking.datumapis.com/v1alpha + kind: NetworkInterfaceClaim + name: drain-member-1 + - assert: + timeout: 180s + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: NetworkService + metadata: + name: drain-svc + status: + summary: + locations: 0 + members: 0 + healthy: 0 + (conditions[?type == 'MembersResolved'] | [0].status): "False" + (conditions[?type == 'MembersResolved'] | [0].reason): NoMatchingInterfaces + - script: + content: | + echo "--- networkservice after the drain" + kubectl -n "$NAMESPACE" get networkservice drain-svc -o yaml + echo "--- httpproxy after the drain" + kubectl -n "$NAMESPACE" get httpproxy drain-proxy -o yaml + echo "--- upstream endpointslice after the drain" + kubectl -n "$NAMESPACE" get endpointslice drain-proxy-0-0 -o yaml + + - name: The same request after the drain + try: + - script: + skipCommandOutput: true + skipLogOutput: true + content: | + kubectl -n "$NAMESPACE" get httpproxy drain-proxy -o jsonpath='{.status.canonicalHostname}' + outputs: + - name: proxyHostname + value: ($stdout) + - script: + cluster: nso-downstream + timeout: 300s + env: + - name: HOST + value: ($proxyHostname) + - name: OWNING_GC + value: ($downstreamGatewayClass) + - name: ENVOY_NS + value: ($envoyNamespace) + content: | + set -eu + IP=$(kubectl get svc -n "$ENVOY_NS" \ + -l "gateway.envoyproxy.io/owning-gatewayclass=${OWNING_GC}" \ + -o jsonpath='{.items[0].spec.clusterIP}') + echo "--- full response after the drain" + kubectl -n default exec drain-conntest -- \ + curl -ksS -i --max-time 20 --resolve "${HOST}:80:${IP}" "http://${HOST}/get" || true + echo "--- downstream route conditions after the drain" + kubectl get httproute -A -o json \ + | python3 -c ' + import json,sys + d=json.load(sys.stdin) + for r in d.get("items",[]): + for p in r.get("status",{}).get("parents",[]): + for c in p.get("conditions",[]): + print(r["metadata"]["name"], c["type"], c["status"], c.get("reason"), "|", c.get("message")) + ' diff --git a/test/e2e-edge/networkservice-no-endpoints/chainsaw-test.yaml b/test/e2e-edge/networkservice-no-endpoints/chainsaw-test.yaml new file mode 100644 index 00000000..bf777f7c --- /dev/null +++ b/test/e2e-edge/networkservice-no-endpoints/chainsaw-test.yaml @@ -0,0 +1,298 @@ +# yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json +# +# A NetworkService with no members leaves the proxy in front of it looking +# healthy while nothing can be served through it. +# +# This is the repro for issue #502. Two shapes of the same state are covered: +# +# * never populated — the selector matches nothing, so MembersResolved is +# false for NoMatchingInterfaces from the moment the service is written. +# +# * drained — a service that resolved a real member, served a real 200 +# through the edge, and then lost its member when the claim went away. +# This is the transition production hit. +# +# What is being captured in both shapes is the gap between what the platform +# reports and what a user gets: the HTTPProxy stays Accepted and Programmed, +# the downstream Service and EndpointSlice exist, and the request fails. +apiVersion: chainsaw.kyverno.io/v1alpha1 +kind: Test +metadata: + name: networkservice-no-endpoints +spec: + concurrent: false + cluster: nso-upstream + namespaceTemplate: + metadata: + labels: + meta.datumapis.com/upstream-cluster-name: cluster-project-alpha + meta.datumapis.com/upstream-namespace: default + bindings: + - name: downstreamGatewayClass + value: datum-downstream-gateway-e2e + - name: envoyNamespace + value: datum-downstream-gateway + - name: downstreamNode + value: nso-downstream-control-plane + - name: adminLocalPort + value: "19903" + - name: boundAddressFile + value: /tmp/nsvc-empty-bound-addresses + steps: + - name: A service whose selector matches nothing does not resolve members + description: | + The plain shape of the bug. Nothing holds the label, so the service + reports NoMatchingInterfaces and summarises zero of everything. + try: + - create: + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: NetworkService + metadata: + name: empty-svc + spec: + networkInterfaces: + selector: + matchLabels: + compute.datumapis.com/workload-name: nothing-holds-this + ports: + - name: http + port: 8080 + - assert: + timeout: 120s + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: NetworkService + metadata: + name: empty-svc + status: + summary: + locations: 0 + members: 0 + healthy: 0 + (conditions[?type == 'MembersResolved'] | [0].status): "False" + (conditions[?type == 'MembersResolved'] | [0].reason): NoMatchingInterfaces + (conditions[?type == 'Ready'] | [0].status): "False" + (conditions[?type == 'Ready'] | [0].reason): NoMatchingInterfaces + - script: + content: | + echo "--- networkservice empty-svc" + kubectl -n "$NAMESPACE" get networkservice empty-svc -o yaml + + - name: An HTTPProxy over the empty service reports itself healthy + description: | + The heart of #502. The proxy has nowhere to send a request and says + nothing about it: Accepted and Programmed both stay true, and the + hostname is published as if the service were serving. + try: + - create: + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: empty-proxy + spec: + rules: + - matches: + - path: + type: PathPrefix + value: / + backends: + - networkService: + name: empty-svc + port: http + - script: + timeout: 300s + content: | + set -eu + for i in $(seq 1 40); do + H=$(kubectl -n "$NAMESPACE" get httpproxy empty-proxy \ + -o jsonpath='{.status.canonicalHostname}' 2>/dev/null || true) + if [ -n "${H}" ]; then echo "canonical hostname: ${H}"; break; fi + echo "waiting for HTTPProxy canonicalHostname... ${i}" >&2 + sleep 5 + done + echo "--- httpproxy empty-proxy status" + kubectl -n "$NAMESPACE" get httpproxy empty-proxy -o yaml + - assert: + timeout: 120s + resource: + apiVersion: networking.datumapis.com/v1alpha + kind: HTTPProxy + metadata: + name: empty-proxy + status: + (conditions[?type == 'Accepted'] | [0].status): "True" + (conditions[?type == 'Programmed'] | [0].status): "True" + + - name: The upstream EndpointSlice is present and carries no endpoints + description: | + The slice is deliberately written empty rather than withheld, so the + Gateway controller's reference stays resolvable. It is also what makes + the state invisible: the object exists and looks well-formed. + try: + - script: + timeout: 120s + content: | + set -eu + for i in $(seq 1 24); do + if kubectl -n "$NAMESPACE" get endpointslice empty-proxy-0-0 >/dev/null 2>&1; then + break + fi + sleep 5 + done + echo "--- upstream endpointslice" + kubectl -n "$NAMESPACE" get endpointslice empty-proxy-0-0 -o yaml + n=$(kubectl -n "$NAMESPACE" get endpointslice empty-proxy-0-0 \ + -o jsonpath='{.endpoints}' | tr -d '[:space:]') + [ -z "$n" ] || [ "$n" = "[]" ] || [ "$n" = "null" ] \ + || { echo "ERROR: slice carries endpoints: $n"; exit 1; } + echo "OK: the slice exists and carries no endpoints" + + - name: The downstream Service is headless and its slice is empty + description: | + What the edge cluster actually holds. The Service is written headless + with no selector, so nothing in that cluster will ever fill it in; the + only endpoints it can have are the ones mirrored from upstream, and + there are none. + try: + - script: + skipCommandOutput: true + skipLogOutput: true + content: | + kubectl -n "$NAMESPACE" get httproute empty-proxy \ + -o jsonpath='{.spec.rules[0].backendRefs[0].name}' + outputs: + - name: upstreamSliceName + value: ($stdout) + - script: + cluster: nso-downstream + timeout: 300s + env: + - name: ENVOY_NS + value: ($envoyNamespace) + - name: UPSTREAM_SLICE + value: ($upstreamSliceName) + content: | + set -eu + echo "upstream slice the route names: ${UPSTREAM_SLICE}" + NAME="" + for i in $(seq 1 30); do + NAME=$(kubectl get endpointslice -A \ + -l "meta.datumapis.com/upstream-name=${UPSTREAM_SLICE}" \ + -o jsonpath='{.items[0].metadata.name}' 2>/dev/null || true) + DS_NS=$(kubectl get endpointslice -A \ + -l "meta.datumapis.com/upstream-name=${UPSTREAM_SLICE}" \ + -o jsonpath='{.items[0].metadata.namespace}' 2>/dev/null || true) + [ -n "$NAME" ] && break + echo "attempt ${i}: no mirrored downstream slice yet" + sleep 5 + done + [ -n "$NAME" ] || { echo "ERROR: nothing mirrored downstream"; exit 1; } + + echo "the mapped downstream namespace: ${DS_NS}" + echo "--- downstream service ${NAME}" + kubectl -n "$DS_NS" get svc "$NAME" -o yaml + echo "--- downstream endpointslice ${NAME}" + kubectl -n "$DS_NS" get endpointslice "$NAME" -o yaml + + ip=$(kubectl -n "$DS_NS" get svc "$NAME" -o jsonpath='{.spec.clusterIP}') + [ "$ip" = "None" ] || { echo "ERROR: downstream service clusterIP is ${ip}, want None"; exit 1; } + sel=$(kubectl -n "$DS_NS" get svc "$NAME" -o jsonpath='{.spec.selector}') + [ -z "$sel" ] || { echo "ERROR: downstream service carries selector ${sel}"; exit 1; } + eps=$(kubectl -n "$DS_NS" get endpointslice "$NAME" \ + -o jsonpath='{.endpoints}' | tr -d '[:space:]') + [ -z "$eps" ] || [ "$eps" = "[]" ] || [ "$eps" = "null" ] \ + || { echo "ERROR: downstream slice carries endpoints: $eps"; exit 1; } + echo "OK: headless service, no selector, zero endpoints" + + - name: What the downstream Gateway and HTTPRoute say about it + description: | + Captured rather than asserted: whether anything in the Gateway API + surfaces the unresolvable backend, and under which condition and + reason, is exactly the question #502 asks. + try: + - script: + cluster: nso-downstream + env: + - name: ENVOY_NS + value: ($envoyNamespace) + - name: OWNING_GC + value: ($downstreamGatewayClass) + content: | + set -eu + echo "--- downstream gateways" + kubectl get gateway -A -o yaml + echo "--- downstream httproutes" + kubectl get httproute -A -o yaml + echo "--- every route condition, whatever its type" + kubectl get httproute -A -o json \ + | python3 -c ' + import json,sys + d=json.load(sys.stdin) + for r in d.get("items",[]): + for p in r.get("status",{}).get("parents",[]): + for c in p.get("conditions",[]): + print(r["metadata"]["name"], c["type"], c["status"], c.get("reason"), "|", c.get("message")) + ' + + - name: A real request to the empty proxy + description: | + The verdict a user gets. Whatever the control plane reports, this is + the response. + try: + - script: + skipCommandOutput: true + skipLogOutput: true + content: | + kubectl -n "$NAMESPACE" get httpproxy empty-proxy -o jsonpath='{.status.canonicalHostname}' + outputs: + - name: proxyHostname + value: ($stdout) + - apply: + cluster: nso-downstream + resource: + apiVersion: v1 + kind: Pod + metadata: + name: empty-conntest + namespace: default + spec: + containers: + - name: curl + image: curlimages/curl:8.11.1 + command: ["sleep", "infinity"] + resources: + requests: + cpu: 50m + memory: 64Mi + limits: + cpu: 100m + memory: 128Mi + terminationGracePeriodSeconds: 0 + - script: + cluster: nso-downstream + timeout: 480s + env: + - name: HOST + value: ($proxyHostname) + - name: OWNING_GC + value: ($downstreamGatewayClass) + - name: ENVOY_NS + value: ($envoyNamespace) + content: | + set -eu + kubectl -n default wait pod/empty-conntest --for=condition=Ready --timeout=240s + IP=$(kubectl get svc -n "$ENVOY_NS" \ + -l "gateway.envoyproxy.io/owning-gatewayclass=${OWNING_GC}" \ + -o jsonpath='{.items[0].spec.clusterIP}') + echo "proxy hostname ${HOST} at edge ${IP}" + echo "--- full response" + kubectl -n default exec empty-conntest -- \ + curl -ksS -i --max-time 20 --resolve "${HOST}:80:${IP}" "http://${HOST}/get" + cleanup: + - script: + cluster: nso-downstream + content: | + kubectl -n default delete pod empty-conntest \ + --ignore-not-found --force --grace-period=0 || true diff --git a/test/parity/scan.go b/test/parity/scan.go index 0b4d5294..bf4ca475 100644 --- a/test/parity/scan.go +++ b/test/parity/scan.go @@ -23,6 +23,7 @@ const ( connectorInternalTransport = "envoy.transport_sockets.internal_upstream" hcmNetworkFilterName = "envoy.filters.network.http_connection_manager" offlineBodyMarker = "Tunnel not online" + offlineTunnelCluster = "datum-offline-tunnel" ) // ScanActual scans the proxy's live configuration and assembles what it @@ -72,7 +73,7 @@ func scanRouteConfig(rc *routev3.RouteConfiguration, act *Actual) { act.Keys[FamilyConnectorRoute] = append(act.Keys[FamilyConnectorRoute], connectorRouteKey(rcName, vhName, rt.GetName())) } - if isOfflineDirectResponse(rt) { + if isOfflineRoute(rt) { act.Keys[FamilyConnectorOffline] = append(act.Keys[FamilyConnectorOffline], connectorRouteKey(rcName, vhName, rt.GetName())) } @@ -143,12 +144,17 @@ func isConnectRoute(rt *routev3.Route) bool { return false } -func isOfflineDirectResponse(rt *routev3.Route) bool { - dr := rt.GetDirectResponse() - if dr == nil { - return false +// isOfflineRoute reports whether a route is part of the connector offline path. +// That path produces two shapes, so both count: the CONNECT route answering the +// connector agent keeps a terse direct_response, while user-facing routes +// forward to the shared endpoint-less cluster when the branded error page is +// configured, so that the response carries the flag the offline page needs. +func isOfflineRoute(rt *routev3.Route) bool { + if ra := rt.GetRoute(); ra != nil { + return ra.GetCluster() == offlineTunnelCluster } - if dr.GetStatus() != 503 { + dr := rt.GetDirectResponse() + if dr == nil || dr.GetStatus() != 503 { return false } return dr.GetBody().GetInlineString() == offlineBodyMarker From c1ef6604c9e2415b5c231d57bf4175851b74d090 Mon Sep 17 00:00:00 2001 From: Scot Wells Date: Tue, 29 Sep 2026 17:43:43 -0500 Subject: [PATCH 3/3] chore: Trim comments to the repo style Comment blocks added with the offline-page fix explained mechanism at length where the code already carries it. Keep the reasoning a reader cannot recover from the code and drop the rest. Also revert the operator image tags three kustomizations picked up from a local test environment. Key changes: - Reduce doc comments in the mutate, metrics and scan paths - Shorten both new chainsaw scenario headers - Restore config image tags to their committed values Co-Authored-By: Claude Opus 5 (1M context) --- .../cell-controllers/kustomization.yaml | 20 +++--- config/e2e-downstream/kustomization.yaml | 2 +- config/manager/kustomization.yaml | 2 +- internal/extensionserver/metrics/metrics.go | 13 ++-- internal/extensionserver/mutate/connector.go | 29 +++------ .../extensionserver/mutate/connector_test.go | 6 +- .../extensionserver/mutate/emptybackend.go | 64 +++++-------------- .../mutate/emptybackend_test.go | 13 ++-- .../server/programmedset_scan.go | 13 ++-- internal/extensionserver/server/server.go | 22 ++----- .../chainsaw-test.yaml | 21 ++---- .../chainsaw-test.yaml | 17 +---- test/parity/scan.go | 8 +-- 13 files changed, 72 insertions(+), 158 deletions(-) diff --git a/config/components/cell-controllers/kustomization.yaml b/config/components/cell-controllers/kustomization.yaml index 47cebcfe..503ed273 100644 --- a/config/components/cell-controllers/kustomization.yaml +++ b/config/components/cell-controllers/kustomization.yaml @@ -2,21 +2,21 @@ apiVersion: kustomize.config.k8s.io/v1alpha1 kind: Component resources: -- deployment.yaml -- metrics_service.yaml + - deployment.yaml + - metrics_service.yaml # A component's transformers apply to the parent's whole accumulation, so this # matches a name only the cell Deployment carries. Overlays pinning # ghcr.io/datum-cloud/network-services-operator still reach both Deployments: # their transformer runs after this one has resolved the name. images: -- name: network-services-operator-cell - newName: ghcr.io/datum-cloud/network-services-operator - newTag: bf172257 + - name: network-services-operator-cell + newName: ghcr.io/datum-cloud/network-services-operator + newTag: latest configMapGenerator: -- files: - - config.yaml - name: cell-config - options: - disableNameSuffixHash: true + - name: cell-config + files: + - config.yaml + options: + disableNameSuffixHash: true diff --git a/config/e2e-downstream/kustomization.yaml b/config/e2e-downstream/kustomization.yaml index c1c06250..2f7f5f4f 100644 --- a/config/e2e-downstream/kustomization.yaml +++ b/config/e2e-downstream/kustomization.yaml @@ -22,7 +22,7 @@ resources: images: - name: ghcr.io/datum-cloud/network-services-operator newName: ghcr.io/datum-cloud/network-services-operator - newTag: bf172257 + newTag: 342d69f # The default e2e path has no Prometheus operator; drop the ServiceMonitor so # the apply doesn't fail on the missing monitoring.coreos.com CRD. (Re-add via diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index d3213b8f..657a599a 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -6,7 +6,7 @@ resources: images: - name: ghcr.io/datum-cloud/network-services-operator newName: ghcr.io/datum-cloud/network-services-operator - newTag: bf172257 + newTag: 342d69f configMapGenerator: - files: - config.yaml diff --git a/internal/extensionserver/metrics/metrics.go b/internal/extensionserver/metrics/metrics.go index affafcaa..0342361d 100644 --- a/internal/extensionserver/metrics/metrics.go +++ b/internal/extensionserver/metrics/metrics.go @@ -164,9 +164,7 @@ var ( // ConnectorOfflineRoutesTotal counts total user-facing forwarding routes // moved off an offline connector's own cluster across all hook invocations. - // With the branded error page configured they are pointed at the shared - // endpoint-less cluster, so the user gets the offline page; without it they - // return a deterministic 503. Use rate()/sum() to derive per-build averages. + // Use rate()/sum() to derive per-build averages. ConnectorOfflineRoutesTotal = promauto.NewCounter( prometheus.CounterOpts{ Name: "nso_extension_connector_offline_routes_total", @@ -174,11 +172,10 @@ var ( }, ) - // EmptyBackendRoutesTotal counts total routes Envoy Gateway collapsed to a - // bodiless 503 direct_response (backend Service present, no ready - // endpoints) that were rewritten to forward to the shared endpoint-less - // cluster, across all hook invocations. Each increment = one route that now - // answers with the branded offline page instead of the generic error page. + // EmptyBackendRoutesTotal counts total routes with no ready endpoints + // pointed at the shared offline backend cluster across all hook + // invocations. Each increment is one route that now answers with the + // branded offline page instead of the generic error page. EmptyBackendRoutesTotal = promauto.NewCounter( prometheus.CounterOpts{ Name: "nso_extension_empty_backend_routes_total", diff --git a/internal/extensionserver/mutate/connector.go b/internal/extensionserver/mutate/connector.go index 2e34d57b..34cfa254 100644 --- a/internal/extensionserver/mutate/connector.go +++ b/internal/extensionserver/mutate/connector.go @@ -78,14 +78,10 @@ func ReplaceConnectorClusters( // clients) and rewrite the user-facing forwarding routes (see the offline // branch for why). // -// brandedOffline selects what a user reaching an offline tunnel gets. When the -// branded error page is configured, the user-facing routes forward to the -// shared endpoint-less cluster, which makes Envoy set the UH response flag the -// offline page selects on. When it is not, they keep the deterministic 503 -// direct_response, which is what they have always returned. -// -// The CONNECT route is unaffected either way: it answers the connector agent, -// not a browser, and a terse body is the right answer there. +// brandedOffline selects what a user reaching an offline tunnel gets: the +// shared endpoint-less cluster when there is a branded error page to serve, +// otherwise the deterministic 503 they have always returned. The CONNECT route +// is unaffected either way, since it answers the connector agent, not a browser. // // Returns the number of VirtualHosts mutated and the number of user-facing // forwarding routes rewritten. @@ -139,18 +135,11 @@ func ApplyConnectorRoutes( } vh.Routes = append([]*routev3.Route{newRoute}, vh.Routes...) - // Move user traffic off the connector's own endpoint-less cluster, - // which would otherwise report retry and connect failures against a - // per-connector cluster. Replacing only the Action oneof preserves - // each route's match/metadata; idempotent because neither - // replacement carries the connector cluster to re-match. - // - // With branding on, the shared endpoint-less cluster is the target, - // because a direct_response short-circuits before the router filter - // and so never carries the UH flag the offline page selects on. The - // cluster is shared, so this adds no per-connector data-plane stats. - // With branding off, a deterministic 503 remains the better answer - // than Envoy's generic no_healthy_upstream text. + // Move user traffic off the connector's own endpoint-less + // cluster, which would otherwise report retry and connect + // failures against a per-connector cluster. Replacing only the + // Action oneof preserves each route's match and metadata, and + // neither replacement leaves the connector cluster to re-match. for _, rt := range vh.GetRoutes() { if routeCluster(rt) != connectorCluster { continue diff --git a/internal/extensionserver/mutate/connector_test.go b/internal/extensionserver/mutate/connector_test.go index 0ae007a4..31c13be2 100644 --- a/internal/extensionserver/mutate/connector_test.go +++ b/internal/extensionserver/mutate/connector_test.go @@ -504,10 +504,8 @@ func TestApplyConnectorRoutes_EmptyRouteConfiguration_NoOp(t *testing.T) { assert.Equal(t, 0, converted) } -// With the branded page configured, an offline connector's user-facing routes -// must forward to the shared endpoint-less cluster rather than answer with a -// direct_response, because only the forwarded request carries the UH flag the -// offline page selects on. +// Only a forwarded request carries the UH flag the offline page selects on, so +// with the branded page configured the user-facing routes must forward. func TestApplyConnectorRoutes_Offline_BrandedRoutesToTunnelCluster(t *testing.T) { idx := connectorPolicyIndex(false) clusterName := testClusterName() diff --git a/internal/extensionserver/mutate/emptybackend.go b/internal/extensionserver/mutate/emptybackend.go index fa5851a2..2c066d90 100644 --- a/internal/extensionserver/mutate/emptybackend.go +++ b/internal/extensionserver/mutate/emptybackend.go @@ -8,14 +8,6 @@ import ( "google.golang.org/protobuf/encoding/protojson" ) -// The endpoint-less clusters user traffic is pointed at when its backend cannot -// serve. One shared cluster per reason rather than one per backend or per -// connector: an endpoint-less cluster carries ~108 resident stats in the data -// plane, so per-backend clusters would scale that by the number of idle -// services, while these two keep it constant. -// -// The two reasons stay apart so the data-plane stats say which one a request -// hit, and so the parity scanner can tell the families apart. const ( // OfflineBackendClusterName serves a backend with no ready endpoints. OfflineBackendClusterName = "datum-offline-backend" @@ -25,20 +17,13 @@ const ( // emptyBackendStatus is the status Envoy Gateway collapses a route to when the // backend Service exists but has no ready endpoints. It is the only collapse -// case EG answers with 503 — every other one (filter error, invalid -// destination, all-zero weights) uses 500 — which is what makes the status a -// safe discriminator. See EG internal/gatewayapi/route.go, "return 503 if no -// ready endpoints exist". +// case EG answers with 503; every other one uses 500. const emptyBackendStatus = 503 -// EnsureOfflineCluster appends the named shared endpoint-less cluster to the -// xDS cluster set if it is not already present, returning the (possibly -// extended) set and whether it added one. -// -// The cluster is STATIC with an empty endpoint list. A request routed to it -// fails at host selection, so Envoy answers 503 with the UH (no healthy -// upstream) response flag and never attempts a connection. UH is what the -// branded offline page keys on; see buildLocalReplyConfig in localreply.go. +// EnsureOfflineCluster appends the named endpoint-less cluster if it is absent, +// returning the cluster set and whether it added one. A request routed there +// fails at host selection, so Envoy answers with the UH response flag the +// branded offline page selects on. func EnsureOfflineCluster(clusters []*clusterv3.Cluster, name string) ([]*clusterv3.Cluster, bool, error) { for _, c := range clusters { if c.GetName() == name { @@ -60,29 +45,15 @@ func EnsureOfflineCluster(clusters []*clusterv3.Cluster, name string) ([]*cluste return append(clusters, c), true, nil } -// RouteEmptyBackendsToOfflineCluster rewrites every route Envoy Gateway -// collapsed to a bodiless 503 direct_response so it forwards to the shared -// endpoint-less cluster instead. -// -// A direct_response short-circuits before the router filter runs, so such a -// route produces no response flag and no upstream attempt. That makes the -// offline page unreachable: local_reply_config mappers can only select on the -// request and on Envoy's own response metadata, and a direct_response supplies -// neither a UH flag nor any route-supplied header (route-level -// request_headers_to_add is applied by the router filter, which never runs). -// A direct_response body is no use either — local_reply_config overrides it. -// Forwarding to an endpoint-less cluster restores the UH flag, which is the -// only signal the mapper can actually match. +// RouteEmptyBackendsToOfflineCluster points every route Envoy Gateway collapsed +// to a bodiless 503 at the shared endpoint-less cluster, returning how many it +// rewrote. A direct_response short-circuits before the router filter, so it +// carries neither the UH flag nor any route-supplied header the offline page +// could select on, and its body is overridden by local_reply_config. Forwarding +// restores the flag. // -// Only the Action oneof is replaced, so each route's match, metadata and -// typed_per_filter_config survive — a route that already carries Coraza -// per-route config keeps it. No retry policy is attached: with no hosts there -// is nothing to retry onto, and Envoy fails fast at host selection. -// -// Idempotent: a rewritten route is no longer a direct_response, so a second -// pass does not match it. -// -// Returns the number of routes rewritten. +// Replacing only the Action oneof preserves each route's match, metadata and +// per-filter config, and makes a second pass a no-op. func RouteEmptyBackendsToOfflineCluster(rc *routev3.RouteConfiguration) int { rewritten := 0 for _, vh := range rc.GetVirtualHosts() { @@ -103,12 +74,9 @@ func RouteEmptyBackendsToOfflineCluster(rc *routev3.RouteConfiguration) int { return rewritten } -// isEmptyBackendDirectResponse reports whether a route is EG's "backend has no -// ready endpoints" collapse, as opposed to any other direct_response. -// -// The body check is what separates it from NSO's own connector-offline routes, -// which carry an explicit body, and from a Gateway API filter's direct -// response. EG sets no body on the collapse. +// isEmptyBackendDirectResponse reports whether a route is EG's "no ready +// endpoints" collapse. The absent body is what separates it from NSO's own +// connector-offline routes and from a Gateway API filter's direct response. func isEmptyBackendDirectResponse(rt *routev3.Route) bool { dr := rt.GetDirectResponse() if dr == nil || dr.GetStatus() != emptyBackendStatus { diff --git a/internal/extensionserver/mutate/emptybackend_test.go b/internal/extensionserver/mutate/emptybackend_test.go index f0094e1c..455a540a 100644 --- a/internal/extensionserver/mutate/emptybackend_test.go +++ b/internal/extensionserver/mutate/emptybackend_test.go @@ -91,9 +91,8 @@ func TestRouteEmptyBackendsToOfflineCluster(t *testing.T) { } } -// A rewritten route must keep everything the route carried besides its action — -// most importantly the Coraza per-route config a governed route holds, which -// lives in typed_per_filter_config. +// A governed route's WAF config lives in typed_per_filter_config and must +// survive the rewrite. func TestRouteEmptyBackendsPreservesRouteState(t *testing.T) { perFilter, err := anypb.New(&routev3.FilterConfig{}) if err != nil { @@ -153,8 +152,8 @@ func TestEnsureOfflineCluster(t *testing.T) { if c.GetType() != clusterv3.Cluster_STATIC { t.Errorf("type = %v, want STATIC", c.GetType()) } - // The empty endpoint list is the whole point: it is what makes Envoy fail - // host selection and set the UH flag the offline page matches on. + // The empty endpoint list is what makes Envoy fail host selection and set + // the UH flag the offline page matches on. if eps := c.GetLoadAssignment().GetEndpoints(); len(eps) != 0 { t.Errorf("len(endpoints) = %d, want 0", len(eps)) } @@ -171,8 +170,8 @@ func TestEnsureOfflineCluster(t *testing.T) { } } -// The two sinks must stay distinct. A shared name would make the parity scanner -// count an idle backend as an offline tunnel. +// A shared name would make the parity scanner count an idle backend as an +// offline tunnel. func TestEnsureOfflineClusterKeepsTheTwoSinksApart(t *testing.T) { if OfflineBackendClusterName == OfflineTunnelClusterName { t.Fatal("the empty-backend and offline-tunnel sinks share a name") diff --git a/internal/extensionserver/server/programmedset_scan.go b/internal/extensionserver/server/programmedset_scan.go index f24f37f0..daa547ff 100644 --- a/internal/extensionserver/server/programmedset_scan.go +++ b/internal/extensionserver/server/programmedset_scan.go @@ -80,11 +80,9 @@ func isConnectRoute(rt *routev3.Route) bool { return false } -// isOfflineRoute reports whether a route is part of the connector offline path. -// That path produces two shapes, so both count: the CONNECT route answering the -// connector agent keeps a terse direct_response, while user-facing routes -// forward to the shared endpoint-less cluster when the branded error page is -// configured, so that the response carries the flag the offline page needs. +// isOfflineRoute reports whether a route is part of the connector offline path, +// which has two shapes: the CONNECT route keeps a terse direct_response, while +// user-facing routes forward to the endpoint-less cluster. func isOfflineRoute(rt *routev3.Route) bool { if ra := rt.GetRoute(); ra != nil { return ra.GetCluster() == offlineTunnelCluster @@ -96,10 +94,7 @@ func isOfflineRoute(rt *routev3.Route) bool { return dr.GetBody().GetInlineString() == offlineBodyMarker } -// offlineBodyMarker is the response body the connector offline path writes on -// its CONNECT route, and offlineTunnelCluster is the shared endpoint-less -// cluster its user-facing routes point at. Both duplicated here so the scanner -// needs no import dependency. +// Duplicated from the mutate package so the scanner needs no import dependency. const ( offlineBodyMarker = "Tunnel not online" offlineTunnelCluster = "datum-offline-tunnel" diff --git a/internal/extensionserver/server/server.go b/internal/extensionserver/server/server.go index ed547fd9..e59907e6 100644 --- a/internal/extensionserver/server/server.go +++ b/internal/extensionserver/server/server.go @@ -245,10 +245,9 @@ func (s *Server) PostTranslateModify( } s.markTPPsProgrammed(ctx, appliedTPPs) - // Whether the branded error page is configured at all. Both the connector - // offline path and the empty-backend path route user traffic to the shared - // endpoint-less cluster only when there is a page to serve; with no page, - // each keeps the answer it gave before. + // Both offline paths below route user traffic to an endpoint-less cluster + // only when there is a page to serve. With no page, each keeps the answer + // it gave before. brandedOffline := !s.cfg.LocalReply.Disabled && s.cfg.LocalReply.OfflineBodyHTML != "" // --- Connector family --- @@ -295,24 +294,17 @@ func (s *Server) PostTranslateModify( connRoutesSpan.End() // --- Empty backend family (#502) --- - // EG collapses a route whose backend has no ready endpoints to a bodiless - // 503 direct_response, which short-circuits before the router filter and so - // carries no UH flag for the branded offline page to match. Point those - // routes at a shared endpoint-less cluster instead, which restores the flag. // Runs after the connector family so connector-offline routes already carry - // their body and are therefore not mistaken for EG's collapse. - // - // Gated on the branded page being configured: with no offline page to - // serve, rewriting would only trade EG's deterministic 503 for Envoy's - // generic no_healthy_upstream and buy nothing. + // their body and are not mistaken for EG's own collapse. var emptyBackendCount int if brandedOffline { _, emptyBackendSpan := tr.Start(mctx, "emptybackend.routes") for _, rc := range routes { emptyBackendCount += mutate.RouteEmptyBackendsToOfflineCluster(rc) } - // Each family has its own shared sink, added only when something points - // at it, so the data-plane stats say which reason a request hit. + // Each family gets its own sink so the data-plane stats say which + // reason a request hit, and an endpoint-less cluster costs ~108 + // resident stats, so both are shared and added only when used. wanted := map[string]bool{ mutate.OfflineBackendClusterName: emptyBackendCount > 0, mutate.OfflineTunnelClusterName: offlineRtCount > 0, diff --git a/test/e2e-edge/networkservice-drained-to-zero/chainsaw-test.yaml b/test/e2e-edge/networkservice-drained-to-zero/chainsaw-test.yaml index ed860acc..fcb047e4 100644 --- a/test/e2e-edge/networkservice-drained-to-zero/chainsaw-test.yaml +++ b/test/e2e-edge/networkservice-drained-to-zero/chainsaw-test.yaml @@ -1,22 +1,13 @@ # yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json # -# The transition production hit in issue #502: a NetworkService that was -# serving loses its last member. -# -# A service resolves one member, a server answers at that member's allocated -# address, and a real request through the edge comes back 200. The claim is -# then released — the equivalent of the workload behind it going to zero — and -# the same request is sent again. -# -# The question this holds open is what changes when that happens. The service's -# own status does change. Whether anything on the HTTPProxy, on the downstream -# Gateway, or on the downstream HTTPRoute changes with it is the gap, and both -# responses are printed side by side so the difference is the record. +# A NetworkService that was serving loses its last member. The service resolves +# one member, a real request through the edge returns 200, the claim is then +# released, and the same request is sent again. Both responses are printed so +# the difference is the record. # # The member address is bound on the downstream node and served by a -# host-network pod, exactly as in ../networkservice-endpoints — there is no -# compute in this environment, so nothing answers at an allocated address on -# its own. +# host-network pod, as in ../networkservice-endpoints, because this environment +# has no compute to answer at an allocated address on its own. apiVersion: chainsaw.kyverno.io/v1alpha1 kind: Test metadata: diff --git a/test/e2e-edge/networkservice-no-endpoints/chainsaw-test.yaml b/test/e2e-edge/networkservice-no-endpoints/chainsaw-test.yaml index bf777f7c..018c6990 100644 --- a/test/e2e-edge/networkservice-no-endpoints/chainsaw-test.yaml +++ b/test/e2e-edge/networkservice-no-endpoints/chainsaw-test.yaml @@ -1,20 +1,7 @@ # yaml-language-server: $schema=https://raw.githubusercontent.com/kyverno/chainsaw/main/.schemas/json/test-chainsaw-v1alpha1.json # -# A NetworkService with no members leaves the proxy in front of it looking -# healthy while nothing can be served through it. -# -# This is the repro for issue #502. Two shapes of the same state are covered: -# -# * never populated — the selector matches nothing, so MembersResolved is -# false for NoMatchingInterfaces from the moment the service is written. -# -# * drained — a service that resolved a real member, served a real 200 -# through the edge, and then lost its member when the claim went away. -# This is the transition production hit. -# -# What is being captured in both shapes is the gap between what the platform -# reports and what a user gets: the HTTPProxy stays Accepted and Programmed, -# the downstream Service and EndpointSlice exist, and the request fails. +# A NetworkService whose selector matches nothing leaves the proxy in front of +# it Accepted and Programmed while no request can be served through it. apiVersion: chainsaw.kyverno.io/v1alpha1 kind: Test metadata: diff --git a/test/parity/scan.go b/test/parity/scan.go index bf4ca475..8b1ceb92 100644 --- a/test/parity/scan.go +++ b/test/parity/scan.go @@ -144,11 +144,9 @@ func isConnectRoute(rt *routev3.Route) bool { return false } -// isOfflineRoute reports whether a route is part of the connector offline path. -// That path produces two shapes, so both count: the CONNECT route answering the -// connector agent keeps a terse direct_response, while user-facing routes -// forward to the shared endpoint-less cluster when the branded error page is -// configured, so that the response carries the flag the offline page needs. +// isOfflineRoute reports whether a route is part of the connector offline path, +// which has two shapes: the CONNECT route keeps a terse direct_response, while +// user-facing routes forward to the endpoint-less cluster. func isOfflineRoute(rt *routev3.Route) bool { if ra := rt.GetRoute(); ra != nil { return ra.GetCluster() == offlineTunnelCluster