From e3549646ec30948bc489e94a2f2f69a238504ec0 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 6 Jan 2022 13:53:42 -0500 Subject: [PATCH] pkg/proxy: Simplify LocalTrafficDetector Now that we don't have to always append all of the iptables args into a single array, there's no reason to have LocalTrafficDetector take in a set of args to prepend to its own output, and also not much point in having it write out the "-j CHAIN" by itself either. --- pkg/proxy/iptables/proxier.go | 14 +++-- pkg/proxy/ipvs/proxier.go | 13 ++++- pkg/proxy/util/iptables/traffic.go | 39 ++++++------- pkg/proxy/util/iptables/traffic_test.go | 78 +++++++++---------------- 4 files changed, 65 insertions(+), 79 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index ba2f1fa4a0143..5dbbe3b76185d 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/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.natRules.Write(proxier.localDetector.JumpIfLocal(args, string(svcChain))) + proxier.localDetector.IfLocal(), + "-j", string(svcChain)) } // Next, redirect all src-type=LOCAL -> LB IP to the service chain for externalTrafficPolicy=Local diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index ba7a86d3d3982..e424fe6e49979 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/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:. // 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)) } } diff --git a/pkg/proxy/util/iptables/traffic.go b/pkg/proxy/util/iptables/traffic.go index 2ee0ae015becf..f96f8f44e0972 100644 --- a/pkg/proxy/util/iptables/traffic.go +++ b/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 - // of local origin - JumpIfLocal(args []string, toChain string) []string + // IfLocal returns iptables arguments that will match traffic from a pod + IfLocal() []string - // JumpINotfLocal appends conditions to jump to a target chain if traffic detected not to be - // of local origin - JumpIfNotLocal(args []string, toChain string) []string + // IfNotLocal returns iptables arguments that will match traffic that is not from a pod + IfNotLocal() []string } type noOpLocalDetector struct{} @@ -50,16 +47,17 @@ func (n *noOpLocalDetector) IsImplemented() bool { return false } -func (n *noOpLocalDetector) JumpIfLocal(args []string, toChain string) []string { - return args // no-op +func (n *noOpLocalDetector) IfLocal() []string { + return nil // no-op; matches all traffic } -func (n *noOpLocalDetector) JumpIfNotLocal(args []string, toChain string) []string { - return args // no-op +func (n *noOpLocalDetector) IfNotLocal() []string { + 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 { - line := append(args, "-s", d.cidr, "-j", toChain) - klog.V(4).InfoS("Detect Local By CIDR", "cidr", d.cidr, "jumpLocal", line) - return line +func (d *detectLocalByCIDR) IfLocal() []string { + return d.ifLocal } -func (d *detectLocalByCIDR) JumpIfNotLocal(args []string, toChain string) []string { - line := append(args, "!", "-s", d.cidr, "-j", toChain) - klog.V(4).InfoS("Detect Local By CIDR", "cidr", d.cidr, "jumpNotLocal", line) - return line +func (d *detectLocalByCIDR) IfNotLocal() []string { + return d.ifNotLocal } diff --git a/pkg/proxy/util/iptables/traffic_test.go b/pkg/proxy/util/iptables/traffic_test.go index c346dfb79fca0..d54ec61f8964c 100644 --- a/pkg/proxy/util/iptables/traffic_test.go +++ b/pkg/proxy/util/iptables/traffic_test.go @@ -25,35 +25,19 @@ import ( ) func TestNoOpLocalDetector(t *testing.T) { - cases := []struct { - chain string - args []string - expectedJumpIfOutput []string - expectedJumpIfNotOutput []string - }{ - { - chain: "TEST", - args: []string{"arg1", "arg2"}, - expectedJumpIfOutput: []string{"arg1", "arg2"}, - expectedJumpIfNotOutput: []string{"arg1", "arg2"}, - }, + localDetector := NewNoOpLocalDetector() + if localDetector.IsImplemented() { + t.Error("NoOpLocalDetector returns true for IsImplemented") } - for _, c := range cases { - localDetector := NewNoOpLocalDetector() - if localDetector.IsImplemented() { - t.Error("DetectLocalByCIDR returns true for IsImplemented") - } - - jumpIf := localDetector.JumpIfLocal(c.args, c.chain) - jumpIfNot := localDetector.JumpIfNotLocal(c.args, c.chain) - if !reflect.DeepEqual(jumpIf, c.expectedJumpIfOutput) { - t.Errorf("JumpIf, expected: '%v', but got: '%v'", c.expectedJumpIfOutput, jumpIf) - } + ifLocal := localDetector.IfLocal() + if len(ifLocal) != 0 { + t.Errorf("NoOpLocalDetector returns %v for IsLocal (expected nil)", ifLocal) + } - if !reflect.DeepEqual(jumpIfNot, c.expectedJumpIfNotOutput) { - t.Errorf("JumpIfNot, expected: '%v', but got: '%v'", c.expectedJumpIfNotOutput, jumpIfNot) - } + ifNotLocal := localDetector.IfNotLocal() + if len(ifNotLocal) != 0 { + t.Errorf("NoOpLocalDetector returns %v for IsNotLocal (expected nil)", ifNotLocal) } } @@ -120,28 +104,22 @@ func TestNewDetectLocalByCIDR(t *testing.T) { func TestDetectLocalByCIDR(t *testing.T) { cases := []struct { - cidr string - ipt utiliptables.Interface - chain string - args []string - expectedJumpIfOutput []string - expectedJumpIfNotOutput []string + cidr string + ipt utiliptables.Interface + expectedIfLocalOutput []string + expectedIfNotLocalOutput []string }{ { - cidr: "10.0.0.0/14", - ipt: iptablestest.NewFake(), - chain: "TEST", - args: []string{"arg1", "arg2"}, - 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: "10.0.0.0/14", + ipt: iptablestest.NewFake(), + expectedIfLocalOutput: []string{"-s", "10.0.0.0/14"}, + expectedIfNotLocalOutput: []string{"!", "-s", "10.0.0.0/14"}, }, { - cidr: "2002::1234:abcd:ffff:c0a8:101/64", - ipt: iptablestest.NewIPv6Fake(), - chain: "TEST", - args: []string{"arg1", "arg2"}, - 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"}, + cidr: "2002::1234:abcd:ffff:c0a8:101/64", + ipt: iptablestest.NewIPv6Fake(), + expectedIfLocalOutput: []string{"-s", "2002::1234:abcd:ffff:c0a8:101/64"}, + expectedIfNotLocalOutput: []string{"!", "-s", "2002::1234:abcd:ffff:c0a8:101/64"}, }, } 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) - jumpIfNot := localDetector.JumpIfNotLocal(c.args, c.chain) + ifLocal := localDetector.IfLocal() + ifNotLocal := localDetector.IfNotLocal() - if !reflect.DeepEqual(jumpIf, c.expectedJumpIfOutput) { - t.Errorf("JumpIf, expected: '%v', but got: '%v'", c.expectedJumpIfOutput, jumpIf) + if !reflect.DeepEqual(ifLocal, c.expectedIfLocalOutput) { + t.Errorf("IfLocal, expected: '%v', but got: '%v'", c.expectedIfLocalOutput, ifLocal) } - if !reflect.DeepEqual(jumpIfNot, c.expectedJumpIfNotOutput) { - t.Errorf("JumpIfNot, expected: '%v', but got: '%v'", c.expectedJumpIfNotOutput, jumpIfNot) + if !reflect.DeepEqual(ifNotLocal, c.expectedIfNotLocalOutput) { + t.Errorf("IfNotLocal, expected: '%v', but got: '%v'", c.expectedIfNotLocalOutput, ifNotLocal) } } }