-
Notifications
You must be signed in to change notification settings - Fork 753
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
Enable ENABLE_V6_EGRESS on Clusters with Mixed IPv6/IPv4 Subnets #2754
Enable ENABLE_V6_EGRESS on Clusters with Mixed IPv6/IPv4 Subnets #2754
Conversation
@sergeylanzman I am confused by this PR. EKS only supports a single Kubernetes IP family for pods, so is this an IPv4 cluster that you are trying to migrate to IPv6? Are pods using IPv4 or IPv6 addresses? Note that |
@jdn5126 My use case involves multiple groups of nodes. In one of these groups, there is a node that supports both IPv6 and IPv4 (private), and all pods are operating on IPv4. My requirement is to enable pods from the node group with IPv6 to communicate with IPv6 destinations. EKS offers the currently I need use IPv6 on one of group nodes, but's it's can be usable for slowly migration to ipv6 |
If this is an IPv4 cluster, how do you have nodes with IPv6-only subnets? Or maybe I am misreading and it is an IPv4 cluster where only some nodes have IPv6 addresses assigned. If it is the latter, we unfortunately have a limitation where |
@jdn5126 second option. Some node with ipv6 Why this limitation? Ipv6 not in internal cluster network only egress. So it’s per node configuration. We can support v6_egress only on node with ipv6. On others nodes(ipv4 only) it’s still disabled. |
The limitation was added to enforce consistency, as we cannot detect between the case where a node is supposed to have an IPv6 address (but is misconfigured) vs the case where the node should not have an IPv6 address. I see your point, though. We could instead skip adding the egress plugin and log a warning here if the IPv6 address cannot be found. I'll need to talk to my team about this to see if this is a change we are ok with. |
Exactly, that's the functionality introduced by the PR. Here's the breakdown: If ENABLE_V6_EGRESS is enabled and the node has IPv6: IPv6 egress is enabled. I waiting for your team's anwser |
Thanks for explaining @sergeylanzman. I think we can make this PR a bit simpler by just keeping |
@sergeylanzman we are good to take an enhancement here where we log a warning and continue when |
I am interesting. What need? |
So all we should need to do is change this error to a warning and set |
what about other places where used |
Usage in both cases should be harmless. In the init container, it will enable IPv6 forwarding on the node, which will not impact anything if you are not sending IPv6 traffic. And in |
c82dedf
to
e1cb012
Compare
Done |
The change generally looks good, thank you! I will pull it tomorrow and do some further testing. |
any updates? |
Sorry for the delay, I added this validation to my current sprint and plan to finish by the end of the week |
Thank you |
Testing that I carried out:
@sergeylanzman can you share some of the test coverage that you got? |
yes, my test lab for now:
init container set log from ipv4 nodes
|
df41d77
to
c23692c
Compare
@sergeylanzman awesome, thank you for confirming! I am going to do a bit more testing to confirm that the sysctls have no adverse impact, then I can merge this in tomorrow as everything looks good to me. |
@sergeylanzman actually, sorry, one more thing: can we add a comment by this change that says:
|
c23692c
to
128e906
Compare
Added. I think you mean this place? |
Yep, thank you! Just approved. Once merged, this will ship in the next VPC CNI release, which is currently targeting mid-February. |
What type of PR is this?
feature
Which issue does this PR fix:
N/A
What does this PR do / Why do we need it:
Today, the inability to enable ENABLE_V6_EGRESS on clusters with both IPv6 and IPv4 subnets is addressed in this PR. This enhancement allows for the coexistence of nodes operating exclusively on IPv6, where pods can utilize IPv6 egress, alongside nodes functioning solely on IPv4 without ENABLE_V6_EGRESS being active. This integration enhances network flexibility and functionality.
If an issue # is not available please add repro steps and logs from IPAMD/CNI showing the issue:
Will this PR introduce any new dependencies?:
N/A
Will this break upgrades or downgrades? Has updating a running cluster been tested?:
No
Does this change require updates to the CNI daemonset config files to work?:
No
If this change does not work with a "kubectl patch" of the image tag, please explain why.
No
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.