From a9877ec62b2dbb05d5ffde89db95243034a89972 Mon Sep 17 00:00:00 2001 From: bussyjd Date: Thu, 16 Jul 2026 20:46:51 +0400 Subject: [PATCH] fix(serviceoffercontroller): gate httpRouteAccepted on observedGeneration httpRouteAccepted ignored status.parents[].conditions[].observedGeneration, so the #767 route-acceptance gate (apply -> Get -> httpRouteAccepted before RoutePublished=True) was bypassed for updates to an already-accepted HTTPRoute: the Get right after applyObject could return the PREVIOUS generation's Accepted=True/ResolvedRefs=True, flipping RoutePublished True even though Traefik hasn't reconciled the new spec yet (and may go on to reject it). httpRouteAccepted now requires each trusted condition's observedGeneration to equal the route's metadata.generation; a stale condition is skipped, so the parent falls through to not-accepted and the existing 5s requeue re-checks. Both the main and host route accept-gates (controller.go ~852-874) go through this same function. Claude-Session: https://claude.ai/code/session_014YjPMViNrZ7zBVgUQzwEKk --- internal/serviceoffercontroller/controller.go | 21 ++++- .../serviceoffercontroller/helpers_test.go | 79 ++++++++++++++++++- internal/serviceoffercontroller/render.go | 26 +++++- .../route_accept_test.go | 51 ++++++++++++ 4 files changed, 173 insertions(+), 4 deletions(-) diff --git a/internal/serviceoffercontroller/controller.go b/internal/serviceoffercontroller/controller.go index 4d836045..a17c067b 100644 --- a/internal/serviceoffercontroller/controller.go +++ b/internal/serviceoffercontroller/controller.go @@ -1678,6 +1678,10 @@ func httpRouteAccepted(route *unstructured.Unstructured) bool { if err != nil || !found { return false } + // metadata.generation defaults to 0 when absent (e.g. hand-built test + // fixtures), matching a condition's absent observedGeneration below so + // older/incomplete fixtures still compare equal. + routeGeneration, _, _ := unstructured.NestedInt64(route.Object, "metadata", "generation") for _, parent := range parents { parentMap, ok := parent.(map[string]any) if !ok { @@ -1687,13 +1691,28 @@ func httpRouteAccepted(route *unstructured.Unstructured) bool { if !ok { continue } + // Both default to false: each must be affirmatively reported True at + // the CURRENT generation. Defaulting resolvedRefs to true would be + // fail-open — a stale or not-yet-emitted ResolvedRefs (behind + // metadata.generation, or False from a rejected prior spec) would be + // skipped by the observedGeneration guard below and leave the true + // default intact, publishing a route Traefik hasn't resolved. accepted := false - resolvedRefs := true + resolvedRefs := false for _, condition := range conditions { condMap, ok := condition.(map[string]any) if !ok { continue } + // A condition Traefik hasn't reconciled against the current spec + // yet (observedGeneration behind metadata.generation) still + // reflects the PREVIOUS generation's verdict — don't trust it, + // or an update to an already-accepted route would flip + // RoutePublished True before Traefik has actually checked it. + observedGeneration, _, _ := unstructured.NestedInt64(condMap, "observedGeneration") + if observedGeneration != routeGeneration { + continue + } condType, _ := condMap["type"].(string) condStatus, _ := condMap["status"].(string) switch condType { diff --git a/internal/serviceoffercontroller/helpers_test.go b/internal/serviceoffercontroller/helpers_test.go index f05bc842..4c6a171d 100644 --- a/internal/serviceoffercontroller/helpers_test.go +++ b/internal/serviceoffercontroller/helpers_test.go @@ -139,7 +139,7 @@ func TestHTTPRouteAccepted(t *testing.T) { want: false, }, { - name: "only Accepted condition (ResolvedRefs implicitly True by default)", + name: "only Accepted condition, ResolvedRefs absent — not yet accepted (fail closed)", route: &unstructured.Unstructured{Object: map[string]any{ "status": map[string]any{ "parents": []any{ @@ -151,7 +151,7 @@ func TestHTTPRouteAccepted(t *testing.T) { }, }, }}, - want: true, // function defaults resolvedRefs to true when absent + want: false, // ResolvedRefs must be affirmatively True; absent is not trusted }, { name: "multiple parents: first bad, second good", @@ -185,6 +185,81 @@ func TestHTTPRouteAccepted(t *testing.T) { }}, want: false, }, + { + // #767 route-acceptance gate: an UPDATE to an already-accepted + // route must not be trusted until Traefik reconciles the new + // spec. Status still reflects generation 1's verdict while the + // route is now at generation 2. + name: "Accepted=True but observedGeneration behind metadata.generation", + route: &unstructured.Unstructured{Object: map[string]any{ + "metadata": map[string]any{"generation": int64(2)}, + "status": map[string]any{ + "parents": []any{ + map[string]any{ + "conditions": []any{ + map[string]any{"type": "Accepted", "status": "True", "observedGeneration": int64(1)}, + map[string]any{"type": "ResolvedRefs", "status": "True", "observedGeneration": int64(1)}, + }, + }, + }, + }, + }}, + want: false, + }, + { + name: "Accepted=True with observedGeneration matching metadata.generation", + route: &unstructured.Unstructured{Object: map[string]any{ + "metadata": map[string]any{"generation": int64(2)}, + "status": map[string]any{ + "parents": []any{ + map[string]any{ + "conditions": []any{ + map[string]any{"type": "Accepted", "status": "True", "observedGeneration": int64(2)}, + map[string]any{"type": "ResolvedRefs", "status": "True", "observedGeneration": int64(2)}, + }, + }, + }, + }, + }}, + want: true, + }, + { + // Fail-closed: Accepted is current+True but ResolvedRefs is + // stale (behind metadata.generation) and False. The stale + // ResolvedRefs must NOT be ignored-as-true — the route has not + // been resolved against the current spec. + name: "Accepted current True but ResolvedRefs stale False", + route: &unstructured.Unstructured{Object: map[string]any{ + "metadata": map[string]any{"generation": int64(2)}, + "status": map[string]any{ + "parents": []any{ + map[string]any{ + "conditions": []any{ + map[string]any{"type": "Accepted", "status": "True", "observedGeneration": int64(2)}, + map[string]any{"type": "ResolvedRefs", "status": "False", "observedGeneration": int64(1)}, + }, + }, + }, + }, + }}, + want: false, + }, + { + name: "Accepted current True but ResolvedRefs absent", + route: &unstructured.Unstructured{Object: map[string]any{ + "metadata": map[string]any{"generation": int64(2)}, + "status": map[string]any{ + "parents": []any{ + map[string]any{ + "conditions": []any{ + map[string]any{"type": "Accepted", "status": "True", "observedGeneration": int64(2)}, + }, + }, + }, + }, + }}, + want: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/serviceoffercontroller/render.go b/internal/serviceoffercontroller/render.go index e8b810b5..cd05d294 100644 --- a/internal/serviceoffercontroller/render.go +++ b/internal/serviceoffercontroller/render.go @@ -940,8 +940,15 @@ func hostRouteRules(offer *monetizeapi.ServiceOffer, exactTo, exactToShared func exactTo("/.well-known/agent-registration.json", "agent-registration.json"), } if offer.IsAgent() { + // The chat page holds a hot session key and signs USDC transfers — + // frame-ancestors 'self' stops it being clickjacked into a + // cross-origin iframe (Connect/Fund/Withdraw/Send). The offer's own + // landing page embeds it same-origin (TestOfferLandingChatEmbed), so + // that legitimate embed still works. + chatRule := exactTo("/chat", "chat.html") + addResponseHeader(chatRule, "Content-Security-Policy", "frame-ancestors 'self'") rules = append(rules, - exactTo("/chat", "chat.html"), + chatRule, exactToShared("/chat-vendor.js", "chat-vendor.js"), ) } @@ -956,6 +963,23 @@ func hostRouteRules(offer *monetizeapi.ServiceOffer, exactTo, exactToShared func }) } +// addResponseHeader appends a Set entry to a rule's existing +// ResponseHeaderModifier filter (every exactTo rule has exactly one, from +// cacheFilter) — Gateway API's core filters are unspecified/implementation +// -defined if repeated within one rule, so extra headers merge into the +// existing filter instead of adding a second one. +func addResponseHeader(rule map[string]any, name, value string) { + for _, f := range rule["filters"].([]any) { + fm := f.(map[string]any) + if fm["type"] != "ResponseHeaderModifier" { + continue + } + mod := fm["responseHeaderModifier"].(map[string]any) + mod["set"] = append(mod["set"].([]any), map[string]any{"name": name, "value": value}) + return + } +} + // sharedOriginRule is the /services/ PathPrefix rule → verifier, // with the protection middleware attached when spec.limits is set. func sharedOriginRule(offer *monetizeapi.ServiceOffer) map[string]any { diff --git a/internal/serviceoffercontroller/route_accept_test.go b/internal/serviceoffercontroller/route_accept_test.go index ba55e747..155f11e8 100644 --- a/internal/serviceoffercontroller/route_accept_test.go +++ b/internal/serviceoffercontroller/route_accept_test.go @@ -103,6 +103,57 @@ func acceptedHTTPRoute(namespace, name string) *unstructured.Unstructured { }} } +// staleGenerationHTTPRoute seeds an HTTPRoute at metadata.generation 2 whose +// status.parents conditions are Accepted=True / ResolvedRefs=True but still +// carry observedGeneration 1 — Traefik reported on the PREVIOUS spec and +// hasn't reconciled the update yet. +func staleGenerationHTTPRoute(namespace, name string) *unstructured.Unstructured { + return &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "gateway.networking.k8s.io/v1", + "kind": "HTTPRoute", + "metadata": map[string]any{ + "name": name, + "namespace": namespace, + "generation": int64(2), + }, + "status": map[string]any{ + "parents": []any{ + map[string]any{ + "conditions": []any{ + map[string]any{"type": "Accepted", "status": "True", "observedGeneration": int64(1)}, + map[string]any{"type": "ResolvedRefs", "status": "True", "observedGeneration": int64(1)}, + }, + }, + }, + }, + }} +} + +func TestReconcileRoute_WaitsWhenAcceptanceIsStale(t *testing.T) { + offer := readyOffer("svc") + offer.Namespace = "stalegen" + dynClient := fake.NewSimpleDynamicClientWithCustomListKinds( + runtime.NewScheme(), + routeListKinds(), + // applyObject's fake-SSA reactor merges the applied object over this + // fixture without touching .status, so the route Get right after + // apply still returns this stale generation/status pairing. + staleGenerationHTTPRoute(offer.Namespace, childName(offer.Name)), + ) + c := controllerForRouteTest(dynClient) + status := &monetizeapi.ServiceOfferStatus{} + + if err := c.reconcileRoute(context.Background(), status, offer); err != nil { + t.Fatalf("reconcileRoute: %v", err) + } + if isConditionTrue(*status, "RoutePublished") { + t.Fatalf("RoutePublished should stay False when Traefik hasn't reconciled the current generation: %+v", status.Conditions) + } + if reason := conditionReason(status, "RoutePublished"); reason != "WaitingForTraefikAcceptance" { + t.Fatalf("RoutePublished reason = %q, want WaitingForTraefikAcceptance", reason) + } +} + func conditionReason(status *monetizeapi.ServiceOfferStatus, conditionType string) string { for _, c := range status.Conditions { if c.Type == conditionType {