Repository: linode/linode-cloud-controller-manager (go)

Issue:
🐛 Nodes added following initial LoadBalancer creation missing Shared IP with Cilium BGP LoadBalancing
## General:
When using Cilium BGP loadbalancing, nodes that join the cluster following the initial call to EnsureLoadBalancer are not getting the shared IP address applied.

* [X] Have you removed all sensitive information, including but not limited to access keys and passwords?
* [X] Have you checked to ensure there aren't other open or closed [Pull Requests](../../pulls) for the same bug/feature/question?

----

## Bug Reporting

### Expected Behavior
Applicable new nodes should get the shared IP assigned when they join the cluster if a cilium BGP Load Balancer exists.

### Actual Behavior
New nodes are not getting the shared IP assigned.

### Steps to Reproduce the Problem

  1. Deploy CCM with cilium BGP load balancing enabled
  1. Deploy a sample deployment such as nginx
  1. Create a service of type LoadBalancer to expose the sample deployment
  1. Scale up the cluster to add a new node
  1. Observe the new node does not get the shared IP attached to it in the Linode Cloud Manager.

### Environment Specifications

#### Screenshots, Code Blocks, and Logs

#### Additional Notes

----

For general help or discussion, join the [Kubernetes Slack team](https://kubernetes.slack.com/messages/CD4B15LUR/details/) channel `#linode`. To sign up, use the [Kubernetes Slack inviter](http://slack.kubernetes.io/).

The [Linode Community](https://www.linode.com/community/questions/) is a great place to get additional support.


Pull request description:
[fix] getExistingSharedIPs should get Public IPs from the holder
### General:

* [X] Have you removed all sensitive information, including but not limited to access keys and passwords?
* [X] Have you checked to ensure there aren't other open or closed [Pull Requests](../../pulls) for the same bug/feature/question?

### Pull Request Guidelines:

1. [X] Does your submission pass tests?
1. [X] Have you added tests? 
1. [X] Are you addressing a single feature in this PR? 
1. [X] Are your commits atomic, addressing one change per commit?
1. [X] Are you following the conventions of the language? 
1. [X] Have you saved your large formatting changes for a different PR, so we can focus on your work?
1. [X] Have you explained your rationale for why this feature is needed? 
1. [X] Have you linked your PR to an [open issue](https://blog.github.com/2013-05-14-closing-issues-via-pull-requests/)

### Description

Calls to `getExistingSharedIPs` should return Public IPs instead of Shared IPs, because the IPs on the holder nanode are not returned under the Shared IP's section of the API response.

fixes #231 

Patch:
diff --git a/cloud/linode/cilium_loadbalancers.go b/cloud/linode/cilium_loadbalancers.go
index 4e74adc..478131a 100644
--- a/cloud/linode/cilium_loadbalancers.go
+++ b/cloud/linode/cilium_loadbalancers.go
@@ -96,8 +96,8 @@ func (l *loadbalancers) getExistingSharedIPs(ctx context.Context, ipHolder *lino
 	if err != nil {
 		return nil, err
 	}
-	addrs := make([]string, 0, len(ipHolderAddrs.IPv4.Shared))
-	for _, addr := range ipHolderAddrs.IPv4.Shared {
+	addrs := make([]string, 0, len(ipHolderAddrs.IPv4.Public))
+	for _, addr := range ipHolderAddrs.IPv4.Public {
 		addrs = append(addrs, addr.Address)
 	}
 	return addrs, nil


Test patch:
diff --git a/cloud/linode/cilium_loadbalancers_test.go b/cloud/linode/cilium_loadbalancers_test.go
index 9d44b46..a422624 100644
--- a/cloud/linode/cilium_loadbalancers_test.go
+++ b/cloud/linode/cilium_loadbalancers_test.go
@@ -58,6 +58,17 @@ var (
 			},
 		},
 	}
+	additionalNodes = []*v1.Node{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:   "node-5",
+				Labels: map[string]string{"cilium-bgp-peering": "true"},
+			},
+			Spec: v1.NodeSpec{
+				ProviderID: fmt.Sprintf("%s%d", providerIDPrefix, 55555),
+			},
+		},
+	}
 	publicIPv4       = net.ParseIP("45.76.101.25")
 	ipHolderInstance = linodego.Instance{
 		ID:     12345,
@@ -93,6 +104,10 @@ func TestCiliumCCMLoadBalancers(t *testing.T) {
 			name: "Delete Cilium Load Balancer",
 			f:    testEnsureCiliumLoadBalancerDeleted,
 		},
+		{
+			name: "Add node to existing Cilium Load Balancer",
+			f:    testCiliumUpdateLoadBalancerAddNode,
+		},
 	}
 	for _, tc := range testCases {
 		ctrl := gomock.NewController(t)
@@ -165,7 +180,7 @@ func testNoBGPNodeLabel(t *testing.T, mc *mocks.MockClient) {
 	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}},
+			Public: []*linodego.InstanceIP{{Address: publicIPv4.String()}, {Address: dummySharedIP}},
 		},
 	}, nil)
 	mc.EXPECT().AddInstanceIPAddress(gomock.Any(), ipHolderInstance.ID, true).Times(1).Return(&linodego.InstanceIP{Address: dummySharedIP}, nil)
