update client/kernel version requirements for nftables kube-proxy #124152

danwinship · 2024-04-02T14:41:15Z

What type of PR is this?

/kind cleanup
/sig network
/area kube-proxy
/priority important-soon

What this PR does / why we need it:

Addresses the problem that (a) sometimes older versions of nft will crash if they see rules created by newer versions of nft, and that (b) with nft < 1.0.1, when you do nft -f ... it will try to parse the entire ruleset at startup, rather than only trying to parse the tables that you actually reference in the commands passed to nft. Thus:

Require that kube-proxy be using nft 1.0.1 or later, to ensure that other people's use of nftables won't break kube-proxy.
Require that the kernel is 5.13 or newer, to hopefully ensure that the system has nft 1.0.1 or later, to ensure that kube-proxy's use of nftables won't break other people. (We can't directly check the system nft version, but all known distros with kernel 5.13 or later also have nft 1.0.1 or later.)
The nftables.skipKernelVersionCheck config option allows bypassing the above check for dev/testing purposes. (As of April 2024 there are still supported versions of LTS distros using slightly older kernels, though by the time we go GA this should no longer be true.)

More discussion in #122743.

Which issue(s) this PR fixes:

Fixes #122743

Special notes for your reviewer:

knftables rebase stolen from #123389...

Does this PR introduce a user-facing change?

The (alpha) nftables mode of kube-proxy now requires version 1.0.1 or later
of the nft command-line, and kernel 5.13 or later. (For testing/development
purposes, you can use older kernels, as far back as 5.4, if you set the
`nftables.skipKernelVersionCheck` option in the kube-proxy config, but this is not
recommended in production since it may cause problems with other nftables
users on the system.)

k8s-ci-robot · 2024-04-02T14:41:20Z

Please note that we're already in Test Freeze for the release-1.30 branch. This means every merged PR will be automatically fast-forwarded via the periodic ci-fast-forward job to the release branch of the upcoming v1.30.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Tue Apr 2 14:19:09 UTC 2024.

k8s-ci-robot · 2024-04-02T14:41:24Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-triage-robot · 2024-04-02T14:52:06Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

bowei · 2024-04-02T15:47:28Z

pkg/proxy/apis/config/types.go

+	// should only be used if you know nothing else on the system is using nftables,
+	// since nft 0.9.8 and older will crash if kube-proxy's nftables rules are
+	// present.
+	AllowOlderKernel bool


Should this be more specific?

AllowNFTablesLEQ_0_9_8

I have some concerns that AllowOlderKernel is pretty vague as a flag name.

The vagueness is intentional, since we can't actually check for the thing that we want to check for (host filesystem's nft is 0.9.8 or later), so we're doing something vague to approximate it.

This flag will hopefully go away in GA anyway because (a) by that point the "older kernels" will be old enough that we don't have to feel as guilty about not supporting them, and (b) the problems caused by too-old-nft are annoying enough that we don't really want anybody using this option in production.

Probably I should document that better...

OK, renamed this to skipKernelVersionCheck and tried to clarify the docs. I didn't say anything about removing this in GA because maybe we won't.

pkg/proxy/nftables/proxier.go

cici37 · 2024-04-04T17:01:48Z

/remove-sig api-machinery

tnqn · 2024-04-07T09:18:32Z

/lgtm

The PR description is stale about the name of the new option.

k8s-ci-robot · 2024-04-07T09:18:37Z

LGTM label has been added.

Git tree hash: fac0697c8091a789ac01e4d7a7df4fd8103dcc56

danwinship · 2024-04-07T13:58:37Z

/retest-required
/assign @thockin
for KubeProxyConfiguration API changes (and vendor update but that commit is identical to the already-/approved one in #123389)

aojea · 2024-04-07T14:56:18Z

/test pull-kubernetes-e2e-kind-nftables

aojea · 2024-04-07T15:08:35Z

pkg/proxy/nftables/proxier.go

@@ -261,7 +265,7 @@ func NewProxier(ipFamily v1.IPFamily,
 }

 // Create a knftables.Interface and check if we can use the nftables proxy mode on this host.
-func getNFTablesInterface(ipFamily v1.IPFamily) (knftables.Interface, error) {
+func getNFTablesInterface(ipFamily v1.IPFamily, skipKernelVersionCheck bool) (knftables.Interface, error) {


in iptables we do this stuff before creating the proxy

kubernetes/cmd/kube-proxy/app/server_linux.go

Lines 108 to 111 in 9791f0d

// getIPTables returns an array of [IPv4, IPv6] utiliptables.Interfaces. If primaryFamily

// is not v1.IPFamilyUnknown then it will also separately return the interface for just

// that family.

func getIPTables(primaryFamily v1.IPFamily) ([2]utiliptables.Interface, utiliptables.Interface) {

should we move this validation to that place instead at the time of creating the proxier?

we do it that way for iptables to avoid having identical iptables-constructing functions in pkg/proxy/iptables and pkg/proxy/ipvs. But really, the logic belongs inside the individual backends; cmd/kube-proxy shoudn't be making assumptions about what they need.

or alternatively, we could call into a pkg/proxy/nftables function from cmd/kube-proxy to construct and sanity-check before we actually create the Proxier; we sort of do that with ipvs. But that's mostly only advantageous because it lets us avoid separately doing sanity-checking for the IPv4 and IPv6 Proxiers, and I have plans to eventually fix that anyway

it was a curious comment 👍

aojea · 2024-04-09T17:04:41Z

pkg/proxy/apis/config/types.go

+	// versions of the nft CLI in the host filesystem. Skipping the kernel version
+	// check will allow running nftables kube-proxy on older distros, but beware that
+	// it may interfere with the host OS's use of nftables.
+	SkipKernelVersionCheck bool


Thought: @thockin @danwinship these options are kind of developer options, adding it to the config sounds a bit excessive to me, we have some places that use environment variables for similar reasons

staging/src/k8s.io/apimachinery/pkg/util/net/http.go: if s := os.Getenv("DISABLE_HTTP2"); len(s) > 0 { staging/src/k8s.io/apimachinery/pkg/util/net/http.go: if s := os.Getenv("HTTP2_READ_IDLE_TIMEOUT_SECONDS"); len(s) > 0 { staging/src/k8s.io/apimachinery/pkg/util/net/http.go: if s := os.Getenv("HTTP2_PING_TIMEOUT_SECONDS"); len(s) > 0 { staging/src/k8s.io/apiserver/pkg/storage/etcd3/watcher.go: fatalOnDecodeError, _ = strconv.ParseBool(os.Getenv("KUBE_PANIC_WATCH_DECODE_ERROR"))

WDYT if we use an environment variables for this instead of leaking this into the config?
KUBE_PROXY_NFTABLES_SKIP_KERNEL_VERSION_CHECK

I don't love it, but I see the argument. Config feels "more serious".

Another option would be a devMode sub-stanza of the config?

alexzielenski · 2024-04-09T19:22:44Z

/remove-sig api-machinery

danwinship · 2024-04-10T17:28:45Z

Another possibility is that we could have kubelet pass an "nftables hint" to kube-proxy like we do with iptables. eg

table inet kubernetes-hints {
    chain networking.k8s.io/nft-version {
        continue comment "v1.0.7"
    }
}

or something. (Older-but-still-supported kernels don't allow putting comments on chains or tables, so we need an actual rule (eg continue).)

k8s-ci-robot · 2024-04-19T00:47:26Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

aojea · 2024-04-19T11:23:14Z

Another possibility is that we could have kubelet pass an "nftables hint" to kube-proxy like we do with iptables. eg
table inet kubernetes-hints {
    chain networking.k8s.io/nft-version {
        continue comment "v1.0.7"
    }
}
or something. (Older-but-still-supported kernels don't allow putting comments on chains or tables, so we need an actual rule (eg continue).)

it took you a while to get rid of the kproxy/kubelet combo , I think we should avoid creating dependencies between components if possible

What about the magic env variable #124152 (comment) ,

danwinship · 2024-04-19T12:06:16Z

it took you a while to get rid of the kproxy/kubelet combo , I think we should avoid creating dependencies between components if possible

The annoying part of IPTablesOwnershipCleanup was the fact that kube-proxy's rules depended on chains that kubelet created. The KUBE-IPTABLES-HINT has always worked pretty well and not caused problems. (And it helps components other than kube-proxy too.)

What about the magic env variable #124152 (comment) ,

I'm not opposed but I feel like that's less kubernetes-y than having a proper config option. I guess it partly depends on whether we expect this flag to go away or not. (I don't know of any reason right now why we'd need it in a year or so, but I can't say for sure that we wouldn't either.)

k8s-ci-robot · 2024-05-24T15:38:56Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2024-05-24T15:39:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danwinship
Once this PR has been reviewed and has the lgtm label, please ask for approval from thockin. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

~~cmd/kube-proxy/OWNERS~~ [danwinship]
pkg/generated/openapi/OWNERS
~~pkg/proxy/OWNERS~~ [danwinship]
~~pkg/util/kernel/OWNERS~~ [danwinship]
staging/src/k8s.io/kube-proxy/config/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Move the "nftables is supported" check into a separate function, and call it before the --init-only return.

cici37 · 2024-05-30T16:34:52Z

/remove-sig api-machinery

k8s-ci-robot added area/code-generation area/dependency Issues or PRs related to dependency changes kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Apr 2, 2024

k8s-ci-robot requested review from andrewsykim, bowei and a team April 2, 2024 14:42

danwinship mentioned this pull request Apr 2, 2024

nftables kube-proxy TODO #122572

Open

27 tasks

bowei reviewed Apr 2, 2024

View reviewed changes

pkg/proxy/nftables/proxier.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 4, 2024

danwinship force-pushed the knftables-versions branch from 9cfb85a to edda5e7 Compare April 5, 2024 13:41

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 5, 2024

k8s-ci-robot assigned tnqn Apr 7, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 7, 2024

k8s-ci-robot assigned thockin Apr 7, 2024

aojea reviewed Apr 7, 2024

View reviewed changes

aojea reviewed Apr 9, 2024

View reviewed changes

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 9, 2024

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 19, 2024

danwinship force-pushed the knftables-versions branch from edda5e7 to 5578e5b Compare May 24, 2024 15:38

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2024

k8s-ci-robot requested review from thockin and tnqn May 24, 2024 15:38

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 24, 2024

Reorganize nftables proxy init

8899191

Move the "nftables is supported" check into a separate function, and call it before the --init-only return.

danwinship force-pushed the knftables-versions branch from 5578e5b to 7da9b2e Compare May 24, 2024 15:40

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2024

danwinship added 2 commits May 24, 2024 12:57

Require kernel 5.13 for nftables kube-proxy

1d41169

Add nftables.skipKernelVersionCheck option

f4a189d

danwinship force-pushed the knftables-versions branch from 7da9b2e to f4a189d Compare May 24, 2024 16:57

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update client/kernel version requirements for nftables kube-proxy #124152

update client/kernel version requirements for nftables kube-proxy #124152

danwinship commented Apr 2, 2024 •

edited

k8s-ci-robot commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

k8s-triage-robot commented Apr 2, 2024

bowei Apr 2, 2024

danwinship Apr 2, 2024

danwinship Apr 5, 2024

cici37 commented Apr 4, 2024

tnqn commented Apr 7, 2024

k8s-ci-robot commented Apr 7, 2024

danwinship commented Apr 7, 2024

aojea commented Apr 7, 2024

aojea Apr 7, 2024

danwinship Apr 9, 2024

danwinship Apr 9, 2024 •

edited

aojea Apr 9, 2024

aojea Apr 9, 2024 •

edited

thockin May 24, 2024

alexzielenski commented Apr 9, 2024

danwinship commented Apr 10, 2024

k8s-ci-robot commented Apr 19, 2024

aojea commented Apr 19, 2024

danwinship commented Apr 19, 2024

k8s-ci-robot commented May 24, 2024

k8s-ci-robot commented May 24, 2024

cici37 commented May 30, 2024

	// getIPTables returns an array of [IPv4, IPv6] utiliptables.Interfaces. If primaryFamily
	// is not v1.IPFamilyUnknown then it will also separately return the interface for just
	// that family.
	func getIPTables(primaryFamily v1.IPFamily) ([2]utiliptables.Interface, utiliptables.Interface) {

update client/kernel version requirements for nftables kube-proxy #124152

Are you sure you want to change the base?

update client/kernel version requirements for nftables kube-proxy #124152

Conversation

danwinship commented Apr 2, 2024 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Apr 2, 2024

k8s-ci-robot commented Apr 2, 2024

k8s-triage-robot commented Apr 2, 2024

bowei Apr 2, 2024

Choose a reason for hiding this comment

danwinship Apr 2, 2024

Choose a reason for hiding this comment

danwinship Apr 5, 2024

Choose a reason for hiding this comment

cici37 commented Apr 4, 2024

tnqn commented Apr 7, 2024

k8s-ci-robot commented Apr 7, 2024

danwinship commented Apr 7, 2024

aojea commented Apr 7, 2024

aojea Apr 7, 2024

Choose a reason for hiding this comment

danwinship Apr 9, 2024

Choose a reason for hiding this comment

danwinship Apr 9, 2024 • edited

Choose a reason for hiding this comment

aojea Apr 9, 2024

Choose a reason for hiding this comment

aojea Apr 9, 2024 • edited

Choose a reason for hiding this comment

thockin May 24, 2024

Choose a reason for hiding this comment

alexzielenski commented Apr 9, 2024

danwinship commented Apr 10, 2024

k8s-ci-robot commented Apr 19, 2024

aojea commented Apr 19, 2024

danwinship commented Apr 19, 2024

k8s-ci-robot commented May 24, 2024

k8s-ci-robot commented May 24, 2024

cici37 commented May 30, 2024

danwinship commented Apr 2, 2024 •

edited

danwinship Apr 9, 2024 •

edited

aojea Apr 9, 2024 •

edited