Repository: hetznercloud/hcloud-cloud-controller-manager (go)

Issue:
Working routes to alias IPs are being deleted by the HCCM
## Summary:

If the hcloud Network that HCCM is attached to contains a route that points to an _alias_ IP assigned to a server, the HCCM  assumes this route is not functional and deletes it:

```
I0417 13:36:28.843781       1 routes.go:75] "server for route not
found, deleting route in hcloud because the route is not functional"
op="hcloud/ListRoutes" gateway="10.XYZ.0.254"
err="hcloud/hcloudRouteToRoute: hcops/AllServersCache.ByPrivateIP:
10.XYZ.0.254 hcops/AllServersCache.getCache: not found"
```

This is not expected behaviour, because using an alias IP in a route is supported by Hetzner cloud -- so in practice the HCCM is deleting functional routes that should be kept in place.

Version in use: `hetznercloud/hcloud-cloud-controller-manager:v1.15.0`

##  In detail, with context:

I'm trying to run K3S worker/slave nodes without public IPs in an HA cluster, since assigning precious routable IPv4 on them would be wasteful. However, since Hetzner cloud doesn't provide a NAT gateway service, I also need to run my own NAT gateway to provide internet connectivity to them.

Running a simple NAT GW is trivial: `iptables -t nat ...` on a VM with a public and private ip, ask the hcloud network to route 0.0.0.0/0 viat that VM's private IP, and you're done. 

However, an HA K3S cluster also needs an HA NAT gateway. Since there are no floating private IPs in hcloud, the next best option is to run a bunch of redundant servers that function as NAT gateways, choose an *alias* private IP (here `10.XYZ.0.254`) as a Virtual IP, and run keepalived on these servers to assign the alias IP to a currently working server (by hooking in a script that calls the hcloud API whenever the alias IP needs to switch owners). In short:

```
$ hcloud server describe my-cluster-node-01
ID:             SERVER_ID
Public Net:
  IPv4:
    ID:         NNNNN
    IP:         167.NNN.NNN.NNN
Private Net:
  - ID:                 $NETWORK_ID
    Name:               my-cluster-net
    IP:                 10.XYZ.0.11
    Alias IPs:
     -                  10.XYZ.0.254
.... 

$ hcloud network add-route  $NETWORK_ID --destination 0.0.0.0/0 --gateway 10.XYZ.0.254

$ hcloud network describe $NETWORK_ID
ID:             $NETWORK_ID
Name:           my-cluster-net
IP Range:       10.XYZ.0.0/24
Subnets:
  - Type:               cloud
    Network Zone:       eu-central
    IP Range:           10.XYZ.0.0/24
    Gateway:            10.XYZ.0.1
Routes:
  - Destination:        10.42.2.0/24
    Gateway:            10.XYZ.0.2
  - Destination:        10.42.1.0/24
    Gateway:            10.XYZ.0.12
  - Destination:        0.0.0.0/0
    Gateway:            10.XYZ.0.254
  - Destination:        10.42.0.0/24
    Gateway:            10.XYZ.0.11
 ```

As mentioned above, a couple of seconds after adding the 0.0.0.0/0 route, it it vanishes from `hcloud network describe` output -- with the message at the top of the report logged by the HCCM container.

(FWIW -- in this use case, I could probably work around the issue by having the keepaliveds modify the *hcloud network's 0.0.0.0/0 route* instead of the alias IP, pointing it to the relevant server's "primary" private IP, and maybe even dropping the alias IP altogether. 

But conceptually, switching a Virtual IP between gateways would be a more conventional and recognizable pattern, and in general, I suppose HCCM should recognize routes to alias IPs as well.)


Pull request:
fix(routes): Only delete routes in the Cluster CIDR
This is a two-part fix for #425.

Closes #425 

