fix: routes controller on node name drift

If a Kubernetes node got successfully initialized we have the Server
ID stored in the node objets `providerID`. When renaming a Server,
causing a drift between the node and Server name, we should still
be able to identify the Server via its ID. This is currently not
implemented in the routes' controller, where we rely purly on the
Servers name.

As the upstream library only provides us with the Kubernetes node name
and not the full object, we have to first fetch it from the Kubernetes
API. With the node object at hand we can identify and fetch the Server
by its ID. Furthermore, we can use the Kubernetes node object to improve
our handling of Kubernetes events. The event emitted on a CIDR mismatch
is currently not associated with the affected node, because so far we
did not have access to the nodes UUID.

Author: lukasmetzner

hcloud/cloud.go

@@ -28,6 +28,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/client-go/kubernetes/scheme"
 	v1 "k8s.io/client-go/kubernetes/typed/core/v1"
+	corelisters "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/record"
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/klog/v2"
@@ -55,9 +56,10 @@ type cloud struct {
 	recorder    record.EventRecorder
 	networkID   int64
 	cidr        string
+	nodeLister  corelisters.NodeLister
 }
 
-func NewCloud(cidr string) (cloudprovider.Interface, error) {
+func NewCloud(cidr string, nodeLister corelisters.NodeLister) (cloudprovider.Interface, error) {
 	const op = "hcloud/newCloud"
 	metrics.OperationCalled.WithLabelValues(op).Inc()
 
@@ -147,6 +149,7 @@ func NewCloud(cidr string) (cloudprovider.Interface, error) {
 		cfg:         cfg,
 		networkID:   networkID,
 		cidr:        cidr,
+		nodeLister:  nodeLister,
 	}, nil
 }
 
@@ -213,6 +216,7 @@ func (c *cloud) Routes() (cloudprovider.Routes, bool) {
 		c.networkID,
 		c.cidr,
 		c.recorder,
+		c.nodeLister,
 	)
 	if err != nil {
 		klog.ErrorS(err, "create routes provider", "networkID", c.networkID)

hcloud/cloud_test.go

@@ -95,7 +95,7 @@ func TestNewCloud(t *testing.T) {
 		json.NewEncoder(w).Encode(schema.LocationListResponse{Locations: []schema.Location{}})
 	})
 
-	_, err := NewCloud(DefaultClusterCIDR)
+	_, err := NewCloud(DefaultClusterCIDR, nil)
 	assert.NoError(t, err)
 }
 
@@ -107,7 +107,7 @@ func TestNewCloudConnectionNotPossible(t *testing.T) {
 	)
 	defer resetEnv()
 
-	_, err := NewCloud(DefaultClusterCIDR)
+	_, err := NewCloud(DefaultClusterCIDR, nil)
 	assert.EqualError(t, err,
 		`hcloud/newCloud: Get "http://127.0.0.1:4711/v1/locations?": dial tcp 127.0.0.1:4711: connect: connection refused`)
 }
@@ -135,7 +135,7 @@ func TestNewCloudInvalidToken(t *testing.T) {
 		)
 	})
 
-	_, err := NewCloud(DefaultClusterCIDR)
+	_, err := NewCloud(DefaultClusterCIDR, nil)
 	assert.EqualError(t, err, "hcloud/newCloud: unable to authenticate (unauthorized)")
 }
 
@@ -172,7 +172,7 @@ func TestCloud(t *testing.T) {
 		)
 	})
 
-	cloud, err := NewCloud(DefaultClusterCIDR)
+	cloud, err := NewCloud(DefaultClusterCIDR, nil)
 	if err != nil {
 		t.Fatalf("Unexpected error: %v", err)
 	}
@@ -229,7 +229,7 @@ func TestCloud(t *testing.T) {
 		)
 		defer resetEnv()
 
-		c, err := NewCloud(DefaultClusterCIDR)
+		c, err := NewCloud(DefaultClusterCIDR, nil)
 		if err != nil {
 			t.Errorf("%s", err)
 		}

hcloud/routes.go

