From 709b4f696d0df0c168aeae9572388b173847d864 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 13 Apr 2022 13:08:21 -0400 Subject: [PATCH 1/3] proxy/iptables: test LoadBalancerSourceRanges vs node IP The LoadBalancer rules change if the node IP is in one of the LoadBalancerSourceRange subnets, so make sure to set nodeIP on the fake proxier so we can test this, and add a second source range to TestLoadBalancer containing the node IP. (This changes the result of one flow test that previously expected that node-to-LB would be dropped.) --- pkg/proxy/iptables/proxier_test.go | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go index fc2feadd874..7be539f944d 100644 --- a/pkg/proxy/iptables/proxier_test.go +++ b/pkg/proxy/iptables/proxier_test.go @@ -420,6 +420,7 @@ func NewFakeProxier(ipt utiliptables.Interface) *Proxier { filterRules: utilproxy.LineBuffer{}, natChains: utilproxy.LineBuffer{}, natRules: utilproxy.LineBuffer{}, + nodeIP: netutils.ParseIPSloppy(testNodeIP), nodePortAddresses: make([]string, 0), networkInterfacer: networkInterfacer, } @@ -2389,9 +2390,12 @@ func TestLoadBalancer(t *testing.T) { svc.Status.LoadBalancer.Ingress = []v1.LoadBalancerIngress{{ IP: svcLBIP, }} - // Also ensure that invalid LoadBalancerSourceRanges will not result - // in a crash. - svc.Spec.LoadBalancerSourceRanges = []string{" 203.0.113.0/25"} + svc.Spec.LoadBalancerSourceRanges = []string{ + "192.168.0.0/24", + + // Regression test that excess whitespace gets ignored + " 203.0.113.0/25", + } }), ) @@ -2438,7 +2442,9 @@ func TestLoadBalancer(t *testing.T) { -A KUBE-SERVICES -m comment --comment "kubernetes service nodeports; NOTE: this must be the last rule in this chain" -m addrtype --dst-type LOCAL -j KUBE-NODEPORTS -A KUBE-EXT-XPGD46QRK7WJZT7O -m comment --comment "masquerade traffic for ns1/svc1:p80 external destinations" -j KUBE-MARK-MASQ -A KUBE-EXT-XPGD46QRK7WJZT7O -j KUBE-SVC-XPGD46QRK7WJZT7O + -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 192.168.0.0/24 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 203.0.113.0/25 -j KUBE-EXT-XPGD46QRK7WJZT7O + -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 1.2.3.4 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -j KUBE-MARK-DROP -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN @@ -2501,11 +2507,25 @@ func TestLoadBalancer(t *testing.T) { output: "DROP", }, { - name: "node to LB (blocked by LoadBalancerSourceRanges)", + name: "node to LB (allowed by LoadBalancerSourceRanges)", sourceIP: testNodeIP, destIP: svcLBIP, destPort: svcPort, - output: "DROP", + output: fmt.Sprintf("%s:%d", epIP, svcPort), + masq: true, + }, + + // The LB rules assume that when you connect from a node to a LB IP, that + // something external to kube-proxy will cause the connection to be + // SNATted to the LB IP, so if the LoadBalancerSourceRanges include the + // node IP, then we add a rule allowing traffic from the LB IP as well... + { + name: "same node to LB, SNATted to LB (implicitly allowed)", + sourceIP: svcLBIP, + destIP: svcLBIP, + destPort: svcPort, + output: fmt.Sprintf("%s:%d", epIP, svcPort), + masq: true, }, }) } From df589b46a104a576a84c05b3324d6e1441ce1a04 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 5 May 2022 09:17:54 -0400 Subject: [PATCH 2/3] proxy/iptables: test multiple LoadBalancer IPs on one service --- pkg/proxy/iptables/proxier_test.go | 76 ++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 15 deletions(-) diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go index 7be539f944d..d66a6de02bd 100644 --- a/pkg/proxy/iptables/proxier_test.go +++ b/pkg/proxy/iptables/proxier_test.go @@ -2370,7 +2370,8 @@ func TestLoadBalancer(t *testing.T) { svcIP := "172.30.0.41" svcPort := 80 svcNodePort := 3001 - svcLBIP := "1.2.3.4" + svcLBIP1 := "1.2.3.4" + svcLBIP2 := "5.6.7.8" svcPortName := proxy.ServicePortName{ NamespacedName: makeNSN("ns1", "svc1"), Port: "p80", @@ -2387,9 +2388,10 @@ func TestLoadBalancer(t *testing.T) { Protocol: v1.ProtocolTCP, NodePort: int32(svcNodePort), }} - svc.Status.LoadBalancer.Ingress = []v1.LoadBalancerIngress{{ - IP: svcLBIP, - }} + svc.Status.LoadBalancer.Ingress = []v1.LoadBalancerIngress{ + {IP: svcLBIP1}, + {IP: svcLBIP2}, + } svc.Spec.LoadBalancerSourceRanges = []string{ "192.168.0.0/24", @@ -2439,6 +2441,7 @@ func TestLoadBalancer(t *testing.T) { -A KUBE-NODEPORTS -m comment --comment ns1/svc1:p80 -m tcp -p tcp --dport 3001 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-SERVICES -m comment --comment "ns1/svc1:p80 cluster IP" -m tcp -p tcp -d 172.30.0.41 --dport 80 -j KUBE-SVC-XPGD46QRK7WJZT7O -A KUBE-SERVICES -m comment --comment "ns1/svc1:p80 loadbalancer IP" -m tcp -p tcp -d 1.2.3.4 --dport 80 -j KUBE-FW-XPGD46QRK7WJZT7O + -A KUBE-SERVICES -m comment --comment "ns1/svc1:p80 loadbalancer IP" -m tcp -p tcp -d 5.6.7.8 --dport 80 -j KUBE-FW-XPGD46QRK7WJZT7O -A KUBE-SERVICES -m comment --comment "kubernetes service nodeports; NOTE: this must be the last rule in this chain" -m addrtype --dst-type LOCAL -j KUBE-NODEPORTS -A KUBE-EXT-XPGD46QRK7WJZT7O -m comment --comment "masquerade traffic for ns1/svc1:p80 external destinations" -j KUBE-MARK-MASQ -A KUBE-EXT-XPGD46QRK7WJZT7O -j KUBE-SVC-XPGD46QRK7WJZT7O @@ -2446,6 +2449,10 @@ func TestLoadBalancer(t *testing.T) { -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 203.0.113.0/25 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 1.2.3.4 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -j KUBE-MARK-DROP + -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 192.168.0.0/24 -j KUBE-EXT-XPGD46QRK7WJZT7O + -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 203.0.113.0/25 -j KUBE-EXT-XPGD46QRK7WJZT7O + -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 5.6.7.8 -j KUBE-EXT-XPGD46QRK7WJZT7O + -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -j KUBE-MARK-DROP -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 -A KUBE-POSTROUTING -m mark ! --mark 0x4000/0x4000 -j RETURN -A KUBE-POSTROUTING -j MARK --xor-mark 0x4000 @@ -2485,31 +2492,61 @@ func TestLoadBalancer(t *testing.T) { masq: true, }, { - name: "accepted external to LB", + name: "accepted external to LB1", sourceIP: testExternalClient, - destIP: svcLBIP, + destIP: svcLBIP1, destPort: svcPort, output: fmt.Sprintf("%s:%d", epIP, svcPort), masq: true, }, { - name: "blocked external to LB", + name: "accepted external to LB2", + sourceIP: testExternalClient, + destIP: svcLBIP2, + destPort: svcPort, + output: fmt.Sprintf("%s:%d", epIP, svcPort), + masq: true, + }, + { + name: "blocked external to LB1", sourceIP: testExternalClientBlocked, - destIP: svcLBIP, + destIP: svcLBIP1, destPort: svcPort, output: "DROP", }, { - name: "pod to LB (blocked by LoadBalancerSourceRanges)", + name: "blocked external to LB2", + sourceIP: testExternalClientBlocked, + destIP: svcLBIP2, + destPort: svcPort, + output: "DROP", + }, + { + name: "pod to LB1 (blocked by LoadBalancerSourceRanges)", sourceIP: "10.0.0.2", - destIP: svcLBIP, + destIP: svcLBIP1, destPort: svcPort, output: "DROP", }, { - name: "node to LB (allowed by LoadBalancerSourceRanges)", + name: "pod to LB2 (blocked by LoadBalancerSourceRanges)", + sourceIP: "10.0.0.2", + destIP: svcLBIP2, + destPort: svcPort, + output: "DROP", + }, + { + name: "node to LB1 (allowed by LoadBalancerSourceRanges)", sourceIP: testNodeIP, - destIP: svcLBIP, + destIP: svcLBIP1, + destPort: svcPort, + output: fmt.Sprintf("%s:%d", epIP, svcPort), + masq: true, + }, + { + name: "node to LB2 (allowed by LoadBalancerSourceRanges)", + sourceIP: testNodeIP, + destIP: svcLBIP2, destPort: svcPort, output: fmt.Sprintf("%s:%d", epIP, svcPort), masq: true, @@ -2520,13 +2557,22 @@ func TestLoadBalancer(t *testing.T) { // SNATted to the LB IP, so if the LoadBalancerSourceRanges include the // node IP, then we add a rule allowing traffic from the LB IP as well... { - name: "same node to LB, SNATted to LB (implicitly allowed)", - sourceIP: svcLBIP, - destIP: svcLBIP, + name: "same node to LB1, SNATted to LB1 (implicitly allowed)", + sourceIP: svcLBIP1, + destIP: svcLBIP1, destPort: svcPort, output: fmt.Sprintf("%s:%d", epIP, svcPort), masq: true, }, + // FIXME: this fails + // { + // name: "same node to LB2, SNATted to LB2 (implicitly allowed)", + // sourceIP: svcLBIP2, + // destIP: svcLBIP2, + // destPort: svcPort, + // output: fmt.Sprintf("%s:%d", epIP, svcPort), + // masq: true, + // }, }) } From 813aca47afc0a008be42ce2c0d1322963d9ff054 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Wed, 6 Apr 2022 11:08:28 -0400 Subject: [PATCH 3/3] proxy/iptables: fix firewall rules with multiple LB IPs The various loops in the LoadBalancer rule section were mis-nested such that if a service had multiple LoadBalancer IPs, we would write out the firewall rules multiple times (and the allowFromNode rule for the second and later IPs would end up being written after the "else DROP" rule from the first IP). --- pkg/proxy/iptables/proxier.go | 78 +++++++++++++++--------------- pkg/proxy/iptables/proxier_test.go | 20 +++----- 2 files changed, 48 insertions(+), 50 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 5f8094ef605..d5d1c95080f 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -1243,6 +1243,46 @@ func (proxier *Proxier) syncProxyRules() { // The firewall chain will jump to the "external destination" // chain. nextChain = svcInfo.firewallChainName + + // The service firewall rules are created based on the + // loadBalancerSourceRanges field. This only works for + // VIP-like loadbalancers that preserve source IPs. For + // loadbalancers which direct traffic to service NodePort, the + // firewall rules will not apply. + args = append(args[:0], + "-A", string(nextChain), + "-m", "comment", "--comment", fmt.Sprintf(`"%s loadbalancer IP"`, svcNameString), + ) + + // firewall filter based on each source range + allowFromNode := false + for _, src := range svcInfo.LoadBalancerSourceRanges() { + proxier.natRules.Write(args, "-s", src, "-j", string(externalTrafficChain)) + _, cidr, err := netutils.ParseCIDRSloppy(src) + if err != nil { + klog.ErrorS(err, "Error parsing CIDR in LoadBalancerSourceRanges, dropping it", "cidr", cidr) + } else if cidr.Contains(proxier.nodeIP) { + allowFromNode = true + } + } + // For VIP-like LBs, the VIP is often added as a local + // address (via an IP route rule). In that case, a request + // from a node to the VIP will not hit the loadbalancer but + // will loop back with the source IP set to the VIP. We + // need the following rules to allow requests from this node. + if allowFromNode { + for _, lbip := range svcInfo.LoadBalancerIPStrings() { + proxier.natRules.Write( + args, + "-s", lbip, + "-j", string(externalTrafficChain)) + } + } + + // If the packet was able to reach the end of firewall chain, + // then it did not get DNATed. It means the packet cannot go + // thru the firewall, then mark it for DROP. + proxier.natRules.Write(args, "-j", string(KubeMarkDropChain)) } for _, lbip := range svcInfo.LoadBalancerIPStrings() { @@ -1254,44 +1294,6 @@ func (proxier *Proxier) syncProxyRules() { "--dport", strconv.Itoa(svcInfo.Port()), "-j", string(nextChain)) - // The service firewall rules are created based on the - // loadBalancerSourceRanges field. This only works for - // VIP-like loadbalancers that preserve source IPs. For - // loadbalancers which direct traffic to service NodePort, the - // firewall rules will not apply. - if len(svcInfo.LoadBalancerSourceRanges()) > 0 { - args = append(args[:0], - "-A", string(nextChain), - "-m", "comment", "--comment", fmt.Sprintf(`"%s loadbalancer IP"`, svcNameString), - ) - - // firewall filter based on each source range - allowFromNode := false - for _, src := range svcInfo.LoadBalancerSourceRanges() { - proxier.natRules.Write(args, "-s", src, "-j", string(externalTrafficChain)) - _, cidr, err := netutils.ParseCIDRSloppy(src) - if err != nil { - klog.ErrorS(err, "Error parsing CIDR in LoadBalancerSourceRanges, dropping it", "cidr", cidr) - } else if cidr.Contains(proxier.nodeIP) { - allowFromNode = true - } - } - // For VIP-like LBs, the VIP is often added as a local - // address (via an IP route rule). In that case, a request - // from a node to the VIP will not hit the loadbalancer but - // will loop back with the source IP set to the VIP. We - // need the following rule to allow requests from this node. - if allowFromNode { - proxier.natRules.Write( - args, - "-s", lbip, - "-j", string(externalTrafficChain)) - } - // If the packet was able to reach the end of firewall chain, - // then it did not get DNATed. It means the packet cannot go - // thru the firewall, then mark it for DROP. - proxier.natRules.Write(args, "-j", string(KubeMarkDropChain)) - } } } else { // No endpoints. diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go index d66a6de02bd..2af561cd2af 100644 --- a/pkg/proxy/iptables/proxier_test.go +++ b/pkg/proxy/iptables/proxier_test.go @@ -2448,9 +2448,6 @@ func TestLoadBalancer(t *testing.T) { -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 192.168.0.0/24 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 203.0.113.0/25 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 1.2.3.4 -j KUBE-EXT-XPGD46QRK7WJZT7O - -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -j KUBE-MARK-DROP - -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 192.168.0.0/24 -j KUBE-EXT-XPGD46QRK7WJZT7O - -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 203.0.113.0/25 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -s 5.6.7.8 -j KUBE-EXT-XPGD46QRK7WJZT7O -A KUBE-FW-XPGD46QRK7WJZT7O -m comment --comment "ns1/svc1:p80 loadbalancer IP" -j KUBE-MARK-DROP -A KUBE-MARK-MASQ -j MARK --or-mark 0x4000 @@ -2564,15 +2561,14 @@ func TestLoadBalancer(t *testing.T) { output: fmt.Sprintf("%s:%d", epIP, svcPort), masq: true, }, - // FIXME: this fails - // { - // name: "same node to LB2, SNATted to LB2 (implicitly allowed)", - // sourceIP: svcLBIP2, - // destIP: svcLBIP2, - // destPort: svcPort, - // output: fmt.Sprintf("%s:%d", epIP, svcPort), - // masq: true, - // }, + { + name: "same node to LB2, SNATted to LB2 (implicitly allowed)", + sourceIP: svcLBIP2, + destIP: svcLBIP2, + destPort: svcPort, + output: fmt.Sprintf("%s:%d", epIP, svcPort), + masq: true, + }, }) }