Merge pull request #110 from leaseweb/fix/ensure_host_assoc

Fix/Ensure host assocation with LB rules in EnsureLoadBalancer

Author: hrak

.golangci.yml

@@ -0,0 +1,65 @@
+linters-settings:
+  gci:
+    sections:
+      - standard
+      - default
+      - prefix(github.com/apache/cloudstack-kubernetes-provider)
+  gofumpt:
+    module-path: "github.com/apache/cloudstack-kubernetes-provider"
+  nolintlint:
+    # https://github.com/golangci/golangci-lint/issues/3228
+    allow-unused: true
+
+linters:
+  disable:
+    - goheader
+    - depguard
+    - wrapcheck
+    - mnd
+    - varnamelen
+    - testpackage
+    - tagliatelle
+    - paralleltest
+    - ireturn
+    - err113
+    - gochecknoglobals
+    - wsl
+    - exhaustive
+    - nosprintfhostport
+    - nonamedreturns
+    - interfacebloat
+    - exhaustruct
+    - lll
+    - gosec
+    - gomoddirectives
+    - godox
+    - gochecknoinits
+    - funlen
+    - dupl
+    - cyclop
+    - inamedparam
+    # deprecated linters
+    - tenv
+  enable-all: true
+
+issues:
+  # Maximum issues count per one linter. Set to 0 to disable. Default is 50.
+  max-issues-per-linter: 0
+  # Maximum count of issues with the same text. Set to 0 to disable. Default is 3.
+  max-same-issues: 0
+  exclude-rules:
+    - path: (.+)_test.go$
+      linters:
+        - gocognit
+        - goconst
+        - gocyclo
+        - errcheck
+        - dupl
+        - gosec
+        - forbidigo
+        - maintidx
+
+run:
+  timeout: 10m
+  allow-parallel-runners: true
+

Makefile

@@ -40,7 +40,7 @@ BIN_DIR ?= bin
 GO_INSTALL := ./hack/go_install.sh
 
 GOLANGCI_LINT_BIN := golangci-lint
-GOLANGCI_LINT_VER := v1.63.4
+GOLANGCI_LINT_VER := v1.64.8
 GOLANGCI_LINT := $(abspath $(TOOLS_BIN_DIR)/$(GOLANGCI_LINT_BIN)-$(GOLANGCI_LINT_VER))
 GOLANGCI_LINT_PKG := github.com/golangci/golangci-lint/cmd/golangci-lint
 

cloudstack/cloudstack_instances_test.go

