diff --git a/README.md b/README.md index 316e8e33..cf936d2a 100644 --- a/README.md +++ b/README.md @@ -95,6 +95,9 @@ 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 service updates the CloudStack load balancer rule in place: +the rule keeps its identity and its public port is not interrupted. + **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:** @@ -389,13 +392,23 @@ account when migrating from the old cloud provider to the standalone controller. ### Load Balancer -Load balancer rule names now include the protocol in addition to the LB name and service port. -This was added to distinguish tcp, udp and tcp-proxy services operating on the same port. -Without this change, it would not be possible to map a service that runs on both TCP and UDP port 8000, for example. +Load balancer rule names now include the protocol in addition to the LB name and service port, so +that a rule identifies the protocol it serves. The controller keeps the name in step with the +protocol, so a rule renamed from `-tcp-` to `-tcp-proxy-` reflects a protocol change rather than a +new rule. + +Note that CloudStack rejects two load balancer rules with overlapping port ranges on the same public +IP regardless of their protocols, so a service cannot expose the same port over both TCP and UDP +through one IP address. Use separate services on separate IPs for that. + +A rule created by the old provider carries no protocol in its name. The controller adopts such a +rule on the first reconcile, matching it by public IP, IP protocol and public port, and renames it +to the current scheme. Existing rules therefore do not have to be removed before migrating. -:warning: **If you have existing rules, remove them before the migration, and add them back afterwards.** +:warning: **Rules of a Service that is recreated during the migration are not adopted.** -If you don't do this, you will end up with duplicate rules for the same service, which won't work. +Rule names derive from the Service UID, so a Service recreated with a new UID leaves its old rules +behind. Remove those rules and release their public IPs by hand. ### Metadata diff --git a/cloudstack_loadbalancer.go b/cloudstack_loadbalancer.go index 899d3186..9e27f4f4 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 @@ -70,9 +76,36 @@ type loadBalancer struct { projectID string rules map[string]*cloudstack.LoadBalancerRule duplicateRules []*cloudstack.LoadBalancerRule + networks map[string]*cloudstack.Network ipAssociatedByController bool } +// ruleChange is what applying a desired service port does to its load balancer rule. +type ruleChange int + +const ( + ruleMissing ruleChange = iota // no rule exists, so one is created + ruleUpToDate // the existing rule is left alone + ruleNeedsUpdate // the existing rule is updated in place + ruleNeedsRecreate // the existing rule is deleted and created again +) + +// desiredLBRule describes the load balancer rule a service port should be represented by, +// together with the existing CloudStack rule it resolved to (if any) and what applying it does. +type desiredLBRule struct { + name string + port corev1.ServicePort + protocol LoadBalancerProtocol + existing *cloudstack.LoadBalancerRule // nil when change is ruleMissing + change ruleChange +} + +// createsRule reports whether applying this port creates a load balancer rule, and so needs +// its public port free of any other rule first. +func (d desiredLBRule) createsRule() bool { + return d.change == ruleMissing || d.change == ruleNeedsRecreate +} + // 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) @@ -157,96 +190,33 @@ 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) - } + 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, desired, 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, desired, network); err != nil { + return nil, err } status = &corev1.LoadBalancerStatus{} @@ -485,7 +455,9 @@ func (cs *CSCloud) getLoadBalancer(service *corev1.Service) (*loadBalancer, erro lbRules = dedupeByID(lbRules, func(rule *cloudstack.LoadBalancerRule) string { return rule.Id }) // Keeping the rule on the address the Service is already published on stops a - // duplicate sweep from deleting the rule that clients and DNS are pointing at. + // duplicate sweep from deleting the rule that clients and DNS are pointing at. The + // same address is the one the rules are reconciled towards when they span several. + // Without one, the first-listed rule wins here as it does in the sweep. preferredIP := service.Spec.LoadBalancerIP if preferredIP == "" && len(service.Status.LoadBalancer.Ingress) > 0 { preferredIP = service.Status.LoadBalancer.Ingress[0].IP @@ -509,8 +481,10 @@ func (cs *CSCloud) getLoadBalancer(service *corev1.Service) (*loadBalancer, erro klog.Warningf("Load balancer for service %v/%v has rules associated with different IP's: %v, %v", service.Namespace, service.Name, lb.ipAddr, lbRule.Publicip) } - lb.ipAddr = lbRule.Publicip - lb.ipAddrID = lbRule.Publicipid + if lb.ipAddr == "" || (lbRule.Publicip == preferredIP && lb.ipAddr != preferredIP) { + lb.ipAddr = lbRule.Publicip + lb.ipAddrID = lbRule.Publicipid + } } klog.V(4).Infof("Load balancer %v contains %d rule(s)", lb.name, len(lb.rules)) @@ -711,61 +685,442 @@ 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 { - return nil, false, nil +// 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. It changes nothing in CloudStack: a rule +// that has to be recreated is only deleted when it is applied, so an error on a later port, or +// anywhere before the apply phase, leaves the existing rule serving. +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 := lb.findLoadBalancerRule(lbRuleName, port, protocol) + change, err := lb.checkLoadBalancerRule(lbRule, 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, + change: change, + }) } - cidrList, err := lb.getCIDRList(service) + 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 +} + +// 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.createsRule() { + 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] + + 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 + } + + // Conflicts are per public IP, so only a rule on the IP being reconciled towards can + // block a create. + blocksACreate := lbRule.Publicipid == lb.ipAddrID && neededPorts[int32(port)] + + // A protocol the provider cannot interpret leaves its firewall or ACL rule + // unresolvable, so such a rule is normally left alone. One holding a port a create + // needs still has to go, or CloudStack rejects that create as a port conflict. + protocol := ProtocolFromLoadBalancer(lbRule.Protocol) + if protocol == LoadBalancerProtocolInvalid && !blocksACreate { + klog.Errorf("Skipping obsolete load balancer rule %v with unknown protocol %v", lbRule.Name, lbRule.Protocol) + continue + } + + obsolete := obsoleteRule{ + rule: lbRule, + protocol: protocol, + tuple: portProtocol{protocol.IPProtocol(), int32(port)}, + } + + if blocksACreate { + blocking = append(blocking, obsolete) + } else { + rest = append(rest, obsolete) + } + } + + return blocking, rest +} + +// ruleNetworkID is the network whose ACL rules an existing load balancer rule was opened in, or +// "" when that network cannot be established. CloudStack omits the network on rules of some +// network types; such a rule is known to belong to the network being reconciled towards only +// when it sits on the public IP being reconciled towards. +func (lb *loadBalancer) ruleNetworkID(lbRule *cloudstack.LoadBalancerRule) string { + if lbRule.Networkid != "" { + return lbRule.Networkid + } + if lbRule.Publicipid == lb.ipAddrID { + return lb.networkID + } + return "" +} + +// claimedTuples are the tuples the desired service ports still need, and whose firewall or +// network ACL rules therefore have to survive a prune. +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 +} + +// pruneFirewallRule deletes the firewall rule admitting traffic to an obsolete load balancer +// rule. Firewall rules belong to a single public IP, so a claim only covers a rule on the IP +// the service is being reconciled towards. +func (lb *loadBalancer) pruneFirewallRule(o obsoleteRule, claimed map[portProtocol]bool) error { + lbRule, port := o.rule, int(o.tuple.publicPort) + + 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, o.protocol, lbRule.Publicip, port) + return nil + } + + klog.V(4).Infof("Deleting firewall rules associated with load balancer rule: %v (%v:%v:%v)", lbRule.Name, o.protocol, lbRule.Publicip, port) + _, err := lb.deleteFirewallRule(lbRule.Publicipid, port, o.protocol) + return err +} + +// pruneNetworkACLRule deletes the network ACL rule admitting traffic to an obsolete load +// balancer rule, in the network that rule belongs to. ACL rules belong to a network rather than +// an IP, so a claim only covers a rule in the network the service is being reconciled towards. +func (lb *loadBalancer) pruneNetworkACLRule(o obsoleteRule, claimed map[portProtocol]bool, networkID string) error { + lbRule, port := o.rule, int(o.tuple.publicPort) + + if claimed[o.tuple] && networkID == lb.networkID { + klog.V(4).Infof("Keeping Network ACL rules of obsolete load balancer rule %v (%v:%v:%v): still claimed by a service port", lbRule.Name, o.protocol, networkID, port) + return nil + } + + klog.V(4).Infof("Deleting Network ACL rules associated with load balancer rule: %v (%v:%v:%v)", lbRule.Name, o.protocol, networkID, port) + _, err := lb.deleteNetworkACLRule(port, o.protocol, networkID) + return err +} + +// rememberNetwork records a network already fetched, so resolving the network of a rule in it +// costs no further call. +func (lb *loadBalancer) rememberNetwork(networkID string, network *cloudstack.Network) { + if lb.networks == nil { + lb.networks = make(map[string]*cloudstack.Network) + } + lb.networks[networkID] = network +} + +// networkByID is the network with the given ID, or nil when CloudStack no longer has it. An +// empty ID is nil rather than a lookup, which GetNetworkByID would answer with an arbitrary +// network from an unfiltered list. GetNetworkByID reports not-found as an error with a count of +// 0, so the count has to be checked before the error. +func (lb *loadBalancer) networkByID(networkID string) (*cloudstack.Network, error) { + if networkID == "" { + return nil, nil + } + if network, ok := lb.networks[networkID]; ok { + return network, nil + } + + network, count, err := lb.Network.GetNetworkByID(networkID, cloudstack.WithProject(lb.projectID)) + switch { + case count == 0: + network = nil + case err != nil: + return nil, fmt.Errorf("error fetching network %v: %v", networkID, err) + } + lb.rememberNetwork(networkID, network) + + return network, nil +} + +// ruleNetwork is the network an existing load balancer rule was created in, or nil when that +// network cannot be established, either because CloudStack reported no network for the rule or +// because the network has since been deleted. +func (lb *loadBalancer) ruleNetwork(lbRule *cloudstack.LoadBalancerRule) (*cloudstack.Network, error) { + return lb.networkByID(lb.ruleNetworkID(lbRule)) +} + +// pruneRuleOpening deletes the firewall or network ACL rule admitting traffic to an obsolete +// load balancer rule, unless a desired service port still claims that same opening. Which of +// the two a rule has follows the network that rule belongs to, not the one being reconciled +// towards, so a rule left behind in a network of the other kind does not keep its opening. +// +// A rule with an uninterpretable protocol keeps its opening, which cannot be identified without +// one. A rule whose network cannot be established still has its firewall rule deleted, that +// being scoped to the rule's own public IP, while any ACL rule is left in place. +func (lb *loadBalancer) pruneRuleOpening(o obsoleteRule, claimed map[portProtocol]bool) error { + if o.protocol == LoadBalancerProtocolInvalid { + klog.Warningf("Leaving the firewall or Network ACL rule of obsolete load balancer rule %v in place: unknown protocol %v", o.rule.Name, o.rule.Protocol) + return nil + } + + network, err := lb.ruleNetwork(o.rule) if err != nil { - return nil, false, err + return err } - var lbRuleCidrList []string - if lbRule.Cidrlist != "" { - lbRuleCidrList = strings.Split(lbRule.Cidrlist, " ") - for i, cidr := range lbRuleCidrList { - cidr = strings.TrimSpace(cidr) - lbRuleCidrList[i] = cidr + switch { + case network == nil: + klog.Warningf("Cannot establish the network of obsolete load balancer rule %v; leaving any Network ACL rule of it in place", o.rule.Name) + return lb.pruneFirewallRule(o, claimed) + case isFirewallSupported(network.Service): + return lb.pruneFirewallRule(o, claimed) + case isNetworkACLSupported(network.Service): + return lb.pruneNetworkACLRule(o, claimed, network.Id) + } + + return nil +} + +// 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. +// +// The network being reconciled towards is taken as already fetched, so only a rule belonging to +// some other network costs a lookup of its own. +// +// A rule that fails to delete is reported but does not stop the others being pruned. +func (lb *loadBalancer) pruneRules(obsolete []obsoleteRule, desired []desiredLBRule, network *cloudstack.Network) error { + lb.rememberNetwork(network.Id, network) + claimed := claimedTuples(desired) + + 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 { + if err := lb.pruneRuleOpening(o, claimed); err != nil { + recordErr(err) + continue + } + + klog.V(4).Infof("Deleting obsolete load balancer rule: %v", o.rule.Name) + if err := lb.deleteLoadBalancerRule(o.rule); err != nil { + recordErr(err) + } + } + + return firstErr +} + +// ensureLoadBalancerRule brings the load balancer rule of one desired service port in line and +// returns it: an up-to-date rule is left alone, an outdated one is updated in place, and a +// missing one is created. A rule that cannot be updated is deleted immediately before its +// replacement is created, which keeps the port unserved for as short a time as possible. +func (lb *loadBalancer) ensureLoadBalancerRule(d desiredLBRule, service *corev1.Service, version semver.Version) (*cloudstack.LoadBalancerRule, error) { + switch d.change { + case ruleUpToDate: + klog.V(4).Infof("Load balancer rule %v is up-to-date", d.name) + return d.existing, nil + case ruleNeedsUpdate: + klog.V(4).Infof("Updating load balancer rule: %v", d.name) + return d.existing, lb.updateLoadBalancerRule(d.existing, d.name, d.protocol, service, version) + case ruleNeedsRecreate: + klog.V(4).Infof("Deleting load balancer rule %v so it can be created again", d.existing.Name) + if err := lb.deleteLoadBalancerRule(d.existing); err != nil { + return nil, err + } + } + + klog.V(4).Infof("Creating load balancer rule: %v", d.name) + lbRule, err := lb.createLoadBalancerRule(d.name, d.port, d.protocol, service) + if err != nil { + return nil, err + } + + 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 nil, err + } + + return lbRule, nil +} + +// 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, err := lb.ensureLoadBalancerRule(d, service, version) + if 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 +} + +// effectiveSourceCIDRs is the source CIDR list a load balancer rule enforces. An empty list +// allows every source, since the virtual router's HAProxy configuration adds no source filter +// for it. Rules created by the in-tree provider, and by this one before it sent a CIDR list, +// report an empty list, and must compare equal to an unrestricted Service rather than be +// recreated on every CloudStack release that cannot update the list in place. +func effectiveSourceCIDRs(cidrs []string) []string { + if len(cidrs) == 0 { + return []string{defaultAllowedCIDR} + } + return cidrs +} + +// checkLoadBalancerRule decides what applying a service port does to the given existing rule +// (nil if none was found): nothing, an update call, or deleting and creating it again. It makes +// no CloudStack call. +func (lb *loadBalancer) checkLoadBalancerRule(lbRule *cloudstack.LoadBalancerRule, lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) (ruleChange, error) { + if lbRule == nil { + return ruleMissing, nil + } + + cidrList, err := lb.getCIDRList(service) + if err != nil { + return ruleMissing, err + } + // 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) || !compareStringSlice(cidrList, lbRuleCidrList) + cidrListChanged := !compareStringSlice(effectiveSourceCIDRs(cidrList), effectiveSourceCIDRs(splitCIDRList(lbRule.Cidrlist))) + 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})) { - // 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 + // A CIDR change on an older CloudStack can only be applied by recreating the rule. + if !basicPropsMatch || (cidrListChanged && version.LT(cidrListUpdateVersion)) { + return ruleNeedsRecreate, nil } - // 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 + if updateAlgo || updateProto || updateName || cidrListChanged { + return ruleNeedsUpdate, nil + } + return ruleUpToDate, 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 @@ -1098,7 +1453,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 @@ -1196,7 +1551,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 1eba6e34..7933fcca 100644 --- a/cloudstack_loadbalancer_test.go +++ b/cloudstack_loadbalancer_test.go @@ -589,256 +589,338 @@ func TestGetCIDRList(t *testing.T) { } func TestCheckLoadBalancerRule(t *testing.T) { - t.Run("rule not present returns nil", func(t *testing.T) { - lb := &loadBalancer{ - rules: map[string]*cloudstack.LoadBalancerRule{}, - } - port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} - service := &corev1.Service{} + port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} + belowCIDRUpdate := semver.MustParse("4.21.0") + withCIDRUpdate := semver.MustParse("4.22.0") - rule, needsUpdate, err := lb.checkLoadBalancerRule("missing", port, LoadBalancerProtocolTCP, service, semver.Version{}) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if rule != nil { - t.Fatalf("expected nil rule, got %v", rule) + existingRule := func(edit func(*cloudstack.LoadBalancerRule)) *cloudstack.LoadBalancerRule { + lbRule := &cloudstack.LoadBalancerRule{ + Id: "rule-id", + Name: "rule", + Publicip: "1.1.1.1", + Privateport: "30000", + Publicport: "80", + Cidrlist: defaultAllowedCIDR, + Algorithm: "roundrobin", + Protocol: LoadBalancerProtocolTCP.CSProtocol(), } - if needsUpdate { - t.Fatalf("expected needsUpdate to be false") + if edit != nil { + edit(lbRule) } - }) + return lbRule + } + unrestricted := &corev1.Service{} + restrictedTo := func(cidrs string) *corev1.Service { + return &corev1.Service{ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{ + ServiceAnnotationLoadBalancerSourceCidrs: cidrs, + }}} + } - t.Run("basic property mismatch deletes rule", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) + tests := []struct { + name string + existing *cloudstack.LoadBalancerRule + ruleName string + protocol LoadBalancerProtocol + service *corev1.Service + version semver.Version + want ruleChange + }{ + { + name: "no existing rule is created", + service: unrestricted, + version: withCIDRUpdate, + want: ruleMissing, + }, + { + name: "matching rule is left alone", + existing: existingRule(nil), + service: unrestricted, + version: withCIDRUpdate, + want: ruleUpToDate, + }, + { + name: "rule on another public IP is recreated", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Publicip = "2.2.2.2" }), + service: unrestricted, + version: withCIDRUpdate, + want: ruleNeedsRecreate, + }, + { + name: "changed node port is recreated", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Privateport = "30001" }), + service: unrestricted, + version: withCIDRUpdate, + want: ruleNeedsRecreate, + }, + { + name: "changed algorithm is updated", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Algorithm = "source" }), + service: unrestricted, + version: withCIDRUpdate, + want: ruleNeedsUpdate, + }, + { + name: "protocol change is updated in place", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Name = "rule-tcp-80" }), + ruleName: "rule-tcp-proxy-80", + protocol: LoadBalancerProtocolTCPProxy, + service: unrestricted, + version: withCIDRUpdate, + want: ruleNeedsUpdate, + }, + { + name: "cidr change is updated where the API accepts a cidrlist", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Cidrlist = "10.0.0.0/8" }), + service: restrictedTo("10.0.0.0/8,192.168.0.0/16"), + version: withCIDRUpdate, + want: ruleNeedsUpdate, + }, + // CloudStack moves from 4.x to 24.0 after 4.23, so the 4.22 feature gate has + // to keep treating the new numbering as newer rather than older. + { + name: "cidr change is updated on the 24.0 series", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Cidrlist = "10.0.0.0/8" }), + service: restrictedTo("10.0.0.0/8,192.168.0.0/16"), + version: semver.MustParse("24.0.0"), + want: ruleNeedsUpdate, + }, + { + name: "cidr change is recreated below the cidrlist update release", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Cidrlist = "10.0.0.0/8" }), + service: restrictedTo("10.0.0.0/8,192.168.0.0/16"), + version: belowCIDRUpdate, + want: ruleNeedsRecreate, + }, + { + name: "matching multi-CIDR list is left alone", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Cidrlist = "10.0.0.0/8,192.168.0.0/16" }), + service: restrictedTo("10.0.0.0/8,192.168.0.0/16"), + version: withCIDRUpdate, + want: ruleUpToDate, + }, + { + name: "rule with no cidrlist matches an unrestricted service", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Cidrlist = "" }), + service: unrestricted, + version: belowCIDRUpdate, + want: ruleUpToDate, + }, + { + name: "rule with no cidrlist is recreated for a restricted service below the update release", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Cidrlist = "" }), + service: restrictedTo("10.0.0.0/8"), + version: belowCIDRUpdate, + want: ruleNeedsRecreate, + }, + { + name: "rule with no cidrlist is updated for a restricted service", + existing: existingRule(func(r *cloudstack.LoadBalancerRule) { r.Cidrlist = "" }), + service: restrictedTo("10.0.0.0/8"), + version: withCIDRUpdate, + want: ruleNeedsUpdate, + }, + } - mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) - deleteParams := &cloudstack.DeleteLoadBalancerRuleParams{} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + // No expectations: deciding a change must not call CloudStack. + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{LoadBalancer: cloudstack.NewMockLoadBalancerServiceIface(ctrl)}, + ipAddr: "1.1.1.1", + algorithm: "roundrobin", + } + ruleName := tt.ruleName + if ruleName == "" { + ruleName = "rule" + } - gomock.InOrder( - mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-id").Return(deleteParams), - mockLB.EXPECT().DeleteLoadBalancerRule(deleteParams).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), - ) + got, err := lb.checkLoadBalancerRule(tt.existing, ruleName, port, tt.protocol, tt.service, tt.version) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tt.want { + t.Errorf("checkLoadBalancerRule = %v, want %v", got, tt.want) + } + }) + } - lb := &loadBalancer{ - CloudStackClient: &cloudstack.CloudStackClient{ - LoadBalancer: mockLB, - }, - ipAddr: "1.1.1.1", - rules: map[string]*cloudstack.LoadBalancerRule{ - "rule": { - Id: "rule-id", - Name: "rule", - Publicip: "2.2.2.2", - Privateport: "30000", - Publicport: "80", - Cidrlist: defaultAllowedCIDR, - Algorithm: "roundrobin", - Protocol: LoadBalancerProtocolTCP.CSProtocol(), - }, - }, - } - port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} - service := &corev1.Service{} + t.Run("invalid cidr returns error", func(t *testing.T) { + lb := &loadBalancer{ipAddr: "1.1.1.1", algorithm: "roundrobin"} - rule, needsUpdate, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 21, Patch: 0}) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if rule != nil { - t.Fatalf("expected nil rule after deletion, got %v", rule) - } - if needsUpdate { - t.Fatalf("expected needsUpdate to be false") - } - if _, exists := lb.rules["rule"]; exists { - t.Fatalf("expected rule entry to be removed from map") + if _, err := lb.checkLoadBalancerRule(existingRule(nil), "rule", port, LoadBalancerProtocolTCP, restrictedTo("bad-cidr"), withCIDRUpdate); err == nil { + t.Fatalf("expected error for invalid CIDR") } }) +} - t.Run("cidr change triggers update on supported version", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) +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"}, + }, + } - // No expectations on the mock; any delete call would fail the test. - mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + 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]) + } + } + }) + } +} - lbRule := &cloudstack.LoadBalancerRule{ - Id: "rule-id", - Name: "rule", - Publicip: "1.1.1.1", - Privateport: "30000", - Publicport: "80", - Cidrlist: "10.0.0.0/8", - Algorithm: "roundrobin", - Protocol: LoadBalancerProtocolTCP.CSProtocol(), - } +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{ - 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", - }, - }, + ipAddr: "10.0.0.1", + ipAddrID: "ip-1", + rules: map[string]*cloudstack.LoadBalancerRule{}, } - - rule, needsUpdate, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) - if err != nil { - t.Fatalf("unexpected error: %v", err) + for _, r := range rules { + lb.rules[r.Name] = r } - if rule != lbRule { - t.Fatalf("expected existing rule to be returned") + 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", } - if !needsUpdate { - t.Fatalf("expected needsUpdate to be true due to CIDR change") + } + + 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) } }) - // CloudStack moves from 4.x to 24.0 after 4.23, so the 4.22 feature gate has - // to keep treating the new numbering as newer rather than older. - t.Run("cidr change triggers update on the 24.0 series", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - // No expectations on the mock; any delete call would fail the test. - mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + 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) - lbRule := &cloudstack.LoadBalancerRule{ - Id: "rule-id", - Name: "rule", - Publicip: "1.1.1.1", - Privateport: "30000", - Publicport: "80", - Cidrlist: "10.0.0.0/8", - Algorithm: "roundrobin", - Protocol: LoadBalancerProtocolTCP.CSProtocol(), + if got := lb.findLoadBalancerRule("lb-tcp-proxy-80", port80, LoadBalancerProtocolTCPProxy); got != tcpRule { + t.Fatalf("findLoadBalancerRule = %v, want fallback match %v", got, tcpRule) } + }) - 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", - }, - }, + 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) } + }) - rule, needsUpdate, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.MustParse("24.0.0")) - if err != nil { - t.Fatalf("unexpected error: %v", err) + 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) } - if rule != lbRule { - t.Fatalf("expected existing rule to be returned") + }) + + 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) } - if !needsUpdate { - t.Fatalf("expected needsUpdate to be true due to CIDR change") + // 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("cidr change triggers delete with older version", func(t *testing.T) { - ctrl := gomock.NewController(t) - t.Cleanup(ctrl.Finish) - - // No expectations on the mock; any delete or create call would fail the test. - mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + t.Run("port mismatch returns nil", func(t *testing.T) { + lb := newLB(rule("lb-tcp-443", "tcp", "443")) - deleteParams := &cloudstack.DeleteLoadBalancerRuleParams{} + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != nil { + t.Fatalf("findLoadBalancerRule = %v, want nil", got) + } + }) - gomock.InOrder( - mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-id").Return(deleteParams), - mockLB.EXPECT().DeleteLoadBalancerRule(deleteParams).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), - ) + 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) - lbRule := &cloudstack.LoadBalancerRule{ - Id: "rule-id", - Name: "rule", - Publicip: "1.1.1.1", - Privateport: "30000", - Publicport: "80", - Cidrlist: "10.0.0.0/8", - Algorithm: "roundrobin", - Protocol: LoadBalancerProtocolTCP.CSProtocol(), + if got := lb.findLoadBalancerRule("lb-tcp-80", port80, LoadBalancerProtocolTCP); got != nil { + t.Fatalf("findLoadBalancerRule = %v, want nil for a rule on another IP", got) } + }) - 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", - }, - }, - } + 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) - rule, needsUpdate, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 12, Patch: 0}) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if rule != nil { - t.Fatalf("expected nil rule after deletion, got %v", rule) - } - if needsUpdate { - t.Fatalf("expected needsUpdate to be false due to CIDR change with older version") + // 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("invalid cidr returns error", func(t *testing.T) { - lb := &loadBalancer{ - rules: map[string]*cloudstack.LoadBalancerRule{ - "rule": { - Id: "rule-id", - Name: "rule", - Publicip: "1.1.1.1", - Privateport: "30000", - Publicport: "80", - Cidrlist: defaultAllowedCIDR, - Algorithm: "roundrobin", - Protocol: LoadBalancerProtocolTCP.CSProtocol(), - }, - }, - } - port := corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP} - service := &corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Annotations: map[string]string{ - ServiceAnnotationLoadBalancerSourceCidrs: "bad-cidr", - }, - }, + 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) + } } + }) - _, _, err := lb.checkLoadBalancerRule("rule", port, LoadBalancerProtocolTCP, service, semver.Version{Major: 4, Minor: 22, Patch: 0}) - if err == nil { - t.Fatalf("expected error for invalid CIDR") + 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) } }) } @@ -2048,10 +2130,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) { @@ -2082,10 +2167,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) { @@ -2123,10 +2215,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"}) + } }) // The release after 4.23 is numbered 24.0, which must still reach the @@ -2166,7 +2261,7 @@ func TestUpdateLoadBalancerRule(t *testing.T) { }, } - if err := lb.updateLoadBalancerRule("test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.MustParse("24.0.0")); err != nil { + if err := lb.updateLoadBalancerRule(lb.rules["test-rule-tcp-80"], "test-rule-tcp-80", LoadBalancerProtocolTCP, service, semver.MustParse("24.0.0")); err != nil { t.Fatalf("unexpected error: %v", err) } @@ -2208,7 +2303,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") } @@ -3048,7 +3143,7 @@ func TestUpdateNetworkACL(t *testing.T) { } }) - t.Run("rule already exists", func(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) @@ -3067,15 +3162,13 @@ func TestUpdateNetworkACL(t *testing.T) { listParams := &cloudstack.ListNetworkACLsParams{} listResp := &cloudstack.ListNetworkACLsResponse{ - Count: 1, - NetworkACLs: []*cloudstack.NetworkACL{ - { - Id: "acl-rule-123", - Protocol: "tcp", - Startport: "80", - Endport: "80", - }, - }, + Count: 0, + NetworkACLs: []*cloudstack.NetworkACL{}, + } + + createParams := &cloudstack.CreateNetworkACLParams{} + createResp := &cloudstack.CreateNetworkACLResponse{ + Id: "acl-rule-123", } gomock.InOrder( @@ -3083,6 +3176,9 @@ func TestUpdateNetworkACL(t *testing.T) { mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-456", gomock.Any()).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{ @@ -3092,7 +3188,7 @@ func TestUpdateNetworkACL(t *testing.T) { }, } - updated, err := lb.updateNetworkACL(80, LoadBalancerProtocolTCP, "net-123") + updated, err := lb.updateNetworkACL(80, LoadBalancerProtocolTCPProxy, "net-123") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -3101,7 +3197,7 @@ func TestUpdateNetworkACL(t *testing.T) { } }) - t.Run("default ACL - skip", func(t *testing.T) { + t.Run("tcp-proxy matches existing tcp ACL rule", func(t *testing.T) { ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -3115,7 +3211,114 @@ func TestUpdateNetworkACL(t *testing.T) { aclListResp := &cloudstack.NetworkACLList{ Id: "acl-456", - Name: "default_allow", + 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", gomock.Any()).Return(networkResp, 1, nil), + mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-456", gomock.Any()).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) + + 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", + }, + }, + } + + gomock.InOrder( + mockNetwork.EXPECT().GetNetworkByID("net-123", gomock.Any()).Return(networkResp, 1, nil), + mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-456", gomock.Any()).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, LoadBalancerProtocolTCP, "net-123") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !updated { + t.Errorf("updated = false, want true") + } + }) + + t.Run("default ACL - skip", 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: "default_allow", } gomock.InOrder( @@ -3644,6 +3847,47 @@ func TestGetLoadBalancer(t *testing.T) { lb := listDuplicates(t, onAutoIP.Publicip, onRequestedIP.Publicip, onRequestedIP, onAutoIP) assertKept(t, lb, onAutoIP, onRequestedIP) }) + + // Rules with distinct names are not duplicates, so what settles the IP the service is + // reconciled towards is the preferred address, and without one the first-listed rule, + // matching the duplicate sweep. + onPort80 := &cloudstack.LoadBalancerRule{Id: "rule-80", Name: "test-service-tcp-80", Publicip: "203.0.113.9", Publicipid: "ip-requested"} + onPort443 := &cloudstack.LoadBalancerRule{Id: "rule-443", Name: "test-service-tcp-443", Publicip: "203.0.113.1", Publicipid: "ip-auto"} + + assertResolvedIP := func(t *testing.T, lb *loadBalancer, wantIP, wantIPID string) { + t.Helper() + if lb.ipAddr != wantIP || lb.ipAddrID != wantIPID { + t.Errorf("ipAddr/ipAddrID = %v/%v, want %v/%v", lb.ipAddr, lb.ipAddrID, wantIP, wantIPID) + } + if len(lb.rules) != 2 { + t.Errorf("rules count = %d, want 2", len(lb.rules)) + } + } + + t.Run("the requested IP outranks a stale IP listed after it", func(t *testing.T) { + lb := listDuplicates(t, onPort80.Publicip, "", onPort80, onPort443) + assertResolvedIP(t, lb, onPort80.Publicip, onPort80.Publicipid) + }) + + t.Run("the published ingress IP outranks a stale IP listed after it", func(t *testing.T) { + lb := listDuplicates(t, "", onPort80.Publicip, onPort80, onPort443) + assertResolvedIP(t, lb, onPort80.Publicip, onPort80.Publicipid) + }) + + t.Run("the requested IP outranks a stale IP listed before it", func(t *testing.T) { + lb := listDuplicates(t, onPort80.Publicip, "", onPort443, onPort80) + assertResolvedIP(t, lb, onPort80.Publicip, onPort80.Publicipid) + }) + + t.Run("the first-listed IP wins when none is preferred", func(t *testing.T) { + lb := listDuplicates(t, "", "", onPort443, onPort80) + assertResolvedIP(t, lb, onPort443.Publicip, onPort443.Publicipid) + }) + + t.Run("a preferred IP no rule is on leaves the first-listed IP", func(t *testing.T) { + lb := listDuplicates(t, "198.51.100.7", "", onPort443, onPort80) + assertResolvedIP(t, lb, onPort443.Publicip, onPort443.Publicipid) + }) } func TestGetLoadBalancerDeduplicatesPagedRules(t *testing.T) { @@ -4367,6 +4611,773 @@ func TestVerifyHostsPagination(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--. + 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 +} + +// expectHosts registers the node lookup every run performs before resolving rules. +func (e *ensureLBTestEnv) expectHosts() { + 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) +} + +// expectHostsAndNetwork registers the host and network lookups every run performs. +func (e *ensureLBTestEnv) expectHostsAndNetwork() { + e.expectHosts() + 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("rule named by the old provider is adopted and renamed", func(t *testing.T) { + // Migration from the in-tree provider, whose rule names carried no protocol and whose + // protocol was sent in upper case, which CloudStack before 4.21 stored verbatim. The + // rule must be adopted rather than recreated, so the README can stop asking operators + // to delete their rules before migrating. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{tcpPort80}) + env.expectHostsAndNetwork() + + legacyRule := existingTCPRule() + legacyRule.Id = "rule-legacy" + legacyRule.Name = "atestuid-80" + legacyRule.Protocol = "TCP" + + updateParams := &cloudstack.UpdateLoadBalancerRuleParams{} + + // No create and no 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{legacyRule}, + }, nil), + env.lb.EXPECT().NewUpdateLoadBalancerRuleParams("rule-legacy").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), + ) + + if _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if name, _ := updateParams.GetName(); name != "atestuid-tcp-80" { + t.Errorf("updated name = %q, want %q", name, "atestuid-tcp-80") + } + if proto, _ := updateParams.GetProtocol(); proto != "tcp" { + t.Errorf("updated protocol = %q, want %q", proto, "tcp") + } + }) + + t.Run("rule named by the old provider is renamed in place below the cidrlist update release", func(t *testing.T) { + // In-tree rules carry no cidrlist. That allows every source, so on a release that can + // only apply a CIDR change by recreating the rule it must still count as unchanged. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{tcpPort80}) + env.cs.version = semver.MustParse("4.21.0") + env.expectHostsAndNetwork() + + legacyRule := existingTCPRule() + legacyRule.Id = "rule-legacy" + legacyRule.Name = "atestuid-80" + legacyRule.Protocol = "TCP" + legacyRule.Cidrlist = "" + + updateParams := &cloudstack.UpdateLoadBalancerRuleParams{} + + // No create and no 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{legacyRule}, + }, nil), + env.lb.EXPECT().NewUpdateLoadBalancerRuleParams("rule-legacy").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), + ) + + if _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if name, _ := updateParams.GetName(); name != "atestuid-tcp-80" { + t.Errorf("updated name = %q, want %q", name, "atestuid-tcp-80") + } + }) + + t.Run("an unsupported port leaves a rule that needs recreating in place", func(t *testing.T) { + // Port 80 needs recreating for its new nodePort, and the SCTP port after it fails + // the reconcile. That fails every retry, so deleting port 80 before the error would + // leave it without a rule for good. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + movedPort80 := corev1.ServicePort{Port: 80, NodePort: 30001, Protocol: corev1.ProtocolTCP} + sctpPort := corev1.ServicePort{Port: 9000, NodePort: 30900, Protocol: corev1.ProtocolSCTP} + env := newEnsureLBTestEnv(ctrl, nil, []corev1.ServicePort{movedPort80, sctpPort}) + env.expectHosts() + + // No delete expectation: the existing port 80 rule must survive. + 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), + ) + + if _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes); err == nil { + t.Fatalf("expected the unsupported SCTP port to fail the reconcile") + } + }) + + t.Run("a failure before the apply phase leaves a rule that needs recreating in place", func(t *testing.T) { + 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.expectHosts() + env.network.EXPECT().GetNetworkByID("net-1", gomock.Any()).Return(nil, -1, fmt.Errorf("API unavailable")) + + // No delete expectation: the existing port 80 rule must survive. + 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), + ) + + if _, err := env.cs.EnsureLoadBalancer(context.TODO(), "test-cluster", env.service, env.nodes); err == nil { + t.Fatalf("expected the network lookup failure to fail the reconcile") + } + }) + + t.Run("a rule that needs recreating is replaced in the apply phase", func(t *testing.T) { + 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() + + // The kept firewall rule shows the recreate is not mistaken for an obsolete rule. + 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().NewDeleteLoadBalancerRuleParams("rule-1").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + 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("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 has to be + // recreated. The other rule 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 blocking rule is pruned first... + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-b").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + + // ...and the matched rule, which cannot take a new nodePort, is deleted only + // immediately before its replacement is created. + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-a").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + 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("unparseable rule holding a needed port is pruned first", func(t *testing.T) { + // A rule the provider cannot interpret still occupies its public port, so one sitting + // on a port a create needs has to go before that create, or CloudStack rejects it. + 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-80" + junkRule.Protocol = "http" + + gomock.InOrder( + env.lb.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{}), + env.lb.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{ + Count: 1, + LoadBalancerRules: []*cloudstack.LoadBalancerRule{junkRule}, + }, nil), + + // No firewall expectations for the junk rule: its protocol cannot be resolved, so + // only the load balancer rule itself is deleted, and before the create. + env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-junk").Return(&cloudstack.DeleteLoadBalancerRuleParams{}), + env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil), + + env.lb.EXPECT().NewCreateLoadBalancerRuleParams("roundrobin", "atestuid-tcp-80", 30000, 80).Return(&cloudstack.CreateLoadBalancerRuleParams{}), + env.lb.EXPECT().CreateLoadBalancerRule(gomock.Any()).Return(&cloudstack.CreateLoadBalancerRuleResponse{ + Id: "rule-2", + Name: "atestuid-tcp-80", + 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), + ) + + 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.service.Status.LoadBalancer.Ingress = []corev1.LoadBalancerIngress{{IP: "10.0.0.1"}} + 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 first, yet the published ingress IP makes ip-1 the address reconciled towards. + 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) + } + }) +} + func TestUpdateLoadBalancerPagination(t *testing.T) { // Instances of a load balancer rule are one per load balanced node, so on a // large cluster the un-paged response was truncated and the stale nodes on @@ -4449,3 +5460,214 @@ func TestUpdateLoadBalancerPagination(t *testing.T) { t.Errorf("vm-599 is on the second page and should have been removed, got %v", removed) } } + +// A Network ACL rule is scoped to a network, so an obsolete rule left over in another network +// is not protected by a desired port that happens to share its protocol and port. +func TestPruneRulesScopesNetworkACLsToTheirNetwork(t *testing.T) { + aclNetwork := &cloudstack.Network{ + Id: "net-new", + Service: []cloudstack.NetworkServiceInternal{{Name: "NetworkACL"}}, + } + firewallNetwork := &cloudstack.Network{ + Id: "net-new", + Service: []cloudstack.NetworkServiceInternal{{Name: "Firewall"}}, + } + obsoleteRuleTier := &cloudstack.Network{ + Id: "net-old", + Service: []cloudstack.NetworkServiceInternal{{Name: "NetworkACL"}}, + } + + desired := []desiredLBRule{{ + name: "atestuid-tcp-80", + port: corev1.ServicePort{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP}, + protocol: LoadBalancerProtocolTCP, + }} + desiredOn443 := []desiredLBRule{{ + name: "atestuid-tcp-443", + port: corev1.ServicePort{Port: 443, NodePort: 30443, Protocol: corev1.ProtocolTCP}, + protocol: LoadBalancerProtocolTCP, + }} + + obsoleteIn := func(networkID string) []obsoleteRule { + return []obsoleteRule{{ + rule: &cloudstack.LoadBalancerRule{ + Id: "rule-old", + Name: "atestuid-tcp-80", + Publicipid: "ip-2", + Publicport: "80", + Protocol: "tcp", + Networkid: networkID, + }, + protocol: LoadBalancerProtocolTCP, + tuple: portProtocol{"tcp", 80}, + }} + } + obsoleteWithoutNetworkOn := func(publicIPID string) []obsoleteRule { + obsolete := obsoleteIn("") + obsolete[0].rule.Publicipid = publicIPID + return obsolete + } + + type pruneMocks struct { + acl *cloudstack.MockNetworkACLServiceIface + network *cloudstack.MockNetworkServiceIface + firewall *cloudstack.MockFirewallServiceIface + } + + newLB := func(ctrl *gomock.Controller) (*loadBalancer, *pruneMocks) { + m := &pruneMocks{ + acl: cloudstack.NewMockNetworkACLServiceIface(ctrl), + network: cloudstack.NewMockNetworkServiceIface(ctrl), + firewall: cloudstack.NewMockFirewallServiceIface(ctrl), + } + mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl) + mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-old").Return(&cloudstack.DeleteLoadBalancerRuleParams{}) + mockLB.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{}, nil) + + return &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: mockLB, + NetworkACL: m.acl, + Network: m.network, + Firewall: m.firewall, + }, + networkID: "net-new", + ipAddrID: "ip-current", + }, m + } + + expectACLRuleDeleted := func(m *pruneMocks) *cloudstack.ListNetworkACLsParams { + listParams := &cloudstack.ListNetworkACLsParams{} + gomock.InOrder( + m.acl.EXPECT().NewListNetworkACLsParams().Return(listParams), + m.acl.EXPECT().ListNetworkACLs(gomock.Any()).Return(&cloudstack.ListNetworkACLsResponse{ + Count: 1, + NetworkACLs: []*cloudstack.NetworkACL{ + {Id: "acl-rule-old", Protocol: "tcp", Startport: "80", Endport: "80"}, + }, + }, nil), + m.acl.EXPECT().NewDeleteNetworkACLParams("acl-rule-old").Return(&cloudstack.DeleteNetworkACLParams{}), + m.acl.EXPECT().DeleteNetworkACL(gomock.Any()).Return(&cloudstack.DeleteNetworkACLResponse{}, nil), + ) + return listParams + } + + t.Run("a rule in another network has its ACL rule deleted there", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + lb, m := newLB(ctrl) + m.network.EXPECT().GetNetworkByID("net-old", gomock.Any()).Return(obsoleteRuleTier, 1, nil) + listParams := expectACLRuleDeleted(m) + + if err := lb.pruneRules(obsoleteIn("net-old"), desired, aclNetwork); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if networkID, _ := listParams.GetNetworkid(); networkID != "net-old" { + t.Errorf("ACL rules listed on network %q, want the obsolete rule's own %q", networkID, "net-old") + } + }) + + t.Run("a rule in the reconciled network keeps its claimed ACL rule", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + // No ACL expectations: the desired tcp/80 port still claims that opening. No network + // lookup either: the reconciled network is already known. + lb, _ := newLB(ctrl) + + if err := lb.pruneRules(obsoleteIn("net-new"), desired, aclNetwork); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("a rule in a VPC tier is cleaned there while a firewall network reconciles", func(t *testing.T) { + // Choosing the mechanism from the reconciled network would delete a firewall rule + // that does not exist and leave this ACL rule open. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + lb, m := newLB(ctrl) + m.network.EXPECT().GetNetworkByID("net-old", gomock.Any()).Return(obsoleteRuleTier, 1, nil) + listParams := expectACLRuleDeleted(m) + + if err := lb.pruneRules(obsoleteIn("net-old"), desiredOn443, firewallNetwork); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if networkID, _ := listParams.GetNetworkid(); networkID != "net-old" { + t.Errorf("ACL rules listed on network %q, want the obsolete rule's own %q", networkID, "net-old") + } + }) + + t.Run("a rule in a deleted network keeps its ACL rule", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + lb, m := newLB(ctrl) + // GetNetworkByID reports not-found as an error alongside a count of 0. + m.network.EXPECT().GetNetworkByID("net-old", gomock.Any()).Return(nil, 0, fmt.Errorf("No match found for net-old")) + // Only the IP-scoped firewall rule is attempted: no network remains to place an + // ACL rule in. + gomock.InOrder( + m.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + m.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{}, nil), + ) + + if err := lb.pruneRules(obsoleteIn("net-old"), desiredOn443, aclNetwork); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("a failed network lookup keeps the rule and reports the error", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + // No delete expectations: the rule must survive until its network can be looked up. + mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl) + mockNetwork.EXPECT().GetNetworkByID("net-old", gomock.Any()).Return(nil, -1, fmt.Errorf("API unavailable")) + lb := &loadBalancer{ + CloudStackClient: &cloudstack.CloudStackClient{ + LoadBalancer: cloudstack.NewMockLoadBalancerServiceIface(ctrl), + Network: mockNetwork, + }, + networkID: "net-new", + ipAddrID: "ip-current", + } + + if err := lb.pruneRules(obsoleteIn("net-old"), desiredOn443, aclNetwork); err == nil { + t.Fatalf("expected the lookup failure to be reported") + } + }) + + t.Run("a rule with no network has only its firewall rule deleted", func(t *testing.T) { + // An ACL rule deleted in a guessed network could be the only opening another service + // has, while a firewall rule is scoped to this rule's own public IP. + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + lb, m := newLB(ctrl) + gomock.InOrder( + m.firewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{}), + m.firewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{}, nil), + ) + + if err := lb.pruneRules(obsoleteWithoutNetworkOn("ip-2"), desiredOn443, aclNetwork); err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + + t.Run("a rule with no network on the reconciled IP uses that network", func(t *testing.T) { + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + + lb, m := newLB(ctrl) + listParams := expectACLRuleDeleted(m) + + if err := lb.pruneRules(obsoleteWithoutNetworkOn("ip-current"), desiredOn443, aclNetwork); err != nil { + t.Fatalf("unexpected error: %v", err) + } + if networkID, _ := listParams.GetNetworkid(); networkID != "net-new" { + t.Errorf("ACL rules listed on network %q, want the reconciled %q", networkID, "net-new") + } + }) +} diff --git a/protocol.go b/protocol.go index 07d81748..f35f8ba0 100644 --- a/protocol.go +++ b/protocol.go @@ -20,6 +20,8 @@ package cloudstack import ( + "strings" + v1 "k8s.io/api/core/v1" ) @@ -98,9 +100,12 @@ func ProtocolFromServicePort(port v1.ServicePort, service *v1.Service) LoadBalan } // ProtocolFromLoadBalancer returns the protocol corresponding to the -// CloudStack load balancer protocol name. +// CloudStack load balancer protocol name. The comparison ignores case: +// CloudStack releases before 4.21 stored the name exactly as the client +// sent it, and the in-tree provider sent it in upper case, so rules +// created there still report "TCP" and "UDP". func ProtocolFromLoadBalancer(protocol string) LoadBalancerProtocol { - switch protocol { + switch strings.ToLower(protocol) { case "tcp": return LoadBalancerProtocolTCP case "udp": diff --git a/protocol_test.go b/protocol_test.go index 84ff78a3..0f1c03db 100644 --- a/protocol_test.go +++ b/protocol_test.go @@ -158,9 +158,19 @@ func TestProtocolFromLoadBalancer(t *testing.T) { want: LoadBalancerProtocolInvalid, }, { - name: "uppercase TCP returns invalid", + name: "uppercase TCP from the in-tree provider", protocol: "TCP", - want: LoadBalancerProtocolInvalid, + want: LoadBalancerProtocolTCP, + }, + { + name: "uppercase UDP from the in-tree provider", + protocol: "UDP", + want: LoadBalancerProtocolUDP, + }, + { + name: "mixed-case tcp-proxy", + protocol: "TCP-Proxy", + want: LoadBalancerProtocolTCPProxy, }, } diff --git a/test/e2e/annotations_test.go b/test/e2e/annotations_test.go index 05548522..f5513487 100644 --- a/test/e2e/annotations_test.go +++ b/test/e2e/annotations_test.go @@ -33,9 +33,10 @@ import ( ) const ( - annotationSourceCidrs = "service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs" - annotationHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname" - annotationIPAssociated = "service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller" //nolint:gosec + annotationSourceCidrs = "service.beta.kubernetes.io/cloudstack-load-balancer-source-cidrs" + annotationHostname = "service.beta.kubernetes.io/cloudstack-load-balancer-hostname" + annotationIPAssociated = "service.beta.kubernetes.io/cloudstack-load-balancer-ip-associated-by-controller" //nolint:gosec + annotationProxyProtocol = "service.beta.kubernetes.io/cloudstack-load-balancer-proxy-protocol" ) func TestAnnot_SourceCIDRs(t *testing.T) { @@ -171,3 +172,83 @@ func TestAnnot_ExplicitLoadBalancerIP(t *testing.T) { return ip.Allocated == "", nil }) } + +// TestAnnot_ProxyProtocolToggle is the end-to-end regression test for issue #2: +// toggling the proxy protocol annotation on a live service used to wedge +// reconciliation for good. The rule name embeds the protocol and rules were +// looked up by name, so the changed protocol missed the lookup and the +// controller tried to create a second rule on a public port the old rule still +// held, which CloudStack rejects as a port conflict. +func TestAnnot_ProxyProtocolToggle(t *testing.T) { + f := NewFramework(t) + svc := f.CreateLBService(nil) + lbName := defaultLoadBalancerName(svc) + + f.WaitForIngressIP(svc) + rules := f.WaitForLBRules(lbName, 1) + originalRuleID := rules[0].Id + if rules[0].Protocol != "tcp" { + t.Fatalf("rule protocol = %q, want tcp before the toggle", rules[0].Protocol) + } + + // settle waits for exactly one rule carrying the wanted protocol and name, + // and returns its ID so the caller can tell an update from a recreate. + settle := func(protocol string) string { + t.Helper() + wantName := fmt.Sprintf("%s-%s-80", lbName, protocol) + var ruleID string + f.Eventually(lbSyncTimeout, lbSyncInterval, "the rule to settle on "+protocol, + func() (bool, error) { + current, err := f.LBRules(lbName) + if err != nil { + return false, err + } + if len(current) != 1 { + return false, fmt.Errorf("saw %d rules, want 1", len(current)) + } + if current[0].Protocol != protocol { + return false, fmt.Errorf("protocol is %q, want %q", current[0].Protocol, protocol) + } + if current[0].Name != wantName { + return false, fmt.Errorf("name is %q, want %q", current[0].Name, wantName) + } + ruleID = current[0].Id + return true, nil + }) + return ruleID + } + + f.UpdateService(svc, func(s *corev1.Service) { + s.Annotations = map[string]string{annotationProxyProtocol: "true"} + }) + proxyRuleID := settle("tcp-proxy") + if proxyRuleID != originalRuleID { + t.Errorf("enabling the proxy protocol recreated the rule (%s -> %s), want an in-place update", + originalRuleID, proxyRuleID) + } + + f.UpdateService(svc, func(s *corev1.Service) { + delete(s.Annotations, annotationProxyProtocol) + }) + revertedRuleID := settle("tcp") + if revertedRuleID != proxyRuleID { + t.Errorf("disabling the proxy protocol recreated the rule (%s -> %s), want an in-place update", + proxyRuleID, revertedRuleID) + } + + // The public port stayed open throughout: the firewall rule is keyed on the + // IP protocol, which both tcp and tcp-proxy map to. + fwRules, err := f.FirewallRules(rules[0].Publicipid) + if err != nil { + t.Fatalf("listing firewall rules: %v", err) + } + found := false + for _, fw := range fwRules { + if fw.Startport == 80 && fw.Endport == 80 && strings.EqualFold(fw.Protocol, "tcp") { + found = true + } + } + if !found { + t.Errorf("no tcp firewall rule for port 80 after the toggle; got %+v", fwRules) + } +} diff --git a/test/e2e/vpc_test.go b/test/e2e/vpc_test.go index 15271eee..817f8bce 100644 --- a/test/e2e/vpc_test.go +++ b/test/e2e/vpc_test.go @@ -259,3 +259,68 @@ func TestVPC_ExplicitLoadBalancerIPReleased(t *testing.T) { return ip.Allocated == "", nil }) } + +// TestVPC_ProxyProtocolACL covers the proxy protocol on a VPC tier, where +// ingress is opened with a Network ACL rule rather than a firewall rule. +// updateNetworkACL used to create the rule with the CloudStack protocol name +// tcp-proxy, which the API rejects, so a proxy protocol service on a tier never +// reconciled at all. The ACL rule is keyed on the IP protocol, so it must be +// created as tcp and be the same single rule before and after the toggle. +func TestVPC_ProxyProtocolACL(t *testing.T) { + f, aclID, _ := vpcFramework(t) + + // An ACL rule belongs to the tier, so this test uses a port of its own. Note + // that 8081 is the virtual router's HAProxy stats port, which CloudStack + // refuses to load balance. + const port = "8085" + svc := f.CreateLBService(func(s *corev1.Service) { + s.Annotations = map[string]string{annotationProxyProtocol: "true"} + s.Spec.Ports = []corev1.ServicePort{ + {Name: "http", Port: 8085, Protocol: corev1.ProtocolTCP}, + } + }) + lbName := defaultLoadBalancerName(svc) + + f.WaitForIngressIP(svc) + rules := f.WaitForLBRules(lbName, 1) + if rules[0].Protocol != "tcp-proxy" { + t.Errorf("rule protocol = %q, want tcp-proxy", rules[0].Protocol) + } + + f.Eventually(lbSyncTimeout, lbSyncInterval, "the tcp network ACL rule for port "+port, + func() (bool, error) { + n, err := countACLRules(f, aclID, port) + return n >= 1, err + }) + + aclRules, err := f.ACLRules(aclID) + if err != nil { + t.Fatalf("listing ACL rules: %v", err) + } + for _, r := range aclRules { + if r.Startport == port && !strings.EqualFold(r.Protocol, "tcp") { + t.Errorf("ACL rule for port %s has protocol %q, want tcp", port, r.Protocol) + } + } + + // Turning the annotation off keeps the one ACL rule: both protocols share it. + f.UpdateService(svc, func(s *corev1.Service) { + delete(s.Annotations, annotationProxyProtocol) + }) + f.Eventually(lbSyncTimeout, lbSyncInterval, "the rule to settle back on tcp", + func() (bool, error) { + current, err := f.LBRules(lbName) + if err != nil || len(current) != 1 { + return false, err + } + return current[0].Protocol == "tcp", nil + }) + + n, err := countACLRules(f, aclID, port) + if err != nil { + t.Fatalf("counting ACL rules: %v", err) + } + if n != 1 { + t.Errorf("ACL rules for port %s = %d, want exactly 1 across the toggle", port, n) + } +}