Fix: egress rules & tags check existing

api/v1beta3/cloudstackisolatednetwork_types.go

@@ -73,6 +73,9 @@ type CloudStackIsolatedNetworkStatus struct {
 	// Empty value means the network mode is NATTED, not ROUTED.
 	RoutingMode string `json:"routingMode,omitempty"`
 
+	// Firewall rules open status.
+	FirewallRulesOpened bool `json:"firewallRulesOpened,omitempty"`
+
 	// Ready indicates the readiness of this provider resource.
 	Ready bool `json:"ready"`
 }

config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackisolatednetworks.yaml

@@ -251,6 +251,9 @@ spec:
             description: CloudStackIsolatedNetworkStatus defines the observed state
               of CloudStackIsolatedNetwork
             properties:
+              firewallRulesOpened:
+                description: Firewall rules open status.
+                type: boolean
               loadBalancerRuleID:
                 description: The ID of the lb rule used to assign VMs to the lb.
                 type: string

pkg/cloud/isolated_network.go

@@ -154,54 +154,150 @@ func (c *client) CreateIsolatedNetwork(fd *infrav1.CloudStackFailureDomain, isoN
 
 // OpenFirewallRules opens a CloudStack egress firewall for an isolated network.
 func (c *client) OpenFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork) (retErr error) {
+	// Early return if VPC is present
 	if isoNet.Spec.VPC != nil && isoNet.Spec.VPC.ID != "" {
 		return nil
 	}
 
-	// If network's egress policy is true, then we don't need to open the firewall rules for all protocols
-	network, count, err := c.cs.Network.GetNetworkByID(isoNet.Spec.ID, cloudstack.WithProject(c.user.Project.ID))
+	// Get network details
+	network, err := c.getNetwork(isoNet.Spec.ID)
 	if err != nil {
-		return errors.Wrapf(err, "failed to get network by ID %s", isoNet.Spec.ID)
-	}
-	if count == 0 {
-		return errors.Errorf("no network found with ID %s", isoNet.Spec.ID)
+		return err
 	}
+
+	// Early return if egress default policy is true
 	if network.Egressdefaultpolicy {
+		isoNet.Status.FirewallRulesOpened = true
 		return nil
 	}
 
+	// Check if all required firewall rules exist
+	allRulesPresent, err := c.checkFirewallRules(isoNet)
+	if err != nil {
+		return err
+	}
+	if allRulesPresent {
+		isoNet.Status.FirewallRulesOpened = true
+		return nil
+	}
+
+	// Reset status if rules are missing
+	isoNet.Status.FirewallRulesOpened = false
+
+	// Create firewall rules for each protocol
 	protocols := []string{NetworkProtocolTCP, NetworkProtocolUDP, NetworkProtocolICMP}
 	for _, proto := range protocols {
-		var err error
-		if isoNet.Status.RoutingMode != "" {
-			p := c.cs.Firewall.NewCreateRoutingFirewallRuleParams(isoNet.Spec.ID, proto)
-			if proto == "icmp" {
-				p.SetIcmptype(-1)
-				p.SetIcmpcode(-1)
-			}
-			_, err = c.cs.Firewall.CreateRoutingFirewallRule(p)
-		} else {
-			p := c.cs.Firewall.NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, proto)
+		if err := c.createFirewallRule(isoNet, proto); err != nil {
+			retErr = err
+		}
+	}
+
+	// Mark firewall rules as opened if no errors occurred
+	if retErr == nil {
+		isoNet.Status.FirewallRulesOpened = true
+	}
+
+	c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(retErr)
+	return retErr
+}
+
+// Helper function to check if all required firewall rules exist
+func (c *client) checkFirewallRules(isoNet *infrav1.CloudStackIsolatedNetwork) (bool, error) {
+	protocols := []string{NetworkProtocolTCP, NetworkProtocolUDP, NetworkProtocolICMP}
+	p := c.cs.Firewall.NewListEgressFirewallRulesParams()
+	p.SetNetworkid(isoNet.Spec.ID)
+	setIfNotEmpty(c.user.Project.ID, p.SetProjectid)
+
+	rules, err := c.cs.Firewall.ListEgressFirewallRules(p)
+	if err != nil {
+		return false, errors.Wrapf(err, "failed to list egress firewall rules for network ID %s", isoNet.Spec.ID)
+	}
 
-			if proto == "icmp" {
-				p.SetIcmptype(-1)
-				p.SetIcmpcode(-1)
+	// Create a map to track found protocols
+	foundProtocols := make(map[string]bool)
+	for _, proto := range protocols {
+		foundProtocols[proto] = false
+	}
+
+	// Check each rule for matching protocol and parameters
+	for _, rule := range rules.EgressFirewallRules {
+		if _, exists := foundProtocols[rule.Protocol]; exists {
+			if rule.Protocol == "icmp" {
+				// For ICMP, ensure icmptype and icmpcode are -1
+				if rule.Icmptype == -1 && rule.Icmpcode == -1 {
+					foundProtocols[rule.Protocol] = true
+				}
+			} else {
+				// For TCP/UDP, ensure no specific ports are set (all ports)
+				if rule.Startport == 0 && rule.Endport == 0 {
+					foundProtocols[rule.Protocol] = true
+				}
 			}
+		}
+	}
 
-			_, err = c.cs.Firewall.CreateEgressFirewallRule(p)
+	// Return true only if all required protocols are found
+	for _, proto := range protocols {
+		if !foundProtocols[proto] {
+			return false, nil
 		}
-		if err != nil &&
-			// Ignore errors regarding already existing fw rules for TCP/UDP for non-dynamic routing mode
-			!strings.Contains(strings.ToLower(err.Error()), "there is already") &&
-			!strings.Contains(strings.ToLower(err.Error()), "conflicts with rule") &&
-			// Ignore errors regarding already existing fw rule for ICMP
-			!strings.Contains(strings.ToLower(err.Error()), "new rule conflicts with existing rule") {
-			retErr = errors.Wrapf(
-				err, "failed creating egress firewall rule for network ID %s protocol %s", isoNet.Spec.ID, proto)
+	}
+	return true, nil
+}
+
+// Helper function to get network details
+func (c *client) getNetwork(networkID string) (*cloudstack.Network, error) {
+	network, count, err := c.cs.Network.GetNetworkByID(networkID, cloudstack.WithProject(c.user.Project.ID))
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to get network by ID %s", networkID)
+	}
+	if count == 0 {
+		return nil, errors.Errorf("no network found with ID %s", networkID)
+	}
+	return network, nil
+}
+
+// Helper function to create a firewall rule for a given protocol
+func (c *client) createFirewallRule(isoNet *infrav1.CloudStackIsolatedNetwork, proto string) error {
+	var p interface{}
+	if isoNet.Status.RoutingMode != "" {
+		p = c.cs.Firewall.NewCreateRoutingFirewallRuleParams(isoNet.Spec.ID, proto)
+	} else {
+		p = c.cs.Firewall.NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, proto)
+	}
+
+	// Set ICMP-specific parameters
+	if proto == "icmp" {
+		if routingP, ok := p.(*cloudstack.CreateRoutingFirewallRuleParams); ok {
+			routingP.SetIcmptype(-1)
+			routingP.SetIcmpcode(-1)
+		} else if egressP, ok := p.(*cloudstack.CreateEgressFirewallRuleParams); ok {
+			egressP.SetIcmptype(-1)
+			egressP.SetIcmpcode(-1)
 		}
 	}
-	c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(retErr)
-	return retErr
+
+	// Create the firewall rule
+	var err error
+	if routingP, ok := p.(*cloudstack.CreateRoutingFirewallRuleParams); ok {
+		_, err = c.cs.Firewall.CreateRoutingFirewallRule(routingP)
+	} else if egressP, ok := p.(*cloudstack.CreateEgressFirewallRuleParams); ok {
+		_, err = c.cs.Firewall.CreateEgressFirewallRule(egressP)
+	}
+
+	// Ignore specific errors
+	if err != nil && !c.isIgnorableError(err) {
+		return errors.Wrapf(err, "failed creating firewall rule for network ID %s protocol %s", isoNet.Spec.ID, proto)
+	}
+	return nil
+}
+
+// Helper function to check if an error is ignorable
+func (c *client) isIgnorableError(err error) bool {
+	errorMsg := strings.ToLower(err.Error())
+	return strings.Contains(errorMsg, "there is already") ||
+		strings.Contains(errorMsg, "conflicts with rule") ||
+		strings.Contains(errorMsg, "new rule conflicts with existing rule")
 }
 
 // GetPublicIP gets a public IP with ID for cluster endpoint.

