From 19b3a9e19436328eaaa247ead8afbdd501a08e85 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Fri, 2 Feb 2024 09:44:40 -0500 Subject: [PATCH] (Mostly) Revert "change --nodeport-addresses behavior to default to primary node ip only" This reverts commit 8bccf4873bd9fd612aa340c024c47dbf4cb03fa0, except for the nftables unit test changes, since we still want the "new" results (not to mention the bugfixes), just for a different reason now. --- pkg/proxy/healthcheck/healthcheck_test.go | 4 +- pkg/proxy/iptables/proxier.go | 2 +- pkg/proxy/iptables/proxier_test.go | 6 +-- pkg/proxy/ipvs/proxier.go | 2 +- pkg/proxy/ipvs/proxier_test.go | 10 ++-- pkg/proxy/nftables/proxier.go | 4 +- pkg/proxy/nftables/proxier_test.go | 4 +- pkg/proxy/util/nodeport_addresses.go | 28 ++++------- pkg/proxy/util/nodeport_addresses_test.go | 60 ++--------------------- pkg/proxy/util/utils.go | 3 +- pkg/proxy/winkernel/proxier.go | 2 +- 11 files changed, 33 insertions(+), 92 deletions(-) diff --git a/pkg/proxy/healthcheck/healthcheck_test.go b/pkg/proxy/healthcheck/healthcheck_test.go index fc82049dcfe..c1c451ef8c2 100644 --- a/pkg/proxy/healthcheck/healthcheck_test.go +++ b/pkg/proxy/healthcheck/healthcheck_test.go @@ -150,7 +150,7 @@ func (fake fakeProxierHealthChecker) IsHealthy() bool { func TestServer(t *testing.T) { listener := newFakeListener() httpFactory := newFakeHTTPServerFactory() - nodePortAddresses := proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{}, nil) + nodePortAddresses := proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{}) proxyChecker := &fakeProxierHealthChecker{true} hcsi := newServiceHealthServer("hostname", nil, listener, httpFactory, nodePortAddresses, proxyChecker) @@ -664,7 +664,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 := proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{"127.0.0.0/8"}, nil) + nodePortAddresses := proxyutil.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 d0c427d5e41..2771801b69c 100644 --- a/pkg/proxy/iptables/proxier.go +++ b/pkg/proxy/iptables/proxier.go @@ -232,7 +232,7 @@ func NewProxier(ipFamily v1.IPFamily, nodePortAddressStrings []string, initOnly bool, ) (*Proxier, error) { - nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings, nil) + nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings) if !nodePortAddresses.ContainsIPv4Loopback() { localhostNodePorts = false diff --git a/pkg/proxy/iptables/proxier_test.go b/pkg/proxy/iptables/proxier_test.go index 45f75be1ae4..cb434c1263c 100644 --- a/pkg/proxy/iptables/proxier_test.go +++ b/pkg/proxy/iptables/proxier_test.go @@ -133,7 +133,7 @@ func NewFakeProxier(ipt utiliptables.Interface) *Proxier { natRules: proxyutil.NewLineBuffer(), nodeIP: netutils.ParseIPSloppy(testNodeIP), localhostNodePorts: true, - nodePortAddresses: proxyutil.NewNodePortAddresses(ipfamily, nil, nil), + nodePortAddresses: proxyutil.NewNodePortAddresses(ipfamily, nil), networkInterfacer: networkInterfacer, } p.setInitialized(true) @@ -2342,7 +2342,7 @@ func TestNodePorts(t *testing.T) { fp := NewFakeProxier(ipt) fp.localhostNodePorts = tc.localhostNodePorts if tc.nodePortAddresses != nil { - fp.nodePortAddresses = proxyutil.NewNodePortAddresses(tc.family, tc.nodePortAddresses, nil) + fp.nodePortAddresses = proxyutil.NewNodePortAddresses(tc.family, tc.nodePortAddresses) } makeServiceMap(fp, @@ -2490,7 +2490,7 @@ func TestNodePorts(t *testing.T) { func TestHealthCheckNodePort(t *testing.T) { ipt := iptablestest.NewFake() fp := NewFakeProxier(ipt) - fp.nodePortAddresses = proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{"127.0.0.0/8"}, nil) + fp.nodePortAddresses = proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{"127.0.0.0/8"}) svcIP := "172.30.0.42" svcPort := 80 diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 5c492eb7b66..daf28fee6be 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -359,7 +359,7 @@ func NewProxier(ipFamily v1.IPFamily, scheduler = defaultScheduler } - nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings, nil) + nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings) serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer) diff --git a/pkg/proxy/ipvs/proxier_test.go b/pkg/proxy/ipvs/proxier_test.go index 55cc280a831..33af6d72b61 100644 --- a/pkg/proxy/ipvs/proxier_test.go +++ b/pkg/proxy/ipvs/proxier_test.go @@ -158,7 +158,7 @@ func NewFakeProxier(ipt utiliptables.Interface, ipvs utilipvs.Interface, ipset u filterRules: proxyutil.NewLineBuffer(), netlinkHandle: netlinkHandle, ipsetList: ipsetList, - nodePortAddresses: proxyutil.NewNodePortAddresses(ipFamily, nil, nil), + nodePortAddresses: proxyutil.NewNodePortAddresses(ipFamily, nil), networkInterfacer: proxyutiltest.NewFakeNetwork(), gracefuldeleteManager: NewGracefulTerminationManager(ipvs), ipFamily: ipFamily, @@ -945,7 +945,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 = proxyutil.NewNodePortAddresses(v1.IPv4Protocol, test.nodePortAddresses, nil) + fp.nodePortAddresses = proxyutil.NewNodePortAddresses(v1.IPv4Protocol, test.nodePortAddresses) makeServiceMap(fp, test.services...) populateEndpointSlices(fp, test.endpoints...) @@ -1287,7 +1287,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 = proxyutil.NewNodePortAddresses(v1.IPv6Protocol, test.nodePortAddresses, nil) + fp.nodePortAddresses = proxyutil.NewNodePortAddresses(v1.IPv6Protocol, test.nodePortAddresses) makeServiceMap(fp, test.services...) populateEndpointSlices(fp, test.endpoints...) @@ -2040,7 +2040,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 = proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{"100.101.102.0/24"}, nil) + fp.nodePortAddresses = proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{"100.101.102.0/24"}) fp.syncProxyRules() @@ -2128,7 +2128,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 = proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{"100.101.102.0/24"}, nil) + fp.nodePortAddresses = proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{"100.101.102.0/24"}) fp.syncProxyRules() diff --git a/pkg/proxy/nftables/proxier.go b/pkg/proxy/nftables/proxier.go index 3e54e7ee4c9..490ea19d3ba 100644 --- a/pkg/proxy/nftables/proxier.go +++ b/pkg/proxy/nftables/proxier.go @@ -211,8 +211,6 @@ func NewProxier(ipFamily v1.IPFamily, nodePortAddressStrings []string, initOnly bool, ) (*Proxier, error) { - nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings, nodeIP) - if initOnly { klog.InfoS("System initialized and --init-only specified") return nil, nil @@ -223,6 +221,8 @@ func NewProxier(ipFamily v1.IPFamily, masqueradeMark := fmt.Sprintf("%#08x", masqueradeValue) klog.V(2).InfoS("Using nftables mark for masquerade", "ipFamily", ipFamily, "mark", masqueradeMark) + nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings) + serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer) var nftablesFamily knftables.Family diff --git a/pkg/proxy/nftables/proxier_test.go b/pkg/proxy/nftables/proxier_test.go index 1b67de4f75c..fcf67d05afc 100644 --- a/pkg/proxy/nftables/proxier_test.go +++ b/pkg/proxy/nftables/proxier_test.go @@ -126,7 +126,7 @@ func NewFakeProxier(ipFamily v1.IPFamily) (*knftables.Fake, *Proxier) { hostname: testHostname, serviceHealthServer: healthcheck.NewFakeServiceHealthServer(), nodeIP: nodeIP, - nodePortAddresses: proxyutil.NewNodePortAddresses(ipFamily, nodePortAddresses, nodeIP), + nodePortAddresses: proxyutil.NewNodePortAddresses(ipFamily, nodePortAddresses), networkInterfacer: networkInterfacer, staleChains: make(map[string]time.Time), serviceCIDRs: serviceCIDRs, @@ -952,7 +952,7 @@ func TestNodePorts(t *testing.T) { nodeIP = testNodeIPv6 } if tc.nodePortAddresses != nil { - fp.nodePortAddresses = proxyutil.NewNodePortAddresses(tc.family, tc.nodePortAddresses, netutils.ParseIPSloppy(nodeIP)) + fp.nodePortAddresses = proxyutil.NewNodePortAddresses(tc.family, tc.nodePortAddresses) } makeServiceMap(fp, diff --git a/pkg/proxy/util/nodeport_addresses.go b/pkg/proxy/util/nodeport_addresses.go index ab11d08917b..c5332a07958 100644 --- a/pkg/proxy/util/nodeport_addresses.go +++ b/pkg/proxy/util/nodeport_addresses.go @@ -20,7 +20,7 @@ import ( "fmt" "net" - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" netutils "k8s.io/utils/net" ) @@ -37,12 +37,11 @@ type NodePortAddresses struct { var ipv4LoopbackStart = net.IPv4(127, 0, 0, 0) // NewNodePortAddresses takes an IP family and the `--nodeport-addresses` value (which is -// assumed to contain only valid CIDRs, potentially of both IP families) and the primary IP -// (which will be used as node port address when `--nodeport-addresses` is empty). -// It will return 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, primaryIP net.IP) *NodePortAddresses { +// 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 @@ -52,24 +51,17 @@ func NewNodePortAddresses(family v1.IPFamily, cidrStrings []string, primaryIP ne } } if len(npa.cidrStrings) == 0 { - if primaryIP == nil { - if family == v1.IPv4Protocol { - npa.cidrStrings = []string{IPv4ZeroCIDR} - } else { - npa.cidrStrings = []string{IPv6ZeroCIDR} - } + if family == v1.IPv4Protocol { + npa.cidrStrings = []string{IPv4ZeroCIDR} } else { - if family == v1.IPv4Protocol { - npa.cidrStrings = []string{fmt.Sprintf("%s/32", primaryIP.String())} - } else { - npa.cidrStrings = []string{fmt.Sprintf("%s/128", primaryIP.String())} - } + npa.cidrStrings = []string{IPv6ZeroCIDR} } } // Now parse for _, str := range npa.cidrStrings { _, cidr, _ := netutils.ParseCIDRSloppy(str) + if netutils.IsIPv4CIDR(cidr) { if cidr.IP.IsLoopback() || cidr.Contains(ipv4LoopbackStart) { npa.containsIPv4Loopback = true diff --git a/pkg/proxy/util/nodeport_addresses_test.go b/pkg/proxy/util/nodeport_addresses_test.go index fad92ef6a54..c66db1b024f 100644 --- a/pkg/proxy/util/nodeport_addresses_test.go +++ b/pkg/proxy/util/nodeport_addresses_test.go @@ -21,7 +21,7 @@ import ( "net" "testing" - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/util/sets" fake "k8s.io/kubernetes/pkg/proxy/util/testing" netutils "k8s.io/utils/net" @@ -60,8 +60,6 @@ func TestGetNodeIPs(t *testing.T) { cidrs []string itfAddrsPairs []InterfaceAddrsPair expected map[v1.IPFamily]expectation - // nodeIP will take effect when `--nodeport-addresses` is empty - nodeIP net.IP }{ { name: "IPv4 single", @@ -371,53 +369,6 @@ func TestGetNodeIPs(t *testing.T) { }, }, }, - { - name: "ipv4 with nodeIP", - 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("127.0.0.1"), Mask: net.CIDRMask(8, 32)}, - }, - }, - }, - expected: map[v1.IPFamily]expectation{ - v1.IPv4Protocol: { - ips: sets.New[string]("1.2.3.4"), - }, - }, - nodeIP: netutils.ParseIPSloppy("1.2.3.4"), - }, - { - name: "ipv6 with nodeIP", - 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(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)}, - }, - }, - }, - expected: map[v1.IPFamily]expectation{ - v1.IPv6Protocol: { - matchAll: true, - ips: sets.New[string]("2001:db8::1", "::1"), - }, - }, - nodeIP: netutils.ParseIPSloppy("1.2.3.4"), - }, } for _, tc := range testCases { @@ -428,10 +379,7 @@ func TestGetNodeIPs(t *testing.T) { } for _, family := range []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol} { - if tc.nodeIP != nil && v1.IPFamily(fmt.Sprintf("IPv%s", netutils.IPFamilyOf(tc.nodeIP))) != family { - continue - } - npa := NewNodePortAddresses(family, tc.cidrs, tc.nodeIP) + npa := NewNodePortAddresses(family, tc.cidrs) if npa.MatchAll() != tc.expected[family].matchAll { t.Errorf("unexpected MatchAll(%s), expected: %v", family, tc.expected[family].matchAll) @@ -503,12 +451,12 @@ func TestContainsIPv4Loopback(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - npa := NewNodePortAddresses(v1.IPv4Protocol, tt.cidrStrings, nil) + npa := NewNodePortAddresses(v1.IPv4Protocol, tt.cidrStrings) if got := npa.ContainsIPv4Loopback(); 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, nil) + 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/util/utils.go b/pkg/proxy/util/utils.go index da232fef982..260894461d5 100644 --- a/pkg/proxy/util/utils.go +++ b/pkg/proxy/util/utils.go @@ -29,10 +29,11 @@ import ( utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/tools/events" utilsysctl "k8s.io/component-helpers/node/util/sysctl" - "k8s.io/klog/v2" helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" "k8s.io/kubernetes/pkg/features" netutils "k8s.io/utils/net" + + "k8s.io/klog/v2" ) const ( diff --git a/pkg/proxy/winkernel/proxier.go b/pkg/proxy/winkernel/proxier.go index a09a693f5ff..a1f9bade5f2 100644 --- a/pkg/proxy/winkernel/proxier.go +++ b/pkg/proxy/winkernel/proxier.go @@ -666,7 +666,7 @@ func NewProxier( } // windows listens to all node addresses - nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nil, nil) + nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nil) serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer) var healthzPort int