This is an automated email from the ASF dual-hosted git repository. vishesh92 pushed a commit to branch fix-protocol-toggle in repository https://gitbox.apache.org/repos/asf/cloudstack-kubernetes-provider.git
commit 345a563e510086962acadec849c9828b363a3693 Author: vishesh92 <[email protected]> AuthorDate: Wed Aug 26 17:40:54 2026 +0530 Update load balancer rules in place when the protocol changes --- README.md | 2 + cloudstack.go | 11 +- cloudstack_loadbalancer.go | 422 +++++++++++++----- cloudstack_loadbalancer_test.go | 932 +++++++++++++++++++++++++++++++++++++++- cloudstack_test.go | 61 +++ 5 files changed, 1309 insertions(+), 119 deletions(-) diff --git a/README.md b/README.md index 381f2748..08ebc30d 100644 --- a/README.md +++ b/README.md @@ -83,6 +83,8 @@ The CloudStack Kubernetes Provider supports several annotations on LoadBalancer **Description:** Enables the [HAProxy Proxy Protocol](https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt) on a CloudStack load balancer. This annotation only applies to TCP service ports and requires CloudStack 4.6 or later. +Toggling this annotation on an existing LoadBalancer service updates the CloudStack load balancer rule in place — the rule is not recreated and its public port stays uninterrupted. + **Use Case:** Use this annotation when you need to preserve the original client IP address through the load balancer. This is commonly required for ingress controllers like Traefik or Nginx that need to know the client's real IP address. **Example:** diff --git a/cloudstack.go b/cloudstack.go index 2294de6f..2667cd79 100644 --- a/cloudstack.go +++ b/cloudstack.go @@ -127,10 +127,17 @@ func (cs *CSCloud) getManagementServerVersion() (semver.Version, error) { if msServersResp.Count == 0 { return semver.Version{}, errors.New("no management servers found") } + // CloudStack reports four-part versions such as "4.22.0.0". Keep at most the leading three + // parts so the result parses as semver, and so that a build suffix on the fourth part is + // dropped rather than read as a pre-release (which would compare lower than the release). version := msServersResp.ManagementServersMetrics[0].Version - v, err := semver.ParseTolerant(strings.Join(strings.Split(version, ".")[0:3], ".")) + parts := strings.Split(version, ".") + if len(parts) > 3 { + parts = parts[:3] + } + v, err := semver.ParseTolerant(strings.Join(parts, ".")) if err != nil { - klog.Errorf("failed to parse management server version: %v", err) + klog.Errorf("failed to parse management server version %q: %v", version, err) return semver.Version{}, err } return v, nil diff --git a/cloudstack_loadbalancer.go b/cloudstack_loadbalancer.go index ffbdd7cd..bd1bf764 100644 --- a/cloudstack_loadbalancer.go +++ b/cloudstack_loadbalancer.go @@ -23,6 +23,7 @@ import ( "context" "fmt" "net" + "sort" "strconv" "strings" @@ -58,6 +59,11 @@ const ( ServiceAnnotationLoadBalancerIPAssociatedByController = "service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller" //nolint:gosec ) +// cidrListUpdateVersion is the first CloudStack release whose updateLoadBalancerRule API +// accepts a cidrlist. Below it, a changed source CIDR list can only be applied by deleting +// the rule and creating it again. +var cidrListUpdateVersion = semver.Version{Major: 4, Minor: 22, Patch: 0} + type loadBalancer struct { *cloudstack.CloudStackClient @@ -72,6 +78,16 @@ type loadBalancer struct { ipAssociatedByController bool } +// desiredLBRule describes the load balancer rule a service port should be represented by, +// together with the existing CloudStack rule it resolved to (if any). +type desiredLBRule struct { + name string + port corev1.ServicePort + protocol LoadBalancerProtocol + existing *cloudstack.LoadBalancerRule // nil means the rule must be created + update bool // the existing rule needs an update call +} + // GetLoadBalancer returns whether the specified load balancer exists, and if so, what its status is. func (cs *CSCloud) GetLoadBalancer(ctx context.Context, clusterName string, service *corev1.Service) (*corev1.LoadBalancerStatus, bool, error) { klog.V(4).Infof("GetLoadBalancer(%v, %v, %v)", clusterName, service.Namespace, service.Name) @@ -152,96 +168,34 @@ func (cs *CSCloud) EnsureLoadBalancer(ctx context.Context, clusterName string, s klog.V(4).Infof("Load balancer %v is associated with IP %v", lb.name, lb.ipAddr) - for _, port := range service.Spec.Ports { - // Construct the protocol name first, we need it a few times - protocol := ProtocolFromServicePort(port, service) - if protocol == LoadBalancerProtocolInvalid { - return nil, fmt.Errorf("unsupported load balancer protocol: %v", port.Protocol) - } - - // All ports have their own load balancer rule, so add the port to lbName to keep the names unique. - 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, 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, service, cs.version); 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) - } - } else { - klog.V(4).Infof("Creating load balancer rule: %v", lbRuleName) - lbRule, err = lb.createLoadBalancerRule(lbRuleName, port, protocol, service) - if err != nil { - return nil, err - } - - klog.V(4).Infof("Assigning hosts (%v) to load balancer rule: %v", lb.hostIDs, lbRuleName) - if err = lb.assignHostsToRule(lbRule, lb.hostIDs); err != nil { - return nil, err - } - } - - network, count, err := lb.Network.GetNetworkByID(lb.networkID, cloudstack.WithProject(lb.projectID)) - if err != nil { - if count == 0 { - return nil, err - } - return nil, err - } + // Resolve every service port to the rule that should represent it. + desired, err := lb.resolveLoadBalancerRules(service, cs.version) + if err != nil { + return nil, err + } - if lbRule != nil { - if isFirewallSupported(network.Service) { - klog.V(4).Infof("Creating firewall rules for load balancer rule: %v (%v:%v:%v)", lbRuleName, protocol, lbRule.Publicip, port.Port) - if _, err := lb.updateFirewallRule(lbRule.Publicipid, int(port.Port), protocol, service.Spec.LoadBalancerSourceRanges); err != nil { - return nil, err - } - } else if isNetworkACLSupported(network.Service) { - klog.V(4).Infof("Creating ACL rules for load balancer rule: %v (%v:%v:%v)", lbRuleName, protocol, lbRule.Publicip, port.Port) - if _, err := lb.updateNetworkACL(int(port.Port), protocol, network.Id); err != nil { - return nil, err - } - } - } + network, _, err := lb.Network.GetNetworkByID(lb.networkID, cloudstack.WithProject(lb.projectID)) + if err != nil { + return nil, err } - // Cleanup any rules that are now still in the rules map, as they are no longer needed. - for _, lbRule := range lb.rules { - protocol := ProtocolFromLoadBalancer(lbRule.Protocol) - if protocol == LoadBalancerProtocolInvalid { - return nil, fmt.Errorf("error parsing protocol %v: %v", lbRule.Protocol, err) - } - port, err := strconv.ParseInt(lbRule.Publicport, 10, 32) - if err != nil { - return nil, fmt.Errorf("error parsing port %s: %v", lbRule.Publicport, err) - } + claimed := claimedTuples(desired) + blocking, rest := lb.partitionObsoleteRules(desired) - klog.V(4).Infof("Deleting firewall rules associated with load balancer rule: %v (%v:%v:%v)", lbRule.Name, protocol, lbRule.Publicip, port) - if _, err := lb.deleteFirewallRule(lbRule.Publicipid, int(port), protocol); err != nil { - return nil, err - } + // Obsolete rules holding a public port that a new rule needs have to go first, or + // CloudStack rejects the create as a port conflict. + if err := lb.pruneRules(blocking, claimed, network); err != nil { + return nil, err + } - klog.V(4).Infof("Deleting Network ACL rules associated with load balancer rule: %v (%v:%v)", lbRule.Name, protocol, port) - if _, err := lb.deleteNetworkACLRule(int(port), protocol, lb.networkID); err != nil { - return nil, err - } + if err := lb.applyLoadBalancerRules(desired, service, network, cs.version); err != nil { + return nil, err + } - klog.V(4).Infof("Deleting obsolete load balancer rule: %v", lbRule.Name) - if err := lb.deleteLoadBalancerRule(lbRule); err != nil { - return nil, err - } + // Everything else is removed only once the desired rules are in place, so a failure here + // can never leave the service without the rules it does need. + if err := lb.pruneRules(rest, claimed, network); err != nil { + return nil, err } status = &corev1.LoadBalancerStatus{} @@ -649,11 +603,264 @@ func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error) { 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, service *corev1.Service, version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) { - lbRule, ok := lb.rules[lbRuleName] - if !ok { +// splitCIDRList splits the CIDR list of an existing CloudStack rule into its entries. +// CloudStack has reported these both comma and space separated, and a CIDR can contain +// neither character, so treat both as separators. +func splitCIDRList(cidrList string) []string { + return strings.FieldsFunc(cidrList, func(r rune) bool { + return r == ',' || r == ' ' + }) +} + +// resolveLoadBalancerRules maps every service port to the load balancer rule that should +// represent it, claiming each match as it goes so that what remains in lb.rules is exactly +// the obsolete set and no rule can be claimed twice. +func (lb *loadBalancer) resolveLoadBalancerRules(service *corev1.Service, version semver.Version) ([]desiredLBRule, error) { + desired := make([]desiredLBRule, 0, len(service.Spec.Ports)) + + for _, port := range service.Spec.Ports { + // Construct the protocol name first, we need it a few times + protocol := ProtocolFromServicePort(port, service) + if protocol == LoadBalancerProtocolInvalid { + return nil, fmt.Errorf("unsupported load balancer protocol: %v", port.Protocol) + } + + // All ports have their own load balancer rule, so add the port to lbName to keep the names unique. + lbRuleName := fmt.Sprintf("%s-%s-%d", lb.name, protocol, port.Port) + + lbRule, needsUpdate, err := lb.checkLoadBalancerRule(lb.findLoadBalancerRule(lbRuleName, port, protocol), lbRuleName, port, protocol, service, version) + if err != nil { + return nil, err + } + + if lbRule != nil { + // Claim by the rule's actual name: after a protocol change it still carries the old one. + delete(lb.rules, lbRule.Name) + } + + desired = append(desired, desiredLBRule{ + name: lbRuleName, + port: port, + protocol: protocol, + existing: lbRule, + update: needsUpdate, + }) + } + + return desired, nil +} + +// findLoadBalancerRule locates the existing CloudStack rule for a desired service port. It +// prefers an exact name match, then falls back to matching on the tuple. That fallback is what +// lets a protocol change (tcp <-> tcp-proxy) update the existing rule instead of creating a +// conflicting one. +// +// Only rules on the IP being reconciled towards are eligible; a rule on any other IP is left +// for the prune pass, which also cleans up the firewall rules it leaves behind. +func (lb *loadBalancer) findLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol) *cloudstack.LoadBalancerRule { + if lbRule, ok := lb.rules[lbRuleName]; ok && lbRule.Publicipid == lb.ipAddrID { + return lbRule + } + + publicPort := strconv.Itoa(int(port.Port)) + var names []string + for name, lbRule := range lb.rules { + if lbRule.Publicipid == lb.ipAddrID && + ProtocolFromLoadBalancer(lbRule.Protocol).IPProtocol() == protocol.IPProtocol() && + lbRule.Publicport == publicPort { + names = append(names, name) + } + } + if len(names) == 0 { + return nil + } + + // Map iteration order is randomized; sort so the pick is deterministic. + sort.Strings(names) + if len(names) > 1 { + klog.Warningf("Multiple load balancer rules match %s port %s: %v; using %v", protocol.IPProtocol(), publicPort, names, names[0]) + } + return lb.rules[names[0]] +} + +// portProtocol is the tuple CloudStack refuses to place two load balancer rules on, and that +// firewall and network ACL rules are keyed on. IPProtocol maps both tcp and tcp-proxy to +// "tcp", so a tcp and a tcp-proxy rule on one port share a tuple, and one firewall/ACL rule. +type portProtocol struct { + ipProtocol string + publicPort int32 +} + +// obsoleteRule is a rule no desired service port claimed, with its tuple already parsed. +type obsoleteRule struct { + rule *cloudstack.LoadBalancerRule + protocol LoadBalancerProtocol + tuple portProtocol +} + +// claimedTuples returns the tuples the service still needs. +func claimedTuples(desired []desiredLBRule) map[portProtocol]bool { + claimed := make(map[portProtocol]bool, len(desired)) + for _, d := range desired { + claimed[portProtocol{d.protocol.IPProtocol(), d.port.Port}] = true + } + return claimed +} + +// partitionObsoleteRules splits the rules left in lb.rules — those no desired port claimed — +// into the ones holding a tuple that a rule still to be created needs, and the rest. +func (lb *loadBalancer) partitionObsoleteRules(desired []desiredLBRule) (blocking, rest []obsoleteRule) { + // CloudStack refuses two load balancer rules with overlapping public port ranges on one + // IP whatever their protocols, so the port alone decides what blocks a create. Note this + // is deliberately coarser than the firewall/ACL claim, which is per protocol because + // firewall rules are. + neededPorts := make(map[int32]bool) + for _, d := range desired { + if d.existing == nil { + neededPorts[d.port.Port] = true + } + } + + // Iterate in name order so the prune sequence is reproducible. + names := make([]string, 0, len(lb.rules)) + for name := range lb.rules { + names = append(names, name) + } + sort.Strings(names) + + for _, name := range names { + lbRule := lb.rules[name] + + protocol := ProtocolFromLoadBalancer(lbRule.Protocol) + if protocol == LoadBalancerProtocolInvalid { + klog.Errorf("Skipping obsolete load balancer rule %v with unknown protocol %v", lbRule.Name, lbRule.Protocol) + continue + } + port, err := strconv.ParseInt(lbRule.Publicport, 10, 32) + if err != nil { + klog.Errorf("Skipping obsolete load balancer rule %v with invalid public port %v: %v", lbRule.Name, lbRule.Publicport, err) + continue + } + + obsolete := obsoleteRule{ + rule: lbRule, + protocol: protocol, + tuple: portProtocol{protocol.IPProtocol(), int32(port)}, + } + + // Conflicts are per public IP, so only a rule on the IP being reconciled towards can + // block a create. + if lbRule.Publicipid == lb.ipAddrID && neededPorts[obsolete.tuple.publicPort] { + blocking = append(blocking, obsolete) + } else { + rest = append(rest, obsolete) + } + } + + return blocking, rest +} + +// pruneRules deletes the given obsolete rules along with their firewall or network ACL rules. +// A firewall/ACL rule is kept when a desired port still claims the same tuple, since the two +// load balancer rules share it and pruning would strip the survivor of its opening. +// +// A rule that fails to delete is reported but does not stop the others being pruned. +func (lb *loadBalancer) pruneRules(obsolete []obsoleteRule, claimed map[portProtocol]bool, network *cloudstack.Network) error { + var firstErr error + recordErr := func(err error) { + klog.Errorf("Error pruning obsolete load balancer rule: %v", err) + if firstErr == nil { + firstErr = err + } + } + + for _, o := range obsolete { + lbRule, protocol, port := o.rule, o.protocol, o.tuple.publicPort + + if isFirewallSupported(network.Service) { + // Firewall rules belong to a single public IP, so a claim only covers a rule on + // the IP the service is being reconciled towards. + if claimed[o.tuple] && lbRule.Publicipid == lb.ipAddrID { + klog.V(4).Infof("Keeping firewall rules of obsolete load balancer rule %v (%v:%v:%v): still claimed by a service port", lbRule.Name, protocol, lbRule.Publicip, port) + } else { + klog.V(4).Infof("Deleting firewall rules associated with load balancer rule: %v (%v:%v:%v)", lbRule.Name, protocol, lbRule.Publicip, port) + if _, err := lb.deleteFirewallRule(lbRule.Publicipid, int(port), protocol); err != nil { + recordErr(err) + continue + } + } + } else if isNetworkACLSupported(network.Service) { + // ACL rules belong to the network rather than an IP, so the claim always applies. + if claimed[o.tuple] { + klog.V(4).Infof("Keeping Network ACL rules of obsolete load balancer rule %v (%v:%v): still claimed by a service port", lbRule.Name, protocol, port) + } else { + klog.V(4).Infof("Deleting Network ACL rules associated with load balancer rule: %v (%v:%v)", lbRule.Name, protocol, port) + if _, err := lb.deleteNetworkACLRule(int(port), protocol, lb.networkID); err != nil { + recordErr(err) + continue + } + } + } + + klog.V(4).Infof("Deleting obsolete load balancer rule: %v", lbRule.Name) + if err := lb.deleteLoadBalancerRule(lbRule); err != nil { + recordErr(err) + } + } + + return firstErr +} + +// applyLoadBalancerRules creates or updates the load balancer rule of every desired service +// port and reconciles the firewall or network ACL rules it needs. +func (lb *loadBalancer) applyLoadBalancerRules(desired []desiredLBRule, service *corev1.Service, network *cloudstack.Network, version semver.Version) error { + for _, d := range desired { + lbRule := d.existing + + if lbRule != nil { + if d.update { + klog.V(4).Infof("Updating load balancer rule: %v", d.name) + if err := lb.updateLoadBalancerRule(lbRule, d.name, d.protocol, service, version); err != nil { + return err + } + } else { + klog.V(4).Infof("Load balancer rule %v is up-to-date", d.name) + } + } else { + klog.V(4).Infof("Creating load balancer rule: %v", d.name) + newRule, err := lb.createLoadBalancerRule(d.name, d.port, d.protocol, service) + if err != nil { + return err + } + lbRule = newRule + + klog.V(4).Infof("Assigning hosts (%v) to load balancer rule: %v", lb.hostIDs, d.name) + if err := lb.assignHostsToRule(lbRule, lb.hostIDs); err != nil { + return err + } + } + + if isFirewallSupported(network.Service) { + klog.V(4).Infof("Creating firewall rules for load balancer rule: %v (%v:%v:%v)", d.name, d.protocol, lbRule.Publicip, d.port.Port) + if _, err := lb.updateFirewallRule(lbRule.Publicipid, int(d.port.Port), d.protocol, service.Spec.LoadBalancerSourceRanges); err != nil { + return err + } + } else if isNetworkACLSupported(network.Service) { + klog.V(4).Infof("Creating ACL rules for load balancer rule: %v (%v:%v:%v)", d.name, d.protocol, lbRule.Publicip, d.port.Port) + if _, err := lb.updateNetworkACL(int(d.port.Port), d.protocol, network.Id); err != nil { + return err + } + } + } + + return nil +} + +// checkLoadBalancerRule checks if the given existing rule (nil if none was found) is up to +// date, can be brought up to date with an update call, or must be recreated. If it must be +// recreated, the existing rule is deleted so it can be created again. +func (lb *loadBalancer) checkLoadBalancerRule(lbRule *cloudstack.LoadBalancerRule, lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) { + if lbRule == nil { return nil, false, nil } @@ -662,14 +869,7 @@ func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.Ser 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 - } - } + lbRuleCidrList := splitCIDRList(lbRule.Cidrlist) // Check if basic properties match (IP and ports). If not, we need to recreate the rule. basicPropsMatch := lbRule.Publicip == lb.ipAddr && @@ -677,9 +877,12 @@ func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.Ser lbRule.Publicport == strconv.Itoa(int(port.Port)) cidrListChanged := len(cidrList) != len(lbRuleCidrList) || !compareStringSlice(cidrList, lbRuleCidrList) + updateProto := lbRule.Protocol != protocol.CSProtocol() - // 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})) { + // A CIDR change on an older CloudStack can only be applied by recreating the rule. + // Applying a protocol change in place needs CloudStack 4.17.1 or later; below that the + // protocol parameter is accepted and ignored. + if !basicPropsMatch || (cidrListChanged && version.LT(cidrListUpdateVersion)) { // 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 @@ -689,21 +892,22 @@ func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.Ser // Rule can be updated. Check what needs updating. updateAlgo := lbRule.Algorithm != lb.algorithm - updateProto := lbRule.Protocol != protocol.CSProtocol() + // The name encodes the protocol, so a rule matched across a protocol change needs renaming. + updateName := lbRule.Name != lbRuleName - return lbRule, updateAlgo || updateProto || cidrListChanged, nil + return lbRule, updateAlgo || updateProto || updateName || cidrListChanged, nil } // updateLoadBalancerRule updates a load balancer rule. -func (lb *loadBalancer) updateLoadBalancerRule(lbRuleName string, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) error { - lbRule := lb.rules[lbRuleName] - +func (lb *loadBalancer) updateLoadBalancerRule(lbRule *cloudstack.LoadBalancerRule, lbRuleName string, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) error { p := lb.LoadBalancer.NewUpdateLoadBalancerRuleParams(lbRule.Id) p.SetAlgorithm(lb.algorithm) p.SetProtocol(protocol.CSProtocol()) + p.SetName(lbRuleName) - // If version >= 4.22, we can update the CIDR list. - if version.GTE(semver.Version{Major: 4, Minor: 22, Patch: 0}) { + // Only send the CIDR list where the API accepts it; checkLoadBalancerRule recreates the + // rule instead on older releases, so a change can never be silently dropped here. + if version.GTE(cidrListUpdateVersion) { cidrList, err := lb.getCIDRList(service) if err != nil { return err @@ -937,7 +1141,7 @@ func (lb *loadBalancer) updateFirewallRule(publicIpId string, publicPort int, pr // determine if we already have a rule with matching cidrs var match *cloudstack.FirewallRule for rule := range filtered { - cidrlist := strings.Split(rule.Cidrlist, ",") + cidrlist := splitCIDRList(rule.Cidrlist) if compareStringSlice(cidrlist, allowedIPs) { klog.V(4).Infof("Found identical rule: %v", rule) match = rule @@ -1025,7 +1229,9 @@ func (lb *loadBalancer) updateNetworkACL(publicPort int, protocol LoadBalancerPr } // create ACL rule - acl := lb.NetworkACL.NewCreateNetworkACLParams(protocol.CSProtocol()) + // ACL rules only know tcp/udp/icmp, so tcp-proxy maps to tcp. This also matches the + // filter above, which would otherwise never find the rule again. + acl := lb.NetworkACL.NewCreateNetworkACLParams(protocol.IPProtocol()) acl.SetAclid(network.Aclid) acl.SetAction("Allow") acl.SetCidrlist([]string{"0.0.0.0/0"}) diff --git a/cloudstack_loadbalancer_test.go b/cloudstack_loadbalancer_test.go index 4bbf38e7..d5c949ca 100644 --- a/cloudstack_loadbalancer_test.go +++ b/cloudstack_loadbalancer_test.go @@ -20,6 +20,7 @@ package cloudstack import ( + "context" "fmt" "reflect" "sort" @@ -570,7 +571,7 @@ func TestCheckLoadBalancerRule(t *testing.T) { port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} service := &corev1.Service{} - rule, needsUpdate, err := lb.checkLoadBalancerRule("missing", port, LoadBalancerProtocolTCP, service, semver.Version{}) + rule, needsUpdate, err := lb.checkLoadBalancerRule(lb.findLoadBalancerRule("missing", port, LoadBalancerProtocolTCP), "missing", port, LoadBalancerProtocolTCP, service, semver.Version{}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -615,7 +616,7 @@ func TestCheckLoadBalancerRule(t *testing.T) { port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} service := &corev1.Service{} - rule, needsUpdate, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 21, Patch: 0}) + rule, needsUpdate, err := lb.checkLoadBalancerRule(lb.rules["rule"], "rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 21, Patch: 0}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -667,7 +668,7 @@ func TestCheckLoadBalancerRule(t *testing.T) { }, } - rule, needsUpdate, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) + rule, needsUpdate, err := lb.checkLoadBalancerRule(lb.rules["rule"], "rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -679,6 +680,55 @@ func TestCheckLoadBalancerRule(t *testing.T) { } }) + t.Run("matching multi-CIDR list needs no update", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + // No expectations: an unchanged CIDR list must not trigger any API call. + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + + lbRule := &cloudstack.LoadBalancerRule{ + Id: "rule-id", + Name: "rule", + Publicip: "1.1.1.1", + Privateport: "30000", + Publicport: "80", + Cidrlist: "10.0.0.0/8,192.168.0.0/16", + Algorithm: "roundrobin", + Protocol: LoadBalancerProtocolTCP.CSProtocol(), + } + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + ipAddr: "1.1.1.1", + algorithm: "roundrobin", + rules: map[string]*cloudstack.LoadBalancerRule{ + "rule": lbRule, + }, + } + port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} + service := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + ServiceAnnotationLoadBalancerSourceCidrs: "10.0.0.0/8,192.168.0.0/16", + }, + }, + } + + rule, needsUpdate, err := lb.checkLoadBalancerRule(lb.rules["rule"], "rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule != lbRule { + t.Fatalf("expected existing rule to be returned") + } + if needsUpdate { + t.Fatalf("expected needsUpdate to be false for an unchanged CIDR list") + } + }) + t.Run("cidr change triggers delete with older version", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -723,7 +773,7 @@ func TestCheckLoadBalancerRule(t *testing.T) { }, } - rule, needsUpdate, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 12, Patch: 0}) + rule, needsUpdate, err := lb.checkLoadBalancerRule(lb.rules["rule"], "rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 12, Patch: 0}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -735,6 +785,50 @@ func TestCheckLoadBalancerRule(t *testing.T) { } }) + t.Run("protocol change updates in place", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + // No expectations: a protocol change is an update, never a recreate. Note this needs + // CloudStack 4.17.1+, which is where the protocol parameter started being applied. + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + + lbRule := &cloudstack.LoadBalancerRule{ + Id: "rule-id", + Name: "rule-tcp-80", + Publicip: "1.1.1.1", + Privateport: "30000", + Publicport: "80", + Cidrlist: defaultAllowedCIDR, + Algorithm: "roundrobin", + Protocol: LoadBalancerProtocolTCP.CSProtocol(), + } + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + }, + ipAddr: "1.1.1.1", + algorithm: "roundrobin", + rules: map[string]*cloudstack.LoadBalancerRule{ + "rule-tcp-80": lbRule, + }, + } + port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} + service := &corev1.Service{} + + rule, needsUpdate, err := lb.checkLoadBalancerRule(lbRule, "rule-tcp-proxy-80", port, LoadBalancerProtocolTCPProxy, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if rule != lbRule { + t.Fatalf("expected existing rule to be returned") + } + if !needsUpdate { + t.Fatalf("expected needsUpdate to be true for a protocol and name change") + } + }) + t.Run("invalid cidr returns error", func(t *testing.T) { lb := &loadBalancer{ rules: map[string]*cloudstack.LoadBalancerRule{ @@ -759,13 +853,182 @@ func TestCheckLoadBalancerRule(t *testing.T) { }, } - _, _, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) + _, _, err := lb.checkLoadBalancerRule(lb.rules["rule"], "rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) if err == nil { t.Fatalf("expected error for invalid CIDR") } }) } +func TestSplitCIDRList(t *testing.T) { + tests := []struct { + name string + cidrList string + want []string + }{ + {name: "empty", cidrList: "", want: nil}, + {name: "single", cidrList: "10.0.0.0/8", want: []string{"10.0.0.0/8"}}, + { + name: "comma separated", + cidrList: "10.0.0.0/8,192.168.0.0/16", + want: []string{"10.0.0.0/8", "192.168.0.0/16"}, + }, + { + name: "space separated", + cidrList: "10.0.0.0/8 192.168.0.0/16", + want: []string{"10.0.0.0/8", "192.168.0.0/16"}, + }, + { + name: "comma and surrounding spaces", + cidrList: "10.0.0.0/8, 192.168.0.0/16", + want: []string{"10.0.0.0/8", "192.168.0.0/16"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := splitCIDRList(tt.cidrList) + if len(got) != len(tt.want) { + t.Fatalf("splitCIDRList(%q) = %v, want %v", tt.cidrList, got, tt.want) + } + for i := range got { + if got[i] != tt.want[i] { + t.Errorf("splitCIDRList(%q)[%d] = %q, want %q", tt.cidrList, i, got[i], tt.want[i]) + } + } + }) + } +} + +func TestFindLoadBalancerRule(t *testing.T) { + port80 := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} + + // newLB builds a load balancer reconciling towards ip-1, holding the given rules. + newLB := func(rules ...*cloudstack.LoadBalancerRule) *loadBalancer { + lb := &loadBalancer{ + ipAddr: "10.0.0.1", + ipAddrID: "ip-1", + rules: map[string]*cloudstack.LoadBalancerRule{}, + } + for _, r := range rules { + lb.rules[r.Name] = r + } + return lb + } + rule := func(name, protocol, publicPort string) *cloudstack.LoadBalancerRule { + return &cloudstack.LoadBalancerRule{ + Name: name, Protocol: protocol, Publicport: publicPort, + Publicip: "10.0.0.1", Publicipid: "ip-1", + } + } + + t.Run("exact name match", func(t *testing.T) { + tcpRule := rule("lb-tcp-80", "tcp", "80") + lb := newLB(tcpRule) + + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != tcpRule { + t.Fatalf("findLoadBalancerRule = %v, want exact match %v", got, tcpRule) + } + }) + + t.Run("protocol toggle falls back to IP protocol and port", func(t *testing.T) { + tcpRule := rule("lb-tcp-80", "tcp", "80") + lb := newLB(tcpRule) + + if got := lb.findLoadBalancerRule("lb-tcp-proxy-80", port80, LoadBalancerProtocolTCPProxy); got != tcpRule { + t.Fatalf("findLoadBalancerRule = %v, want fallback match %v", got, tcpRule) + } + }) + + t.Run("reverse protocol toggle", func(t *testing.T) { + proxyRule := rule("lb-tcp-proxy-80", "tcp-proxy", "80") + lb := newLB(proxyRule) + + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != proxyRule { + t.Fatalf("findLoadBalancerRule = %v, want fallback match %v", got, proxyRule) + } + }) + + t.Run("udp rule does not match tcp port", func(t *testing.T) { + lb := newLB(rule("lb-udp-80", "udp", "80")) + + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != nil { + t.Fatalf("findLoadBalancerRule = %v, want nil (udp must not satisfy tcp)", got) + } + }) + + t.Run("tcp and udp on the same port stay distinct", func(t *testing.T) { + tcpRule := rule("lb-tcp-8000", "tcp", "8000") + udpRule := rule("lb-udp-8000", "udp", "8000") + lb := newLB(tcpRule, udpRule) + port := corev1.ServicePort{Port: 8000, NodePort: 30800, Protocol: corev1.ProtocolUDP} + + if got := lb.findLoadBalancerRule("lb-udp-8000", port, LoadBalancerProtocolUDP); got != udpRule { + t.Fatalf("findLoadBalancerRule = %v, want %v", got, udpRule) + } + // A proxy-protocol toggle on the tcp port must resolve to the tcp rule, never the udp one. + port.Protocol = corev1.ProtocolTCP + if got := lb.findLoadBalancerRule("lb-tcp-proxy-8000", port, LoadBalancerProtocolTCPProxy); got != tcpRule { + t.Fatalf("findLoadBalancerRule = %v, want %v", got, tcpRule) + } + }) + + t.Run("port mismatch returns nil", func(t *testing.T) { + lb := newLB(rule("lb-tcp-443", "tcp", "443")) + + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != nil { + t.Fatalf("findLoadBalancerRule = %v, want nil", got) + } + }) + + t.Run("rule on another IP is not reused", func(t *testing.T) { + // Reusing a rule on a stale IP would delete it via checkLoadBalancerRule, stranding + // its firewall rule. It must be left for the prune pass instead. + staleName := rule("lb-tcp-80", "tcp", "80") + staleName.Publicip, staleName.Publicipid = "10.0.0.2", "ip-2" + staleFallback := rule("lb-tcp-proxy-80", "tcp-proxy", "80") + staleFallback.Publicip, staleFallback.Publicipid = "10.0.0.2", "ip-2" + lb := newLB(staleName, staleFallback) + + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != nil { + t.Fatalf("findLoadBalancerRule = %v, want nil for a rule on another IP", got) + } + }) + + t.Run("current IP preferred over exact name on another IP", func(t *testing.T) { + staleName := rule("lb-tcp-80", "tcp", "80") + staleName.Publicip, staleName.Publicipid = "10.0.0.2", "ip-2" + current := rule("lb-tcp-proxy-80", "tcp-proxy", "80") + lb := newLB(staleName, current) + + // The exact name lives on the stale IP; the fallback must find the current-IP rule. + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != current { + t.Fatalf("findLoadBalancerRule = %v, want current-IP rule %v", got, current) + } + }) + + t.Run("multiple candidates picked deterministically", func(t *testing.T) { + ruleA := rule("lb-tcp-80-a", "tcp", "80") + ruleB := rule("lb-tcp-80-b", "tcp-proxy", "80") + lb := newLB(ruleA, ruleB) + + // Both share (tcp, 80); the pick must follow name order, not map order. + for i := 0; i < 10; i++ { + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != ruleA { + t.Fatalf("findLoadBalancerRule = %v, want deterministic first-by-name %v", got, ruleA) + } + } + }) + + t.Run("empty rules map returns nil", func(t *testing.T) { + lb := newLB() + + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != nil { + t.Fatalf("findLoadBalancerRule = %v, want nil", got) + } + }) +} + func TestRuleToString(t *testing.T) { tests := []struct { name string @@ -1971,10 +2234,13 @@ func TestUpdateLoadBalancerRule(t *testing.T) { service := &corev1.Service{} - err := lb.updateLoadBalancerRule("test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) + err := lb.updateLoadBalancerRule(lb.rules["test-rule-tcp-80"], "test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) if err != nil { t.Fatalf("unexpected error: %v", err) } + if algo, _ := updateParams.GetAlgorithm(); algo != "source" { + t.Errorf("algorithm = %q, want %q", algo, "source") + } }) t.Run("update protocol", func(t *testing.T) { @@ -2005,10 +2271,17 @@ func TestUpdateLoadBalancerRule(t *testing.T) { service := &corev1.Service{} - err := lb.updateLoadBalancerRule("test-rule-tcp-80", LoadBalancerProtocolTCPProxy, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) + // Matched under the old name, so the update must switch protocol and rename. + err := lb.updateLoadBalancerRule(lb.rules["test-rule-tcp-80"], "test-rule-tcp-proxy-80", LoadBalancerProtocolTCPProxy, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) if err != nil { t.Fatalf("unexpected error: %v", err) } + if proto, _ := updateParams.GetProtocol(); proto != "tcp-proxy" { + t.Errorf("protocol = %q, want %q", proto, "tcp-proxy") + } + if name, _ := updateParams.GetName(); name != "test-rule-tcp-proxy-80" { + t.Errorf("name = %q, want %q", name, "test-rule-tcp-proxy-80") + } }) t.Run("update CIDR list (CS >= 4.22)", func(t *testing.T) { @@ -2046,10 +2319,13 @@ func TestUpdateLoadBalancerRule(t *testing.T) { }, } - err := lb.updateLoadBalancerRule("test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) + err := lb.updateLoadBalancerRule(lb.rules["test-rule-tcp-80"], "test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) if err != nil { t.Fatalf("unexpected error: %v", err) } + if cidrs, _ := updateParams.GetCidrlist(); len(cidrs) != 1 || cidrs[0] != "10.0.0.0/8" { + t.Errorf("cidrlist = %v, want %v", cidrs, []string{"10.0.0.0/8"}) + } }) t.Run("error updating rule", func(t *testing.T) { @@ -2081,7 +2357,7 @@ func TestUpdateLoadBalancerRule(t *testing.T) { service := &corev1.Service{} - err := lb.updateLoadBalancerRule("test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) + err := lb.updateLoadBalancerRule(lb.rules["test-rule-tcp-80"], "test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) if err == nil { t.Fatalf("expected error") } @@ -2853,6 +3129,114 @@ func TestUpdateNetworkACL(t *testing.T) { } }) + t.Run("tcp-proxy creates ACL rule with tcp protocol", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl) + mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl) + networkResp := &cloudstack.Network{ + Id: "net-123", + Aclid: "acl-456", + Service: []cloudstack.NetworkServiceInternal{}, + } + + aclListResp := &cloudstack.NetworkACLList{ + Id: "acl-456", + Name: "custom-acl", + } + + listParams := &cloudstack.ListNetworkACLsParams{} + listResp := &cloudstack.ListNetworkACLsResponse{ + Count: 0, + NetworkACLs: []*cloudstack.NetworkACL{}, + } + + createParams := &cloudstack.CreateNetworkACLParams{} + createResp := &cloudstack.CreateNetworkACLResponse{ + Id: "acl-rule-123", + } + + gomock.InOrder( + mockNetwork.EXPECT().GetNetworkByID("net-123").Return(networkResp, 1, nil), + mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-456").Return(aclListResp, 1, nil), + mockNetworkACL.EXPECT().NewListNetworkACLsParams().Return(listParams), + mockNetworkACL.EXPECT().ListNetworkACLs(gomock.Any()).Return(listResp, nil), + // tcp-proxy must be created as tcp. + mockNetworkACL.EXPECT().NewCreateNetworkACLParams("tcp").Return(createParams), + mockNetworkACL.EXPECT().CreateNetworkACL(gomock.Any()).Return(createResp, nil), + ) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + Network: mockNetwork, + NetworkACL: mockNetworkACL, + }, + } + + updated, err := lb.updateNetworkACL(80, LoadBalancerProtocolTCPProxy, "net-123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !updated { + t.Errorf("updated = false, want true") + } + }) + + t.Run("tcp-proxy matches existing tcp ACL rule", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl) + mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl) + networkResp := &cloudstack.Network{ + Id: "net-123", + Aclid: "acl-456", + Service: []cloudstack.NetworkServiceInternal{}, + } + + aclListResp := &cloudstack.NetworkACLList{ + Id: "acl-456", + Name: "custom-acl", + } + + listParams := &cloudstack.ListNetworkACLsParams{} + listResp := &cloudstack.ListNetworkACLsResponse{ + Count: 1, + NetworkACLs: []*cloudstack.NetworkACL{ + { + Id: "acl-rule-123", + Protocol: "tcp", + Startport: "80", + Endport: "80", + }, + }, + } + + // No create expectations: the tcp rule already satisfies the tcp-proxy port. + gomock.InOrder( + mockNetwork.EXPECT().GetNetworkByID("net-123").Return(networkResp, 1, nil), + mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-456").Return(aclListResp, 1, nil), + mockNetworkACL.EXPECT().NewListNetworkACLsParams().Return(listParams), + mockNetworkACL.EXPECT().ListNetworkACLs(gomock.Any()).Return(listResp, nil), + ) + + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + Network: mockNetwork, + NetworkACL: mockNetworkACL, + }, + } + + updated, err := lb.updateNetworkACL(80, LoadBalancerProtocolTCPProxy, "net-123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !updated { + t.Errorf("updated = false, want true") + } + }) + t.Run("rule already exists", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -3685,3 +4069,533 @@ func TestVerifyHosts(t *testing.T) { } }) } + +// ensureLBTestEnv holds the fixtures shared by the TestEnsureLoadBalancer subtests. +// Each subtest sets its own mock expectations. +type ensureLBTestEnv struct { + cs *CSCloud + lb *cloudstack.MockLoadBalancerServiceIface + vm *cloudstack.MockVirtualMachineServiceIface + network *cloudstack.MockNetworkServiceIface + firewall *cloudstack.MockFirewallServiceIface + service *corev1.Service + nodes []*corev1.Node +} + +func newEnsureLBTestEnv(ctrl *gomock.Controller, annotations map[string]string, ports []corev1.ServicePort) *ensureLBTestEnv { + e := &ensureLBTestEnv{ + lb: cloudstack.NewMockLoadBalancerServiceIface(ctrl), + vm: cloudstack.NewMockVirtualMachineServiceIface(ctrl), + network: cloudstack.NewMockNetworkServiceIface(ctrl), + firewall: cloudstack.NewMockFirewallServiceIface(ctrl), + } + + e.cs = &CSCloud{ + client: &cloudstack.CloudStackClient{ + LoadBalancer: e.lb, + VirtualMachine: e.vm, + Network: e.network, + Firewall: e.firewall, + }, + version: semver.Version{Major: 4, Minor: 22, Patch: 0}, + } + + // UID "test-uid" makes the load balancer name "atestuid", so rules are atestuid-<protocol>-<port>. + e.service = &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-service", + Namespace: "default", + UID: "test-uid", + Annotations: annotations, + }, + Spec: corev1.ServiceSpec{ + SessionAffinity: corev1.ServiceAffinityNone, + Ports: ports, + }, + } + + e.nodes = []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}}} + + return e +} + +// expectHostsAndNetwork registers the host and network lookups every run performs. +func (e *ensureLBTestEnv) expectHostsAndNetwork() { + e.vm.EXPECT().NewListVirtualMachinesParams().Return(&cloudstack.ListVirtualMachinesParams{}) + e.vm.EXPECT().ListVirtualMachines(gomock.Any()).Return(&cloudstack.ListVirtualMachinesResponse{ + Count: 1, + VirtualMachines: []*cloudstack.VirtualMachine{ + {Id: "vm-1", Name: "node-1", Nic: []cloudstack.Nic{{Networkid: "net-1"}}}, + }, + }, nil) + e.network.EXPECT().GetNetworkByID("net-1", gomock.Any()).Return(&cloudstack.Network{ + Id: "net-1", + Service: []cloudstack.NetworkServiceInternal{{Name: "Firewall"}}, + }, 1, nil) +} + +func TestEnsureLoadBalancer(t *testing.T) { + tcpPort80 := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} + + existingTCPRule := func() *cloudstack.LoadBalancerRule { + return &cloudstack.LoadBalancerRule{ + Id: "rule-1", + Name: "atestuid-tcp-80", + Publicip: "10.0.0.1", + Publicipid: "ip-1", + Publicport: "80", + Privateport: "30000", + Cidrlist: defaultAllowedCIDR, + Algorithm: "roundrobin", + Protocol: "tcp", + Networkid: "net-1", + } + } + + matchingFirewallRule := &cloudstack.FirewallRule{ + Id: "fw-1", + Protocol: "tcp", + Startport: 80, + Endport: 80, + Cidrlist: defaultAllowedCIDR, + } + + t.Run("proxy protocol toggle updates rule in place", func(t *testing.T) { + // Regression test for issue #2: enabling the annotation on a live service must update + // the existing tcp rule, not create a conflicting tcp-proxy rule on the same port. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, map[string]string{ + ServiceAnnotationLoadBalancerProxyProtocol: "true", + }, []corev1.ServicePort{tcpPort80}) + env.expectHostsAndNetwork() + + updateParams := &cloudstack.UpdateLoadBalancerRuleParams{} + + // No create or delete expectations: either call fails the test. + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{existingTCPRule()}, + }, nil), + env.lb.EXPECT().NewUpdateLoadBalancerRuleParams("rule-1").Return(updateParams), + env.lb.EXPECT().UpdateLoadBalancerRule(gomock.Any()).Return(&cloudstack.UpdateLoadBalancerRuleResponse{}, nil), + ) + + // The existing tcp/80 firewall rule also serves tcp-proxy. + gomock.InOrder( + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{matchingFirewallRule}, + }, nil), + ) + + status, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(status.Ingress) != 1 || status.Ingress[0].IP != "10.0.0.1" { + t.Errorf("status.Ingress = %v, want IP 10.0.0.1", status.Ingress) + } + if proto, _ := updateParams.GetProtocol(); proto != "tcp-proxy" { + t.Errorf("updated protocol = %q, want %q", proto, "tcp-proxy") + } + if name, _ := updateParams.GetName(); name != "atestuid-tcp-proxy-80" { + t.Errorf("updated name = %q, want %q", name, "atestuid-tcp-proxy-80") + } + }) + + t.Run("proxy protocol removal updates rule in place", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{tcpPort80}) + env.expectHostsAndNetwork() + + existingProxyRule := existingTCPRule() + existingProxyRule.Name = "atestuid-tcp-proxy-80" + existingProxyRule.Protocol = "tcp-proxy" + + updateParams := &cloudstack.UpdateLoadBalancerRuleParams{} + + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{existingProxyRule}, + }, nil), + env.lb.EXPECT().NewUpdateLoadBalancerRuleParams("rule-1").Return(updateParams), + env.lb.EXPECT().UpdateLoadBalancerRule(gomock.Any()).Return(&cloudstack.UpdateLoadBalancerRuleResponse{}, nil), + ) + + gomock.InOrder( + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{matchingFirewallRule}, + }, nil), + ) + + _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if proto, _ := updateParams.GetProtocol(); proto != "tcp" { + t.Errorf("updated protocol = %q, want %q", proto, "tcp") + } + if name, _ := updateParams.GetName(); name != "atestuid-tcp-80" { + t.Errorf("updated name = %q, want %q", name, "atestuid-tcp-80") + } + }) + + t.Run("obsolete rule pruned after new rule created", func(t *testing.T) { + // The service moved from port 80 to 443. The new rule is created first, so a failure + // while pruning port 80 can never leave the service with no rule at all. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + tcpPort443 := corev1.ServicePort{Port: 443, NodePort: 30443, Protocol: corev1.ProtocolTCP} + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{tcpPort443}) + env.expectHostsAndNetwork() + + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{existingTCPRule()}, + }, nil), + + // The port 443 rule is created, then its hosts and firewall reconciled. + env.lb.EXPECT().NewCreateLoadBalancerRuleParams("roundrobin", "atestuid-tcp-443", 30443, 443).Return(&cloudstack.CreateLoadBalancerRuleParams{}), + env.lb.EXPECT().CreateLoadBalancerRule(gomock.Any()).Return(&cloudstack.CreateLoadBalancerRuleResponse{ + Id: "rule-2", + Name: "atestuid-tcp-443", + Publicip: "10.0.0.1", + Publicipid: "ip-1", + Protocol: "tcp", + }, nil), + env.lb.EXPECT().NewAssignToLoadBalancerRuleParams("rule-2").Return(&cloudstack.AssignToLoadBalancerRuleParams{}), + env.lb.EXPECT().AssignToLoadBalancerRule(gomock.Any()).Return(&cloudstack.AssignToLoadBalancerRuleResponse{}, nil), + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{}, nil), + env.firewall.EXPECT().NewCreateFirewallRuleParams("ip-1", "tcp").Return(&cloudstack.CreateFirewallRuleParams{}), + env.firewall.EXPECT().CreateFirewallRule(gomock.Any()).Return(&cloudstack.CreateFirewallRuleResponse{}, nil), + + // Only then is the obsolete port 80 rule pruned: firewall rule, then the LB rule. + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{matchingFirewallRule}, + }, nil), + env.firewall.EXPECT().NewDeleteFirewallRuleParams("fw-1").Return(&cloudstack.DeleteFirewallRuleParams{}), + env.firewall.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&cloudstack.DeleteFirewallRuleResponse{}, nil), + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-1").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + ) + + _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("duplicate rule on claimed port keeps firewall rules", func(t *testing.T) { + // A leftover tcp rule shares (tcp, 80) with the desired tcp-proxy rule. The duplicate + // is pruned, but its firewall rule is the one the kept rule needs, so it must stay. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, map[string]string{ + ServiceAnnotationLoadBalancerProxyProtocol: "true", + }, []corev1.ServicePort{tcpPort80}) + env.expectHostsAndNetwork() + + duplicateProxyRule := existingTCPRule() + duplicateProxyRule.Id = "rule-2" + duplicateProxyRule.Name = "atestuid-tcp-proxy-80" + duplicateProxyRule.Protocol = "tcp-proxy" + + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 2, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{existingTCPRule(), duplicateProxyRule}, + }, nil), + // The obsolete tcp rule is deleted, the tcp-proxy rule is kept as-is. + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-1").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + ) + + // One firewall listing (the apply pass) and no deletions. + gomock.InOrder( + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{matchingFirewallRule}, + }, nil), + ) + + _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("rule blocking a create is pruned before the create", func(t *testing.T) { + // Two rules share (tcp, 80) and the nodePort changed, so the matched rule is deleted + // and has to be recreated. The surviving duplicate still holds public port 80, so it + // must be deleted before the create or CloudStack rejects it with a port conflict. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + movedPort80 := corev1.ServicePort{Port: 80, NodePort: 30001, Protocol: corev1.ProtocolTCP} + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{movedPort80}) + env.expectHostsAndNetwork() + + matched := existingTCPRule() + matched.Id = "rule-a" + duplicate := existingTCPRule() + duplicate.Id = "rule-b" + duplicate.Name = "atestuid-tcp-proxy-80" + duplicate.Protocol = "tcp-proxy" + + // No firewall delete expectation: the tcp/80 opening is still claimed by the desired + // port, so pruning the duplicate must leave it alone. + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 2, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{matched, duplicate}, + }, nil), + + // The matched rule cannot take a new nodePort, so it is dropped while resolving. + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-a").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + + // The duplicate is pruned next, freeing public port 80... + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-b").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + + // ...so that this create cannot conflict. + env.lb.EXPECT().NewCreateLoadBalancerRuleParams("roundrobin", "atestuid-tcp-80", 30001, 80).Return(&cloudstack.CreateLoadBalancerRuleParams{}), + env.lb.EXPECT().CreateLoadBalancerRule(gomock.Any()).Return(&cloudstack.CreateLoadBalancerRuleResponse{ + Id: "rule-new", + Name: "atestuid-tcp-80", + Publicip: "10.0.0.1", + Publicipid: "ip-1", + Protocol: "tcp", + }, nil), + env.lb.EXPECT().NewAssignToLoadBalancerRuleParams("rule-new").Return(&cloudstack.AssignToLoadBalancerRuleParams{}), + env.lb.EXPECT().AssignToLoadBalancerRule(gomock.Any()).Return(&cloudstack.AssignToLoadBalancerRuleResponse{}, nil), + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{matchingFirewallRule}, + }, nil), + ) + + if _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("udp rule blocks a tcp create on the same port", func(t *testing.T) { + // CloudStack rejects two load balancer rules with overlapping ports on one IP whatever + // their protocols, so an obsolete udp/80 rule must be pruned before the tcp/80 create + // even though the two never match each other. Its udp firewall rule is not claimed by + // the desired tcp port, so that goes too. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{tcpPort80}) + env.expectHostsAndNetwork() + + udpRule := existingTCPRule() + udpRule.Id = "rule-udp" + udpRule.Name = "atestuid-udp-80" + udpRule.Protocol = "udp" + + udpFirewallRule := &cloudstack.FirewallRule{ + Id: "fw-udp", Protocol: "udp", Startport: 80, Endport: 80, Cidrlist: defaultAllowedCIDR, + } + + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{udpRule}, + }, nil), + + // Prune first: the udp firewall rule, then the udp load balancer rule. + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{udpFirewallRule}, + }, nil), + env.firewall.EXPECT().NewDeleteFirewallRuleParams("fw-udp").Return(&cloudstack.DeleteFirewallRuleParams{}), + env.firewall.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&cloudstack.DeleteFirewallRuleResponse{}, nil), + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-udp").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + + // Only then can the tcp rule be created on the freed port. + env.lb.EXPECT().NewCreateLoadBalancerRuleParams("roundrobin", "atestuid-tcp-80", 30000, 80).Return(&cloudstack.CreateLoadBalancerRuleParams{}), + env.lb.EXPECT().CreateLoadBalancerRule(gomock.Any()).Return(&cloudstack.CreateLoadBalancerRuleResponse{ + Id: "rule-tcp", Name: "atestuid-tcp-80", Publicip: "10.0.0.1", Publicipid: "ip-1", Protocol: "tcp", + }, nil), + env.lb.EXPECT().NewAssignToLoadBalancerRuleParams("rule-tcp").Return(&cloudstack.AssignToLoadBalancerRuleParams{}), + env.lb.EXPECT().AssignToLoadBalancerRule(gomock.Any()).Return(&cloudstack.AssignToLoadBalancerRuleResponse{}, nil), + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{}, nil), + env.firewall.EXPECT().NewCreateFirewallRuleParams("ip-1", "tcp").Return(&cloudstack.CreateFirewallRuleParams{}), + env.firewall.EXPECT().CreateFirewallRule(gomock.Any()).Return(&cloudstack.CreateFirewallRuleResponse{}, nil), + ) + + if _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("unparseable leftover rule does not block reconciliation", func(t *testing.T) { + // A rule the provider cannot interpret must be skipped, not abort the whole sync: + // the desired ports still have to be reconciled. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{tcpPort80}) + env.expectHostsAndNetwork() + + junkRule := existingTCPRule() + junkRule.Id = "rule-junk" + junkRule.Name = "atestuid-http-8080" + junkRule.Protocol = "http" + junkRule.Publicport = "8080" + + // No delete expectation for rule-junk: it is skipped, not deleted. + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 2, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{junkRule, existingTCPRule()}, + }, nil), + ) + + // The desired tcp/80 rule is still reconciled. + gomock.InOrder( + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{matchingFirewallRule}, + }, nil), + ) + + if _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("prune failure still reconciles desired rules", func(t *testing.T) { + // A failed delete is still reported, even though the desired rules were applied fine. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{tcpPort80}) + env.expectHostsAndNetwork() + + obsolete := existingTCPRule() + obsolete.Id = "rule-obsolete" + obsolete.Name = "atestuid-tcp-8080" + obsolete.Publicport = "8080" + + deleteErr := fmt.Errorf("delete rule API error") + + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 2, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{obsolete, existingTCPRule()}, + }, nil), + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-obsolete").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(nil, deleteErr), + ) + + gomock.InOrder( + // Apply pass: the desired tcp/80 firewall rule already matches. + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{matchingFirewallRule}, + }, nil), + // Prune pass: nothing matches the obsolete port 8080. + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{}, nil), + ) + + _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes) + if err == nil { + t.Fatalf("expected the prune failure to be reported") + } + if !strings.Contains(err.Error(), "delete rule API error") { + t.Errorf("error = %v, want it to mention the delete failure", err) + } + }) + + t.Run("obsolete rule on another IP has firewall rules deleted", func(t *testing.T) { + // An obsolete rule on another public IP shares (tcp, 80) with a desired port. Claims + // are per IP, so the old IP's firewall rule must still be deleted. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{tcpPort80}) + env.expectHostsAndNetwork() + + oldIPRule := existingTCPRule() + oldIPRule.Id = "rule-9" + oldIPRule.Name = "atestuid-tcp-80-old" + oldIPRule.Publicip = "10.0.0.2" + oldIPRule.Publicipid = "ip-2" + + oldIPFirewallRule := &cloudstack.FirewallRule{ + Id: "fw-2", + Protocol: "tcp", + Startport: 80, + Endport: 80, + Cidrlist: defaultAllowedCIDR, + } + + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + // Listed last so the load balancer resolves to ip-1. + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 2, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{oldIPRule, existingTCPRule()}, + }, nil), + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-9").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + ) + + gomock.InOrder( + // Apply pass: the kept rule's firewall rule already matches. + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{matchingFirewallRule}, + }, nil), + // Prune pass: the old IP's rule is unclaimed, so it is deleted. + env.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + env.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{ + Count: 1, + FirewallRules: []*cloudstack.FirewallRule{oldIPFirewallRule}, + }, nil), + env.firewall.EXPECT().NewDeleteFirewallRuleParams("fw-2").Return(&cloudstack.DeleteFirewallRuleParams{}), + env.firewall.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&cloudstack.DeleteFirewallRuleResponse{}, nil), + ) + + _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) +} diff --git a/cloudstack_test.go b/cloudstack_test.go index 87ed02fd..c5ae31ff 100644 --- a/cloudstack_test.go +++ b/cloudstack_test.go @@ -160,6 +160,67 @@ func TestGetManagementServerVersion(t *testing.T) { } }) + t.Run("returns parsed version for short version strings", func(t *testing.T) { + // A version with fewer than three parts must not slice out of range. + for _, tc := range []struct{ reported, want string }{ + {reported: "4.22", want: "4.22.0"}, + {reported: "4", want: "4.0.0"}, + } { + ctrl := gomock.NewController(t) + mockMgmt := cloudstack.NewMockManagementServiceIface(ctrl) + params := &cloudstack.ListManagementServersMetricsParams{} + + gomock.InOrder( + mockMgmt.EXPECT().NewListManagementServersMetricsParams().Return(params), + mockMgmt.EXPECT().ListManagementServersMetrics(params).Return(&cloudstack.ListManagementServersMetricsResponse{ + Count: 1, + ManagementServersMetrics: []*cloudstack.ManagementServersMetric{ + {Version: tc.reported}, + }, + }, nil), + ) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{Management: mockMgmt}, + } + + version, err := cs.getManagementServerVersion() + if err != nil { + t.Fatalf("version %q: unexpected error: %v", tc.reported, err) + } + if want := semver.MustParse(tc.want); !version.Equals(want) { + t.Errorf("version %q parsed to %v, want %v", tc.reported, version, want) + } + ctrl.Finish() + } + }) + + t.Run("returns error for an empty version string", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + mockMgmt := cloudstack.NewMockManagementServiceIface(ctrl) + params := &cloudstack.ListManagementServersMetricsParams{} + + gomock.InOrder( + mockMgmt.EXPECT().NewListManagementServersMetricsParams().Return(params), + mockMgmt.EXPECT().ListManagementServersMetrics(params).Return(&cloudstack.ListManagementServersMetricsResponse{ + Count: 1, + ManagementServersMetrics: []*cloudstack.ManagementServersMetric{ + {Version: ""}, + }, + }, nil), + ) + + cs := &CSCloud{ + client: &cloudstack.CloudStackClient{Management: mockMgmt}, + } + + if _, err := cs.getManagementServerVersion(); err == nil { + t.Fatalf("expected an error for an empty version string") + } + }) + t.Run("returns correct parsed version with development server", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish)
