From 37ada4b04f4a21daf0e3628c63226531b51ee6bf Mon Sep 17 00:00:00 2001
From: Dan Winship <danwinship@redhat.com>
Date: Fri, 7 Jan 2022 14:17:11 -0500
Subject: [PATCH] proxy/iptables: Don't create unused chains, and enable the
 unit test for that

---
 pkg/proxy/iptables/proxier.go      |  2 +-
 pkg/proxy/iptables/proxier_test.go | 32 +++++++++++++++++++++---------
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go
index 0855096c1b1..341004e2e3f 100644
--- a/pkg/proxy/iptables/proxier.go
+++ b/pkg/proxy/iptables/proxier.go
@@ -1101,7 +1101,7 @@ func (proxier *Proxier) syncProxyRules() {
 		}
 
 		svcXlbChain := svcInfo.serviceLBChainName
-		if svcInfo.NodeLocalExternal() {
+		if hasEndpoints && svcInfo.NodeLocalExternal() {
 			// Only for services request OnlyLocal traffic
 			// create the per-service LB chain, retaining counters if possible.
 			if lbChain, ok := existingNATChains[svcXlbChain]; ok {
diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go
index d0d93a03ab0..8c19ce939a4 100644
--- a/pkg/proxy/iptables/proxier_test.go
+++ b/pkg/proxy/iptables/proxier_test.go
@@ -721,6 +721,10 @@ func checkIPTablesRuleJumps(ruleData string) error {
 		// Find all of the lines like ":KUBE-SERVICES", indicating chains that
 		// iptables-restore would create when loading the data.
 		createdChains := sets.NewString(findAllMatches(lines, `^:([^ ]*)`)...)
+		// Find all of the lines like "-X KUBE-SERVICES ..." indicating chains
+		// that we are deleting because they are no longer used, and remove
+		// those chains from createdChains.
+		createdChains = createdChains.Delete(findAllMatches(lines, `-X ([^ ]*)`)...)
 
 		// Find all of the lines like "-A KUBE-SERVICES ..." indicating chains
 		// that we are adding at least one rule to.
@@ -754,12 +758,11 @@ func checkIPTablesRuleJumps(ruleData string) error {
 
 		// Find cases where we have ":BAR" but no "-A FOO ... -j BAR", meaning
 		// that we are creating an empty chain but not using it for anything.
-		// FIXME: This currently fails
-		// extraChains := createdChains.Difference(jumpedChains)
-		// extraChains.Delete(string(kubeServicesChain), string(kubeExternalServicesChain), string(kubeNodePortsChain), string(kubePostroutingChain), string(kubeForwardChain), string(KubeMarkMasqChain))
-		// if len(extraChains) > 0 {
-		// 	return fmt.Errorf("some chains in %s are created but not used: %v", tableName, extraChains.List())
-		// }
+		extraChains := createdChains.Difference(jumpedChains)
+		extraChains.Delete(string(kubeServicesChain), string(kubeExternalServicesChain), string(kubeNodePortsChain), string(kubePostroutingChain), string(kubeForwardChain), string(KubeMarkMasqChain))
+		if len(extraChains) > 0 {
+			return fmt.Errorf("some chains in %s are created but not used: %v", tableName, extraChains.List())
+		}
 	}
 
 	return nil
@@ -826,6 +829,20 @@ COMMIT
 `,
 			error: "some chains in nat are used but were not created: [KUBE-SVC-XPGD46QRK7WJZT7O]",
 		},
+		{
+			name: "can't create chain and then not use it",
+			input: `
+*filter
+COMMIT
+*nat
+:KUBE-MARK-MASQ - [0:0]
+:KUBE-SERVICES - [0:0]
+:KUBE-SVC-XPGD46QRK7WJZT7O - [0:0]
+-A KUBE-SERVICES -m comment --comment "ns1/svc1:p80 cluster IP" ...
+COMMIT
+`,
+			error: "some chains in nat are created but not used: [KUBE-SVC-XPGD46QRK7WJZT7O]",
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			err := checkIPTablesRuleJumps(tc.input)
@@ -1899,7 +1916,6 @@ COMMIT
 :KUBE-NODEPORTS - [0:0]
 :KUBE-POSTROUTING - [0:0]
 :KUBE-MARK-MASQ - [0:0]
-:KUBE-XLB-XPGD46QRK7WJZT7O - [0:0]
 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN
 -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000
 -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE
@@ -2289,7 +2305,6 @@ COMMIT
 :KUBE-NODEPORTS - [0:0]
 :KUBE-POSTROUTING - [0:0]
 :KUBE-MARK-MASQ - [0:0]
-:KUBE-XLB-XPGD46QRK7WJZT7O - [0:0]
 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN
 -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000
 -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE
@@ -5117,7 +5132,6 @@ COMMIT
 :KUBE-NODEPORTS - [0:0]
 :KUBE-POSTROUTING - [0:0]
 :KUBE-MARK-MASQ - [0:0]
-:KUBE-XLB-AQI2S6QIMU7PVVRP - [0:0]
 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN
 -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000
 -A KUBE-POSTROUTING -m comment --comment "kubernetes service traffic requiring SNAT" -j MASQUERADE