[fix] make VLAN IPs unique across controller pod restarts (#544)

With the VLAN clusters, a in-memory map was maintained for each cluster's IPs. When the controller pod restarts, that IP map is lost, and any further cluster scaling activities cause the nodes to get overlapping IPs.

Author: tchinmai7

controller/linodemachine_controller_helpers.go

@@ -132,7 +132,10 @@ func newCreateConfig(ctx context.Context, machineScope *scope.MachineScope, logg
 
 	// if vlan is enabled, attach additional interface as eth0 to linode
 	if machineScope.LinodeCluster.Spec.Network.UseVlan {
-		iface := getVlanInterfaceConfig(machineScope, logger)
+		iface, err := getVlanInterfaceConfig(ctx, machineScope, logger)
+		if err != nil {
+			return nil, err
+		}
 		if iface != nil {
 			// add VLAN interface as first interface
 			createConfig.Interfaces = slices.Insert(createConfig.Interfaces, 0, *iface)
@@ -358,18 +361,21 @@ func getFirewallID(ctx context.Context, machineScope *scope.MachineScope, logger
 	return *linodeFirewall.Spec.FirewallID, nil
 }
 
-func getVlanInterfaceConfig(machineScope *scope.MachineScope, logger logr.Logger) *linodego.InstanceConfigInterfaceCreateOptions {
+func getVlanInterfaceConfig(ctx context.Context, machineScope *scope.MachineScope, logger logr.Logger) (*linodego.InstanceConfigInterfaceCreateOptions, error) {
 	logger = logger.WithValues("vlanName", machineScope.Cluster.Name)
 
 	// Try to obtain a IP for the machine using its name
-	ip := util.GetNextVlanIP(machineScope.Cluster.Name, machineScope.Cluster.Namespace)
-	logger.Info("obtained IP for machine", "name", machineScope.LinodeMachine.Name, "ip", ip)
+	ip, err := util.GetNextVlanIP(ctx, machineScope.Cluster.Name, machineScope.Cluster.Namespace, machineScope.Client)
+	if err != nil {
+		return nil, fmt.Errorf("getting vlanIP: %w", err)
+	}
 
+	logger.Info("obtained IP for machine", "name", machineScope.LinodeMachine.Name, "ip", ip)
 	return &linodego.InstanceConfigInterfaceCreateOptions{
 		Purpose:     linodego.InterfacePurposeVLAN,
 		Label:       machineScope.Cluster.Name,
 		IPAMAddress: fmt.Sprintf(vlanIPFormat, ip),
-	}
+	}, nil
 }
 
 func getVPCInterfaceConfig(ctx context.Context, machineScope *scope.MachineScope, interfaces []linodego.InstanceConfigInterfaceCreateOptions, logger logr.Logger) (*linodego.InstanceConfigInterfaceCreateOptions, error) {

util/vlanips.go

@@ -17,10 +17,19 @@ limitations under the License.
 package util
 
 import (
+	"context"
 	"fmt"
+	"net"
 	"net/netip"
 	"slices"
 	"sync"
+
+	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	"github.com/linode/cluster-api-provider-linode/api/v1alpha2"
 )
 
 var (
@@ -35,16 +44,52 @@ type ClusterIPs struct {
 	ips []string
 }
 
-func getClusterIPs(key string) *ClusterIPs {
+func getExistingIPsForCluster(ctx context.Context, clusterName, namespace string, kubeclient client.Client) ([]string, error) {
+	clusterReq, err := labels.NewRequirement("cluster.x-k8s.io/cluster-name", selection.Equals, []string{clusterName})
+	if err != nil {
+		return nil, fmt.Errorf("building label selector: %w", err)
+	}
+
+	selector := labels.NewSelector()
+	selector = selector.Add(*clusterReq)
+	var linodeMachineList v1alpha2.LinodeMachineList
+	err = kubeclient.List(ctx, &linodeMachineList, &client.ListOptions{Namespace: namespace, LabelSelector: selector})
+	if err != nil {
+		return nil, fmt.Errorf("listing all linodeMachines %w", err)
+	}
+
+	_, ipnet, err := net.ParseCIDR(vlanIPRange)
+	if err != nil {
+		return nil, fmt.Errorf("parsing vlanIPRange: %w", err)
+	}
+
+	existingIPs := []string{}
+	for _, lm := range linodeMachineList.Items {
+		for _, addr := range lm.Status.Addresses {
+			if addr.Type == clusterv1.MachineInternalIP && ipnet.Contains(net.ParseIP(addr.Address)) {
+				existingIPs = append(existingIPs, addr.Address)
+			}
+		}
+	}
+	return existingIPs, nil
+}
+
+func getClusterIPs(ctx context.Context, clusterName, namespace string, kubeclient client.Client) (*ClusterIPs, error) {
+	key := fmt.Sprintf("%s.%s", namespace, clusterName)
 	vlanIPsMu.Lock()
 	defer vlanIPsMu.Unlock()
-	ips, exists := vlanIPsMap[key]
+	clusterIps, exists := vlanIPsMap[key]
 	if !exists {
-		ips = &ClusterIPs{
-			ips: []string{},
+		ips, err := getExistingIPsForCluster(ctx, clusterName, namespace, kubeclient)
+		if err != nil {
+			return nil, fmt.Errorf("getting existingIPs for a cluster: %w", err)
+		}
+		clusterIps = &ClusterIPs{
+			ips: ips,
 		}
+		vlanIPsMap[key] = clusterIps
 	}
-	return ips
+	return clusterIps, nil
 }
 
 func (c *ClusterIPs) getNextIP() string {
@@ -66,10 +111,12 @@ func (c *ClusterIPs) getNextIP() string {
 }
 
 // GetNextVlanIP returns the next available IP for a cluster
-func GetNextVlanIP(clusterName, namespace string) string {
-	key := fmt.Sprintf("%s.%s", namespace, clusterName)
-	clusterIPs := getClusterIPs(key)
-	return clusterIPs.getNextIP()
+func GetNextVlanIP(ctx context.Context, clusterName, namespace string, kubeclient client.Client) (string, error) {
+	clusterIPs, err := getClusterIPs(ctx, clusterName, namespace, kubeclient)
+	if err != nil {
+		return "", err
+	}
+	return clusterIPs.getNextIP(), nil
 }
 
 func DeleteClusterIPs(clusterName, namespace string) {

util/vlanips_test.go

@@ -17,8 +17,13 @@ limitations under the License.
 package util
 
 import (
+	"context"
 	"reflect"
 	"testing"
+
+	"go.uber.org/mock/gomock"
+
+	"github.com/linode/cluster-api-provider-linode/mock"
 )
 
 func TestGetNextVlanIP(t *testing.T) {
@@ -28,27 +33,42 @@ func TestGetNextVlanIP(t *testing.T) {
 		clusterName      string
 		clusterNamespace string
 		want             string
+		expects          func(mock *mock.MockK8sClient)
 	}{
 		{
 			name:             "provide key which exists in map",
 			clusterName:      "test",
 			clusterNamespace: "testna",
 			want:             "10.0.0.3",
+			expects: func(mock *mock.MockK8sClient) {
+			},
 		},
 		{
 			name:             "provide key which doesn't exist",
 			clusterName:      "test",
 			clusterNamespace: "testnonexistent",
 			want:             "10.0.0.1",
+			expects: func(mock *mock.MockK8sClient) {
+				mock.EXPECT().List(gomock.Any(), gomock.Any(), gomock.Any()).MinTimes(1)
+			},
 		},
 	}
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	mockK8sClient := mock.NewMockK8sClient(ctrl)
+
 	for _, tt := range tests {
 		vlanIPsMap["testna.test"] = &ClusterIPs{
 			ips: []string{"10.0.0.1", "10.0.0.2"},
 		}
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			if got := GetNextVlanIP(tt.clusterName, tt.clusterNamespace); !reflect.DeepEqual(got, tt.want) {
+			tt.expects(mockK8sClient)
+			got, err := GetNextVlanIP(context.Background(), tt.clusterName, tt.clusterNamespace, mockK8sClient)
+			if err != nil {
+				t.Error("error")
+			}
+			if !reflect.DeepEqual(got, tt.want) {
 				t.Errorf("GetNextVlanIP() = %v, want %v", got, tt.want)
 			}
 		})