Repository: kubernetes/kubernetes (language: go)

Issue:
kube-proxy should periodically resync iptables to recover from unexpected drift
### What would you like to be added?

kube-proxy performs a periodic full resync of iptables to correct unexpected drift between expected and actual iptables state.

### Why is this needed?

Today, kube-proxy performs a full resync of iptables only under certain conditions. In AKS, we have sometimes seen cases where iptables rules are accidentally modified (e.g. by daemonsets installed by a customer). Without some trigger (like a restart or endpoints created/deleted), kube-proxy won't detect or correct the drift between the actual/expected state of iptables.

A specific scenario in which this can happen:
1. iptables monitor polls until the canary chain in the mangle table is flushed, but doesn't check the canary chains in NAT or filter table: https://github.com/kubernetes/kubernetes/blob/d001d5684e69b08e120dc328917925318c5f324c/pkg/util/iptables/iptables.go#L586
2. Someone accidentally flushes the NAT table (`iptables -t nat -X`), but not the mangle table.


Discussion / hints:
/sig network
Discussed in Slack: https://kubernetes.slack.com/archives/C09QYUH5W/p1730303453175959

The current assumption in kube-proxy is that people should avoid modifying iptables in a way that breaks kube-proxy.

This issue is to discuss whether it's worth adding a periodic resync (say once an hour) to ensure that kube-proxy self-recovers eventually if this assumption is violated. 
This is an interesting topic, so kube-proxy reconciles based on events triggered by Services and Endpoints... 

iptables mode also reconciles on an event triggered by a missing canary chain, but basically because as Dan explains in the slack thread iptables maintain a single global state for all rules and chains, so it is not uncommon to have unintended interactions between different process modifying the rules. nftables allows for per-table state, meaning each table has its own independent state that reduces the risk of unintended interactions.

Checking the code I can see

https://github.com/kubernetes/kubernetes/blob/d001d5684e69b08e120dc328917925318c5f324c/pkg/proxy/iptables/proxier.go#L325-L328

and 

https://github.com/kubernetes/kubernetes/blob/d001d5684e69b08e120dc328917925318c5f324c/pkg/proxy/nftables/proxier.go#L279-L280

is not NewBoundedFrequencyRunner already running after 1 hour if nothing happens?
if is running, is the problem we are not doing a full sync during this execution?

/cc @danwinship 
as he is the most knowledgeable of this area 

> is not NewBoundedFrequencyRunner already running after 1 hour if nothing happens?
if is running, is the problem we are not doing a full sync during this execution?

Yes, exactly. `proxy.syncRunner` invokes `proxier.syncProxyRules` every 1hr. However, `syncProxyRules` won't perform a full sync unless `proxier.needFullSync` is true
https://github.com/kubernetes/kubernetes/blob/a499facee693a1a83daadb82d88f7b51d324ffc5/pkg/proxy/iptables/proxier.go#L811

In contrast, `ipt.Monitor` calls `proxier.forceSyncProxyRules` when it detects that the mangle canary chain has been deleted
https://github.com/kubernetes/kubernetes/blob/a499facee693a1a83daadb82d88f7b51d324ffc5/pkg/proxy/iptables/proxier.go#L330-L331

which does a full sync
https://github.com/kubernetes/kubernetes/blob/a499facee693a1a83daadb82d88f7b51d324ffc5/pkg/proxy/iptables/proxier.go#L788-L794
Yeah, so the history is:

