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 {