This is an automated email from the ASF dual-hosted git repository. vishesh92 pushed a commit to branch fix-acl-ownership in repository https://gitbox.apache.org/repos/asf/cloudstack-kubernetes-provider.git
commit 7bf1f07ef565a23bfc615f4dc8f0ca5e808dd46e Author: vishesh92 <[email protected]> AuthorDate: Fri Oct 9 14:48:23 2026 +0530 Give each Service its own Network ACL rule On a VPC tier the controller opened a port with an ACL rule matched only on protocol and port. Every Service using that port shared the rule, and it was deleted when any of them was deleted or moved off the port, so the other Services lost traffic on it. Each rule the controller creates now carries the reason "Managed by the CloudStack Kubernetes Provider for <lb name>", and only rules with the Service's own reason are deleted. A rule created by an earlier release is taken over on the first sync; rules someone else made are left alone. A Service whose ACL rule cannot be deleted keeps its load balancer rule, so Kubernetes retries the cleanup. Fixes #107 --- README.md | 95 ++++- cloudstack_loadbalancer.go | 273 +++++++++++---- cloudstack_loadbalancer_test.go | 758 ++++++++++++++++++++++++++++++++++++++-- docs/development.md | 7 +- test/e2e/framework.go | 54 +++ test/e2e/vpc_test.go | 194 +++++++++- 6 files changed, 1268 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..bac62704 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(lb.ipAddrID) + 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,176 @@ 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: %s", 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 { + switch { + case aclRule.State == "Deleting": + return aclRuleIgnored + case aclRule.Reason == reason: + return aclRuleOwn + case strings.HasPrefix(aclRule.Reason, networkACLReasonPrefix): + return aclRuleIgnored + case 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: %s", publicPort, err) } - return true, err + + return nil } // deleteFirewallRule deletes the firewall rule associated with the ip:port:protocol combo @@ -2057,51 +2196,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) - } - - networkAcls, err := listAll(p, func() (int, []*cloudstack.NetworkACL, error) { - r, err := lb.NetworkACL.ListNetworkACLs(p) - if err != nil { - return 0, nil, err - } - - return r.Count, r.NetworkACLs, nil - }) + aclRules, err := lb.listNetworkACLRules(networkID, protocol.IPProtocol(), publicPort) 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) + owned := 0 + var firstErr error + for _, aclRule := range aclRules { + if aclRule.Reason != lb.networkACLReason() { + continue } - } + owned++ - // 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 + 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 + } + } } - 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..eed71df5 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,258 @@ 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) + delete80 := &cloudstack.DeleteNetworkACLParams{} + delete8080 := &cloudstack.DeleteNetworkACLParams{} + mockNetworkACL.EXPECT().NewDeleteNetworkACLParams("acl-own-80").Return(delete80) + mockNetworkACL.EXPECT().DeleteNetworkACL(delete80).Return(nil, fmt.Errorf("router unreachable")) + mockNetworkACL.EXPECT().NewDeleteNetworkACLParams("acl-own-8080").Return(delete8080) + mockNetworkACL.EXPECT().DeleteNetworkACL(delete8080).Return(&cloudstack.DeleteNetworkACLResponse{}, nil) + 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) + } + }) +} + +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 +6287,7 @@ func TestPruneRulesScopesNetworkACLsToTheirNetwork(t *testing.T) { Network: m.network, Firewall: m.firewall, }, + name: "atestuid", networkID: "net-new", ipAddrID: "ip-current", }, m @@ -5601,9 +6298,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..36e4ec8e 100644 --- a/docs/development.md +++ b/docs/development.md @@ -381,6 +381,10 @@ 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 ACL rule on their port, 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 +424,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 the port that the CCM did not create, which it leaves the port to. | +| 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..0cf4d0c9 100644 --- a/test/e2e/vpc_test.go +++ b/test/e2e/vpc_test.go @@ -28,6 +28,7 @@ import ( "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 +56,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 +86,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 +165,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 +272,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 +307,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 +328,177 @@ 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 rules for the port are left over from an earlier run, +// since the tests below count every rule on their port. +func requireNoACLRules(f *Framework, aclID, port string) { + f.T.Helper() + rules, err := aclRulesOnPort(f, aclID, port) + if err != nil { + f.T.Fatalf("listing ACL rules for port %s: %v", port, err) + } + for _, r := range rules { + f.T.Errorf("leftover ACL rule %s for port %s (reason %q); delete it before rerunning", r.Id, port, r.Reason) + } + if len(rules) > 0 { + 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" && + strings.Contains(r.Cidrlist, "0.0.0.0/0") { + return true + } + } + return false +} + +// 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) + } +}
