From 5310305098a1026f74297aa917e7a3de47df2d5b Mon Sep 17 00:00:00 2001 From: Lars Ekman Date: Thu, 27 Apr 2023 20:45:27 +0200 Subject: [PATCH 1/2] proxy/ipvs: add a GetAllLocalAddressesExcept() function --- pkg/proxy/ipvs/netlink.go | 5 +++++ pkg/proxy/ipvs/netlink_linux.go | 28 +++++++++++++++++++++++++++ pkg/proxy/ipvs/netlink_unsupported.go | 5 +++++ pkg/proxy/ipvs/proxier.go | 3 +-- pkg/proxy/ipvs/testing/fake.go | 15 ++++++++++++++ pkg/proxy/ipvs/testing/fake_test.go | 8 +++++++- 6 files changed, 61 insertions(+), 3 deletions(-) diff --git a/pkg/proxy/ipvs/netlink.go b/pkg/proxy/ipvs/netlink.go index ab0b9eaaa14..cc173eae5c1 100644 --- a/pkg/proxy/ipvs/netlink.go +++ b/pkg/proxy/ipvs/netlink.go @@ -40,4 +40,9 @@ type NetLinkHandle interface { // Only the addresses of the current family are returned. // IPv6 link-local and loopback addresses are excluded. GetLocalAddresses(dev string) (sets.Set[string], error) + // GetAllLocalAddressesExcept return all local addresses on the node, except from the passed dev. + // This is not the same as to take the diff between GetAllLocalAddresses and GetLocalAddresses + // since an address can be assigned to many interfaces. This problem raised + // https://github.com/kubernetes/kubernetes/issues/114815 + GetAllLocalAddressesExcept(dev string) (sets.Set[string], error) } diff --git a/pkg/proxy/ipvs/netlink_linux.go b/pkg/proxy/ipvs/netlink_linux.go index f4d2368885d..1c0f8c2b343 100644 --- a/pkg/proxy/ipvs/netlink_linux.go +++ b/pkg/proxy/ipvs/netlink_linux.go @@ -24,6 +24,7 @@ import ( "net" "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/klog/v2" utilproxy "k8s.io/kubernetes/pkg/proxy/util" netutils "k8s.io/utils/net" @@ -164,3 +165,30 @@ func (h *netlinkHandle) isValidForSet(ip net.IP) bool { } return true } + +// GetAllLocalAddressesExcept return all local addresses on the node, +// except from the passed dev. This is not the same as to take the +// diff between GetAllLocalAddresses and GetLocalAddresses since an +// address can be assigned to many interfaces. This problem raised +// https://github.com/kubernetes/kubernetes/issues/114815 +func (h *netlinkHandle) GetAllLocalAddressesExcept(dev string) (sets.Set[string], error) { + ifaces, err := net.Interfaces() + if err != nil { + return nil, err + } + var addr []net.Addr + for _, iface := range ifaces { + if iface.Name == dev { + continue + } + ifadr, err := iface.Addrs() + if err != nil { + // This may happen if the interface was deleted. Ignore + // but log the error. + klog.ErrorS(err, "Reading addresses", "interface", iface.Name) + continue + } + addr = append(addr, ifadr...) + } + return utilproxy.AddressSet(h.isValidForSet, addr), nil +} diff --git a/pkg/proxy/ipvs/netlink_unsupported.go b/pkg/proxy/ipvs/netlink_unsupported.go index 31f3fb7406b..1cb38d3fb8f 100644 --- a/pkg/proxy/ipvs/netlink_unsupported.go +++ b/pkg/proxy/ipvs/netlink_unsupported.go @@ -71,6 +71,11 @@ func (h *netlinkHandle) GetLocalAddresses(dev string) (sets.Set[string], error) return nil, fmt.Errorf("netlink is not supported in this platform") } +// GetAllLocalAddressesExcept is part of interface. +func (h *netlinkHandle) GetAllLocalAddressesExcept(dev string) (sets.Set[string], error) { + return nil, fmt.Errorf("netlink is not supported in this platform") +} + // Must match the one in proxier_test.go func (h *netlinkHandle) isValidForSet(ip net.IP) bool { return false diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index a3f70752e67..93a3ed4620e 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -990,11 +990,10 @@ func (proxier *Proxier) syncProxyRules() { klog.ErrorS(err, "Error listing addresses binded to dummy interface") } // nodeAddressSet All addresses *except* those on the dummy interface - nodeAddressSet, err := proxier.netlinkHandle.GetAllLocalAddresses() + nodeAddressSet, err := proxier.netlinkHandle.GetAllLocalAddressesExcept(defaultDummyDevice) if err != nil { klog.ErrorS(err, "Error listing node addresses") } - nodeAddressSet = nodeAddressSet.Difference(alreadyBoundAddrs) hasNodePort := false for _, svc := range proxier.svcPortMap { diff --git a/pkg/proxy/ipvs/testing/fake.go b/pkg/proxy/ipvs/testing/fake.go index 3ae6c00750e..b0cdce8abd1 100644 --- a/pkg/proxy/ipvs/testing/fake.go +++ b/pkg/proxy/ipvs/testing/fake.go @@ -140,6 +140,21 @@ func (h *FakeNetlinkHandle) GetAllLocalAddresses() (sets.Set[string], error) { return res, nil } +func (h *FakeNetlinkHandle) GetAllLocalAddressesExcept(dev string) (sets.Set[string], error) { + res := sets.New[string]() + for linkName := range h.localAddresses { + if linkName == dev { + continue + } + for _, addr := range h.localAddresses[linkName] { + if h.isValidForSet(addr) { + res.Insert(addr) + } + } + } + return res, nil +} + // SetLocalAddresses set IP addresses to the given interface device. It's not part of interface. func (h *FakeNetlinkHandle) SetLocalAddresses(dev string, ips ...string) error { if h.localAddresses == nil { diff --git a/pkg/proxy/ipvs/testing/fake_test.go b/pkg/proxy/ipvs/testing/fake_test.go index a8cd2b5cb7e..bba5503f386 100644 --- a/pkg/proxy/ipvs/testing/fake_test.go +++ b/pkg/proxy/ipvs/testing/fake_test.go @@ -23,6 +23,7 @@ import ( "k8s.io/kubernetes/pkg/proxy/ipvs" ) +// (I am unsure if this test has any value since it only tests the fake implementation) func TestSetGetLocalAddresses(t *testing.T) { fake := NewFakeNetlinkHandle(false) _ = ipvs.NetLinkHandle(fake) // Ensure that the interface is honored @@ -43,10 +44,15 @@ func TestSetGetLocalAddresses(t *testing.T) { if !addr.Equal(expected) { t.Errorf("Unexpected mismatch, expected: %v, got: %v", expected, addr) } - fake.SetLocalAddresses("kube-ipvs0", "4.3.2.1") + fake.SetLocalAddresses("kube-ipvs0", "1.2.3.4", "4.3.2.1") addr, _ = fake.GetAllLocalAddresses() expected = sets.New("1.2.3.4", "4.3.2.1") if !addr.Equal(expected) { t.Errorf("Unexpected mismatch, expected: %v, got: %v", expected, addr) } + addr, _ = fake.GetAllLocalAddressesExcept("kube-ipvs0") + expected = sets.New("1.2.3.4") + if !addr.Equal(expected) { + t.Errorf("Unexpected mismatch, expected: %v, got: %v", expected, addr) + } } From 5ece6541b805379099c4bc1b1191b973d52561ec Mon Sep 17 00:00:00 2001 From: Lars Ekman Date: Thu, 27 Apr 2023 20:47:10 +0200 Subject: [PATCH 2/2] proxy/ipvs: don't bind nodeips to the dummy device --- pkg/proxy/ipvs/proxier.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/pkg/proxy/ipvs/proxier.go b/pkg/proxy/ipvs/proxier.go index 93a3ed4620e..7c5152f34aa 100644 --- a/pkg/proxy/ipvs/proxier.go +++ b/pkg/proxy/ipvs/proxier.go @@ -1169,9 +1169,13 @@ func (proxier *Proxier) syncProxyRules() { if proxier.ipvsScheduler == "mh" { serv.Flags |= utilipvs.FlagSourceHash } - if err := proxier.syncService(svcPortNameString, serv, true, alreadyBoundAddrs); err == nil { + // We must not add the address to the dummy device if it exist on another interface + shouldBind := !nodeAddressSet.Has(serv.Address.String()) + if err := proxier.syncService(svcPortNameString, serv, shouldBind, alreadyBoundAddrs); err == nil { activeIPVSServices.Insert(serv.String()) - activeBindAddrs.Insert(serv.Address.String()) + if shouldBind { + activeBindAddrs.Insert(serv.Address.String()) + } if err := proxier.syncEndpoint(svcPortName, svcInfo.ExternalPolicyLocal(), serv); err != nil { klog.ErrorS(err, "Failed to sync endpoint for service", "servicePortName", svcPortName, "virtualServer", serv) } @@ -1272,9 +1276,13 @@ func (proxier *Proxier) syncProxyRules() { if proxier.ipvsScheduler == "mh" { serv.Flags |= utilipvs.FlagSourceHash } - if err := proxier.syncService(svcPortNameString, serv, true, alreadyBoundAddrs); err == nil { + // We must not add the address to the dummy device if it exist on another interface + shouldBind := !nodeAddressSet.Has(serv.Address.String()) + if err := proxier.syncService(svcPortNameString, serv, shouldBind, alreadyBoundAddrs); err == nil { activeIPVSServices.Insert(serv.String()) - activeBindAddrs.Insert(serv.Address.String()) + if shouldBind { + activeBindAddrs.Insert(serv.Address.String()) + } if err := proxier.syncEndpoint(svcPortName, svcInfo.ExternalPolicyLocal(), serv); err != nil { klog.ErrorS(err, "Failed to sync endpoint for service", "servicePortName", svcPortName, "virtualServer", serv) }