- pre-[`iptables.Monitor`](https://github.com/kubernetes/kubernetes/pull/81517):
  - kube-proxy queues a complete ruleset rewrite:
    - at startup as soon as it has synced with the apiserver
    - within 1 second of any Service or endpoint changing
    - within 1 second of the node topology labels changing
    - at least once every 30 seconds even if nothing changes.

- post-`iptables.Monitor`, pre-[`MinimizeIPTablesRestore`](https://github.com/kubernetes/kubernetes/pull/110268):
  - kube-proxy queues a complete ruleset rewrite:
    - at startup as soon as it has synced with the apiserver
    - within 1 second of any Service or endpoint changing
    - within 1 second of the node topology labels changing
    - within 30 seconds of a complete iptables ruleset flush
    - at least once an hour even if nothing changes.

- post-`MinimizeIPTablesRestore`:
  - kube-proxy queues a complete ruleset rewrite:
    - at startup as soon as it has synced with the apiserver
    - within 1 second of the node topology labels changing
    - within 30 seconds of a complete iptables ruleset flush
  - kube-proxy queues a partial sync for only changed Services and endpoints:
    - within 1 second of any Service or endpoint changing
    - at least once an hour even if nothing changes.

(where "1 second" means "`MinSyncPeriod`", "30 seconds" means "`SyncPeriod`", and "one hour" is not configurable.)

So the "at least once an hour" sync is now mostly a no-op. It should have ended up in the "rewrites every rule" pile rather than the "rewrites changed rules" pile, but it didn't.
> So the "at least once an hour" sync is now mostly a no-op. It should have ended up in the "rewrites every rule" pile rather than the "rewrites changed rules" pile, but it didn't.

I think is reasonable to expect that the after one hour of inactivity we do a full resync, I defer to Dan to ack to accept this feature, I'm +1 on changing this behavior to do a full sync after the hour
oh, yeah, I definitely think we should fix this. I didn't notice it was still needs-triage

/triage accepted

can let me try? I raised an [issue](https://github.com/kubernetes/kubernetes/issues/128879) about iptables mode  a while ago, and I want to learn more about kube-proxy iptables mode based on this issue.
@olderTaoist Sure! The Kubernetes Contributor website has a bunch of [docs on how to get started](https://www.kubernetes.dev/docs/); if you have more specific questions on where to start or what approach to take for this particular bug, you can ask here.
/assign
We can experiment with netlink to see if the canary chain exists in all the tables, that shouldn't have any performance penalty.

I couldn't find any API to read iptables rules, maybe we can use nftables netlink API to read rules created by iptables-nft.
@danwinship @aojea  We can experiment with https://github.com/google/nftables 

nftables can monitor changes on the objects https://wiki.nftables.org/wiki-nftables/index.php/Monitoring_ruleset_updates
I don't think we want to make the iptables backend _require_ iptables-nft rather than iptables-legacy.

FWIW recent kernels introduce the `owned` and `persistent` flags for nftables tables, which makes it so that the table is owned by a single process and no other processes can modify it. So eventually we'll be able to depend on that in the nftables backend.

Fix patch:
diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go
index da61d13124820..5649a88b93666 100644
--- a/pkg/proxy/iptables/proxier.go
+++ b/pkg/proxy/iptables/proxier.go
@@ -159,6 +159,7 @@ type Proxier struct {
 	// updating iptables with some partial data after kube-proxy restart.
 	endpointSlicesSynced bool
 	servicesSynced       bool
+	lastFullSync         time.Time
 	needFullSync         bool
 	initialized          int32
 	syncRunner           *async.BoundedFrequencyRunner // governs calls to syncProxyRules
@@ -325,7 +326,7 @@ func NewProxier(ctx context.Context,
 	// We pass syncPeriod to ipt.Monitor, which will call us only if it needs to.
 	// We need to pass *some* maxInterval to NewBoundedFrequencyRunner anyway though.
 	// time.Hour is arbitrary.
-	proxier.syncRunner = async.NewBoundedFrequencyRunner("sync-runner", proxier.syncProxyRules, minSyncPeriod, time.Hour, burstSyncs)
+	proxier.syncRunner = async.NewBoundedFrequencyRunner("sync-runner", proxier.syncProxyRules, minSyncPeriod, proxyutil.FullSyncPeriod, burstSyncs)
 
 	go ipt.Monitor(kubeProxyCanaryChain, []utiliptables.Table{utiliptables.TableMangle, utiliptables.TableNAT, utiliptables.TableFilter},
 		proxier.forceSyncProxyRules, syncPeriod, wait.NeverStop)
@@ -541,6 +542,7 @@ func (proxier *Proxier) SyncLoop() {
 	// synthesize "last change queued" time as the informers are syncing.
 	metrics.SyncProxyRulesLastQueuedTimestamp.WithLabelValues(string(proxier.ipFamily)).SetToCurrentTime()
 	proxier.syncRunner.Loop(wait.NeverStop)
+
 }
 
 func (proxier *Proxier) setInitialized(value bool) {
@@ -806,15 +808,14 @@ func (proxier *Proxier) syncProxyRules() {
 		return
 	}
 
-	// The value of proxier.needFullSync may change before the defer funcs run, so
-	// we need to keep track of whether it was set at the *start* of the sync.
-	tryPartialSync := !proxier.needFullSync
-
 	// Keep track of how long syncs take.
 	start := time.Now()
+
+	doFullSync := proxier.needFullSync || (time.Since(proxier.lastFullSync) > proxyutil.FullSyncPeriod)
+
 	defer func() {
 		metrics.SyncProxyRulesLatency.WithLabelValues(string(proxier.ipFamily)).Observe(metrics.SinceInSeconds(start))
-		if tryPartialSync {
+		if !doFullSync {
 			metrics.SyncPartialProxyRulesLatency.WithLabelValues(string(proxier.ipFamily)).Observe(metrics.SinceInSeconds(start))
 		} else {
 			metrics.SyncFullProxyRulesLatency.WithLabelValues(string(proxier.ipFamily)).Observe(metrics.SinceInSeconds(start))
@@ -825,24 +826,26 @@ func (proxier *Proxier) syncProxyRules() {
 	serviceUpdateResult := proxier.svcPortMap.Update(proxier.serviceChanges)
 	endpointUpdateResult := proxier.endpointsMap.Update(proxier.endpointsChanges)
 
-	proxier.logger.V(2).Info("Syncing iptables rules")
+	proxier.logger.V(2).Info("Syncing iptables rules", "fullSync", doFullSync)
 
 	success := false
 	defer func() {
 		if !success {
 			proxier.logger.Info("Sync failed", "retryingTime", proxier.syncPeriod)
 			proxier.syncRunner.RetryAfter(proxier.syncPeriod)
-			if tryPartialSync {
+			if !doFullSync {
 				metrics.IPTablesPartialRestoreFailuresTotal.WithLabelValues(string(proxier.ipFamily)).Inc()
 			}
 			// proxier.serviceChanges and proxier.endpointChanges have already
 			// been flushed, so we've lost the state needed to be able to do
 			// a partial sync.
 			proxier.needFullSync = true
+		} else if doFullSync {
+			proxier.lastFullSync = time.Now()
 		}
 	}()
 
-	if !tryPartialSync {
+	if doFullSync {
 		// Ensure that our jump rules (eg from PREROUTING to KUBE-SERVICES) exist.
 		// We can't do this as part of the iptables-restore because we don't want
 		// to specify/replace *all* of the rules in PREROUTING, etc.
@@ -1235,7 +1238,7 @@ func (proxier *Proxier) syncProxyRules() {
 		// then we can omit them from the restore input. 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 && !serviceUpdateResult.UpdatedServices.Has(svcName.NamespacedName) && !endpointUpdateResult.UpdatedServices.Has(svcName.NamespacedName) {
+		if !doFullSync && !serviceUpdateResult.UpdatedServices.Has(svcName.NamespacedName) && !endpointUpdateResult.UpdatedServices.Has(svcName.NamespacedName) {
 			natChains = skippedNatChains
 			natRules = skippedNatRules
 		}
diff --git a/pkg/proxy/nftables/proxier.go b/pkg/proxy/nftables/proxier.go
index db993dab5ade8..16a78dba78f2d 100644
--- a/pkg/proxy/nftables/proxier.go
+++ b/pkg/proxy/nftables/proxier.go
@@ -163,6 +163,7 @@ type Proxier struct {
 	// updating nftables with some partial data after kube-proxy restart.
 	endpointSlicesSynced bool
 	servicesSynced       bool
+	lastFullSync         time.Time
 	needFullSync         bool
 	initialized          int32
 	syncRunner           *async.BoundedFrequencyRunner // governs calls to syncProxyRules
@@ -281,7 +282,7 @@ func NewProxier(ctx context.Context,
 	burstSyncs := 2
 	logger.V(2).Info("NFTables sync params", "minSyncPeriod", minSyncPeriod, "syncPeriod", syncPeriod, "burstSyncs", burstSyncs)
 	// We need to pass *some* maxInterval to NewBoundedFrequencyRunner. time.Hour is arbitrary.
-	proxier.syncRunner = async.NewBoundedFrequencyRunner("sync-runner", proxier.syncProxyRules, minSyncPeriod, time.Hour, burstSyncs)
+	proxier.syncRunner = async.NewBoundedFrequencyRunner("sync-runner", proxier.syncProxyRules, minSyncPeriod, proxyutil.FullSyncPeriod, burstSyncs)
 
 	return proxier, nil
 }
@@ -1167,19 +1168,18 @@ func (proxier *Proxier) syncProxyRules() {
 		return
 	}
 
+	// Keep track of how long syncs take.
+	start := time.Now()
+
 	//
 	// Below this point we will not return until we try to write the nftables rules.
 	//
 
-	// The value of proxier.needFullSync may change before the defer funcs run, so
-	// we need to keep track of whether it was set at the *start* of the sync.
-	tryPartialSync := !proxier.needFullSync
+	doFullSync := proxier.needFullSync || (time.Since(proxier.lastFullSync) > proxyutil.FullSyncPeriod)
 
-	// Keep track of how long syncs take.
-	start := time.Now()
 	defer func() {
 		metrics.SyncProxyRulesLatency.WithLabelValues(string(proxier.ipFamily)).Observe(metrics.SinceInSeconds(start))
-		if tryPartialSync {
+		if !doFullSync {
 			metrics.SyncPartialProxyRulesLatency.WithLabelValues(string(proxier.ipFamily)).Observe(metrics.SinceInSeconds(start))
 		} else {
 			metrics.SyncFullProxyRulesLatency.WithLabelValues(string(proxier.ipFamily)).Observe(metrics.SinceInSeconds(start))
@@ -1190,7 +1190,7 @@ func (proxier *Proxier) syncProxyRules() {
 	serviceUpdateResult := proxier.svcPortMap.Update(proxier.serviceChanges)
 	endpointUpdateResult := proxier.endpointsMap.Update(proxier.endpointsChanges)
 
-	proxier.logger.V(2).Info("Syncing nftables rules")
+	proxier.logger.V(2).Info("Syncing nftables rules", "fullSync", doFullSync)
 
 	success := false
 	defer func() {
@@ -1201,6 +1201,8 @@ func (proxier *Proxier) syncProxyRules() {
 			// been flushed, so we've lost the state needed to be able to do
 			// a partial sync.
 			proxier.needFullSync = true
+		} else if doFullSync {
+			proxier.lastFullSync = time.Now()
 		}
 	}()
 
@@ -1228,7 +1230,7 @@ func (proxier *Proxier) syncProxyRules() {
 				// (with a later timestamp) at the end of the sync.
 				proxier.logger.Error(err, "Unable to delete stale chains; will retry later")
 				metrics.NFTablesCleanupFailuresTotal.WithLabelValues(string(proxier.ipFamily)).Inc()
-				tryPartialSync = false
+				doFullSync = true
 
 				// Log failed transaction and list full kube-proxy table.
 				proxier.logFailure(tx)
@@ -1238,7 +1240,7 @@ func (proxier *Proxier) syncProxyRules() {
 
 	// Now start the actual syncing transaction
 	tx := proxier.nftables.NewTransaction()
-	if !tryPartialSync {
+	if doFullSync {
 		proxier.setupNFTables(tx)
 	}
 
@@ -1286,7 +1288,7 @@ func (proxier *Proxier) syncProxyRules() {
 
 		// skipServiceUpdate is used for all service-related chains and their elements.
 		// If no changes were done to the service or its endpoints, these objects may be skipped.
-		skipServiceUpdate := tryPartialSync &&
+		skipServiceUpdate := !doFullSync &&
 			!serviceUpdateResult.UpdatedServices.Has(svcName.NamespacedName) &&
 			!endpointUpdateResult.UpdatedServices.Has(svcName.NamespacedName)
 
diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go
index d9b13f9a18e84..4aa642b2ebb37 100644
--- a/pkg/proxy/util/utils.go
+++ b/pkg/proxy/util/utils.go
@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"net"
 	"strings"
+	"time"
 
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -37,6 +38,9 @@ const (
 
 	// IPv6ZeroCIDR is the CIDR block for the whole IPv6 address space
 	IPv6ZeroCIDR = "::/0"
+
+	// FullSyncPeriod is iptables and nftables proxier full sync period
+	FullSyncPeriod = 1 * time.Hour
 )
 
 // IsZeroCIDR checks whether the input CIDR string is either


Test patch: