proxy: remove iptables wait interval flag #131961

cyclinder · 2025-05-26T03:27:15Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Add version check for iptables cli wait interval flag compatibility, see #131948

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kube-proxy:  Remove iptables cli wait interval flag

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2025-05-26T03:27: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-sigs/prow repository.

pacoxu · 2025-05-26T03:36:45Z

/assign @aojea @dims @BenTheElder

pacoxu · 2025-05-26T06:08:17Z

UT needs an update.

https://storage.googleapis.com/kubernetes-ci-logs/pr-logs/pull/131961/pull-kubernetes-e2e-kind/1926863738980274176/artifacts/kind-control-plane/pods/kube-system_kube-proxy-rwwzw_ad824401-c81b-4006-8912-6c7a411f0e60/kube-proxy/0.log

2025-05-26T05:15:24.697334818Z stderr F E0526 05:15:24.697140       1 proxier.go:1565] "Failed to execute iptables-restore" err=<
2025-05-26T05:15:24.697425572Z stderr F 	exit status 2: Ignoring deprecated --wait-interval option.
2025-05-26T05:15:24.697433541Z stderr F 	Warning: Extension MARK revision 0 not supported, missing kernel module?
2025-05-26T05:15:24.697438482Z stderr F 	ip6tables-restore v1.8.9 (legacy): unknown option "--xor-mark"
2025-05-26T05:15:24.697444083Z stderr F 	Error occurred at line: 17
2025-05-26T05:15:24.697448297Z stderr F 	Try `ip6tables-restore -h' or 'ip6tables-restore --help' for more information.
2025-05-26T05:15:24.697454045Z stderr F  > ipFamily="IPv6"
2025-05-26T05:15:24.697570358Z stderr F I0526 05:15:24.697190       1 proxier.go:832] "Sync failed" ipFamily="IPv6" retryingTime="30s"

still failed for --xor-mark flag missing.

pacoxu · 2025-05-26T06:09:40Z

kubernetes/pkg/proxy/iptables/proxier.go

Lines 918 to 922 in 3044a4c

    
           // Clear the mark to avoid re-masquerading if the packet re-traverses the network stack. 
        
           proxier.natRules.Write( 
        
           	"-A", string(kubePostroutingChain), 
        
           	"-j", "MARK", "--xor-mark", proxier.masqueradeMark, 
        
           )

needs an update. So does */ipvs/proxier.go

cyclinder · 2025-05-26T07:01:49Z

still failed for --xor-mark flag missing.

I think it's a kernel issue? see k3s-io/k3s#11175, Is better to upgrade the kernel to fix it?

aojea · 2025-05-26T08:08:10Z

pkg/util/iptables/iptables.go

@@ -703,7 +706,7 @@ func getIPTablesVersion(exec utilexec.Interface, protocol Protocol) (*utilversio
 // Checks if iptables version has a "wait" flag
 func getIPTablesWaitFlag(version *utilversion.Version) []string {
 	switch {
-	case version.AtLeast(WaitIntervalMinVersion):
+	case version.AtLeast(WaitIntervalMinVersion) && version.LessThan(WaitIntervalMaxVersion):


I think we also need to change getIPTablesRestoreWaitFlag in line 718

ip6tables-legacy-restore -w 5 -W 100000 < save.txt Ignoring deprecated --wait-interval option. # ip6tables-legacy -L -w 5 -W 10000 Ignoring deprecated --wait-interval option.

thanks! I have added it.

aojea · 2025-05-26T08:13:07Z

still failed for --xor-mark flag missing.

I think it's a kernel issue? see k3s-io/k3s#11175, Is better to upgrade the kernel to fix it?

yeah, that part will be handled in kubernetes/test-infra#34851

aojea · 2025-05-26T08:15:30Z

@cyclinder one last comment https://github.com/kubernetes/kubernetes/pull/131961/files#r2106792344 to fix the restore part too and this lgtm

aojea · 2025-05-26T13:47:07Z

unti test failure looks legit

{Failed  === RUN   TestRestoreAllWait
    iptables_test.go:1362: wrong CombinedOutput() log, got [iptables-restore -w 5 --noflush --counters]

// TestRestoreAllWait tests that the "wait" flag is passed to a compatible iptables-restore
func TestRestoreAllWait(t *testing.T) {
	fcmd := fakeexec.FakeCmd{
		CombinedOutputScript: []fakeexec.FakeAction{
			// iptables version check
			func() ([]byte, []byte, error) { return []byte("iptables v1.9.22"), nil, nil },
			func() ([]byte, []byte, error) { return []byte{}, nil, nil },
			func() ([]byte, []byte, error) { return nil, nil, &fakeexec.FakeExitError{Status: 1} },
		},

aojea · 2025-05-26T13:48:55Z

/assign @danwinship @aojea

BenTheElder · 2025-05-28T16:37:53Z

Needs hack/update-gofmt.sh https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/131961/pull-kubernetes-verify/1927734084864315392
Unit test failure also looks related to this change https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/131961/pull-kubernetes-unit/1927734081907331072

BenTheElder · 2025-05-28T16:44:45Z

fixes #132006

aojea · 2025-05-28T16:54:41Z

fixes #132006

@BenTheElder this is not a problem, is a warning #131961 (comment)

aojea · 2025-05-28T16:57:09Z

pkg/util/iptables/iptables_test.go

-				waitFlag:        []string{"-w", "5", "-W", "100000"},
-				restoreWaitFlag: []string{"-w", "5", "-W", "100000"},
+				waitFlag:        []string{"-w", "5"},
+				restoreWaitFlag: []string{"-w", "5"},
 			},


you need to update line 332 to remove the -W since the test panics now

-- FAIL: TestNewDualStack/both_available (0.00s) === RUN TestNewDualStack --- FAIL: TestNewDualStack (0.00s) panic: command (iptables) received with extra/missing arguments. Expected [iptables -w 5 -W 100000 -S POSTROUTING -t nat], Received [iptables -w 5 -S POSTROUTING -t nat] [recovered] panic: command (iptables) received with extra/missing arguments. Expected [iptables -w 5 -W 100000 -S POSTROUTING -t nat], Received [iptables -w 5 -S POSTROUTING -t nat] goroutine 15 [running]: testing.tRunner.func1.2({0xb1b040, 0xc000063590}) /home/prow/go/src/k8s.io/kubernetes/_output/local/go/cache/mod/golang.org/toolchain@v0.0.1-go1.24.3.linux-amd64/src/testing/testing.go:1734 +0x3eb testing.tRunner.func1() /home/prow/go/src/k8s.io/kubernetes/_output/local/go/cache/mod/golang.org/toolchain@v0.0.1-go1.24.3.linux-amd64/src/testing/testing.go:1737 +0x696 panic({0xb1b040?, 0xc000063590?}) /home/prow/go/src/k8s.io/kubernetes/_output/local/go/cache/mod/golang.org/toolchain@v0.0.1-go1.24.3.linux-amd64/src/runtime/panic.go:792 +0x132 k8s.io/utils/exec/testing.(*FakeExec).Command(0xc00029c400, {0xbf748a, 0x8}, {0xc000209c80, 0x6, 0x8}) /home/prow/go/src/k8s.io/kubernetes/vendor/k8s.io/utils/exec/testing/fake_exec.go:65 +0x82e k8s.io/utils/exec/testing.(*FakeExec).CommandContext(0xc00029c400, {0x6?, 0x8?}, {0xbf748a, 0x8}, {0xc000209c80, 0x6, 0x8}) /home/prow/go/src/k8s.io/kubernetes/vendor/k8s.io/utils/exec/testing/fake_exec.go:90 +0x65 k8s.io/kubernetes/pkg/util/iptables.(*runner).runContext(0xc000209b80, {0xce9be0, 0x1077b40}, {0xbf5d45, 0x2}, {0xc000205d60, 0x3, 0x20?}) /home/prow/go/src/k8s.io/kubernetes/pkg/util/iptables/iptables.go:493 +0x44f k8s.io/kubernetes/pkg/util/iptables.(*runner).run(...) /home/prow/go/src/k8s.io/kubernetes/pkg/util/iptables/iptables.go:482 k8s.io/kubernetes/pkg/util/iptables.(*runner).ChainExists(0xc000209b80, {0xbf5eaa, 0x3}, {0xbf9041, 0xb}) /home/prow/go/src/k8s.io/kubernetes/pkg/util/iptables/iptables.go:651 +0x373 k8s.io/kubernetes/pkg/util/iptables.(*runner).Present(0xc000209b80) /home/prow/go/src/k8s.io/kubernetes/pkg/util/iptables/iptables.go:759 +0x3f k8s.io/kubernetes/pkg/util/iptables.newDualStackInternal({0xce5ea8, 0xc00029c400}) /home/prow/go/src/k8s.io/kubernetes/pkg/util/iptables/iptables.go:255 +0x78 k8s.io/kubernetes/pkg/util/iptables.TestNewDualStack.func17(0xc000239dc0) /home/prow/go/src/k8s.io/kubernetes/pkg/util/iptables/iptables_test.go:332 +0x4b testing.tRunner(0xc000239dc0, 0xc00029c3c0) /home/prow/go/src/k8s.io/kubernetes/_output/local/go/cache/mod/golang.org/toolchain@v0.0.1-go1.24.3.linux-amd64/src/testing/testing.go:1792 +0x226 created by testing.(*T).Run in goroutine 14 /home/prow/go/src/k8s.io/kubernetes/_output/local/go/cache/mod/golang.org/toolchain@v0.0.1-go1.24.3.linux-amd64/src/testing/testing.go:1851 +0x8f3 }

Yes, updated.

BenTheElder · 2025-05-28T16:57:12Z

fixes #132006

@BenTheElder this is not a problem, is a warning #131961 (comment)

Check the issue :-)
I filed a dedicated issue for this warning.
IMHO warnings about deprecated flag usage are future problems.
#132006 tracks only this warning, separate from the issue we had bringing up clusters (which have been resolved already).

aojea · 2025-05-28T18:05:49Z

lol, answering from email so I assumed it was the same issue,

aojea · 2025-05-29T08:24:27Z

pkg/util/iptables/monitor_test.go

+	// Find the operation, chain name, and table name in the arguments
+	// The command structure can vary based on the iptables version and wait flags
+	// but will always have these components
+	var op operation
+	var chainName, tableName string
+	tableIndex := -1
+	opIndex := -1
+
+	// Find the table argument (-t)
+	for i, arg := range mfc.args {
+		if arg == "-t" && i+1 < len(mfc.args) {
+			tableName = mfc.args[i+1]
+			tableIndex = i
+			break
+		}
+	}
+
+	if tableIndex == -1 {
+		panic(fmt.Sprintf("no table specified in args %#v", mfc.args))
 	}
-	op := operation(mfc.args[4])
-	chainName := mfc.args[5]
-	tableName := mfc.args[7]
+
+	// Find the operation (like -N, -X, -F, etc.)
+	for i, arg := range mfc.args {
+		if i > 0 && strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "--") &&
+			arg != "-t" && arg != "-w" && arg != "-W" && i != tableIndex+1 {
+			op = operation(arg)
+			opIndex = i
+			break
+		}
+	}
+
+	if opIndex == -1 || opIndex+1 >= len(mfc.args) {
+		panic(fmt.Sprintf("no operation found in args %#v", mfc.args))
+	}
+
+	// The chain name typically follows the operation
+	chainName = mfc.args[opIndex+1]


we move from a statically hardcoded arguments match to a new dynamic logic to find the same parameters, @danwinship already did this for kube-proxy so he'll be the better to judge it, either works for me

/lgtm

This isn't necessary. monitorFakeExec/monitorFakeCmd isn't intended to handle arbitrary iptables commands, it's intended only to handle the things that Monitor / TestIPTablesMonitor do. (eg, 99% of the monitorFakeCmd methods just panic, because the code "knows" that they won't be called.) All of the commands we have to parse are generated by runContext() from iptables.go, and it always puts the arguments in the same order.

Ok, I didn't know the contexts. I see some UT fails in the monitorFakeExec/monitorFakeCmd when I removed the -W flag, this function CombinedOutput checks the mfc.args[2] != WaitIntervalString || mfc.args[3] != WaitIntervalUsecondsValue, so I made these changes. I can revert these changes, but I need to find a Linux machine to test it.

k8s-ci-robot · 2025-05-29T08:24:33Z

LGTM label has been added.

Git tree hash: c00bdddeeb3d38d9c44f6d6bd4f3dbdcc1d70869

Signed-off-by: cyclinder <kuocyclinder@gmail.com>

aojea · 2025-05-30T14:09:21Z

pkg/util/iptables/iptables.go

@@ -192,12 +192,6 @@ const WaitString = "-w"
 // WaitSecondsValue a constant for specifying the default wait seconds
 const WaitSecondsValue = "5"

-// WaitIntervalString a constant for specifying the wait interval flag


WaitIntervalMinVersion line 180 can also be removed but this can be a follow up, seems we are all set now

/lgtm

k8s-ci-robot · 2025-05-30T14:09:27Z

LGTM label has been added.

Git tree hash: 22f63bd86df41c5405262dbdc8cebbd5056500bf

aojea · 2025-06-01T19:09:03Z

/lgtm
/approve

We can also remove a constant #131961 (comment)

k8s-ci-robot · 2025-06-01T19:09:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, cyclinder

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/util/iptables/OWNERS~~ [aojea]

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

cyclinder · 2025-06-03T01:56:48Z

We can also remove a constant #131961 (comment)

I will open another PR to fix #131961 (comment) and #131961 (comment)

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 26, 2025

k8s-ci-robot requested review from robscott and thockin May 26, 2025 03:27

k8s-ci-robot assigned aojea, BenTheElder and dims May 26, 2025

cyclinder force-pushed the fix_proxy_unknown_option branch from dfe2c52 to d0c611b Compare May 26, 2025 04:51

aojea reviewed May 26, 2025

View reviewed changes

cyclinder force-pushed the fix_proxy_unknown_option branch from d0c611b to 858fd5e Compare May 26, 2025 09:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 26, 2025

k8s-ci-robot assigned danwinship May 26, 2025

cyclinder force-pushed the fix_proxy_unknown_option branch from 858fd5e to 70e18c2 Compare May 26, 2025 14:24

k8s-ci-robot requested review from BenTheElder, danwinship and dims May 28, 2025 13:56

cyclinder changed the title ~~fix: add version check for iptables wait interval flag compatibility~~ proxy: remove iptables wait interval flag May 28, 2025

cyclinder force-pushed the fix_proxy_unknown_option branch 2 times, most recently from cd4b3b2 to fc881da Compare May 28, 2025 14:29

aojea reviewed May 28, 2025

View reviewed changes

cyclinder force-pushed the fix_proxy_unknown_option branch from fc881da to 50fb132 Compare May 29, 2025 02:04

aojea reviewed May 29, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2025

proxy: remove iptables wait interval flag

0a96613

Signed-off-by: cyclinder <kuocyclinder@gmail.com>

cyclinder force-pushed the fix_proxy_unknown_option branch from 50fb132 to 0a96613 Compare May 30, 2025 08:21

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

k8s-ci-robot requested a review from aojea May 30, 2025 08:21

aojea reviewed May 30, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2025

k8s-ci-robot merged commit 7f13945 into kubernetes:master Jun 1, 2025
14 checks passed

k8s-ci-robot added this to the v1.34 milestone Jun 1, 2025

shaneutt mentioned this pull request Jun 5, 2025

kube-proxy uses deprecated --wait-interval option #132006

Open

proxy: remove iptables wait interval flag #131961

proxy: remove iptables wait interval flag #131961

Uh oh!

Conversation

cyclinder commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

Uh oh!

k8s-ci-robot commented May 26, 2025

Uh oh!

pacoxu commented May 26, 2025

Uh oh!

pacoxu commented May 26, 2025

Uh oh!

pacoxu commented May 26, 2025

Uh oh!

cyclinder commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aojea commented May 26, 2025

Uh oh!

aojea commented May 26, 2025

Uh oh!

aojea commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aojea commented May 26, 2025

Uh oh!

BenTheElder commented May 28, 2025

Uh oh!

BenTheElder commented May 28, 2025

Uh oh!

aojea commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenTheElder commented May 28, 2025

Uh oh!

aojea commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented May 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented May 30, 2025

Uh oh!

aojea commented Jun 1, 2025

Uh oh!

k8s-ci-robot commented Jun 1, 2025

Uh oh!

Uh oh!

cyclinder commented Jun 3, 2025

Uh oh!

Uh oh!

cyclinder commented May 26, 2025 •

edited

Loading

cyclinder commented May 26, 2025 •

edited

Loading

aojea commented May 26, 2025 •

edited

Loading