pkg/cloud/tags.go

@@ -45,15 +45,6 @@ const (
 	ResourceTypeIPAddress ResourceType = "PublicIpAddress"
 )
 
-// ignoreAlreadyPresentErrors returns nil if the error is an already present tag error.
-func ignoreAlreadyPresentErrors(err error, rType ResourceType, rID string) error {
-	matchSubString := strings.ToLower("already on " + string(rType) + " with id " + rID)
-	if err != nil && !strings.Contains(strings.ToLower(err.Error()), matchSubString) {
-		return err
-	}
-	return nil
-}
-
 func (c *client) IsCapcManaged(resourceType ResourceType, resourceID string) (bool, error) {
 	tags, err := c.GetTags(resourceType, resourceID)
 	if err != nil {
@@ -117,10 +108,30 @@ func (c *client) DoClusterTagsAllowDisposal(resourceType ResourceType, resourceI
 
 // AddTags adds arbitrary tags to a resource.
 func (c *client) AddTags(resourceType ResourceType, resourceID string, tags map[string]string) error {
-	p := c.cs.Resourcetags.NewCreateTagsParams([]string{resourceID}, string(resourceType), tags)
-	_, err := c.cs.Resourcetags.CreateTags(p)
-	c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
-	return ignoreAlreadyPresentErrors(err, resourceType, resourceID)
+	// Retrieve existing tags
+	existingTags, err := c.GetTags(resourceType, resourceID)
+	if err != nil {
+		return errors.Wrapf(err, "fetching tags for resource %s with ID %s", resourceType, resourceID)
+	}
+
+	// Identify tags that need to be added or updated
+	tagsToAdd := make(map[string]string)
+	for key, value := range tags {
+		if existingValue, exists := existingTags[key]; !exists || existingValue != value {
+			tagsToAdd[key] = value
+		}
+	}
+
+	// If there are tags to add, call the API
+	if len(tagsToAdd) > 0 {
+		p := c.cs.Resourcetags.NewCreateTagsParams([]string{resourceID}, string(resourceType), tagsToAdd)
+		if _, err := c.cs.Resourcetags.CreateTags(p); err != nil {
+			c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err)
+			return errors.Wrapf(err, "creating tags for resource %s with ID %s", resourceType, resourceID)
+		}
+	}
+
+	return nil
 }
 
 // GetTags gets all of a resource's tags.