Skip to content

fix(gateway): correct BackendRef matching and canary header build#342

Open
marcoma2018 wants to merge 2 commits into
openkruise:masterfrom
marcoma2018:marco/gateway
Open

fix(gateway): correct BackendRef matching and canary header build#342
marcoma2018 wants to merge 2 commits into
openkruise:masterfrom
marcoma2018:marco/gateway

Conversation

@marcoma2018

Copy link
Copy Markdown
Contributor

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.

@kruise-bot kruise-bot requested review from FillZpp and zmberg June 10, 2026 04:11
@kruise-bot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign fillzpp for approval by writing /assign @fillzpp in a comment. For more information see:The Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@codecov

codecov Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 75.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.87%. Comparing base (b2600e9) to head (e2ea613).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
pkg/trafficrouting/network/gateway/gateway.go 75.00% 1 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #342      +/-   ##
==========================================
+ Coverage   51.38%   51.87%   +0.49%     
==========================================
  Files          66       66              
  Lines        8559     8559              
==========================================
+ Hits         4398     4440      +42     
+ Misses       3575     3530      -45     
- Partials      586      589       +3     
Flag Coverage Δ
unittests 51.87% <75.00%> (+0.49%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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>
Signed-off-by: Marco Ma <qingjin_ma@163.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants