Skip to content

fix: handle httproute multi backend refs #2540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ronething merged 3 commits into from
Sep 4, 2025
Merged

fix: handle httproute multi backend refs #2540

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ab62d56
fix: handle httproute multi backend refs
ronething Sep 2, 2025
5e4c67c
fix: r
ronething Sep 2, 2025
699d910
Update internal/adc/translator/httproute.go
ronething Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ require (
github.com/gavv/httpexpect/v2 v2.16.0
github.com/go-logr/logr v1.4.2
github.com/go-logr/zapr v1.3.0
github.com/google/go-cmp v0.6.0
github.com/google/uuid v1.6.0
github.com/gorilla/websocket v1.5.1
github.com/gruntwork-io/terratest v0.50.0
github.com/hashicorp/go-memdb v1.3.4
github.com/incubator4/go-resty-expr v0.1.1
Expand Down Expand Up @@ -108,11 +110,9 @@ require (
github.com/golang/protobuf v1.5.4 // indirect
github.com/google/cel-go v0.20.1 // indirect
github.com/google/gnostic-models v0.6.8 // indirect
github.com/google/go-cmp v0.6.0 // indirect
github.com/google/go-querystring v1.1.0 // indirect
github.com/google/gofuzz v1.2.0 // indirect
github.com/google/pprof v0.0.0-20240727154555-813a5fbdbec8 // indirect
github.com/gorilla/websocket v1.5.1 // indirect
github.com/grpc-ecosystem/grpc-gateway/v2 v2.20.0 // indirect
github.com/gruntwork-io/go-commons v0.8.0 // indirect
github.com/hashicorp/errwrap v1.1.0 // indirect
Expand Down
80 changes: 69 additions & 11 deletions internal/adc/translator/httproute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (

adctypes "github.com/apache/apisix-ingress-controller/api/adc"
"github.com/apache/apisix-ingress-controller/api/v1alpha1"
apiv2 "github.com/apache/apisix-ingress-controller/api/v2"
"github.com/apache/apisix-ingress-controller/internal/controller/label"
"github.com/apache/apisix-ingress-controller/internal/id"
"github.com/apache/apisix-ingress-controller/internal/provider"
Expand Down Expand Up @@ -466,32 +467,89 @@ func (t *Translator) TranslateHTTPRoute(tctx *provider.TranslateContext, httpRou
labels := label.GenLabel(httpRoute)

for ruleIndex, rule := range rules {
upstream := adctypes.NewDefaultUpstream()
var backendErr error
service := adctypes.NewDefaultService()
service.Labels = labels

service.Name = adctypes.ComposeServiceNameWithRule(httpRoute.Namespace, httpRoute.Name, fmt.Sprintf("%d", ruleIndex))
service.ID = id.GenID(service.Name)
service.Hosts = hosts

var (
upstreams = make([]*adctypes.Upstream, 0)
weightedUpstreams = make([]adctypes.TrafficSplitConfigRuleWeightedUpstream, 0)
backendErr error
)

for _, backend := range rule.BackendRefs {
if backend.Namespace == nil {
namespace := gatewayv1.Namespace(httpRoute.Namespace)
backend.Namespace = &namespace
}
upstream := adctypes.NewDefaultUpstream()
upNodes, err := t.translateBackendRef(tctx, backend.BackendRef, DefaultEndpointFilter)
if err != nil {
backendErr = err
continue
}
if len(upNodes) == 0 {
continue
}

t.AttachBackendTrafficPolicyToUpstream(backend.BackendRef, tctx.BackendTrafficPolicies, upstream)
upstream.Nodes = append(upstream.Nodes, upNodes...)
upstream.Nodes = upNodes
upstreams = append(upstreams, upstream)
}

// todo: support multiple backends
service := adctypes.NewDefaultService()
service.Labels = labels
// Handle multiple backends with traffic-split plugin
if len(upstreams) == 0 {
// Create a default upstream if no valid backends
upstream := adctypes.NewDefaultUpstream()
service.Upstream = upstream
} else if len(upstreams) == 1 {
// Single backend - use directly as service upstream
service.Upstream = upstreams[0]
} else {
// Multiple backends - use traffic-split plugin
service.Upstream = upstreams[0]
upstreams = upstreams[1:]

// Set weight in traffic-split for the default upstream
weight := apiv2.DefaultWeight
if rule.BackendRefs[0].Weight != nil {
weight = int(*rule.BackendRefs[0].Weight)
}
weightedUpstreams = append(weightedUpstreams, adctypes.TrafficSplitConfigRuleWeightedUpstream{
Weight: weight,
})
Copy link
Contributor

@bzp2010 bzp2010 Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: 5

Changes 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.

Copy link
Contributor Author

@ronething ronething Sep 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@bzp2010 bzp2010 Sep 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, as long as we can finish it before the next release. 🫡


service.Name = adctypes.ComposeServiceNameWithRule(httpRoute.Namespace, httpRoute.Name, fmt.Sprintf("%d", ruleIndex))
service.ID = id.GenID(service.Name)
service.Hosts = hosts
service.Upstream = upstream
// Set other upstreams in traffic-split
for i, upstream := range upstreams {
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)
}
Copy link

Copilot AI Sep 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
weightedUpstreams = append(weightedUpstreams, adctypes.TrafficSplitConfigRuleWeightedUpstream{
Upstream: upstream,
Weight: weight,
})
}

if len(weightedUpstreams) > 0 {
if service.Plugins == nil {
service.Plugins = make(map[string]any)
}
service.Plugins["traffic-split"] = &adctypes.TrafficSplitConfig{
Rules: []adctypes.TrafficSplitConfigRule{
{
WeightedUpstreams: weightedUpstreams,
},
},
}
}
}

if backendErr != nil && len(upstream.Nodes) == 0 {
if backendErr != nil && (service.Upstream == nil || len(service.Upstream.Nodes) == 0) {
if service.Plugins == nil {
service.Plugins = make(map[string]any)
}
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/framework/manifests/nginx.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ kind: Deployment
metadata:
name: nginx
spec:
replicas: 1
replicas: {{ .Replicas | default 1 }}
selector:
matchLabels:
app: nginx
Expand Down
1 change: 1 addition & 0 deletions test/e2e/framework/nginx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ var (

type NginxOptions struct {
Namespace string
Replicas *int32
}

func init() {
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/gatewayapi/httproute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/utils/ptr"
"sigs.k8s.io/gateway-api/apis/v1alpha2"

"github.com/apache/apisix-ingress-controller/api/v1alpha1"
Expand Down Expand Up @@ -1882,6 +1883,7 @@ spec:
beforeEachHTTP()
s.DeployNginx(framework.NginxOptions{
Namespace: s.Namespace(),
Replicas: ptr.To(int32(2)),
})
})
It("HTTPRoute Canary", func() {
Expand Down
Loading