fix: handle httproute multi backend refs #2540
Conversation
Signed-off-by: Ashing Zheng <[email protected]>
Signed-off-by: Ashing Zheng <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR fixes HTTPRoute handling to support multiple backend references by implementing traffic-split functionality instead of using only a single upstream.
- Implements traffic-split plugin configuration for HTTPRoute with multiple BackendRefs
- Updates test infrastructure to support multiple nginx replicas for testing multi-backend scenarios
- Moves go-cmp and gorilla/websocket from indirect to direct dependencies in go.mod
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/adc/translator/httproute.go | Core logic to handle multiple backends using traffic-split plugin |
| test/e2e/gatewayapi/httproute.go | Test setup to deploy nginx with 2 replicas for multi-backend testing |
| test/e2e/framework/nginx.go | Add Replicas field to NginxOptions struct |
| test/e2e/framework/manifests/nginx.yaml | Template update to support configurable replica count |
| go.mod | Move dependencies from indirect to direct imports |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| weight := apiv2.DefaultWeight | ||
| // get weight from the backend ref start from the second backend | ||
| if i+1 < len(rule.BackendRefs) && rule.BackendRefs[i+1].Weight != nil { | ||
| weight = int(*rule.BackendRefs[i+1].Weight) | ||
| } |
There was a problem hiding this comment.
The weight assignment logic is incorrect. The loop index i corresponds to upstreams[i], but the weight is taken from rule.BackendRefs[i+1]. This creates an off-by-one error since upstreams starts from the second backend (index 1 in BackendRefs), so the weight should come from rule.BackendRefs[i+1] which is actually rule.BackendRefs[i+2] in the original BackendRefs array.
Co-authored-by: Copilot <[email protected]>
| } | ||
| weightedUpstreams = append(weightedUpstreams, adctypes.TrafficSplitConfigRuleWeightedUpstream{ | ||
| Weight: weight, | ||
| }) |
There was a problem hiding this comment.
The problem is the same as the previous one: it's best to avoid using inline upstreams. Otherwise, any change to the endpoints will trigger a complete rebuild of the routing tree. Essentially, that involves modifying plugins on the service, which is equivalent to modifying the service itself.
Instead, ADC provides full capabilities to define separate upstream resources and assign IDs. You can then reference those IDs in traffic-split. This functionality is available out of the box for every ADC backend.
services:
- name: demo
upstream:
nodes:
- host: inlined upstream
port: 443
weight: 5
upstreams:
- name: upstream2
bala: bala
# id: 293fcc9825f66b97b257289235c6f0334befe769 # by default sha1(name)
- id: custom-id-for-upstream3 # It is recommended that the ingress controller generate this ID using built-in rules.
name: upstream3
bala: bala
plugins:
traffic-split:
rules:
- weight: 5
- upstream_id: 293fcc9825f66b97b257289235c6f0334befe769
weight: 10
- upstream_id: custom-id-for-upstream3
weight: 5Changes to each individual upstream are independent and do not cause changes to the service or route. This avoids unnecessary rebuilds of the routing tree and meaningless changes to the conf version.
There was a problem hiding this comment.
Yes, we are aware this issue currently exists. It's not limited to HTTPRoute, similar problems also exist in the current translate logic of APISIXRoute. After discussing with @AlinsRan this morning, we've decided to address this uniformly in the next pull request. This pull request is solely intended to resolve the previous issue where multiple Upstreams were not correctly utilizing traffic-split plugin.
What do you think?
There was a problem hiding this comment.
Great, as long as we can finish it before the next release. 🫡
4 participants
Type of change:
What this PR does / why we need it:
When there are multiple BackendRefs in HTTPRoute, we need to use the traffic-split plugin, rather than just use one upstream.
Partial code reference:
apisix-ingress-controller/internal/adc/translator/apisixroute.go
Lines 264 to 295 in 95787e6
{ "upstreams": [ { "modifiedIndex": 1756805747874, "scheme": "http", "id": "29143272", "labels": { "managed-by": "apisix-ingress-controller" }, "nodes": [ { "weight": 100, "port": 80, "host": "10.244.0.15" } ], "type": "roundrobin", "name": "ingress-apisix-e2e-tests-default-217673000_httpbin_0" } ], "services": [ { "modifiedIndex": 1756805747874, "upstream_id": "29143272", "id": "29143272", "labels": { "k8s/controller-name": "apisix.apache.org/apisix-ingress-controller/ingress-apisix-e2e-tests-default-217673000", "k8s/kind": "HTTPRoute", "k8s/name": "httpbin", "manager-by": "apisix-ingress-controller", "k8s/namespace": "ingress-apisix-e2e-tests-default-217673000" }, "name": "ingress-apisix-e2e-tests-default-217673000_httpbin_0", "hosts": [ "httpbin.example" ], "plugins": { "traffic-split": { "rules": [ { "weighted_upstreams": [ { "weight": 100 }, { "weight": 0, "upstream": { "labels": { "managed-by": "apisix-ingress-controller" }, "scheme": "http", "nodes": [ { "weight": 0, "port": 80, "host": "10.244.0.18" }, { "weight": 0, "port": 80, "host": "10.244.0.19" } ], "type": "roundrobin" } } ] } ] } } } ] }Pre-submission checklist: