Skip to content

Commit

Permalink
pkg/proxy: Simplify LocalTrafficDetector
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@danwinship
danwinship committed Mar 18, 2022
1 parent 3a4064c commit e354964
Show file tree
Hide file tree
Showing 4 changed files with 65 additions and 79 deletions.
14 changes: 9 additions & 5 deletions pkg/proxy/iptables/proxier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
13 changes: 10 additions & 3 deletions pkg/proxy/ipvs/proxier.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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))
}
}

Expand Down
39 changes: 18 additions & 21 deletions pkg/proxy/util/iptables/traffic.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ package iptables
import (
"fmt"

"k8s.io/klog/v2"
utiliptables "k8s.io/kubernetes/pkg/util/iptables"
netutils "k8s.io/utils/net"
)
Expand All @@ -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{}
Expand All @@ -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
Expand All @@ -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
}
78 changes: 28 additions & 50 deletions pkg/proxy/util/iptables/traffic_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
}
}

0 comments on commit e354964

Please sign in to comment.