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/07/04 02:59:35 UTC
[PR] fix: skip terminating endpoint [apisix-ingress-controller]
ronething opened a new pull request, #2451:
URL: https://github.com/apache/apisix-ingress-controller/pull/2451
<!-- 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. -->
### 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: skip terminating endpoint [apisix-ingress-controller]
Posted by "dspo (via GitHub)" <gi...@apache.org>.
dspo commented on code in PR #2451:
URL: https://github.com/apache/apisix-ingress-controller/pull/2451#discussion_r2184227539
##########
internal/provider/adc/translator/httproute.go:
##########
@@ -295,6 +295,14 @@ func (t *Translator) translateEndpointSlice(portName *string, weight int, endpoi
}
for _, endpoint := range endpointSlice.Endpoints {
for _, addr := range endpoint.Addresses {
+ if endpoint.Conditions.Ready != nil && !*endpoint.Conditions.Ready {
+ log.Debugw("skip not ready endpoint", zap.Any("endpoint", endpoint))
+ continue
+ }
+ if endpoint.Conditions.Terminating != nil && *endpoint.Conditions.Terminating {
+ log.Debugw("skip terminating endpoint", zap.Any("endpoint", endpoint))
+ continue
+ }
Review Comment:
It is recommended to put the filter in the controller layer and not manage the status of k8s resources in the translation layer.
https://github.com/apache/apisix-ingress-controller/blob/master/internal/controller/utils.go#L1447
--
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: support filter endpoint when translate backend ref. [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething commented on code in PR #2451:
URL: https://github.com/apache/apisix-ingress-controller/pull/2451#discussion_r2184469943
##########
internal/provider/adc/translator/httproute.go:
##########
@@ -312,11 +315,19 @@ func (t *Translator) translateEndpointSlice(portName *string, weight int, endpoi
return nodes
}
-func (t *Translator) TranslateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef) (adctypes.UpstreamNodes, error) {
- return t.translateBackendRef(tctx, ref)
+func DefaultEndpointFilter(endpoint *discoveryv1.Endpoint) bool {
+ if endpoint.Conditions.Ready != nil && !*endpoint.Conditions.Ready {
+ log.Debugw("skip not ready endpoint", zap.Any("endpoint", endpoint))
+ return false
+ }
+ return true
+}
+
+func (t *Translator) TranslateBackendRefWithFilter(tctx *provider.TranslateContext, ref gatewayv1.BackendRef, endpointFilter func(*discoveryv1.Endpoint) bool) (adctypes.UpstreamNodes, error) {
+ return t.translateBackendRef(tctx, ref, endpointFilter)
}
-func (t *Translator) translateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef) (adctypes.UpstreamNodes, error) {
+func (t *Translator) translateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef, endpointFilter func(*discoveryv1.Endpoint) bool) (adctypes.UpstreamNodes, error) {
Review Comment:
Currently, only the code that retrieves apisix standalone nodes calls the `TranslateBackendRefWithFilter` method, as it is called from another package, so it has been exported.
We can consider making a unified adjustment in the next PR.
--
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: skip terminating endpoint [apisix-ingress-controller]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2451:
URL: https://github.com/apache/apisix-ingress-controller/pull/2451#discussion_r2184197127
##########
internal/provider/adc/translator/httproute.go:
##########
@@ -295,6 +295,10 @@ func (t *Translator) translateEndpointSlice(portName *string, weight int, endpoi
}
for _, endpoint := range endpointSlice.Endpoints {
for _, addr := range endpoint.Addresses {
+ if endpoint.Conditions.Terminating != nil && *endpoint.Conditions.Terminating {
+ log.Debugw("skip terminating endpoint", zap.Any("endpoint", endpoint))
Review Comment:
[nitpick] Enhance the debug log with more context (e.g., port name or backend reference) to make it easier to trace which endpoint was skipped.
```suggestion
log.Debugw("skip terminating endpoint",
zap.Any("endpoint", endpoint),
zap.String("portName", func() string {
if portName != nil {
return *portName
}
return "nil"
}()),
zap.Any("endpointSlice", endpointSlice))
```
--
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: support filter endpoint when translate backend ref. [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething merged PR #2451:
URL: https://github.com/apache/apisix-ingress-controller/pull/2451
--
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: support filter endpoint when translate backend ref. [apisix-ingress-controller]
Posted by "ronething (via GitHub)" <gi...@apache.org>.
ronething commented on code in PR #2451:
URL: https://github.com/apache/apisix-ingress-controller/pull/2451#discussion_r2184467656
##########
internal/provider/adc/translator/httproute.go:
##########
@@ -283,7 +283,7 @@ func (t *Translator) fillHTTPRoutePolicies(routes []*adctypes.Route, policies []
}
}
-func (t *Translator) translateEndpointSlice(portName *string, weight int, endpointSlices []discoveryv1.EndpointSlice) adctypes.UpstreamNodes {
+func (t *Translator) translateEndpointSlice(portName *string, weight int, endpointSlices []discoveryv1.EndpointSlice, endpointFilter func(*discoveryv1.Endpoint) bool) adctypes.UpstreamNodes {
Review Comment:
There is still a need for more clarification. It may not be appropriate to make adjustments in this PR, as it involves more code changes.
--
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: support filter endpoint when translate backend ref. [apisix-ingress-controller]
Posted by "dspo (via GitHub)" <gi...@apache.org>.
dspo commented on code in PR #2451:
URL: https://github.com/apache/apisix-ingress-controller/pull/2451#discussion_r2184269082
##########
internal/provider/adc/translator/httproute.go:
##########
@@ -312,11 +315,19 @@ func (t *Translator) translateEndpointSlice(portName *string, weight int, endpoi
return nodes
}
-func (t *Translator) TranslateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef) (adctypes.UpstreamNodes, error) {
- return t.translateBackendRef(tctx, ref)
+func DefaultEndpointFilter(endpoint *discoveryv1.Endpoint) bool {
+ if endpoint.Conditions.Ready != nil && !*endpoint.Conditions.Ready {
+ log.Debugw("skip not ready endpoint", zap.Any("endpoint", endpoint))
+ return false
+ }
+ return true
+}
+
+func (t *Translator) TranslateBackendRefWithFilter(tctx *provider.TranslateContext, ref gatewayv1.BackendRef, endpointFilter func(*discoveryv1.Endpoint) bool) (adctypes.UpstreamNodes, error) {
+ return t.translateBackendRef(tctx, ref, endpointFilter)
}
-func (t *Translator) translateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef) (adctypes.UpstreamNodes, error) {
+func (t *Translator) translateBackendRef(tctx *provider.TranslateContext, ref gatewayv1.BackendRef, endpointFilter func(*discoveryv1.Endpoint) bool) (adctypes.UpstreamNodes, error) {
Review Comment:
These two functions are exactly the same. Can they be combined into one function?
##########
internal/provider/adc/translator/httproute.go:
##########
@@ -283,7 +283,7 @@ func (t *Translator) fillHTTPRoutePolicies(routes []*adctypes.Route, policies []
}
}
-func (t *Translator) translateEndpointSlice(portName *string, weight int, endpointSlices []discoveryv1.EndpointSlice) adctypes.UpstreamNodes {
+func (t *Translator) translateEndpointSlice(portName *string, weight int, endpointSlices []discoveryv1.EndpointSlice, endpointFilter func(*discoveryv1.Endpoint) bool) adctypes.UpstreamNodes {
Review Comment:
This method seems to cover the functionality of `translateEndpointSliceForIngress`. It seems that `translateEndpointSliceForIngress` can be removed or called `translateEndpointSlice ` in it.
--
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