diff --git a/api/v1beta1/zz_generated.conversion.go b/api/v1beta1/zz_generated.conversion.go index 51eb2885..a5f0e855 100644 --- a/api/v1beta1/zz_generated.conversion.go +++ b/api/v1beta1/zz_generated.conversion.go @@ -529,6 +529,7 @@ func autoConvert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta1_CloudStackIs out.PublicIPID = in.PublicIPID out.LBRuleID = in.LBRuleID // WARNING: in.RoutingMode requires manual conversion: does not exist in peer-type + // WARNING: in.FirewallRulesOpened requires manual conversion: does not exist in peer-type out.Ready = in.Ready return nil } diff --git a/api/v1beta2/zz_generated.conversion.go b/api/v1beta2/zz_generated.conversion.go index 38e70b6d..58c81e96 100644 --- a/api/v1beta2/zz_generated.conversion.go +++ b/api/v1beta2/zz_generated.conversion.go @@ -838,6 +838,7 @@ func autoConvert_v1beta3_CloudStackIsolatedNetworkStatus_To_v1beta2_CloudStackIs out.PublicIPID = in.PublicIPID out.LBRuleID = in.LBRuleID // WARNING: in.RoutingMode requires manual conversion: does not exist in peer-type + // WARNING: in.FirewallRulesOpened requires manual conversion: does not exist in peer-type out.Ready = in.Ready return nil } diff --git a/api/v1beta3/cloudstackisolatednetwork_types.go b/api/v1beta3/cloudstackisolatednetwork_types.go index fc868f1d..17bd0bd5 100644 --- a/api/v1beta3/cloudstackisolatednetwork_types.go +++ b/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"` } diff --git a/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackisolatednetworks.yaml b/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackisolatednetworks.yaml index b12fa680..1dcfbc36 100644 --- a/config/crd/bases/infrastructure.cluster.x-k8s.io_cloudstackisolatednetworks.yaml +++ b/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 diff --git a/pkg/cloud/isolated_network.go b/pkg/cloud/isolated_network.go index b761a7c7..dfce7468 100644 --- a/pkg/cloud/isolated_network.go +++ b/pkg/cloud/isolated_network.go @@ -154,54 +154,149 @@ 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 + // Firewall rules are not opened for isolated networks within a VPC because VPCs have their own mechanisms for managing firewall rules. 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 { + c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(err) + return errors.Wrapf(err, "failed creating firewall rule for network ID %s", isoNet.Spec.ID) + } + } + + // Mark firewall rules as opened if all rules were created successfully + isoNet.Status.FirewallRulesOpened = true + return nil +} + +// 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 + } } + } + } + + // Return true only if all required protocols are found + for _, proto := range protocols { + if !foundProtocols[proto] { + return false, nil + } + } + 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 +} - _, err = c.cs.Firewall.CreateEgressFirewallRule(p) +// Helper function to create a firewall rule for a given protocol +func (c *client) createFirewallRule(isoNet *infrav1.CloudStackIsolatedNetwork, proto string) error { + if isoNet.Status.RoutingMode != "" { + // Handle routing firewall rules + p := c.cs.Firewall.NewCreateRoutingFirewallRuleParams(isoNet.Spec.ID, proto) + if proto == "icmp" { + p.SetIcmptype(-1) + p.SetIcmpcode(-1) } - 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) + _, err := c.cs.Firewall.CreateRoutingFirewallRule(p) + if err != nil && !c.isIgnorableError(err) { + return errors.Wrapf(err, "failed creating routing firewall rule for network ID %s protocol %s", isoNet.Spec.ID, proto) } + return nil } - c.customMetrics.EvaluateErrorAndIncrementAcsReconciliationErrorCounter(retErr) - return retErr + + // Handle egress firewall rules + p := c.cs.Firewall.NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, proto) + if proto == "icmp" { + p.SetIcmptype(-1) + p.SetIcmpcode(-1) + } + _, err := c.cs.Firewall.CreateEgressFirewallRule(p) + if err != nil && !c.isIgnorableError(err) { + return errors.Wrapf(err, "failed creating egress firewall rule for network ID %s protocol %s", isoNet.Spec.ID, proto) + } + return nil +} + +// Named constants for ignorable error substrings +const ( + ErrAlreadyExists = "there is already" + ErrRuleConflict = "conflicts with rule" + ErrNewRuleConflict = "new rule conflicts with existing rule" +) + +// 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, ErrAlreadyExists) || + strings.Contains(errorMsg, ErrRuleConflict) || + strings.Contains(errorMsg, ErrNewRuleConflict) } // GetPublicIP gets a public IP with ID for cluster endpoint. @@ -300,9 +395,9 @@ func (c *client) GetOrCreateIsolatedNetwork( isoNet *infrav1.CloudStackIsolatedNetwork, csCluster *infrav1.CloudStackCluster, ) error { - // Get or create the isolated network itself and resolve details into passed custom resources. net := isoNet.Network() - if err := c.ResolveNetwork(net); err != nil { // Doesn't exist, create isolated network. + err := c.ResolveNetwork(net) + if err != nil { if err = c.CreateIsolatedNetwork(fd, isoNet); err != nil { return errors.Wrap(err, "creating a new isolated network") } diff --git a/pkg/cloud/isolated_network_test.go b/pkg/cloud/isolated_network_test.go index 8db967d7..841b1029 100644 --- a/pkg/cloud/isolated_network_test.go +++ b/pkg/cloud/isolated_network_test.go @@ -70,6 +70,8 @@ var _ = ginkgo.Describe("Network", func() { ginkgo.It("calls to create an isolated network when not found", func() { dummies.Zone1.Network = dummies.ISONet1 dummies.Zone1.Network.ID = "" + dummies.CSISONet1.Spec.VPC = nil // Explicitly disable VPC for CSISONet1. + dummies.ISONet1.VPC = nil // Explicitly disable VPC for ISONet1. nos.EXPECT().GetNetworkOfferingID(gomock.Any()).Return("someOfferingID", 1, nil) ns.EXPECT().NewCreateNetworkParams(gomock.Any(), gomock.Any(), gomock.Any()). @@ -85,6 +87,15 @@ var _ = ginkgo.Describe("Network", func() { as.EXPECT().NewAssociateIpAddressParams().Return(&csapi.AssociateIpAddressParams{}) as.EXPECT().AssociateIpAddress(gomock.Any()) ns.EXPECT().GetNetworkByID(dummies.ISONet1.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil) + + // Mock firewall rule check: no existing rules, so create TCP, UDP, ICMP rules + fs.EXPECT().NewListEgressFirewallRulesParams().Return(&csapi.ListEgressFirewallRulesParams{}) + fs.EXPECT().ListEgressFirewallRules(gomock.Any()).Return(&csapi.ListEgressFirewallRulesResponse{ + Count: 0, + EgressFirewallRules: []*csapi.EgressFirewallRule{}, + }, nil) + + // Mock firewall rule creation fs.EXPECT().NewCreateEgressFirewallRuleParams(dummies.ISONet1.ID, gomock.Any()). DoAndReturn(func(_ string, protocol string) *csapi.CreateEgressFirewallRuleParams { p := &csapi.CreateEgressFirewallRuleParams{} @@ -102,17 +113,29 @@ var _ = ginkgo.Describe("Network", func() { fs.EXPECT().CreateEgressFirewallRule(&csapi.CreateEgressFirewallRuleParams{}). Return(&csapi.CreateEgressFirewallRuleResponse{}, nil).Times(2), fs.EXPECT().CreateEgressFirewallRule(ruleParamsICMP). - Return(&csapi.CreateEgressFirewallRuleResponse{}, nil)) + Return(&csapi.CreateEgressFirewallRuleResponse{}, nil), + ) - // Will add cluster tag once to Network and once to PublicIP. - createdByResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}} + // Expect 6 ListTags calls: 3 for network (AddCreatedByCAPCTag, AddClusterTag x2), + // 3 for public IP (GetPublicIP, AddClusterTag, AddCreatedByCAPCTag) + emptyResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{}} + clusterTagResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}} gomock.InOrder( - rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), - rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil), - rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), - rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil)) - - // Will add creation and cluster tags to network and PublicIP. + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddCreatedByCAPCTag (Network) + rs.EXPECT().ListTags(gomock.Any()).Return(emptyResponse, nil), + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddClusterTag (Network, IsCapcManaged) + rs.EXPECT().ListTags(gomock.Any()).Return(clusterTagResponse, nil), + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddClusterTag (Network, AddTags) + rs.EXPECT().ListTags(gomock.Any()).Return(clusterTagResponse, nil), + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For GetPublicIP (PublicIpAddress) + rs.EXPECT().ListTags(gomock.Any()).Return(clusterTagResponse, nil), + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddClusterTag (PublicIpAddress) + rs.EXPECT().ListTags(gomock.Any()).Return(clusterTagResponse, nil), + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // For AddCreatedByCAPCTag (PublicIpAddress) + rs.EXPECT().ListTags(gomock.Any()).Return(emptyResponse, nil), + ) + + // Expect 4 CreateTags calls: 2 for network (cluster + created-by), 2 for public IP (cluster + created-by) rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()). Return(&csapi.CreateTagsParams{}).Times(4) rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(4) @@ -136,11 +159,24 @@ var _ = ginkgo.Describe("Network", func() { }) }) - ginkgo.Context("for a closed firewall", func() { - ginkgo.It("OpenFirewallRule asks CloudStack to open the firewall", func() { - dummies.Zone1.Network = dummies.ISONet1 - ns.EXPECT().GetNetworkByID(dummies.ISONet1.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil) - fs.EXPECT().NewCreateEgressFirewallRuleParams(dummies.ISONet1.ID, gomock.Any()). + ginkgo.Context("OpenFirewallRules", func() { + ginkgo.It("creates firewall rules when none exist", func() { + isoNet := dummies.CSISONet1 + isoNet.Status.RoutingMode = "" // Egress rules + isoNet.Spec.ID = "net-123" + + // Mock GetNetworkByID to return a network with Egressdefaultpolicy set to false + ns.EXPECT().GetNetworkByID(isoNet.Spec.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil) + + // Mock firewall rule check: no existing rules + fs.EXPECT().NewListEgressFirewallRulesParams().Return(&csapi.ListEgressFirewallRulesParams{}) + fs.EXPECT().ListEgressFirewallRules(gomock.Any()).Return(&csapi.ListEgressFirewallRulesResponse{ + Count: 0, + EgressFirewallRules: []*csapi.EgressFirewallRule{}, + }, nil) + + // Mock firewall rule creation for TCP, UDP, ICMP + fs.EXPECT().NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, gomock.Any()). DoAndReturn(func(_ string, protocol string) *csapi.CreateEgressFirewallRuleParams { p := &csapi.CreateEgressFirewallRuleParams{} if protocol == "icmp" { @@ -149,45 +185,62 @@ var _ = ginkgo.Describe("Network", func() { } return p }).Times(3) + fs.EXPECT().CreateEgressFirewallRule(gomock.Any()).Return(&csapi.CreateEgressFirewallRuleResponse{}, nil).Times(3) - ruleParamsICMP := &csapi.CreateEgressFirewallRuleParams{} - ruleParamsICMP.SetIcmptype(-1) - ruleParamsICMP.SetIcmpcode(-1) - gomock.InOrder( - fs.EXPECT().CreateEgressFirewallRule(&csapi.CreateEgressFirewallRuleParams{}). - Return(&csapi.CreateEgressFirewallRuleResponse{}, nil).Times(2), - fs.EXPECT().CreateEgressFirewallRule(ruleParamsICMP). - Return(&csapi.CreateEgressFirewallRuleResponse{}, nil)) + err := client.OpenFirewallRules(isoNet) + gomega.Expect(err).To(gomega.BeNil()) + gomega.Expect(isoNet.Status.FirewallRulesOpened).To(gomega.BeTrue()) + }) - gomega.Ω(client.OpenFirewallRules(dummies.CSISONet1)).Should(gomega.Succeed()) + ginkgo.It("skips creating firewall rules if they already exist", func() { + isoNet := dummies.CSISONet1 + isoNet.Status.RoutingMode = "" // Egress rules + isoNet.Spec.ID = "net-123" + + // Mock GetNetworkByID to return a network with Egressdefaultpolicy set to false + ns.EXPECT().GetNetworkByID(isoNet.Spec.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil) + + // Mock firewall rule check: all required rules exist + fs.EXPECT().NewListEgressFirewallRulesParams().Return(&csapi.ListEgressFirewallRulesParams{}) + fs.EXPECT().ListEgressFirewallRules(gomock.Any()).Return(&csapi.ListEgressFirewallRulesResponse{ + Count: 3, + EgressFirewallRules: []*csapi.EgressFirewallRule{ + {Protocol: "tcp"}, + {Protocol: "udp"}, + {Protocol: "icmp", Icmptype: -1, Icmpcode: -1}, + }, + }, nil) + + err := client.OpenFirewallRules(isoNet) + gomega.Expect(err).To(gomega.BeNil()) + gomega.Expect(isoNet.Status.FirewallRulesOpened).To(gomega.BeTrue()) }) - }) - ginkgo.Context("for an open firewall", func() { - ginkgo.It("OpenFirewallRule asks CloudStack to open the firewall anyway, but doesn't fail", func() { - dummies.Zone1.Network = dummies.ISONet1 + ginkgo.It("handles error during firewall rule creation", func() { + isoNet := dummies.CSISONet1 + isoNet.Status.RoutingMode = "" // Egress rules + isoNet.Spec.ID = "net-123" - ns.EXPECT().GetNetworkByID(dummies.ISONet1.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil) - fs.EXPECT().NewCreateEgressFirewallRuleParams(dummies.ISONet1.ID, gomock.Any()). - DoAndReturn(func(_ string, protocol string) *csapi.CreateEgressFirewallRuleParams { - p := &csapi.CreateEgressFirewallRuleParams{} - if protocol == "icmp" { - p.SetIcmptype(-1) - p.SetIcmpcode(-1) - } - return p - }).Times(3) + // Mock GetNetworkByID to return a network with Egressdefaultpolicy set to false + ns.EXPECT().GetNetworkByID(isoNet.Spec.ID, gomock.Any()).Return(&csapi.Network{Egressdefaultpolicy: false}, 1, nil) - ruleParamsICMP := &csapi.CreateEgressFirewallRuleParams{} - ruleParamsICMP.SetIcmptype(-1) - ruleParamsICMP.SetIcmpcode(-1) - gomock.InOrder( - fs.EXPECT().CreateEgressFirewallRule(&csapi.CreateEgressFirewallRuleParams{}). - Return(&csapi.CreateEgressFirewallRuleResponse{}, nil).Times(2), - fs.EXPECT().CreateEgressFirewallRule(ruleParamsICMP). - Return(&csapi.CreateEgressFirewallRuleResponse{}, nil)) + // Mock firewall rule check: no existing rules + fs.EXPECT().NewListEgressFirewallRulesParams().Return(&csapi.ListEgressFirewallRulesParams{}) + fs.EXPECT().ListEgressFirewallRules(gomock.Any()).Return(&csapi.ListEgressFirewallRulesResponse{ + Count: 0, + EgressFirewallRules: []*csapi.EgressFirewallRule{}, + }, nil) + + // Mock firewall rule creation: fail on UDP + fs.EXPECT().NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, "tcp").Return(&csapi.CreateEgressFirewallRuleParams{}) + fs.EXPECT().CreateEgressFirewallRule(gomock.Any()).Return(&csapi.CreateEgressFirewallRuleResponse{}, nil) + fs.EXPECT().NewCreateEgressFirewallRuleParams(isoNet.Spec.ID, "udp").Return(&csapi.CreateEgressFirewallRuleParams{}) + fs.EXPECT().CreateEgressFirewallRule(gomock.Any()).Return(nil, errors.New("failed to create UDP rule")) - gomega.Ω(client.OpenFirewallRules(dummies.CSISONet1)).Should(gomega.Succeed()) + err := client.OpenFirewallRules(isoNet) + gomega.Expect(err).To(gomega.HaveOccurred()) + gomega.Expect(err.Error()).To(gomega.ContainSubstring("failed creating egress firewall rule for network ID net-123 protocol udp")) + gomega.Expect(isoNet.Status.FirewallRulesOpened).To(gomega.BeFalse()) }) }) @@ -252,17 +305,19 @@ var _ = ginkgo.Describe("Network", func() { aip := &csapi.AssociateIpAddressParams{} as.EXPECT().NewAssociateIpAddressParams().Return(aip) as.EXPECT().AssociateIpAddress(aip).Return(&csapi.AssociateIpAddressResponse{}, nil) - // Will add cluster tag once to Network and once to PublicIP. createdByResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}} + emptyResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{}} gomock.InOrder( - rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), - rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil)) - - // Will add creation and cluster tags to network and PublicIP. + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // AddClusterTag: IsCapcManaged + rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil), + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // AddClusterTag: AddTags + rs.EXPECT().ListTags(gomock.Any()).Return(emptyResponse, nil), + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), // AddCreatedByCAPCTag: AddTags + rs.EXPECT().ListTags(gomock.Any()).Return(emptyResponse, nil), + ) rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()). Return(&csapi.CreateTagsParams{}).Times(2) rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil).Times(2) - gomega.Ω(client.AssociatePublicIPAddress(dummies.CSFailureDomain1, dummies.CSISONet1, dummies.CSCluster)).Should(gomega.Succeed()) }) diff --git a/pkg/cloud/tags.go b/pkg/cloud/tags.go index 28dc7907..57b73306 100644 --- a/pkg/cloud/tags.go +++ b/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. diff --git a/pkg/cloud/tags_test.go b/pkg/cloud/tags_test.go index 4c92343a..459bb77c 100644 --- a/pkg/cloud/tags_test.go +++ b/pkg/cloud/tags_test.go @@ -156,8 +156,10 @@ var _ = ginkgo.Describe("Tag Unit Tests", func() { createdByCAPCResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}} rtlp := &csapi.ListTagsParams{} ctp := &csapi.CreateTagsParams{} - rs.EXPECT().NewListTagsParams().Return(rtlp) - rs.EXPECT().ListTags(rtlp).Return(createdByCAPCResponse, nil) + // Expecting NewListTagsParams to be called twice: + // Once for verifying existing tags and once for creating new tags. + rs.EXPECT().NewListTagsParams().Return(rtlp).Times(2) + rs.EXPECT().ListTags(rtlp).Return(createdByCAPCResponse, nil).Times(2) rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).Return(ctp) rs.EXPECT().CreateTags(ctp).Return(&csapi.CreateTagsResponse{}, nil) gomega.Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.CSISONet1.Spec.ID, dummies.CSCluster)).Should(gomega.Succeed()) diff --git a/pkg/cloud/vpc_test.go b/pkg/cloud/vpc_test.go index ca966d07..3606ba7c 100644 --- a/pkg/cloud/vpc_test.go +++ b/pkg/cloud/vpc_test.go @@ -169,6 +169,8 @@ var _ = ginkgo.Describe("VPC", func() { vs.EXPECT().NewCreateVPCParams(dummyVPC.Name, dummyVPC.Name, offeringID, dummyFD.Spec.Zone.ID).Return(createVPCParams) vs.EXPECT().CreateVPC(createVPCParams).Return(createVPCResponse, nil) rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).Return(&csapi.CreateTagsParams{}) + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}) + rs.EXPECT().ListTags(gomock.Any()).Return(&csapi.ListTagsResponse{}, nil) rs.EXPECT().CreateTags(gomock.Any()).Return(&csapi.CreateTagsResponse{}, nil) gomega.Ω(client.CreateVPC(&dummyFD, &dummyVPC)).Should(gomega.Succeed())