(Mostly) Revert "change --nodeport-addresses behavior to default to primary node ip only"

This reverts commit 8bccf4873b, 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.
This commit is contained in:
Dan Winship 2024-02-02 09:44:40 -05:00
parent fde1af55d2
commit 19b3a9e194
11 changed files with 33 additions and 92 deletions

View File

@ -150,7 +150,7 @@ func (fake fakeProxierHealthChecker) IsHealthy() bool {
func TestServer(t *testing.T) { func TestServer(t *testing.T) {
listener := newFakeListener() listener := newFakeListener()
httpFactory := newFakeHTTPServerFactory() httpFactory := newFakeHTTPServerFactory()
nodePortAddresses := proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{}, nil) nodePortAddresses := proxyutil.NewNodePortAddresses(v1.IPv4Protocol, []string{})
proxyChecker := &fakeProxierHealthChecker{true} proxyChecker := &fakeProxierHealthChecker{true}
hcsi := newServiceHealthServer("hostname", nil, listener, httpFactory, nodePortAddresses, proxyChecker) 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 // 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 // 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) hcsi := newServiceHealthServer("hostname", nil, listener, httpFactory, nodePortAddresses, proxyChecker)
hcs := hcsi.(*server) hcs := hcsi.(*server)

View File

@ -232,7 +232,7 @@ func NewProxier(ipFamily v1.IPFamily,
nodePortAddressStrings []string, nodePortAddressStrings []string,
initOnly bool, initOnly bool,
) (*Proxier, error) { ) (*Proxier, error) {
nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings, nil) nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings)
if !nodePortAddresses.ContainsIPv4Loopback() { if !nodePortAddresses.ContainsIPv4Loopback() {
localhostNodePorts = false localhostNodePorts = false

View File

@ -133,7 +133,7 @@ func NewFakeProxier(ipt utiliptables.Interface) *Proxier {
natRules: proxyutil.NewLineBuffer(), natRules: proxyutil.NewLineBuffer(),
nodeIP: netutils.ParseIPSloppy(testNodeIP), nodeIP: netutils.ParseIPSloppy(testNodeIP),
localhostNodePorts: true, localhostNodePorts: true,
nodePortAddresses: proxyutil.NewNodePortAddresses(ipfamily, nil, nil), nodePortAddresses: proxyutil.NewNodePortAddresses(ipfamily, nil),
networkInterfacer: networkInterfacer, networkInterfacer: networkInterfacer,
} }
p.setInitialized(true) p.setInitialized(true)
@ -2342,7 +2342,7 @@ func TestNodePorts(t *testing.T) {
fp := NewFakeProxier(ipt) fp := NewFakeProxier(ipt)
fp.localhostNodePorts = tc.localhostNodePorts fp.localhostNodePorts = tc.localhostNodePorts
if tc.nodePortAddresses != nil { if tc.nodePortAddresses != nil {
fp.nodePortAddresses = proxyutil.NewNodePortAddresses(tc.family, tc.nodePortAddresses, nil) fp.nodePortAddresses = proxyutil.NewNodePortAddresses(tc.family, tc.nodePortAddresses)
} }
makeServiceMap(fp, makeServiceMap(fp,
@ -2490,7 +2490,7 @@ func TestNodePorts(t *testing.T) {
func TestHealthCheckNodePort(t *testing.T) { func TestHealthCheckNodePort(t *testing.T) {
ipt := iptablestest.NewFake() ipt := iptablestest.NewFake()
fp := NewFakeProxier(ipt) 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" svcIP := "172.30.0.42"
svcPort := 80 svcPort := 80

View File

@ -359,7 +359,7 @@ func NewProxier(ipFamily v1.IPFamily,
scheduler = defaultScheduler scheduler = defaultScheduler
} }
nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings, nil) nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings)
serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer) serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer)

View File

@ -158,7 +158,7 @@ func NewFakeProxier(ipt utiliptables.Interface, ipvs utilipvs.Interface, ipset u
filterRules: proxyutil.NewLineBuffer(), filterRules: proxyutil.NewLineBuffer(),
netlinkHandle: netlinkHandle, netlinkHandle: netlinkHandle,
ipsetList: ipsetList, ipsetList: ipsetList,
nodePortAddresses: proxyutil.NewNodePortAddresses(ipFamily, nil, nil), nodePortAddresses: proxyutil.NewNodePortAddresses(ipFamily, nil),
networkInterfacer: proxyutiltest.NewFakeNetwork(), networkInterfacer: proxyutiltest.NewFakeNetwork(),
gracefuldeleteManager: NewGracefulTerminationManager(ipvs), gracefuldeleteManager: NewGracefulTerminationManager(ipvs),
ipFamily: ipFamily, ipFamily: ipFamily,
@ -945,7 +945,7 @@ func TestNodePortIPv4(t *testing.T) {
ipvs := ipvstest.NewFake() ipvs := ipvstest.NewFake()
ipset := ipsettest.NewFake(testIPSetVersion) ipset := ipsettest.NewFake(testIPSetVersion)
fp := NewFakeProxier(ipt, ipvs, ipset, test.nodeIPs, nil, v1.IPv4Protocol) 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...) makeServiceMap(fp, test.services...)
populateEndpointSlices(fp, test.endpoints...) populateEndpointSlices(fp, test.endpoints...)
@ -1287,7 +1287,7 @@ func TestNodePortIPv6(t *testing.T) {
ipvs := ipvstest.NewFake() ipvs := ipvstest.NewFake()
ipset := ipsettest.NewFake(testIPSetVersion) ipset := ipsettest.NewFake(testIPSetVersion)
fp := NewFakeProxier(ipt, ipvs, ipset, test.nodeIPs, nil, v1.IPv6Protocol) 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...) makeServiceMap(fp, test.services...)
populateEndpointSlices(fp, test.endpoints...) 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)}} 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(&itf, addrs)
fp.networkInterfacer.(*proxyutiltest.FakeNetwork).AddInterfaceAddr(&itf1, addrs1) 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() 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)}} 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(&itf, addrs)
fp.networkInterfacer.(*proxyutiltest.FakeNetwork).AddInterfaceAddr(&itf1, addrs1) 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() fp.syncProxyRules()

