Re: [PR] Fix protocol changes on existing load balancer rules [cloudstack-kubernetes-provider]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #104:
URL: https://github.com/apache/cloudstack-kubernetes-provider/pull/104#discussion_r4034181074


##########
cloudstack_loadbalancer.go:
##########
@@ -681,11 +634,261 @@ func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error) {
 	return cidrList, nil
 }
 
-// checkLoadBalancerRule checks if the rule already exists and if it does, if it can be updated. If
-// it does exist but cannot be updated, it will delete the existing rule so it can be created again.
-func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) {
-	lbRule, ok := lb.rules[lbRuleName]
-	if !ok {
+// splitCIDRList splits the CIDR list of an existing CloudStack rule into its entries.
+// CloudStack has reported these both comma and space separated, and a CIDR can contain
+// neither character, so treat both as separators.
+func splitCIDRList(cidrList string) []string {
+	return strings.FieldsFunc(cidrList, func(r rune) bool {
+		return r == ',' || r == ' '
+	})
+}
+
+// resolveLoadBalancerRules maps every service port to the load balancer rule that should
+// represent it, claiming each match as it goes so that what remains in lb.rules is exactly
+// the obsolete set and no rule can be claimed twice.
+func (lb *loadBalancer) resolveLoadBalancerRules(service *corev1.Service, version semver.Version) ([]desiredLBRule, error) {
+	desired := make([]desiredLBRule, 0, len(service.Spec.Ports))
+
+	for _, port := range service.Spec.Ports {
+		// Construct the protocol name first, we need it a few times
+		protocol := ProtocolFromServicePort(port, service)
+		if protocol == LoadBalancerProtocolInvalid {
+			return nil, fmt.Errorf("unsupported load balancer protocol: %v", port.Protocol)
+		}
+
+		// All ports have their own load balancer rule, so add the port to lbName to keep the names unique.
+		lbRuleName := fmt.Sprintf("%s-%s-%d", lb.name, protocol, port.Port)
+
+		lbRule, needsUpdate, err := lb.checkLoadBalancerRule(lb.findLoadBalancerRule(lbRuleName, port, protocol), lbRuleName, port, protocol, service, version)
+		if err != nil {
+			return nil, err
+		}
+
+		if lbRule != nil {
+			// Claim by the rule's actual name: after a protocol change it still carries the old one.
+			delete(lb.rules, lbRule.Name)
+		}
+
+		desired = append(desired, desiredLBRule{
+			name:     lbRuleName,
+			port:     port,
+			protocol: protocol,
+			existing: lbRule,
+			update:   needsUpdate,
+		})
+	}
+
+	return desired, nil
+}
+
+// findLoadBalancerRule locates the existing CloudStack rule for a desired service port. It
+// prefers an exact name match, then falls back to matching on the tuple. That fallback is what
+// lets a protocol change (tcp <-> tcp-proxy) update the existing rule instead of creating a
+// conflicting one.
+//
+// Only rules on the IP being reconciled towards are eligible; a rule on any other IP is left
+// for the prune pass, which also cleans up the firewall rules it leaves behind.
+func (lb *loadBalancer) findLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol) *cloudstack.LoadBalancerRule {
+	if lbRule, ok := lb.rules[lbRuleName]; ok && lbRule.Publicipid == lb.ipAddrID {

Review Comment:
   This filter assumes `lb.ipAddrID` is the service's current public IP, but `getLoadBalancer` assigns that field from every returned rule, so a response containing both current and stale-IP rules leaves the result dependent on API order. In that case a valid rule on the published IP can be treated as obsolete while reconciliation updates or creates rules on the stale IP. Select the preferred/requested/status IP before resolving rules, or pass the target IP explicitly.



##########
cloudstack_loadbalancer_test.go:
##########
@@ -2854,6 +3128,114 @@ func TestUpdateNetworkACL(t *testing.T) {
 		}
 	})
 
+	t.Run("tcp-proxy creates ACL rule with tcp protocol", func(t *testing.T) {
+		ctrl := gomock.NewController(t)
+		t.Cleanup(ctrl.Finish)
+
+		mockNetwork := cloudstack.NewMockNetworkServiceIface(ctrl)
+		mockNetworkACL := cloudstack.NewMockNetworkACLServiceIface(ctrl)
+		networkResp := &cloudstack.Network{
+			Id:      "net-123",
+			Aclid:   "acl-456",
+			Service: []cloudstack.NetworkServiceInternal{},
+		}
+
+		aclListResp := &cloudstack.NetworkACLList{
+			Id:   "acl-456",
+			Name: "custom-acl",
+		}
+
+		listParams := &cloudstack.ListNetworkACLsParams{}
+		listResp := &cloudstack.ListNetworkACLsResponse{
+			Count:       0,
+			NetworkACLs: []*cloudstack.NetworkACL{},
+		}
+
+		createParams := &cloudstack.CreateNetworkACLParams{}
+		createResp := &cloudstack.CreateNetworkACLResponse{
+			Id: "acl-rule-123",
+		}
+
+		gomock.InOrder(
+			mockNetwork.EXPECT().GetNetworkByID("net-123").Return(networkResp, 1, nil),
+			mockNetworkACL.EXPECT().GetNetworkACLListByID("acl-456").Return(aclListResp, 1, nil),

Review Comment:
   These expectations omit the variadic project option, but `updateNetworkACL` always calls both methods with `cloudstack.WithProject(lb.projectID)` (even when the project ID is empty). Gomock will see two arguments and reject these one-argument expectations, so this new regression test fails before exercising the ACL fix. Match the option as the existing test above does.
   
   This issue also appears on line 3217 of the same file.



##########
cloudstack_loadbalancer.go:
##########
@@ -681,11 +634,261 @@ func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error) {
 	return cidrList, nil
 }
 
-// checkLoadBalancerRule checks if the rule already exists and if it does, if it can be updated. If
-// it does exist but cannot be updated, it will delete the existing rule so it can be created again.
-func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) {
-	lbRule, ok := lb.rules[lbRuleName]
-	if !ok {
+// splitCIDRList splits the CIDR list of an existing CloudStack rule into its entries.
+// CloudStack has reported these both comma and space separated, and a CIDR can contain
+// neither character, so treat both as separators.
+func splitCIDRList(cidrList string) []string {
+	return strings.FieldsFunc(cidrList, func(r rune) bool {
+		return r == ',' || r == ' '
+	})
+}
+
+// resolveLoadBalancerRules maps every service port to the load balancer rule that should
+// represent it, claiming each match as it goes so that what remains in lb.rules is exactly
+// the obsolete set and no rule can be claimed twice.
+func (lb *loadBalancer) resolveLoadBalancerRules(service *corev1.Service, version semver.Version) ([]desiredLBRule, error) {
+	desired := make([]desiredLBRule, 0, len(service.Spec.Ports))
+
+	for _, port := range service.Spec.Ports {
+		// Construct the protocol name first, we need it a few times
+		protocol := ProtocolFromServicePort(port, service)
+		if protocol == LoadBalancerProtocolInvalid {
+			return nil, fmt.Errorf("unsupported load balancer protocol: %v", port.Protocol)
+		}
+
+		// All ports have their own load balancer rule, so add the port to lbName to keep the names unique.
+		lbRuleName := fmt.Sprintf("%s-%s-%d", lb.name, protocol, port.Port)
+
+		lbRule, needsUpdate, err := lb.checkLoadBalancerRule(lb.findLoadBalancerRule(lbRuleName, port, protocol), lbRuleName, port, protocol, service, version)
+		if err != nil {
+			return nil, err
+		}
+
+		if lbRule != nil {
+			// Claim by the rule's actual name: after a protocol change it still carries the old one.
+			delete(lb.rules, lbRule.Name)
+		}
+
+		desired = append(desired, desiredLBRule{
+			name:     lbRuleName,
+			port:     port,
+			protocol: protocol,
+			existing: lbRule,
+			update:   needsUpdate,
+		})
+	}
+
+	return desired, nil
+}
+
+// findLoadBalancerRule locates the existing CloudStack rule for a desired service port. It
+// prefers an exact name match, then falls back to matching on the tuple. That fallback is what
+// lets a protocol change (tcp <-> tcp-proxy) update the existing rule instead of creating a
+// conflicting one.
+//
+// Only rules on the IP being reconciled towards are eligible; a rule on any other IP is left
+// for the prune pass, which also cleans up the firewall rules it leaves behind.
+func (lb *loadBalancer) findLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol) *cloudstack.LoadBalancerRule {
+	if lbRule, ok := lb.rules[lbRuleName]; ok && lbRule.Publicipid == lb.ipAddrID {
+		return lbRule
+	}
+
+	publicPort := strconv.Itoa(int(port.Port))
+	var names []string
+	for name, lbRule := range lb.rules {
+		if lbRule.Publicipid == lb.ipAddrID &&
+			ProtocolFromLoadBalancer(lbRule.Protocol).IPProtocol() == protocol.IPProtocol() &&
+			lbRule.Publicport == publicPort {
+			names = append(names, name)
+		}
+	}
+	if len(names) == 0 {
+		return nil
+	}
+
+	// Map iteration order is randomized; sort so the pick is deterministic.
+	sort.Strings(names)
+	if len(names) > 1 {
+		klog.Warningf("Multiple load balancer rules match %s port %s: %v; using %v", protocol.IPProtocol(), publicPort, names, names[0])
+	}
+	return lb.rules[names[0]]
+}
+
+// portProtocol is the tuple CloudStack refuses to place two load balancer rules on, and that
+// firewall and network ACL rules are keyed on. IPProtocol maps both tcp and tcp-proxy to
+// "tcp", so a tcp and a tcp-proxy rule on one port share a tuple, and one firewall/ACL rule.
+type portProtocol struct {
+	ipProtocol string
+	publicPort int32
+}
+
+// obsoleteRule is a rule no desired service port claimed, with its tuple already parsed.
+type obsoleteRule struct {
+	rule     *cloudstack.LoadBalancerRule
+	protocol LoadBalancerProtocol
+	tuple    portProtocol
+}
+
+// partitionObsoleteRules splits the rules left in lb.rules — those no desired port claimed —
+// into the ones holding a tuple that a rule still to be created needs, and the rest.
+func (lb *loadBalancer) partitionObsoleteRules(desired []desiredLBRule) (blocking, rest []obsoleteRule) {
+	// CloudStack refuses two load balancer rules with overlapping public port ranges on one
+	// IP whatever their protocols, so the port alone decides what blocks a create. Note this
+	// is deliberately coarser than the firewall/ACL claim, which is per protocol because
+	// firewall rules are.
+	neededPorts := make(map[int32]bool)
+	for _, d := range desired {
+		if d.existing == nil {
+			neededPorts[d.port.Port] = true
+		}
+	}
+
+	// Iterate in name order so the prune sequence is reproducible.
+	names := make([]string, 0, len(lb.rules))
+	for name := range lb.rules {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+
+	for _, name := range names {
+		lbRule := lb.rules[name]
+
+		protocol := ProtocolFromLoadBalancer(lbRule.Protocol)
+		if protocol == LoadBalancerProtocolInvalid {
+			klog.Errorf("Skipping obsolete load balancer rule %v with unknown protocol %v", lbRule.Name, lbRule.Protocol)
+			continue
+		}
+		port, err := strconv.ParseInt(lbRule.Publicport, 10, 32)
+		if err != nil {
+			klog.Errorf("Skipping obsolete load balancer rule %v with invalid public port %v: %v", lbRule.Name, lbRule.Publicport, err)
+			continue
+		}
+
+		obsolete := obsoleteRule{
+			rule:     lbRule,
+			protocol: protocol,
+			tuple:    portProtocol{protocol.IPProtocol(), int32(port)},
+		}
+
+		// Conflicts are per public IP, so only a rule on the IP being reconciled towards can
+		// block a create.
+		if lbRule.Publicipid == lb.ipAddrID && neededPorts[obsolete.tuple.publicPort] {
+			blocking = append(blocking, obsolete)
+		} else {
+			rest = append(rest, obsolete)
+		}
+	}
+
+	return blocking, rest
+}
+
+// pruneRules deletes the given obsolete rules along with their firewall or network ACL rules.
+// A firewall/ACL rule is kept when a desired port still claims the same tuple, since the two
+// load balancer rules share it and pruning would strip the survivor of its opening.
+//
+// A rule that fails to delete is reported but does not stop the others being pruned.
+func (lb *loadBalancer) pruneRules(obsolete []obsoleteRule, desired []desiredLBRule, network *cloudstack.Network) error {
+	// Tuples the service still needs, and whose firewall/ACL rules therefore have to survive.
+	claimed := make(map[portProtocol]bool, len(desired))
+	for _, d := range desired {
+		claimed[portProtocol{d.protocol.IPProtocol(), d.port.Port}] = true
+	}
+
+	var firstErr error
+	recordErr := func(err error) {
+		klog.Errorf("Error pruning obsolete load balancer rule: %v", err)
+		if firstErr == nil {
+			firstErr = err
+		}
+	}
+
+	for _, o := range obsolete {
+		lbRule, protocol, port := o.rule, o.protocol, o.tuple.publicPort
+
+		if isFirewallSupported(network.Service) {
+			// Firewall rules belong to a single public IP, so a claim only covers a rule on
+			// the IP the service is being reconciled towards.
+			if claimed[o.tuple] && lbRule.Publicipid == lb.ipAddrID {
+				klog.V(4).Infof("Keeping firewall rules of obsolete load balancer rule %v (%v:%v:%v): still claimed by a service port", lbRule.Name, protocol, lbRule.Publicip, port)
+			} else {
+				klog.V(4).Infof("Deleting firewall rules associated with load balancer rule: %v (%v:%v:%v)", lbRule.Name, protocol, lbRule.Publicip, port)
+				if _, err := lb.deleteFirewallRule(lbRule.Publicipid, int(port), protocol); err != nil {
+					recordErr(err)
+					continue
+				}
+			}
+		} else if isNetworkACLSupported(network.Service) {
+			// ACL rules belong to the network rather than an IP, so the claim always applies.
+			if claimed[o.tuple] {
+				klog.V(4).Infof("Keeping Network ACL rules of obsolete load balancer rule %v (%v:%v): still claimed by a service port", lbRule.Name, protocol, port)
+			} else {
+				klog.V(4).Infof("Deleting Network ACL rules associated with load balancer rule: %v (%v:%v)", lbRule.Name, protocol, port)

Review Comment:
   Network ACLs are network-scoped, but `claimed` contains only protocol and port and this branch keeps an obsolete rule's ACL whenever the desired rule has the same tuple. If a service has stale rules from an old VPC/network, the desired rule on the new network incorrectly protects the old ACL from deletion; the deletion path also uses `lb.networkID` rather than the obsolete rule's network. Include network identity in the claim and delete against the obsolete rule's network.



##########
cloudstack_loadbalancer.go:
##########
@@ -681,11 +634,261 @@ func (lb *loadBalancer) getCIDRList(service *corev1.Service) ([]string, error) {
 	return cidrList, nil
 }
 
-// checkLoadBalancerRule checks if the rule already exists and if it does, if it can be updated. If
-// it does exist but cannot be updated, it will delete the existing rule so it can be created again.
-func (lb *loadBalancer) checkLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol, service *corev1.Service, version semver.Version) (*cloudstack.LoadBalancerRule, bool, error) {
-	lbRule, ok := lb.rules[lbRuleName]
-	if !ok {
+// splitCIDRList splits the CIDR list of an existing CloudStack rule into its entries.
+// CloudStack has reported these both comma and space separated, and a CIDR can contain
+// neither character, so treat both as separators.
+func splitCIDRList(cidrList string) []string {
+	return strings.FieldsFunc(cidrList, func(r rune) bool {
+		return r == ',' || r == ' '
+	})
+}
+
+// resolveLoadBalancerRules maps every service port to the load balancer rule that should
+// represent it, claiming each match as it goes so that what remains in lb.rules is exactly
+// the obsolete set and no rule can be claimed twice.
+func (lb *loadBalancer) resolveLoadBalancerRules(service *corev1.Service, version semver.Version) ([]desiredLBRule, error) {
+	desired := make([]desiredLBRule, 0, len(service.Spec.Ports))
+
+	for _, port := range service.Spec.Ports {
+		// Construct the protocol name first, we need it a few times
+		protocol := ProtocolFromServicePort(port, service)
+		if protocol == LoadBalancerProtocolInvalid {
+			return nil, fmt.Errorf("unsupported load balancer protocol: %v", port.Protocol)
+		}
+
+		// All ports have their own load balancer rule, so add the port to lbName to keep the names unique.
+		lbRuleName := fmt.Sprintf("%s-%s-%d", lb.name, protocol, port.Port)
+
+		lbRule, needsUpdate, err := lb.checkLoadBalancerRule(lb.findLoadBalancerRule(lbRuleName, port, protocol), lbRuleName, port, protocol, service, version)
+		if err != nil {
+			return nil, err
+		}
+
+		if lbRule != nil {
+			// Claim by the rule's actual name: after a protocol change it still carries the old one.
+			delete(lb.rules, lbRule.Name)
+		}
+
+		desired = append(desired, desiredLBRule{
+			name:     lbRuleName,
+			port:     port,
+			protocol: protocol,
+			existing: lbRule,
+			update:   needsUpdate,
+		})
+	}
+
+	return desired, nil
+}
+
+// findLoadBalancerRule locates the existing CloudStack rule for a desired service port. It
+// prefers an exact name match, then falls back to matching on the tuple. That fallback is what
+// lets a protocol change (tcp <-> tcp-proxy) update the existing rule instead of creating a
+// conflicting one.
+//
+// Only rules on the IP being reconciled towards are eligible; a rule on any other IP is left
+// for the prune pass, which also cleans up the firewall rules it leaves behind.
+func (lb *loadBalancer) findLoadBalancerRule(lbRuleName string, port corev1.ServicePort, protocol LoadBalancerProtocol) *cloudstack.LoadBalancerRule {
+	if lbRule, ok := lb.rules[lbRuleName]; ok && lbRule.Publicipid == lb.ipAddrID {
+		return lbRule
+	}
+
+	publicPort := strconv.Itoa(int(port.Port))
+	var names []string
+	for name, lbRule := range lb.rules {
+		if lbRule.Publicipid == lb.ipAddrID &&
+			ProtocolFromLoadBalancer(lbRule.Protocol).IPProtocol() == protocol.IPProtocol() &&
+			lbRule.Publicport == publicPort {
+			names = append(names, name)
+		}
+	}
+	if len(names) == 0 {
+		return nil
+	}
+
+	// Map iteration order is randomized; sort so the pick is deterministic.
+	sort.Strings(names)
+	if len(names) > 1 {
+		klog.Warningf("Multiple load balancer rules match %s port %s: %v; using %v", protocol.IPProtocol(), publicPort, names, names[0])
+	}
+	return lb.rules[names[0]]
+}
+
+// portProtocol is the tuple CloudStack refuses to place two load balancer rules on, and that
+// firewall and network ACL rules are keyed on. IPProtocol maps both tcp and tcp-proxy to
+// "tcp", so a tcp and a tcp-proxy rule on one port share a tuple, and one firewall/ACL rule.
+type portProtocol struct {
+	ipProtocol string
+	publicPort int32
+}
+
+// obsoleteRule is a rule no desired service port claimed, with its tuple already parsed.
+type obsoleteRule struct {
+	rule     *cloudstack.LoadBalancerRule
+	protocol LoadBalancerProtocol
+	tuple    portProtocol
+}
+
+// partitionObsoleteRules splits the rules left in lb.rules — those no desired port claimed —
+// into the ones holding a tuple that a rule still to be created needs, and the rest.
+func (lb *loadBalancer) partitionObsoleteRules(desired []desiredLBRule) (blocking, rest []obsoleteRule) {
+	// CloudStack refuses two load balancer rules with overlapping public port ranges on one
+	// IP whatever their protocols, so the port alone decides what blocks a create. Note this
+	// is deliberately coarser than the firewall/ACL claim, which is per protocol because
+	// firewall rules are.
+	neededPorts := make(map[int32]bool)
+	for _, d := range desired {
+		if d.existing == nil {
+			neededPorts[d.port.Port] = true
+		}
+	}
+
+	// Iterate in name order so the prune sequence is reproducible.
+	names := make([]string, 0, len(lb.rules))
+	for name := range lb.rules {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+
+	for _, name := range names {
+		lbRule := lb.rules[name]
+
+		protocol := ProtocolFromLoadBalancer(lbRule.Protocol)
+		if protocol == LoadBalancerProtocolInvalid {
+			klog.Errorf("Skipping obsolete load balancer rule %v with unknown protocol %v", lbRule.Name, lbRule.Protocol)
+			continue

Review Comment:
   This skips an obsolete rule before partitioning it. If CloudStack returns an unrecognized protocol for a rule on the target IP and a needed port such as 80, that rule still occupies the port, but no `blocking` entry is produced; the apply phase then attempts the desired rule and can hit the same port-conflict error this change is intended to avoid. Invalid protocols need a separate cleanup/blocking path rather than being silently left in place.



##########
cloudstack.go:
##########
@@ -131,7 +131,7 @@ func (cs *CSCloud) getManagementServerVersion() (semver.Version, error) {
 	parts := strings.Split(version, ".")
 	v, err := semver.ParseTolerant(strings.Join(parts[:min(len(parts), 3)], "."))
 	if err != nil {
-		klog.Errorf("failed to parse management server version: %v", err)
+		klog.Errorf("failed to parse management server version %q: %v", version, err)

Review Comment:
   The advertised short-version startup fix is not present in this PR: the unchanged production line 132 already bounds the slice with `min(len(parts), 3)`, and the existing `handles short version strings without panicking` test already covers `4.22` and `4`. This hunk only changes logging, so either remove the duplicate tests/claim or include the actual behavior change if it is still required.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@cloudstack.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org