@@ -5,19 +5,21 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"slices"
 	"time"
 
 	"golang.org/x/time/rate"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	corelisters "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/record"
 	cloudprovider "k8s.io/cloud-provider"
 	"k8s.io/klog/v2"
 
 	"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/hcops"
 	"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/metrics"
+	"github.com/hetznercloud/hcloud-cloud-controller-manager/internal/providerid"
 	"github.com/hetznercloud/hcloud-go/v2/hcloud"
 )
 
@@ -29,9 +31,10 @@ type routes struct {
 	serverCache *hcops.AllServersCache
 	clusterCIDR *net.IPNet
 	recorder    record.EventRecorder
+	nodeLister  corelisters.NodeLister
 }
 
-func newRoutes(client *hcloud.Client, networkID int64, clusterCIDR string, recorder record.EventRecorder) (*routes, error) {
+func newRoutes(client *hcloud.Client, networkID int64, clusterCIDR string, recorder record.EventRecorder, nodeLister corelisters.NodeLister) (*routes, error) {
 	const op = "hcloud/newRoutes"
 	metrics.OperationCalled.WithLabelValues(op).Inc()
 
@@ -60,6 +63,7 @@ func newRoutes(client *hcloud.Client, networkID int64, clusterCIDR string, recor
 		},
 		clusterCIDR: cidr,
 		recorder:    recorder,
+		nodeLister:  nodeLister,
 	}, nil
 }
 
