xds: fix ipFamily always nil #4782
Conversation
juwon8891
force-pushed
the
always-nil
branch
from
8c45abb
to
e029e70
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4782 +/- ##
==========================================
+ Coverage 65.60% 65.66% +0.06%
==========================================
Files 211 211
Lines 31982 32030 +48
==========================================
+ Hits 20983 21034 +51
+ Misses 9758 9757 -1
+ Partials 1241 1239 -2 ☔ View full report in Codecov by Sentry. |
| @@ -463,6 +463,7 @@ func (t *Translator) addRouteToRouteConfig( | |||
| ea := &ExtraArgs{ | |||
| metrics: metrics, | |||
| http1Settings: httpListener.HTTP1, | |||
| ipFamily: httpListener.IPFamily, | |||
There was a problem hiding this comment.
this is incorrect, you are using the listener level IPFamily value in the cluster
you should be deriving this value using a new IPFamily field in
https://github.com/envoyproxy/gateway/blame/48a0310082ceeaf4d3af17e6b6844c7d0fb5db0b/internal/ir/xds.go#L1297 instead and this should be set in the gateway api layer using the Backend Service / EndpointSlice info
There was a problem hiding this comment.
Thanks for the feedback! I’ve updated the code to use a new IPFamily field
cc @zirain
juwon8891
force-pushed
the
always-nil
branch
2 times, most recently
from
f620e41
to
64f4832
Compare
internal/gatewayapi/route.go
Outdated
| @@ -1261,9 +1261,11 @@ func (t *Translator) processDestination(backendRefContext BackendRefContext, | |||
| Protocol: protocol, | |||
| Endpoints: endpoints, | |||
| AddressType: addrType, | |||
| IPFamily: getIPFamily(envoyProxy), | |||
There was a problem hiding this comment.
getIPFamily gets the value from the envoyProxy which is the listener's IP Family, not the backend's. This value should be coming from https://kubernetes.io/docs/concepts/services-networking/dual-stack/#dual-stack-service-configuration-scenarios
There was a problem hiding this comment.
Thanks for the feedback! I’ve updated the code to use a new IPFamily field
cc @zirain
| @@ -465,6 +465,10 @@ func (t *Translator) addRouteToRouteConfig( | |||
| http1Settings: httpListener.HTTP1, | |||
| } | |||
|
|
|||
| if len(httpRoute.Destination.Settings) > 0 { | |||
There was a problem hiding this comment.
is it possible to have different IPFamily in destination settings?
There was a problem hiding this comment.
Yes, it's possible to have different IPFamily settings in destination configurations. I've also added test cases to confirm this behavior.
There was a problem hiding this comment.
should can complete IPFamily from all destinations:
- if all v4, use v4
- if all v6, use v6
- if has both, use Dual?
cc @arkodg , WDYT?
Signed-off-by: Juwon Hwang (Kevin) <[email protected]>
Signed-off-by: Juwon Hwang (Kevin) <[email protected]>
Signed-off-by: Juwon Hwang (Kevin) <[email protected]>
Signed-off-by: Juwon Hwang (Kevin) <[email protected]>
juwon8891
force-pushed
the
always-nil
branch
from
0d9a6cc
to
4ff001e
Compare
Signed-off-by: Juwon Hwang (Kevin) <[email protected]>
Signed-off-by: Juwon Hwang (Kevin) <[email protected]>
chore: fix ipFamily always nil
What this PR does / why we need it:
This PR improves IP family configuration handling across different listener types and updates the DNS lookup strategy for better dual-stack support.
Which issue(s) this PR fixes:
Fixes #4761