More details can be found in [this comment](https://github.com/hetznercloud/hcloud-cloud-controller-manager/issues/425#issuecomment-1525776879)

## Part 1: [fix(routes): correctly handle delete for routes without server](https://github.com/hetznercloud/hcloud-cloud-controller-manager/commit/ac55e6f91d815813469fc01801cbd436c608ad9e)

We previously required that the route we were about to delete [had a valid server](https://github.com/hetznercloud/hcloud-cloud-controller-manager/blob/892c20e199e1aee64e731aa9f05e9320dfc5d1bd/hcloud/routes.go#L161-L169). This does not make sense because in most cases we delete the route precisely because the server was deleted.

## Part 2: [fix(routes): let k/cloud-provider decide which routes to delete](https://github.com/hetznercloud/hcloud-cloud-controller-manager/commit/e2ea24c8f30ae49c0ea13b263e5ecb1f6b900dbf)

We implemented a route cleanup in https://github.com/hetznercloud/hcloud-cloud-controller-manager/pull/238 that deletes all routes in the network that can not be mapped to the private IP of a node. This causes issues for users who create their own routes, e.g. to an alias IP.

We do not need to implement this ourselves, because`kubernetes/cloud-provider` already [implemented all the checks](https://github.com/kubernetes/cloud-provider/blob/22e08785c750bd4610af7198d43ecf72b688e9ed/controllers/route/route_controller.go#L220-L260) for us, with more consideration about which routes actually need to be deleted.

In particular, the ccm should only delete routes that are within the configured `--cluster-cidr` range.

Patch:
diff --git a/hcloud/routes.go b/hcloud/routes.go
index d58bf280..424efcb3 100644
--- a/hcloud/routes.go
+++ b/hcloud/routes.go
@@ -65,22 +65,10 @@ func (r *routes) ListRoutes(ctx context.Context, _ string) ([]*cloudprovider.Rou
 		return nil, fmt.Errorf("%s: %w", op, err)
 	}
 
-	// We do not now the exact length here, as the network might have outdated routes with servers that do not exist
-	// anymore.
-	var routes []*cloudprovider.Route //nolint:prealloc
+	routes := make([]*cloudprovider.Route, 0, len(r.network.Routes))
 	for _, route := range r.network.Routes {
 		ro, err := r.hcloudRouteToRoute(route)
 		if err != nil {
-			if errors.Is(err, hcops.ErrNotFound) {
-				klog.InfoS("server for route not found, deleting route in hcloud because the route is not functional",
-					"op", op, "gateway", route.Gateway.String(), "err", fmt.Sprintf("%v", err))
-				err = r.deleteRouteFromHcloud(ctx, route.Destination, route.Gateway)
-				if err != nil {
-					klog.InfoS("deleting the route failed, continue",
-						"op", op, "gateway", route.Gateway.String(), "err", fmt.Sprintf("%v", err))
-				}
-				continue
-			}
 			return routes, fmt.Errorf("%s: %w", op, err)
 		}
 		routes = append(routes, ro)
@@ -158,20 +146,23 @@ func (r *routes) DeleteRoute(ctx context.Context, _ string, route *cloudprovider
 	const op = "hcloud/DeleteRoute"
 	metrics.OperationCalled.WithLabelValues(op).Inc()
 
-	srv, err := r.serverCache.ByName(string(route.TargetNode))
-	if err != nil {
-		return fmt.Errorf("%s: %v", op, err)
+	// Get target IP from current list of routes, routes can be uniquely identified by their destination cidr.
+	var ip net.IP
+	for _, cloudRoute := range r.network.Routes {
+		if cloudRoute.Destination.String() == route.DestinationCIDR {
+			ip = cloudRoute.Gateway
+			break
+		}
 	}
-	privNet, ok := findServerPrivateNetByID(srv, r.network.ID)
-	if !ok {
-		return fmt.Errorf("%s: server %v: no network with id: %d", op, route.TargetNode, r.network.ID)
+	if ip.IsUnspecified() {
+		return fmt.Errorf("%s: route %s not found in cloud network routes", op, route.Name)
 	}
-	ip := privNet.IP
 
 	_, cidr, err := net.ParseCIDR(route.DestinationCIDR)
 	if err != nil {
 		return fmt.Errorf("%s: %w", op, err)
 	}
+
 	err = r.deleteRouteFromHcloud(ctx, cidr, ip)
 	if err != nil {
 		return fmt.Errorf("%s: %w", op, err)
@@ -212,17 +203,24 @@ func (r *routes) hcloudRouteToRoute(route hcloud.NetworkRoute) (*cloudprovider.R
 	const op = "hcloud/hcloudRouteToRoute"
 	metrics.OperationCalled.WithLabelValues(op).Inc()
 
+	cpRoute := &cloudprovider.Route{
+		DestinationCIDR: route.Destination.String(),
+		Name:            fmt.Sprintf("%s-%s", route.Gateway.String(), route.Destination.String()),
+	}
+
 	srv, err := r.serverCache.ByPrivateIP(route.Gateway)
 	if err != nil {
+		if errors.Is(err, hcops.ErrNotFound) {
+			// Route belongs to non-existing target
+			cpRoute.Blackhole = true
+			return cpRoute, nil
+		}
+
 		return nil, fmt.Errorf("%s: %w", op, err)
 	}
-	nodeName := srv.Name
 
-	return &cloudprovider.Route{
-		DestinationCIDR: route.Destination.String(),
-		Name:            fmt.Sprintf("%s-%s", route.Gateway.String(), route.Destination.String()),
-		TargetNode:      types.NodeName(nodeName),
-	}, nil
+	cpRoute.TargetNode = types.NodeName(srv.Name)
+	return cpRoute, nil
 }
 
 func (r *routes) checkIfRouteAlreadyExists(ctx context.Context, route *cloudprovider.Route) (bool, error) {


Test patch:
diff --git a/hcloud/routes_test.go b/hcloud/routes_test.go
index fe347ec3..69f49e3c 100644
--- a/hcloud/routes_test.go
+++ b/hcloud/routes_test.go
@@ -140,28 +140,18 @@ func TestRoutes_ListRoutes(t *testing.T) {
 func TestRoutes_DeleteRoute(t *testing.T) {
 	env := newTestEnv()
 	defer env.Teardown()
-	env.Mux.HandleFunc("/servers", func(w http.ResponseWriter, r *http.Request) {
-		json.NewEncoder(w).Encode(schema.ServerListResponse{
-			Servers: []schema.Server{
-				{
-					ID:   1,
-					Name: "node15",
-					PrivateNet: []schema.ServerPrivateNet{
-						{
-							Network: 1,
-							IP:      "10.0.0.2",
-						},
-					},
-				},
-			},
-		})
-	})
 	env.Mux.HandleFunc("/networks/1", func(w http.ResponseWriter, r *http.Request) {
 		json.NewEncoder(w).Encode(schema.NetworkGetResponse{
 			Network: schema.Network{
 				ID:      1,
 				Name:    "network-1",
 				IPRange: "10.0.0.0/8",
+				Routes: []schema.NetworkRoute{
+					{
+						Destination: "10.5.0.0/24",
+						Gateway:     "10.0.0.2",
+					},
+				},
 			},
 		})
 	})
diff --git a/tests/e2e/e2e_test.go b/tests/e2e/e2e_test.go
index aece4b40..86f5a5b0 100644
--- a/tests/e2e/e2e_test.go
+++ b/tests/e2e/e2e_test.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"math/rand"
+	"net"
 	"os"
 	"strings"
 	"testing"
@@ -36,7 +37,7 @@ func TestMain(m *testing.M) {
 	os.Exit(rc)
 }
 
-func TestCloudControllerManagerPodIsPresent(t *testing.T) {
+func TestPodIsPresent(t *testing.T) {
 	t.Parallel()
 
 	t.Run("hcloud-cloud-controller-manager pod is present in kube-system", func(t *testing.T) {
@@ -68,7 +69,7 @@ func TestCloudControllerManagerPodIsPresent(t *testing.T) {
 	})
 }
 
-func TestCloudControllerManagerSetCorrectNodeLabelsAndIPAddresses(t *testing.T) {
+func TestNodeSetCorrectNodeLabelsAndIPAddresses(t *testing.T) {
 	t.Parallel()
 
 	node, err := testCluster.k8sClient.CoreV1().Nodes().Get(context.Background(), "hccm-"+testCluster.scope+"-1", metav1.GetOptions{})
@@ -113,7 +114,7 @@ func TestCloudControllerManagerSetCorrectNodeLabelsAndIPAddresses(t *testing.T)
 	}
 }
 
-func TestCloudControllerManagerLoadBalancersMinimalSetup(t *testing.T) {
+func TestServiceLoadBalancersMinimalSetup(t *testing.T) {
 	t.Parallel()
 
 	lbTest := lbTestHelper{
@@ -138,7 +139,7 @@ func TestCloudControllerManagerLoadBalancersMinimalSetup(t *testing.T) {
 	lbTest.TearDown()
 }
 
-func TestCloudControllerManagerLoadBalancersHTTPS(t *testing.T) {
+func TestServiceLoadBalancersHTTPS(t *testing.T) {
 	t.Parallel()
 
 	cert := testCluster.CreateTLSCertificate(t, "loadbalancer-https")
@@ -167,7 +168,7 @@ func TestCloudControllerManagerLoadBalancersHTTPS(t *testing.T) {
 	lbTest.TearDown()
 }
 
-func TestCloudControllerManagerLoadBalancersHTTPSWithManagedCertificate(t *testing.T) {
+func TestServiceLoadBalancersHTTPSWithManagedCertificate(t *testing.T) {
 	t.Parallel()
 
 	domainName := fmt.Sprintf("%d-ccm-test.hc-certs.de", rand.Int())
@@ -205,7 +206,7 @@ func TestCloudControllerManagerLoadBalancersHTTPSWithManagedCertificate(t *testi
 	assert.NoError(t, err)
 }
 
-func TestCloudControllerManagerLoadBalancersWithPrivateNetwork(t *testing.T) {
+func TestServiceLoadBalancersWithPrivateNetwork(t *testing.T) {
 	t.Parallel()
 
 	lbTest := lbTestHelper{t: t, K8sClient: testCluster.k8sClient, podName: "loadbalancer-private-network"}
@@ -227,7 +228,7 @@ func TestCloudControllerManagerLoadBalancersWithPrivateNetwork(t *testing.T) {
 	lbTest.TearDown()
 }
 
-func TestCloudControllerManagerNetworksPodIPsAreAccessible(t *testing.T) {
+func TestRouteNetworksPodIPsAreAccessible(t *testing.T) {
 	t.Parallel()
 
 	err := wait.Poll(1*time.Second, 2*time.Minute, func() (bool, error) {
@@ -256,3 +257,88 @@ func TestCloudControllerManagerNetworksPodIPsAreAccessible(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestRouteDeleteCorrectRoutes(t *testing.T) {
+	t.Parallel()
+
+	// This test tests for:
+	// - hccm is keeping routes that are outside of its scope (e.g. 0.0.0.0/0)
+	// - hccm is deleting routes that are inside the cluster cidr, but for which no server exists (10.254.0.0/24)
+	//
+	// Testing for no-op (keep route) is hard, but by combining the test with a deletion test we can be reasonably sure
+	// that it was not tried in the same reconcile loop.
+
+	ctx := context.Background()
+
+	testGateway := net.ParseIP("10.0.0.254")
+	_, defaultDestination, err := net.ParseCIDR("0.0.0.0/0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, outdatedDestination, err := net.ParseCIDR("10.244.254.0/24")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	network, _, err := testCluster.hcloud.Network.Get(ctx, "hccm-"+testCluster.scope)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Remove routes from previous tests
+	for _, route := range network.Routes {
+		if route.Gateway.Equal(testGateway) {
+			action, _, err := testCluster.hcloud.Network.DeleteRoute(ctx, network, hcloud.NetworkDeleteRouteOpts{Route: route})
+			if err != nil {
+				t.Fatal(err)
+			}
+			if err := hcops.WatchAction(ctx, &testCluster.hcloud.Action, action); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
+
+	action, _, err := testCluster.hcloud.Network.AddRoute(ctx, network, hcloud.NetworkAddRouteOpts{Route: hcloud.NetworkRoute{
+		Destination: defaultDestination,
+		Gateway:     testGateway,
+	}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := hcops.WatchAction(ctx, &testCluster.hcloud.Action, action); err != nil {
+		t.Fatal(err)
+	}
+
+	action, _, err = testCluster.hcloud.Network.AddRoute(ctx, network, hcloud.NetworkAddRouteOpts{Route: hcloud.NetworkRoute{
+		Destination: outdatedDestination,
+		Gateway:     testGateway,
+	}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := hcops.WatchAction(ctx, &testCluster.hcloud.Action, action); err != nil {
+		t.Fatal(err)
+	}
+
+	err = wait.Poll(1*time.Second, 2*time.Minute, func() (bool, error) {
+		network, _, err = testCluster.hcloud.Network.Get(ctx, "hccm-"+testCluster.scope)
+		if err != nil {
+			return false, err
+		}
+
+		hasDefaultRoute := false
+		for _, route := range network.Routes {
+			switch route.Destination.String() {
+			case defaultDestination.String():
+				hasDefaultRoute = true
+			case outdatedDestination.String():
+				// Route for outdated destination still exists
+				return false, nil
+			}
+		}
+		return hasDefaultRoute, nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+}