Merge pull request #108811 from danwinship/simplify-local-traffic-detector

pkg/proxy: Simplify LocalTrafficDetector

pkg/proxy/iptables/proxier.go

@@ -1117,7 +1117,9 @@ func (proxier *Proxier) syncProxyRules() {
 				// If/when we support "Local" policy for VIPs, we should update this.
 				proxier.natRules.Write(
 					"-A", string(svcChain),
-					proxier.localDetector.JumpIfNotLocal(args, string(KubeMarkMasqChain)))
+					args,
+					proxier.localDetector.IfNotLocal(),
+					"-j", string(KubeMarkMasqChain))
 			}
 			proxier.natRules.Write(
 				"-A", string(kubeServicesChain),
@@ -1157,7 +1159,9 @@ func (proxier *Proxier) syncProxyRules() {
 					if proxier.localDetector.IsImplemented() {
 						proxier.natRules.Write(
 							appendTo,
-							proxier.localDetector.JumpIfNotLocal(args, string(KubeMarkMasqChain)))
+							args,
+							proxier.localDetector.IfNotLocal(),
+							"-j", string(KubeMarkMasqChain))
 					} else {
 						proxier.natRules.Write(
 							appendTo,
@@ -1348,12 +1352,12 @@ func (proxier *Proxier) syncProxyRules() {
 		// Service's ClusterIP instead. This happens whether or not we have local
 		// endpoints; only if localDetector is implemented
 		if proxier.localDetector.IsImplemented() {
-			args = append(args[:0],
+			proxier.natRules.Write(
 				"-A", string(svcXlbChain),
 				"-m", "comment", "--comment",
 				`"Redirect pods trying to reach external loadbalancer VIP to clusterIP"`,
-			)
+				proxier.localDetector.IfLocal(),
-			proxier.natRules.Write(proxier.localDetector.JumpIfLocal(args, string(svcChain)))
+				"-j", string(svcChain))
 		}
 
 		// Next, redirect all src-type=LOCAL -> LB IP to the service chain for externalTrafficPolicy=Local

pkg/proxy/ipvs/proxier.go

@@ -1639,14 +1639,19 @@ func (proxier *Proxier) writeIptablesRules() {
 			"-m", "set", "--match-set", proxier.ipsetList[kubeClusterIPSet].Name,
 		)
 		if proxier.masqueradeAll {
-			proxier.natRules.Write(args, "dst,dst", "-j", string(KubeMarkMasqChain))
+			proxier.natRules.Write(
+				args, "dst,dst",
+				"-j", string(KubeMarkMasqChain))
 		} else if proxier.localDetector.IsImplemented() {
 			// This masquerades off-cluster traffic to a service VIP.  The idea
 			// is that you can establish a static route for your Service range,
 			// routing to any node, and that node will bridge into the Service
 			// for you.  Since that might bounce off-node, we masquerade here.
 			// If/when we support "Local" policy for VIPs, we should update this.
-			proxier.natRules.Write(proxier.localDetector.JumpIfNotLocal(append(args, "dst,dst"), string(KubeMarkMasqChain)))
+			proxier.natRules.Write(
+				args, "dst,dst",
+				proxier.localDetector.IfNotLocal(),
+				"-j", string(KubeMarkMasqChain))
 		} else {
 			// Masquerade all OUTPUT traffic coming from a service ip.
 			// The kube dummy interface has all service VIPs assigned which
@@ -1655,7 +1660,9 @@ func (proxier *Proxier) writeIptablesRules() {
 			// VIP:<service port>.
 			// Always masquerading OUTPUT (node-originating) traffic with a VIP
 			// source ip and service port destination fixes the outgoing connections.
-			proxier.natRules.Write(args, "src,dst", "-j", string(KubeMarkMasqChain))
+			proxier.natRules.Write(
+				args, "src,dst",
+				"-j", string(KubeMarkMasqChain))
 		}
 	}
 

pkg/proxy/util/iptables/traffic.go

@@ -19,7 +19,6 @@ package iptables
 import (
 	"fmt"
 
-	"k8s.io/klog/v2"
 	utiliptables "k8s.io/kubernetes/pkg/util/iptables"
 	netutils "k8s.io/utils/net"
 )
@@ -30,13 +29,11 @@ type LocalTrafficDetector interface {
 	// IsImplemented returns true if the implementation does something, false otherwise
 	IsImplemented() bool
 
-	// JumpIfLocal appends conditions to jump to a target chain if traffic detected to be
+	// IfLocal returns iptables arguments that will match traffic from a pod
-	// of local origin
+	IfLocal() []string
-	JumpIfLocal(args []string, toChain string) []string
 
-	// JumpINotfLocal appends conditions to jump to a target chain if traffic detected not to be
+	// IfNotLocal returns iptables arguments that will match traffic that is not from a pod
-	// of local origin
+	IfNotLocal() []string
-	JumpIfNotLocal(args []string, toChain string) []string
 }
 
 type noOpLocalDetector struct{}
@@ -50,16 +47,17 @@ func (n *noOpLocalDetector) IsImplemented() bool {
 	return false
 }
 
-func (n *noOpLocalDetector) JumpIfLocal(args []string, toChain string) []string {
+func (n *noOpLocalDetector) IfLocal() []string {
-	return args // no-op
+	return nil // no-op; matches all traffic
 }
 
-func (n *noOpLocalDetector) JumpIfNotLocal(args []string, toChain string) []string {
+func (n *noOpLocalDetector) IfNotLocal() []string {
-	return args // no-op
+	return nil // no-op; matches all traffic
 }
 
 type detectLocalByCIDR struct {
-	cidr string
+	ifLocal    []string
+	ifNotLocal []string
 }
 
 // NewDetectLocalByCIDR implements the LocalTrafficDetector interface using a CIDR. This can be used when a single CIDR
@@ -72,21 +70,20 @@ func NewDetectLocalByCIDR(cidr string, ipt utiliptables.Interface) (LocalTraffic
 	if err != nil {
 		return nil, err
 	}
-	return &detectLocalByCIDR{cidr: cidr}, nil
+	return &detectLocalByCIDR{
+		ifLocal:    []string{"-s", cidr},
+		ifNotLocal: []string{"!", "-s", cidr},
+	}, nil
 }
 
 func (d *detectLocalByCIDR) IsImplemented() bool {
 	return true
 }
 
-func (d *detectLocalByCIDR) JumpIfLocal(args []string, toChain string) []string {
+func (d *detectLocalByCIDR) IfLocal() []string {
-	line := append(args, "-s", d.cidr, "-j", toChain)
+	return d.ifLocal
-	klog.V(4).InfoS("Detect Local By CIDR", "cidr", d.cidr, "jumpLocal", line)
-	return line
 }
 
-func (d *detectLocalByCIDR) JumpIfNotLocal(args []string, toChain string) []string {
+func (d *detectLocalByCIDR) IfNotLocal() []string {
-	line := append(args, "!", "-s", d.cidr, "-j", toChain)
+	return d.ifNotLocal
-	klog.V(4).InfoS("Detect Local By CIDR", "cidr", d.cidr, "jumpNotLocal", line)
-	return line
 }

pkg/proxy/util/iptables/traffic_test.go

@@ -25,35 +25,19 @@ import (
 )
 
 func TestNoOpLocalDetector(t *testing.T) {
-	cases := []struct {
+	localDetector := NewNoOpLocalDetector()
-		chain                   string
+	if localDetector.IsImplemented() {
-		args                    []string
+		t.Error("NoOpLocalDetector returns true for IsImplemented")
-		expectedJumpIfOutput    []string
-		expectedJumpIfNotOutput []string
-	}{
-		{
-			chain:                   "TEST",
-			args:                    []string{"arg1", "arg2"},
-			expectedJumpIfOutput:    []string{"arg1", "arg2"},
-			expectedJumpIfNotOutput: []string{"arg1", "arg2"},
-		},
 	}
-	for _, c := range cases {
-		localDetector := NewNoOpLocalDetector()
-		if localDetector.IsImplemented() {
-			t.Error("DetectLocalByCIDR returns true for IsImplemented")
-		}
 
-		jumpIf := localDetector.JumpIfLocal(c.args, c.chain)
+	ifLocal := localDetector.IfLocal()
-		jumpIfNot := localDetector.JumpIfNotLocal(c.args, c.chain)
+	if len(ifLocal) != 0 {
+		t.Errorf("NoOpLocalDetector returns %v for IsLocal (expected nil)", ifLocal)
+	}
 
-		if !reflect.DeepEqual(jumpIf, c.expectedJumpIfOutput) {
+	ifNotLocal := localDetector.IfNotLocal()
-			t.Errorf("JumpIf, expected: '%v', but got: '%v'", c.expectedJumpIfOutput, jumpIf)
+	if len(ifNotLocal) != 0 {
-		}
+		t.Errorf("NoOpLocalDetector returns %v for IsNotLocal (expected nil)", ifNotLocal)
-
-		if !reflect.DeepEqual(jumpIfNot, c.expectedJumpIfNotOutput) {
-			t.Errorf("JumpIfNot, expected: '%v', but got: '%v'", c.expectedJumpIfNotOutput, jumpIfNot)
-		}
 	}
 }
 
@@ -120,28 +104,22 @@ func TestNewDetectLocalByCIDR(t *testing.T) {
 
 func TestDetectLocalByCIDR(t *testing.T) {
 	cases := []struct {
-		cidr                    string
+		cidr                     string
-		ipt                     utiliptables.Interface
+		ipt                      utiliptables.Interface
-		chain                   string
+		expectedIfLocalOutput    []string
-		args                    []string
+		expectedIfNotLocalOutput []string
-		expectedJumpIfOutput    []string
-		expectedJumpIfNotOutput []string
 	}{
 		{
-			cidr:                    "10.0.0.0/14",
+			cidr:                     "10.0.0.0/14",
-			ipt:                     iptablestest.NewFake(),
+			ipt:                      iptablestest.NewFake(),
-			chain:                   "TEST",
+			expectedIfLocalOutput:    []string{"-s", "10.0.0.0/14"},
-			args:                    []string{"arg1", "arg2"},
+			expectedIfNotLocalOutput: []string{"!", "-s", "10.0.0.0/14"},
-			expectedJumpIfOutput:    []string{"arg1", "arg2", "-s", "10.0.0.0/14", "-j", "TEST"},
-			expectedJumpIfNotOutput: []string{"arg1", "arg2", "!", "-s", "10.0.0.0/14", "-j", "TEST"},
 		},
 		{
-			cidr:                    "2002::1234:abcd:ffff:c0a8:101/64",
+			cidr:                     "2002::1234:abcd:ffff:c0a8:101/64",
-			ipt:                     iptablestest.NewIPv6Fake(),
+			ipt:                      iptablestest.NewIPv6Fake(),
-			chain:                   "TEST",
+			expectedIfLocalOutput:    []string{"-s", "2002::1234:abcd:ffff:c0a8:101/64"},
-			args:                    []string{"arg1", "arg2"},
+			expectedIfNotLocalOutput: []string{"!", "-s", "2002::1234:abcd:ffff:c0a8:101/64"},
-			expectedJumpIfOutput:    []string{"arg1", "arg2", "-s", "2002::1234:abcd:ffff:c0a8:101/64", "-j", "TEST"},
-			expectedJumpIfNotOutput: []string{"arg1", "arg2", "!", "-s", "2002::1234:abcd:ffff:c0a8:101/64", "-j", "TEST"},
 		},
 	}
 	for _, c := range cases {
@@ -154,15 +132,15 @@ func TestDetectLocalByCIDR(t *testing.T) {
 			t.Error("DetectLocalByCIDR returns false for IsImplemented")
 		}
 
-		jumpIf := localDetector.JumpIfLocal(c.args, c.chain)
+		ifLocal := localDetector.IfLocal()
-		jumpIfNot := localDetector.JumpIfNotLocal(c.args, c.chain)
+		ifNotLocal := localDetector.IfNotLocal()
 
-		if !reflect.DeepEqual(jumpIf, c.expectedJumpIfOutput) {
+		if !reflect.DeepEqual(ifLocal, c.expectedIfLocalOutput) {
-			t.Errorf("JumpIf, expected: '%v', but got: '%v'", c.expectedJumpIfOutput, jumpIf)
+			t.Errorf("IfLocal, expected: '%v', but got: '%v'", c.expectedIfLocalOutput, ifLocal)
 		}
 
-		if !reflect.DeepEqual(jumpIfNot, c.expectedJumpIfNotOutput) {
+		if !reflect.DeepEqual(ifNotLocal, c.expectedIfNotLocalOutput) {
-			t.Errorf("JumpIfNot, expected: '%v', but got: '%v'", c.expectedJumpIfNotOutput, jumpIfNot)
+			t.Errorf("IfNotLocal, expected: '%v', but got: '%v'", c.expectedIfNotLocalOutput, ifNotLocal)
 		}
 	}
 }