This is an automated email from the ASF dual-hosted git repository.
vishesh92 pushed a commit to branch main
in repository
https://gitbox.apache.org/repos/asf/cloudstack-kubernetes-provider.git
The following commit(s) were added to refs/heads/main by this push:
new 2a46b8e4 Update load balancer rules in place when the protocol changes
(#104)
2a46b8e4 is described below
commit 2a46b8e43382bbd1564db7a9bfa56f9caa872d13
Author: Vishesh <[email protected]>
AuthorDate: Mon Oct 5 18:49:26 2026 +0530
Update load balancer rules in place when the protocol changes (#104)
---
README.md | 23 +-
cloudstack_loadbalancer.go | 595 +++++++++++---
cloudstack_loadbalancer_test.go | 1674 +++++++++++++++++++++++++++++++++------
protocol.go | 9 +-
protocol_test.go | 14 +-
test/e2e/annotations_test.go | 87 +-
test/e2e/vpc_test.go | 67 ++
7 files changed, 2112 insertions(+), 357 deletions(-)
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..481c114b 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.Warningf("Skipping obsolete load balancer rule %v
(%v on public IP %v) with invalid public port %q: %v", lbRule.Name, lbRule.Id,
lbRule.Publicipid, 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.Warningf("Skipping obsolete load balancer rule %v
(%v on public IP %v) with unknown protocol %q", lbRule.Name, lbRule.Id,
lbRule.Publicipid, 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-<protocol>-<port>.
+ e.service = &corev1.Service{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "test-service",
+ Namespace: "default",
+ UID: "test-uid",
+ Annotations: annotations,
+ },
+ Spec: corev1.ServiceSpec{
+ SessionAffinity: corev1.ServiceAffinityNone,
+ Ports: ports,
+ },
+ }
+
+ e.nodes = []*corev1.Node{{ObjectMeta: metav1.ObjectMeta{Name:
"node-1"}}}
+
+ return e
+}
+
+// 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..55392178 100644
--- a/test/e2e/vpc_test.go
+++ b/test/e2e/vpc_test.go
@@ -24,6 +24,7 @@ package e2e
import (
"context"
"os"
+ "strconv"
"strings"
"testing"
@@ -259,3 +260,69 @@ 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 servicePort int32 = 8085
+ port := strconv.Itoa(int(servicePort))
+ svc := f.CreateLBService(func(s *corev1.Service) {
+ s.Annotations = map[string]string{annotationProxyProtocol:
"true"}
+ s.Spec.Ports = []corev1.ServicePort{
+ {Name: "http", Port: servicePort, 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)
+ }
+}
