fix(routes): deleting wrong routes when other server has same private…

… IP (#472)

In case another server in the project has the same Private IP (in
another network) as one of our cluster nodes, hccm would happily
delete&recreate the route on every route reconciliation.

This fixes the bug by only adding the Private IPs in the correct network
to the AllServersCache ByPrivateIP lookup map.

Fixes #470

Co-authored-by: Jonas Lammler <ljonas@riseup.net>

hcloud/routes.go

@@ -35,9 +35,14 @@ func newRoutes(client *hcloud.Client, networkID int64) (*routes, error) {
 	}
 
 	return &routes{
-		client:      client,
-		network:     networkObj,
-		serverCache: &hcops.AllServersCache{LoadFunc: client.Server.All},
+		client:  client,
+		network: networkObj,
+		serverCache: &hcops.AllServersCache{
+			// client.Server.All will load ALL the servers in the project, even those
+			// that are not part of the Kubernetes cluster.
+			LoadFunc: client.Server.All,
+			Network:  networkObj,
+		},
 	}, nil
 }
 

internal/hcops/server.go

@@ -7,6 +7,8 @@ import (
 	"sync"
 	"time"
 
+	"k8s.io/klog/v2"
+
 	"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/metrics"
 	"github.com/hetznercloud/hcloud-go/v2/hcloud"
 )
@@ -21,6 +23,9 @@ type AllServersCache struct {
 	LoadTimeout time.Duration
 	MaxAge      time.Duration
 
+	// If set, only IPs in this network will be considered for [ByPrivateIP]
+	Network *hcloud.Network
+
 	lastRefresh time.Time
 	byPrivIP    map[string]*hcloud.Server
 	byName      map[string]*hcloud.Server
@@ -102,9 +107,18 @@ func (c *AllServersCache) getCache(getSrv func() (*hcloud.Server, bool)) (*hclou
 	for _, srv := range srvs {
 		// Index servers by the IPs of their private networks
 		for _, n := range srv.PrivateNet {
+			if c.Network != nil {
+				if n.Network.ID != c.Network.ID {
+					// Only consider private IPs in the right network
+					continue
+				}
+			}
 			if n.IP == nil {
 				continue
 			}
+			if _, ok := c.byPrivIP[n.IP.String()]; ok {
+				klog.Warningf("Overriding AllServersCache entry for private ip %s with server %s\n", n.IP.String(), srv.Name)
+			}
 			c.byPrivIP[n.IP.String()] = srv
 		}
 

internal/hcops/server_test.go

@@ -197,6 +197,52 @@ func TestAllServersCache_ClientError(t *testing.T) {
 	runAllServersCacheTests(t, "Not found", tmpl, cacheOps)
 }
 
+func TestAllServersCache_DuplicatePrivateIP(t *testing.T) {
+	// Regression test for /~https://github.com/hetznercloud/hcloud-cloud-controller-manager/issues/470
+
+	network := &hcloud.Network{
+		ID:   12345,
+		Name: "cluster-network",
+	}
+	srv := &hcloud.Server{
+		ID:   101010,
+		Name: "cluster-node",
+		PrivateNet: []hcloud.ServerPrivateNet{
+			{
+				IP:      net.ParseIP("10.0.0.4"),
+				Network: network,
+			},
+		},
+	}
+	srvInvalid := &hcloud.Server{
+		ID:   101012,
+		Name: "invalid-node",
+		PrivateNet: []hcloud.ServerPrivateNet{
+			{
+				IP: net.ParseIP("10.0.0.4"),
+				Network: &hcloud.Network{
+					ID:   54321,
+					Name: "invalid-network",
+				},
+			},
+		},
+	}
+
+	cacheOps := newAllServersCacheOps(t, srv)
+	tmpl := allServersCacheTestCase{
+		SetUp: func(t *testing.T, tt *allServersCacheTestCase) {
+			tt.Cache.Network = network
+
+			tt.ServerClient.
+				On("All", mock.Anything).
+				Return([]*hcloud.Server{srv, srvInvalid}, nil)
+		},
+		Expected: srv,
+	}
+
+	runAllServersCacheTests(t, "DuplicatePrivateIP", tmpl, cacheOps)
+}
+
 type allServersCacheOp func(c *hcops.AllServersCache) (*hcloud.Server, error)
 
 func newAllServersCacheOps(t *testing.T, srv *hcloud.Server) map[string]allServersCacheOp {