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

iptables: handle case where kernel IPv6 support is disabled #20680

Merged
merged 1 commit into from
Jul 29, 2022

Conversation

jibi
Copy link
Member

@jibi jibi commented Jul 28, 2022

Currently, even if the kernel IPv6 support is disabled (i.e. the
ipv6.disable=1 kernel parameter is set), the agent will try to find or
load the ip6tables kernel modules, and if successful it will assume
ip6tables is supported.

This will result in a fatal error since all ip6tables commands will
fail with "Address family not supported".

This commit fixes this by setting the haveIp6tables IptablesManager's
property to false in case the kernel does not support IPv6.

Fixes: d812b92 ("iptables: don't ignore errors")
Fixes: #18513
Fixes: #20672

@jibi jibi added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.10 labels Jul 28, 2022
@jibi jibi requested a review from a team as a code owner July 28, 2022 10:04
@jibi jibi requested a review from YutaroHayakawa July 28, 2022 10:04
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.11.8 Jul 28, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.14 Jul 28, 2022
@jibi jibi requested a review from julianwiedmann July 28, 2022 10:04
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

ugh, I remember having an uneasy feeling about this...

For the commit message:

  • can you please add what specific case this covers? It's the ipv6.disable=1 module parameter, correct?
  • think we need a Fixes: tag for the commit that introduced the regression.

pkg/datapath/iptables/iptables.go Outdated Show resolved Hide resolved
@jibi jibi force-pushed the pr/jibi/fix-iptables-without-ipv6-kernel-support branch 2 times, most recently from 4e38f16 to f7d7c6a Compare July 28, 2022 11:56
@jibi
Copy link
Member Author

jibi commented Jul 28, 2022

/test

Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for the quick fix jibi! One further comment, but feel free to ignore.

pkg/datapath/iptables/iptables.go Show resolved Hide resolved
@jibi jibi force-pushed the pr/jibi/fix-iptables-without-ipv6-kernel-support branch from f7d7c6a to e9a2928 Compare July 28, 2022 13:55
Currently, even if the kernel IPv6 support is disabled (i.e. the
ipv6.disable=1 kernel parameter is set), the agent will try to find or
load the ip6tables kernel modules, and if successful it will assume
ip6tables is supported.

This will result in a fatal error since all ip6tables commands will
fail with "Address family not supported".

This commit fixes this by setting the haveIp6tables IptablesManager's
property to false in case the kernel does not support IPv6.

Fixes: d812b92 ("iptables: don't ignore errors")
Fixes: #18513
Fixes: #20672
Signed-off-by: Gilberto Bertin <jibi@cilium.io>
@jibi jibi force-pushed the pr/jibi/fix-iptables-without-ipv6-kernel-support branch from e9a2928 to aa2081c Compare July 28, 2022 14:02
@jibi
Copy link
Member Author

jibi commented Jul 28, 2022

/test

Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

LGTM with a nit (almost the same as Julian's one) 👍

"Unable to read /sys/module/ipv6/parameters/disable, disabling IPv6 iptables support")
haveIp6tables = false
} else if strings.TrimSuffix(string(ipv6Disabled), "\n") == "1" {
log.Debug(
Copy link
Member

Choose a reason for hiding this comment

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

I think we should show this message to users even if it is not a Fatal. Maybe it should be a Warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm following the same pattern as above (i.e. emitting a debug logline in case we fail to find/load ip6tables modules):

if err := modulesManager.FindOrLoadModules(
"ip6_tables", "ip6table_mangle", "ip6table_raw", "ip6table_filter"); err != nil {
if option.Config.EnableIPv6 {
log.WithError(err).Fatal(
"IPv6 is enabled and ip6tables modules could not be initialized (try disabling IPv6 in Cilium or loading ip6_tables, ip6table_mangle, ip6table_raw and ip6table_filter kernel modules)")
}
log.WithError(err).Debug(
"ip6tables kernel modules could not be loaded, so IPv6 cannot be used")

@jibi jibi closed this Jul 29, 2022
@jibi jibi reopened this Jul 29, 2022
@brb
Copy link
Member

brb commented Jul 29, 2022

I thought that the issue got fixed with the closure of #18904

@jibi
Copy link
Member Author

jibi commented Jul 29, 2022

I thought that the issue got fixed with the closure of #18904

That change only handles the case where IPv6 support is enabled in Cilium but we can't find the related ip6tables modules (but apparently we can still load them even if the kernel has disabled IPv6 support)

@maintainer-s-little-helper 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 Jul 29, 2022
@aanm aanm merged commit 03b89ec into master Jul 29, 2022
@aanm aanm deleted the pr/jibi/fix-iptables-without-ipv6-kernel-support branch July 29, 2022 12:58
@aanm aanm mentioned this pull request Jul 29, 2022
2 tasks
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.14 Aug 9, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.11.8 Aug 9, 2022
@tklauser tklauser added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels Aug 11, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.11 in 1.11.8 Aug 11, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.14 Aug 11, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.14 Aug 11, 2022
@tklauser tklauser added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.14
Backport done to v1.10
1.11.8
Backport done to v1.11
8 participants