Minor changes as suggested in comments

Signed-off-by: Harsh Thakur <[email protected]>

cloud-controller-manager/civo/loadbalancer.go

@@ -91,7 +91,7 @@ func (l *loadbalancer) EnsureLoadBalancer(ctx context.Context, clusterName strin
 		return nil, err
 	}
 
-	// CivLB has been found
+	// CivoLB has been found
 	if err == nil {
 		ul, err := l.updateLBConfig(civolb, service, nodes)
 		if err != nil {
@@ -146,7 +146,7 @@ func (l *loadbalancer) updateLBConfig(civolb *civogo.LoadBalancer, service *v1.S
 	}
 	lbuc.Backends = backends
 
-	if ip := getReservedIP(service); ip != "" {
+	if ip := getReservedIPFromAnnotation(service); ip != "" {
 		rip, err := l.client.civoClient.FindIP(ip)
 		if err != nil {
 			klog.Errorf("Unable to find reserved IP, error: %v", err)
@@ -162,12 +162,12 @@ func (l *loadbalancer) updateLBConfig(civolb *civogo.LoadBalancer, service *v1.S
 			}
 		}
 	} else {
-		ips, err := l.client.civoClient.ListIPs()
+		ip, err := findIPWithLBID(l.client.civoClient, civolb.ID)
 		if err != nil {
-			klog.Errorf("Unable to list IPs, error: %v", err)
+			klog.Errorf("Unable to find IP with loadbalancer ID, error: %v", err)
 			return nil, err
 		}
-		ip := findIPWithLBID(ips.Items, civolb.ID)
+
 		if ip != nil {
 			_, err = l.client.civoClient.UnassignIP(ip.ID)
 			if err != nil {
@@ -187,13 +187,21 @@ func (l *loadbalancer) updateLBConfig(civolb *civogo.LoadBalancer, service *v1.S
 
 }
 
-func findIPWithLBID(ips []civogo.IP, lbID string) *civogo.IP {
-	for _, ip := range ips {
+// there's no direct way to find if the LB is using a reserved IP. This method lists all the reserved IPs in the account
+// and checks if the loadbalancer is using one of them.
+func findIPWithLBID(civo civogo.Clienter, lbID string) (*civogo.IP, error) {
+	ips, err := civo.ListIPs()
+	if err != nil {
+		klog.Errorf("Unable to list IPs, error: %v", err)
+		return nil, err
+	}
+
+	for _, ip := range ips.Items {
 		if ip.AssignedTo.ID == lbID {
-			return &ip
+			return &ip, nil
 		}
 	}
-	return nil
+	return nil, nil
 }
 
 func lbStatusFor(civolb *civogo.LoadBalancer) (*v1.LoadBalancerStatus, error) {
@@ -389,30 +397,19 @@ func getEnableProxyProtocol(service *v1.Service) string {
 
 // getAlgorithm returns the algorithm value from the service annotation.
 func getAlgorithm(service *v1.Service) string {
-	algorithm, ok := service.Annotations[annotationCivoLoadBalancerAlgorithm]
-	if !ok {
-		return ""
-	}
+	algorithm, _ := service.Annotations[annotationCivoLoadBalancerAlgorithm]
 
 	return algorithm
 }
 
-// getReservedIP returns the reservedIP value from the service annotation.
-func getReservedIP(service *v1.Service) string {
-	ip, ok := service.Annotations[annotationCivoIPv4]
-	if !ok {
-		return ""
-	}
-
+// getReservedIPFromAnnotation returns the reservedIP value from the service annotation.
+func getReservedIPFromAnnotation(service *v1.Service) string {
+	ip, _ := service.Annotations[annotationCivoIPv4]
 	return ip
 }
 
 // getFirewallID returns the firewallID value from the service annotation.
 func getFirewallID(service *v1.Service) string {
-	firewallID, ok := service.Annotations[annotationCivoFirewallID]
-	if !ok {
-		return ""
-	}
-
+	firewallID, _ := service.Annotations[annotationCivoFirewallID]
 	return firewallID
 }

e2e/loadbalancer_test.go

@@ -109,12 +109,13 @@ func TestLoadbalancerProxy(t *testing.T) {
 
 }
 
-func TestLoadbalancerPublicIP(t *testing.T) {
+func TestLoadbalancerReservedIP(t *testing.T) {
 	g := NewGomegaWithT(t)
 
 	_, err := deployMirrorPods(e2eTest.tenantClient)
 	g.Expect(err).ShouldNot(HaveOccurred())
 
+	fmt.Println("Create a reserved IP for e2e test (if it doesn't exist)")
 	ip, err := getOrCreateIP(e2eTest.civo)
 	g.Expect(err).ShouldNot(HaveOccurred())
 
@@ -124,7 +125,21 @@ func TestLoadbalancerPublicIP(t *testing.T) {
 	}, "2m", "5s").ShouldNot(BeEmpty())
 
 	fmt.Println("Creating Service")
-	svc, err := getOrCreateSvc(e2eTest.tenantClient, ip)
+	svc, err := getOrCreateSvc(e2eTest.tenantClient)
+	g.Expect(err).ShouldNot(HaveOccurred())
+
+	patchSvc := &corev1.Service{}
+	err = e2eTest.tenantClient.Get(context.TODO(), client.ObjectKeyFromObject(svc), patchSvc)
+	originalSvc := svc.DeepCopy()
+	if patchSvc.Annotations == nil {
+		patchSvc.Annotations = make(map[string]string, 0)
+	}
+	patchSvc.Annotations = map[string]string{
+		"kubernetes.civo.com/ipv4-address": ip.IP,
+	}
+
+	fmt.Println("Updating service with reserved IP annotation")
+	err = e2eTest.tenantClient.Patch(context.TODO(), patchSvc, client.MergeFrom(originalSvc))
 	g.Expect(err).ShouldNot(HaveOccurred())
 
 	g.Eventually(func() string {
@@ -141,6 +156,7 @@ func TestLoadbalancerPublicIP(t *testing.T) {
 	err = e2eTest.tenantClient.Update(context.TODO(), svc)
 	g.Expect(err).ShouldNot(HaveOccurred())
 
+	fmt.Println("Waiting for auto-assigned IP to be attached to LB")
 	g.Eventually(func() string {
 		err = e2eTest.tenantClient.Get(context.TODO(), client.ObjectKeyFromObject(svc), svc)
 		if len(svc.Status.LoadBalancer.Ingress) == 0 {
@@ -184,20 +200,17 @@ func getOrCreateIP(c *civogo.Client) (*civogo.IP, error) {
 	return ip, err
 }
 
-func getOrCreateSvc(c client.Client, ip *civogo.IP) (*corev1.Service, error) {
+func getOrCreateSvc(c client.Client) (*corev1.Service, error) {
 	svc := &corev1.Service{}
 	err := c.Get(context.TODO(), client.ObjectKey{Name: "echo-pods", Namespace: "default"}, svc)
 	if err != nil && errors.IsNotFound(err) {
 		lbls := map[string]string{"app": "mirror-pod"}
 		// Create a service of type: LoadBalancer
-		fmt.Println("Creating Service with IP: ", ip.IP)
 		svc = &corev1.Service{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:      "echo-pods",
-				Namespace: "default",
-				Annotations: map[string]string{
-					"kubernetes.civo.com/ipv4-address": ip.IP,
-				},
+				Name:        "echo-pods",
+				Namespace:   "default",
+				Annotations: map[string]string{},
 			},
 			Spec: corev1.ServiceSpec{
 				Ports: []corev1.ServicePort{