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) }