From f7bb9a9a0a79efe7c4e22d49e1d993d72de9ae25 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 3 Mar 2023 18:05:07 -0500 Subject: [PATCH 1/5] Remove a mostly-unused variable in the ipvs proxy It probably was used for something else in the past but it's pointless now. --- pkg/proxy/ipvs/proxier.go | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index f312e68ebc1..c2c7d316277 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1004,22 +1004,15 @@ func (proxier *Proxier) syncProxyRules() { } } - // Both nodeAddresses and nodeIPs can be reused for all nodePort services - // and only need to be computed if we have at least one nodePort service. - var ( - // List of node addresses to listen on if a nodePort is set. - nodeAddresses []string - // List of node IP addresses to be used as IPVS services if nodePort is set. - nodeIPs []net.IP - ) - + // List of node IP addresses to be used as IPVS services if nodePort is set. This + // can be reused for all nodePort services. + var nodeIPs []net.IP if hasNodePort { nodeAddrSet, err := proxier.nodePortAddresses.GetNodeAddresses(proxier.networkInterfacer) if err != nil { klog.ErrorS(err, "Failed to get node IP address matching nodeport cidr") } else { - nodeAddresses = nodeAddrSet.UnsortedList() - for _, address := range nodeAddresses { + for _, address := range nodeAddrSet.UnsortedList() { a := netutils.ParseIPSloppy(address) if a.IsLoopback() { continue @@ -1292,7 +1285,7 @@ func (proxier *Proxier) syncProxyRules() { } if svcInfo.NodePort() != 0 { - if len(nodeAddresses) == 0 || len(nodeIPs) == 0 { + if len(nodeIPs) == 0 { // Skip nodePort configuration since an error occurred when // computing nodeAddresses or nodeIPs. continue From 9ac657bb94e076d147603c570d3b359863c3cd39 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sat, 14 Jan 2023 16:54:56 -0500 Subject: [PATCH 2/5] Make NodePortAddresses explicitly IP-family-specific Both proxies handle IPv4 and IPv6 nodeport addresses separately, but GetNodeAddresses went out of its way to make that difficult. Fix that. This commit does not change any externally-visible semantics, but it makes the existing weird semantics more obvious. Specifically, if you say "--nodeport-addresses 10.0.0.0/8,192.168.0.0/16", then the dual-stack proxy code would have split that into a list of IPv4 CIDRs (["10.0.0.0/8", "192.168.0.0/16"]) to pass to the IPv4 proxier, and a list of IPv6 CIDRs ([]) to pass to the IPv6 proxier, and then the IPv6 proxier would say "well since the list of nodeport addresses is empty, I'll listen on all IPv6 addresses", which probably isn't what you meant, but that's what it did. --- pkg/proxy/healthcheck/healthcheck_test.go | 5 +- pkg/proxy/iptables/proxier.go | 22 +-- pkg/proxy/iptables/proxier_test.go | 13 +- pkg/proxy/ipvs/proxier.go | 12 +- pkg/proxy/ipvs/proxier_test.go | 10 +- pkg/proxy/util/nodeport_addresses.go | 62 +++++---- pkg/proxy/util/nodeport_addresses_test.go | 155 ++++++++++++++++++---- pkg/proxy/winkernel/proxier.go | 13 +- 8 files changed, 196 insertions(+), 96 deletions(-) diff --git a/pkg/proxy/healthcheck/healthcheck_test.go b/pkg/proxy/healthcheck/healthcheck_test.go index bbb7489e67d..4d6734145d6 100644 --- a/pkg/proxy/healthcheck/healthcheck_test.go +++ b/pkg/proxy/healthcheck/healthcheck_test.go @@ -24,6 +24,7 @@ import ( "testing" "time" + "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/dump" "k8s.io/apimachinery/pkg/util/sets" @@ -140,7 +141,7 @@ func (fake fakeProxierHealthChecker) IsHealthy() bool { func TestServer(t *testing.T) { listener := newFakeListener() httpFactory := newFakeHTTPServerFactory() - nodePortAddresses := utilproxy.NewNodePortAddresses([]string{}) + nodePortAddresses := utilproxy.NewNodePortAddresses(v1.IPv4Protocol, []string{}) proxyChecker := &fakeProxierHealthChecker{true} hcsi := newServiceHealthServer("hostname", nil, listener, httpFactory, nodePortAddresses, proxyChecker) @@ -470,7 +471,7 @@ func TestServerWithSelectiveListeningAddress(t *testing.T) { // limiting addresses to loop back. We don't want any cleverness here around getting IP for // machine nor testing ipv6 || ipv4. using loop back guarantees the test will work on any machine - nodePortAddresses := utilproxy.NewNodePortAddresses([]string{"127.0.0.0/8"}) + nodePortAddresses := utilproxy.NewNodePortAddresses(v1.IPv4Protocol, []string{"127.0.0.0/8"}) hcsi := newServiceHealthServer("hostname", nil, listener, httpFactory, nodePortAddresses, proxyChecker) hcs := hcsi.(*server) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 68f259c0e6a..eb50769daf4 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -240,7 +240,7 @@ func NewProxier(ipFamily v1.IPFamily, healthzServer healthcheck.ProxierHealthUpdater, nodePortAddressStrings []string, ) (*Proxier, error) { - nodePortAddresses := utilproxy.NewNodePortAddresses(nodePortAddressStrings) + nodePortAddresses := utilproxy.NewNodePortAddresses(ipFamily, nodePortAddressStrings) if !nodePortAddresses.ContainsIPv4Loopback() { localhostNodePorts = false @@ -334,17 +334,16 @@ func NewDualStackProxier( nodePortAddresses []string, ) (proxy.Provider, error) { // Create an ipv4 instance of the single-stack proxier - ipFamilyMap := utilproxy.MapCIDRsByIPFamily(nodePortAddresses) ipv4Proxier, err := NewProxier(v1.IPv4Protocol, ipt[0], sysctl, exec, syncPeriod, minSyncPeriod, masqueradeAll, localhostNodePorts, masqueradeBit, localDetectors[0], hostname, - nodeIP[0], recorder, healthzServer, ipFamilyMap[v1.IPv4Protocol]) + nodeIP[0], recorder, healthzServer, nodePortAddresses) if err != nil { return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err) } ipv6Proxier, err := NewProxier(v1.IPv6Protocol, ipt[1], sysctl, exec, syncPeriod, minSyncPeriod, masqueradeAll, false, masqueradeBit, localDetectors[1], hostname, - nodeIP[1], recorder, healthzServer, ipFamilyMap[v1.IPv6Protocol]) + nodeIP[1], recorder, healthzServer, nodePortAddresses) if err != nil { return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err) } @@ -1420,15 +1419,6 @@ func (proxier *Proxier) syncProxyRules() { if err != nil { klog.ErrorS(err, "Failed to get node ip address matching nodeport cidrs, services with nodeport may not work as intended", "CIDRs", proxier.nodePortAddresses) } - // nodeAddresses may contain dual-stack zero-CIDRs if proxier.nodePortAddresses is empty. - // Ensure nodeAddresses only contains the addresses for this proxier's IP family. - for addr := range nodeAddresses { - if utilproxy.IsZeroCIDR(addr) && isIPv6 == netutils.IsIPv6CIDRString(addr) { - // if any of the addresses is zero cidr of this IP family, non-zero IPs can be excluded. - nodeAddresses = sets.New[string](addr) - break - } - } for address := range nodeAddresses { if utilproxy.IsZeroCIDR(address) { @@ -1455,12 +1445,6 @@ func (proxier *Proxier) syncProxyRules() { break } - // Ignore IP addresses with incorrect version - if isIPv6 && !netutils.IsIPv6String(address) || !isIPv6 && netutils.IsIPv6String(address) { - klog.ErrorS(nil, "IP has incorrect IP version", "IP", address) - continue - } - // For ipv6, Regardless of the value of localhostNodePorts is true or false, we should disallow access // to the nodePort via lookBack address. if isIPv6 && utilproxy.IsLoopBack(address) { diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go index 328edf8efba..030ab07d828 100644 --- a/pkg/proxy/iptables/proxier_test.go +++ b/pkg/proxy/iptables/proxier_test.go @@ -303,6 +303,9 @@ func NewFakeProxier(ipt utiliptables.Interface) *Proxier { itf1 := net.Interface{Index: 1, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0} addrs1 := []net.Addr{ &net.IPNet{IP: netutils.ParseIPSloppy(testNodeIP), Mask: net.CIDRMask(24, 32)}, + // (This IP never actually gets used; it's only here to test that it gets + // filtered out correctly in the IPv4 nodeport tests.) + &net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(64, 128)}, } networkInterfacer.AddInterfaceAddr(&itf1, addrs1) @@ -327,7 +330,7 @@ func NewFakeProxier(ipt utiliptables.Interface) *Proxier { natRules: utilproxy.LineBuffer{}, nodeIP: netutils.ParseIPSloppy(testNodeIP), localhostNodePorts: true, - nodePortAddresses: utilproxy.NewNodePortAddresses(nil), + nodePortAddresses: utilproxy.NewNodePortAddresses(ipfamily, nil), networkInterfacer: networkInterfacer, } p.setInitialized(true) @@ -2461,7 +2464,7 @@ func TestNodePort(t *testing.T) { func TestHealthCheckNodePort(t *testing.T) { ipt := iptablestest.NewFake() fp := NewFakeProxier(ipt) - fp.nodePortAddresses = utilproxy.NewNodePortAddresses([]string{"127.0.0.0/8"}) + fp.nodePortAddresses = utilproxy.NewNodePortAddresses(v1.IPv4Protocol, []string{"127.0.0.0/8"}) svcIP := "172.30.0.42" svcPort := 80 @@ -3390,7 +3393,7 @@ func TestDisableLocalhostNodePortsIPv4WithNodeAddress(t *testing.T) { fp.localDetector = proxyutiliptables.NewNoOpLocalDetector() fp.localhostNodePorts = false fp.networkInterfacer.InterfaceAddrs() - fp.nodePortAddresses = utilproxy.NewNodePortAddresses([]string{"127.0.0.0/8"}) + fp.nodePortAddresses = utilproxy.NewNodePortAddresses(v1.IPv4Protocol, []string{"127.0.0.0/8"}) expected := dedent.Dedent(` *filter @@ -3671,7 +3674,7 @@ func TestOnlyLocalNodePortsNoClusterCIDR(t *testing.T) { ipt := iptablestest.NewFake() fp := NewFakeProxier(ipt) fp.localDetector = proxyutiliptables.NewNoOpLocalDetector() - fp.nodePortAddresses = utilproxy.NewNodePortAddresses([]string{"192.168.0.0/24"}) + fp.nodePortAddresses = utilproxy.NewNodePortAddresses(v1.IPv4Protocol, []string{"192.168.0.0/24", "2001:db8::/64"}) fp.localhostNodePorts = false expected := dedent.Dedent(` @@ -3720,7 +3723,7 @@ func TestOnlyLocalNodePortsNoClusterCIDR(t *testing.T) { func TestOnlyLocalNodePorts(t *testing.T) { ipt := iptablestest.NewFake() fp := NewFakeProxier(ipt) - fp.nodePortAddresses = utilproxy.NewNodePortAddresses([]string{"192.168.0.0/24"}) + fp.nodePortAddresses = utilproxy.NewNodePortAddresses(v1.IPv4Protocol, []string{"192.168.0.0/24", "2001:db8::/64"}) fp.localhostNodePorts = false expected := dedent.Dedent(` diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index c2c7d316277..b4fc92d3023 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -409,7 +409,7 @@ func NewProxier(ipFamily v1.IPFamily, scheduler = defaultScheduler } - nodePortAddresses := utilproxy.NewNodePortAddresses(nodePortAddressStrings) + nodePortAddresses := utilproxy.NewNodePortAddresses(ipFamily, nodePortAddressStrings) serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer) @@ -490,14 +490,12 @@ func NewDualStackProxier( safeIpset := newSafeIpset(ipset) - ipFamilyMap := utilproxy.MapCIDRsByIPFamily(nodePortAddresses) - // Create an ipv4 instance of the single-stack proxier ipv4Proxier, err := NewProxier(v1.IPv4Protocol, ipt[0], ipvs, safeIpset, sysctl, exec, syncPeriod, minSyncPeriod, filterCIDRs(false, excludeCIDRs), strictARP, tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit, localDetectors[0], hostname, nodeIP[0], - recorder, healthzServer, scheduler, ipFamilyMap[v1.IPv4Protocol], kernelHandler) + recorder, healthzServer, scheduler, nodePortAddresses, kernelHandler) if err != nil { return nil, fmt.Errorf("unable to create ipv4 proxier: %v", err) } @@ -506,7 +504,7 @@ func NewDualStackProxier( exec, syncPeriod, minSyncPeriod, filterCIDRs(true, excludeCIDRs), strictARP, tcpTimeout, tcpFinTimeout, udpTimeout, masqueradeAll, masqueradeBit, localDetectors[1], hostname, nodeIP[1], - recorder, healthzServer, scheduler, ipFamilyMap[v1.IPv6Protocol], kernelHandler) + recorder, healthzServer, scheduler, nodePortAddresses, kernelHandler) if err != nil { return nil, fmt.Errorf("unable to create ipv6 proxier: %v", err) } @@ -1024,9 +1022,7 @@ func (proxier *Proxier) syncProxyRules() { } break } - if getIPFamily(a) == proxier.ipFamily { - nodeIPs = append(nodeIPs, a) - } + nodeIPs = append(nodeIPs, a) } } } diff --git a/pkg/proxy/ipvs/proxier_test.go b/pkg/proxy/ipvs/proxier_test.go index f03d805325e..6ef2756dd27 100644 --- a/pkg/proxy/ipvs/proxier_test.go +++ b/pkg/proxy/ipvs/proxier_test.go @@ -169,7 +169,7 @@ func NewFakeProxier(ipt utiliptables.Interface, ipvs utilipvs.Interface, ipset u filterRules: utilproxy.LineBuffer{}, netlinkHandle: netlinkHandle, ipsetList: ipsetList, - nodePortAddresses: utilproxy.NewNodePortAddresses(nil), + nodePortAddresses: utilproxy.NewNodePortAddresses(ipFamily, nil), networkInterfacer: proxyutiltest.NewFakeNetwork(), gracefuldeleteManager: NewGracefulTerminationManager(ipvs), ipFamily: ipFamily, @@ -960,7 +960,7 @@ func TestNodePortIPv4(t *testing.T) { ipvs := ipvstest.NewFake() ipset := ipsettest.NewFake(testIPSetVersion) fp := NewFakeProxier(ipt, ipvs, ipset, test.nodeIPs, nil, v1.IPv4Protocol) - fp.nodePortAddresses = utilproxy.NewNodePortAddresses(test.nodePortAddresses) + fp.nodePortAddresses = utilproxy.NewNodePortAddresses(v1.IPv4Protocol, test.nodePortAddresses) makeServiceMap(fp, test.services...) populateEndpointSlices(fp, test.endpoints...) @@ -1305,7 +1305,7 @@ func TestNodePortIPv6(t *testing.T) { ipvs := ipvstest.NewFake() ipset := ipsettest.NewFake(testIPSetVersion) fp := NewFakeProxier(ipt, ipvs, ipset, test.nodeIPs, nil, v1.IPv6Protocol) - fp.nodePortAddresses = utilproxy.NewNodePortAddresses(test.nodePortAddresses) + fp.nodePortAddresses = utilproxy.NewNodePortAddresses(v1.IPv6Protocol, test.nodePortAddresses) makeServiceMap(fp, test.services...) populateEndpointSlices(fp, test.endpoints...) @@ -2068,7 +2068,7 @@ func TestOnlyLocalNodePorts(t *testing.T) { addrs1 := []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::"), Mask: net.CIDRMask(64, 128)}} fp.networkInterfacer.(*proxyutiltest.FakeNetwork).AddInterfaceAddr(&itf, addrs) fp.networkInterfacer.(*proxyutiltest.FakeNetwork).AddInterfaceAddr(&itf1, addrs1) - fp.nodePortAddresses = utilproxy.NewNodePortAddresses([]string{"100.101.102.0/24"}) + fp.nodePortAddresses = utilproxy.NewNodePortAddresses(v1.IPv4Protocol, []string{"100.101.102.0/24"}) fp.syncProxyRules() @@ -2156,7 +2156,7 @@ func TestHealthCheckNodePort(t *testing.T) { addrs1 := []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::"), Mask: net.CIDRMask(64, 128)}} fp.networkInterfacer.(*proxyutiltest.FakeNetwork).AddInterfaceAddr(&itf, addrs) fp.networkInterfacer.(*proxyutiltest.FakeNetwork).AddInterfaceAddr(&itf1, addrs1) - fp.nodePortAddresses = utilproxy.NewNodePortAddresses([]string{"100.101.102.0/24"}) + fp.nodePortAddresses = utilproxy.NewNodePortAddresses(v1.IPv4Protocol, []string{"100.101.102.0/24"}) fp.syncProxyRules() diff --git a/pkg/proxy/util/nodeport_addresses.go b/pkg/proxy/util/nodeport_addresses.go index fc03f6bfde1..44eabebc4f7 100644 --- a/pkg/proxy/util/nodeport_addresses.go +++ b/pkg/proxy/util/nodeport_addresses.go @@ -20,6 +20,7 @@ import ( "fmt" "net" + "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" netutils "k8s.io/utils/net" ) @@ -35,27 +36,45 @@ type NodePortAddresses struct { // RFC 5735 127.0.0.0/8 - This block is assigned for use as the Internet host loopback address var ipv4LoopbackStart = net.IPv4(127, 0, 0, 0) -// NewNodePortAddresses takes the `--nodeport-addresses` value (which is assumed to -// contain only valid CIDRs) and returns a NodePortAddresses object. If cidrStrings is -// empty, this is treated as `["0.0.0.0/0", "::/0"]`. -func NewNodePortAddresses(cidrStrings []string) *NodePortAddresses { - if len(cidrStrings) == 0 { - cidrStrings = []string{IPv4ZeroCIDR, IPv6ZeroCIDR} - } - - npa := &NodePortAddresses{ - cidrStrings: cidrStrings, +// NewNodePortAddresses takes an IP family and the `--nodeport-addresses` value (which is +// assumed to contain only valid CIDRs, potentially of both IP families) and returns a +// NodePortAddresses object for the given family. If there are no CIDRs of the given +// family then the CIDR "0.0.0.0/0" or "::/0" will be added (even if there are CIDRs of +// the other family). +func NewNodePortAddresses(family v1.IPFamily, cidrStrings []string) *NodePortAddresses { + npa := &NodePortAddresses{} + + // Filter CIDRs to correct family + for _, str := range cidrStrings { + if (family == v1.IPv4Protocol) == netutils.IsIPv4CIDRString(str) { + npa.cidrStrings = append(npa.cidrStrings, str) + } + } + if len(npa.cidrStrings) == 0 { + if family == v1.IPv4Protocol { + npa.cidrStrings = []string{IPv4ZeroCIDR} + } else { + npa.cidrStrings = []string{IPv6ZeroCIDR} + } } + // Now parse for _, str := range npa.cidrStrings { _, cidr, _ := netutils.ParseCIDRSloppy(str) - npa.cidrs = append(npa.cidrs, cidr) if netutils.IsIPv4CIDR(cidr) { if cidr.IP.IsLoopback() || cidr.Contains(ipv4LoopbackStart) { npa.containsIPv4Loopback = true } } + + if IsZeroCIDR(str) { + // Ignore everything else + npa.cidrs = []*net.IPNet{cidr} + break + } + + npa.cidrs = append(npa.cidrs, cidr) } return npa @@ -65,10 +84,10 @@ func (npa *NodePortAddresses) String() string { return fmt.Sprintf("%v", npa.cidrStrings) } -// GetNodeAddresses return all matched node IP addresses for npa's CIDRs. -// If npa's CIDRs include "0.0.0.0/0" and/or "::/0", then those values will be returned -// verbatim in the response and no actual IPs of that family will be returned. -// If no matching IPs are found, GetNodeAddresses will return an error. +// GetNodeAddresses returns all matched node IP addresses for npa's IP family. If npa's +// CIDRs include "0.0.0.0/0" or "::/0", then that value will be returned verbatim in +// the response and no actual IPs of that family will be returned. If no matching IPs are +// found, GetNodeAddresses will return an error. // NetworkInterfacer is injected for test purpose. func (npa *NodePortAddresses) GetNodeAddresses(nw NetworkInterfacer) (sets.Set[string], error) { uniqueAddressList := sets.New[string]() @@ -77,6 +96,7 @@ func (npa *NodePortAddresses) GetNodeAddresses(nw NetworkInterfacer) (sets.Set[s for _, cidr := range npa.cidrStrings { if IsZeroCIDR(cidr) { uniqueAddressList.Insert(cidr) + return uniqueAddressList, nil } } @@ -85,12 +105,7 @@ func (npa *NodePortAddresses) GetNodeAddresses(nw NetworkInterfacer) (sets.Set[s return nil, fmt.Errorf("error listing all interfaceAddrs from host, error: %v", err) } - // Second round of iteration to parse IPs based on cidr. for _, cidr := range npa.cidrs { - if IsZeroCIDR(cidr.String()) { - continue - } - for _, addr := range addrs { var ip net.IP // nw.InterfaceAddrs may return net.IPAddr or net.IPNet on windows, and it will return net.IPNet on linux. @@ -104,12 +119,7 @@ func (npa *NodePortAddresses) GetNodeAddresses(nw NetworkInterfacer) (sets.Set[s } if cidr.Contains(ip) { - if netutils.IsIPv6(ip) && !uniqueAddressList.Has(IPv6ZeroCIDR) { - uniqueAddressList.Insert(ip.String()) - } - if !netutils.IsIPv6(ip) && !uniqueAddressList.Has(IPv4ZeroCIDR) { - uniqueAddressList.Insert(ip.String()) - } + uniqueAddressList.Insert(ip.String()) } } } diff --git a/pkg/proxy/util/nodeport_addresses_test.go b/pkg/proxy/util/nodeport_addresses_test.go index aba782fce13..d6cf286af07 100644 --- a/pkg/proxy/util/nodeport_addresses_test.go +++ b/pkg/proxy/util/nodeport_addresses_test.go @@ -20,6 +20,7 @@ import ( "net" "testing" + "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" fake "k8s.io/kubernetes/pkg/proxy/util/testing" netutils "k8s.io/utils/net" @@ -31,11 +32,15 @@ type InterfaceAddrsPair struct { } func TestGetNodeAddresses(t *testing.T) { + type expectation struct { + ips sets.Set[string] + } + testCases := []struct { name string cidrs []string itfAddrsPairs []InterfaceAddrsPair - expected sets.Set[string] + expected map[v1.IPFamily]expectation }{ { name: "IPv4 single", @@ -50,7 +55,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("100.200.201.1"), Mask: net.CIDRMask(24, 32)}}, }, }, - expected: sets.New[string]("10.20.30.51"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("10.20.30.51"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "IPv4 zero CIDR", @@ -65,7 +77,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, }, }, - expected: sets.New[string]("0.0.0.0/0"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("0.0.0.0/0"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "IPv6 multiple", @@ -80,7 +99,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, }, }, - expected: sets.New[string]("2001:db8::1", "::1"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("0.0.0.0/0"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("2001:db8::1", "::1"), + }, + }, }, { name: "IPv6 zero CIDR", @@ -95,7 +121,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, }, }, - expected: sets.New[string]("::/0"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("0.0.0.0/0"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "IPv4 localhost exact", @@ -110,7 +143,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, }, }, - expected: sets.New[string]("127.0.0.1"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("127.0.0.1"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "IPv4 localhost subnet", @@ -121,7 +161,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.1.1"), Mask: net.CIDRMask(8, 32)}}, }, }, - expected: sets.New[string]("127.0.1.1"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("127.0.1.1"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "IPv4 multiple", @@ -136,7 +183,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("100.200.201.1"), Mask: net.CIDRMask(24, 32)}}, }, }, - expected: sets.New[string]("10.20.30.51", "100.200.201.1"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("10.20.30.51", "100.200.201.1"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "IPv4 multiple, no match", @@ -151,7 +205,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, }, }, - expected: nil, + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: nil, + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "empty list, IPv4 addrs", @@ -166,7 +227,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, }, }, - expected: sets.New[string]("0.0.0.0/0", "::/0"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("0.0.0.0/0"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "empty list, IPv6 addrs", @@ -181,7 +249,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, }, }, - expected: sets.New[string]("0.0.0.0/0", "::/0"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("0.0.0.0/0"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "IPv4 redundant CIDRs", @@ -192,7 +267,14 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, }, }, - expected: sets.New[string]("0.0.0.0/0"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("0.0.0.0/0"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, { name: "Dual-stack, redundant IPv4", @@ -213,7 +295,14 @@ func TestGetNodeAddresses(t *testing.T) { }, }, }, - expected: sets.New[string]("0.0.0.0/0", "2001:db8::1"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("0.0.0.0/0"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("2001:db8::1"), + }, + }, }, { name: "Dual-stack, redundant IPv6", @@ -234,7 +323,14 @@ func TestGetNodeAddresses(t *testing.T) { }, }, }, - expected: sets.New[string]("::/0", "1.2.3.4"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + ips: sets.New[string]("1.2.3.4"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("::/0"), + }, + }, }, } @@ -245,16 +341,20 @@ func TestGetNodeAddresses(t *testing.T) { nw.AddInterfaceAddr(&pair.itf, pair.addrs) } - npa := NewNodePortAddresses(tc.cidrs) - addrList, err := npa.GetNodeAddresses(nw) - // The fake InterfaceAddrs() never returns an error, so the only - // error GetNodeAddresses will return is "no addresses found". - if err != nil && tc.expected != nil { - t.Errorf("unexpected error: %v", err) - } + for _, family := range []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol} { + npa := NewNodePortAddresses(family, tc.cidrs) - if !addrList.Equal(tc.expected) { - t.Errorf("unexpected mismatch, expected: %v, got: %v", tc.expected, addrList) + addrList, err := npa.GetNodeAddresses(nw) + // The fake InterfaceAddrs() never returns an error, so + // the only error GetNodeAddresses will return is "no + // addresses found". + if err != nil && tc.expected[family].ips != nil { + t.Errorf("unexpected error: %v", err) + } + + if !addrList.Equal(tc.expected[family].ips) { + t.Errorf("unexpected mismatch for %s, expected: %v, got: %v", family, tc.expected[family].ips, addrList) + } } }) } @@ -308,9 +408,14 @@ func TestContainsIPv4Loopback(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - npa := NewNodePortAddresses(tt.cidrStrings) + npa := NewNodePortAddresses(v1.IPv4Protocol, tt.cidrStrings) if got := npa.ContainsIPv4Loopback(); got != tt.want { - t.Errorf("ContainsIPv4Loopback() = %v, want %v", got, tt.want) + t.Errorf("IPv4 ContainsIPv4Loopback() = %v, want %v", got, tt.want) + } + // ContainsIPv4Loopback should always be false for family=IPv6 + npa = NewNodePortAddresses(v1.IPv6Protocol, tt.cidrStrings) + if got := npa.ContainsIPv4Loopback(); got { + t.Errorf("IPv6 ContainsIPv4Loopback() = %v, want %v", got, false) } }) } diff --git a/pkg/proxy/winkernel/proxier.go b/pkg/proxy/winkernel/proxier.go index 8f0dde7d6ea..2f12451896c 100644 --- a/pkg/proxy/winkernel/proxier.go +++ b/pkg/proxy/winkernel/proxier.go @@ -685,8 +685,14 @@ func NewProxier( klog.InfoS("ClusterCIDR not specified, unable to distinguish between internal and external traffic") } + isIPv6 := netutils.IsIPv6(nodeIP) + ipFamily := v1.IPv4Protocol + if isIPv6 { + ipFamily = v1.IPv6Protocol + } + // windows listens to all node addresses - nodePortAddresses := utilproxy.NewNodePortAddresses(nil) + nodePortAddresses := utilproxy.NewNodePortAddresses(ipFamily, nil) serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer) hns, supportedFeatures := newHostNetworkService() @@ -764,7 +770,6 @@ func NewProxier( } } - isIPv6 := netutils.IsIPv6(nodeIP) proxier := &Proxier{ endPointsRefCount: make(endPointsReferenceCountMap), svcPortMap: make(proxy.ServicePortMap), @@ -788,10 +793,6 @@ func NewProxier( mapStaleLoadbalancers: make(map[string]bool), } - ipFamily := v1.IPv4Protocol - if isIPv6 { - ipFamily = v1.IPv6Protocol - } serviceChanges := proxy.NewServiceChangeTracker(proxier.newServiceInfo, ipFamily, recorder, proxier.serviceMapChange) endPointChangeTracker := proxy.NewEndpointChangeTracker(hostname, proxier.newEndpointInfo, ipFamily, recorder, proxier.endpointsMapChange) proxier.endpointsChanges = endPointChangeTracker From 2ca215fd994192291e2856f7f732a3bd3cbdb79e Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sat, 24 Dec 2022 13:21:17 -0500 Subject: [PATCH 3/5] Add NodePortAddresses.MatchAll() Rather than having GetNodeAddresses() return a special magic value indicating that it matches all IPs, add a separate method to check that. (And have GetNodeAddresses() just return the IPs as expected instead.) --- pkg/proxy/healthcheck/service_health.go | 20 +++--- pkg/proxy/iptables/proxier.go | 82 +++++++++++------------ pkg/proxy/ipvs/proxier.go | 27 ++++---- pkg/proxy/util/nodeport_addresses.go | 28 +++----- pkg/proxy/util/nodeport_addresses_test.go | 63 +++++++++++------ 5 files changed, 115 insertions(+), 105 deletions(-) diff --git a/pkg/proxy/healthcheck/service_health.go b/pkg/proxy/healthcheck/service_health.go index bb62bf607ae..72fd67744b2 100644 --- a/pkg/proxy/healthcheck/service_health.go +++ b/pkg/proxy/healthcheck/service_health.go @@ -59,20 +59,18 @@ type proxierHealthChecker interface { } func newServiceHealthServer(hostname string, recorder events.EventRecorder, listener listener, factory httpServerFactory, nodePortAddresses *utilproxy.NodePortAddresses, healthzServer proxierHealthChecker) ServiceHealthServer { - - nodeAddresses, err := nodePortAddresses.GetNodeAddresses(utilproxy.RealNetwork{}) - if err != nil || nodeAddresses.Len() == 0 { - klog.ErrorS(err, "Failed to get node ip address matching node port addresses, health check port will listen to all node addresses", "nodePortAddresses", nodePortAddresses) - nodeAddresses = sets.New[string]() - nodeAddresses.Insert(utilproxy.IPv4ZeroCIDR) - } + var nodeAddresses sets.Set[string] // if any of the addresses is zero cidr then we listen // to old style : - for _, addr := range nodeAddresses.UnsortedList() { - if utilproxy.IsZeroCIDR(addr) { - nodeAddresses = sets.New[string]("") - break + if nodePortAddresses.MatchAll() { + nodeAddresses = sets.New("") + } else { + var err error + nodeAddresses, err = nodePortAddresses.GetNodeAddresses(utilproxy.RealNetwork{}) + if err != nil || nodeAddresses.Len() == 0 { + klog.ErrorS(err, "Failed to get node ip address matching node port addresses, health check port will listen to all node addresses", "nodePortAddresses", nodePortAddresses) + nodeAddresses = sets.New(utilproxy.IPv4ZeroCIDR) } } diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index eb50769daf4..29120a40768 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -1415,55 +1415,53 @@ func (proxier *Proxier) syncProxyRules() { // Finally, tail-call to the nodePorts chain. This needs to be after all // other service portal rules. - nodeAddresses, err := proxier.nodePortAddresses.GetNodeAddresses(proxier.networkInterfacer) - if err != nil { - klog.ErrorS(err, "Failed to get node ip address matching nodeport cidrs, services with nodeport may not work as intended", "CIDRs", proxier.nodePortAddresses) - } - - for address := range nodeAddresses { - if utilproxy.IsZeroCIDR(address) { - destinations := []string{"-m", "addrtype", "--dst-type", "LOCAL"} - if isIPv6 { - // For IPv6, Regardless of the value of localhostNodePorts is true - // or false, we should disable access to the nodePort via localhost. Since it never works and always - // cause kernel warnings. - destinations = append(destinations, "!", "-d", "::1/128") - } - - if !proxier.localhostNodePorts && !isIPv6 { - // If set localhostNodePorts to "false"(route_localnet=0), We should generate iptables rules that - // disable NodePort services to be accessed via localhost. Since it doesn't work and causes - // the kernel to log warnings if anyone tries. - destinations = append(destinations, "!", "-d", "127.0.0.0/8") - } - - proxier.natRules.Write( - "-A", string(kubeServicesChain), - "-m", "comment", "--comment", `"kubernetes service nodeports; NOTE: this must be the last rule in this chain"`, - destinations, - "-j", string(kubeNodePortsChain)) - break + if proxier.nodePortAddresses.MatchAll() { + destinations := []string{"-m", "addrtype", "--dst-type", "LOCAL"} + if isIPv6 { + // For IPv6, Regardless of the value of localhostNodePorts is true + // or false, we should disable access to the nodePort via localhost. Since it never works and always + // cause kernel warnings. + destinations = append(destinations, "!", "-d", "::1/128") } - // For ipv6, Regardless of the value of localhostNodePorts is true or false, we should disallow access - // to the nodePort via lookBack address. - if isIPv6 && utilproxy.IsLoopBack(address) { - klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv6 localhost address", "IP", address) - continue + if !proxier.localhostNodePorts && !isIPv6 { + // If set localhostNodePorts to "false"(route_localnet=0), We should generate iptables rules that + // disable NodePort services to be accessed via localhost. Since it doesn't work and causes + // the kernel to log warnings if anyone tries. + destinations = append(destinations, "!", "-d", "127.0.0.0/8") } - // For ipv4, When localhostNodePorts is set to false, Ignore ipv4 lookBack address - if !isIPv6 && utilproxy.IsLoopBack(address) && !proxier.localhostNodePorts { - klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv4 localhost address", "IP", address) - continue - } - - // create nodeport rules for each IP one by one proxier.natRules.Write( "-A", string(kubeServicesChain), "-m", "comment", "--comment", `"kubernetes service nodeports; NOTE: this must be the last rule in this chain"`, - "-d", address, + destinations, "-j", string(kubeNodePortsChain)) + } else { + nodeAddresses, err := proxier.nodePortAddresses.GetNodeAddresses(proxier.networkInterfacer) + if err != nil { + klog.ErrorS(err, "Failed to get node ip address matching nodeport cidrs, services with nodeport may not work as intended", "CIDRs", proxier.nodePortAddresses) + } + for address := range nodeAddresses { + // For ipv6, Regardless of the value of localhostNodePorts is true or false, we should disallow access + // to the nodePort via lookBack address. + if isIPv6 && utilproxy.IsLoopBack(address) { + klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv6 localhost address", "IP", address) + continue + } + + // For ipv4, When localhostNodePorts is set to false, Ignore ipv4 lookBack address + if !isIPv6 && utilproxy.IsLoopBack(address) && !proxier.localhostNodePorts { + klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv4 localhost address", "IP", address) + continue + } + + // create nodeport rules for each IP one by one + proxier.natRules.Write( + "-A", string(kubeServicesChain), + "-m", "comment", "--comment", `"kubernetes service nodeports; NOTE: this must be the last rule in this chain"`, + "-d", address, + "-j", string(kubeNodePortsChain)) + } } // Drop the packets in INVALID state, which would potentially cause @@ -1521,7 +1519,7 @@ func (proxier *Proxier) syncProxyRules() { klog.V(9).InfoS("Restoring iptables", "rules", proxier.iptablesData.Bytes()) // NOTE: NoFlushTables is used so we don't flush non-kubernetes chains in the table - err = proxier.iptables.RestoreAll(proxier.iptablesData.Bytes(), utiliptables.NoFlushTables, utiliptables.RestoreCounters) + err := proxier.iptables.RestoreAll(proxier.iptablesData.Bytes(), utiliptables.NoFlushTables, utiliptables.RestoreCounters) if err != nil { if pErr, ok := err.(utiliptables.ParseError); ok { lines := utiliptables.ExtractLines(proxier.iptablesData.Bytes(), pErr.Line(), 3) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index b4fc92d3023..9fe380e1696 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1006,23 +1006,22 @@ func (proxier *Proxier) syncProxyRules() { // can be reused for all nodePort services. var nodeIPs []net.IP if hasNodePort { - nodeAddrSet, err := proxier.nodePortAddresses.GetNodeAddresses(proxier.networkInterfacer) - if err != nil { - klog.ErrorS(err, "Failed to get node IP address matching nodeport cidr") + if proxier.nodePortAddresses.MatchAll() { + for _, ipStr := range nodeAddressSet.UnsortedList() { + nodeIPs = append(nodeIPs, netutils.ParseIPSloppy(ipStr)) + } } else { - for _, address := range nodeAddrSet.UnsortedList() { - a := netutils.ParseIPSloppy(address) - if a.IsLoopback() { - continue - } - if utilproxy.IsZeroCIDR(address) { - nodeIPs = nil - for _, ipStr := range nodeAddressSet.UnsortedList() { - nodeIPs = append(nodeIPs, netutils.ParseIPSloppy(ipStr)) + nodeAddrSet, err := proxier.nodePortAddresses.GetNodeAddresses(proxier.networkInterfacer) + if err != nil { + klog.ErrorS(err, "Failed to get node IP address matching nodeport cidr") + } else { + for address := range nodeAddrSet { + a := netutils.ParseIPSloppy(address) + if a.IsLoopback() { + continue } - break + nodeIPs = append(nodeIPs, a) } - nodeIPs = append(nodeIPs, a) } } } diff --git a/pkg/proxy/util/nodeport_addresses.go b/pkg/proxy/util/nodeport_addresses.go index 44eabebc4f7..b257c9808a7 100644 --- a/pkg/proxy/util/nodeport_addresses.go +++ b/pkg/proxy/util/nodeport_addresses.go @@ -31,6 +31,7 @@ type NodePortAddresses struct { cidrs []*net.IPNet containsIPv4Loopback bool + matchAll bool } // RFC 5735 127.0.0.0/8 - This block is assigned for use as the Internet host loopback address @@ -71,6 +72,7 @@ func NewNodePortAddresses(family v1.IPFamily, cidrStrings []string) *NodePortAdd if IsZeroCIDR(str) { // Ignore everything else npa.cidrs = []*net.IPNet{cidr} + npa.matchAll = true break } @@ -84,27 +86,21 @@ func (npa *NodePortAddresses) String() string { return fmt.Sprintf("%v", npa.cidrStrings) } -// GetNodeAddresses returns all matched node IP addresses for npa's IP family. If npa's -// CIDRs include "0.0.0.0/0" or "::/0", then that value will be returned verbatim in -// the response and no actual IPs of that family will be returned. If no matching IPs are -// found, GetNodeAddresses will return an error. +// MatchAll returns true if npa matches all node IPs (of npa's given family) +func (npa *NodePortAddresses) MatchAll() bool { + return npa.matchAll +} + +// GetNodeAddresses return all matched node IP addresses for npa's CIDRs. If no matching +// IPs are found, it returns an empty list. // NetworkInterfacer is injected for test purpose. func (npa *NodePortAddresses) GetNodeAddresses(nw NetworkInterfacer) (sets.Set[string], error) { - uniqueAddressList := sets.New[string]() - - // First round of iteration to pick out `0.0.0.0/0` or `::/0` for the sake of excluding non-zero IPs. - for _, cidr := range npa.cidrStrings { - if IsZeroCIDR(cidr) { - uniqueAddressList.Insert(cidr) - return uniqueAddressList, nil - } - } - addrs, err := nw.InterfaceAddrs() if err != nil { return nil, fmt.Errorf("error listing all interfaceAddrs from host, error: %v", err) } + uniqueAddressList := sets.New[string]() for _, cidr := range npa.cidrs { for _, addr := range addrs { var ip net.IP @@ -124,10 +120,6 @@ func (npa *NodePortAddresses) GetNodeAddresses(nw NetworkInterfacer) (sets.Set[s } } - if uniqueAddressList.Len() == 0 { - return nil, fmt.Errorf("no addresses found for cidrs %v", npa.cidrStrings) - } - return uniqueAddressList, nil } diff --git a/pkg/proxy/util/nodeport_addresses_test.go b/pkg/proxy/util/nodeport_addresses_test.go index d6cf286af07..32a18b3ce00 100644 --- a/pkg/proxy/util/nodeport_addresses_test.go +++ b/pkg/proxy/util/nodeport_addresses_test.go @@ -33,7 +33,8 @@ type InterfaceAddrsPair struct { func TestGetNodeAddresses(t *testing.T) { type expectation struct { - ips sets.Set[string] + matchAll bool + ips sets.Set[string] } testCases := []struct { @@ -60,7 +61,8 @@ func TestGetNodeAddresses(t *testing.T) { ips: sets.New[string]("10.20.30.51"), }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: nil, }, }, }, @@ -79,10 +81,12 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: map[v1.IPFamily]expectation{ v1.IPv4Protocol: { - ips: sets.New[string]("0.0.0.0/0"), + matchAll: true, + ips: sets.New[string]("10.20.30.51", "127.0.0.1"), }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: nil, }, }, }, @@ -101,7 +105,8 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: map[v1.IPFamily]expectation{ v1.IPv4Protocol: { - ips: sets.New[string]("0.0.0.0/0"), + matchAll: true, + ips: nil, }, v1.IPv6Protocol: { ips: sets.New[string]("2001:db8::1", "::1"), @@ -123,10 +128,12 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: map[v1.IPFamily]expectation{ v1.IPv4Protocol: { - ips: sets.New[string]("0.0.0.0/0"), + matchAll: true, + ips: nil, }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: sets.New[string]("2001:db8::1", "::1"), }, }, }, @@ -148,7 +155,8 @@ func TestGetNodeAddresses(t *testing.T) { ips: sets.New[string]("127.0.0.1"), }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: nil, }, }, }, @@ -166,7 +174,8 @@ func TestGetNodeAddresses(t *testing.T) { ips: sets.New[string]("127.0.1.1"), }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: nil, }, }, }, @@ -188,7 +197,8 @@ func TestGetNodeAddresses(t *testing.T) { ips: sets.New[string]("10.20.30.51", "100.200.201.1"), }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: nil, }, }, }, @@ -210,7 +220,8 @@ func TestGetNodeAddresses(t *testing.T) { ips: nil, }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: nil, }, }, }, @@ -229,10 +240,12 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: map[v1.IPFamily]expectation{ v1.IPv4Protocol: { - ips: sets.New[string]("0.0.0.0/0"), + matchAll: true, + ips: sets.New[string]("192.168.1.2", "127.0.0.1"), }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: nil, }, }, }, @@ -251,10 +264,12 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: map[v1.IPFamily]expectation{ v1.IPv4Protocol: { - ips: sets.New[string]("0.0.0.0/0"), + matchAll: true, + ips: nil, }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: sets.New[string]("2001:db8::1", "::1"), }, }, }, @@ -269,10 +284,12 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: map[v1.IPFamily]expectation{ v1.IPv4Protocol: { - ips: sets.New[string]("0.0.0.0/0"), + matchAll: true, + ips: sets.New[string]("1.2.3.4"), }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: nil, }, }, }, @@ -297,7 +314,8 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: map[v1.IPFamily]expectation{ v1.IPv4Protocol: { - ips: sets.New[string]("0.0.0.0/0"), + matchAll: true, + ips: sets.New[string]("1.2.3.4", "127.0.0.1"), }, v1.IPv6Protocol: { ips: sets.New[string]("2001:db8::1"), @@ -328,7 +346,8 @@ func TestGetNodeAddresses(t *testing.T) { ips: sets.New[string]("1.2.3.4"), }, v1.IPv6Protocol: { - ips: sets.New[string]("::/0"), + matchAll: true, + ips: sets.New[string]("2001:db8::1", "::1"), }, }, }, @@ -344,11 +363,15 @@ func TestGetNodeAddresses(t *testing.T) { for _, family := range []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol} { npa := NewNodePortAddresses(family, tc.cidrs) + if npa.MatchAll() != tc.expected[family].matchAll { + t.Errorf("unexpected MatchAll(%s), expected: %v", family, tc.expected[family].matchAll) + } + addrList, err := npa.GetNodeAddresses(nw) // The fake InterfaceAddrs() never returns an error, so // the only error GetNodeAddresses will return is "no // addresses found". - if err != nil && tc.expected[family].ips != nil { + if err != nil { t.Errorf("unexpected error: %v", err) } From a744a186b67b3f1304becd62498442ab3b2c85fc Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 3 Mar 2023 17:43:15 -0500 Subject: [PATCH 4/5] Rename GetNodeAddresses to GetNodeIPs, return net.IP --- pkg/proxy/healthcheck/healthcheck_test.go | 2 +- pkg/proxy/healthcheck/service_health.go | 36 ++++++++++------------- pkg/proxy/iptables/proxier.go | 14 ++++----- pkg/proxy/ipvs/proxier.go | 10 +++---- pkg/proxy/util/nodeport_addresses.go | 17 +++++++---- pkg/proxy/util/nodeport_addresses_test.go | 32 ++++++++++++++++---- 6 files changed, 65 insertions(+), 46 deletions(-) diff --git a/pkg/proxy/healthcheck/healthcheck_test.go b/pkg/proxy/healthcheck/healthcheck_test.go index 4d6734145d6..a1b92f7e7f9 100644 --- a/pkg/proxy/healthcheck/healthcheck_test.go +++ b/pkg/proxy/healthcheck/healthcheck_test.go @@ -178,7 +178,7 @@ func TestServer(t *testing.T) { if len(listener.openPorts) != 1 { t.Errorf("expected 1 open port, got %d\n%s", len(listener.openPorts), dump.Pretty(listener.openPorts)) } - if !listener.hasPort(":9376") { + if !listener.hasPort("0.0.0.0:9376") { t.Errorf("expected port :9376 to be open\n%s", dump.Pretty(listener.openPorts)) } // test the handler diff --git a/pkg/proxy/healthcheck/service_health.go b/pkg/proxy/healthcheck/service_health.go index 72fd67744b2..e6af90abc5a 100644 --- a/pkg/proxy/healthcheck/service_health.go +++ b/pkg/proxy/healthcheck/service_health.go @@ -32,7 +32,6 @@ import ( api "k8s.io/kubernetes/pkg/apis/core" utilerrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/apimachinery/pkg/util/sets" utilproxy "k8s.io/kubernetes/pkg/proxy/util" ) @@ -59,18 +58,16 @@ type proxierHealthChecker interface { } func newServiceHealthServer(hostname string, recorder events.EventRecorder, listener listener, factory httpServerFactory, nodePortAddresses *utilproxy.NodePortAddresses, healthzServer proxierHealthChecker) ServiceHealthServer { - var nodeAddresses sets.Set[string] + // It doesn't matter whether we listen on "0.0.0.0", "::", or ""; go + // treats them all the same. + nodeIPs := []net.IP{net.IPv4zero} - // if any of the addresses is zero cidr then we listen - // to old style : - if nodePortAddresses.MatchAll() { - nodeAddresses = sets.New("") - } else { - var err error - nodeAddresses, err = nodePortAddresses.GetNodeAddresses(utilproxy.RealNetwork{}) - if err != nil || nodeAddresses.Len() == 0 { + if !nodePortAddresses.MatchAll() { + ips, err := nodePortAddresses.GetNodeIPs(utilproxy.RealNetwork{}) + if err == nil { + nodeIPs = ips + } else { klog.ErrorS(err, "Failed to get node ip address matching node port addresses, health check port will listen to all node addresses", "nodePortAddresses", nodePortAddresses) - nodeAddresses = sets.New(utilproxy.IPv4ZeroCIDR) } } @@ -81,7 +78,7 @@ func newServiceHealthServer(hostname string, recorder events.EventRecorder, list httpFactory: factory, healthzServer: healthzServer, services: map[types.NamespacedName]*hcInstance{}, - nodeAddresses: nodeAddresses, + nodeIPs: nodeIPs, } } @@ -93,10 +90,10 @@ func NewServiceHealthServer(hostname string, recorder events.EventRecorder, node type server struct { hostname string // node addresses where health check port will listen on - nodeAddresses sets.Set[string] - recorder events.EventRecorder // can be nil - listener listener - httpFactory httpServerFactory + nodeIPs []net.IP + recorder events.EventRecorder // can be nil + listener listener + httpFactory httpServerFactory healthzServer proxierHealthChecker @@ -167,12 +164,11 @@ func (hcI *hcInstance) listenAndServeAll(hcs *server) error { var err error var listener net.Listener - addresses := hcs.nodeAddresses.UnsortedList() - hcI.httpServers = make([]httpServer, 0, len(addresses)) + hcI.httpServers = make([]httpServer, 0, len(hcs.nodeIPs)) // for each of the node addresses start listening and serving - for _, address := range addresses { - addr := net.JoinHostPort(address, fmt.Sprint(hcI.port)) + for _, ip := range hcs.nodeIPs { + addr := net.JoinHostPort(ip.String(), fmt.Sprint(hcI.port)) // create http server httpSrv := hcs.httpFactory.New(addr, hcHandler{name: hcI.nsn, hcs: hcs}) // start listener diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index 29120a40768..bb8043d04a8 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -1437,21 +1437,21 @@ func (proxier *Proxier) syncProxyRules() { destinations, "-j", string(kubeNodePortsChain)) } else { - nodeAddresses, err := proxier.nodePortAddresses.GetNodeAddresses(proxier.networkInterfacer) + nodeIPs, err := proxier.nodePortAddresses.GetNodeIPs(proxier.networkInterfacer) if err != nil { klog.ErrorS(err, "Failed to get node ip address matching nodeport cidrs, services with nodeport may not work as intended", "CIDRs", proxier.nodePortAddresses) } - for address := range nodeAddresses { + for _, ip := range nodeIPs { // For ipv6, Regardless of the value of localhostNodePorts is true or false, we should disallow access // to the nodePort via lookBack address. - if isIPv6 && utilproxy.IsLoopBack(address) { - klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv6 localhost address", "IP", address) + if isIPv6 && ip.IsLoopback() { + klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv6 localhost address", "IP", ip.String()) continue } // For ipv4, When localhostNodePorts is set to false, Ignore ipv4 lookBack address - if !isIPv6 && utilproxy.IsLoopBack(address) && !proxier.localhostNodePorts { - klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv4 localhost address", "IP", address) + if !isIPv6 && ip.IsLoopback() && !proxier.localhostNodePorts { + klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv4 localhost address", "IP", ip.String()) continue } @@ -1459,7 +1459,7 @@ func (proxier *Proxier) syncProxyRules() { proxier.natRules.Write( "-A", string(kubeServicesChain), "-m", "comment", "--comment", `"kubernetes service nodeports; NOTE: this must be the last rule in this chain"`, - "-d", address, + "-d", ip.String(), "-j", string(kubeNodePortsChain)) } } diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 9fe380e1696..2c8e0a34af2 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1011,16 +1011,14 @@ func (proxier *Proxier) syncProxyRules() { nodeIPs = append(nodeIPs, netutils.ParseIPSloppy(ipStr)) } } else { - nodeAddrSet, err := proxier.nodePortAddresses.GetNodeAddresses(proxier.networkInterfacer) + allNodeIPs, err := proxier.nodePortAddresses.GetNodeIPs(proxier.networkInterfacer) if err != nil { klog.ErrorS(err, "Failed to get node IP address matching nodeport cidr") } else { - for address := range nodeAddrSet { - a := netutils.ParseIPSloppy(address) - if a.IsLoopback() { - continue + for _, ip := range allNodeIPs { + if !ip.IsLoopback() { + nodeIPs = append(nodeIPs, ip) } - nodeIPs = append(nodeIPs, a) } } } diff --git a/pkg/proxy/util/nodeport_addresses.go b/pkg/proxy/util/nodeport_addresses.go index b257c9808a7..c5332a07958 100644 --- a/pkg/proxy/util/nodeport_addresses.go +++ b/pkg/proxy/util/nodeport_addresses.go @@ -21,7 +21,6 @@ import ( "net" "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/util/sets" netutils "k8s.io/utils/net" ) @@ -91,16 +90,17 @@ func (npa *NodePortAddresses) MatchAll() bool { return npa.matchAll } -// GetNodeAddresses return all matched node IP addresses for npa's CIDRs. If no matching +// GetNodeIPs return all matched node IP addresses for npa's CIDRs. If no matching // IPs are found, it returns an empty list. // NetworkInterfacer is injected for test purpose. -func (npa *NodePortAddresses) GetNodeAddresses(nw NetworkInterfacer) (sets.Set[string], error) { +func (npa *NodePortAddresses) GetNodeIPs(nw NetworkInterfacer) ([]net.IP, error) { addrs, err := nw.InterfaceAddrs() if err != nil { return nil, fmt.Errorf("error listing all interfaceAddrs from host, error: %v", err) } - uniqueAddressList := sets.New[string]() + // Use a map to dedup matches + addresses := make(map[string]net.IP) for _, cidr := range npa.cidrs { for _, addr := range addrs { var ip net.IP @@ -115,12 +115,17 @@ func (npa *NodePortAddresses) GetNodeAddresses(nw NetworkInterfacer) (sets.Set[s } if cidr.Contains(ip) { - uniqueAddressList.Insert(ip.String()) + addresses[ip.String()] = ip } } } - return uniqueAddressList, nil + ips := make([]net.IP, 0, len(addresses)) + for _, ip := range addresses { + ips = append(ips, ip) + } + + return ips, nil } // ContainsIPv4Loopback returns true if npa's CIDRs contain an IPv4 loopback address. diff --git a/pkg/proxy/util/nodeport_addresses_test.go b/pkg/proxy/util/nodeport_addresses_test.go index 32a18b3ce00..c66db1b024f 100644 --- a/pkg/proxy/util/nodeport_addresses_test.go +++ b/pkg/proxy/util/nodeport_addresses_test.go @@ -17,6 +17,7 @@ limitations under the License. package util import ( + "fmt" "net" "testing" @@ -31,7 +32,24 @@ type InterfaceAddrsPair struct { addrs []net.Addr } -func TestGetNodeAddresses(t *testing.T) { +func checkNodeIPs(expected sets.Set[string], actual []net.IP) error { + notFound := expected.Clone() + extra := sets.New[string]() + for _, ip := range actual { + str := ip.String() + if notFound.Has(str) { + notFound.Delete(str) + } else { + extra.Insert(str) + } + } + if len(notFound) != 0 || len(extra) != 0 { + return fmt.Errorf("not found: %v, extra: %v", notFound.UnsortedList(), extra.UnsortedList()) + } + return nil +} + +func TestGetNodeIPs(t *testing.T) { type expectation struct { matchAll bool ips sets.Set[string] @@ -367,16 +385,18 @@ func TestGetNodeAddresses(t *testing.T) { t.Errorf("unexpected MatchAll(%s), expected: %v", family, tc.expected[family].matchAll) } - addrList, err := npa.GetNodeAddresses(nw) + ips, err := npa.GetNodeIPs(nw) + expectedIPs := tc.expected[family].ips + // The fake InterfaceAddrs() never returns an error, so - // the only error GetNodeAddresses will return is "no + // the only error GetNodeIPs will return is "no // addresses found". if err != nil { t.Errorf("unexpected error: %v", err) } - - if !addrList.Equal(tc.expected[family].ips) { - t.Errorf("unexpected mismatch for %s, expected: %v, got: %v", family, tc.expected[family].ips, addrList) + err = checkNodeIPs(expectedIPs, ips) + if err != nil { + t.Errorf("unexpected mismatch for %s: %v", family, err) } } }) From 0e456dcf86689817656d6a304fdf5466036985f4 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Mon, 15 May 2023 11:05:17 -0400 Subject: [PATCH 5/5] Clarify localhost nodeport comments/errors --- pkg/proxy/iptables/proxier.go | 31 +++++++++++-------------------- 1 file changed, 11 insertions(+), 20 deletions(-) diff --git a/pkg/proxy/iptables/proxier.go b/pkg/proxy/iptables/proxier.go index bb8043d04a8..7b0011bfc17 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -1417,17 +1417,11 @@ func (proxier *Proxier) syncProxyRules() { // other service portal rules. if proxier.nodePortAddresses.MatchAll() { destinations := []string{"-m", "addrtype", "--dst-type", "LOCAL"} + // Block localhost nodePorts if they are not supported. (For IPv6 they never + // work, and for IPv4 they only work if we previously set `route_localnet`.) if isIPv6 { - // For IPv6, Regardless of the value of localhostNodePorts is true - // or false, we should disable access to the nodePort via localhost. Since it never works and always - // cause kernel warnings. destinations = append(destinations, "!", "-d", "::1/128") - } - - if !proxier.localhostNodePorts && !isIPv6 { - // If set localhostNodePorts to "false"(route_localnet=0), We should generate iptables rules that - // disable NodePort services to be accessed via localhost. Since it doesn't work and causes - // the kernel to log warnings if anyone tries. + } else if !proxier.localhostNodePorts { destinations = append(destinations, "!", "-d", "127.0.0.0/8") } @@ -1442,17 +1436,14 @@ func (proxier *Proxier) syncProxyRules() { klog.ErrorS(err, "Failed to get node ip address matching nodeport cidrs, services with nodeport may not work as intended", "CIDRs", proxier.nodePortAddresses) } for _, ip := range nodeIPs { - // For ipv6, Regardless of the value of localhostNodePorts is true or false, we should disallow access - // to the nodePort via lookBack address. - if isIPv6 && ip.IsLoopback() { - klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv6 localhost address", "IP", ip.String()) - continue - } - - // For ipv4, When localhostNodePorts is set to false, Ignore ipv4 lookBack address - if !isIPv6 && ip.IsLoopback() && !proxier.localhostNodePorts { - klog.ErrorS(nil, "disallow nodePort services to be accessed via ipv4 localhost address", "IP", ip.String()) - continue + if ip.IsLoopback() { + if isIPv6 { + klog.ErrorS(nil, "--nodeport-addresses includes localhost but localhost NodePorts are not supported on IPv6", "address", ip.String()) + continue + } else if !proxier.localhostNodePorts { + klog.ErrorS(nil, "--nodeport-addresses includes localhost but --iptables-localhost-nodeports=false was passed", "address", ip.String()) + continue + } } // create nodeport rules for each IP one by one