You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by "ronething (via GitHub)" <gi...@apache.org> on 2025/09/02 09:51:51 UTC
[PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
ronething opened a new pull request, #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540
<!-- Please answer these questions before submitting a pull request -->
### Type of change:
<!-- Please delete options that are not relevant. -->
<!-- Select all the options from below that matches the type your PR best -->
- [x] Bugfix
- [ ] New feature provided
- [ ] Improve performance
- [ ] Backport patches
- [ ] Documentation
- [ ] Refactor
- [ ] Chore
- [ ] CI/CD or Tests
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
When there are multiple BackendRefs in HTTPRoute, we need to use the traffic-split plugin, rather than just use one upstream.
- dataplane config example:
```json
{
"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:
<!--
Please follow the requirements:
1. Use Draft if the PR is not ready to be reviewed
2. Test is required for the feat/fix PR, unless you have a good reason
3. Doc is required for the feat PR
4. Use a new commit to resolve review instead of `push -f`
5. Use "request review" to notify the reviewer once you have resolved the review
-->
- [ ] Did you explain what problem does this PR solve? Or what new features have been added?
- [ ] Have you added corresponding test cases?
- [ ] Have you modified the corresponding document?
- [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [mailing list](https://github.com/apache/apisix-ingress-controller#community) first**
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2318400577
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review Comment:
This aligns with the previous issue: it's best to avoid using inline upstreams. Otherwise, any change to the endpoints will trigger a complete rebuild of the routing tree.
Instead, ADC provides full capabilities to define separate upstream resources and assign IDs. You can then reference those IDs in traffic splits. This functionality is available out of the box for every ADC backend.
```yaml
services:
- name: demo
upstream:
nodes:
- host: inlined upstream
port: 443
weight: 5
upstreams:
- name: upstream2
bala: bala
id: sha1("upstream2") # by default
- id: custom-id-for-upstream3
name: upstream3
bala: bala
plugins:
traffic-split:
rules:
- weight: 5
- upstream_id: sha1("upstream2")
weight: 10
- upstream_id: custom-id-for-upstream3
weight: 5
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2321056171
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review Comment:
Great, as long as we can finish it before the next release. 🫡
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2318400577
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review 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.
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.
```yaml
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2318400577
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review 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.
Instead, ADC provides full capabilities to define separate upstream resources and assign IDs. You can then reference those IDs in traffic splits. This functionality is available out of the box for every ADC backend.
```yaml
services:
- name: demo
upstream:
nodes:
- host: inlined upstream
port: 443
weight: 5
upstreams:
- name: upstream2
bala: bala
id: sha1("upstream2") # by default
- id: custom-id-for-upstream3
name: upstream3
bala: bala
plugins:
traffic-split:
rules:
- weight: 5
- upstream_id: sha1("upstream2")
weight: 10
- upstream_id: custom-id-for-upstream3
weight: 5
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2318429890
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2318400577
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review 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.
Instead, ADC provides full capabilities to define separate upstream resources and assign IDs. You can then reference those IDs in traffic splits. This functionality is available out of the box for every ADC backend.
```yaml
services:
- name: demo
upstream:
nodes:
- host: inlined upstream
port: 443
weight: 5
upstreams:
- name: upstream2
bala: bala
id: sha1("upstream2") # by default
- id: custom-id-for-upstream3
name: upstream3
bala: bala
plugins:
traffic-split:
rules:
- weight: 5
- upstream_id: sha1("upstream2")
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2318400577
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review 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.
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.
```yaml
services:
- name: demo
upstream:
nodes:
- host: inlined upstream
port: 443
weight: 5
upstreams:
- name: upstream2
bala: bala
id: sha1("upstream2") # by default
- id: custom-id-for-upstream3
name: upstream3
bala: bala
plugins:
traffic-split:
rules:
- weight: 5
- upstream_id: sha1("upstream2")
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2318429890
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review Comment:
Yes, we are aware this issue currently exists. It's not limited to HTTPRoute, similar problems also exist in the current conversion 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "bzp2010 (via GitHub)" <gi...@apache.org>.
bzp2010 commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2318400577
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
Review 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.
```yaml
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething merged PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix: handle httproute multi backend refs [apisix-ingress-controller]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2540:
URL: https://github.com/apache/apisix-ingress-controller/pull/2540#discussion_r2317638973
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
- 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
Review Comment:
The comment has a grammatical error. It should read 'get weight from the backend ref starting from the second backend' or 'get weight from the backend refs starting from the second backend'.
```suggestion
// get weight from the backend refs starting from the second backend
```
##########
internal/adc/translator/httproute.go:
##########
@@ -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,
+ })
- 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)
+ }
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org