Merge pull request #36833 from mandarjog/issue_36652

Automatic merge from submit-queue

Handle Empty clusterCIDR

**What this PR does / why we need it**:
Handles empty clusterCIDR by skipping the corresponding rule.

**Which issue this PR fixes** 
fixes #36652

**Special notes for your reviewer**:
1. Added test to check for presence/absence of XLB to SVC rule
2. Changed an error statement to log rules along with the error string in case of a failure; This ensures that full debug info is available in case of iptables-restore errors.


Empty clusterCIDR causes invalid rules generation.
Fixes issue #36652

pkg/proxy/iptables/proxier.go

@@ -250,6 +250,10 @@ func NewProxier(ipt utiliptables.Interface, sysctl utilsysctl.Interface, exec ut
 		nodeIP = net.ParseIP("127.0.0.1")
 	}
 
+	if len(clusterCIDR) == 0 {
+		glog.Warningf("clusterCIDR not specified, unable to distinguish between internal and external traffic")
+	}
+
 	go healthcheck.Run()
 
 	var throttle flowcontrol.RateLimiter
@@ -1221,13 +1225,17 @@ func (proxier *Proxier) syncProxyRules() {
 		}
 		// First rule in the chain redirects all pod -> external vip traffic to the
 		// Service's ClusterIP instead. This happens whether or not we have local
-		// endpoints.
+		// endpoints; only if clusterCIDR is specified
-		args = []string{
+		if len(proxier.clusterCIDR) > 0 {
-			"-A", string(svcXlbChain),
+			args = []string{
-			"-m", "comment", "--comment",
+				"-A", string(svcXlbChain),
-			fmt.Sprintf(`"Redirect pods trying to reach external loadbalancer VIP to clusterIP"`),
+				"-m", "comment", "--comment",
+				fmt.Sprintf(`"Redirect pods trying to reach external loadbalancer VIP to clusterIP"`),
+				"-s", proxier.clusterCIDR,
+				"-j", string(svcChain),
+			}
+			writeLine(natRules, args...)
 		}
-		writeLine(natRules, append(args, "-s", proxier.clusterCIDR, "-j", string(svcChain))...)
 
 		numLocalEndpoints := len(localEndpointChains)
 		if numLocalEndpoints == 0 {
@@ -1300,7 +1308,7 @@ func (proxier *Proxier) syncProxyRules() {
 	glog.V(3).Infof("Restoring iptables rules: %s", lines)
 	err = proxier.iptables.RestoreAll(lines, utiliptables.NoFlushTables, utiliptables.RestoreCounters)
 	if err != nil {
-		glog.Errorf("Failed to execute iptables-restore: %v", err)
+		glog.Errorf("Failed to execute iptables-restore: %v\nRules:\n%s", err, lines)
 		// Revert new local ports.
 		revertPorts(replacementPortsMap, proxier.portsMap)
 		return

pkg/proxy/iptables/proxier_test.go

@@ -696,9 +696,22 @@ func TestOnlyLocalLoadBalancing(t *testing.T) {
 	}
 }
 
+func TestOnlyLocalNodePortsNoClusterCIDR(t *testing.T) {
+	ipt := iptablestest.NewFake()
+	fp := NewFakeProxier(ipt)
+	// set cluster CIDR to empty before test
+	fp.clusterCIDR = ""
+	onlyLocalNodePorts(t, fp, ipt)
+}
+
 func TestOnlyLocalNodePorts(t *testing.T) {
 	ipt := iptablestest.NewFake()
 	fp := NewFakeProxier(ipt)
+	onlyLocalNodePorts(t, fp, ipt)
+}
+
+func onlyLocalNodePorts(t *testing.T, fp *Proxier, ipt *iptablestest.FakeIPTables) {
+	shouldLBTOSVCRuleExist := len(fp.clusterCIDR) > 0
 	svcName := "svc1"
 	svcIP := net.IPv4(10, 20, 30, 41)
 
@@ -724,10 +737,18 @@ func TestOnlyLocalNodePorts(t *testing.T) {
 		errorf(fmt.Sprintf("Failed to find jump to lb chain %v", lbChain), kubeNodePortRules, t)
 	}
 
+	svcChain := string(servicePortChainName(svc, strings.ToLower(string(api.ProtocolTCP))))
 	lbRules := ipt.GetRules(lbChain)
 	if hasJump(lbRules, nonLocalEpChain, "", "") {
 		errorf(fmt.Sprintf("Found jump from lb chain %v to non-local ep %v", lbChain, nonLocalEp), lbRules, t)
 	}
+	if hasJump(lbRules, svcChain, "", "") != shouldLBTOSVCRuleExist {
+		prefix := "Did not find "
+		if !shouldLBTOSVCRuleExist {
+			prefix = "Found "
+		}
+		errorf(fmt.Sprintf("%s jump from lb chain %v to svc %v", prefix, lbChain, svcChain), lbRules, t)
+	}
 	if !hasJump(lbRules, localEpChain, "", "") {
 		errorf(fmt.Sprintf("Didn't find jump from lb chain %v to local ep %v", lbChain, nonLocalEp), lbRules, t)
 	}