fix: Make sure lb rules are deleted when a port is removed from addit…

…ionalPorts

pkg/cloud/isolated_network.go

@@ -236,26 +236,25 @@ func (c *client) ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNe
 		return errors.Wrap(err, "retrieving load balancer rules")
 	}
 
+	// Create a map for easy lookup of existing rules
+	portsAndIDs := make(map[string]string)
+	for _, rule := range lbr {
+		portsAndIDs[rule.Publicport] = rule.Id
+	}
+
 	ports := []int{int(csCluster.Spec.ControlPlaneEndpoint.Port)}
 	if len(csCluster.Spec.APIServerLoadBalancer.AdditionalPorts) > 0 {
 		ports = append(ports, csCluster.Spec.APIServerLoadBalancer.AdditionalPorts...)
 	}
 
 	lbRuleIDs := make([]string, 0)
-	var found bool
 	for _, port := range ports {
-		var ruleID string
-		found = false
 		// Check if lb rule for port already exists
-		for _, rule := range lbr {
-			ruleID = rule.Id
-			if rule.Publicport == strconv.Itoa(port) {
-				found = true
-				lbRuleIDs = append(lbRuleIDs, ruleID)
-			}
-		}
-		// If not found, create the lb rule for port
-		if !found {
+		ruleID, found := portsAndIDs[strconv.Itoa(port)]
+		if found {
+			lbRuleIDs = append(lbRuleIDs, ruleID)
+		} else {
+			// If not found, create the lb rule for port
 			ruleID, err = c.CreateLoadBalancerRule(isoNet, port)
 			if err != nil {
 				return errors.Wrap(err, "creating load balancer rule")
@@ -269,6 +268,20 @@ func (c *client) ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNe
 		}
 	}
 
+	for port, ruleID := range portsAndIDs {
+		intport, err := strconv.Atoi(port)
+		if err != nil {
+			return errors.Wrap(err, "converting port to int")
+		}
+
+		if !slices.Contains(ports, intport) {
+			success, err := c.DeleteLoadBalancerRule(ruleID)
+			if err != nil || !success {
+				return errors.Wrap(err, "deleting firewall rule")
+			}
+		}
+	}
+
 	if len(lbRuleIDs) > 1 {
 		capcstrings.Canonicalize(lbRuleIDs)
 	}
@@ -304,6 +317,19 @@ func (c *client) CreateLoadBalancerRule(isoNet *infrav1.CloudStackIsolatedNetwor
 	return resp.Id, nil
 }
 
+// DeleteLoadBalancerRule deletes an existing load balancer rule.
+func (c *client) DeleteLoadBalancerRule(id string) (bool, error) {
+	p := c.cs.LoadBalancer.NewDeleteLoadBalancerRuleParams(id)
+	resp, err := c.cs.LoadBalancer.DeleteLoadBalancerRule(p)
+	if err != nil {
+		c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
+
+		return false, err
+	}
+
+	return resp.Success, nil
+}
+
 // GetFirewallRules fetches the current firewall rules for the isolated network.
 func (c *client) GetFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork) ([]*cloudstack.FirewallRule, error) {
 	p := c.cs.Firewall.NewListFirewallRulesParams()

pkg/cloud/isolated_network_test.go

@@ -383,14 +383,16 @@ var _ = Describe("Network", func() {
 			lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{})
 			lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).
 				Return(&csapi.ListLoadBalancerRulesResponse{
-					LoadBalancerRules: []*csapi.LoadBalancerRule{{Publicport: "7443", Id: dummies.LBRuleID}}}, nil)
+					LoadBalancerRules: []*csapi.LoadBalancerRule{}}, nil)
 			lbs.EXPECT().NewCreateLoadBalancerRuleParams(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).
 				Return(&csapi.CreateLoadBalancerRuleParams{})
 			lbs.EXPECT().CreateLoadBalancerRule(gomock.Any()).
-				Return(&csapi.CreateLoadBalancerRuleResponse{Id: "2ndLBRuleID"}, nil)
+				Return(&csapi.CreateLoadBalancerRuleResponse{Id: dummies.LBRuleID}, nil)
+			lbs.EXPECT().NewDeleteLoadBalancerRuleParams(gomock.Any()).Times(0)
+			lbs.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Times(0)
 
 			Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed())
-			loadBalancerRuleIDs := []string{"2ndLBRuleID"}
+			loadBalancerRuleIDs := []string{dummies.LBRuleID}
 			Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(loadBalancerRuleIDs))
 		})
 
@@ -406,13 +408,34 @@ var _ = Describe("Network", func() {
 				Return(&csapi.CreateLoadBalancerRuleParams{})
 			lbs.EXPECT().CreateLoadBalancerRule(gomock.Any()).
 				Return(&csapi.CreateLoadBalancerRuleResponse{Id: "2ndLBRuleID"}, nil)
+			lbs.EXPECT().NewDeleteLoadBalancerRuleParams(gomock.Any()).Times(0)
+			lbs.EXPECT().DeleteLoadBalancerRule(gomock.Any()).Times(0)
 
 			dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{dummies.LBRuleID}
 			Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed())
 			dummies.LoadBalancerRuleIDs = []string{"2ndLBRuleID", dummies.LBRuleID}
 			Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(dummies.LoadBalancerRuleIDs))
 		})
 
+		It("when API load balancer additional ports are defined, and a port is removed, deletes related rules", func() {
+			lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{})
+			lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).Return(
+				&csapi.ListLoadBalancerRulesResponse{LoadBalancerRules: []*csapi.LoadBalancerRule{
+					{Publicport: strconv.Itoa(int(dummies.EndPointPort)), Id: dummies.LBRuleID},
+					{Publicport: strconv.Itoa(456), Id: "2ndLBRuleID"},
+				}}, nil)
+
+			lbs.EXPECT().NewDeleteLoadBalancerRuleParams(gomock.Any()).
+				Return(&csapi.DeleteLoadBalancerRuleParams{}).Times(1)
+			lbs.EXPECT().DeleteLoadBalancerRule(gomock.Any()).
+				Return(&csapi.DeleteLoadBalancerRuleResponse{Success: true}, nil).Times(1)
+
+			dummies.CSISONet1.Status.LoadBalancerRuleIDs = []string{"2ndLBRuleID", dummies.LBRuleID}
+			Ω(client.ReconcileLoadBalancerRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed())
+			dummies.LoadBalancerRuleIDs = []string{dummies.LBRuleID}
+			Ω(dummies.CSISONet1.Status.LoadBalancerRuleIDs).Should(Equal(dummies.LoadBalancerRuleIDs))
+		})
+
 		It("Fails to resolve load balancer rule details", func() {
 			lbs.EXPECT().NewListLoadBalancerRulesParams().Return(&csapi.ListLoadBalancerRulesParams{})
 			lbs.EXPECT().ListLoadBalancerRules(gomock.Any()).