@@ -105,85 +109,132 @@ func (r *routes) CreateRoute(ctx context.Context, _ string, _ string, route *clo
 	const op = "hcloud/CreateRoute"
 	metrics.OperationCalled.WithLabelValues(op).Inc()
 
-	srv, err := r.serverCache.ByName(ctx, string(route.TargetNode))
+	node, gateway, err := r.resolveRouteTarget(ctx, string(route.TargetNode))
 	if err != nil {
 		return fmt.Errorf("%s: %w", op, err)
 	}
 
-	privNet, ok := findServerPrivateNetByID(srv, r.network.ID)
-	if !ok {
-		r.serverCache.InvalidateCache()
-		srv, err = r.serverCache.ByName(ctx, string(route.TargetNode))
-		if err != nil {
-			return fmt.Errorf("%s: %w", op, err)
-		}
-
-		privNet, ok = findServerPrivateNetByID(srv, r.network.ID)
-		if !ok {
-			return fmt.Errorf("%s: server %v: network with id %d not attached to this server", op, route.TargetNode, r.network.ID)
-		}
+	if !slices.ContainsFunc(route.TargetNodeAddresses, func(target corev1.NodeAddress) bool {
+		return target.Type == corev1.NodeInternalIP && target.Address == gateway.String()
+	}) {
+		return fmt.Errorf("%s: IP %s not part of routes target addresses", op, gateway.String())
 	}
-	ip := privNet.IP
 
 	_, cidr, err := net.ParseCIDR(route.DestinationCIDR)
 	if err != nil {
 		return fmt.Errorf("%s: %w", op, err)
 	}
 
-	clusterPrefixLen, _ := r.clusterCIDR.Mask.Size()
-	destPrefixLen, _ := cidr.Mask.Size()
+	r.warnCIDRMismatch(cidr, node)
 
-	if !r.clusterCIDR.Contains(cidr.IP) || destPrefixLen < clusterPrefixLen {
-		node := &corev1.Node{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      string(route.TargetNode),
-				Namespace: "",
-			},
-		}
-		// Event is only visible via `kubectl get events` and not `kubectl describe node`,
-		// as we do not have the UID here and `kubectl describe node` filters by UID.
-		// Because of this behavior we are also dispatching a log message.
-		r.recorder.Eventf(
-			node,
-			corev1.EventTypeWarning,
-			"ClusterCIDRMisconfigured",
-			"route CIDR %s is not contained within cluster CIDR %s",
-			route.DestinationCIDR,
-			r.clusterCIDR.String(),
-		)
-		klog.Warningf(
-			"route CIDR %s is not contained within cluster CIDR %s",
-			route.DestinationCIDR,
-			r.clusterCIDR.String(),
-		)
+	return r.upsertRoute(ctx, gateway, cidr, string(route.TargetNode))
+}
+
+// resolveRouteTarget returns the k8s node and the hcloud server's private IP on the routes
+// network — everything needed to create a route for this node (gateway IP) and record events
+// against it (node).
+// Looks up the server by ProviderID to survive k8s node-name drift, with a ByName fallback for
+// ID changes (e.g. server recreated). Refreshes the cache once if the private-net attachment
+// isn't yet reflected.
+func (r *routes) resolveRouteTarget(ctx context.Context, nodeName string) (*corev1.Node, net.IP, error) {
+	const op = "hcloud/resolveRouteTarget"
+	metrics.OperationCalled.WithLabelValues(op).Inc()
+
+	node, err := r.nodeLister.Get(nodeName)
+	if err != nil {
+		return nil, nil, fmt.Errorf("%s: %w", op, err)
+	}
+
+	if node.Spec.ProviderID == "" {
+		return nil, nil, fmt.Errorf("%s: node %q not yet initialized", op, node.Name)
+	}
+
+	id, isCloudServer, err := providerid.ToServerID(node.Spec.ProviderID)
+	if err != nil {
+		return nil, nil, fmt.Errorf("%s: %w", op, err)
+	}
+	if !isCloudServer {
+		return nil, nil, fmt.Errorf("%s: node %q is not a Cloud server, routes are only supported for Cloud servers", op, node.Name)
 	}
 
-	routeExists, err := r.checkIfRouteAlreadyExists(ctx, route)
+	server, err := r.serverCache.ByID(ctx, id)
+	if errors.Is(err, hcops.ErrNotFound) {
+		server, err = r.serverCache.ByName(ctx, node.Name)
+	}
 	if err != nil {
+		return nil, nil, fmt.Errorf("%s: %w", op, err)
+	}
+
+	privNet, ok := findServerPrivateNetByID(server, r.network.ID)
+	if !ok {
+		r.serverCache.InvalidateCache()
+		server, err = r.serverCache.ByID(ctx, server.ID)
+		if err != nil {
+			return nil, nil, fmt.Errorf("%s: %w", op, err)
+		}
+		privNet, ok = findServerPrivateNetByID(server, r.network.ID)
+		if !ok {
+			return nil, nil, fmt.Errorf("%s: server %q: network with id %d not attached to this server", op, node.Name, r.network.ID)
+		}
+	}
+
+	return node, privNet.IP, nil
+}
+
+// upsertRoute ensures the hcloud network has a route for cidr pointing at gateway. A matching
+// route is a no-op; a stale route with a different gateway is replaced in place. nodeName is
+// used only for logging and for surfacing API conflicts against the right k8s object.
+func (r *routes) upsertRoute(ctx context.Context, gateway net.IP, cidr *net.IPNet, nodeName string) error {
+	const op = "hcloud/upsertRoute"
+	metrics.OperationCalled.WithLabelValues(op).Inc()
+
+	if err := r.reloadNetwork(ctx); err != nil {
 		return fmt.Errorf("%s: %w", op, err)
 	}
 
-	if routeExists {
+	destination := cidr.String()
+	existingIdx := slices.IndexFunc(r.network.Routes, func(nr hcloud.NetworkRoute) bool {
+		return nr.Destination.String() == destination
+	})
+	if existingIdx >= 0 {
+		existing := r.network.Routes[existingIdx]
+		if existing.Gateway.Equal(gateway) {
+			klog.InfoS(
+				"route already exists: skipping creation",
+				"target-node", nodeName,
+				"destination-cidr", destination,
+			)
+			return nil
+		}
+
+		action, _, err := r.client.Network.DeleteRoute(ctx, r.network, hcloud.NetworkDeleteRouteOpts{
+			Route: existing,
+		})
+		if err != nil {
+			return fmt.Errorf("%s: %w", op, err)
+		}
+		if err := r.client.Action.WaitFor(ctx, action); err != nil {
+			return fmt.Errorf("%s: %w", op, err)
+		}
 		klog.InfoS(
-			"route already exists: skipping creation",
-			"target-node", route.TargetNode,
-			"destination-cidr", route.DestinationCIDR,
+			"deleted stale route with wrong gateway; recreating",
+			"target-node", nodeName,
+			"destination-cidr", destination,
 		)
-		return nil
 	}
 
 	opts := hcloud.NetworkAddRouteOpts{
 		Route: hcloud.NetworkRoute{
 			Destination: cidr,
-			Gateway:     ip,
+			Gateway:     gateway,
 		},
 	}
 	action, _, err := r.client.Network.AddRoute(ctx, r.network, opts)
 	if err != nil {
 		if hcloud.IsError(err, hcloud.ErrorCodeLocked, hcloud.ErrorCodeConflict) {
 			return apierrors.NewConflict(
 				corev1.Resource("nodes"),
-				string(route.TargetNode),
+				nodeName,
 				err,
 			)
 		}
@@ -220,17 +271,6 @@ func (r *routes) DeleteRoute(ctx context.Context, _ string, route *cloudprovider
 		return fmt.Errorf("%s: %w", op, err)
 	}
 
-	err = r.deleteRouteFromHcloud(ctx, cidr, ip)
-	if err != nil {
-		return fmt.Errorf("%s: %w", op, err)
-	}
-	return nil
-}
-
-func (r *routes) deleteRouteFromHcloud(ctx context.Context, cidr *net.IPNet, ip net.IP) error {
-	const op = "hcloud/deleteRouteFromHcloud"
-	metrics.OperationCalled.WithLabelValues(op).Inc()
-
 	opts := hcloud.NetworkDeleteRouteOpts{
 		Route: hcloud.NetworkRoute{
 			Destination: cidr,
@@ -272,43 +312,19 @@ func (r *routes) hcloudRouteToRoute(ctx context.Context, route hcloud.NetworkRou
 	return cpRoute, nil
 }
 
-func (r *routes) checkIfRouteAlreadyExists(ctx context.Context, route *cloudprovider.Route) (bool, error) {
-	const op = "hcloud/checkIfRouteAlreadyExists"
-	metrics.OperationCalled.WithLabelValues(op).Inc()
+func (r *routes) warnCIDRMismatch(cidr *net.IPNet, node *corev1.Node) {
+	clusterPrefixLen, _ := r.clusterCIDR.Mask.Size()
+	destPrefixLen, _ := cidr.Mask.Size()
 
-	if err := r.reloadNetwork(ctx); err != nil {
-		return false, fmt.Errorf("%s: %w", op, err)
-	}
-
-	for _, _route := range r.network.Routes {
-		if _route.Destination.String() == route.DestinationCIDR {
-			srv, err := r.serverCache.ByName(ctx, string(route.TargetNode))
-			if err != nil {
-				return false, fmt.Errorf("%s: %w", op, err)
-			}
-			privNet, ok := findServerPrivateNetByID(srv, r.network.ID)
-			if !ok {
-				return false, fmt.Errorf("%s: server %v: no network with id: %d", op, route.TargetNode, r.network.ID)
-			}
-			ip := privNet.IP
-
-			if !_route.Gateway.Equal(ip) {
-				action, _, err := r.client.Network.DeleteRoute(ctx, r.network, hcloud.NetworkDeleteRouteOpts{
-					Route: _route,
-				})
-				if err != nil {
-					return false, fmt.Errorf("%s: %w", op, err)
-				}
-
-				if err := r.client.Action.WaitFor(ctx, action); err != nil {
-					return false, fmt.Errorf("%s: %w", op, err)
-				}
-			}
-
-			return true, nil
-		}
+	if !r.clusterCIDR.Contains(cidr.IP) || destPrefixLen < clusterPrefixLen {
+		warnMsg := fmt.Sprintf(
+			"route CIDR %s is not contained within cluster CIDR %s",
+			cidr.String(),
+			r.clusterCIDR.String(),
+		)
+		klog.Warning(warnMsg)
+		r.recorder.Event(node, corev1.EventTypeWarning, "ClusterCIDRMisconfigured", warnMsg)
 	}
-	return false, nil
 }
 
 func findServerPrivateNetByID(srv *hcloud.Server, id int64) (hcloud.ServerPrivateNet, bool) {