Merge pull request #215 from linode/ip-holder-ip-check

[fix] don't try to share IPs that may have been removed on the ip-holder outside of the CCM logic

cloud/linode/cilium_loadbalancers.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"slices"
 	"strings"
 
 	"github.com/cilium/cilium/pkg/k8s/apis/cilium.io/v2alpha1"
@@ -65,10 +66,10 @@ var (
 	errNoBGPSelector = errors.New("no BGP node selector set to configure IP sharing")
 )
 
-// getExistingSharedIPs determines the list of addresses to share on nodes by checking the
+// getExistingSharedIPsInCluster determines the list of addresses to share on nodes by checking the
 // CiliumLoadBalancerIPPools created by the CCM in createCiliumLBIPPool
 // NOTE: Cilium CRDs must be installed for this to work
-func (l *loadbalancers) getExistingSharedIPs(ctx context.Context) ([]string, error) {
+func (l *loadbalancers) getExistingSharedIPsInCluster(ctx context.Context) ([]string, error) {
 	addrs := []string{}
 	if err := l.retrieveCiliumClientset(); err != nil {
 		return addrs, err
@@ -87,6 +88,21 @@ func (l *loadbalancers) getExistingSharedIPs(ctx context.Context) ([]string, err
 	return addrs, nil
 }
 
+func (l *loadbalancers) getExistingSharedIPs(ctx context.Context, ipHolder *linodego.Instance) ([]string, error) {
+	if ipHolder == nil {
+		return nil, nil
+	}
+	ipHolderAddrs, err := l.client.GetInstanceIPAddresses(ctx, ipHolder.ID)
+	if err != nil {
+		return nil, err
+	}
+	addrs := make([]string, 0, len(ipHolderAddrs.IPv4.Shared))
+	for _, addr := range ipHolderAddrs.IPv4.Shared {
+		addrs = append(addrs, addr.Address)
+	}
+	return addrs, nil
+}
+
 // shareIPs shares the given list of IP addresses on the given Node
 func (l *loadbalancers) shareIPs(ctx context.Context, addrs []string, node *v1.Node) error {
 	nodeLinodeID, err := parseProviderID(node.Spec.ProviderID)
@@ -150,11 +166,29 @@ func (l *loadbalancers) handleIPSharing(ctx context.Context, node *v1.Node) erro
 	}
 	// Get the IPs to be shared on the Node and configure sharing.
 	// This also annotates the node that IPs have been shared.
-	addrs, err := l.getExistingSharedIPs(ctx)
+	inClusterAddrs, err := l.getExistingSharedIPsInCluster(ctx)
+	if err != nil {
+		klog.Infof("error getting shared IPs in cluster: %s", err.Error())
+		return err
+	}
+	// if any of the addrs don't exist on the ip-holder (e.g. someone manually deleted it outside the CCM),
+	// we need to exclude that from the list
+	// TODO: also clean up the CiliumLoadBalancerIPPool for that missing IP if that happens
+	ipHolder, err := l.getIPHolder(ctx)
 	if err != nil {
-		klog.Infof("error getting shared IPs: %s", err.Error())
 		return err
 	}
+	ipHolderAddrs, err := l.getExistingSharedIPs(ctx, ipHolder)
+	if err != nil {
+		klog.Infof("error getting shared IPs in cluster: %s", err.Error())
+		return err
+	}
+	addrs := []string{}
+	for _, i := range inClusterAddrs {
+		if slices.Contains(ipHolderAddrs, i) {
+			addrs = append(addrs, i)
+		}
+	}
 	if err = l.shareIPs(ctx, addrs, node); err != nil {
 		klog.Infof("error sharing IPs: %s", err.Error())
 		return err
@@ -182,11 +216,25 @@ func (l *loadbalancers) createSharedIP(ctx context.Context, nodes []*v1.Node) (s
 
 	// need to retrieve existing public IPs on the IP holder since ShareIPAddresses
 	// expects the full list of IPs to be shared
-	addrs, err := l.getExistingSharedIPs(ctx)
+	inClusterAddrs, err := l.getExistingSharedIPsInCluster(ctx)
 	if err != nil {
 		return "", err
 	}
-	addrs = append(addrs, newSharedIP.Address)
+	inClusterAddrs = append(inClusterAddrs, newSharedIP.Address)
+	// if any of the addrs don't exist on the ip-holder (e.g. someone manually deleted it outside the CCM),
+	// we need to exclude that from the list
+	// TODO: also clean up the CiliumLoadBalancerIPPool for that missing IP if that happens
+	ipHolderAddrs, err := l.getExistingSharedIPs(ctx, ipHolder)
+	if err != nil {
+		klog.Infof("error getting shared IPs in cluster: %s", err.Error())
+		return "", err
+	}
+	addrs := []string{}
+	for _, i := range inClusterAddrs {
+		if slices.Contains(ipHolderAddrs, i) {
+			addrs = append(addrs, i)
+		}
+	}
 
 	// share the IPs with nodes participating in Cilium BGP peering
 	kv := strings.Split(Options.BGPNodeSelector, "=")

cloud/linode/cilium_loadbalancers_test.go

@@ -188,6 +188,11 @@ func testCreateWithExistingIPHolder(t *testing.T, mc *mocks.MockClient) {
 	mc.EXPECT().ListInstances(gomock.Any(), linodego.NewListOptions(1, string(rawFilter))).Times(1).Return([]linodego.Instance{ipHolderInstance}, nil)
 	dummySharedIP := "45.76.101.26"
 	mc.EXPECT().AddInstanceIPAddress(gomock.Any(), ipHolderInstance.ID, true).Times(1).Return(&linodego.InstanceIP{Address: dummySharedIP}, nil)
+	mc.EXPECT().GetInstanceIPAddresses(gomock.Any(), ipHolderInstance.ID).Times(1).Return(&linodego.InstanceIPAddressResponse{
+		IPv4: &linodego.InstanceIPv4Response{
+			Shared: []*linodego.InstanceIP{{Address: dummySharedIP}},
+		},
+	}, nil)
 	mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
 		IPs:      []string{dummySharedIP},
 		LinodeID: 11111,
@@ -221,6 +226,11 @@ func testCreateWithNoExistingIPHolder(t *testing.T, mc *mocks.MockClient) {
 	mc.EXPECT().ListInstances(gomock.Any(), linodego.NewListOptions(1, string(rawFilter))).Times(1).Return([]linodego.Instance{}, nil)
 	dummySharedIP := "45.76.101.26"
 	mc.EXPECT().CreateInstance(gomock.Any(), gomock.Any()).Times(1).Return(&ipHolderInstance, nil)
+	mc.EXPECT().GetInstanceIPAddresses(gomock.Any(), ipHolderInstance.ID).Times(1).Return(&linodego.InstanceIPAddressResponse{
+		IPv4: &linodego.InstanceIPv4Response{
+			Shared: []*linodego.InstanceIP{{Address: dummySharedIP}},
+		},
+	}, nil)
 	mc.EXPECT().AddInstanceIPAddress(gomock.Any(), ipHolderInstance.ID, true).Times(1).Return(&linodego.InstanceIP{Address: dummySharedIP}, nil)
 	mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
 		IPs:      []string{dummySharedIP},