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 `, where + `` 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 `` 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..37c14f90 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,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: %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 { + 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: %w", 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) + 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..5a3802b3 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,330 @@ 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) + } + }) +} + +// 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 +6359,7 @@ func TestPruneRulesScopesNetworkACLsToTheirNetwork(t *testing.T) { Network: m.network, Firewall: m.firewall, }, + name: "atestuid", networkID: "net-new", ipAddrID: "ip-current", }, m @@ -5601,9 +6370,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//`, 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 ``; 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) + } +}