From 47fcc786065a32e56258689b6efc7f322b343bcf Mon Sep 17 00:00:00 2001
From: Chris Tarazi <chris@isovalent.com>
Date: Thu, 27 Apr 2023 17:28:17 -0700
Subject: [PATCH] node/manager: Utilize set.SliceSubsetOf in ipcache deletion

This cleans up the code to have proper diffing logic when delete node
IPs from the ipcache. Previously, the logic had a potential for a bug
because it assumed that oldIPs and newIPs were sorted.

FWIW, no bug has been reported in regards to the previous code fixed by
this commit, so it's theoretical for now. In the end, this is a
defensive change to prevent it from happening.

Additionally, this commit will ease the changes that will be introduced
in https://github.com/cilium/cilium/pull/23208.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
---
 pkg/node/manager/manager.go | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/pkg/node/manager/manager.go b/pkg/node/manager/manager.go
index 5f7011e87bbd..49866e3459c9 100644
--- a/pkg/node/manager/manager.go
+++ b/pkg/node/manager/manager.go
@@ -33,6 +33,7 @@ import (
 	nodeTypes "github.com/cilium/cilium/pkg/node/types"
 	"github.com/cilium/cilium/pkg/option"
 	"github.com/cilium/cilium/pkg/rand"
+	"github.com/cilium/cilium/pkg/set"
 	"github.com/cilium/cilium/pkg/source"
 )
 
@@ -554,19 +555,9 @@ func (m *manager) upsertIntoIDMD(prefix netip.Prefix, id identity.NumericIdentit
 // deleteIPCache deletes the IP addresses from the IPCache with the 'oldSource'
 // if they are not found in the newIPs slice.
 func (m *manager) deleteIPCache(oldSource source.Source, oldIPs []string, newIPs []string) {
-	for _, address := range oldIPs {
-		var found bool
-		for _, ipAdded := range newIPs {
-			if ipAdded == address {
-				found = true
-				break
-			}
-		}
-		// Delete from the IPCache if the node's IP addresses was not
-		// added in this update.
-		if !found {
-			m.ipcache.Delete(address, oldSource)
-		}
+	_, diff := set.SliceSubsetOf(oldIPs, newIPs)
+	for _, address := range diff {
+		m.ipcache.Delete(address, oldSource)
 	}
 }
 
@@ -603,6 +594,11 @@ func (m *manager) NodeDeleted(n nodeTypes.Node) {
 		return
 	}
 
+	extraIPs := []net.IP{
+		entry.node.IPv4HealthIP, entry.node.IPv6HealthIP,
+		entry.node.IPv4IngressIP, entry.node.IPv6IngressIP,
+	}
+	toDelete := make([]string, 0, len(entry.node.IPAddresses)+len(extraIPs))
 	for _, address := range entry.node.IPAddresses {
 		if option.Config.NodeIpsetNeeded() && address.Type == addressing.NodeInternalIP {
 			iptables.RemoveFromNodeIpset(address.IP)
@@ -618,17 +614,14 @@ func (m *manager) NodeDeleted(n nodeTypes.Node) {
 		} else {
 			prefix = ip.IPToNetPrefix(address.IP.To16())
 		}
-		m.ipcache.Delete(prefix.String(), n.Source)
+		toDelete = append(toDelete, prefix.String())
 	}
-
-	for _, address := range []net.IP{
-		entry.node.IPv4HealthIP, entry.node.IPv6HealthIP,
-		entry.node.IPv4IngressIP, entry.node.IPv6IngressIP,
-	} {
+	for _, address := range extraIPs {
 		if address != nil {
-			m.ipcache.Delete(address.String(), n.Source)
+			toDelete = append(toDelete, address.String())
 		}
 	}
+	m.deleteIPCache(n.Source, toDelete, nil)
 
 	m.metricNumNodes.Dec()