Fix race condition in iptables partial sync handling #122204

danwinship · 2023-12-06T18:05:55Z

What type of PR is this?

/kind bug
/kind regression

What this PR does / why we need it:

Fixes a race condition in the iptables proxy partial sync handling; the methods that process apiserver events (proxier.OnServiceUpdated etc) rely on the change trackers' mutexes rather than the proxier's mutexes (since they don't use any mutable variables in the proxier), but syncProxyRules currently makes two separate calls into the change trackers, PendingUpdates() and Update(), so it's possible that an event could arrive between them, causing the two results to reflect inconsistent state. (#121362 (comment))

Which issue(s) this PR fixes:

Fixes #121362

Special notes for your reviewer:

This does not add any regression test since the bug is a race condition and I don't think there's any easy way we could force it?

Does this PR introduce a user-facing change?

Fixes a race condition in the iptables mode of kube-proxy in 1.27 and later
that could result in some updates getting lost (e.g., when a service gets a
new endpoint, the rules for the new endpoint might not be added until
much later).

/sig network
/priority important-soon
cc @aojea @juliantaylor

k8s-ci-robot · 2023-12-06T18:06:00Z

Please note that we're already in Test Freeze for the release-1.29 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.29.0 release.

Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Wed Dec 6 10:11:59 UTC 2023.

k8s-ci-robot · 2023-12-06T18:06:03Z

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-ci-robot · 2023-12-06T18:06:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship

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/proxy/OWNERS~~ [danwinship]

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

aojea · 2023-12-10T14:26:57Z

pkg/proxy/endpointslicecache.go

-func (cache *EndpointSliceCache) checkoutChanges() []*endpointsChange {
-	changes := []*endpointsChange{}
+func (cache *EndpointSliceCache) checkoutChanges() map[types.NamespacedName]*endpointsChange {
+	changes := make(map[types.NamespacedName]*endpointsChange)


this is obtained from trackerByServiceMap so there is no risk of collapsing two different changes associated to the same service

aojea · 2023-12-10T14:39:36Z

pkg/proxy/endpoints_test.go

-			for i, change := range changes {
-				expectedChange := tc.expectedChanges[i]
+			for _, change := range changes {
+				// The test only supports 0 or 1 changes, so if we're here,


I didn't check in detail , but is this because the test can not support more than 1 change or existing testcases only have 0 or 1?

aojea · 2023-12-10T14:40:08Z

pkg/proxy/iptables/proxier.go

@@ -1207,7 +1201,7 @@ func (proxier *Proxier) syncProxyRules() {
 		// them in activeNATChains, so they won't get deleted.) However, we have
 		// to still figure out how many chains we _would_ have written to make the
 		// metrics come out right, so we just compute them and throw them away.
-		if tryPartialSync && !serviceChanged.Has(svcName.NamespacedName.String()) && !endpointsChanged.Has(svcName.NamespacedName.String()) {
+		if tryPartialSync && !serviceUpdateResult.UpdatedServices.Has(svcName.NamespacedName) && !endpointUpdateResult.UpdatedServices.Has(svcName.NamespacedName) {


this seems much better and easier to think about

aojea · 2023-12-10T14:42:05Z

only one question about one test, but LGTM

aojea · 2023-12-18T22:18:06Z

/lgtm

k8s-ci-robot · 2023-12-18T22:18:14Z

LGTM label has been added.

Git tree hash: d37505ac6dbd12087479890dcc2c5a76946b3872

aojea · 2023-12-19T20:46:49Z

@danwinship the bot says it needs rebase

danwinship · 2023-12-19T23:27:17Z

it doesn't; the tag is stale...
maybe if I rebase and repush it will notice this time...

ServicePortMap.Update() and EndpointsMap.Update() were just a tiny wrappers around the corresponding apply() methods, which had no other callers. So squash them together. (Also fix the variable naming in ServicePortMap.Update() to match other methods.)

…ults This fixes a race condition where the tracker could be updated in between us calling .PendingChanges() and .Update().

danwinship · 2023-12-20T12:07:08Z

/retest-required

aojea · 2023-12-20T12:35:13Z

/lgtm

k8s-ci-robot · 2023-12-20T12:35:21Z

LGTM label has been added.

Git tree hash: b3c85bffa5dd1428437dc6809eece403275fd28f

danwinship · 2023-12-20T16:55:52Z

/retest-required

rikatz · 2024-01-13T13:03:53Z

/cherrypick release-1.28

…04-upstream-release-1.27 Automated cherry pick of #122204: Fix race condition in iptables partial sync handling

…04-upstream-release-1.28 Cherry pick of #122204: Fix race condition in iptables partial sync handling

…04-upstream-release-1.29 Automated cherry pick of #122204: Fix race condition in iptables partial sync handling

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kube-proxy labels Dec 6, 2023

k8s-ci-robot requested review from andrewsykim and dcbw December 6, 2023 18:06

danwinship force-pushed the proxy-pending-changes branch from 902aaf3 to abbcd67 Compare December 6, 2023 23:54

aojea reviewed Dec 10, 2023

View reviewed changes

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

danwinship force-pushed the proxy-pending-changes branch from abbcd67 to 0c1ffec Compare December 18, 2023 22:09

k8s-ci-robot assigned aojea Dec 18, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 18, 2023

danwinship mentioned this pull request Dec 19, 2023

Add a hack to iptables kube-proxy to work around certain missed updates #122048

Closed

Drop PendingChanges methods from change trackers, move into UpdateRes…

626f349

…ults This fixes a race condition where the tracker could be updated in between us calling .PendingChanges() and .Update().

danwinship force-pushed the proxy-pending-changes branch from 0c1ffec to 626f349 Compare December 19, 2023 23:27

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 19, 2023

k8s-ci-robot requested a review from aojea December 19, 2023 23:27

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2023

k8s-ci-robot merged commit 247ea20 into kubernetes:master Dec 20, 2023
14 checks passed

k8s-ci-robot added this to the v1.30 milestone Dec 20, 2023

danwinship deleted the proxy-pending-changes branch December 20, 2023 20:29

This was referenced Jan 13, 2024

Automated cherry pick of #122204: Fix race condition in iptables partial sync handling #122756

Merged

Cherry pick of #122204: Fix race condition in iptables partial sync handling #122757

Merged

wedaly mentioned this pull request Jan 16, 2024

Automated cherry pick of #122204: Fix race condition in iptables partial sync handling #122815

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 8, 2024

Merge pull request #122815 from wedaly/automated-cherry-pick-of-#1222…

593d5ec

…04-upstream-release-1.27 Automated cherry pick of #122204: Fix race condition in iptables partial sync handling

k8s-ci-robot added a commit that referenced this pull request Feb 9, 2024

Merge pull request #122757 from hakman/automated-cherry-pick-of-#1222…

ce411eb

…04-upstream-release-1.28 Cherry pick of #122204: Fix race condition in iptables partial sync handling

k8s-ci-robot added a commit that referenced this pull request Feb 9, 2024

Merge pull request #122756 from hakman/automated-cherry-pick-of-#1222…

97d1f81

…04-upstream-release-1.29 Automated cherry pick of #122204: Fix race condition in iptables partial sync handling

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix race condition in iptables partial sync handling #122204

Fix race condition in iptables partial sync handling #122204

danwinship commented Dec 6, 2023

k8s-ci-robot commented Dec 6, 2023

k8s-ci-robot commented Dec 6, 2023

k8s-ci-robot commented Dec 6, 2023

aojea Dec 10, 2023

aojea Dec 10, 2023

danwinship Dec 18, 2023

aojea Dec 10, 2023

aojea commented Dec 10, 2023

aojea commented Dec 18, 2023

k8s-ci-robot commented Dec 18, 2023

aojea commented Dec 19, 2023

danwinship commented Dec 19, 2023

danwinship commented Dec 20, 2023

aojea commented Dec 20, 2023

k8s-ci-robot commented Dec 20, 2023

danwinship commented Dec 20, 2023

rikatz commented Jan 13, 2024

Fix race condition in iptables partial sync handling #122204

Fix race condition in iptables partial sync handling #122204

Conversation

danwinship commented Dec 6, 2023

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 Dec 6, 2023

k8s-ci-robot commented Dec 6, 2023

k8s-ci-robot commented Dec 6, 2023

aojea Dec 10, 2023

Choose a reason for hiding this comment

aojea Dec 10, 2023

Choose a reason for hiding this comment

danwinship Dec 18, 2023

Choose a reason for hiding this comment

aojea Dec 10, 2023

Choose a reason for hiding this comment

aojea commented Dec 10, 2023

aojea commented Dec 18, 2023

k8s-ci-robot commented Dec 18, 2023

aojea commented Dec 19, 2023

danwinship commented Dec 19, 2023

danwinship commented Dec 20, 2023

aojea commented Dec 20, 2023

k8s-ci-robot commented Dec 20, 2023

danwinship commented Dec 20, 2023

rikatz commented Jan 13, 2024