Add support to update the loadbalancer rule when source cidr list is updated

Author: vishesh92

cloudstack.go

@@ -25,8 +25,10 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"strings"
 
 	"github.com/apache/cloudstack-go/v2/cloudstack"
+	"github.com/blang/semver/v4"
 	"gopkg.in/gcfg.v1"
 	"k8s.io/apimachinery/pkg/types"
 	cloudprovider "k8s.io/cloud-provider"
@@ -53,6 +55,7 @@ type CSCloud struct {
 	client    *cloudstack.CloudStackClient
 	projectID string // If non-"", all resources will be created within this project
 	zone      string
+	version   semver.Version
 }
 
 func init() {
@@ -85,6 +88,7 @@ func newCSCloud(cfg *CSConfig) (*CSCloud, error) {
 	cs := &CSCloud{
 		projectID: cfg.Global.ProjectID,
 		zone:      cfg.Global.Zone,
+		version:   semver.Version{},
 	}
 
 	if cfg.Global.APIURL != "" && cfg.Global.APIKey != "" && cfg.Global.SecretKey != "" {
@@ -95,9 +99,32 @@ func newCSCloud(cfg *CSConfig) (*CSCloud, error) {
 		return nil, errors.New("no cloud provider config given")
 	}
 
+	version, err := cs.getManagementServerVersion()
+	if err != nil {
+		return nil, err
+	}
+	cs.version = version
+
 	return cs, nil
 }
 
+func (cs *CSCloud) getManagementServerVersion() (semver.Version, error) {
+	msServersResp, err := cs.client.Management.ListManagementServersMetrics(cs.client.Management.NewListManagementServersMetricsParams())
+	if err != nil {
+		return semver.Version{}, err
+	}
+	if msServersResp.Count == 0 {
+		return semver.Version{}, errors.New("no management servers found")
+	}
+	version := msServersResp.ManagementServersMetrics[0].Version
+	v, err := semver.ParseTolerant(strings.Join(strings.Split(version, ".")[0:3], "."))
+	if err != nil {
+		klog.Errorf("failed to parse management server version: %v", err)
+		return semver.Version{}, err
+	}
+	return v, nil
+}
+
 // Initialize passes a Kubernetes clientBuilder interface to the cloud provider
 func (cs *CSCloud) Initialize(clientBuilder cloudprovider.ControllerClientBuilder, stop <-chan struct{}) {
 }

cloudstack_loadbalancer.go

@@ -27,6 +27,7 @@ import (
 	"strings"
 
 	"github.com/apache/cloudstack-go/v2/cloudstack"
+	"github.com/blang/semver/v4"
 	"k8s.io/klog/v2"
 
 	corev1 "k8s.io/api/core/v1"
@@ -44,7 +45,12 @@ const (
 	// CloudStack >= 4.6 is required for it to work.
 	ServiceAnnotationLoadBalancerProxyProtocol        = "service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol"
 	ServiceAnnotationLoadBalancerLoadbalancerHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname"
-	ServiceAnnotationLoadBalancerSourceCidrs          = "service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs"
+
+	// ServiceAnnotationLoadBalancerSourceCidrs is the annotation used on the
+	// service to specify the source CIDR list for a CloudStack load balancer.
+	// The CIDR list is a comma-separated list of CIDR ranges (e.g., "10.0.0.0/8,192.168.1.0/24").
+	// If not specified, the default is to allow all sources ("0.0.0.0/0").
+	ServiceAnnotationLoadBalancerSourceCidrs = "service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs"
 )
 
 type loadBalancer struct {
@@ -143,15 +149,15 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s
 		lbRuleName := fmt.Sprintf("%s-%s-%d", lb.name, protocol, port.Port)
 
 		// If the load balancer rule exists and is up-to-date, we move on to the next rule.
-		lbRule, needsUpdate, err := lb.checkLoadBalancerRule(lbRuleName, port, protocol)
+		lbRule, needsUpdate, err := lb.checkLoadBalancerRule(lbRuleName, port, protocol, service, cs.version)
 		if err != nil {
 			return nil, err
 		}
 
 		if lbRule != nil {
 			if needsUpdate {
 				klog.V(4).Infof("Updating load balancer rule: %v", lbRuleName)
-				if err := lb.updateLoadBalancerRule(lbRuleName, protocol); err != nil {
+				if err := lb.updateLoadBalancerRule(lbRuleName, protocol, service, cs.version); err != nil {
 					return nil, err
 				}
 				// Delete the rule from the map, to prevent it being deleted.
@@ -561,37 +567,111 @@ func (lb *loadBalancer) releaseLoadBalancerIP() error {
 	return nil
 }
 
+func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error) {
+	sourceCIDRs := getStringFromServiceAnnotation(service, ServiceAnnotationLoadBalancerSourceCidrs, defaultAllowedCIDR)
+	var cidrList []string
+	if sourceCIDRs != "" {
+		cidrList = strings.Split(sourceCIDRs, ",")
+		for i, cidr := range cidrList {
+			cidr = strings.TrimSpace(cidr)
+			if _, _, err := net.ParseCIDR(cidr); err != nil {
+				return nil, fmt.Errorf("invalid CIDR %s in annotation %s: %w", cidr, ServiceAnnotationLoadBalancerSourceCidrs, err)
+			}
+			cidrList[i] = cidr
+		}
+	}
+	return cidrList, nil
+}
+
 // checkLoadBalancerRule checks if the rule already exists and if it does, if it can be updated. If
 // it does exist but cannot be updated, it will delete the existing rule so it can be created again.
-func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol) (*cloudstack.LoadBalancerRule, bool, error) {
+func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) {
 	lbRule, ok := lb.rules[lbRuleName]
 	if !ok {
 		return nil, false, nil
 	}
 
-	// Check if any of the values we cannot update (those that require a new load balancer rule) are changed.
-	if lbRule.Publicip == lb.ipAddr && lbRule.Privateport == strconv.Itoa(int(port.NodePort)) && lbRule.Publicport == strconv.Itoa(int(port.Port)) {
-		updateAlgo := lbRule.Algorithm != lb.algorithm
-		updateProto := lbRule.Protocol != protocol.CSProtocol()
-		return lbRule, updateAlgo || updateProto, nil
+	cidrList, err := lb.getCIDRList(service)
+	if err != nil {
+		return nil, false, err
 	}
 
-	// Delete the load balancer rule so we can create a new one using the new values.
-	if err := lb.deleteLoadBalancerRule(lbRule); err != nil {
-		return nil, false, err
+	var lbRuleCidrList []string
+	if lbRule.Cidrlist != "" {
+		lbRuleCidrList = strings.Split(lbRule.Cidrlist, " ")
+		for i, cidr := range lbRuleCidrList {
+			cidr = strings.TrimSpace(cidr)
+			lbRuleCidrList[i] = cidr
+		}
+	}
+
+	// Check if basic properties match (IP and ports). If not, we need to recreate the rule.
+	basicPropsMatch := lbRule.Publicip == lb.ipAddr &&
+		lbRule.Privateport == strconv.Itoa(int(port.NodePort)) &&
+		lbRule.Publicport == strconv.Itoa(int(port.Port))
+
+	cidrListChanged := len(cidrList) != len(lbRuleCidrList) || !setsEqual(cidrList, lbRuleCidrList)
+
+	// Check if CIDR list also changed and version < 4.22, then we must recreate the rule.
+	if !basicPropsMatch || (cidrListChanged && version.LT(semver.Version{Major: 4, Minor: 22, Patch: 0})) {
+		// Delete the load balancer rule so we can create a new one using the new values.
+		if err := lb.deleteLoadBalancerRule(lbRule); err != nil {
+			return nil, false, err
+		}
+		return nil, false, nil
+	}
+
+	// Rule can be updated. Check what needs updating.
+	updateAlgo := lbRule.Algorithm != lb.algorithm
+	updateProto := lbRule.Protocol != protocol.CSProtocol()
+	updateCidrlist := cidrListChanged
+
+	return lbRule, updateAlgo || updateProto || updateCidrlist, nil
+}
+
+// setsEqual checks if two slices contain the exact same unique elements, regardless of order.
+func setsEqual(listA, listB []string) bool {
+	createSet := func(list []string) map[string]bool {
+		set := make(map[string]bool)
+		for _, item := range list {
+			set[item] = true
+		}
+		return set
+	}
+
+	setA := createSet(listA)
+	setB := createSet(listB)
+
+	if len(setA) != len(setB) {
+		return false
+	}
+
+	for item := range setA {
+		if _, found := setB[item]; !found {
+			return false
+		}
 	}
 
-	return nil, false, nil
+	return true
 }
 
 // updateLoadBalancerRule updates a load balancer rule.
-func (lb *loadBalancer) updateLoadBalancerRule(lbRuleName string, protocol LoadBalancerProtocol) error {
+func (lb *loadBalancer) updateLoadBalancerRule(lbRuleName string, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) error {
 	lbRule := lb.rules[lbRuleName]
 
 	p := lb.LoadBalancer.NewUpdateLoadBalancerRuleParams(lbRule.Id)
 	p.SetAlgorithm(lb.algorithm)
 	p.SetProtocol(protocol.CSProtocol())
 
+	// TODO: Uncomment after updating cloudstack-go SDK to support CIDR list update.
+	// if version.GTE(semver.Version{Major: 4, Minor: 22, Patch: 0}) {
+	// cidrList, err := lb.getCIDRList(service)
+	// 	if err != nil {
+	// 		return err
+	// 	}
+	// 	p.SetCidrlist(strings.Join(cidrList, ","))
+	// }
+
 	_, err := lb.LoadBalancer.UpdateLoadBalancerRule(p)
 	return err
 }
@@ -613,19 +693,9 @@ func (lb *loadBalancer) createLoadBalancerRule(lbRuleName string, port corev1.Se
 	p.SetOpenfirewall(false)
 
 	// Read the source CIDR annotation
-	sourceCIDRs, ok := service.Annotations[ServiceAnnotationLoadBalancerSourceCidrs]
-	var cidrList []string
-	if ok && sourceCIDRs != "" {
-		cidrList = strings.Split(sourceCIDRs, ",")
-		for i, cidr := range cidrList {
-			cidr = strings.TrimSpace(cidr)
-			if _, _, err := net.ParseCIDR(cidr); err != nil {
-				return nil, fmt.Errorf("invalid CIDR in annotation %s: %s", ServiceAnnotationLoadBalancerSourceCidrs, cidr)
-			}
-			cidrList[i] = cidr
-		}
-	} else {
-		cidrList = []string{defaultAllowedCIDR}
+	cidrList, err := lb.getCIDRList(service)
+	if err != nil {
+		return nil, err
 	}
 
 	// Set the CIDR list in the parameters

go.mod

@@ -4,6 +4,7 @@ go 1.23.0
 
 require (
 	github.com/apache/cloudstack-go/v2 v2.17.1
+	github.com/blang/semver/v4 v4.0.0
 	github.com/spf13/pflag v1.0.5
 	gopkg.in/gcfg.v1 v1.2.3
 	k8s.io/api v0.24.17
@@ -17,7 +18,6 @@ require (
 	github.com/Azure/go-ansiterm v0.0.0-20210617225240-d185dfc1b5a1 // indirect
 	github.com/NYTimes/gziphandler v1.1.1 // indirect
 	github.com/beorn7/perks v1.0.1 // indirect
-	github.com/blang/semver/v4 v4.0.0 // indirect
 	github.com/cespare/xxhash/v2 v2.2.0 // indirect
 	github.com/coreos/go-semver v0.3.0 // indirect
 	github.com/coreos/go-systemd/v22 v22.3.2 // indirect