@@ -1,7 +1,6 @@
 package cloudstack
 
 import (
-	"context"
 	"errors"
 	"testing"
 
@@ -94,8 +93,10 @@ func TestInstanceExists(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
+	ctx := t.Context()
+
 	cs := cloudstack.NewMockClient(mockCtrl)
-	ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) //nolint: forcetypeassert
+	ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) //nolint:forcetypeassert
 
 	fakeInstances := &CSCloud{
 		client: cs,
@@ -142,13 +143,13 @@ func TestInstanceExists(t *testing.T) {
 				if test.node.Name == "testDummyVM" {
 					ms.EXPECT().GetVirtualMachineByName("testDummyVM", gomock.Any()).Return(test.mockedCSOutput, 1, nil)
 				} else {
-					ms.EXPECT().GetVirtualMachineByName("nonExistingVM", gomock.Any()).Return(test.mockedCSOutput, 0, errors.New("No match found for ...")) //nolint: revive
+					ms.EXPECT().GetVirtualMachineByName("nonExistingVM", gomock.Any()).Return(test.mockedCSOutput, 0, errors.New("No match found for ...")) //nolint:revive
 				}
 			} else {
 				ms.EXPECT().GetVirtualMachineByID("915653c4-298b-4d74-bdee-4ced282114f1", gomock.Any()).Return(test.mockedCSOutput, 1, nil)
 			}
 
-			exists, err := fakeInstances.InstanceExists(context.TODO(), test.node)
+			exists, err := fakeInstances.InstanceExists(ctx, test.node)
 			if err != nil {
 				t.Errorf("InstanceExists failed with node %v: %v", nodeName, err)
 			}
@@ -164,8 +165,10 @@ func TestInstanceShutdown(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
+	ctx := t.Context()
+
 	cs := cloudstack.NewMockClient(mockCtrl)
-	ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) //nolint: forcetypeassert
+	ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) //nolint:forcetypeassert
 
 	fakeInstances := &CSCloud{
 		client: cs,
@@ -225,7 +228,7 @@ func TestInstanceShutdown(t *testing.T) {
 				ms.EXPECT().GetVirtualMachineByID("915653c4-298b-4d74-bdee-4ced282114f1", gomock.Any()).Return(test.mockedCSOutput, 1, nil)
 			}
 
-			shutdown, err := fakeInstances.InstanceShutdown(context.TODO(), test.node)
+			shutdown, err := fakeInstances.InstanceShutdown(ctx, test.node)
 			if err != nil {
 				t.Logf("InstanceShutdown failed with node %v: %v", nodeName, err)
 			}
@@ -241,8 +244,10 @@ func TestInstanceMetadata(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	defer mockCtrl.Finish()
 
+	ctx := t.Context()
+
 	cs := cloudstack.NewMockClient(mockCtrl)
-	ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) //nolint: forcetypeassert
+	ms := cs.VirtualMachine.(*cloudstack.MockVirtualMachineServiceIface) //nolint:forcetypeassert
 
 	fakeInstances := &CSCloud{
 		client: cs,
@@ -374,7 +379,7 @@ func TestInstanceMetadata(t *testing.T) {
 				ms.EXPECT().GetVirtualMachineByID("915653c4-298b-4d74-bdee-4ced282114f1", gomock.Any()).Return(test.mockedCSOutput, 1, nil)
 			}
 
-			metadata, err := fakeInstances.InstanceMetadata(context.TODO(), test.node)
+			metadata, err := fakeInstances.InstanceMetadata(ctx, test.node)
 			if err != nil {
 				t.Logf("InstanceMetadata failed with node %v: %v", nodeName, err)
 			}

cloudstack/cloudstack_loadbalancer.go

@@ -96,7 +96,7 @@ func (cs *CSCloud) GetLoadBalancer(ctx context.Context, clusterName string, serv
 }
 
 // EnsureLoadBalancer creates a new load balancer, or updates the existing one. Returns the status of the balancer.
-func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, service *corev1.Service, nodes []*corev1.Node) (status *corev1.LoadBalancerStatus, err error) { //nolint:gocognit,gocyclo,nestif
+func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, service *corev1.Service, nodes []*corev1.Node) (status *corev1.LoadBalancerStatus, err error) { //nolint:gocognit,gocyclo,nestif,maintidx
 	klog.V(4).InfoS("EnsureLoadBalancer", "cluster", clusterName, "service", klog.KObj(service))
 	serviceName := fmt.Sprintf("%s/%s", service.Namespace, service.Name)
 
@@ -141,7 +141,6 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s
 		msg := fmt.Sprintf("Created new load balancer for service %s with algorithm '%s' and IP address %s", serviceName, lb.algorithm, lb.ipAddr)
 		cs.eventRecorder.Event(service, corev1.EventTypeNormal, "CreatedLoadBalancer", msg)
 		klog.Info(msg)
-
 	} else if service.Spec.LoadBalancerIP != "" && service.Spec.LoadBalancerIP != lb.ipAddr {
 		// LoadBalancerIP was specified and it's different from the current IP
 		// Release the old IP first
@@ -161,8 +160,10 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s
 				// CloudStack sometimes reports deletes as "invalid value" when the entity is already gone.
 				if strings.Contains(delErr.Error(), "does not exist") || strings.Contains(delErr.Error(), "Invalid parameter id value") {
 					klog.V(4).Infof("Load balancer rule %s already removed, continuing: %v", oldRule.Name, delErr)
+
 					continue
 				}
+
 				return nil, delErr
 			}
 		}
@@ -172,11 +173,13 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s
 
 		if err := lb.releaseLoadBalancerIP(); err != nil {
 			klog.Errorf("attempt to release old load balancer IP failed: %s", err.Error())
+
 			return nil, fmt.Errorf("failed to release old load balancer IP: %w", err)
 		}
 
 		if err := lb.getLoadBalancerIP(service.Spec.LoadBalancerIP); err != nil {
 			klog.Errorf("failed to allocate specified IP %v: %v", service.Spec.LoadBalancerIP, err)
+
 			return nil, fmt.Errorf("failed to allocate specified load balancer IP: %w", err)
 		}
 
@@ -211,13 +214,16 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s
 				if err := lb.updateLoadBalancerRule(lbRuleName, protocol); err != nil {
 					return nil, err
 				}
