kubernetes / kubeadm

Aggregator for issues filed against kubeadm
Apache License 2.0
3.75k stars 714 forks source link

disable the FileContent--proc-sys-net-bridge-bridge-nf-call-iptables check in a future release #3025

Closed dims closed 7 months ago

dims commented 7 months ago

Just ran into this kubeadm check for /proc/sys/net/bridge/bridge-nf-call-iptables on AL2023 and had to add it to the ignore list. Also looking through our code base, we do seem to ignore this a bit (minikube, CAPD etc). So question is, does it still serve a purpose?

https://cs.k8s.io/?q=FileContent--proc-sys-net-bridge-bridge-nf-call-iptables&i=nope&files=&excludeFiles=&repos=

thanks, Dims

/sig cluster-lifecycle


update by neolti123

remove the kubeadm check

k8s.io docs can be tracked elsewhere those fall under node/network.

dims commented 7 months ago

cc @neolit123

neolit123 commented 7 months ago

Just ran into this kubeadm check for /proc/sys/net/bridge/bridge-nf-call-iptables on AL2023 and had to add it to the ignore list. Also looking through our code base, we do seem to ignore this a bit (minikube, CAPD etc). So question is, does it still serve a purpose?

that's a good question. but i think it's up to sig-network and sig-node to tell us what to do about this.

it boils down to if the user wants iptables to see bridged traffic in their setup and i don't think it's up to kubeadm to warn or error about these options at this point. it used to be a hard requirement for dockershim, IIRC.

in the docs for k8s container runtimes it is still a requirement. https://kubernetes.io/docs/setup/production-environment/container-runtimes/#forwarding-ipv4-and-letting-iptables-see-bridged-traffic https://kubernetes.io/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins/#network-plugin-requirements

i authored the above container runtimes section. after my simple research i found that non-docker container runtime users were also setting it randomly on the internet. https://kubernetes.io/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins/#network-plugin-requirements

so we can update kubeadm's preflight check, but we also need to update the docs IMO. after advisory from the relevant SIGs.

/sig node network cluster-lifecycle @pacoxu @aojea please add more folks who may have context.

/kind feature documentation /remove-kind bug

neolit123 commented 7 months ago

/triage accepted /area kubeadm

aojea commented 7 months ago

see https://unix.stackexchange.com/a/720107

dims commented 7 months ago

@aojea @neolit123 Does this mean when iptables-nft are used then we can disable this check?

aojea commented 7 months ago

@aojea @neolit123 Does this mean when iptables-nft are used then we can disable this check?

@danwinship will know more for sure

neolit123 commented 7 months ago

seems like we can disable the kubeadm check conditionally in 1.32, if:

https://github.com/kubernetes/enhancements/blob/master/keps/sig-network/3866-nftables-proxy/README.md#summary

This KEP proposes the creation of a new official/supported nftables backend for kube-proxy. While it is hoped that this backend will eventually replace both the iptables and ipvs backends and become the default kube-proxy mode on Linux, that replacement/deprecation would be handled in a separate future KEP.

yep, switching the default can be a breaking change for distros where nftables is not available.

if that happens eventually, we can also update docs. right now, i'd argue that most users don't opt-in IPVS, so this information: https://kubernetes.io/docs/setup/production-environment/container-runtimes/#forwarding-ipv4-and-letting-iptables-see-bridged-traffic

and the kubeadm check are valid.

neolit123 commented 7 months ago

/transfer kubeadm

for (better) tracking

dims commented 7 months ago

@neolit123 ACK. thanks for the additional research! 🙏🏾

danwinship commented 7 months ago

In clusters where the sysctl is needed, it's not necessarily needed just for kube-proxy. (For instance, if you're using Calico for NetworkPolicy you probably need it to be set even if you're using nftables kube-proxy mode.)

But this isn't kubeadm's problem and we should just remove the checks (and the docs @neolit123 linked to). In 2016, right after we made kube-proxy stop unconditionally setting the sysctl, there was definitely a problem where some network implementations required the sysctl to be set but weren't setting it themselves. But this should all have been resolved by 2018 or so.

neolit123 commented 7 months ago

In clusters where the sysctl is needed, it's not necessarily needed just for kube-proxy. (For instance, if you're using Calico for NetworkPolicy you probably need it to be set even if you're using nftables kube-proxy mode.)

that's a good point. if we remove it from kubeadm's preflight and the CR docs, some CNI might break if they are missing it in their docs.

(just as a point of information) @caseydavenport @fasaxc we are trying to understand if CNIs that require settings like:

net.bridge.bridge-nf-call-iptables  = 1
net.bridge.bridge-nf-call-ip6tables = 1
net.ipv4.ip_forward                 = 1

already cover this in their documentation or rely on the upstream k8s docs https://kubernetes.io/docs/setup/production-environment/container-runtimes/#forwarding-ipv4-and-letting-iptables-see-bridged-traffic i could not find any mentions of that in: https://docs.tigera.io/calico/latest/getting-started/kubernetes/quickstart

neolit123 commented 7 months ago

But this isn't kubeadm's problem and we should just remove the checks (and the docs @neolit123 linked to). In 2016, right after we made kube-proxy stop unconditionally setting the sysctl, there was definitely a problem where some network implementations required the sysctl to be set but weren't setting it themselves. But this should all have been resolved by 2018 or so.

i still see this in setup guides everywhere. but if sig net's advice is to just remove it from kubeadm and docs, we can do it for 1.30.

my problem is that i don't want to triage additional tickets about it from new users, who need the setting for some legacy reason. removing the preflight check is safer, but not so sure about remove the mention in CR docs (yet). that i would leave to sig-network/sig-node.

danwinship commented 7 months ago

i still see this in setup guides everywhere.

Presumably they're just copying us.

The documentation was useful, a long time ago; there were cluster configurations that would have worked out of the box in Kubernetes 1.1, but that wouldn't work in Kubernetes 1.2, because kube-proxy no longer set the sysctl. But that was a long time ago.

my problem is that i don't want to triage additional tickets about it from new users

Right, but the authors of network plugins that require bridge-nf-call-itpables to be set don't want to triage tickets from new users either, so it seems reasonable to assume that at some point in the last 8 years they would have added the single line of code to set the sysctl themselves, rather than dealing with sporadic bug reports from people who hadn't noticed the warnings in the upstream documentation.

that i would leave to sig-network/sig-node.

FTR, we did just remove the corresponding warning from kube-proxy (#120818) last year. The only reason we didn't remove it sooner is because nobody ever really thought to ask whether it was still necessary.

neolit123 commented 7 months ago

ok, thanks for the additional information. given that, we can remove the check in kubeadm for 1.30, but we can leave this ticket open if needed for more discussion.

neolit123 commented 7 months ago

Right, but the authors of network plugins that require bridge-nf-call-itpables to be set don't want to triage tickets from new users either, so it seems reasonable to assume that at some point in the last 8 years they would have added the single line of code to set the sysctl themselves, rather than dealing with sporadic bug reports from people who hadn't noticed the warnings in the upstream documentation.

FWIW, we just close CNI tickets from this repo and tell users to ask for support in the CNI projects or on k8s support channels.

SataQiu commented 7 months ago

/cc