fix: Delete related fw rule when a port is removed from additionalPor…

…ts + add test

pkg/cloud/isolated_network.go

@@ -268,6 +268,7 @@ func (c *client) ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNe
 		}
 	}
 
+	// Delete any existing rules with a port that is no longer part of ports.
 	for port, ruleID := range portsAndIDs {
 		intport, err := strconv.Atoi(port)
 		if err != nil {
@@ -277,7 +278,7 @@ func (c *client) ReconcileLoadBalancerRules(isoNet *infrav1.CloudStackIsolatedNe
 		if !slices.Contains(ports, intport) {
 			success, err := c.DeleteLoadBalancerRule(ruleID)
 			if err != nil || !success {
-				return errors.Wrap(err, "deleting firewall rule")
+				return errors.Wrap(err, "deleting load balancer rule")
 			}
 		}
 	}
@@ -351,6 +352,14 @@ func (c *client) ReconcileFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwor
 		return errors.Wrap(err, "retrieving load balancer rules")
 	}
 
+	// Create a map for easy lookup of existing rules
+	portsAndIDs := make(map[int]string)
+	for _, rule := range fwr {
+		if rule.Startport == rule.Endport {
+			portsAndIDs[rule.Startport] = rule.Id
+		}
+	}
+
 	ports := []int{int(csCluster.Spec.ControlPlaneEndpoint.Port)}
 	if len(csCluster.Spec.APIServerLoadBalancer.AdditionalPorts) > 0 {
 		ports = append(ports, csCluster.Spec.APIServerLoadBalancer.AdditionalPorts...)
@@ -387,6 +396,17 @@ func (c *client) ReconcileFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwor
 			}
 		}
 	}
+
+	// Delete any existing rules with a port that is no longer part of ports.
+	for port, ruleID := range portsAndIDs {
+		if !slices.Contains(ports, port) {
+			success, err := c.DeleteFirewallRule(ruleID)
+			if err != nil || !success {
+				return errors.Wrap(err, "deleting firewall rule")
+			}
+		}
+	}
+
 	// Update the list of allowed CIDRs in the status
 	isoNet.Status.APIServerLoadBalancer.AllowedCIDRs = allowedCIDRS
 

pkg/cloud/isolated_network_test.go

@@ -508,6 +508,37 @@ var _ = Describe("Network", func() {
 
 			Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed())
 		})
+
+		It("calls delete firewall rule when a port is removed from additionalPorts", func() {
+			// We pretend that port 6565 was removed from additionalPorts
+			fs.EXPECT().NewListFirewallRulesParams().Return(&csapi.ListFirewallRulesParams{})
+			fs.EXPECT().ListFirewallRules(gomock.Any()).Return(
+				&csapi.ListFirewallRulesResponse{FirewallRules: []*csapi.FirewallRule{
+					{
+						Cidrlist:  "0.0.0.0/0",
+						Startport: int(dummies.EndPointPort),
+						Endport:   int(dummies.EndPointPort),
+						Id:        dummies.FWRuleID,
+					},
+					{
+						Cidrlist:  "0.0.0.0/0",
+						Startport: 6565,
+						Endport:   6565,
+						Id:        "FakeFWRuleID2",
+					},
+				}}, nil)
+
+			fs.EXPECT().NewDeleteFirewallRuleParams("FakeFWRuleID2").DoAndReturn(func(ruleid string) *csapi.DeleteFirewallRuleParams {
+				p := &csapi.DeleteFirewallRuleParams{}
+				p.SetId(ruleid)
+				return p
+			})
+			fs.EXPECT().DeleteFirewallRule(gomock.Any()).Return(&csapi.DeleteFirewallRuleResponse{Success: true}, nil).Times(1)
+			fs.EXPECT().NewCreateFirewallRuleParams(gomock.Any(), gomock.Any()).Times(0)
+			fs.EXPECT().CreateFirewallRule(gomock.Any()).Times(0)
+
+			Ω(client.ReconcileFirewallRules(dummies.CSISONet1, dummies.CSCluster)).Should(Succeed())
+		})
 	})
 
 	Context("The specific firewall rule does not exist", func() {