From fe49e3933d1543c6cc5844d9b6fcdd9c22f67d97 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 15 Jan 2023 10:02:04 -0500 Subject: [PATCH 1/3] Move GetNodeAddresses() and ContainsIPv4Loopback() into a new file Both sound slightly generic, but implement semantics specific to the handling of NodePort addresses. (No changes other than moving code.) --- pkg/proxy/util/nodeport_addresses.go | 120 +++++++++ pkg/proxy/util/nodeport_addresses_test.go | 311 ++++++++++++++++++++++ pkg/proxy/util/utils.go | 95 ------- pkg/proxy/util/utils_test.go | 285 -------------------- 4 files changed, 431 insertions(+), 380 deletions(-) create mode 100644 pkg/proxy/util/nodeport_addresses.go create mode 100644 pkg/proxy/util/nodeport_addresses_test.go diff --git a/pkg/proxy/util/nodeport_addresses.go b/pkg/proxy/util/nodeport_addresses.go new file mode 100644 index 00000000000..694c122c5b8 --- /dev/null +++ b/pkg/proxy/util/nodeport_addresses.go @@ -0,0 +1,120 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package util + +import ( + "fmt" + "net" + + "k8s.io/apimachinery/pkg/util/sets" + netutils "k8s.io/utils/net" +) + +// GetNodeAddresses return all matched node IP addresses based on given cidr slice. +// Some callers, e.g. IPVS proxier, need concrete IPs, not ranges, which is why this exists. +// NetworkInterfacer is injected for test purpose. +// We expect the cidrs passed in is already validated. +// Given an empty input `[]`, it will return `0.0.0.0/0` and `::/0` directly. +// If multiple cidrs is given, it will return the minimal IP sets, e.g. given input `[1.2.0.0/16, 0.0.0.0/0]`, it will +// only return `0.0.0.0/0`. +// NOTE: GetNodeAddresses only accepts CIDRs, if you want concrete IPs, e.g. 1.2.3.4, then the input should be 1.2.3.4/32. +func GetNodeAddresses(cidrs []string, nw NetworkInterfacer) (sets.String, error) { + uniqueAddressList := sets.NewString() + if len(cidrs) == 0 { + uniqueAddressList.Insert(IPv4ZeroCIDR) + uniqueAddressList.Insert(IPv6ZeroCIDR) + return uniqueAddressList, nil + } + // 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 cidrs { + if IsZeroCIDR(cidr) { + uniqueAddressList.Insert(cidr) + } + } + + 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. + for _, cidr := range cidrs { + if IsZeroCIDR(cidr) { + continue + } + + _, ipNet, _ := netutils.ParseCIDRSloppy(cidr) + 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. + switch v := addr.(type) { + case *net.IPAddr: + ip = v.IP + case *net.IPNet: + ip = v.IP + default: + continue + } + + if ipNet.Contains(ip) { + if netutils.IsIPv6(ip) && !uniqueAddressList.Has(IPv6ZeroCIDR) { + uniqueAddressList.Insert(ip.String()) + } + if !netutils.IsIPv6(ip) && !uniqueAddressList.Has(IPv4ZeroCIDR) { + uniqueAddressList.Insert(ip.String()) + } + } + } + } + + if uniqueAddressList.Len() == 0 { + return nil, fmt.Errorf("no addresses found for cidrs %v", cidrs) + } + + return uniqueAddressList, nil +} + +// ContainsIPv4Loopback returns true if the input is empty or one of the CIDR contains an IPv4 loopback address. +func ContainsIPv4Loopback(cidrStrings []string) bool { + if len(cidrStrings) == 0 { + return true + } + // RFC 5735 127.0.0.0/8 - This block is assigned for use as the Internet host loopback address + ipv4LoopbackStart := netutils.ParseIPSloppy("127.0.0.0") + for _, cidr := range cidrStrings { + if IsZeroCIDR(cidr) { + return true + } + + ip, ipnet, err := netutils.ParseCIDRSloppy(cidr) + if err != nil { + continue + } + + if netutils.IsIPv6CIDR(ipnet) { + continue + } + + if ip.IsLoopback() { + return true + } + if ipnet.Contains(ipv4LoopbackStart) { + return true + } + } + return false +} diff --git a/pkg/proxy/util/nodeport_addresses_test.go b/pkg/proxy/util/nodeport_addresses_test.go new file mode 100644 index 00000000000..8cfb9500188 --- /dev/null +++ b/pkg/proxy/util/nodeport_addresses_test.go @@ -0,0 +1,311 @@ +/* +Copyright 2022 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package util + +import ( + "fmt" + "net" + "reflect" + "testing" + + "k8s.io/apimachinery/pkg/util/sets" + fake "k8s.io/kubernetes/pkg/proxy/util/testing" + netutils "k8s.io/utils/net" +) + +type InterfaceAddrsPair struct { + itf net.Interface + addrs []net.Addr +} + +func TestGetNodeAddresses(t *testing.T) { + testCases := []struct { + cidrs []string + nw *fake.FakeNetwork + itfAddrsPairs []InterfaceAddrsPair + expected sets.String + expectedErr error + }{ + { // case 0 + cidrs: []string{"10.20.30.0/24"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("10.20.30.51"), Mask: net.CIDRMask(24, 32)}}, + }, + { + itf: net.Interface{Index: 2, MTU: 0, Name: "eth1", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("100.200.201.1"), Mask: net.CIDRMask(24, 32)}}, + }, + }, + expected: sets.NewString("10.20.30.51"), + }, + { // case 1 + cidrs: []string{"0.0.0.0/0"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("10.20.30.51"), Mask: net.CIDRMask(24, 32)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, + }, + }, + expected: sets.NewString("0.0.0.0/0"), + }, + { // case 2 + cidrs: []string{"2001:db8::/32", "::1/128"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, + }, + }, + expected: sets.NewString("2001:db8::1", "::1"), + }, + { // case 3 + cidrs: []string{"::/0"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, + }, + }, + expected: sets.NewString("::/0"), + }, + { // case 4 + cidrs: []string{"127.0.0.1/32"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("10.20.30.51"), Mask: net.CIDRMask(24, 32)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, + }, + }, + expected: sets.NewString("127.0.0.1"), + }, + { // case 5 + cidrs: []string{"127.0.0.0/8"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.1.1"), Mask: net.CIDRMask(8, 32)}}, + }, + }, + expected: sets.NewString("127.0.1.1"), + }, + { // case 6 + cidrs: []string{"10.20.30.0/24", "100.200.201.0/24"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("10.20.30.51"), Mask: net.CIDRMask(24, 32)}}, + }, + { + itf: net.Interface{Index: 2, MTU: 0, Name: "eth1", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("100.200.201.1"), Mask: net.CIDRMask(24, 32)}}, + }, + }, + expected: sets.NewString("10.20.30.51", "100.200.201.1"), + }, + { // case 7 + cidrs: []string{"10.20.30.0/24", "100.200.201.0/24"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("192.168.1.2"), Mask: net.CIDRMask(24, 32)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, + }, + }, + expected: nil, + expectedErr: fmt.Errorf("no addresses found for cidrs %v", []string{"10.20.30.0/24", "100.200.201.0/24"}), + }, + { // case 8 + cidrs: []string{}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("192.168.1.2"), Mask: net.CIDRMask(24, 32)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, + }, + }, + expected: sets.NewString("0.0.0.0/0", "::/0"), + }, + { // case 9 + cidrs: []string{}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, + }, + }, + expected: sets.NewString("0.0.0.0/0", "::/0"), + }, + { // case 9 + cidrs: []string{"1.2.3.0/24", "0.0.0.0/0"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, + }, + }, + expected: sets.NewString("0.0.0.0/0"), + }, + { // case 10 + cidrs: []string{"0.0.0.0/0", "1.2.3.0/24", "::1/128"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, + }, + }, + expected: sets.NewString("0.0.0.0/0", "::1"), + }, + { // case 11 + cidrs: []string{"::/0", "1.2.3.0/24", "::1/128"}, + nw: fake.NewFakeNetwork(), + itfAddrsPairs: []InterfaceAddrsPair{ + { + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, + }, + { + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, + }, + }, + expected: sets.NewString("::/0", "1.2.3.4"), + }, + } + + for i := range testCases { + for _, pair := range testCases[i].itfAddrsPairs { + testCases[i].nw.AddInterfaceAddr(&pair.itf, pair.addrs) + } + addrList, err := GetNodeAddresses(testCases[i].cidrs, testCases[i].nw) + if !reflect.DeepEqual(err, testCases[i].expectedErr) { + t.Errorf("case [%d], unexpected error: %v", i, err) + } + + if !addrList.Equal(testCases[i].expected) { + t.Errorf("case [%d], unexpected mismatch, expected: %v, got: %v", i, testCases[i].expected, addrList) + } + } +} + +func TestContainsIPv4Loopback(t *testing.T) { + tests := []struct { + name string + cidrStrings []string + want bool + }{ + { + name: "empty", + want: true, + }, + { + name: "all zeros ipv4", + cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "0.0.0.0/0"}, + want: true, + }, + { + name: "all zeros ipv4 and invalid cidr", + cidrStrings: []string{"invalid.cidr", "192.168.0.0/16", "fd00:1:d::/64", "0.0.0.0/0"}, + want: true, + }, + { + name: "all zeros ipv6", // interpret all zeros equal for IPv4 and IPv6 as Golang stdlib + cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "::/0"}, + want: true, + }, + { + name: "ipv4 loopback", + cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "127.0.0.0/8"}, + want: true, + }, + { + name: "ipv6 loopback", + cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "::1/128"}, + want: false, + }, + { + name: "ipv4 loopback smaller range", + cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "127.0.2.0/28"}, + want: true, + }, + { + name: "ipv4 loopback within larger range", + cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "64.0.0.0/2"}, + want: true, + }, + { + name: "non loop loopback", + cidrStrings: []string{"128.0.2.0/28", "224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64"}, + want: false, + }, + { + name: "invalid cidr", + cidrStrings: []string{"invalid.ip/invalid.mask"}, + want: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ContainsIPv4Loopback(tt.cidrStrings); got != tt.want { + t.Errorf("ContainLoopback() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/proxy/util/utils.go b/pkg/proxy/util/utils.go index 92ef4658082..b67a1c1de49 100644 --- a/pkg/proxy/util/utils.go +++ b/pkg/proxy/util/utils.go @@ -78,37 +78,6 @@ func BuildPortsToEndpointsMap(endpoints *v1.Endpoints) map[string][]string { return portsToEndpoints } -// ContainsIPv4Loopback returns true if the input is empty or one of the CIDR contains an IPv4 loopback address. -func ContainsIPv4Loopback(cidrStrings []string) bool { - if len(cidrStrings) == 0 { - return true - } - // RFC 5735 127.0.0.0/8 - This block is assigned for use as the Internet host loopback address - ipv4LoopbackStart := netutils.ParseIPSloppy("127.0.0.0") - for _, cidr := range cidrStrings { - if IsZeroCIDR(cidr) { - return true - } - - ip, ipnet, err := netutils.ParseCIDRSloppy(cidr) - if err != nil { - continue - } - - if netutils.IsIPv6CIDR(ipnet) { - continue - } - - if ip.IsLoopback() { - return true - } - if ipnet.Contains(ipv4LoopbackStart) { - return true - } - } - return false -} - // IsZeroCIDR checks whether the input CIDR string is either // the IPv4 or IPv6 zero CIDR func IsZeroCIDR(cidr string) bool { @@ -228,70 +197,6 @@ func ShouldSkipService(service *v1.Service) bool { return false } -// GetNodeAddresses return all matched node IP addresses based on given cidr slice. -// Some callers, e.g. IPVS proxier, need concrete IPs, not ranges, which is why this exists. -// NetworkInterfacer is injected for test purpose. -// We expect the cidrs passed in is already validated. -// Given an empty input `[]`, it will return `0.0.0.0/0` and `::/0` directly. -// If multiple cidrs is given, it will return the minimal IP sets, e.g. given input `[1.2.0.0/16, 0.0.0.0/0]`, it will -// only return `0.0.0.0/0`. -// NOTE: GetNodeAddresses only accepts CIDRs, if you want concrete IPs, e.g. 1.2.3.4, then the input should be 1.2.3.4/32. -func GetNodeAddresses(cidrs []string, nw NetworkInterfacer) (sets.String, error) { - uniqueAddressList := sets.NewString() - if len(cidrs) == 0 { - uniqueAddressList.Insert(IPv4ZeroCIDR) - uniqueAddressList.Insert(IPv6ZeroCIDR) - return uniqueAddressList, nil - } - // 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 cidrs { - if IsZeroCIDR(cidr) { - uniqueAddressList.Insert(cidr) - } - } - - 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. - for _, cidr := range cidrs { - if IsZeroCIDR(cidr) { - continue - } - - _, ipNet, _ := netutils.ParseCIDRSloppy(cidr) - 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. - switch v := addr.(type) { - case *net.IPAddr: - ip = v.IP - case *net.IPNet: - ip = v.IP - default: - continue - } - - if ipNet.Contains(ip) { - if netutils.IsIPv6(ip) && !uniqueAddressList.Has(IPv6ZeroCIDR) { - uniqueAddressList.Insert(ip.String()) - } - if !netutils.IsIPv6(ip) && !uniqueAddressList.Has(IPv4ZeroCIDR) { - uniqueAddressList.Insert(ip.String()) - } - } - } - } - - if uniqueAddressList.Len() == 0 { - return nil, fmt.Errorf("no addresses found for cidrs %v", cidrs) - } - - return uniqueAddressList, nil -} - // AddressSet validates the addresses in the slice using the "isValid" function. // Addresses that pass the validation are returned as a string Set. func AddressSet(isValid func(ip net.IP) bool, addrs []net.Addr) sets.String { diff --git a/pkg/proxy/util/utils_test.go b/pkg/proxy/util/utils_test.go index d3138f53203..750fdf8a2d2 100644 --- a/pkg/proxy/util/utils_test.go +++ b/pkg/proxy/util/utils_test.go @@ -18,7 +18,6 @@ package util import ( "context" - "fmt" "math/rand" "net" "reflect" @@ -28,7 +27,6 @@ import ( v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/sets" - fake "k8s.io/kubernetes/pkg/proxy/util/testing" netutils "k8s.io/utils/net" ) @@ -399,224 +397,6 @@ func (t *testResolver) LookupIPAddr(_ context.Context, address string) ([]net.IP return t.addrs, t.err } -type InterfaceAddrsPair struct { - itf net.Interface - addrs []net.Addr -} - -func TestGetNodeAddresses(t *testing.T) { - testCases := []struct { - cidrs []string - nw *fake.FakeNetwork - itfAddrsPairs []InterfaceAddrsPair - expected sets.String - expectedErr error - }{ - { // case 0 - cidrs: []string{"10.20.30.0/24"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("10.20.30.51"), Mask: net.CIDRMask(24, 32)}}, - }, - { - itf: net.Interface{Index: 2, MTU: 0, Name: "eth1", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("100.200.201.1"), Mask: net.CIDRMask(24, 32)}}, - }, - }, - expected: sets.NewString("10.20.30.51"), - }, - { // case 1 - cidrs: []string{"0.0.0.0/0"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("10.20.30.51"), Mask: net.CIDRMask(24, 32)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, - }, - }, - expected: sets.NewString("0.0.0.0/0"), - }, - { // case 2 - cidrs: []string{"2001:db8::/32", "::1/128"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, - }, - }, - expected: sets.NewString("2001:db8::1", "::1"), - }, - { // case 3 - cidrs: []string{"::/0"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, - }, - }, - expected: sets.NewString("::/0"), - }, - { // case 4 - cidrs: []string{"127.0.0.1/32"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("10.20.30.51"), Mask: net.CIDRMask(24, 32)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, - }, - }, - expected: sets.NewString("127.0.0.1"), - }, - { // case 5 - cidrs: []string{"127.0.0.0/8"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.1.1"), Mask: net.CIDRMask(8, 32)}}, - }, - }, - expected: sets.NewString("127.0.1.1"), - }, - { // case 6 - cidrs: []string{"10.20.30.0/24", "100.200.201.0/24"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("10.20.30.51"), Mask: net.CIDRMask(24, 32)}}, - }, - { - itf: net.Interface{Index: 2, MTU: 0, Name: "eth1", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("100.200.201.1"), Mask: net.CIDRMask(24, 32)}}, - }, - }, - expected: sets.NewString("10.20.30.51", "100.200.201.1"), - }, - { // case 7 - cidrs: []string{"10.20.30.0/24", "100.200.201.0/24"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("192.168.1.2"), Mask: net.CIDRMask(24, 32)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, - }, - }, - expected: nil, - expectedErr: fmt.Errorf("no addresses found for cidrs %v", []string{"10.20.30.0/24", "100.200.201.0/24"}), - }, - { // case 8 - cidrs: []string{}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("192.168.1.2"), Mask: net.CIDRMask(24, 32)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, - }, - }, - expected: sets.NewString("0.0.0.0/0", "::/0"), - }, - { // case 9 - cidrs: []string{}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, - }, - }, - expected: sets.NewString("0.0.0.0/0", "::/0"), - }, - { // case 9 - cidrs: []string{"1.2.3.0/24", "0.0.0.0/0"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, - }, - }, - expected: sets.NewString("0.0.0.0/0"), - }, - { // case 10 - cidrs: []string{"0.0.0.0/0", "1.2.3.0/24", "::1/128"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, - }, - }, - expected: sets.NewString("0.0.0.0/0", "::1"), - }, - { // case 11 - cidrs: []string{"::/0", "1.2.3.0/24", "::1/128"}, - nw: fake.NewFakeNetwork(), - itfAddrsPairs: []InterfaceAddrsPair{ - { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, - }, - { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, - }, - }, - expected: sets.NewString("::/0", "1.2.3.4"), - }, - } - - for i := range testCases { - for _, pair := range testCases[i].itfAddrsPairs { - testCases[i].nw.AddInterfaceAddr(&pair.itf, pair.addrs) - } - addrList, err := GetNodeAddresses(testCases[i].cidrs, testCases[i].nw) - if !reflect.DeepEqual(err, testCases[i].expectedErr) { - t.Errorf("case [%d], unexpected error: %v", i, err) - } - - if !addrList.Equal(testCases[i].expected) { - t.Errorf("case [%d], unexpected mismatch, expected: %v, got: %v", i, testCases[i].expected, addrList) - } - } -} - func TestAppendPortIfNeeded(t *testing.T) { testCases := []struct { name string @@ -1405,71 +1185,6 @@ func TestAddressSet(t *testing.T) { } } -func TestContainsIPv4Loopback(t *testing.T) { - tests := []struct { - name string - cidrStrings []string - want bool - }{ - { - name: "empty", - want: true, - }, - { - name: "all zeros ipv4", - cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "0.0.0.0/0"}, - want: true, - }, - { - name: "all zeros ipv4 and invalid cidr", - cidrStrings: []string{"invalid.cidr", "192.168.0.0/16", "fd00:1:d::/64", "0.0.0.0/0"}, - want: true, - }, - { - name: "all zeros ipv6", // interpret all zeros equal for IPv4 and IPv6 as Golang stdlib - cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "::/0"}, - want: true, - }, - { - name: "ipv4 loopback", - cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "127.0.0.0/8"}, - want: true, - }, - { - name: "ipv6 loopback", - cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "::1/128"}, - want: false, - }, - { - name: "ipv4 loopback smaller range", - cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "127.0.2.0/28"}, - want: true, - }, - { - name: "ipv4 loopback within larger range", - cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "64.0.0.0/2"}, - want: true, - }, - { - name: "non loop loopback", - cidrStrings: []string{"128.0.2.0/28", "224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64"}, - want: false, - }, - { - name: "invalid cidr", - cidrStrings: []string{"invalid.ip/invalid.mask"}, - want: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := ContainsIPv4Loopback(tt.cidrStrings); got != tt.want { - t.Errorf("ContainLoopback() = %v, want %v", got, tt.want) - } - }) - } -} - func TestIsZeroCIDR(t *testing.T) { testCases := []struct { name string From 53b24f4ddf6be2c584e1ba83c19a778336fa1449 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Thu, 12 Jan 2023 17:52:53 -0500 Subject: [PATCH 2/3] Improve GetNodeAddresses unit test Add names to the tests and use t.Run() (rather than having them just be numbered, with number 9 mistakenly being used twice thus throwing off all the later numbers...) Remove unnecessary FakeNetwork element from the testCases struct since it's always the same. Remove the expectedErr value since a non-nil error is expected if and only if the returned set is nil, and there's no reason to test the exact text of the error message. Fix weird IPv6 subnet sizes. Change the dual-stack tests to (a) actually have dual-stack interface addrs, and (b) use a routable IPv6 address, not just localhost (given that we never actually want to use IPv6 localhost for nodeports). --- pkg/proxy/util/nodeport_addresses_test.go | 130 ++++++++++++---------- 1 file changed, 72 insertions(+), 58 deletions(-) diff --git a/pkg/proxy/util/nodeport_addresses_test.go b/pkg/proxy/util/nodeport_addresses_test.go index 8cfb9500188..3d62c29e0f2 100644 --- a/pkg/proxy/util/nodeport_addresses_test.go +++ b/pkg/proxy/util/nodeport_addresses_test.go @@ -17,9 +17,7 @@ limitations under the License. package util import ( - "fmt" "net" - "reflect" "testing" "k8s.io/apimachinery/pkg/util/sets" @@ -34,15 +32,14 @@ type InterfaceAddrsPair struct { func TestGetNodeAddresses(t *testing.T) { testCases := []struct { + name string cidrs []string - nw *fake.FakeNetwork itfAddrsPairs []InterfaceAddrsPair expected sets.String - expectedErr error }{ - { // case 0 + { + name: "IPv4 single", cidrs: []string{"10.20.30.0/24"}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, @@ -55,9 +52,9 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("10.20.30.51"), }, - { // case 1 + { + name: "IPv4 zero CIDR", cidrs: []string{"0.0.0.0/0"}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, @@ -70,13 +67,13 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("0.0.0.0/0"), }, - { // case 2 - cidrs: []string{"2001:db8::/32", "::1/128"}, - nw: fake.NewFakeNetwork(), + { + name: "IPv6 multiple", + cidrs: []string{"2001:db8::/64", "::1/128"}, itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(64, 128)}}, }, { itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, @@ -85,13 +82,13 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("2001:db8::1", "::1"), }, - { // case 3 + { + name: "IPv6 zero CIDR", cidrs: []string{"::/0"}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(64, 128)}}, }, { itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, @@ -100,9 +97,9 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("::/0"), }, - { // case 4 + { + name: "IPv4 localhost exact", cidrs: []string{"127.0.0.1/32"}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, @@ -115,9 +112,9 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("127.0.0.1"), }, - { // case 5 + { + name: "IPv4 localhost subnet", cidrs: []string{"127.0.0.0/8"}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, @@ -126,9 +123,9 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("127.0.1.1"), }, - { // case 6 + { + name: "IPv4 multiple", cidrs: []string{"10.20.30.0/24", "100.200.201.0/24"}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, @@ -141,9 +138,9 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("10.20.30.51", "100.200.201.1"), }, - { // case 7 + { + name: "IPv4 multiple, no match", cidrs: []string{"10.20.30.0/24", "100.200.201.0/24"}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, @@ -154,12 +151,11 @@ func TestGetNodeAddresses(t *testing.T) { addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}}, }, }, - expected: nil, - expectedErr: fmt.Errorf("no addresses found for cidrs %v", []string{"10.20.30.0/24", "100.200.201.0/24"}), + expected: nil, }, - { // case 8 + { + name: "empty list, IPv4 addrs", cidrs: []string{}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, @@ -172,13 +168,13 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("0.0.0.0/0", "::/0"), }, - { // case 9 + { + name: "empty list, IPv6 addrs", cidrs: []string{}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(32, 128)}}, + addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(64, 128)}}, }, { itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, @@ -187,9 +183,9 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("0.0.0.0/0", "::/0"), }, - { // case 9 + { + name: "IPv4 redundant CIDRs", cidrs: []string{"1.2.3.0/24", "0.0.0.0/0"}, - nw: fake.NewFakeNetwork(), itfAddrsPairs: []InterfaceAddrsPair{ { itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, @@ -198,50 +194,68 @@ func TestGetNodeAddresses(t *testing.T) { }, expected: sets.NewString("0.0.0.0/0"), }, - { // case 10 - cidrs: []string{"0.0.0.0/0", "1.2.3.0/24", "::1/128"}, - nw: fake.NewFakeNetwork(), + { + name: "Dual-stack, redundant IPv4", + cidrs: []string{"0.0.0.0/0", "1.2.3.0/24", "2001:db8::1/128"}, itfAddrsPairs: []InterfaceAddrsPair{ { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{ + &net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}, + &net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(64, 128)}, + }, }, { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{ + &net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}, + &net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}, + }, }, }, - expected: sets.NewString("0.0.0.0/0", "::1"), + expected: sets.NewString("0.0.0.0/0", "2001:db8::1"), }, - { // case 11 - cidrs: []string{"::/0", "1.2.3.0/24", "::1/128"}, - nw: fake.NewFakeNetwork(), + { + name: "Dual-stack, redundant IPv6", + cidrs: []string{"::/0", "1.2.3.0/24", "2001:db8::1/128"}, itfAddrsPairs: []InterfaceAddrsPair{ { - itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}}, + itf: net.Interface{Index: 0, MTU: 0, Name: "eth0", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{ + &net.IPNet{IP: netutils.ParseIPSloppy("1.2.3.4"), Mask: net.CIDRMask(30, 32)}, + &net.IPNet{IP: netutils.ParseIPSloppy("2001:db8::1"), Mask: net.CIDRMask(64, 128)}, + }, }, { - itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, - addrs: []net.Addr{&net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}}, + itf: net.Interface{Index: 1, MTU: 0, Name: "lo", HardwareAddr: nil, Flags: 0}, + addrs: []net.Addr{ + &net.IPNet{IP: netutils.ParseIPSloppy("127.0.0.1"), Mask: net.CIDRMask(8, 32)}, + &net.IPNet{IP: netutils.ParseIPSloppy("::1"), Mask: net.CIDRMask(128, 128)}, + }, }, }, expected: sets.NewString("::/0", "1.2.3.4"), }, } - for i := range testCases { - for _, pair := range testCases[i].itfAddrsPairs { - testCases[i].nw.AddInterfaceAddr(&pair.itf, pair.addrs) - } - addrList, err := GetNodeAddresses(testCases[i].cidrs, testCases[i].nw) - if !reflect.DeepEqual(err, testCases[i].expectedErr) { - t.Errorf("case [%d], unexpected error: %v", i, err) - } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + nw := fake.NewFakeNetwork() + for _, pair := range tc.itfAddrsPairs { + nw.AddInterfaceAddr(&pair.itf, pair.addrs) + } - if !addrList.Equal(testCases[i].expected) { - t.Errorf("case [%d], unexpected mismatch, expected: %v, got: %v", i, testCases[i].expected, addrList) - } + addrList, err := GetNodeAddresses(tc.cidrs, 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) + } + + if !addrList.Equal(tc.expected) { + t.Errorf("unexpected mismatch, expected: %v, got: %v", tc.expected, addrList) + } + }) } } From 463153fb7c51f041953831582222dc26258195b9 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Mon, 16 Jan 2023 12:20:59 -0500 Subject: [PATCH 3/3] Fix ContainsIPv4Loopback() to match its caller's behavior ContainsIPv4Loopback() claimed that "::/0" contains IPv4 loopback IPs (on the theory that listening on "::/0" will listen on "0.0.0.0/0" as well and thus include IPv4 loopback). But its sole caller (the iptables proxier) doesn't use listen() to accept connections, so this theory was completely mistaken; if you passed, eg, `--nodeport-addresses 192.168.0.0/0,::/0`, then it would not create any rule that accepted nodeport connections on 127.0.0.1, but it would nonetheless end up setting route_localnet=1 because ContainsIPv4Loopback() claimed it needed to. Fix this. --- pkg/proxy/util/nodeport_addresses.go | 4 ---- pkg/proxy/util/nodeport_addresses_test.go | 6 +++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/pkg/proxy/util/nodeport_addresses.go b/pkg/proxy/util/nodeport_addresses.go index 694c122c5b8..27d186964d0 100644 --- a/pkg/proxy/util/nodeport_addresses.go +++ b/pkg/proxy/util/nodeport_addresses.go @@ -96,10 +96,6 @@ func ContainsIPv4Loopback(cidrStrings []string) bool { // RFC 5735 127.0.0.0/8 - This block is assigned for use as the Internet host loopback address ipv4LoopbackStart := netutils.ParseIPSloppy("127.0.0.0") for _, cidr := range cidrStrings { - if IsZeroCIDR(cidr) { - return true - } - ip, ipnet, err := netutils.ParseCIDRSloppy(cidr) if err != nil { continue diff --git a/pkg/proxy/util/nodeport_addresses_test.go b/pkg/proxy/util/nodeport_addresses_test.go index 3d62c29e0f2..b77f6075902 100644 --- a/pkg/proxy/util/nodeport_addresses_test.go +++ b/pkg/proxy/util/nodeport_addresses_test.go @@ -280,9 +280,9 @@ func TestContainsIPv4Loopback(t *testing.T) { want: true, }, { - name: "all zeros ipv6", // interpret all zeros equal for IPv4 and IPv6 as Golang stdlib + name: "all zeros ipv6", cidrStrings: []string{"224.0.0.0/24", "192.168.0.0/16", "fd00:1:d::/64", "::/0"}, - want: true, + want: false, }, { name: "ipv4 loopback", @@ -318,7 +318,7 @@ func TestContainsIPv4Loopback(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { if got := ContainsIPv4Loopback(tt.cidrStrings); got != tt.want { - t.Errorf("ContainLoopback() = %v, want %v", got, tt.want) + t.Errorf("ContainsIPv4Loopback() = %v, want %v", got, tt.want) } }) }