diff --git a/pkg/proxy/healthcheck/healthcheck_test.go b/pkg/proxy/healthcheck/healthcheck_test.go index bbb7489e67d..a1b92f7e7f9 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) @@ -177,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 @@ -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/healthcheck/service_health.go b/pkg/proxy/healthcheck/service_health.go index bb62bf607ae..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,20 +58,16 @@ type proxierHealthChecker interface { } func newServiceHealthServer(hostname string, recorder events.EventRecorder, listener listener, factory httpServerFactory, nodePortAddresses *utilproxy.NodePortAddresses, healthzServer proxierHealthChecker) ServiceHealthServer { + // It doesn't matter whether we listen on "0.0.0.0", "::", or ""; go + // treats them all the same. + nodeIPs := []net.IP{net.IPv4zero} - 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) - } - - // 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() { + 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) } } @@ -83,7 +78,7 @@ func newServiceHealthServer(hostname string, recorder events.EventRecorder, list httpFactory: factory, healthzServer: healthzServer, services: map[types.NamespacedName]*hcInstance{}, - nodeAddresses: nodeAddresses, + nodeIPs: nodeIPs, } } @@ -95,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 @@ -169,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 68f259c0e6a..7b0011bfc17 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) } @@ -1416,70 +1415,44 @@ 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) - } - // 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) { - 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"} + // 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 { + destinations = append(destinations, "!", "-d", "::1/128") + } else if !proxier.localhostNodePorts { + destinations = append(destinations, "!", "-d", "127.0.0.0/8") } - // 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) { - 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, + destinations, "-j", string(kubeNodePortsChain)) + } else { + 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 _, ip := range nodeIPs { + 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 + proxier.natRules.Write( + "-A", string(kubeServicesChain), + "-m", "comment", "--comment", `"kubernetes service nodeports; NOTE: this must be the last rule in this chain"`, + "-d", ip.String(), + "-j", string(kubeNodePortsChain)) + } } // Drop the packets in INVALID state, which would potentially cause @@ -1537,7 +1510,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/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 f312e68ebc1..2c8e0a34af2 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) } @@ -1004,35 +1002,23 @@ 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") + if proxier.nodePortAddresses.MatchAll() { + for _, ipStr := range nodeAddressSet.UnsortedList() { + nodeIPs = append(nodeIPs, netutils.ParseIPSloppy(ipStr)) + } } else { - nodeAddresses = nodeAddrSet.UnsortedList() - for _, address := range nodeAddresses { - 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)) + allNodeIPs, err := proxier.nodePortAddresses.GetNodeIPs(proxier.networkInterfacer) + if err != nil { + klog.ErrorS(err, "Failed to get node IP address matching nodeport cidr") + } else { + for _, ip := range allNodeIPs { + if !ip.IsLoopback() { + nodeIPs = append(nodeIPs, ip) } - break - } - if getIPFamily(a) == proxier.ipFamily { - nodeIPs = append(nodeIPs, a) } } } @@ -1292,7 +1278,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 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..c5332a07958 100644 --- a/pkg/proxy/util/nodeport_addresses.go +++ b/pkg/proxy/util/nodeport_addresses.go @@ -20,7 +20,7 @@ import ( "fmt" "net" - "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/api/core/v1" netutils "k8s.io/utils/net" ) @@ -30,32 +30,52 @@ 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 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} + npa.matchAll = true + break + } + + npa.cidrs = append(npa.cidrs, cidr) } return npa @@ -65,32 +85,23 @@ 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. +// MatchAll returns true if npa matches all node IPs (of npa's given family) +func (npa *NodePortAddresses) MatchAll() bool { + return npa.matchAll +} + +// 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) { - 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) - } - } - +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) } - // Second round of iteration to parse IPs based on cidr. + // Use a map to dedup matches + addresses := make(map[string]net.IP) 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,21 +115,17 @@ 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()) - } + addresses[ip.String()] = ip } } } - if uniqueAddressList.Len() == 0 { - return nil, fmt.Errorf("no addresses found for cidrs %v", npa.cidrStrings) + ips := make([]net.IP, 0, len(addresses)) + for _, ip := range addresses { + ips = append(ips, ip) } - return uniqueAddressList, nil + 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 aba782fce13..c66db1b024f 100644 --- a/pkg/proxy/util/nodeport_addresses_test.go +++ b/pkg/proxy/util/nodeport_addresses_test.go @@ -17,9 +17,11 @@ limitations under the License. package util import ( + "fmt" "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" @@ -30,12 +32,34 @@ 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] + } + testCases := []struct { name string cidrs []string itfAddrsPairs []InterfaceAddrsPair - expected sets.Set[string] + expected map[v1.IPFamily]expectation }{ { name: "IPv4 single", @@ -50,7 +74,15 @@ 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: { + matchAll: true, + ips: nil, + }, + }, }, { name: "IPv4 zero CIDR", @@ -65,7 +97,16 @@ 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: { + matchAll: true, + ips: sets.New[string]("10.20.30.51", "127.0.0.1"), + }, + v1.IPv6Protocol: { + matchAll: true, + ips: nil, + }, + }, }, { name: "IPv6 multiple", @@ -80,7 +121,15 @@ 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: { + matchAll: true, + ips: nil, + }, + v1.IPv6Protocol: { + ips: sets.New[string]("2001:db8::1", "::1"), + }, + }, }, { name: "IPv6 zero CIDR", @@ -95,7 +144,16 @@ 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: { + matchAll: true, + ips: nil, + }, + v1.IPv6Protocol: { + matchAll: true, + ips: sets.New[string]("2001:db8::1", "::1"), + }, + }, }, { name: "IPv4 localhost exact", @@ -110,7 +168,15 @@ 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: { + matchAll: true, + ips: nil, + }, + }, }, { name: "IPv4 localhost subnet", @@ -121,7 +187,15 @@ 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: { + matchAll: true, + ips: nil, + }, + }, }, { name: "IPv4 multiple", @@ -136,7 +210,15 @@ 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: { + matchAll: true, + ips: nil, + }, + }, }, { name: "IPv4 multiple, no match", @@ -151,7 +233,15 @@ 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: { + matchAll: true, + ips: nil, + }, + }, }, { name: "empty list, IPv4 addrs", @@ -166,7 +256,16 @@ 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: { + matchAll: true, + ips: sets.New[string]("192.168.1.2", "127.0.0.1"), + }, + v1.IPv6Protocol: { + matchAll: true, + ips: nil, + }, + }, }, { name: "empty list, IPv6 addrs", @@ -181,7 +280,16 @@ 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: { + matchAll: true, + ips: nil, + }, + v1.IPv6Protocol: { + matchAll: true, + ips: sets.New[string]("2001:db8::1", "::1"), + }, + }, }, { name: "IPv4 redundant CIDRs", @@ -192,7 +300,16 @@ 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: { + matchAll: true, + ips: sets.New[string]("1.2.3.4"), + }, + v1.IPv6Protocol: { + matchAll: true, + ips: nil, + }, + }, }, { name: "Dual-stack, redundant IPv4", @@ -213,7 +330,15 @@ func TestGetNodeAddresses(t *testing.T) { }, }, }, - expected: sets.New[string]("0.0.0.0/0", "2001:db8::1"), + expected: map[v1.IPFamily]expectation{ + v1.IPv4Protocol: { + matchAll: true, + ips: sets.New[string]("1.2.3.4", "127.0.0.1"), + }, + v1.IPv6Protocol: { + ips: sets.New[string]("2001:db8::1"), + }, + }, }, { name: "Dual-stack, redundant IPv6", @@ -234,7 +359,15 @@ 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: { + matchAll: true, + ips: sets.New[string]("2001:db8::1", "::1"), + }, + }, }, } @@ -245,16 +378,26 @@ 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) + if npa.MatchAll() != tc.expected[family].matchAll { + t.Errorf("unexpected MatchAll(%s), expected: %v", family, tc.expected[family].matchAll) + } + + ips, err := npa.GetNodeIPs(nw) + expectedIPs := tc.expected[family].ips + + // The fake InterfaceAddrs() never returns an error, so + // the only error GetNodeIPs will return is "no + // addresses found". + if err != nil { + t.Errorf("unexpected error: %v", err) + } + err = checkNodeIPs(expectedIPs, ips) + if err != nil { + t.Errorf("unexpected mismatch for %s: %v", family, err) + } } }) } @@ -308,9 +451,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