fix: Adding the fatal error for ipv6 cilium config on a single stack node #28953

vipul-21 · 2023-11-02T17:38:24Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Are you a user of Cilium? Please add yourself to the Users doc
Thanks for contributing!

Cilium pods on a single stack cluster were failing when ipv6 was enabled. The expectation is that ipv4 should be working even if there are error in ipv6.

Fixes: #28909

ti-mo · 2023-11-06T13:33:28Z

Code itself LGTM, but not sure if continuing silently is a good idea? There's a bunch of downstream logic that runs if option.Config.EnableIPv6 is true, so v6 support should probably be probed early on to set this flag to false if there's no v6 support on the host.

Also cc @cilium/sig-foundations since related to option.Config.

vipul-21 · 2023-11-06T23:20:01Z

@ti-mo So if we want to enable/disable ipv6 earlier would that be under NodeDiscovery or we should change the function IPv6Enabled() in daemonConfig here: https://github.com/cilium/cilium/blob/main/pkg/option/config.go#L2724 to check for host can support ipv6 or not.

jschwinger233

I'm not sure about "The expectation is that ipv4 should be working even if there are errors in getting ipv6 ips". Do you mean we expect cilium to work without IPv6 even if IPv6 is enabled by user when DirectRoutingDevice for v6 can't be found?

vipul-21 · 2023-11-07T16:55:48Z

@jschwinger233 What I meant is that even if we don't have a node that supports ipv6 and the user has enabled ipv6 on that node, the cilium pods should not go into crashloopback.

vipul-21 · 2023-11-07T19:18:42Z

Seeing the code, I see the usage of both option.Config.EnableIPv6 and option.Config.IPv6ENabled() , if we change the function option.Config.IPv6ENabled() to return true or false based on option.Config.EnableIPv6 and host/node supporting ipv6:

this would require us to change the usage of option.Config.EnableIPv6 to option.Config.IPv6ENabled() everywhere ?
How would we let the user know that even though the ipv6 is enabled, the cilium is not acknowledging that due to some issue as we are changing the option.Config.EnableIPv6 based on the flag and host properties ? Warning logs is one option.

In the current implementation, the node ip is fetched for each device or interface, and when we don't find the node ipv6 we continue.

Should we add a similar check earlier(if so where ?) that changes the EnableIPv6 to false or true based on the check ?
If the node does not support ipv6 I don't think we should not change the config value of this EnableIpv6 as other nodes might support v6 @ti-mo

vipul-21 · 2023-11-13T17:57:40Z

Moved the check in the init file and disabled ipv6 with a warning message that node does not have ipv6 support.

vipul-21 · 2023-11-28T19:44:52Z

Based on the conversation in the issue: #28909 (comment) the check is added to make sure that the uniform behavior is provided irrespective of the KPR value.

pippolo84

Thanks for the PR!
I think the approach is coherent with #28909 (comment). I left a couple of suggestions inline.

daemon/k8s/init.go

pippolo84

Thanks! 🚀

pippolo84 · 2024-01-24T17:55:35Z

/test

Cilium pods on a single stack cluster were failing when ipv6 was enabled. The change would make sure that if the underlying node does not have ipv6 configuration enabled it would fail even if KPR is disabled.Earlier, if KPR is enabled, the node would not see a fatal error. Signed-off-by: Vipul Singh <singhvipul@microsoft.com>

vipul-21 · 2024-01-31T20:48:14Z

@pippolo84 can we merge this ?

pippolo84 · 2024-02-01T11:44:46Z

/test

pippolo84 · 2024-02-01T14:15:36Z

E2E flake tracked here, rerunning.

hexchen · 2024-09-13T12:51:42Z

This PR introduced a regression in our environment. #34861 for a detailed description of the problem.

