From 07b22ef24ffda13398d839cac63f6e97342ddd2f Mon Sep 17 00:00:00 2001 From: Tarun Chinmai Sekar Date: Thu, 17 Oct 2024 15:45:05 -0700 Subject: [PATCH 1/4] fixes for repeated ips --- .../linodemachine_controller_helpers.go | 16 +++-- util/vlanips.go | 66 ++++++++++++++++--- util/vlanips_test.go | 22 ++++++- 3 files changed, 88 insertions(+), 16 deletions(-) diff --git a/controller/linodemachine_controller_helpers.go b/controller/linodemachine_controller_helpers.go index 82c26d84..eb9d924f 100644 --- a/controller/linodemachine_controller_helpers.go +++ b/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) { diff --git a/util/vlanips.go b/util/vlanips.go index 44a300c9..ca1578f7 100644 --- a/util/vlanips.go +++ b/util/vlanips.go @@ -17,10 +17,18 @@ limitations under the License. package util import ( + "context" "fmt" + "net" "net/netip" "slices" "sync" + + "github.com/linode/cluster-api-provider-linode/api/v1alpha2" + "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" ) var ( @@ -35,16 +43,54 @@ 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 { + if 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 +112,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) { diff --git a/util/vlanips_test.go b/util/vlanips_test.go index b296edcb..cbc78bfe 100644 --- a/util/vlanips_test.go +++ b/util/vlanips_test.go @@ -17,8 +17,12 @@ limitations under the License. package util import ( + "context" "reflect" "testing" + + "github.com/linode/cluster-api-provider-linode/mock" + "go.uber.org/mock/gomock" ) func TestGetNextVlanIP(t *testing.T) { @@ -28,27 +32,41 @@ 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) } }) From a0ba2cb845c1b863476944ae3c3e2e6afe5bb108 Mon Sep 17 00:00:00 2001 From: Tarun Chinmai Sekar Date: Thu, 17 Oct 2024 15:53:13 -0700 Subject: [PATCH 2/4] parallel --- util/vlanips_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/util/vlanips_test.go b/util/vlanips_test.go index cbc78bfe..b7548c4e 100644 --- a/util/vlanips_test.go +++ b/util/vlanips_test.go @@ -61,6 +61,7 @@ func TestGetNextVlanIP(t *testing.T) { ips: []string{"10.0.0.1", "10.0.0.2"}, } t.Run(tt.name, func(t *testing.T) { + t.Parallel() tt.expects(mockK8sClient) got, err := GetNextVlanIP(context.Background(), tt.clusterName, tt.clusterNamespace, mockK8sClient) if err != nil { From 9a349325047fe69b70c59f07ede21129e08f9fa3 Mon Sep 17 00:00:00 2001 From: Tarun Chinmai Sekar Date: Mon, 21 Oct 2024 12:56:27 -0700 Subject: [PATCH 3/4] fix from review --- util/vlanips.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/util/vlanips.go b/util/vlanips.go index ca1578f7..65dca305 100644 --- a/util/vlanips.go +++ b/util/vlanips.go @@ -65,10 +65,8 @@ func getExistingIPsForCluster(ctx context.Context, clusterName, namespace string existingIPs := []string{} for _, lm := range linodeMachineList.Items { for _, addr := range lm.Status.Addresses { - if addr.Type == clusterv1.MachineInternalIP { - if ipnet.Contains(net.ParseIP(addr.Address)) { - existingIPs = append(existingIPs, addr.Address) - } + if addr.Type == clusterv1.MachineInternalIP && ipnet.Contains(net.ParseIP(addr.Address)) { + existingIPs = append(existingIPs, addr.Address) } } } From 1d817029cdead445db72ebf4955a7ad0cff8fb29 Mon Sep 17 00:00:00 2001 From: Tarun Chinmai Sekar Date: Mon, 21 Oct 2024 13:25:19 -0700 Subject: [PATCH 4/4] lint fix --- util/vlanips.go | 3 ++- util/vlanips_test.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/util/vlanips.go b/util/vlanips.go index 65dca305..28d9c78e 100644 --- a/util/vlanips.go +++ b/util/vlanips.go @@ -24,11 +24,12 @@ import ( "slices" "sync" - "github.com/linode/cluster-api-provider-linode/api/v1alpha2" "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 ( diff --git a/util/vlanips_test.go b/util/vlanips_test.go index b7548c4e..6382dd40 100644 --- a/util/vlanips_test.go +++ b/util/vlanips_test.go @@ -21,8 +21,9 @@ import ( "reflect" "testing" - "github.com/linode/cluster-api-provider-linode/mock" "go.uber.org/mock/gomock" + + "github.com/linode/cluster-api-provider-linode/mock" ) func TestGetNextVlanIP(t *testing.T) {