-				// Delete the rule from the map, to prevent it being deleted.
-				delete(lb.rules, lbRuleName)
 			} else {
 				klog.V(4).Infof("Load balancer rule %v is up-to-date", lbRuleName)
-				// Delete the rule from the map, to prevent it being deleted.
-				delete(lb.rules, lbRuleName)
 			}
+
+			if err := lb.reconcileHostsForRule(lbRule, lb.hostIDs); err != nil {
+				return nil, err
+			}
+
+			// Delete the rule from the map, to prevent it being deleted.
+			delete(lb.rules, lbRuleName)
 		} else {
 			klog.V(4).Infof("Creating load balancer rule: %v", lbRuleName)
 			lbRule, err = lb.createLoadBalancerRule(lbRuleName, port, protocol)
@@ -301,34 +307,8 @@ func (cs *CSCloud) UpdateLoadBalancer(ctx context.Context, clusterName string, s
 	}
 
 	for _, lbRule := range lb.rules {
-		p := lb.LoadBalancer.NewListLoadBalancerRuleInstancesParams(lbRule.Id)
-
-		// Retrieve all VMs currently associated to this load balancer rule.
-		l, err := lb.LoadBalancer.ListLoadBalancerRuleInstances(p)
-		if err != nil {
-			return fmt.Errorf("error retrieving associated instances: %w", err)
-		}
-
-		assign, remove := symmetricDifference(lb.hostIDs, l.LoadBalancerRuleInstances)
-
-		klog.V(4).Infof("Load balancer rule %v: %d host(s) to assign, %d host(s) to remove (wanted: %v, current: %d instances)",
-			lbRule.Name, len(assign), len(remove), lb.hostIDs, len(l.LoadBalancerRuleInstances))
-
-		// IMPORTANT: Assign new hosts BEFORE removing old ones to ensure the load balancer
-		// always has backends during rolling upgrades. If assignment fails, we abort without
-		// removing old hosts so traffic can still be served.
-		if len(assign) > 0 {
-			klog.V(4).Infof("Assigning new hosts (%v) to load balancer rule: %v", assign, lbRule.Name)
-			if err := lb.assignHostsToRule(lbRule, assign); err != nil {
-				return fmt.Errorf("error assigning new hosts to rule %v (old hosts preserved): %w", lbRule.Name, err)
-			}
-		}
-
-		if len(remove) > 0 {
-			klog.V(4).Infof("Removing old hosts (%v) from load balancer rule: %v", remove, lbRule.Name)
-			if err := lb.removeHostsFromRule(lbRule, remove); err != nil {
-				return err
-			}
+		if err := lb.reconcileHostsForRule(lbRule, lb.hostIDs); err != nil {
+			return err
 		}
 	}
 
@@ -362,6 +342,7 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName st
 	// If no rules exist, the load balancer doesn't exist - nothing to delete
 	if len(lb.rules) == 0 {
 		klog.V(4).Infof("No load balancer rules found for service, nothing to delete")
+
 		return nil
 	}
 
@@ -413,16 +394,17 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName st
 	}
 
 	// Delete the public IP address if appropriate
-	if lb.ipAddr != "" {
+	if lb.ipAddr != "" { //nolint:nestif
 		klog.V(4).Infof("Processing public IP deletion for load balancer: IP=%v, ID=%v", lb.ipAddr, lb.ipAddrID)
 
 		// Check if we should release the IP
 		shouldReleaseIP, err := cs.shouldReleaseLoadBalancerIP(lb, service)
-		if err != nil {
+		switch {
+		case err != nil:
 			err := fmt.Errorf("error determining if IP should be released: %w", err)
 			klog.Errorf("%v", err)
 			deletionErrors = append(deletionErrors, err)
-		} else if shouldReleaseIP {
+		case shouldReleaseIP:
 			klog.V(4).Infof("Releasing load balancer IP: %v", lb.ipAddr)
 			if err := lb.releaseLoadBalancerIP(); err != nil {
 				err := fmt.Errorf("error releasing load balancer IP %v: %w", lb.ipAddr, err)
@@ -433,7 +415,7 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName st
 				cs.eventRecorder.Event(service, corev1.EventTypeNormal, "ReleasedLoadBalancerIP", msg)
 				klog.Info(msg)
 			}
-		} else {
+		default:
 			klog.V(4).Infof("Load balancer IP %v is in use by other services, keeping it allocated", lb.ipAddr)
 		}
 	}
@@ -443,23 +425,25 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName st
 		msg := fmt.Sprintf("Encountered %d error(s) while deleting load balancer for service %s", len(deletionErrors), serviceName)
 		klog.Warningf("%s: %v", msg, deletionErrors)
 		cs.eventRecorder.Event(service, corev1.EventTypeWarning, "DeletingLoadBalancerFailed", msg)
+
 		// Return the first error or a combined error message
-		return fmt.Errorf("load balancer deletion completed with errors: %v", deletionErrors[0])
+		return fmt.Errorf("load balancer deletion completed with errors: %w", deletionErrors[0])
 	}
 
-	msg := fmt.Sprintf("Successfully deleted load balancer for service %s", serviceName)
+	msg := "Successfully deleted load balancer for service " + serviceName
 	cs.eventRecorder.Event(service, corev1.EventTypeNormal, "DeletedLoadBalancer", msg)
 	klog.Info(msg)
 
 	return nil
 }
 
-// shouldReleaseLoadBalancerIP determines whether the public IP should be released
+// shouldReleaseLoadBalancerIP determines whether the public IP should be released.
 func (cs *CSCloud) shouldReleaseLoadBalancerIP(lb *loadBalancer, service *corev1.Service) (bool, error) {
 	// If the IP was explicitly specified in the service spec, don't release it
 	// The user is responsible for managing the lifecycle of user-provided IPs
 	if service.Spec.LoadBalancerIP != "" && service.Spec.LoadBalancerIP == lb.ipAddr {
 		klog.V(4).Infof("IP %v was explicitly specified in service spec, not releasing", lb.ipAddr)
+
 		return false, nil
 	}
 
@@ -479,11 +463,13 @@ func (cs *CSCloud) shouldReleaseLoadBalancerIP(lb *loadBalancer, service *corev1
 	// If other rules exist, this IP is in use by other services
 	if otherRules.Count > 0 {
 		klog.V(4).Infof("IP %v has %d other load balancer rule(s) in use, not releasing", lb.ipAddr, otherRules.Count)
+
 		return false, nil
 	}
 
 	// IP is safe to release - it's either controller-allocated or no longer in use
 	klog.V(4).Infof("IP %v is no longer in use and safe to release", lb.ipAddr)
+
 	return true, nil
 }
 
@@ -855,6 +841,39 @@ func (lb *loadBalancer) deleteLoadBalancerRule(lbRule *cloudstack.LoadBalancerRu
 	return nil
 }
 
+// reconcileHostsForRule ensures the load balancer rule has exactly the expected set of hosts.
+// It lists the current members, computes the difference, and assigns new hosts before removing
+// old ones so the rule always has backends during rolling upgrades.
+func (lb *loadBalancer) reconcileHostsForRule(lbRule *cloudstack.LoadBalancerRule, hostIDs []string) error {
+	p := lb.LoadBalancer.NewListLoadBalancerRuleInstancesParams(lbRule.Id)
+
+	l, err := lb.LoadBalancer.ListLoadBalancerRuleInstances(p)
+	if err != nil {
+		return fmt.Errorf("error retrieving associated instances: %w", err)
+	}
+
+	assign, remove := symmetricDifference(hostIDs, l.LoadBalancerRuleInstances)
+
+	klog.V(4).Infof("Reconcile hosts for rule %v: %d host(s) to assign, %d host(s) to remove (wanted: %v, current: %d instances)",
+		lbRule.Name, len(assign), len(remove), hostIDs, len(l.LoadBalancerRuleInstances))
+
+	if len(assign) > 0 {
+		klog.V(4).Infof("Assigning new hosts (%v) to load balancer rule: %v", assign, lbRule.Name)
+		if err := lb.assignHostsToRule(lbRule, assign); err != nil {
+			return fmt.Errorf("error assigning new hosts to rule %v (old hosts preserved): %w", lbRule.Name, err)
+		}
+	}
+
+	if len(remove) > 0 {
+		klog.V(4).Infof("Removing old hosts (%v) from load balancer rule: %v", remove, lbRule.Name)
+		if err := lb.removeHostsFromRule(lbRule, remove); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
 // assignHostsToRule assigns hosts to a load balancer rule.
 func (lb *loadBalancer) assignHostsToRule(lbRule *cloudstack.LoadBalancerRule, hostIDs []string) error {
 	p := lb.LoadBalancer.NewAssignToLoadBalancerRuleParams(lbRule.Id)