This reverts commit 2fc52eb. The commit was added with the intent to have an uniform behavior when Cilium is started on a single stack cluster with ipv6 support enabled, despite the KPR setting being enabled or disabled. Specifically, the added check was stopping the agent startup in case of a missing IPv6 address on the node, even when KPR was disabled. However, the IPv6 detection for NodePort, responsible for the startup error in case of missing IPv6 address with KPR enabled, has been removed in cilium#29033. Therefore, there is no reason anymore to stop the agent at startup when the IPv6 support is enabled and the node is missing an IPv6 address. Moreover, the commit above led to some unintended consequences in environments where KPR is disabled and the IPv6 address is set on the node after the Cilium agent is started (see cilium#34861). The same unintended consequence became more frequent after cilium#32381, where we moved later in the daemon startup process the first call to SetDefaultPrefix. That function, among other things, was in charge of setting up a temporary IPv6 Unique Local Address in case the node does not have one yet. Since now the agent does this later (specifically, while configuring IPAM), the check introduced in 2fc52eb sees the IPv6 node address as nil and stops the agent. Therefore, revert the check to avoid the unintended regressions. Related: cilium#28953 Related: cilium#34861 Related: cilium#29033 Related: cilium#32381 Fixes: cilium#37817 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

This reverts commit 2fc52eb. The commit was added with the intent to have an uniform behavior when Cilium is started on a single stack cluster with ipv6 support enabled, despite the KPR setting being enabled or disabled. Specifically, the added check was stopping the agent startup in case of a missing IPv6 address on the node, even when KPR was disabled. However, the IPv6 detection for NodePort, responsible for the startup error in case of missing IPv6 address with KPR enabled, has been removed in #29033. Therefore, there is no reason anymore to stop the agent at startup when the IPv6 support is enabled and the node is missing an IPv6 address. Moreover, the commit above led to some unintended consequences in environments where KPR is disabled and the IPv6 address is set on the node after the Cilium agent is started (see #34861). The same unintended consequence became more frequent after #32381, where we moved later in the daemon startup process the first call to SetDefaultPrefix. That function, among other things, was in charge of setting up a temporary IPv6 Unique Local Address in case the node does not have one yet. Since now the agent does this later (specifically, while configuring IPAM), the check introduced in 2fc52eb sees the IPv6 node address as nil and stops the agent. Therefore, revert the check to avoid the unintended regressions. Related: #28953 Related: #34861 Related: #29033 Related: #32381 Fixes: #37817 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

[ upstream commit dfd8f93 ] This reverts commit 2fc52eb. The commit was added with the intent to have an uniform behavior when Cilium is started on a single stack cluster with ipv6 support enabled, despite the KPR setting being enabled or disabled. Specifically, the added check was stopping the agent startup in case of a missing IPv6 address on the node, even when KPR was disabled. However, the IPv6 detection for NodePort, responsible for the startup error in case of missing IPv6 address with KPR enabled, has been removed in #29033. Therefore, there is no reason anymore to stop the agent at startup when the IPv6 support is enabled and the node is missing an IPv6 address. Moreover, the commit above led to some unintended consequences in environments where KPR is disabled and the IPv6 address is set on the node after the Cilium agent is started (see #34861). The same unintended consequence became more frequent after #32381, where we moved later in the daemon startup process the first call to SetDefaultPrefix. That function, among other things, was in charge of setting up a temporary IPv6 Unique Local Address in case the node does not have one yet. Since now the agent does this later (specifically, while configuring IPAM), the check introduced in 2fc52eb sees the IPv6 node address as nil and stops the agent. Therefore, revert the check to avoid the unintended regressions. Related: #28953 Related: #34861 Related: #29033 Related: #32381 Fixes: #37817 [ Backporter's notes: - resolved conflict in: daemon/k8s/init.go ] Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>

