Skip to content
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.

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

fix: listener on IPv6 first cluster #4573

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

zirain
Copy link
Contributor

@zirain zirain commented Oct 30, 2024

fix: #4565
xref: #4572
Separate from : #4550

  • EG controller will detect the cluster is IPv6 or IPv4 first base on pod IP.
  • controller will change the default listener socket address base on that, use :: instead of 0.0.0.0 when IPv6 first.

Copy link

codecov bot commented Oct 30, 2024

Codecov Report

Attention: Patch coverage is 81.17647% with 16 lines in your changes missing coverage. Please review.

Project coverage is 65.38%. Comparing base (bb3bbdb) to head (5c4e99b).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/utils/net/ip.go 73.68% 4 Missing and 1 partial ⚠️
internal/xds/translator/listener.go 85.18% 3 Missing and 1 partial ⚠️
internal/gatewayapi/listener.go 57.14% 2 Missing and 1 partial ⚠️
internal/cmd/envoy/shutdown_manager.go 0.00% 2 Missing ⚠️
internal/gatewayapi/runner/runner.go 0.00% 1 Missing ⚠️
internal/infrastructure/host/proxy_infra.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4573      +/-   ##
==========================================
- Coverage   65.47%   65.38%   -0.09%     
==========================================
  Files         211      212       +1     
  Lines       31854    31930      +76     
==========================================
+ Hits        20855    20878      +23     
- Misses       9754     9804      +50     
- Partials     1245     1248       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -46,6 +46,10 @@ spec:
- server
- --config-path=/config/envoy-gateway.yaml
env:
- name: POD_IP
Copy link
Contributor

Choose a reason for hiding this comment

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

its a little fragile to use Envoy Gateway IP type to determine IP type for Envoy Proxy.
instead can we add the env to Envoy Proxy

and use that value directly in bootstrap using env substitution ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

does envoy support something like ${POD_IP} and check the POD_IP and change the listener address to ::1?

Copy link
Contributor

@arkodg arkodg Oct 31, 2024

Choose a reason for hiding this comment

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

yeah, chatgpt says yes, instead of listening on 0.0.0.0 or ::, you could listen on ${POD_IP}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can give a try, and we need POD_IP for dynamic listener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I'm not smart as gpt, but envoy report error with malformed IP address: ${POD_IP}.

Signed-off-by: zirain <[email protected]>
if resp, err := http.Post(fmt.Sprintf("http://%s:%d/%s",
bootstrap.EnvoyAdminAddress, bootstrap.EnvoyAdminPort, path), "application/json", nil); err != nil {
bootstrap.AdminAddress(egv1a1.IPv4), bootstrap.EnvoyAdminPort, path), "application/json", nil); err != nil {
Copy link
Contributor Author

@zirain zirain Oct 31, 2024

Choose a reason for hiding this comment

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

this may not right, what about change it to localhost? cc @arkodg @zhaohuabing

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to localhost, good idea

Copy link
Member

Choose a reason for hiding this comment

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

+1 localhost make things easier

Signed-off-by: zirain <[email protected]>
@zirain zirain added the area/IPv6 IPv6 related issues label Oct 31, 2024
}

func PreferIPFamily(ipv6First bool, envoyProxy *egv1a1.EnvoyProxy) egv1a1.IPFamily {
if ipv6First {
Copy link
Member

@zhaohuabing zhaohuabing Nov 1, 2024

Choose a reason for hiding this comment

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

To align with the IPFamily API docs, EG should default to IPv4.

	// IPFamily specifies the IP family for the EnvoyProxy fleet.
	// This setting only affects the Gateway listener port and does not impact
	// other aspects of the Envoy proxy configuration.
	// If not specified, the system will operate as follows:
	// - It defaults to IPv4 only.
	// - IPv6 and dual-stack environments are not supported in this default configuration.
	// Note: To enable IPv6 or dual-stack functionality, explicit configuration is required.
	// +kubebuilder:validation:Enum=IPv4;IPv6;DualStack
	// +optional
	IPFamily *IPFamily `json:"ipFamily,omitempty"`

Or do we want to change to:

Default to the IP family of the POD IP of the Envoy Proxy/Envoy Gateway?

This would have EG attempt to infer user's intented IP Family for the listening port of the Envoy proxy, which might not always be accurate.

Should we always default to IPv4 and require users to explicitly specify IPv6 if desired? It will make the IPFamily choice more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we always default to IPv4 and require users to explicitly specify IPv6 if desired? It will make the IPFamily choice more explicit.

this will make it's hard to use on IPv6 first cluster.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this approach requires one more IPFamily setting for users who want to use IPv6.

But if we make it implicit, "Default to the IP family of the POD IP of the Envoy Proxy/Envoy Gateway" may be challenging to clearly explain in the EG doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, we should do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/IPv6 IPv6 related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IPv6 dual-stack not working on IPv6 first clusters due to IPv4 fixed listeners
3 participants