From 23b577f055a6fc086183f7b8da53a3ff28b02596 Mon Sep 17 00:00:00 2001 From: Marco Ma Date: Wed, 10 Jun 2026 12:14:16 +0800 Subject: [PATCH 1/2] fix(gateway): correct BackendRef matching and canary header build MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes in pkg/trafficrouting/network/gateway/gateway.go: * getServiceBackendRef: per the Gateway API spec, BackendObjectReference.Kind defaults to "Service" when nil. The previous check `ref.Kind != nil && *ref.Kind == "Service"` skipped refs without an explicit Kind, so HTTPRoutes emitted by controllers that omit the field (e.g. Envoy Gateway) were never recognised as stable/canary backends and the rollout silently failed to inject canary traffic. Treat nil Kind as Service. Also return (-1, nil) on miss instead of (0, nil) to remove the ambiguity between "not found" and "found at index 0". * setServiceBackendRef: symmetric fix — refuse only when an explicit Kind other than Service is set, so canary BackendRefs derived from a stable ref with nil Kind can be written back. Replace the manual slice rebuild with an in-place assignment. * buildCanaryHeaderHttpRoutes: the inner loop indexed `matches[k]` while `k` ranged over `nonPathMatches`. The two slices have different lengths whenever a path-bearing match is not the first element, so headers and query params from the wrong match were grafted onto canary rules. Index `nonPathMatches[k]` instead. * buildCanaryHeaderHttpRoutes: the shallow copy `canaryRuleMatchBase := *canaryRuleMatch` shares the underlying arrays of Headers/QueryParams with the original rule. Subsequent appends could mutate the original HTTPRoute when its slices had spare capacity. Clone the slices before extending; preserve nil-ness to keep output identical to the previous implementation when nothing is appended. * EnsureRoutes: surface the error from intstr.GetScaledValueFromIntOrPercent rather than silently coercing malformed traffic strings to 0 %. * EnsureRoutes / Finalise: pass the inbound ctx to client Get/Update inside the retry closure instead of context.TODO(), so cancellation and deadlines propagate. Tests in pkg/trafficrouting/network/gateway/gateway_test.go: * Added regression cases covering BackendRefs with nil Kind across the weight, header, and finalise code paths. * Added a case where matches mix path and non-path entries so that pathMatches and nonPathMatches diverge in length, exercising the index-correctness fix. * Added a clean-state weight canary case; the existing weight-20 case fed in the final 80/20 shape and only proved idempotence. * Added TestInitialize, TestEnsureRoutesAndFinalise, and TestEnsureRoutesInvalidTraffic to drive the controller end-to-end against a fake client, covering happy path, idempotence, finalise, rollback, and invalid-input rejection. Signed-off-by: Marco Ma --- pkg/trafficrouting/network/gateway/gateway.go | 63 +- .../network/gateway/gateway_test.go | 618 ++++++++++++++++++ 2 files changed, 658 insertions(+), 23 deletions(-) diff --git a/pkg/trafficrouting/network/gateway/gateway.go b/pkg/trafficrouting/network/gateway/gateway.go index 46cd6314..3947d796 100644 --- a/pkg/trafficrouting/network/gateway/gateway.go +++ b/pkg/trafficrouting/network/gateway/gateway.go @@ -15,6 +15,7 @@ package gateway import ( "context" + "fmt" "reflect" "k8s.io/apimachinery/pkg/api/errors" @@ -63,7 +64,10 @@ func (r *gatewayController) EnsureRoutes(ctx context.Context, strategy *v1beta1. var weight *int32 if strategy.Traffic != nil { is := intstr.FromString(*strategy.Traffic) - weightInt, _ := intstr.GetScaledValueFromIntOrPercent(&is, 100, true) + weightInt, err := intstr.GetScaledValueFromIntOrPercent(&is, 100, true) + if err != nil { + return false, fmt.Errorf("invalid traffic %q for %s: %w", *strategy.Traffic, r.conf.Key, err) + } weight = utilpointer.Int32(int32(weightInt)) } matches := strategy.Matches @@ -81,12 +85,12 @@ func (r *gatewayController) EnsureRoutes(ctx context.Context, strategy *v1beta1. // set route routeClone := &gatewayv1beta1.HTTPRoute{} if err = retry.RetryOnConflict(retry.DefaultBackoff, func() error { - if err = r.Client.Get(context.TODO(), types.NamespacedName{Namespace: httpRoute.Namespace, Name: httpRoute.Name}, routeClone); err != nil { + if err = r.Client.Get(ctx, types.NamespacedName{Namespace: httpRoute.Namespace, Name: httpRoute.Name}, routeClone); err != nil { klog.Errorf("error getting updated httpRoute(%s/%s) from client", httpRoute.Namespace, httpRoute.Name) return err } routeClone.Spec.Rules = desiredRule - return r.Client.Update(context.TODO(), routeClone) + return r.Client.Update(ctx, routeClone) }); err != nil { klog.Errorf("update %s httpRoute(%s) failed: %s", r.conf.Key, httpRoute.Name, err.Error()) return false, err @@ -112,12 +116,12 @@ func (r *gatewayController) Finalise(ctx context.Context) (bool, error) { } routeClone := &gatewayv1beta1.HTTPRoute{} if err = retry.RetryOnConflict(retry.DefaultBackoff, func() error { - if err = r.Client.Get(context.TODO(), types.NamespacedName{Namespace: httpRoute.Namespace, Name: httpRoute.Name}, routeClone); err != nil { + if err = r.Client.Get(ctx, types.NamespacedName{Namespace: httpRoute.Namespace, Name: httpRoute.Name}, routeClone); err != nil { klog.Errorf("error getting updated httpRoute(%s/%s) from client", httpRoute.Namespace, httpRoute.Name) return err } routeClone.Spec.Rules = desiredRule - return r.Client.Update(context.TODO(), routeClone) + return r.Client.Update(ctx, routeClone) }); err != nil { klog.Errorf("update %s httpRoute(%s) failed: %s", r.conf.Key, httpRoute.Name, err.Error()) return false, err @@ -194,11 +198,21 @@ func (r *gatewayController) buildCanaryHeaderHttpRoutes(rules []gatewayv1beta1.H canaryRuleMatch := &canaryRule.Matches[j] for k := range nonPathMatches { canaryRuleMatchBase := *canaryRuleMatch - if len(matches[k].Headers) > 0 { - canaryRuleMatchBase.Headers = append(canaryRuleMatchBase.Headers, matches[k].Headers...) + // Clone Headers/QueryParams to avoid mutating the original + // rule's underlying arrays via append. Preserve nil-ness so + // the result is byte-for-byte identical to the previous + // implementation when there is nothing to extend. + if canaryRuleMatch.Headers != nil { + canaryRuleMatchBase.Headers = append([]gatewayv1beta1.HTTPHeaderMatch{}, canaryRuleMatch.Headers...) + } + if canaryRuleMatch.QueryParams != nil { + canaryRuleMatchBase.QueryParams = append([]gatewayv1beta1.HTTPQueryParamMatch{}, canaryRuleMatch.QueryParams...) + } + if len(nonPathMatches[k].Headers) > 0 { + canaryRuleMatchBase.Headers = append(canaryRuleMatchBase.Headers, nonPathMatches[k].Headers...) } - if len(matches[k].QueryParams) > 0 { - canaryRuleMatchBase.QueryParams = append(canaryRuleMatchBase.QueryParams, matches[k].QueryParams...) + if len(nonPathMatches[k].QueryParams) > 0 { + canaryRuleMatchBase.QueryParams = append(canaryRuleMatchBase.QueryParams, nonPathMatches[k].QueryParams...) } newMatches = append(newMatches, canaryRuleMatchBase) } @@ -213,7 +227,12 @@ func (r *gatewayController) buildCanaryHeaderHttpRoutes(rules []gatewayv1beta1.H func (r *gatewayController) buildCanaryWeightHttpRoutes(rules []gatewayv1beta1.HTTPRouteRule, weight *int32) []gatewayv1beta1.HTTPRouteRule { var desired []gatewayv1beta1.HTTPRouteRule for i := range rules { - rule := rules[i] + // DeepCopy the rule so subsequent in-place mutations of BackendRefs + // (via setServiceBackendRef) cannot alias the input slice's + // underlying array. Without this, the diff in EnsureRoutes would see + // desired == current and skip the Update on every step beyond the + // first. + rule := *rules[i].DeepCopy() _, stableRef := getServiceBackendRef(rule, r.conf.StableService) if stableRef == nil { desired = append(desired, rule) @@ -241,19 +260,25 @@ func generateCanaryWeight(canaryPercent int32) (stableWeight int32, canaryWeight return stableWeight, canaryWeight } -// int indicates ref index +// getServiceBackendRef returns the index and a copy of the BackendRef in rule +// that targets the named Service. Per the Gateway API spec, +// BackendObjectReference.Kind defaults to "Service" when nil, so refs without +// an explicit Kind are treated as Service references. +// Returns (-1, nil) when no matching ref is found. func getServiceBackendRef(rule gatewayv1beta1.HTTPRouteRule, serviceName string) (int, *gatewayv1beta1.HTTPBackendRef) { for i := range rule.BackendRefs { ref := rule.BackendRefs[i] - if ref.Kind != nil && *ref.Kind == "Service" && string(ref.Name) == serviceName { + if (ref.Kind == nil || *ref.Kind == "Service") && string(ref.Name) == serviceName { return i, &ref } } - return 0, nil + return -1, nil } func setServiceBackendRef(rule *gatewayv1beta1.HTTPRouteRule, ref gatewayv1beta1.HTTPBackendRef) { - if ref.Kind == nil || *ref.Kind != "Service" { + // Per Gateway API spec, Kind defaults to "Service" when nil; only refuse + // when an explicit Kind other than Service is set. + if ref.Kind != nil && *ref.Kind != "Service" { return } index, currentRef := getServiceBackendRef(*rule, string(ref.Name)) @@ -261,15 +286,7 @@ func setServiceBackendRef(rule *gatewayv1beta1.HTTPRouteRule, ref gatewayv1beta1 rule.BackendRefs = append(rule.BackendRefs, ref) return } - oldRefs := rule.BackendRefs - rule.BackendRefs = []gatewayv1beta1.HTTPBackendRef{} - for i := range oldRefs { - if i == index { - rule.BackendRefs = append(rule.BackendRefs, ref) - } else { - rule.BackendRefs = append(rule.BackendRefs, oldRefs[i]) - } - } + rule.BackendRefs[index] = ref } func filterOutServiceBackendRef(rule *gatewayv1beta1.HTTPRouteRule, serviceName string) { diff --git a/pkg/trafficrouting/network/gateway/gateway_test.go b/pkg/trafficrouting/network/gateway/gateway_test.go index df784f7a..47d91e2c 100644 --- a/pkg/trafficrouting/network/gateway/gateway_test.go +++ b/pkg/trafficrouting/network/gateway/gateway_test.go @@ -14,10 +14,15 @@ limitations under the License. package gateway import ( + "context" "reflect" "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" utilpointer "k8s.io/utils/pointer" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" gatewayv1beta1 "sigs.k8s.io/gateway-api/apis/v1beta1" "github.com/openkruise/rollouts/api/v1beta1" @@ -934,6 +939,402 @@ func TestBuildDesiredHTTPRoute(t *testing.T) { return rules }, }, + // Bug #17/#19 regression: BackendRef without an explicit Kind must be + // treated as a Service per the Gateway API spec (Kind defaults to + // "Service" when nil). Some controllers (e.g. Envoy Gateway) emit + // HTTPRoute objects with Kind unset. + { + name: "kind nil: weight canary", + getRouteRules: func() []gatewayv1beta1.HTTPRouteRule { + rules := routeDemo.DeepCopy().Spec.Rules + // Drop Kind from rules[1] (store-svc) and rules[3] (store-svc). + rules[1].BackendRefs[0].Kind = nil + rules[3].BackendRefs[0].Kind = nil + return rules + }, + getRoutes: func() (*int32, []v1beta1.HttpRouteMatch) { + return utilpointer.Int32(20), nil + }, + desiredRules: func() []gatewayv1beta1.HTTPRouteRule { + rules := routeDemo.DeepCopy().Spec.Rules + // Stable refs keep their original (nil) Kind; canary refs + // inherit it because they are derived via DeepCopy from the + // stable ref then renamed. + rules[1].BackendRefs = []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc", + Port: &portNum, + }, + Weight: utilpointer.Int32(80), + }, + }, + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc-canary", + Port: &portNum, + }, + Weight: utilpointer.Int32(20), + }, + }, + } + rules[3].BackendRefs = []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc", + Port: &portNum, + }, + Weight: utilpointer.Int32(80), + }, + }, + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc-canary", + Port: &portNum, + }, + Weight: utilpointer.Int32(20), + }, + }, + } + return rules + }, + }, + { + name: "kind nil: header canary", + getRouteRules: func() []gatewayv1beta1.HTTPRouteRule { + rules := routeDemo.DeepCopy().Spec.Rules + rules[1].BackendRefs[0].Kind = nil + rules[3].BackendRefs[0].Kind = nil + return rules + }, + getRoutes: func() (*int32, []v1beta1.HttpRouteMatch) { + iType := gatewayv1beta1.HeaderMatchRegularExpression + return nil, []v1beta1.HttpRouteMatch{ + { + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + { + Name: "user_id", + Value: "123*", + Type: &iType, + }, + }, + }, + } + }, + desiredRules: func() []gatewayv1beta1.HTTPRouteRule { + rules := routeDemo.DeepCopy().Spec.Rules + rules[1].BackendRefs[0].Kind = nil + rules[3].BackendRefs[0].Kind = nil + iType := gatewayv1beta1.HeaderMatchRegularExpression + // Canary rule for /store + /v2/store (rules[1]). + rules = append(rules, gatewayv1beta1.HTTPRouteRule{ + Matches: []gatewayv1beta1.HTTPRouteMatch{ + { + Path: &gatewayv1beta1.HTTPPathMatch{ + Value: utilpointer.String("/store"), + }, + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "version", Value: "v2"}, + {Name: "user_id", Value: "123*", Type: &iType}, + }, + }, + { + Path: &gatewayv1beta1.HTTPPathMatch{ + Value: utilpointer.String("/v2/store"), + }, + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "user_id", Value: "123*", Type: &iType}, + }, + }, + }, + BackendRefs: []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc-canary", + Port: &portNum, + }, + }, + }, + }, + }) + // Canary rule for /storage (rules[3]). + rules = append(rules, gatewayv1beta1.HTTPRouteRule{ + Matches: []gatewayv1beta1.HTTPRouteMatch{ + { + Path: &gatewayv1beta1.HTTPPathMatch{ + Value: utilpointer.String("/storage"), + }, + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "user_id", Value: "123*", Type: &iType}, + }, + }, + }, + BackendRefs: []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc-canary", + Port: &portNum, + }, + }, + }, + }, + }) + return rules + }, + }, + { + name: "kind nil: finalise restores stable", + getRouteRules: func() []gatewayv1beta1.HTTPRouteRule { + // Mid-rollout shape: weight has been split to 80/20, stable + // ref has nil Kind, canary ref also has nil Kind. + rules := routeDemo.DeepCopy().Spec.Rules + rules[1].BackendRefs = []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc", + Port: &portNum, + }, + Weight: utilpointer.Int32(80), + }, + }, + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc-canary", + Port: &portNum, + }, + Weight: utilpointer.Int32(20), + }, + }, + } + rules[3].BackendRefs = []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc", + Port: &portNum, + }, + Weight: utilpointer.Int32(80), + }, + }, + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc-canary", + Port: &portNum, + }, + Weight: utilpointer.Int32(20), + }, + }, + } + return rules + }, + getRoutes: func() (*int32, []v1beta1.HttpRouteMatch) { + return utilpointer.Int32(-1), nil + }, + desiredRules: func() []gatewayv1beta1.HTTPRouteRule { + // Canary refs gone; stable refs restored to weight=1, with + // Kind still nil. + rules := routeDemo.DeepCopy().Spec.Rules + rules[1].BackendRefs = []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc", + Port: &portNum, + }, + Weight: utilpointer.Int32(1), + }, + }, + } + rules[3].BackendRefs = []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Name: "store-svc", + Port: &portNum, + }, + Weight: utilpointer.Int32(1), + }, + }, + } + return rules + }, + }, + // Bug #13 regression: nonPathMatches must be indexed by their own + // position, not the position they had in the original matches slice. + // Place a path match first and a non-path match second so the indices + // diverge: pathMatches=[matches[0]], nonPathMatches=[matches[1]]. + { + name: "matches mixed: path first, non-path second", + getRouteRules: func() []gatewayv1beta1.HTTPRouteRule { + return routeDemo.DeepCopy().Spec.Rules + }, + getRoutes: func() (*int32, []v1beta1.HttpRouteMatch) { + iHeaderType := gatewayv1beta1.HeaderMatchRegularExpression + return nil, []v1beta1.HttpRouteMatch{ + { + // Path match — consumed once, attached to the first + // stable rule (/store + /v2/store). + Path: &gatewayv1beta1.HTTPPathMatch{ + Value: utilpointer.String("/storage/v2"), + }, + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "from-path-match", Value: "PATH"}, + }, + }, + { + // Non-path match — must contribute to every stable + // rule. Its Headers must NOT be confused with the + // path match's Headers above. + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "from-nonpath-match", Value: "NONPATH", Type: &iHeaderType}, + }, + }, + } + }, + desiredRules: func() []gatewayv1beta1.HTTPRouteRule { + rules := routeDemo.DeepCopy().Spec.Rules + iHeaderType := gatewayv1beta1.HeaderMatchRegularExpression + // First stable rule (rules[1] for /store+/v2/store) gets: + // * the standalone path match (single entry, from pathMatches) + // * each existing match × each nonPathMatch + rules = append(rules, gatewayv1beta1.HTTPRouteRule{ + Matches: []gatewayv1beta1.HTTPRouteMatch{ + { + Path: &gatewayv1beta1.HTTPPathMatch{ + Value: utilpointer.String("/storage/v2"), + }, + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "from-path-match", Value: "PATH"}, + }, + }, + { + Path: &gatewayv1beta1.HTTPPathMatch{ + Value: utilpointer.String("/store"), + }, + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "version", Value: "v2"}, + {Name: "from-nonpath-match", Value: "NONPATH", Type: &iHeaderType}, + }, + }, + { + Path: &gatewayv1beta1.HTTPPathMatch{ + Value: utilpointer.String("/v2/store"), + }, + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "from-nonpath-match", Value: "NONPATH", Type: &iHeaderType}, + }, + }, + }, + BackendRefs: []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Kind: &kindSvc, + Name: "store-svc-canary", + Port: &portNum, + }, + }, + }, + }, + }) + // Second stable rule (rules[3] for /storage) gets only the + // nonPath match (pathMatches were consumed by the first rule). + rules = append(rules, gatewayv1beta1.HTTPRouteRule{ + Matches: []gatewayv1beta1.HTTPRouteMatch{ + { + Path: &gatewayv1beta1.HTTPPathMatch{ + Value: utilpointer.String("/storage"), + }, + Headers: []gatewayv1beta1.HTTPHeaderMatch{ + {Name: "from-nonpath-match", Value: "NONPATH", Type: &iHeaderType}, + }, + }, + }, + BackendRefs: []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Kind: &kindSvc, + Name: "store-svc-canary", + Port: &portNum, + }, + }, + }, + }, + }) + return rules + }, + }, + // Test #23: from the original routeDemo (no canary injected yet) to + // canary@20. The existing "canary weight: 20" case feeds in the final + // 80/20 shape and only proves idempotence; this case proves the + // transformation from clean state. + { + name: "canary weight: 20 from clean state", + getRouteRules: func() []gatewayv1beta1.HTTPRouteRule { + return routeDemo.DeepCopy().Spec.Rules + }, + getRoutes: func() (*int32, []v1beta1.HttpRouteMatch) { + return utilpointer.Int32(20), nil + }, + desiredRules: func() []gatewayv1beta1.HTTPRouteRule { + rules := routeDemo.DeepCopy().Spec.Rules + rules[1].BackendRefs = []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Kind: &kindSvc, + Name: "store-svc", + Port: &portNum, + }, + Weight: utilpointer.Int32(80), + }, + }, + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Kind: &kindSvc, + Name: "store-svc-canary", + Port: &portNum, + }, + Weight: utilpointer.Int32(20), + }, + }, + } + rules[3].BackendRefs = []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Kind: &kindSvc, + Name: "store-svc", + Port: &portNum, + }, + Weight: utilpointer.Int32(80), + }, + }, + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Kind: &kindSvc, + Name: "store-svc-canary", + Port: &portNum, + }, + Weight: utilpointer.Int32(20), + }, + }, + } + return rules + }, + }, } conf := Config{ @@ -954,3 +1355,220 @@ func TestBuildDesiredHTTPRoute(t *testing.T) { }) } } + +// newGatewayTestScheme registers the types needed by the fake client for the +// end-to-end Initialize/EnsureRoutes/Finalise tests. +func newGatewayTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + s := runtime.NewScheme() + if err := gatewayv1beta1.AddToScheme(s); err != nil { + t.Fatalf("add gatewayv1beta1 to scheme: %v", err) + } + return s +} + +func newTestHTTPRoute(name, namespace, stableSvc string) *gatewayv1beta1.HTTPRoute { + port := gatewayv1beta1.PortNumber(8080) + kind := gatewayv1beta1.Kind("Service") + return &gatewayv1beta1.HTTPRoute{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Spec: gatewayv1beta1.HTTPRouteSpec{ + Rules: []gatewayv1beta1.HTTPRouteRule{ + { + Matches: []gatewayv1beta1.HTTPRouteMatch{ + {Path: &gatewayv1beta1.HTTPPathMatch{Value: utilpointer.String("/api")}}, + }, + BackendRefs: []gatewayv1beta1.HTTPBackendRef{ + { + BackendRef: gatewayv1beta1.BackendRef{ + BackendObjectReference: gatewayv1beta1.BackendObjectReference{ + Kind: &kind, + Name: gatewayv1beta1.ObjectName(stableSvc), + Port: &port, + }, + }, + }, + }, + }, + }, + }, + } +} + +// TestInitialize covers Initialize against a real fake client: the route must +// exist, otherwise an error propagates up. +func TestInitialize(t *testing.T) { + const ns = "default" + const routeName = "demo-route" + scheme := newGatewayTestScheme(t) + + t.Run("route exists", func(t *testing.T) { + fakeCli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(newTestHTTPRoute(routeName, ns, "store-svc")).Build() + ctrl, err := NewGatewayTrafficRouting(fakeCli, Config{ + Key: "rollout/demo", + Namespace: ns, + CanaryService: "store-svc-canary", + StableService: "store-svc", + TrafficConf: &v1beta1.GatewayTrafficRouting{HTTPRouteName: utilpointer.String(routeName)}, + }) + if err != nil { + t.Fatalf("NewGatewayTrafficRouting: %v", err) + } + if err := ctrl.Initialize(context.TODO()); err != nil { + t.Fatalf("Initialize: %v", err) + } + }) + + t.Run("route missing returns error", func(t *testing.T) { + fakeCli := fake.NewClientBuilder().WithScheme(scheme).Build() + ctrl, err := NewGatewayTrafficRouting(fakeCli, Config{ + Key: "rollout/demo", + Namespace: ns, + CanaryService: "store-svc-canary", + StableService: "store-svc", + TrafficConf: &v1beta1.GatewayTrafficRouting{HTTPRouteName: utilpointer.String(routeName)}, + }) + if err != nil { + t.Fatalf("NewGatewayTrafficRouting: %v", err) + } + if err := ctrl.Initialize(context.TODO()); err == nil { + t.Fatalf("expected Initialize to fail with NotFound") + } + }) +} + +// TestEnsureRoutesAndFinalise drives a weight-based canary through +// EnsureRoutes and then rolls it back via Finalise on the same fake client, +// verifying the persisted HTTPRoute at each step. +func TestEnsureRoutesAndFinalise(t *testing.T) { + const ns = "default" + const routeName = "demo-route" + scheme := newGatewayTestScheme(t) + original := newTestHTTPRoute(routeName, ns, "store-svc") + fakeCli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(original.DeepCopy()).Build() + ctrl, err := NewGatewayTrafficRouting(fakeCli, Config{ + Key: "rollout/demo", + Namespace: ns, + CanaryService: "store-svc-canary", + StableService: "store-svc", + TrafficConf: &v1beta1.GatewayTrafficRouting{HTTPRouteName: utilpointer.String(routeName)}, + }) + if err != nil { + t.Fatalf("NewGatewayTrafficRouting: %v", err) + } + + // First EnsureRoutes: should write canary backend with weight 20. + traffic := "20%" + strategy := &v1beta1.TrafficRoutingStrategy{Traffic: &traffic} + done, err := ctrl.EnsureRoutes(context.TODO(), strategy) + if err != nil { + t.Fatalf("EnsureRoutes: %v", err) + } + if done { + t.Fatalf("EnsureRoutes returned done=true on first apply, expected false") + } + + got := &gatewayv1beta1.HTTPRoute{} + if err := fakeCli.Get(context.TODO(), client.ObjectKey{Namespace: ns, Name: routeName}, got); err != nil { + t.Fatalf("Get after EnsureRoutes: %v", err) + } + if len(got.Spec.Rules[0].BackendRefs) != 2 { + t.Fatalf("expected 2 backend refs after EnsureRoutes, got %d (%s)", len(got.Spec.Rules[0].BackendRefs), util.DumpJSON(got.Spec.Rules)) + } + if string(got.Spec.Rules[0].BackendRefs[0].Name) != "store-svc" || *got.Spec.Rules[0].BackendRefs[0].Weight != 80 { + t.Fatalf("expected stable@80, got %s", util.DumpJSON(got.Spec.Rules[0].BackendRefs[0])) + } + if string(got.Spec.Rules[0].BackendRefs[1].Name) != "store-svc-canary" || *got.Spec.Rules[0].BackendRefs[1].Weight != 20 { + t.Fatalf("expected canary@20, got %s", util.DumpJSON(got.Spec.Rules[0].BackendRefs[1])) + } + + // Second EnsureRoutes with the same input: should be idempotent → done=true, no write. + done, err = ctrl.EnsureRoutes(context.TODO(), strategy) + if err != nil { + t.Fatalf("EnsureRoutes (idempotent): %v", err) + } + if !done { + t.Fatalf("EnsureRoutes idempotent call returned done=false, expected true") + } + + // Step 2: bump to 40% — this is the e2e step that broke when the + // in-place update aliased the input slice with the desired slice and + // caused EnsureRoutes to short-circuit on a stale "already done" check. + traffic2 := "40%" + strategy2 := &v1beta1.TrafficRoutingStrategy{Traffic: &traffic2} + done, err = ctrl.EnsureRoutes(context.TODO(), strategy2) + if err != nil { + t.Fatalf("EnsureRoutes (step 2): %v", err) + } + if done { + t.Fatalf("EnsureRoutes returned done=true when bumping 20%%->40%%, expected false") + } + if err := fakeCli.Get(context.TODO(), client.ObjectKey{Namespace: ns, Name: routeName}, got); err != nil { + t.Fatalf("Get after step 2: %v", err) + } + if len(got.Spec.Rules[0].BackendRefs) != 2 { + t.Fatalf("expected 2 backend refs after step 2, got %d", len(got.Spec.Rules[0].BackendRefs)) + } + if w := got.Spec.Rules[0].BackendRefs[0].Weight; w == nil || *w != 60 { + t.Fatalf("expected stable@60 after step 2, got %v", w) + } + if w := got.Spec.Rules[0].BackendRefs[1].Weight; w == nil || *w != 40 { + t.Fatalf("expected canary@40 after step 2, got %v", w) + } + + // Finalise: should drop canary ref and reset stable weight to 1. + modified, err := ctrl.Finalise(context.TODO()) + if err != nil { + t.Fatalf("Finalise: %v", err) + } + if !modified { + t.Fatalf("Finalise returned modified=false on a route still carrying canary, expected true") + } + if err := fakeCli.Get(context.TODO(), client.ObjectKey{Namespace: ns, Name: routeName}, got); err != nil { + t.Fatalf("Get after Finalise: %v", err) + } + if len(got.Spec.Rules[0].BackendRefs) != 1 { + t.Fatalf("expected 1 backend ref after Finalise, got %d", len(got.Spec.Rules[0].BackendRefs)) + } + if string(got.Spec.Rules[0].BackendRefs[0].Name) != "store-svc" { + t.Fatalf("expected stable ref to remain, got %s", got.Spec.Rules[0].BackendRefs[0].Name) + } + if got.Spec.Rules[0].BackendRefs[0].Weight == nil || *got.Spec.Rules[0].BackendRefs[0].Weight != 1 { + t.Fatalf("expected stable weight=1 after finalise, got %v", got.Spec.Rules[0].BackendRefs[0].Weight) + } + + // Second Finalise: route already clean, should report no further work. + modified, err = ctrl.Finalise(context.TODO()) + if err != nil { + t.Fatalf("Finalise (idempotent): %v", err) + } + if modified { + t.Fatalf("Finalise idempotent call returned modified=true, expected false") + } +} + +// TestEnsureRoutesInvalidTraffic verifies that a non-numeric traffic string +// surfaces as an error rather than being silently treated as 0%. +func TestEnsureRoutesInvalidTraffic(t *testing.T) { + const ns = "default" + const routeName = "demo-route" + scheme := newGatewayTestScheme(t) + fakeCli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(newTestHTTPRoute(routeName, ns, "store-svc")).Build() + ctrl, err := NewGatewayTrafficRouting(fakeCli, Config{ + Key: "rollout/demo", + Namespace: ns, + CanaryService: "store-svc-canary", + StableService: "store-svc", + TrafficConf: &v1beta1.GatewayTrafficRouting{HTTPRouteName: utilpointer.String(routeName)}, + }) + if err != nil { + t.Fatalf("NewGatewayTrafficRouting: %v", err) + } + bogus := "not-a-percent" + if _, err := ctrl.EnsureRoutes(context.TODO(), &v1beta1.TrafficRoutingStrategy{Traffic: &bogus}); err == nil { + t.Fatalf("expected EnsureRoutes to reject invalid traffic %q, got nil error", bogus) + } +} From e2ea6133a05046132c97558495fe7a2c317cda90 Mon Sep 17 00:00:00 2001 From: Marco Ma Date: Wed, 10 Jun 2026 21:02:31 +0800 Subject: [PATCH 2/2] tiny change Signed-off-by: Marco Ma --- pkg/trafficrouting/network/gateway/gateway_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/trafficrouting/network/gateway/gateway_test.go b/pkg/trafficrouting/network/gateway/gateway_test.go index 47d91e2c..45966511 100644 --- a/pkg/trafficrouting/network/gateway/gateway_test.go +++ b/pkg/trafficrouting/network/gateway/gateway_test.go @@ -939,7 +939,7 @@ func TestBuildDesiredHTTPRoute(t *testing.T) { return rules }, }, - // Bug #17/#19 regression: BackendRef without an explicit Kind must be + // BackendRef without an explicit Kind must be // treated as a Service per the Gateway API spec (Kind defaults to // "Service" when nil). Some controllers (e.g. Envoy Gateway) emit // HTTPRoute objects with Kind unset. @@ -1168,7 +1168,7 @@ func TestBuildDesiredHTTPRoute(t *testing.T) { return rules }, }, - // Bug #13 regression: nonPathMatches must be indexed by their own + // nonPathMatches must be indexed by their own // position, not the position they had in the original matches slice. // Place a path match first and a non-path match second so the indices // diverge: pathMatches=[matches[0]], nonPathMatches=[matches[1]]. @@ -1274,7 +1274,7 @@ func TestBuildDesiredHTTPRoute(t *testing.T) { return rules }, }, - // Test #23: from the original routeDemo (no canary injected yet) to + // from the original routeDemo (no canary injected yet) to // canary@20. The existing "canary weight: 20" case feeds in the final // 80/20 shape and only proves idempotence; this case proves the // transformation from clean state.