This reverts commit 2fc52eb. The commit was added with the intent to have an uniform behavior when Cilium is started on a single stack cluster with ipv6 support enabled, despite the KPR setting being enabled or disabled. Specifically, the added check was stopping the agent startup in case of a missing IPv6 address on the node, even when KPR was disabled. However, the IPv6 detection for NodePort, responsible for the startup error in case of missing IPv6 address with KPR enabled, has been removed in cilium#29033. Therefore, there is no reason anymore to stop the agent at startup when the IPv6 support is enabled and the node is missing an IPv6 address. Moreover, the commit above led to some unintended consequences in environments where KPR is disabled and the IPv6 address is set on the node after the Cilium agent is started (see cilium#34861). The same unintended consequence became more frequent after cilium#32381, where we moved later in the daemon startup process the first call to SetDefaultPrefix. That function, among other things, was in charge of setting up a temporary IPv6 Unique Local Address in case the node does not have one yet. Since now the agent does this later (specifically, while configuring IPAM), the check introduced in 2fc52eb sees the IPv6 node address as nil and stops the agent. Therefore, revert the check to avoid the unintended regressions. Related: cilium#28953 Related: cilium#34861 Related: cilium#29033 Related: cilium#32381 Fixes: cilium#37817 Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>

[ upstream commit dfd8f93 ] This reverts commit 2fc52eb. The commit was added with the intent to have an uniform behavior when Cilium is started on a single stack cluster with ipv6 support enabled, despite the KPR setting being enabled or disabled. Specifically, the added check was stopping the agent startup in case of a missing IPv6 address on the node, even when KPR was disabled. However, the IPv6 detection for NodePort, responsible for the startup error in case of missing IPv6 address with KPR enabled, has been removed in #29033. Therefore, there is no reason anymore to stop the agent at startup when the IPv6 support is enabled and the node is missing an IPv6 address. Moreover, the commit above led to some unintended consequences in environments where KPR is disabled and the IPv6 address is set on the node after the Cilium agent is started (see #34861). The same unintended consequence became more frequent after #32381, where we moved later in the daemon startup process the first call to SetDefaultPrefix. That function, among other things, was in charge of setting up a temporary IPv6 Unique Local Address in case the node does not have one yet. Since now the agent does this later (specifically, while configuring IPAM), the check introduced in 2fc52eb sees the IPv6 node address as nil and stops the agent. Therefore, revert the check to avoid the unintended regressions. Related: #28953 Related: #34861 Related: #29033 Related: #32381 Fixes: #37817 [ Backporter's notes: - resolved conflict in: daemon/k8s/init.go ] Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com> Signed-off-by: viktor-kurchenko <viktor.kurchenko@isovalent.com>

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 2, 2023

vipul-21 changed the title ~~fix: removing the fatal error for ipv6 cilium config~~ fix: removing the fatal error for ipv6 cilium config on a single stack cluster Nov 2, 2023

vipul-21 force-pushed the dualstackmaster branch 3 times, most recently from f30f597 to 718c26f Compare November 3, 2023 17:04

vipul-21 marked this pull request as ready for review November 3, 2023 17:22

vipul-21 requested review from a team as code owners November 3, 2023 17:22

vipul-21 requested review from marseel and ti-mo November 3, 2023 17:22

ti-mo requested review from a team and jschwinger233 and removed request for a team November 6, 2023 13:28

ti-mo added area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/misc This PR makes changes that have no direct user impact. area/agent Cilium agent related. labels Nov 6, 2023

maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Nov 6, 2023

ti-mo requested a review from a team November 6, 2023 13:33

jschwinger233 reviewed Nov 7, 2023

View reviewed changes

vipul-21 force-pushed the dualstackmaster branch from 718c26f to aa24d40 Compare November 13, 2023 17:54

vipul-21 force-pushed the dualstackmaster branch from aa24d40 to a9537ea Compare November 13, 2023 18:30

vipul-21 force-pushed the dualstackmaster branch from a9537ea to 72dbd32 Compare November 28, 2023 19:36

vipul-21 changed the title ~~fix: removing the fatal error for ipv6 cilium config on a single stack cluster~~ fix: Adding the fatal error for ipv6 cilium config on a single stack node Nov 28, 2023

marseel approved these changes Jan 17, 2024

View reviewed changes

pippolo84 requested changes Jan 19, 2024

View reviewed changes

daemon/k8s/init.go Outdated Show resolved Hide resolved

daemon/k8s/init.go Outdated Show resolved Hide resolved

vipul-21 force-pushed the dualstackmaster branch 2 times, most recently from 3f4dae2 to 380ba0c Compare January 24, 2024 17:19

vipul-21 requested a review from pippolo84 January 24, 2024 17:20

pippolo84 approved these changes Jan 24, 2024

View reviewed changes

vipul-21 force-pushed the dualstackmaster branch from 380ba0c to d1ad232 Compare January 29, 2024 21:55

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 1, 2024

julianwiedmann added the feature/ipv6 Relates to IPv6 protocol support label Feb 1, 2024

julianwiedmann added this pull request to the merge queue Feb 1, 2024

Merged via the queue into cilium:main with commit 2fc52eb Feb 1, 2024

bmhughes mentioned this pull request Sep 4, 2024

Add dual-stack support to vSphere CPI rancher/vsphere-charts#89

Merged

5 tasks

hexchen mentioned this pull request Sep 13, 2024

Regression: initialization of a single-stack IPv6 cluster with --cloud-provider=external #34861

Closed

3 tasks

pippolo84 mentioned this pull request Jun 19, 2025

After upgrading to v1.17.1 from v1.16, Failed to start with No IPv6 support on node as ipv6 address is nil #37817

Closed

3 tasks

pippolo84 mentioned this pull request Jun 20, 2025

Revert "fix: Adding ipv6 check on the node init function" #40143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: Adding the fatal error for ipv6 cilium config on a single stack node #28953

fix: Adding the fatal error for ipv6 cilium config on a single stack node #28953

Uh oh!

vipul-21 commented Nov 2, 2023 •

edited

Loading

Uh oh!

ti-mo commented Nov 6, 2023

Uh oh!

vipul-21 commented Nov 6, 2023

Uh oh!

jschwinger233 left a comment

Uh oh!

vipul-21 commented Nov 7, 2023

Uh oh!

vipul-21 commented Nov 7, 2023 •

edited

Loading

Uh oh!

vipul-21 commented Nov 13, 2023

Uh oh!

vipul-21 commented Nov 28, 2023

Uh oh!

pippolo84 left a comment

Uh oh!

Uh oh!

Uh oh!

pippolo84 left a comment

Uh oh!

pippolo84 commented Jan 24, 2024

Uh oh!

vipul-21 commented Jan 31, 2024

Uh oh!

pippolo84 commented Feb 1, 2024

Uh oh!

pippolo84 commented Feb 1, 2024

Uh oh!

hexchen commented Sep 13, 2024

Uh oh!

Uh oh!

fix: Adding the fatal error for ipv6 cilium config on a single stack node #28953

fix: Adding the fatal error for ipv6 cilium config on a single stack node #28953

Uh oh!

Conversation

vipul-21 commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ti-mo commented Nov 6, 2023

Uh oh!

vipul-21 commented Nov 6, 2023

Uh oh!

jschwinger233 left a comment

Choose a reason for hiding this comment

Uh oh!

vipul-21 commented Nov 7, 2023

Uh oh!

vipul-21 commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vipul-21 commented Nov 13, 2023

Uh oh!

vipul-21 commented Nov 28, 2023

Uh oh!

pippolo84 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pippolo84 left a comment

Choose a reason for hiding this comment

Uh oh!

pippolo84 commented Jan 24, 2024

Uh oh!

vipul-21 commented Jan 31, 2024

Uh oh!

pippolo84 commented Feb 1, 2024

Uh oh!

pippolo84 commented Feb 1, 2024

Uh oh!

hexchen commented Sep 13, 2024

Uh oh!

Uh oh!

vipul-21 commented Nov 2, 2023 •

edited

Loading

vipul-21 commented Nov 7, 2023 •

edited

Loading