@@ -226,7 +241,7 @@ func testCreateWithExistingIPHolder(t *testing.T, mc *mocks.MockClient) {
 	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}},
+			Public: []*linodego.InstanceIP{{Address: publicIPv4.String()}, {Address: dummySharedIP}},
 		},
 	}, nil)
 	mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
@@ -264,7 +279,7 @@ func testCreateWithNoExistingIPHolder(t *testing.T, mc *mocks.MockClient) {
 	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}},
+			Public: []*linodego.InstanceIP{{Address: publicIPv4.String()}, {Address: dummySharedIP}},
 		},
 	}, nil)
 	mc.EXPECT().AddInstanceIPAddress(gomock.Any(), ipHolderInstance.ID, true).Times(1).Return(&linodego.InstanceIP{Address: dummySharedIP}, nil)
@@ -311,3 +326,59 @@ func testEnsureCiliumLoadBalancerDeleted(t *testing.T, mc *mocks.MockClient) {
 		t.Fatalf("expected a nil error, got %v", err)
 	}
 }
+
+func testCiliumUpdateLoadBalancerAddNode(t *testing.T, mc *mocks.MockClient) {
+	Options.BGPNodeSelector = "cilium-bgp-peering=true"
+	svc := createTestService()
+
+	kubeClient, _ := k8sClient.NewFakeClientset()
+	ciliumClient := &fakev2alpha1.FakeCiliumV2alpha1{Fake: &kubeClient.CiliumFakeClientset.Fake}
+	addService(t, kubeClient, svc)
+	addNodes(t, kubeClient, nodes)
+	lb := &loadbalancers{mc, zone, kubeClient, ciliumClient, ciliumLBType}
+
+	filter := map[string]string{"label": fmt.Sprintf("%s-%s", ipHolderLabelPrefix, zone)}
+	rawFilter, _ := json.Marshal(filter)
+	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{
+			Public: []*linodego.InstanceIP{{Address: publicIPv4.String()}, {Address: dummySharedIP}},
+		},
+	}, nil)
+	mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
+		IPs:      []string{dummySharedIP},
+		LinodeID: 11111,
+	}).Times(1)
+	mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
+		IPs:      []string{dummySharedIP},
+		LinodeID: 22222,
+	}).Times(1)
+
+	lbStatus, err := lb.EnsureLoadBalancer(context.TODO(), "linodelb", svc, nodes)
+	if err != nil {
+		t.Fatalf("expected a nil error, got %v", err)
+	}
+	if lbStatus == nil {
+		t.Fatal("expected non-nil lbStatus")
+	}
+
+	// Now add another node to the cluster and assert that it gets the shared IP
+	mc.EXPECT().ListInstances(gomock.Any(), linodego.NewListOptions(1, string(rawFilter))).Times(1).Return([]linodego.Instance{ipHolderInstance}, nil)
+	mc.EXPECT().GetInstanceIPAddresses(gomock.Any(), ipHolderInstance.ID).Times(1).Return(&linodego.InstanceIPAddressResponse{
+		IPv4: &linodego.InstanceIPv4Response{
+			Public: []*linodego.InstanceIP{{Address: publicIPv4.String()}, {Address: dummySharedIP}},
+		},
+	}, nil)
+	mc.EXPECT().ShareIPAddresses(gomock.Any(), linodego.IPAddressesShareOptions{
+		IPs:      []string{dummySharedIP},
+		LinodeID: 55555,
+	}).Times(1)
+	addNodes(t, kubeClient, additionalNodes)
+
+	err = lb.UpdateLoadBalancer(context.TODO(), "linodelb", svc, additionalNodes)
+	if err != nil {
+		t.Fatalf("expected a nil error, got %v", err)
+	}
+}