fix: enable ipv4 compat mode for dual stack cluster support #5018
Conversation
internal/xds/translator/listener.go
Outdated
| @@ -180,8 +180,9 @@ func buildXdsTCPListener( | |||
| }, | |||
| } | |||
|
|
|||
| if ipFamily != nil && *ipFamily == egv1a1.DualStack { | |||
| if ipFamily != nil && *ipFamily == egv1a1.DualStack || *ipFamily == egv1a1.IPv6 { | |||
There was a problem hiding this comment.
based on https://github.com/envoyproxy/gateway/pull/4817/files we shouldnt need this, because it should listen on ::
There was a problem hiding this comment.
Only thing I'm aware of is that this block was added in that PR. If it was necessary to support DualStack family it follows (to me) that it would be necessary in a dual stack cluster. I don't know enough about xds/envoy to explain why.
There was a problem hiding this comment.
Based on envoy docs this seems to be a required field for this use case (IPv6 single stack service on a dual stack IPv4 first cluster).
However, due to envoy defaults, this is likely only valid to set specifically for this usecase (IPv6 singlestack on dualstack cluster) and may break on a different usecase (IPv6 singlestack on IPv6 singlestack cluster).
listener: the ipv4_compat flag can only be set on Ipv6 address and Ipv4-mapped Ipv6 address. A runtime guard is added envoy.reloadable_features.strict_check_on_ipv4_compat and the default is true.
This leads me to believe this should be a separate setting in the EnvoyProxy config. Thoughts?
There was a problem hiding this comment.
Turns out that envoy.reloadable_features.strict_check_on_ipv4_compat was removed from envoy on a subsequent patch.
It seems to me this is a safe change to make.
tekulvw
force-pushed
the
ipv4-compat-dual-stack-cluster
branch
from
d3bdebc to
50f693f
Compare
tekulvw
changed the title
tekulvw
force-pushed
the
ipv4-compat-dual-stack-cluster
branch
from
6b6695a to
c761f28
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5018 +/- ##
==========================================
- Coverage 66.73% 66.69% -0.04%
==========================================
Files 209 209
Lines 32058 32264 +206
==========================================
+ Hits 21394 21520 +126
- Misses 9385 9442 +57
- Partials 1279 1302 +23 ☔ View full report in Codecov by Sentry. |
internal/xds/translator/listener.go
Outdated
| @@ -180,8 +180,9 @@ func buildXdsTCPListener( | |||
| }, | |||
| } | |||
|
|
|||
| if ipFamily != nil && *ipFamily == egv1a1.DualStack { | |||
| if ipFamily != nil && (*ipFamily == egv1a1.DualStack || *ipFamily == egv1a1.IPv6) { | |||
There was a problem hiding this comment.
will this make it impossible to generate a pure IPv6 listener?
There was a problem hiding this comment.
To my understanding, this flag allows envoy to respond to IPv4 requests when the listening address is ::. A pure IPv6 listener on a single stack IPv6 cluster should work, it seems the E2E tests confirm this though I haven't tested it personally.
Is a pure IPv6 listener possible on a dual stack cluster with this change? No. The alternative would be to introduce another parameter to the EnvoyProxy spec which allows this behavior.
tekulvw
changed the title
tekulvw
force-pushed
the
ipv4-compat-dual-stack-cluster
branch
from
c761f28 to
189acd6
Compare
|
I was entirely incorrect about the source of the @zirain this still does not consider a possibility for a pure IPv6 listener. |
tekulvw
force-pushed
the
ipv4-compat-dual-stack-cluster
branch
from
189acd6 to
6a7ae2c
Compare
tekulvw
changed the title
Signed-off-by: Will Tekulve <[email protected]>
tekulvw
force-pushed
the
ipv4-compat-dual-stack-cluster
branch
from
6a7ae2c to
56cf2bf
Compare
What type of PR is this?
Bug fix.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #5017
Release Notes: Yes/No