Skip to content

Commit 42470de

Browse files
committed
fix(gateway): correct BackendRef matching and canary header build
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 <qingjin_ma@163.com>
1 parent b2600e9 commit 42470de

2 files changed

Lines changed: 627 additions & 22 deletions

File tree

pkg/trafficrouting/network/gateway/gateway.go

Lines changed: 34 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ package gateway
1515

1616
import (
1717
"context"
18+
"fmt"
1819
"reflect"
1920

2021
"k8s.io/apimachinery/pkg/api/errors"
@@ -63,7 +64,10 @@ func (r *gatewayController) EnsureRoutes(ctx context.Context, strategy *v1beta1.
6364
var weight *int32
6465
if strategy.Traffic != nil {
6566
is := intstr.FromString(*strategy.Traffic)
66-
weightInt, _ := intstr.GetScaledValueFromIntOrPercent(&is, 100, true)
67+
weightInt, err := intstr.GetScaledValueFromIntOrPercent(&is, 100, true)
68+
if err != nil {
69+
return false, fmt.Errorf("invalid traffic %q for %s: %w", *strategy.Traffic, r.conf.Key, err)
70+
}
6771
weight = utilpointer.Int32(int32(weightInt))
6872
}
6973
matches := strategy.Matches
@@ -81,12 +85,12 @@ func (r *gatewayController) EnsureRoutes(ctx context.Context, strategy *v1beta1.
8185
// set route
8286
routeClone := &gatewayv1beta1.HTTPRoute{}
8387
if err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {
84-
if err = r.Client.Get(context.TODO(), types.NamespacedName{Namespace: httpRoute.Namespace, Name: httpRoute.Name}, routeClone); err != nil {
88+
if err = r.Client.Get(ctx, types.NamespacedName{Namespace: httpRoute.Namespace, Name: httpRoute.Name}, routeClone); err != nil {
8589
klog.Errorf("error getting updated httpRoute(%s/%s) from client", httpRoute.Namespace, httpRoute.Name)
8690
return err
8791
}
8892
routeClone.Spec.Rules = desiredRule
89-
return r.Client.Update(context.TODO(), routeClone)
93+
return r.Client.Update(ctx, routeClone)
9094
}); err != nil {
9195
klog.Errorf("update %s httpRoute(%s) failed: %s", r.conf.Key, httpRoute.Name, err.Error())
9296
return false, err
@@ -112,12 +116,12 @@ func (r *gatewayController) Finalise(ctx context.Context) (bool, error) {
112116
}
113117
routeClone := &gatewayv1beta1.HTTPRoute{}
114118
if err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {
115-
if err = r.Client.Get(context.TODO(), types.NamespacedName{Namespace: httpRoute.Namespace, Name: httpRoute.Name}, routeClone); err != nil {
119+
if err = r.Client.Get(ctx, types.NamespacedName{Namespace: httpRoute.Namespace, Name: httpRoute.Name}, routeClone); err != nil {
116120
klog.Errorf("error getting updated httpRoute(%s/%s) from client", httpRoute.Namespace, httpRoute.Name)
117121
return err
118122
}
119123
routeClone.Spec.Rules = desiredRule
120-
return r.Client.Update(context.TODO(), routeClone)
124+
return r.Client.Update(ctx, routeClone)
121125
}); err != nil {
122126
klog.Errorf("update %s httpRoute(%s) failed: %s", r.conf.Key, httpRoute.Name, err.Error())
123127
return false, err
@@ -194,11 +198,21 @@ func (r *gatewayController) buildCanaryHeaderHttpRoutes(rules []gatewayv1beta1.H
194198
canaryRuleMatch := &canaryRule.Matches[j]
195199
for k := range nonPathMatches {
196200
canaryRuleMatchBase := *canaryRuleMatch
197-
if len(matches[k].Headers) > 0 {
198-
canaryRuleMatchBase.Headers = append(canaryRuleMatchBase.Headers, matches[k].Headers...)
201+
// Clone Headers/QueryParams to avoid mutating the original
202+
// rule's underlying arrays via append. Preserve nil-ness so
203+
// the result is byte-for-byte identical to the previous
204+
// implementation when there is nothing to extend.
205+
if canaryRuleMatch.Headers != nil {
206+
canaryRuleMatchBase.Headers = append([]gatewayv1beta1.HTTPHeaderMatch{}, canaryRuleMatch.Headers...)
207+
}
208+
if canaryRuleMatch.QueryParams != nil {
209+
canaryRuleMatchBase.QueryParams = append([]gatewayv1beta1.HTTPQueryParamMatch{}, canaryRuleMatch.QueryParams...)
210+
}
211+
if len(nonPathMatches[k].Headers) > 0 {
212+
canaryRuleMatchBase.Headers = append(canaryRuleMatchBase.Headers, nonPathMatches[k].Headers...)
199213
}
200-
if len(matches[k].QueryParams) > 0 {
201-
canaryRuleMatchBase.QueryParams = append(canaryRuleMatchBase.QueryParams, matches[k].QueryParams...)
214+
if len(nonPathMatches[k].QueryParams) > 0 {
215+
canaryRuleMatchBase.QueryParams = append(canaryRuleMatchBase.QueryParams, nonPathMatches[k].QueryParams...)
202216
}
203217
newMatches = append(newMatches, canaryRuleMatchBase)
204218
}
@@ -241,35 +255,33 @@ func generateCanaryWeight(canaryPercent int32) (stableWeight int32, canaryWeight
241255
return stableWeight, canaryWeight
242256
}
243257

244-
// int indicates ref index
258+
// getServiceBackendRef returns the index and a copy of the BackendRef in rule
259+
// that targets the named Service. Per the Gateway API spec,
260+
// BackendObjectReference.Kind defaults to "Service" when nil, so refs without
261+
// an explicit Kind are treated as Service references.
262+
// Returns (-1, nil) when no matching ref is found.
245263
func getServiceBackendRef(rule gatewayv1beta1.HTTPRouteRule, serviceName string) (int, *gatewayv1beta1.HTTPBackendRef) {
246264
for i := range rule.BackendRefs {
247265
ref := rule.BackendRefs[i]
248-
if ref.Kind != nil && *ref.Kind == "Service" && string(ref.Name) == serviceName {
266+
if (ref.Kind == nil || *ref.Kind == "Service") && string(ref.Name) == serviceName {
249267
return i, &ref
250268
}
251269
}
252-
return 0, nil
270+
return -1, nil
253271
}
254272

255273
func setServiceBackendRef(rule *gatewayv1beta1.HTTPRouteRule, ref gatewayv1beta1.HTTPBackendRef) {
256-
if ref.Kind == nil || *ref.Kind != "Service" {
274+
// Per Gateway API spec, Kind defaults to "Service" when nil; only refuse
275+
// when an explicit Kind other than Service is set.
276+
if ref.Kind != nil && *ref.Kind != "Service" {
257277
return
258278
}
259279
index, currentRef := getServiceBackendRef(*rule, string(ref.Name))
260280
if currentRef == nil {
261281
rule.BackendRefs = append(rule.BackendRefs, ref)
262282
return
263283
}
264-
oldRefs := rule.BackendRefs
265-
rule.BackendRefs = []gatewayv1beta1.HTTPBackendRef{}
266-
for i := range oldRefs {
267-
if i == index {
268-
rule.BackendRefs = append(rule.BackendRefs, ref)
269-
} else {
270-
rule.BackendRefs = append(rule.BackendRefs, oldRefs[i])
271-
}
272-
}
284+
rule.BackendRefs[index] = ref
273285
}
274286

275287
func filterOutServiceBackendRef(rule *gatewayv1beta1.HTTPRouteRule, serviceName string) {

0 commit comments

Comments
 (0)