This is an automated email from the ASF dual-hosted git repository.
DaanHoogland 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 b4335bfa Give each Service its own Network ACL rule (#109)
b4335bfa is described below
commit b4335bfa4fb9ca034ec3f28642bd3572e0592051
Author: Vishesh <[email protected]>
AuthorDate: Thu Oct 22 20:50:13 2026 +0530
Give each Service its own Network ACL rule (#109)
---
README.md | 95 ++++-
cloudstack_loadbalancer.go | 275 +++++++++----
cloudstack_loadbalancer_test.go | 836 ++++++++++++++++++++++++++++++++++++++--
docs/development.md | 8 +-
test/e2e/framework.go | 54 +++
test/e2e/vpc_test.go | 207 +++++++++-
6 files changed, 1362 insertions(+), 113 deletions(-)
diff --git a/README.md b/README.md
index 8314a37f..c3f9ba93 100644
--- a/README.md
+++ b/README.md
@@ -61,6 +61,10 @@ Stickiness policies also need `listLBStickinessPolicies`,
`createLBStickinessPol
projects cannot call them, so stickiness does not work there yet. Services
without stickiness are
not affected.
+On VPC tiers the controller also calls `updateNetworkACLItem`, to take over
Network ACL rules that
+earlier releases created (see [Per-Service Network ACL
rules](#per-service-network-acl-rules)).
+Without it the controller creates its own rules and leaves the old ones for
you to delete.
+
To create the secret, use the following command:
```bash
kubectl -n kube-system create secret generic cloudstack-secret
--from-file=cloud-config
@@ -406,7 +410,22 @@ chooses the mechanism based on the services the network
offers, so on such netwo
firewall rules instead of Network ACL rules, and
`spec.loadBalancerSourceRanges` is applied to
them.
-Two things to be aware of when the controller manages Network ACL rules:
+When the controller manages Network ACL rules:
+* Each Service gets its own ACL rule for each port, even when another Service
on the tier uses the
+ same port. The rule's reason is `Managed by the CloudStack Kubernetes
Provider for <name>`, where
+ `<name>` is the start of the Service's load balancer rule names: `a`
followed by the Service UID
+ without dashes, cut to 32 characters. The controller only deletes rules with
its Service's reason.
+* If the tier already has an ingress rule for exactly that port (not a port
range) and protocol
+ that the controller did not create, for example one with a restricted CIDR,
a deny rule, or a
+ rule with a reason of its own, the controller adds no rule for that port and
never changes or
+ deletes yours.
+* A rule that looks exactly like the ones earlier releases created (allow,
`0.0.0.0/0`, one port,
+ no reason) is taken over by the first Service that needs its port. Give
rules you create by hand
+ a reason to keep them. Rules in a global ACL list, one that belongs to no
VPC, are never taken
+ over: the controller leaves the port to them and never deletes them.
+* As before, the controller only looks at rules whose protocol is stored in
lower case. Rules stored
+ as `TCP` or `UDP`, such as the ones CloudStack's Kubernetes service adds for
its own ports, are
+ left alone, and the controller adds its own rule next to them.
* The ACL rules the controller creates always allow `0.0.0.0/0`;
`spec.loadBalancerSourceRanges` is
not applied. Use the `cloudstack-load-balancer-source-cidrs` annotation to
restrict sources.
* If the network uses one of the default ACL lists (`default_allow` or
`default_deny`), the
@@ -523,6 +542,80 @@ The controller or CloudStack rejected the stickiness
annotations, and the event
If CloudStack rejects a change, an existing service keeps its current policy.
A new service keeps
its public IP and rules, but stays `<pending>` until the annotation is fixed.
+### A port on a VPC tier stays closed and the controller added no ACL rule for
it
+
+The tier already has an ingress ACL rule for exactly that port and protocol
that the controller did
+not create, so the controller leaves the port to it. Check that rule's action
and CIDRs. If you delete
+it, the controller adds its own rule the next time it syncs the Service, which
happens when the
+Service changes or the controller restarts.
+
+### An ACL rule disappeared when a Service was deleted
+
+The rule allowed exactly `0.0.0.0/0` on one port and had no reason, so the
controller took it over
+as a rule left by an earlier release and deleted it with its Service. Give
rules you create by hand
+a reason.
+
+### A deleted Service stays `Terminating`, log shows `Error deleting Network
ACL rule`
+
+The controller keeps the Service's load balancer rule until its ACL rule is
deleted, because
+Kubernetes only retries the cleanup while a load balancer rule is left. Fix
the cause shown in the
+log, for example the account's permissions, and the cleanup finishes on its
own.
+
+## Upgrading
+
+### Per-Service Network ACL rules
+
+This affects VPC tiers whose ACL rules the controller manages, that is tiers
with a custom ACL list.
+Isolated networks, and VPC networks that use firewall rules, behave as before.
+
+Earlier releases opened a port on a VPC tier with one ACL rule shared by every
Service using that
+port. They deleted it when any of those Services was deleted or stopped using
the port, so the
+other Services lost traffic on that port. Each Service now has its own rule,
marked with its
+reason, and the controller only deletes rules that carry its Service's reason.
+
+What you may notice after upgrading:
+* Existing controller rules get a reason. The controller syncs every Service
when it starts. On that
+ first sync, the first Service using an old rule (allow, `0.0.0.0/0`, one
port, no reason) takes it
+ over by setting its reason.
+* More ACL rules. When several Services use the same port, each gets a rule of
its own, numbered
+ after the existing rules.
+* On that first start, the tier's router applies its ACL list once for each
rule taken over.
+* A rule you created for a Service's port, for example with a restricted CIDR,
a deny action or a
+ reason of its own, is no longer deleted with the Service. As before, the
controller adds no rule
+ of its own on that port.
+* An egress rule for a port no longer stops the controller from opening that
port for ingress. A
+ port that stayed closed because of one opens on the next sync of its Service.
+* Deleting a Service waits until its ACL rule is deleted. While that keeps
failing the Service stays
+ `Terminating`; see
[Troubleshooting](#a-deleted-service-stays-terminating-log-shows-error-deleting-network-acl-rule).
+* Old rules in a global ACL list, one that belongs to no VPC, are never taken
over or deleted.
+
+Before upgrading:
+* Give a reason to every rule you created by hand that allows `0.0.0.0/0` on a
single port of a
+ tier. Without one it looks like an old controller rule, so a Service using
that port takes it
+ over and deletes it together with the Service.
+* Allow the controller's account to call `updateNetworkACLItem`. Without it
the controller cannot
+ take old rules over: it creates a rule of its own next to each one, or keeps
using the old rule if
+ it may not create rules either.
+* Upgrade every controller whose Services share a VPC tier or ACL list, for
example several clusters
+ on one tier. An older controller still treats any rule on its port as its
own and can delete
+ another Service's rule.
+
+After upgrading, check:
+* The controller log for `Cannot adopt Network ACL rule`. The Service now has
a rule of its own, so
+ the old rule named in the message can be deleted once no other Service
relies on it.
+* The controller log for `Cannot create a Network ACL rule for load balancer
... either`. The Service
+ still relies on the old rule named there; keep it, and give the account the
missing permissions.
+* The tier's ACL list for rules with no reason that allow `0.0.0.0/0` on a
port no Service uses.
+ They are left from Services that were deleted, or changed their port or
protocol, before or
+ during the first sync. Old rules in a global ACL list stay too. Delete them
by hand.
+* Services that stay `Terminating`.
+
+If the controller runs with `--concurrent-service-syncs` above 1, two Services
can take over the
+same old rule at once. The one that loses relies on the other's rule until its
next sync.
+
+Downgrading brings the shared behaviour back. An older controller treats any
rule on its port as
+its own, so it can again delete a rule another Service relies on.
+
## Migration Guide
There are several notable differences to the old Kubernetes CloudStack cloud
provider that need to be taken into
diff --git a/cloudstack_loadbalancer.go b/cloudstack_loadbalancer.go
index 753322dc..f94bc01a 100644
--- a/cloudstack_loadbalancer.go
+++ b/cloudstack_loadbalancer.go
@@ -351,6 +351,7 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx
context.Context, clusterName st
klog.Errorf("Error removing duplicate load balancer rules for
%v/%v: %v", service.Namespace, service.Name, sweepErr)
}
+ var aclErr error
for _, lbRule := range lb.rules {
klog.V(4).Infof("Deleting firewall rules / Network ACLs for
load balancer: %v", lbRule.Name)
protocol := ProtocolFromLoadBalancer(lbRule.Protocol)
@@ -361,9 +362,12 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx
context.Context, clusterName st
if err != nil {
klog.Errorf("Error parsing port: %v", err)
} else {
- networkId, err :=
cs.getNetworkIDFromIPAddress(lb.ipAddrID)
- if err != nil {
- return err
+ networkId := lbRule.Networkid
+ if networkId == "" {
+ networkId, err =
cs.getNetworkIDFromIPAddress(lbRule.Publicipid)
+ if err != nil {
+ return err
+ }
}
network, count, err :=
lb.Network.GetNetworkByID(networkId, cloudstack.WithProject(lb.projectID))
if err != nil {
@@ -382,7 +386,13 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx
context.Context, clusterName st
klog.V(4).Infof("Deleting network ACLs
for %v - %v", int(port), protocol)
_, err =
lb.deleteNetworkACLRule(int(port), protocol, networkId)
if err != nil {
- klog.Errorf("Error deleting
Network ACL rule: %v", err)
+ // Kubernetes stops retrying
once no load balancer rule is left, and only
+ // this rule leads back to the
tier and port of its ACL rule, so it is kept.
+ klog.Errorf("Error deleting
Network ACL rule, keeping load balancer rule %v for the retry: %v",
lbRule.Name, err)
+ if aclErr == nil {
+ aclErr = err
+ }
+ continue
}
}
}
@@ -394,6 +404,11 @@ func (cs *CSCloud) EnsureLoadBalancerDeleted(ctx
context.Context, clusterName st
}
}
+ // Releasing the IP would make CloudStack revoke the rules kept for the
retry.
+ if aclErr != nil {
+ return errors.Join(aclErr, sweepErr)
+ }
+
if lb.ipAddr != "" {
// If the IP was allocated by the controller (not specified in
service spec), release it.
if lb.ipAddr != service.Spec.LoadBalancerIP {
@@ -912,9 +927,9 @@ func (lb *loadBalancer) pruneFirewallRule(o obsoleteRule,
claimed map[portProtoc
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.
+// pruneNetworkACLRule deletes this service's 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)
@@ -1653,8 +1668,9 @@ func (lb *loadBalancer) deleteLoadBalancerRule(lbRule
*cloudstack.LoadBalancerRu
// deleteDuplicateRules removes rules that collided by name with the one being
// managed. A duplicate sits on its own public IP, since CloudStack rejects a
-// second rule on the same IP and port, so its firewall rule and IP go with it;
-// network ACL rules are shared per tier and port with the kept rule and stay.
+// second rule on the same IP and port, so its firewall rule and IP go with it.
+// Network ACL rules are never deleted here: a duplicate has the kept rule's
name,
+// so in the same tier it uses the same ACL rule as the kept one.
func (lb *loadBalancer) deleteDuplicateRules() error {
for _, lbRule := range lb.duplicateRules {
klog.V(4).Infof("Deleting duplicate load balancer rule: %v
(%v)", lbRule.Name, lbRule.Id)
@@ -1952,6 +1968,10 @@ func (lb *loadBalancer) updateFirewallRule(publicIpId
string, publicPort int, pr
return true, err
}
+// updateNetworkACL opens a port on a VPC tier with a Network ACL rule this
load balancer owns.
+// Rules of other load balancers never count, so each Service has its own rule
and deleting one
+// Service leaves the port open for the others (issue #107). A rule from an
earlier release is
+// adopted, and when someone else's rule already covers the port none is added.
func (lb *loadBalancer) updateNetworkACL(publicPort int, protocol
LoadBalancerProtocol, networkId string) (bool, error) {
network, _, err := lb.Network.GetNetworkByID(networkId,
cloudstack.WithProject(lb.projectID))
if err != nil {
@@ -1972,57 +1992,178 @@ func (lb *loadBalancer) updateNetworkACL(publicPort
int, protocol LoadBalancerPr
return true, err
}
- networkAclParams := lb.NetworkACL.NewListNetworkACLsParams()
- networkAclParams.SetAclid(network.Aclid)
- networkAclParams.SetNetworkid(networkId)
- networkAclParams.SetListall(true)
+ ipProtocol := protocol.IPProtocol()
+ aclRules, err := lb.listNetworkACLRules(networkId, ipProtocol,
publicPort)
+ if err != nil {
+ return false, fmt.Errorf("error fetching Network ACL with ID:
%v for network with id: %v, due to: %w", network.Aclid, networkId, err)
+ }
+
+ // Only a root admin may change the rules of a global list, the kind
that belongs to no VPC.
+ adoptable := networkAclList.Vpcid != ""
+ kind, aclRule := lb.decidingNetworkACLRule(aclRules, adoptable)
+ switch kind {
+ case aclRuleOwn:
+ klog.V(4).Infof("Network ACL rule %v already opens %v port %v
for load balancer %v", aclRule.Id, protocol, publicPort, lb.name)
+ return true, nil
+ case aclRuleOperator:
+ klog.Infof("Network ACL rule %v already covers %v port %v on
network %v; not adding one for load balancer %v", aclRule.Id, protocol,
publicPort, networkId, lb.name)
+ return true, nil
+ case aclRuleLegacy:
+ if err := lb.adoptNetworkACLRule(aclRule, network.Aclid,
networkId, ipProtocol, publicPort); err != nil {
+ return false, err
+ }
+ return true, nil
+ }
+
+ if err := lb.createNetworkACLRule(network.Aclid, networkId, ipProtocol,
publicPort); err != nil {
+ return false, err
+ }
+ return true, nil
+}
+
+// networkACLReasonPrefix starts the reason of every Network ACL rule the
controller creates.
+// The rest of the reason names the owning load balancer, and only rules
carrying a load
+// balancer's own reason are ever deleted for it, so this text must not change.
+const networkACLReasonPrefix = "Managed by the CloudStack Kubernetes Provider
for "
+
+// networkACLReason is the reason that marks a Network ACL rule as this load
balancer's.
+func (lb *loadBalancer) networkACLReason() string {
+ return networkACLReasonPrefix + lb.name
+}
+
+// networkACLRuleKind is what an existing Network ACL rule on a needed port
means to a load
+// balancer. The kinds are ordered by precedence.
+type networkACLRuleKind int
+
+const (
+ aclRuleIgnored networkACLRuleKind = iota // being deleted, or another
load balancer's
+ aclRuleOperator // someone else's; the port
is left to them
+ aclRuleLegacy // made by a release before
rules had owners
+ aclRuleOwn // this load balancer's
+)
+
+// classifyNetworkACLRule tells what an ingress rule on the needed port is to
the load balancer
+// whose reason is given. CloudStack shows a rule being revoked as Deleting,
and updating such a
+// rule would bring it back, so it never counts. A rule from an earlier
release has exactly the
+// shape those releases created; any other unmarked rule, such as one with a
restricted CIDR, a
+// deny rule or "0.0.0.0/0,::/0", is someone else's.
+func classifyNetworkACLRule(aclRule *cloudstack.NetworkACL, reason string)
networkACLRuleKind {
+ if aclRule.State == "Deleting" {
+ return aclRuleIgnored
+ }
+ if aclRule.Reason == reason {
+ return aclRuleOwn
+ }
+ if strings.HasPrefix(aclRule.Reason, networkACLReasonPrefix) {
+ return aclRuleIgnored
+ }
+ if aclRule.Reason == "" && aclRule.Action == "Allow" &&
+ compareStringSlice(splitCIDRList(aclRule.Cidrlist),
[]string{defaultAllowedCIDR}) {
+ return aclRuleLegacy
+ }
+ return aclRuleOperator
+}
+
+// decidingNetworkACLRule picks the rule that decides what this load balancer
does about a port:
+// its own rule, else one from an earlier release to adopt, else someone
else's. A rule from an
+// earlier release counts as someone else's when it cannot be adopted.
+func (lb *loadBalancer) decidingNetworkACLRule(aclRules
[]*cloudstack.NetworkACL, adoptable bool) (networkACLRuleKind,
*cloudstack.NetworkACL) {
+ kind, deciding := aclRuleIgnored, (*cloudstack.NetworkACL)(nil)
+ for _, aclRule := range aclRules {
+ k := classifyNetworkACLRule(aclRule, lb.networkACLReason())
+ if k == aclRuleLegacy && !adoptable {
+ k = aclRuleOperator
+ }
+ if k > kind {
+ kind, deciding = k, aclRule
+ }
+ }
+ return kind, deciding
+}
+
+// listNetworkACLRules lists the ingress rules of a tier for exactly one port
and IP protocol.
+// Listing by network covers the ACL list the tier uses now. CloudStack stores
the protocol as it
+// was sent, and the protocol is compared exactly, as earlier releases did, so
rules stored in upper
+// case, such as the "TCP" rules CloudStack's Kubernetes service adds for its
own ports, are left
+// alone.
+func (lb *loadBalancer) listNetworkACLRules(networkID, ipProtocol string,
publicPort int) ([]*cloudstack.NetworkACL, error) {
+ p := lb.NetworkACL.NewListNetworkACLsParams()
+ p.SetNetworkid(networkID)
+ p.SetListall(true)
if lb.projectID != "" {
- networkAclParams.SetProjectid(lb.projectID)
+ p.SetProjectid(lb.projectID)
}
- networkAcls, err := listAll(networkAclParams, func() (int,
[]*cloudstack.NetworkACL, error) {
- networkAclResponse, err :=
lb.NetworkACL.ListNetworkACLs(networkAclParams)
+ aclRules, err := listAll(p, func() (int, []*cloudstack.NetworkACL,
error) {
+ r, err := lb.NetworkACL.ListNetworkACLs(p)
if err != nil {
return 0, nil, err
}
- return networkAclResponse.Count,
networkAclResponse.NetworkACLs, nil
+ return r.Count, r.NetworkACLs, nil
})
if err != nil {
- return false, fmt.Errorf("error fetching Network ACL with ID:
%v for network with id: %v, due to: %s", network.Aclid, networkId, err)
+ return nil, err
}
- // find all network ACL rules that have a matching proto+port
- // a map may or may not be faster, but is a bit easier to understand
- filtered := make(map[*cloudstack.NetworkACL]bool)
- for _, netAclRule := range networkAcls {
- if netAclRule.Protocol == protocol.IPProtocol() &&
netAclRule.Startport == strconv.Itoa(publicPort) && netAclRule.Endport ==
strconv.Itoa(publicPort) {
- filtered[netAclRule] = true
+ port := strconv.Itoa(publicPort)
+ var matching []*cloudstack.NetworkACL
+ for _, aclRule := range dedupeByID(aclRules, func(r
*cloudstack.NetworkACL) string { return r.Id }) {
+ if strings.EqualFold(aclRule.Traffictype, "Ingress") &&
aclRule.Protocol == ipProtocol &&
+ aclRule.Startport == port && aclRule.Endport == port {
+ matching = append(matching, aclRule)
}
}
- if len(filtered) > 0 {
- klog.V(4).Infof("Network ACL rule for port %v and protocol %v
already exists. No need to added a duplicate rule", publicPort, protocol)
- return true, err
+ return matching, nil
+}
+
+// adoptNetworkACLRule marks a rule an earlier release created as this load
balancer's. Only the
+// reason is sent: an update is partial by default, so CloudStack keeps the
other fields. An account
+// that may not update ACL rules creates a rule of its own instead, and one
that may not create
+// them either keeps relying on the old rule, as earlier releases did.
+func (lb *loadBalancer) adoptNetworkACLRule(aclRule *cloudstack.NetworkACL,
aclID, networkID, ipProtocol string, publicPort int) error {
+ p := lb.NetworkACL.NewUpdateNetworkACLItemParams(aclRule.Id)
+ p.SetReason(lb.networkACLReason())
+
+ _, err := lb.NetworkACL.UpdateNetworkACLItem(p)
+ if err == nil {
+ klog.Infof("Adopted Network ACL rule %v for load balancer %v",
aclRule.Id, lb.name)
+ return nil
+ }
+ if !isNotAllowed(err) {
+ return fmt.Errorf("error adopting Network ACL rule %v: %w",
aclRule.Id, err)
+ }
+
+ klog.Warningf("Cannot adopt Network ACL rule %v for load balancer %v,
creating a new one; delete the old rule by hand: %v", aclRule.Id, lb.name, err)
+ err = lb.createNetworkACLRule(aclID, networkID, ipProtocol, publicPort)
+ if err != nil && isNotAllowed(err) {
+ klog.Warningf("Cannot create a Network ACL rule for load
balancer %v either; port %v stays open through rule %v: %v", lb.name,
publicPort, aclRule.Id, err)
+ return nil
}
- // create ACL rule
- // 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)
+ return err
+}
+
+// createNetworkACLRule opens a port on a tier with a rule marked as this load
balancer's. ACL
+// rules only know tcp, udp and icmp, so the rule takes the IP protocol and a
tcp-proxy port
+// opens tcp.
+func (lb *loadBalancer) createNetworkACLRule(aclID, networkID, ipProtocol
string, publicPort int) error {
+ acl := lb.NetworkACL.NewCreateNetworkACLParams(ipProtocol)
+ acl.SetAclid(aclID)
acl.SetAction("Allow")
- acl.SetCidrlist([]string{"0.0.0.0/0"})
+ acl.SetCidrlist([]string{defaultAllowedCIDR})
acl.SetStartport(publicPort)
acl.SetEndport(publicPort)
- acl.SetNetworkid(networkId)
+ acl.SetNetworkid(networkID)
acl.SetTraffictype("Ingress")
+ acl.SetReason(lb.networkACLReason())
- _, err = lb.NetworkACL.CreateNetworkACL(acl)
- if err != nil {
- return false, fmt.Errorf("error creating Network ACL for port:
%v, due to: %s", publicPort, err)
+ if _, err := lb.NetworkACL.CreateNetworkACL(acl); err != nil {
+ return fmt.Errorf("error creating Network ACL for port: %v, due
to: %w", publicPort, err)
}
- return true, err
+
+ return nil
}
// deleteFirewallRule deletes the firewall rule associated with the
ip:port:protocol combo
@@ -2057,51 +2198,39 @@ func (lb *loadBalancer) deleteFirewallRule(publicIpId
string, publicPort int, pr
return deleted, err
}
-// Delete Network ACLs deletes the Network ACL rule associated with the
ip:port:protocol combo
+// deleteNetworkACLRule deletes this load balancer's Network ACL rules for a
port on a tier and
+// leaves every other rule alone, so another Service using the same port keeps
it open. A rule
+// CloudStack still shows as Deleting is deleted again, which retries applying
the list; a failure
+// there is only logged, as CloudStack removes the rule on its next successful
apply. Every rule
+// is tried and the first error is returned.
func (lb *loadBalancer) deleteNetworkACLRule(publicPort int, protocol
LoadBalancerProtocol, networkID string) (bool, error) {
- p := lb.NetworkACL.NewListNetworkACLsParams()
- p.SetListall(true)
- p.SetNetworkid(networkID)
- if lb.projectID != "" {
- p.SetProjectid(lb.projectID)
+ aclRules, err := lb.listNetworkACLRules(networkID,
protocol.IPProtocol(), publicPort)
+ if err != nil {
+ return false, fmt.Errorf("error fetching Network ACL rules
Network ID %v: %w", networkID, err)
}
- networkAcls, err := listAll(p, func() (int, []*cloudstack.NetworkACL,
error) {
- r, err := lb.NetworkACL.ListNetworkACLs(p)
- if err != nil {
- return 0, nil, err
+ owned := 0
+ var firstErr error
+ for _, aclRule := range aclRules {
+ if aclRule.Reason != lb.networkACLReason() {
+ continue
}
+ owned++
- return r.Count, r.NetworkACLs, nil
- })
- if err != nil {
- return false, fmt.Errorf("error fetching Network ACL rules
Network ID %v: %v", networkID, err)
- }
-
- // filter by proto:port
- filtered := make([]*cloudstack.NetworkACL, 0, 1)
- for _, rule := range networkAcls {
- if rule.Protocol == protocol.IPProtocol() && rule.Startport ==
strconv.Itoa(publicPort) && rule.Endport == strconv.Itoa(publicPort) {
- filtered = append(filtered, rule)
+ p := lb.NetworkACL.NewDeleteNetworkACLParams(aclRule.Id)
+ if _, err := lb.NetworkACL.DeleteNetworkACL(p); err != nil {
+ klog.Errorf("Error deleting Network ACL rule %v: %v",
aclRule.Id, err)
+ if aclRule.State != "Deleting" && firstErr == nil {
+ firstErr = err
+ }
}
}
- // delete first filtered rules
- if len(filtered) == 0 {
- klog.V(4).Infof("No ACL rules found matching protocol: %v and
port: %v", protocol, publicPort)
- return true, nil
- }
- deleted := false
- ruleToBeDeleted := filtered[0]
- deleteAclParams :=
lb.NetworkACL.NewDeleteNetworkACLParams(ruleToBeDeleted.Id)
- _, err = lb.NetworkACL.DeleteNetworkACL(deleteAclParams)
- if err != nil {
- klog.Errorf("Error deleting old Network ACL rule %v: %v",
ruleToBeDeleted.Id, err)
- } else {
- deleted = true
+ if owned == 0 {
+ klog.V(4).Infof("No Network ACL rules of load balancer %v found
for protocol %v and port %v", lb.name, protocol, publicPort)
}
- return deleted, err
+ return firstErr == nil, firstErr
}
// getStringFromServiceAnnotation searches a given v1.Service for a specific
annotationKey and either returns the annotation's value or a specified
defaultSetting
diff --git a/cloudstack_loadbalancer_test.go b/cloudstack_loadbalancer_test.go
index 24e8a14a..91ac15c4 100644
--- a/cloudstack_loadbalancer_test.go
+++ b/cloudstack_loadbalancer_test.go
@@ -3090,6 +3090,33 @@ func TestDeleteFirewallRule(t *testing.T) {
})
}
+// testACLReason marks a Network ACL rule as owned by the test load balancer
"atestuid".
+const testACLReason = networkACLReasonPrefix + "atestuid"
+
+// legacyACLRule is an ingress tcp rule for port 80 shaped as earlier releases
created it. edit
+// turns it into the kind of rule a test needs.
+func legacyACLRule(id string, edit func(*cloudstack.NetworkACL))
*cloudstack.NetworkACL {
+ aclRule := &cloudstack.NetworkACL{
+ Id: id,
+ Protocol: "tcp",
+ Startport: "80",
+ Endport: "80",
+ Traffictype: "Ingress",
+ Action: "Allow",
+ Cidrlist: defaultAllowedCIDR,
+ State: "Active",
+ }
+ if edit != nil {
+ edit(aclRule)
+ }
+ return aclRule
+}
+
+// ownACLRule is the tcp/80 rule of the test load balancer "atestuid".
+func ownACLRule(id string) *cloudstack.NetworkACL {
+ return legacyACLRule(id, func(r *cloudstack.NetworkACL) { r.Reason =
testACLReason })
+}
+
func TestUpdateNetworkACL(t *testing.T) {
t.Run("create new ACL rule", func(t *testing.T) {
ctrl := gomock.NewController(t)
@@ -3129,6 +3156,7 @@ func TestUpdateNetworkACL(t *testing.T) {
)
lb := &loadBalancer{
+ name: "atestuid",
CloudStackClient: &cloudstack.CloudStackClient{
Network: mockNetwork,
NetworkACL: mockNetworkACL,
@@ -3142,6 +3170,15 @@ func TestUpdateNetworkACL(t *testing.T) {
if !updated {
t.Errorf("updated = false, want true")
}
+ if reason, _ := createParams.GetReason(); reason !=
testACLReason {
+ t.Errorf("reason = %q, want %q", reason, testACLReason)
+ }
+ if cidrs, _ := createParams.GetCidrlist();
!compareStringSlice(cidrs, []string{defaultAllowedCIDR}) {
+ t.Errorf("cidrlist = %v, want [%v]", cidrs,
defaultAllowedCIDR)
+ }
+ if trafficType, _ := createParams.GetTraffictype(); trafficType
!= "Ingress" {
+ t.Errorf("traffictype = %q, want Ingress", trafficType)
+ }
})
t.Run("tcp-proxy creates ACL rule with tcp protocol", func(t
*testing.T) {
@@ -3198,7 +3235,7 @@ func TestUpdateNetworkACL(t *testing.T) {
}
})
- t.Run("tcp-proxy matches existing tcp ACL rule", func(t *testing.T) {
+ t.Run("tcp-proxy matches this service's tcp ACL rule", func(t
*testing.T) {
ctrl := gomock.NewController(t)
t.Cleanup(ctrl.Finish)
@@ -3219,12 +3256,7 @@ func TestUpdateNetworkACL(t *testing.T) {
listResp := &cloudstack.ListNetworkACLsResponse{
Count: 1,
NetworkACLs: []*cloudstack.NetworkACL{
- {
- Id: "acl-rule-123",
- Protocol: "tcp",
- Startport: "80",
- Endport: "80",
- },
+ ownACLRule("acl-rule-123"),
},
}
@@ -3237,6 +3269,7 @@ func TestUpdateNetworkACL(t *testing.T) {
)
lb := &loadBalancer{
+ name: "atestuid",
CloudStackClient: &cloudstack.CloudStackClient{
Network: mockNetwork,
NetworkACL: mockNetworkACL,
@@ -3252,7 +3285,7 @@ func TestUpdateNetworkACL(t *testing.T) {
}
})
- t.Run("rule already exists", func(t *testing.T) {
+ t.Run("this service's rule already exists", func(t *testing.T) {
ctrl := gomock.NewController(t)
t.Cleanup(ctrl.Finish)
@@ -3273,12 +3306,7 @@ func TestUpdateNetworkACL(t *testing.T) {
listResp := &cloudstack.ListNetworkACLsResponse{
Count: 1,
NetworkACLs: []*cloudstack.NetworkACL{
- {
- Id: "acl-rule-123",
- Protocol: "tcp",
- Startport: "80",
- Endport: "80",
- },
+ ownACLRule("acl-rule-123"),
},
}
@@ -3290,6 +3318,7 @@ func TestUpdateNetworkACL(t *testing.T) {
)
lb := &loadBalancer{
+ name: "atestuid",
CloudStackClient: &cloudstack.CloudStackClient{
Network: mockNetwork,
NetworkACL: mockNetworkACL,
@@ -3498,7 +3527,7 @@ func TestUpdateNetworkACL(t *testing.T) {
}
func TestDeleteNetworkACLRule(t *testing.T) {
- t.Run("delete matching rule", func(t *testing.T) {
+ t.Run("delete this service's rule", func(t *testing.T) {
ctrl := gomock.NewController(t)
t.Cleanup(ctrl.Finish)
@@ -3507,12 +3536,7 @@ func TestDeleteNetworkACLRule(t *testing.T) {
listResp := &cloudstack.ListNetworkACLsResponse{
Count: 1,
NetworkACLs: []*cloudstack.NetworkACL{
- {
- Id: "acl-rule-123",
- Protocol: "tcp",
- Startport: "80",
- Endport: "80",
- },
+ ownACLRule("acl-rule-123"),
},
}
@@ -3526,6 +3550,7 @@ func TestDeleteNetworkACLRule(t *testing.T) {
)
lb := &loadBalancer{
+ name: "atestuid",
CloudStackClient: &cloudstack.CloudStackClient{
NetworkACL: mockNetworkACL,
},
@@ -3608,12 +3633,7 @@ func TestDeleteNetworkACLRule(t *testing.T) {
listResp := &cloudstack.ListNetworkACLsResponse{
Count: 1,
NetworkACLs: []*cloudstack.NetworkACL{
- {
- Id: "acl-rule-123",
- Protocol: "tcp",
- Startport: "80",
- Endport: "80",
- },
+ ownACLRule("acl-rule-123"),
},
}
@@ -3628,6 +3648,7 @@ func TestDeleteNetworkACLRule(t *testing.T) {
)
lb := &loadBalancer{
+ name: "atestuid",
CloudStackClient: &cloudstack.CloudStackClient{
NetworkACL: mockNetworkACL,
},
@@ -3643,6 +3664,426 @@ func TestDeleteNetworkACLRule(t *testing.T) {
})
}
+func TestClassifyNetworkACLRule(t *testing.T) {
+ tests := []struct {
+ name string
+ edit func(*cloudstack.NetworkACL)
+ want networkACLRuleKind
+ }{
+ {"this service's rule", func(r *cloudstack.NetworkACL) {
r.Reason = testACLReason }, aclRuleOwn},
+ {"this service's rule being deleted", func(r
*cloudstack.NetworkACL) { r.Reason = testACLReason; r.State = "Deleting" },
aclRuleIgnored},
+ {"this service's rule with a CIDR edited by hand", func(r
*cloudstack.NetworkACL) { r.Reason = testACLReason; r.Cidrlist = "10.0.0.0/8"
}, aclRuleOwn},
+ {"another service's rule", func(r *cloudstack.NetworkACL) {
r.Reason = networkACLReasonPrefix + "aotheruid" }, aclRuleIgnored},
+ {"a rule from an earlier release", nil, aclRuleLegacy},
+ {"a rule from an earlier release being deleted", func(r
*cloudstack.NetworkACL) { r.State = "Deleting" }, aclRuleIgnored},
+ {"a restricted CIDR", func(r *cloudstack.NetworkACL) {
r.Cidrlist = "10.0.0.0/8" }, aclRuleOperator},
+ {"an IPv6 range as well", func(r *cloudstack.NetworkACL) {
r.Cidrlist = "0.0.0.0/0,::/0" }, aclRuleOperator},
+ {"a deny rule", func(r *cloudstack.NetworkACL) { r.Action =
"Deny" }, aclRuleOperator},
+ {"a reason of its own", func(r *cloudstack.NetworkACL) {
r.Reason = "allow https" }, aclRuleOperator},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ if got := classifyNetworkACLRule(legacyACLRule("acl-1",
tt.edit), testACLReason); got != tt.want {
+ t.Errorf("classifyNetworkACLRule() = %v, want
%v", got, tt.want)
+ }
+ })
+ }
+}
+
+func TestListNetworkACLRules(t *testing.T) {
+ t.Run("keeps ingress rules for exactly the port and protocol", func(t
*testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl)
+ listParams := &cloudstack.ListNetworkACLsParams{}
+
mockNetworkACL.EXPECT().NewListNetworkACLsParams().Return(listParams)
+
mockNetworkACL.EXPECT().ListNetworkACLs(listParams).Return(&cloudstack.ListNetworkACLsResponse{
+ Count: 8,
+ NetworkACLs: []*cloudstack.NetworkACL{
+ legacyACLRule("tcp-80", nil),
+ legacyACLRule("upper-case-tcp-80", func(r
*cloudstack.NetworkACL) { r.Protocol = "TCP" }),
+ legacyACLRule("deleting-tcp-80", func(r
*cloudstack.NetworkACL) { r.State = "Deleting" }),
+ legacyACLRule("egress-tcp-80", func(r
*cloudstack.NetworkACL) { r.Traffictype = "Egress" }),
+ legacyACLRule("udp-80", func(r
*cloudstack.NetworkACL) { r.Protocol = "udp" }),
+ legacyACLRule("tcp-81", func(r
*cloudstack.NetworkACL) { r.Startport, r.Endport = "81", "81" }),
+ legacyACLRule("tcp-80-81", func(r
*cloudstack.NetworkACL) { r.Endport = "81" }),
+ legacyACLRule("all", func(r
*cloudstack.NetworkACL) { r.Protocol, r.Startport, r.Endport = "all", "", "" }),
+ },
+ }, nil)
+
+ lb := &loadBalancer{
+ CloudStackClient:
&cloudstack.CloudStackClient{NetworkACL: mockNetworkACL},
+ projectID: "proj-1",
+ }
+
+ aclRules, err := lb.listNetworkACLRules("net-1", "tcp", 80)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+
+ var ids []string
+ for _, aclRule := range aclRules {
+ ids = append(ids, aclRule.Id)
+ }
+ if want := []string{"tcp-80", "deleting-tcp-80"};
!compareStringSlice(ids, want) {
+ t.Errorf("rules = %v, want %v", ids, want)
+ }
+ if networkID, _ := listParams.GetNetworkid(); networkID !=
"net-1" {
+ t.Errorf("networkid = %q, want net-1", networkID)
+ }
+ if listAll, _ := listParams.GetListall(); !listAll {
+ t.Errorf("listall not set")
+ }
+ if projectID, _ := listParams.GetProjectid(); projectID !=
"proj-1" {
+ t.Errorf("projectid = %q, want proj-1", projectID)
+ }
+ })
+
+ t.Run("a rule repeated across pages is returned once", func(t
*testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ var all []*cloudstack.NetworkACL
+ for i := 0; i < 501; i++ {
+ all = append(all, legacyACLRule(fmt.Sprintf("acl-%d",
i), func(r *cloudstack.NetworkACL) { r.Startport, r.Endport = "81", "81" }))
+ }
+ all[499] = ownACLRule("acl-own")
+ all[500] = ownACLRule("acl-own")
+
+ mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl)
+ listParams := &cloudstack.ListNetworkACLsParams{}
+
mockNetworkACL.EXPECT().NewListNetworkACLsParams().Return(listParams)
+
mockNetworkACL.EXPECT().ListNetworkACLs(listParams).DoAndReturn(func(p
*cloudstack.ListNetworkACLsParams) (*cloudstack.ListNetworkACLsResponse, error)
{
+ return &cloudstack.ListNetworkACLsResponse{Count:
len(all), NetworkACLs: pageOf(t, p, all, 500)}, nil
+ }).Times(2)
+
+ lb := &loadBalancer{CloudStackClient:
&cloudstack.CloudStackClient{NetworkACL: mockNetworkACL}}
+
+ aclRules, err := lb.listNetworkACLRules("net-1", "tcp", 80)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if len(aclRules) != 1 {
+ t.Errorf("rules = %d, want acl-own once", len(aclRules))
+ }
+ })
+
+ t.Run("pages through every rule of the tier once", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ var all []*cloudstack.NetworkACL
+ for i := 0; i < 600; i++ {
+ all = append(all, legacyACLRule(fmt.Sprintf("acl-%d",
i), func(r *cloudstack.NetworkACL) { r.Startport, r.Endport = "81", "81" }))
+ }
+ all[550] = ownACLRule("acl-own")
+
+ mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl)
+ listParams := &cloudstack.ListNetworkACLsParams{}
+
mockNetworkACL.EXPECT().NewListNetworkACLsParams().Return(listParams)
+
mockNetworkACL.EXPECT().ListNetworkACLs(listParams).DoAndReturn(func(p
*cloudstack.ListNetworkACLsParams) (*cloudstack.ListNetworkACLsResponse, error)
{
+ return &cloudstack.ListNetworkACLsResponse{Count:
len(all), NetworkACLs: pageOf(t, p, all, 500)}, nil
+ }).Times(2)
+
+ lb := &loadBalancer{CloudStackClient:
&cloudstack.CloudStackClient{NetworkACL: mockNetworkACL}}
+
+ aclRules, err := lb.listNetworkACLRules("net-1", "tcp", 80)
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if len(aclRules) != 1 || aclRules[0].Id != "acl-own" {
+ t.Errorf("rules = %v, want only acl-own from the second
page", aclRules)
+ }
+ })
+}
+
+// sentUpdateFields names the fields of an updateNetworkACLItem call that were
set, apart from
+// the id, so a test can assert that an adoption sends nothing but the reason.
+func sentUpdateFields(p *cloudstack.UpdateNetworkACLItemParams) []string {
+ var sent []string
+ set := func(name string, ok bool) {
+ if ok {
+ sent = append(sent, name)
+ }
+ }
+ _, ok := p.GetAction()
+ set("action", ok)
+ _, ok = p.GetCidrlist()
+ set("cidrlist", ok)
+ _, ok = p.GetCustomid()
+ set("customid", ok)
+ _, ok = p.GetEndport()
+ set("endport", ok)
+ _, ok = p.GetFordisplay()
+ set("fordisplay", ok)
+ _, ok = p.GetIcmpcode()
+ set("icmpcode", ok)
+ _, ok = p.GetIcmptype()
+ set("icmptype", ok)
+ _, ok = p.GetNumber()
+ set("number", ok)
+ _, ok = p.GetPartialupgrade()
+ set("partialupgrade", ok)
+ _, ok = p.GetProtocol()
+ set("protocol", ok)
+ _, ok = p.GetReason()
+ set("reason", ok)
+ _, ok = p.GetStartport()
+ set("startport", ok)
+ _, ok = p.GetTraffictype()
+ set("traffictype", ok)
+ return sent
+}
+
+// Regression test for issue #107: a Network ACL rule is owned by one Service,
so a rule of
+// another Service, or one an operator made, never stands in for this
Service's own rule.
+func TestUpdateNetworkACLOwnership(t *testing.T) {
+ notAllowed := fmt.Errorf("CloudStack API error 432
(CSExceptionErrorCode: 9999): The API [updateNetworkACLItem] does not exist or
is not available for the account")
+ otherService := func(r *cloudstack.NetworkACL) { r.Reason =
networkACLReasonPrefix + "aotheruid" }
+
+ tests := []struct {
+ name string
+ rules []*cloudstack.NetworkACL
+ protocol LoadBalancerProtocol
+ globalList bool
+ adopt string
+ adoptErr error
+ wantCreate bool
+ createErr error
+ wantErr string
+ }{
+ {name: "creates its own rule next to another service's rule",
rules: []*cloudstack.NetworkACL{legacyACLRule("acl-other", otherService)},
wantCreate: true},
+ {name: "an egress rule does not open the port", rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-egress", func(r
*cloudstack.NetworkACL) { r.Traffictype = "Egress" })}, wantCreate: true},
+ {name: "its own rule being deleted does not open the port",
rules: []*cloudstack.NetworkACL{legacyACLRule("acl-own", func(r
*cloudstack.NetworkACL) { r.Reason = testACLReason; r.State = "Deleting" })},
wantCreate: true},
+ {name: "adopts a rule from an earlier release", rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-legacy", nil)}, adopt:
"acl-legacy"},
+ {name: "adopts only one of several rules from an earlier
release", rules: []*cloudstack.NetworkACL{legacyACLRule("acl-legacy-1", nil),
legacyACLRule("acl-legacy-2", nil)}, adopt: "acl-legacy-1"},
+ {name: "keeps its own rule rather than adopting another",
rules: []*cloudstack.NetworkACL{legacyACLRule("acl-legacy", nil),
ownACLRule("acl-own")}},
+ {name: "adopts a rule from an earlier release before deferring
to an operator", rules: []*cloudstack.NetworkACL{legacyACLRule("acl-operator",
func(r *cloudstack.NetworkACL) { r.Cidrlist = "10.0.0.0/8" }),
legacyACLRule("acl-legacy", nil)}, adopt: "acl-legacy"},
+ {name: "creates its own rule when adopting is not allowed",
rules: []*cloudstack.NetworkACL{legacyACLRule("acl-legacy", nil)}, adopt:
"acl-legacy", adoptErr: notAllowed, wantCreate: true},
+ {name: "keeps relying on the old rule when adopting and
creating are both refused", rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-legacy", nil)}, adopt:
"acl-legacy", adoptErr: notAllowed, wantCreate: true, createErr:
fmt.Errorf("CloudStack API error 432 (CSExceptionErrorCode: 9999): The API
[createNetworkACL] does not exist or is not available for the account")},
+ {name: "adopts a udp rule from an earlier release", protocol:
LoadBalancerProtocolUDP, rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-legacy-udp", func(r
*cloudstack.NetworkACL) { r.Protocol = "udp" })}, adopt: "acl-legacy-udp"},
+ {name: "creates a udp rule next to a tcp rule on the same
port", protocol: LoadBalancerProtocolUDP, rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-legacy-tcp", nil)}, wantCreate:
true},
+ {name: "fails when adopting fails otherwise", rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-legacy", nil)}, adopt:
"acl-legacy", adoptErr: fmt.Errorf("router unreachable"), wantErr: "error
adopting Network ACL rule acl-legacy"},
+ {name: "leaves the port to an operator rule with a restricted
CIDR", rules: []*cloudstack.NetworkACL{legacyACLRule("acl-operator", func(r
*cloudstack.NetworkACL) { r.Cidrlist = "10.0.0.0/8" })}},
+ {name: "adds its own rule next to an upper case TCP rule, as
earlier releases did", rules: []*cloudstack.NetworkACL{legacyACLRule("acl-cks",
func(r *cloudstack.NetworkACL) { r.Protocol = "TCP"; r.Cidrlist =
"0.0.0.0/0,::/0" })}, wantCreate: true},
+ {name: "leaves the port to an operator deny rule", rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-deny", func(r
*cloudstack.NetworkACL) { r.Action = "Deny" })}},
+ {name: "never adopts in a global ACL list", rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-legacy", nil)}, globalList: true},
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ listVpcID := "vpc-1"
+ if tt.globalList {
+ listVpcID = ""
+ }
+
+ mockNetwork :=
cloudstack.NewMockNetworkServiceIface(ctrl)
+ mockNetworkACL :=
cloudstack.NewMockNetworkACLServiceIface(ctrl)
+ mockNetwork.EXPECT().GetNetworkByID("net-1",
gomock.Any()).Return(&cloudstack.Network{Id: "net-1", Aclid: "acl-1"}, 1, nil)
+ mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-1",
gomock.Any()).Return(&cloudstack.NetworkACLList{Id: "acl-1", Name: "tier-acl",
Vpcid: listVpcID}, 1, nil)
+
mockNetworkACL.EXPECT().NewListNetworkACLsParams().Return(&cloudstack.ListNetworkACLsParams{})
+
mockNetworkACL.EXPECT().ListNetworkACLs(gomock.Any()).Return(&cloudstack.ListNetworkACLsResponse{Count:
len(tt.rules), NetworkACLs: tt.rules}, nil)
+
+ updateParams := &cloudstack.UpdateNetworkACLItemParams{}
+ if tt.adopt != "" {
+
mockNetworkACL.EXPECT().NewUpdateNetworkACLItemParams(tt.adopt).Return(updateParams)
+
mockNetworkACL.EXPECT().UpdateNetworkACLItem(updateParams).Return(&cloudstack.UpdateNetworkACLItemResponse{},
tt.adoptErr)
+ }
+ createParams := &cloudstack.CreateNetworkACLParams{}
+ if tt.wantCreate {
+
mockNetworkACL.EXPECT().NewCreateNetworkACLParams(tt.protocol.IPProtocol()).Return(createParams)
+
mockNetworkACL.EXPECT().CreateNetworkACL(createParams).Return(&cloudstack.CreateNetworkACLResponse{Id:
"acl-new"}, tt.createErr)
+ }
+
+ lb := &loadBalancer{
+ name: "atestuid",
+ CloudStackClient:
&cloudstack.CloudStackClient{Network: mockNetwork, NetworkACL: mockNetworkACL},
+ }
+
+ updated, err := lb.updateNetworkACL(80, tt.protocol,
"net-1")
+ if tt.wantErr != "" {
+ if err == nil || !strings.Contains(err.Error(),
tt.wantErr) {
+ t.Fatalf("error = %v, want it to
contain %q", err, tt.wantErr)
+ }
+ return
+ }
+ if err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if !updated {
+ t.Errorf("updated = false, want true")
+ }
+ if tt.adopt != "" {
+ if sent := sentUpdateFields(updateParams);
!compareStringSlice(sent, []string{"reason"}) {
+ t.Errorf("adoption sent %v, want only
the reason", sent)
+ }
+ if reason, _ := updateParams.GetReason();
reason != testACLReason {
+ t.Errorf("adopted with reason %q, want
%q", reason, testACLReason)
+ }
+ }
+ if tt.wantCreate {
+ if reason, _ := createParams.GetReason();
reason != testACLReason {
+ t.Errorf("created with reason %q, want
%q", reason, testACLReason)
+ }
+ if networkID, _ := createParams.GetNetworkid();
networkID != "net-1" {
+ t.Errorf("created in network %q, want
net-1", networkID)
+ }
+ }
+ })
+ }
+}
+
+func TestDeleteNetworkACLRuleOwnership(t *testing.T) {
+ otherService := func(r *cloudstack.NetworkACL) { r.Reason =
networkACLReasonPrefix + "aotheruid" }
+ deleting := func(r *cloudstack.NetworkACL) { r.Reason = testACLReason;
r.State = "Deleting" }
+ firstErr := fmt.Errorf("first delete failed")
+
+ tests := []struct {
+ name string
+ protocol LoadBalancerProtocol
+ rules []*cloudstack.NetworkACL
+ deleteErrs map[string]error
+ wantDeleted []string
+ wantErr error
+ }{
+ {
+ name: "deletes only this service's rules",
+ rules: []*cloudstack.NetworkACL{
+ legacyACLRule("acl-other", otherService),
+ ownACLRule("acl-own"),
+ legacyACLRule("acl-legacy", nil),
+ legacyACLRule("acl-operator", func(r
*cloudstack.NetworkACL) { r.Cidrlist = "10.0.0.0/8" }),
+ legacyACLRule("acl-cks", func(r
*cloudstack.NetworkACL) { r.Protocol = "TCP" }),
+ },
+ wantDeleted: []string{"acl-own"},
+ },
+ {
+ name: "deletes its own rule again while it is
being deleted",
+ rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-own", deleting)},
+ wantDeleted: []string{"acl-own"},
+ },
+ {
+ name: "only logs a failure on a rule already
being deleted",
+ rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-own", deleting)},
+ deleteErrs: map[string]error{"acl-own":
fmt.Errorf("router unreachable")},
+ wantDeleted: []string{"acl-own"},
+ },
+ {
+ name: "tries every rule and reports the first
error",
+ rules:
[]*cloudstack.NetworkACL{ownACLRule("acl-own-1"), ownACLRule("acl-own-2")},
+ deleteErrs: map[string]error{"acl-own-1": firstErr,
"acl-own-2": fmt.Errorf("second delete failed")},
+ wantDeleted: []string{"acl-own-1", "acl-own-2"},
+ wantErr: firstErr,
+ },
+ {
+ name: "deletes a udp rule but not the tcp rule on
the same port",
+ protocol: LoadBalancerProtocolUDP,
+ rules: []*cloudstack.NetworkACL{
+ ownACLRule("acl-own-tcp"),
+ legacyACLRule("acl-own-udp", func(r
*cloudstack.NetworkACL) { r.Reason = testACLReason; r.Protocol = "udp" }),
+ },
+ wantDeleted: []string{"acl-own-udp"},
+ },
+ {
+ name: "deletes nothing when no rule is this service's",
+ rules:
[]*cloudstack.NetworkACL{legacyACLRule("acl-legacy", nil),
legacyACLRule("acl-other", otherService)},
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockNetworkACL :=
cloudstack.NewMockNetworkACLServiceIface(ctrl)
+
mockNetworkACL.EXPECT().NewListNetworkACLsParams().Return(&cloudstack.ListNetworkACLsParams{})
+
mockNetworkACL.EXPECT().ListNetworkACLs(gomock.Any()).Return(&cloudstack.ListNetworkACLsResponse{Count:
len(tt.rules), NetworkACLs: tt.rules}, nil)
+
+ var deleted []string
+ for _, id := range tt.wantDeleted {
+ deleteParams :=
&cloudstack.DeleteNetworkACLParams{}
+
mockNetworkACL.EXPECT().NewDeleteNetworkACLParams(id).Return(deleteParams)
+
mockNetworkACL.EXPECT().DeleteNetworkACL(deleteParams).DoAndReturn(func(*cloudstack.DeleteNetworkACLParams)
(*cloudstack.DeleteNetworkACLResponse, error) {
+ deleted = append(deleted, id)
+ if err := tt.deleteErrs[id]; err != nil
{
+ return nil, err
+ }
+ return
&cloudstack.DeleteNetworkACLResponse{}, nil
+ })
+ }
+
+ lb := &loadBalancer{
+ name: "atestuid",
+ CloudStackClient:
&cloudstack.CloudStackClient{NetworkACL: mockNetworkACL},
+ }
+
+ ok, err := lb.deleteNetworkACLRule(80, tt.protocol,
"net-1")
+ if err != tt.wantErr {
+ t.Errorf("error = %v, want %v", err, tt.wantErr)
+ }
+ if ok != (tt.wantErr == nil) {
+ t.Errorf("ok = %v, want %v", ok, tt.wantErr ==
nil)
+ }
+ if !compareStringSlice(deleted, tt.wantDeleted) {
+ t.Errorf("deleted %v, want %v", deleted,
tt.wantDeleted)
+ }
+ })
+ }
+}
+
+// Firewall rules are not shared like Network ACL rules (issue #107): Services
can only share a
+// public IP on different ports, so pruning touches the firewall rules of the
rule's own IP only.
+func TestPruneRulesScopesFirewallRulesToTheirIP(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
+ mockFirewall := cloudstack.NewMockFirewallServiceIface(ctrl)
+ listParams := &cloudstack.ListFirewallRulesParams{}
+ deleteParams := &cloudstack.DeleteFirewallRuleParams{}
+ gomock.InOrder(
+
mockFirewall.EXPECT().NewListFirewallRulesParams().Return(listParams),
+
mockFirewall.EXPECT().ListFirewallRules(listParams).Return(&cloudstack.ListFirewallRulesResponse{
+ Count: 1,
+ FirewallRules: []*cloudstack.FirewallRule{{Id:
"fw-old", Protocol: "tcp", Startport: 80, Endport: 80, Cidrlist:
defaultAllowedCIDR}},
+ }, nil),
+
mockFirewall.EXPECT().NewDeleteFirewallRuleParams("fw-old").Return(deleteParams),
+
mockFirewall.EXPECT().DeleteFirewallRule(deleteParams).Return(&cloudstack.DeleteFirewallRuleResponse{},
nil),
+
mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-old").Return(&cloudstack.DeleteLoadBalancerRuleParams{}),
+
mockLB.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{},
nil),
+ )
+
+ lb := &loadBalancer{
+ CloudStackClient: &cloudstack.CloudStackClient{LoadBalancer:
mockLB, Firewall: mockFirewall},
+ name: "atestuid",
+ networkID: "net-1",
+ ipAddrID: "ip-current",
+ }
+ network := &cloudstack.Network{Id: "net-1", Service:
[]cloudstack.NetworkServiceInternal{{Name: "Firewall"}}}
+ obsolete := []obsoleteRule{{
+ rule: &cloudstack.LoadBalancerRule{Id: "rule-old", Name:
"atestuid-tcp-80", Publicipid: "ip-old", Publicport: "80", Protocol: "tcp",
Networkid: "net-1"},
+ protocol: LoadBalancerProtocolTCP,
+ tuple: portProtocol{"tcp", 80},
+ }}
+ desired := []desiredLBRule{{
+ name: "atestuid-tcp-443",
+ port: corev1.ServicePort{Port: 443, NodePort: 30443,
Protocol: corev1.ProtocolTCP},
+ protocol: LoadBalancerProtocolTCP,
+ }}
+
+ if err := lb.pruneRules(obsolete, desired, network); err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if ipID, _ := listParams.GetIpaddressid(); ipID != "ip-old" {
+ t.Errorf("firewall rules listed on IP %q, want the rule's own
ip-old", ipID)
+ }
+}
+
func TestGetLoadBalancer(t *testing.T) {
t.Run("load balancer with existing rules", func(t *testing.T) {
ctrl := gomock.NewController(t)
@@ -4621,6 +5062,7 @@ type ensureLBTestEnv struct {
network *cloudstack.MockNetworkServiceIface
firewall *cloudstack.MockFirewallServiceIface
address *cloudstack.MockAddressServiceIface
+ acl *cloudstack.MockNetworkACLServiceIface
service *corev1.Service
nodes []*corev1.Node
@@ -4637,6 +5079,7 @@ func newEnsureLBTestEnv(ctrl *gomock.Controller,
annotations map[string]string,
network: cloudstack.NewMockNetworkServiceIface(ctrl),
firewall: cloudstack.NewMockFirewallServiceIface(ctrl),
address: cloudstack.NewMockAddressServiceIface(ctrl),
+ acl: cloudstack.NewMockNetworkACLServiceIface(ctrl),
}
e.cs = &CSCloud{
@@ -4646,6 +5089,7 @@ func newEnsureLBTestEnv(ctrl *gomock.Controller,
annotations map[string]string,
Network: e.network,
Firewall: e.firewall,
Address: e.address,
+ NetworkACL: e.acl,
},
version: semver.Version{Major: 4, Minor: 22, Patch: 0},
}
@@ -5521,6 +5965,336 @@ func TestUpdateLoadBalancerPagination(t *testing.T) {
}
}
+// Regression test for issue #107 through a whole sync of a Service on a VPC
tier.
+func TestEnsureLoadBalancerNetworkACLOwnership(t *testing.T) {
+ vpcTier := &cloudstack.Network{
+ Id: "net-1",
+ Vpcid: "vpc-1",
+ Aclid: "acl-1",
+ Service: []cloudstack.NetworkServiceInternal{{Name:
"NetworkACL"}},
+ }
+ tierACLList := &cloudstack.NetworkACLList{Id: "acl-1", Name:
"tier-acl", Vpcid: "vpc-1"}
+ existingRule := 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",
+ }
+ }
+ otherServiceRule := legacyACLRule("acl-other", func(r
*cloudstack.NetworkACL) { r.Reason = networkACLReasonPrefix + "aotheruid" })
+
+ t.Run("a service sharing a port with another service opens it with its
own rule", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ env := newEnsureLBTestEnv(ctrl, nil,
[]corev1.ServicePort{{Port: 80, NodePort: 30000, Protocol: corev1.ProtocolTCP}})
+ env.expectHosts()
+ env.expectRules(existingRule())
+ env.network.EXPECT().GetNetworkByID("net-1",
gomock.Any()).Return(vpcTier, 1, nil).Times(2)
+ env.acl.EXPECT().GetNetworkACLListByID("acl-1",
gomock.Any()).Return(tierACLList, 1, nil)
+
env.acl.EXPECT().NewListNetworkACLsParams().Return(&cloudstack.ListNetworkACLsParams{})
+
env.acl.EXPECT().ListNetworkACLs(gomock.Any()).Return(&cloudstack.ListNetworkACLsResponse{Count:
1, NetworkACLs: []*cloudstack.NetworkACL{otherServiceRule}}, nil)
+ createParams := &cloudstack.CreateNetworkACLParams{}
+
env.acl.EXPECT().NewCreateNetworkACLParams("tcp").Return(createParams)
+
env.acl.EXPECT().CreateNetworkACL(createParams).Return(&cloudstack.CreateNetworkACLResponse{Id:
"acl-own"}, nil)
+
+ if _, err := env.cs.EnsureLoadBalancer(context.TODO(),
"test-cluster", env.service, env.nodes); err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if reason, _ := createParams.GetReason(); reason !=
testACLReason {
+ t.Errorf("created with reason %q, want %q", reason,
testACLReason)
+ }
+ })
+
+ t.Run("a port change deletes only its own rule on the old port", func(t
*testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ env := newEnsureLBTestEnv(ctrl, nil,
[]corev1.ServicePort{{Port: 8080, NodePort: 30000, Protocol:
corev1.ProtocolTCP}})
+ env.expectHosts()
+ env.expectRules(existingRule())
+ env.network.EXPECT().GetNetworkByID("net-1",
gomock.Any()).Return(vpcTier, 1, nil).Times(2)
+
+ env.lb.EXPECT().NewCreateLoadBalancerRuleParams("roundrobin",
"atestuid-tcp-8080", 30000,
8080).Return(&cloudstack.CreateLoadBalancerRuleParams{})
+
env.lb.EXPECT().CreateLoadBalancerRule(gomock.Any()).Return(&cloudstack.CreateLoadBalancerRuleResponse{
+ Id: "rule-2", Name: "atestuid-tcp-8080", Publicip:
"10.0.0.1", Publicipid: "ip-1", Publicport: "8080", Protocol: "tcp", Networkid:
"net-1",
+ }, nil)
+
env.lb.EXPECT().NewAssignToLoadBalancerRuleParams("rule-2").Return(&cloudstack.AssignToLoadBalancerRuleParams{})
+
env.lb.EXPECT().AssignToLoadBalancerRule(gomock.Any()).Return(&cloudstack.AssignToLoadBalancerRuleResponse{},
nil)
+
+ tierRules := []*cloudstack.NetworkACL{otherServiceRule,
ownACLRule("acl-own-80")}
+ env.acl.EXPECT().GetNetworkACLListByID("acl-1",
gomock.Any()).Return(tierACLList, 1, nil)
+
env.acl.EXPECT().NewListNetworkACLsParams().Return(&cloudstack.ListNetworkACLsParams{}).Times(2)
+
env.acl.EXPECT().ListNetworkACLs(gomock.Any()).Return(&cloudstack.ListNetworkACLsResponse{Count:
len(tierRules), NetworkACLs: tierRules}, nil).Times(2)
+
env.acl.EXPECT().NewCreateNetworkACLParams("tcp").Return(&cloudstack.CreateNetworkACLParams{})
+
env.acl.EXPECT().CreateNetworkACL(gomock.Any()).Return(&cloudstack.CreateNetworkACLResponse{Id:
"acl-own-8080"}, nil)
+ deleteParams := &cloudstack.DeleteNetworkACLParams{}
+
env.acl.EXPECT().NewDeleteNetworkACLParams("acl-own-80").Return(deleteParams)
+
env.acl.EXPECT().DeleteNetworkACL(deleteParams).Return(&cloudstack.DeleteNetworkACLResponse{},
nil)
+
+
env.lb.EXPECT().NewDeleteLoadBalancerRuleParams("rule-1").Return(&cloudstack.DeleteLoadBalancerRuleParams{})
+
env.lb.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{},
nil)
+
+ if _, err := env.cs.EnsureLoadBalancer(context.TODO(),
"test-cluster", env.service, env.nodes); err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ })
+}
+
+func TestEnsureLoadBalancerDeletedNetworkACLOwnership(t *testing.T) {
+ newEnv := func(t *testing.T) (*CSCloud,
*cloudstack.MockLoadBalancerServiceIface,
*cloudstack.MockNetworkACLServiceIface, *cloudstack.MockAddressServiceIface,
*cloudstack.ListNetworkACLsParams) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
+ mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+ mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl)
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+
+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{})
+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
+ Count: 1,
+ LoadBalancerRules: []*cloudstack.LoadBalancerRule{{
+ Id: "rule-1", Name: "atestuid-tcp-80",
Publicip: "10.0.0.1", Publicipid: "ip-1", Publicport: "80", Protocol: "tcp",
Networkid: "net-tier",
+ }},
+ }, nil)
+ mockNetwork.EXPECT().GetNetworkByID("net-tier",
gomock.Any()).Return(&cloudstack.Network{Id: "net-tier", Vpcid: "vpc-1"}, 1,
nil)
+
+ listParams := &cloudstack.ListNetworkACLsParams{}
+
mockNetworkACL.EXPECT().NewListNetworkACLsParams().Return(listParams)
+
mockNetworkACL.EXPECT().ListNetworkACLs(gomock.Any()).Return(&cloudstack.ListNetworkACLsResponse{
+ Count: 3,
+ NetworkACLs: []*cloudstack.NetworkACL{
+ legacyACLRule("acl-other", func(r
*cloudstack.NetworkACL) { r.Reason = networkACLReasonPrefix + "aotheruid" }),
+ ownACLRule("acl-own"),
+ legacyACLRule("acl-operator", func(r
*cloudstack.NetworkACL) { r.Cidrlist = "10.0.0.0/8" }),
+ },
+ }, nil)
+
+ cs := &CSCloud{client: &cloudstack.CloudStackClient{
+ LoadBalancer: mockLB, Network: mockNetwork, NetworkACL:
mockNetworkACL, Address: mockAddress,
+ }}
+ return cs, mockLB, mockNetworkACL, mockAddress, listParams
+ }
+ service := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name:
"test-service", Namespace: "default", UID: "test-uid"}}
+
+ t.Run("deletes only its own ACL rule, in the rule's own network, then
the rule and the IP", func(t *testing.T) {
+ cs, mockLB, mockNetworkACL, mockAddress, listParams := newEnv(t)
+ deleteACL := &cloudstack.DeleteNetworkACLParams{}
+ deleteRule := &cloudstack.DeleteLoadBalancerRuleParams{}
+ release := &cloudstack.DisassociateIpAddressParams{}
+ gomock.InOrder(
+
mockNetworkACL.EXPECT().NewDeleteNetworkACLParams("acl-own").Return(deleteACL),
+
mockNetworkACL.EXPECT().DeleteNetworkACL(deleteACL).Return(&cloudstack.DeleteNetworkACLResponse{},
nil),
+
mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-1").Return(deleteRule),
+
mockLB.EXPECT().DeleteLoadBalancerRule(deleteRule).Return(&cloudstack.DeleteLoadBalancerRuleResponse{},
nil),
+
mockAddress.EXPECT().NewDisassociateIpAddressParams("ip-1").Return(release),
+
mockAddress.EXPECT().DisassociateIpAddress(release).Return(&cloudstack.DisassociateIpAddressResponse{},
nil),
+ )
+
+ if err := cs.EnsureLoadBalancerDeleted(context.TODO(),
"test-cluster", service); err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if networkID, _ := listParams.GetNetworkid(); networkID !=
"net-tier" {
+ t.Errorf("ACL rules listed on network %q, want the
rule's own net-tier", networkID)
+ }
+ })
+
+ t.Run("deletes the other rules, keeps the failing one and reports both
failures", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
+ mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+ mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl)
+ mockFirewall := cloudstack.NewMockFirewallServiceIface(ctrl)
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+
+ rule80 := &cloudstack.LoadBalancerRule{Id: "rule-80", Name:
"atestuid-tcp-80", Publicip: "10.0.0.1", Publicipid: "ip-1", Publicport: "80",
Protocol: "tcp", Networkid: "net-tier"}
+ rule8080 := &cloudstack.LoadBalancerRule{Id: "rule-8080", Name:
"atestuid-tcp-8080", Publicip: "10.0.0.1", Publicipid: "ip-1", Publicport:
"8080", Protocol: "tcp", Networkid: "net-tier"}
+ duplicate := &cloudstack.LoadBalancerRule{Id: "rule-dup", Name:
"atestuid-tcp-80", Publicip: "10.0.0.2", Publicipid: "ip-dup", Publicport:
"80", Protocol: "tcp", Networkid: "net-tier"}
+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{})
+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
+ Count: 3, LoadBalancerRules:
[]*cloudstack.LoadBalancerRule{rule80, rule8080, duplicate},
+ }, nil)
+
mockFirewall.EXPECT().NewListFirewallRulesParams().Return(&cloudstack.ListFirewallRulesParams{})
+
mockFirewall.EXPECT().ListFirewallRules(gomock.Any()).Return(nil,
fmt.Errorf("firewall API down"))
+
+ mockNetwork.EXPECT().GetNetworkByID("net-tier",
gomock.Any()).Return(&cloudstack.Network{Id: "net-tier", Vpcid: "vpc-1"}, 1,
nil).Times(2)
+ tierRules := []*cloudstack.NetworkACL{
+ ownACLRule("acl-own-80"),
+ legacyACLRule("acl-own-8080", func(r
*cloudstack.NetworkACL) { r.Reason = testACLReason; r.Startport, r.Endport =
"8080", "8080" }),
+ }
+
mockNetworkACL.EXPECT().NewListNetworkACLsParams().DoAndReturn(func()
*cloudstack.ListNetworkACLsParams {
+ return &cloudstack.ListNetworkACLsParams{}
+ }).Times(2)
+
mockNetworkACL.EXPECT().ListNetworkACLs(gomock.Any()).Return(&cloudstack.ListNetworkACLsResponse{Count:
len(tierRules), NetworkACLs: tierRules}, nil).Times(2)
+ deleting := map[*cloudstack.DeleteNetworkACLParams]string{}
+
mockNetworkACL.EXPECT().NewDeleteNetworkACLParams(gomock.Any()).DoAndReturn(func(id
string) *cloudstack.DeleteNetworkACLParams {
+ p := &cloudstack.DeleteNetworkACLParams{}
+ deleting[p] = id
+ return p
+ }).Times(2)
+
mockNetworkACL.EXPECT().DeleteNetworkACL(gomock.Any()).DoAndReturn(func(p
*cloudstack.DeleteNetworkACLParams) (*cloudstack.DeleteNetworkACLResponse,
error) {
+ if deleting[p] == "acl-own-80" {
+ return nil, fmt.Errorf("router unreachable")
+ }
+ return &cloudstack.DeleteNetworkACLResponse{}, nil
+ }).Times(2)
+ deleteRule := &cloudstack.DeleteLoadBalancerRuleParams{}
+
mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-8080").Return(deleteRule)
+
mockLB.EXPECT().DeleteLoadBalancerRule(deleteRule).Return(&cloudstack.DeleteLoadBalancerRuleResponse{},
nil)
+
+ cs := &CSCloud{client: &cloudstack.CloudStackClient{
+ LoadBalancer: mockLB, Network: mockNetwork, NetworkACL:
mockNetworkACL, Firewall: mockFirewall, Address: mockAddress,
+ }}
+
+ err := cs.EnsureLoadBalancerDeleted(context.TODO(),
"test-cluster", service)
+ if err == nil || !strings.Contains(err.Error(), "router
unreachable") || !strings.Contains(err.Error(), "firewall API down") {
+ t.Fatalf("error = %v, want both the ACL rule and the
sweep failure", err)
+ }
+ })
+
+ t.Run("keeps the rule and the IP when its ACL rule cannot be deleted",
func(t *testing.T) {
+ cs, _, mockNetworkACL, _, _ := newEnv(t)
+ deleteACL := &cloudstack.DeleteNetworkACLParams{}
+
mockNetworkACL.EXPECT().NewDeleteNetworkACLParams("acl-own").Return(deleteACL)
+ mockNetworkACL.EXPECT().DeleteNetworkACL(deleteACL).Return(nil,
fmt.Errorf("router unreachable"))
+
+ err := cs.EnsureLoadBalancerDeleted(context.TODO(),
"test-cluster", service)
+ if err == nil || !strings.Contains(err.Error(), "router
unreachable") {
+ t.Fatalf("error = %v, want the ACL rule failure
reported for a retry", err)
+ }
+ })
+}
+
+// CloudStack reports a load balancer rule's network, but when it is missing
the network is looked
+// up from the rule's own public IP, not the one the load balancer was
published on.
+func TestEnsureLoadBalancerDeletedLooksUpTheNetworkOfEachRulesIP(t *testing.T)
{
+ service := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name:
"test-service", Namespace: "default", UID: "test-uid"}}
+ ruleOn := func(id, name, port, ip, ipID string)
*cloudstack.LoadBalancerRule {
+ return &cloudstack.LoadBalancerRule{Id: id, Name: name,
Publicip: ip, Publicipid: ipID, Publicport: port, Protocol: "tcp"}
+ }
+
+ t.Run("each rule's network comes from its own IP", func(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
+ mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+ mockFirewall := cloudstack.NewMockFirewallServiceIface(ctrl)
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+
+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{})
+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
+ Count: 2,
+ LoadBalancerRules: []*cloudstack.LoadBalancerRule{
+ ruleOn("rule-80", "atestuid-tcp-80", "80",
"203.0.113.1", "ip-a"),
+ ruleOn("rule-8080", "atestuid-tcp-8080",
"8080", "203.0.113.2", "ip-b"),
+ },
+ }, nil)
+ mockAddress.EXPECT().GetPublicIpAddressByID("ip-a",
gomock.Any()).Return(&cloudstack.PublicIpAddress{Id: "ip-a",
Associatednetworkid: "net-a"}, 1, nil)
+ mockAddress.EXPECT().GetPublicIpAddressByID("ip-b",
gomock.Any()).Return(&cloudstack.PublicIpAddress{Id: "ip-b",
Associatednetworkid: "net-b"}, 1, nil)
+ mockNetwork.EXPECT().GetNetworkByID("net-a",
gomock.Any()).Return(&cloudstack.Network{Id: "net-a"}, 1, nil).Times(2)
+ mockNetwork.EXPECT().GetNetworkByID("net-b",
gomock.Any()).Return(&cloudstack.Network{Id: "net-b"}, 1, nil).Times(2)
+
mockFirewall.EXPECT().NewListFirewallRulesParams().DoAndReturn(func()
*cloudstack.ListFirewallRulesParams {
+ return &cloudstack.ListFirewallRulesParams{}
+ }).Times(2)
+
mockFirewall.EXPECT().ListFirewallRules(gomock.Any()).Return(&cloudstack.ListFirewallRulesResponse{},
nil).Times(2)
+
mockLB.EXPECT().NewDeleteLoadBalancerRuleParams(gomock.Any()).DoAndReturn(func(string)
*cloudstack.DeleteLoadBalancerRuleParams {
+ return &cloudstack.DeleteLoadBalancerRuleParams{}
+ }).Times(2)
+
mockLB.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Return(&cloudstack.DeleteLoadBalancerRuleResponse{},
nil).Times(2)
+ release := &cloudstack.DisassociateIpAddressParams{}
+
mockAddress.EXPECT().NewDisassociateIpAddressParams("ip-a").Return(release)
+
mockAddress.EXPECT().DisassociateIpAddress(release).Return(&cloudstack.DisassociateIpAddressResponse{},
nil)
+
+ cs := &CSCloud{client: &cloudstack.CloudStackClient{
+ LoadBalancer: mockLB, Network: mockNetwork, Firewall:
mockFirewall, Address: mockAddress,
+ }}
+
+ if err := cs.EnsureLoadBalancerDeleted(context.TODO(),
"test-cluster", service); err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ })
+
+ t.Run("a failed lookup keeps the rule and is reported", func(t
*testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{})
+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
+ Count: 1,
+ LoadBalancerRules:
[]*cloudstack.LoadBalancerRule{ruleOn("rule-80", "atestuid-tcp-80", "80",
"203.0.113.1", "ip-a")},
+ }, nil)
+ mockAddress.EXPECT().GetPublicIpAddressByID("ip-a",
gomock.Any()).Return(nil, -1, fmt.Errorf("API unavailable"))
+
+ cs := &CSCloud{client:
&cloudstack.CloudStackClient{LoadBalancer: mockLB, Address: mockAddress}}
+
+ err := cs.EnsureLoadBalancerDeleted(context.TODO(),
"test-cluster", service)
+ if err == nil || !strings.Contains(err.Error(), "API
unavailable") {
+ t.Fatalf("error = %v, want the lookup failure", err)
+ }
+ })
+}
+
+func TestEnsureLoadBalancerDeletedFirewallUsesRuleNetwork(t *testing.T) {
+ ctrl := gomock.NewController(t)
+ t.Cleanup(ctrl.Finish)
+
+ mockLB := cloudstack.NewMockLoadBalancerServiceIface(ctrl)
+ mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+ mockFirewall := cloudstack.NewMockFirewallServiceIface(ctrl)
+ mockAddress := cloudstack.NewMockAddressServiceIface(ctrl)
+
+
mockLB.EXPECT().NewListLoadBalancerRulesParams().Return(&cloudstack.ListLoadBalancerRulesParams{})
+
mockLB.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(&cloudstack.ListLoadBalancerRulesResponse{
+ Count: 1,
+ LoadBalancerRules: []*cloudstack.LoadBalancerRule{{
+ Id: "rule-1", Name: "atestuid-tcp-80", Publicip:
"203.0.113.1", Publicipid: "ip-1", Publicport: "80", Protocol: "tcp",
Networkid: "net-iso",
+ }},
+ }, nil)
+ mockNetwork.EXPECT().GetNetworkByID("net-iso",
gomock.Any()).Return(&cloudstack.Network{Id: "net-iso"}, 1, nil)
+ listParams := &cloudstack.ListFirewallRulesParams{}
+ deleteFirewall := &cloudstack.DeleteFirewallRuleParams{}
+ deleteRule := &cloudstack.DeleteLoadBalancerRuleParams{}
+ release := &cloudstack.DisassociateIpAddressParams{}
+ gomock.InOrder(
+
mockFirewall.EXPECT().NewListFirewallRulesParams().Return(listParams),
+
mockFirewall.EXPECT().ListFirewallRules(listParams).Return(&cloudstack.ListFirewallRulesResponse{
+ Count: 1,
+ FirewallRules: []*cloudstack.FirewallRule{{Id: "fw-1",
Protocol: "tcp", Startport: 80, Endport: 80, Cidrlist: defaultAllowedCIDR}},
+ }, nil),
+
mockFirewall.EXPECT().NewDeleteFirewallRuleParams("fw-1").Return(deleteFirewall),
+
mockFirewall.EXPECT().DeleteFirewallRule(deleteFirewall).Return(&cloudstack.DeleteFirewallRuleResponse{},
nil),
+
mockLB.EXPECT().NewDeleteLoadBalancerRuleParams("rule-1").Return(deleteRule),
+
mockLB.EXPECT().DeleteLoadBalancerRule(deleteRule).Return(&cloudstack.DeleteLoadBalancerRuleResponse{},
nil),
+
mockAddress.EXPECT().NewDisassociateIpAddressParams("ip-1").Return(release),
+
mockAddress.EXPECT().DisassociateIpAddress(release).Return(&cloudstack.DisassociateIpAddressResponse{},
nil),
+ )
+
+ cs := &CSCloud{client: &cloudstack.CloudStackClient{
+ LoadBalancer: mockLB, Network: mockNetwork, Firewall:
mockFirewall, Address: mockAddress,
+ }}
+ service := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Name:
"test-service", Namespace: "default", UID: "test-uid"}}
+
+ if err := cs.EnsureLoadBalancerDeleted(context.TODO(), "test-cluster",
service); err != nil {
+ t.Fatalf("unexpected error: %v", err)
+ }
+ if ipID, _ := listParams.GetIpaddressid(); ipID != "ip-1" {
+ t.Errorf("firewall rules listed on IP %q, want ip-1", ipID)
+ }
+}
+
// 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) {
@@ -5591,6 +6365,7 @@ func TestPruneRulesScopesNetworkACLsToTheirNetwork(t
*testing.T) {
Network: m.network,
Firewall: m.firewall,
},
+ name: "atestuid",
networkID: "net-new",
ipAddrID: "ip-current",
}, m
@@ -5601,9 +6376,12 @@ func TestPruneRulesScopesNetworkACLsToTheirNetwork(t
*testing.T) {
gomock.InOrder(
m.acl.EXPECT().NewListNetworkACLsParams().Return(listParams),
m.acl.EXPECT().ListNetworkACLs(gomock.Any()).Return(&cloudstack.ListNetworkACLsResponse{
- Count: 1,
+ Count: 4,
NetworkACLs: []*cloudstack.NetworkACL{
- {Id: "acl-rule-old", Protocol: "tcp",
Startport: "80", Endport: "80"},
+ legacyACLRule("acl-other-service",
func(r *cloudstack.NetworkACL) { r.Reason = networkACLReasonPrefix +
"aotheruid" }),
+ ownACLRule("acl-rule-old"),
+ legacyACLRule("acl-legacy", nil),
+ legacyACLRule("acl-operator", func(r
*cloudstack.NetworkACL) { r.Cidrlist = "10.0.0.0/8" }),
},
}, nil),
m.acl.EXPECT().NewDeleteNetworkACLParams("acl-rule-old").Return(&cloudstack.DeleteNetworkACLParams{}),
diff --git a/docs/development.md b/docs/development.md
index 9727f2c6..ce78e928 100644
--- a/docs/development.md
+++ b/docs/development.md
@@ -381,6 +381,11 @@ Each test creates its own namespace and cleans up after
itself. Because load
balancer provisioning is asynchronous, all assertions poll rather than
assuming immediate consistency.
+The VPC tests count every rule on their port in the tier's ACL list, and some
+create rules of their own on 8080 and 8085 the way an operator would, deleting
+them on cleanup. They stop at once if a rule for their port is left over from
+an earlier run.
+
### Known limitation: provider IDs
kind starts kubelet with `--provider-id=kind://docker/<cluster>/<node>`, and
@@ -420,7 +425,8 @@ so a run is reproducible; avoid floating tags like `latest`.
| Nodes keep the uninitialized taint; CCM logs `provided node ip for node
"..." is not valid` | The CloudStack VM's NIC IP does not match the IP kubelet
registered with. Recreate the VM with `ipaddress=` set to the kind node's
docker IP. |
| Services stay `<pending>`; CCM logs `none of the hosts matched the list of
VMs retrieved from CS API` | No CloudStack VM has a name matching a Kubernetes
node name. |
| CCM logs `found hosts that belong to different networks` | VMs matching the
node names exist on more than one network — typically leftovers from a previous
scenario. |
-| No ACL rules are created on a VPC network | The tier uses `default_allow` or
`default_deny`. The CCM only manages rules on custom ACL lists. |
+| No ACL rules are created on a VPC network | The tier uses `default_allow` or
`default_deny`; the CCM only manages rules on custom ACL lists. Or the list
already has an ingress rule for that port that the CCM did not create; the CCM
then relies on that rule and adds none of its own. |
+| A VPC test fails at once with `leftover ACL rule` | A rule for its port
survived an earlier run. Delete it from the tier's ACL list and rerun. |
| CoreDNS stuck `Pending` | Expected until the CCM removes the uninitialized
taint. If it persists, the CCM is not working — check its logs. |
| `kubectl logs`/`exec` fail after nodes initialize | The API server is
preferring the Hostname address, which the CCM set to the CloudStack instance
hostname. Use `kubelet-preferred-address-types: InternalIP` as the provided
kind config does. |
| Simulator never becomes ready | It runs `mvn jetty:run` and fetches from
Maven Central at startup. Check `docker logs cloudstack-simulator`. |
diff --git a/test/e2e/framework.go b/test/e2e/framework.go
index bed2cea8..163992ae 100644
--- a/test/e2e/framework.go
+++ b/test/e2e/framework.go
@@ -559,6 +559,60 @@ func (f *Framework) ACLRules(aclListID string)
([]*cloudstack.NetworkACL, error)
return resp.NetworkACLs, nil
}
+// aclRuleMarker is the reason the CCM gives the Network ACL rules it creates
for a load balancer.
+func aclRuleMarker(lbName string) string {
+ return "Managed by the CloudStack Kubernetes Provider for " + lbName
+}
+
+// ACLRule fetches one rule of an ACL list, or returns nil when the rule no
longer exists.
+func (f *Framework) ACLRule(aclListID, id string) (*cloudstack.NetworkACL,
error) {
+ rules, err := f.ACLRules(aclListID)
+ if err != nil {
+ return nil, err
+ }
+ for _, r := range rules {
+ if r.Id == id {
+ return r, nil
+ }
+ }
+ return nil, nil
+}
+
+// CreateACLRule adds an ingress tcp rule with no reason for one port to an
ACL list through the
+// CloudStack API, the way an operator would, and returns its ID. Cleanup
deletes the rule unless
+// it is already gone. The CIDR is always sent, because CloudStack 4.17 and
later stores
+// 0.0.0.0/0,::/0 when it is left out.
+func (f *Framework) CreateACLRule(aclListID string, port int, cidr string)
string {
+ f.T.Helper()
+ p := f.CS.NetworkACL.NewCreateNetworkACLParams("tcp")
+ p.SetAclid(aclListID)
+ p.SetAction("Allow")
+ p.SetCidrlist([]string{cidr})
+ p.SetStartport(port)
+ p.SetEndport(port)
+ p.SetTraffictype("Ingress")
+ resp, err := f.CS.NetworkACL.CreateNetworkACL(p)
+ if err != nil {
+ f.T.Fatalf("creating ACL rule for port %d: %v", port, err)
+ }
+
+ id := resp.Id
+ f.T.Cleanup(func() {
+ aclRule, err := f.ACLRule(aclListID, id)
+ if err != nil {
+ f.T.Errorf("looking up ACL rule %s for cleanup: %v",
id, err)
+ return
+ }
+ if aclRule == nil {
+ return
+ }
+ if _, err :=
f.CS.NetworkACL.DeleteNetworkACL(f.CS.NetworkACL.NewDeleteNetworkACLParams(id));
err != nil {
+ f.T.Errorf("deleting ACL rule %s: %v", id, err)
+ }
+ })
+ return id
+}
+
// PublicIP fetches a public IP address record by its ID.
func (f *Framework) PublicIP(id string) (*cloudstack.PublicIpAddress, error) {
p := f.CS.Address.NewListPublicIpAddressesParams()
diff --git a/test/e2e/vpc_test.go b/test/e2e/vpc_test.go
index 55392178..e3149539 100644
--- a/test/e2e/vpc_test.go
+++ b/test/e2e/vpc_test.go
@@ -24,10 +24,12 @@ package e2e
import (
"context"
"os"
+ "slices"
"strconv"
"strings"
"testing"
+ "github.com/apache/cloudstack-go/v2/cloudstack"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
@@ -55,8 +57,8 @@ func vpcFramework(t *testing.T) (*Framework, string, string) {
// TestVPC_LoadBalancer covers the VPC path end to end: the LB rule is
// created, the public IP is associated with the VPC, ingress traffic is
-// allowed via a Network ACL rule on the custom ACL list (not a firewall
-// rule), and everything is cleaned up on delete.
+// allowed via a Network ACL rule of the Service's own on the custom ACL list
+// (not a firewall rule), and everything is cleaned up on delete.
func TestVPC_LoadBalancer(t *testing.T) {
f, aclID, vpcID := vpcFramework(t)
@@ -85,7 +87,8 @@ func TestVPC_LoadBalancer(t *testing.T) {
if r.Startport == "80" && r.Endport == "80" &&
strings.EqualFold(r.Protocol, "tcp") &&
strings.EqualFold(r.Action, "Allow") &&
- strings.EqualFold(r.Traffictype,
"Ingress") {
+ strings.EqualFold(r.Traffictype,
"Ingress") &&
+ r.Reason == aclRuleMarker(lbName) {
return true, nil
}
}
@@ -163,8 +166,8 @@ func countACLRules(f *Framework, aclID, port string) (int,
error) {
func TestVPC_ACLRuleNotDuplicatedOnResync(t *testing.T) {
f, aclID, _ := vpcFramework(t)
- // An ACL rule belongs to the tier, not the service, so sharing port 80
with
- // TestVPC_LoadBalancer would blur the count.
+ // Every ACL rule on the port is counted, so no other Service may hold
one on
+ // it while this test runs; the VPC tests run one at a time and clean
up.
const port = "8080"
svc := f.CreateLBService(func(s *corev1.Service) {
s.Spec.Ports = []corev1.ServicePort{
@@ -270,9 +273,9 @@ func TestVPC_ExplicitLoadBalancerIPReleased(t *testing.T) {
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.
+ // Every ACL rule on the port is counted, so no other Service may hold
one on
+ // it while this test runs. 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) {
@@ -305,7 +308,7 @@ func TestVPC_ProxyProtocolACL(t *testing.T) {
}
}
- // Turning the annotation off keeps the one ACL rule: both protocols
share it.
+ // Turning the annotation off keeps the Service's one ACL rule: both
protocols share it.
f.UpdateService(svc, func(s *corev1.Service) {
delete(s.Annotations, annotationProxyProtocol)
})
@@ -326,3 +329,189 @@ func TestVPC_ProxyProtocolACL(t *testing.T) {
t.Errorf("ACL rules for port %s = %d, want exactly 1 across the
toggle", port, n)
}
}
+
+// aclRulesOnPort returns the ingress tcp ACL rules on the list for one port.
+func aclRulesOnPort(f *Framework, aclID, port string)
([]*cloudstack.NetworkACL, error) {
+ rules, err := f.ACLRules(aclID)
+ if err != nil {
+ return nil, err
+ }
+ var onPort []*cloudstack.NetworkACL
+ for _, r := range rules {
+ if r.Startport == port && r.Endport == port &&
strings.EqualFold(r.Protocol, "tcp") &&
+ strings.EqualFold(r.Traffictype, "Ingress") {
+ onPort = append(onPort, r)
+ }
+ }
+ return onPort, nil
+}
+
+// hasACLRuleWithReason reports whether one of the rules carries the reason.
+func hasACLRuleWithReason(rules []*cloudstack.NetworkACL, reason string) bool {
+ for _, r := range rules {
+ if r.Reason == reason {
+ return true
+ }
+ }
+ return false
+}
+
+// requireNoACLRules stops the test when a single-port rule for the port,
whatever its protocol or
+// direction, is left over from an earlier run, since the tests below count
those rules. A rule
+// for a port range is not reported: neither these tests nor the controller
look at one.
+func requireNoACLRules(f *Framework, aclID, port string) {
+ f.T.Helper()
+ rules, err := f.ACLRules(aclID)
+ if err != nil {
+ f.T.Fatalf("listing ACL rules for port %s: %v", port, err)
+ }
+ leftover := false
+ for _, r := range rules {
+ if r.Startport == port && r.Endport == port {
+ f.T.Errorf("leftover ACL rule %s for port %s (%s %s,
reason %q); delete it before rerunning",
+ r.Id, port, r.Traffictype, r.Protocol, r.Reason)
+ leftover = true
+ }
+ }
+ if leftover {
+ f.T.FailNow()
+ }
+}
+
+// TestVPC_ACLRulePerService is a regression test for issue #107: two Services
+// on one tier exposing the same port each get an ACL rule of their own, so
+// deleting one Service leaves the port open for the other. Earlier releases
+// shared one rule between them and deleted it with the first Service, which
+// this test reports as the port no longer being open.
+func TestVPC_ACLRulePerService(t *testing.T) {
+ f, aclID, _ := vpcFramework(t)
+
+ const port = "80"
+ requireNoACLRules(f, aclID, port)
+
+ first := f.CreateLBService(nil)
+ second := f.CreateLBService(func(s *corev1.Service) { s.Name =
"e2e-second" })
+ f.WaitForIngressIP(first)
+ f.WaitForIngressIP(second)
+ firstReason := aclRuleMarker(defaultLoadBalancerName(first))
+ secondReason := aclRuleMarker(defaultLoadBalancerName(second))
+
+ f.Eventually(lbSyncTimeout, lbSyncInterval, "port "+port+" to be open
on the tier",
+ func() (bool, error) {
+ rules, err := aclRulesOnPort(f, aclID, port)
+ return opensPortToAll(rules), err
+ })
+
+ f.DeleteServiceAndWait(first)
+
+ rules, err := aclRulesOnPort(f, aclID, port)
+ if err != nil {
+ t.Fatalf("listing ACL rules: %v", err)
+ }
+ if !opensPortToAll(rules) {
+ t.Fatalf("port %s is no longer open on the tier after deleting
one of the two Services using it", port)
+ }
+ if !hasACLRuleWithReason(rules, secondReason) {
+ t.Errorf("the remaining Service has no ACL rule of its own for
port %s", port)
+ }
+ f.Eventually(lbSyncTimeout, lbSyncInterval, "the deleted Service's ACL
rule to be removed",
+ func() (bool, error) {
+ rules, err := aclRulesOnPort(f, aclID, port)
+ return !hasACLRuleWithReason(rules, firstReason), err
+ })
+}
+
+// opensPortToAll reports whether one of the rules allows the port from
anywhere and is not being
+// deleted.
+func opensPortToAll(rules []*cloudstack.NetworkACL) bool {
+ for _, r := range rules {
+ if strings.EqualFold(r.Action, "Allow") && r.State !=
"Deleting" && hasCIDR(r.Cidrlist, "0.0.0.0/0") {
+ return true
+ }
+ }
+ return false
+}
+
+// hasCIDR reports whether a rule's CIDR list holds exactly the given entry.
CloudStack has
+// reported these lists both comma and space separated.
+func hasCIDR(cidrList, cidr string) bool {
+ entries := strings.FieldsFunc(cidrList, func(r rune) bool { return r ==
',' || r == ' ' })
+ return slices.Contains(entries, cidr)
+}
+
+// TestVPC_LegacyACLRuleAdopted covers the upgrade path of issue #107: a rule
+// shaped like the ones earlier releases created is adopted by the Service that
+// needs its port, not duplicated, and is removed with that Service.
+func TestVPC_LegacyACLRuleAdopted(t *testing.T) {
+ f, aclID, _ := vpcFramework(t)
+
+ const servicePort int32 = 8080
+ port := strconv.Itoa(int(servicePort))
+ requireNoACLRules(f, aclID, port)
+
+ legacyID := f.CreateACLRule(aclID, int(servicePort), "0.0.0.0/0")
+ svc := f.CreateLBService(func(s *corev1.Service) {
+ s.Spec.Ports = []corev1.ServicePort{
+ {Name: "http", Port: servicePort, Protocol:
corev1.ProtocolTCP},
+ }
+ })
+ reason := aclRuleMarker(defaultLoadBalancerName(svc))
+ f.WaitForIngressIP(svc)
+
+ f.Eventually(lbSyncTimeout, lbSyncInterval, "the existing ACL rule to
be adopted",
+ func() (bool, error) {
+ r, err := f.ACLRule(aclID, legacyID)
+ return r != nil && r.Reason == reason, err
+ })
+ if n, err := countACLRules(f, aclID, port); err != nil || n != 1 {
+ t.Errorf("ACL rules for port %s = %d (error %v), want exactly
1; the Service added a rule instead of adopting", port, n, err)
+ }
+
+ f.DeleteServiceAndWait(svc)
+ f.Eventually(lbSyncTimeout, lbSyncInterval, "the adopted ACL rule to be
removed with its Service",
+ func() (bool, error) {
+ r, err := f.ACLRule(aclID, legacyID)
+ return r == nil, err
+ })
+}
+
+// TestVPC_OperatorACLRuleKept checks that the controller leaves an ACL rule
+// someone else made for its port alone (issue #107): it adds no rule of its
+// own and keeps the other rule when the Service is deleted.
+func TestVPC_OperatorACLRuleKept(t *testing.T) {
+ f, aclID, _ := vpcFramework(t)
+
+ const servicePort int32 = 8085
+ const operatorCIDR = "10.0.0.0/8"
+ port := strconv.Itoa(int(servicePort))
+ requireNoACLRules(f, aclID, port)
+
+ operatorID := f.CreateACLRule(aclID, int(servicePort), operatorCIDR)
+ svc := f.CreateLBService(func(s *corev1.Service) {
+ s.Spec.Ports = []corev1.ServicePort{
+ {Name: "http", Port: servicePort, Protocol:
corev1.ProtocolTCP},
+ }
+ })
+ f.WaitForIngressIP(svc)
+
+ rules, err := aclRulesOnPort(f, aclID, port)
+ if err != nil {
+ t.Fatalf("listing ACL rules: %v", err)
+ }
+ if len(rules) != 1 || rules[0].Id != operatorID {
+ t.Errorf("ACL rules for port %s = %d, want only the hand-made
rule %s", port, len(rules), operatorID)
+ }
+
+ f.DeleteServiceAndWait(svc)
+
+ r, err := f.ACLRule(aclID, operatorID)
+ if err != nil {
+ t.Fatalf("looking up the hand-made ACL rule: %v", err)
+ }
+ if r == nil {
+ t.Fatalf("the hand-made ACL rule for port %s was deleted with
the Service", port)
+ }
+ if r.Cidrlist != operatorCIDR || r.Reason != "" {
+ t.Errorf("the hand-made ACL rule changed: cidrlist %q, reason
%q", r.Cidrlist, r.Reason)
+ }
+}