View File

@ -211,8 +211,6 @@ func NewProxier(ipFamily v1.IPFamily,
nodePortAddressStrings []string, nodePortAddressStrings []string,
initOnly bool, initOnly bool,
) (*Proxier, error) { ) (*Proxier, error) {
nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nodePortAddressStrings, nodeIP)
if initOnly { if initOnly {
klog.InfoS("System initialized and --init-only specified") klog.InfoS("System initialized and --init-only specified")
return nil, nil return nil, nil
@ -223,6 +221,8 @@ func NewProxier(ipFamily v1.IPFamily,
masqueradeMark := fmt.Sprintf("%#08x", masqueradeValue) masqueradeMark := fmt.Sprintf("%#08x", masqueradeValue)
klog.V(2).InfoS("Using nftables mark for masquerade", "ipFamily", ipFamily, "mark", masqueradeMark) 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) serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer)
var nftablesFamily knftables.Family var nftablesFamily knftables.Family

View File

@ -126,7 +126,7 @@ func NewFakeProxier(ipFamily v1.IPFamily) (*knftables.Fake, *Proxier) {
hostname: testHostname, hostname: testHostname,
serviceHealthServer: healthcheck.NewFakeServiceHealthServer(), serviceHealthServer: healthcheck.NewFakeServiceHealthServer(),
nodeIP: nodeIP, nodeIP: nodeIP,
nodePortAddresses: proxyutil.NewNodePortAddresses(ipFamily, nodePortAddresses, nodeIP), nodePortAddresses: proxyutil.NewNodePortAddresses(ipFamily, nodePortAddresses),
networkInterfacer: networkInterfacer, networkInterfacer: networkInterfacer,
staleChains: make(map[string]time.Time), staleChains: make(map[string]time.Time),
serviceCIDRs: serviceCIDRs, serviceCIDRs: serviceCIDRs,
@ -952,7 +952,7 @@ func TestNodePorts(t *testing.T) {
nodeIP = testNodeIPv6 nodeIP = testNodeIPv6
} }
if tc.nodePortAddresses != nil { 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, makeServiceMap(fp,

View File

@ -20,7 +20,7 @@ import (
"fmt" "fmt"
"net" "net"
v1 "k8s.io/api/core/v1" "k8s.io/api/core/v1"
netutils "k8s.io/utils/net" netutils "k8s.io/utils/net"
) )
@ -37,12 +37,11 @@ type NodePortAddresses struct {
var ipv4LoopbackStart = net.IPv4(127, 0, 0, 0) var ipv4LoopbackStart = net.IPv4(127, 0, 0, 0)
// NewNodePortAddresses takes an IP family and the `--nodeport-addresses` value (which is // 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 // assumed to contain only valid CIDRs, potentially of both IP families) and returns a
// (which will be used as node port address when `--nodeport-addresses` is empty). // NodePortAddresses object for the given family. If there are no CIDRs of the given
// It will return a NodePortAddresses object for the given family. If there are no CIDRs of // family then the CIDR "0.0.0.0/0" or "::/0" will be added (even if there are CIDRs of
// the given family then the CIDR "0.0.0.0/0" or "::/0" will be added (even if there are // the other family).
// CIDRs of the other family). func NewNodePortAddresses(family v1.IPFamily, cidrStrings []string) *NodePortAddresses {
func NewNodePortAddresses(family v1.IPFamily, cidrStrings []string, primaryIP net.IP) *NodePortAddresses {
npa := &NodePortAddresses{} npa := &NodePortAddresses{}
// Filter CIDRs to correct family // Filter CIDRs to correct family
@ -52,24 +51,17 @@ func NewNodePortAddresses(family v1.IPFamily, cidrStrings []string, primaryIP ne
} }
} }
if len(npa.cidrStrings) == 0 { if len(npa.cidrStrings) == 0 {
if primaryIP == nil {
if family == v1.IPv4Protocol { if family == v1.IPv4Protocol {
npa.cidrStrings = []string{IPv4ZeroCIDR} npa.cidrStrings = []string{IPv4ZeroCIDR}
} else { } else {
npa.cidrStrings = []string{IPv6ZeroCIDR} npa.cidrStrings = []string{IPv6ZeroCIDR}
} }
} else {
if family == v1.IPv4Protocol {
npa.cidrStrings = []string{fmt.Sprintf("%s/32", primaryIP.String())}
} else {
npa.cidrStrings = []string{fmt.Sprintf("%s/128", primaryIP.String())}
}
}
} }
// Now parse // Now parse
for _, str := range npa.cidrStrings { for _, str := range npa.cidrStrings {
_, cidr, _ := netutils.ParseCIDRSloppy(str) _, cidr, _ := netutils.ParseCIDRSloppy(str)
if netutils.IsIPv4CIDR(cidr) { if netutils.IsIPv4CIDR(cidr) {
if cidr.IP.IsLoopback() || cidr.Contains(ipv4LoopbackStart) { if cidr.IP.IsLoopback() || cidr.Contains(ipv4LoopbackStart) {
npa.containsIPv4Loopback = true npa.containsIPv4Loopback = true

View File

@ -21,7 +21,7 @@ import (
"net" "net"
"testing" "testing"
v1 "k8s.io/api/core/v1" "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/sets"
fake "k8s.io/kubernetes/pkg/proxy/util/testing" fake "k8s.io/kubernetes/pkg/proxy/util/testing"
netutils "k8s.io/utils/net" netutils "k8s.io/utils/net"
@ -60,8 +60,6 @@ func TestGetNodeIPs(t *testing.T) {
cidrs []string cidrs []string
itfAddrsPairs []InterfaceAddrsPair itfAddrsPairs []InterfaceAddrsPair
expected map[v1.IPFamily]expectation expected map[v1.IPFamily]expectation
// nodeIP will take effect when `--nodeport-addresses` is empty
nodeIP net.IP
}{ }{
{ {
name: "IPv4 single", 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 { for _, tc := range testCases {
@ -428,10 +379,7 @@ func TestGetNodeIPs(t *testing.T) {
} }
for _, family := range []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol} { for _, family := range []v1.IPFamily{v1.IPv4Protocol, v1.IPv6Protocol} {
if tc.nodeIP != nil && v1.IPFamily(fmt.Sprintf("IPv%s", netutils.IPFamilyOf(tc.nodeIP))) != family { npa := NewNodePortAddresses(family, tc.cidrs)
continue
}
npa := NewNodePortAddresses(family, tc.cidrs, tc.nodeIP)
if npa.MatchAll() != tc.expected[family].matchAll { if npa.MatchAll() != tc.expected[family].matchAll {
t.Errorf("unexpected MatchAll(%s), expected: %v", family, 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 { for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) { 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 { if got := npa.ContainsIPv4Loopback(); got != tt.want {
t.Errorf("IPv4 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 // 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 { if got := npa.ContainsIPv4Loopback(); got {
t.Errorf("IPv6 ContainsIPv4Loopback() = %v, want %v", got, false) t.Errorf("IPv6 ContainsIPv4Loopback() = %v, want %v", got, false)
} }

View File

@ -29,10 +29,11 @@ import (
utilfeature "k8s.io/apiserver/pkg/util/feature" utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/client-go/tools/events" "k8s.io/client-go/tools/events"
utilsysctl "k8s.io/component-helpers/node/util/sysctl" utilsysctl "k8s.io/component-helpers/node/util/sysctl"
"k8s.io/klog/v2"
helper "k8s.io/kubernetes/pkg/apis/core/v1/helper" helper "k8s.io/kubernetes/pkg/apis/core/v1/helper"
"k8s.io/kubernetes/pkg/features" "k8s.io/kubernetes/pkg/features"
netutils "k8s.io/utils/net" netutils "k8s.io/utils/net"
"k8s.io/klog/v2"
) )
const ( const (

View File

@ -666,7 +666,7 @@ func NewProxier(
} }
// windows listens to all node addresses // windows listens to all node addresses
nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nil, nil) nodePortAddresses := proxyutil.NewNodePortAddresses(ipFamily, nil)
serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer) serviceHealthServer := healthcheck.NewServiceHealthServer(hostname, recorder, nodePortAddresses, healthzServer)
var healthzPort int var healthzPort int