From 003b7ee7d146af873add44111952144aebc8467f Mon Sep 17 00:00:00 2001 From: Piyush Kumar Date: Fri, 24 Oct 2025 01:43:53 +0530 Subject: [PATCH 1/2] PCP-5371: fixed network tags to always have one capc cluster tag at a time --- .gitignore | 2 ++ pkg/cloud/tags.go | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/.gitignore b/.gitignore index 48558efab..6bc0e6143 100644 --- a/.gitignore +++ b/.gitignore @@ -49,3 +49,5 @@ cloud-config.yaml *~ examples/* hack/tools/share/* + +vendor/ \ No newline at end of file diff --git a/pkg/cloud/tags.go b/pkg/cloud/tags.go index 28dc79073..544b3f063 100644 --- a/pkg/cloud/tags.go +++ b/pkg/cloud/tags.go @@ -65,11 +65,18 @@ func (c *client) IsCapcManaged(resourceType ResourceType, resourceID string) (bo } // AddClusterTag adds cluster tag to a resource. This tag indicates the resource is used by a given the cluster. +// If old cluster tags exist (from previous cluster UIDs), they are removed first to prevent dual-tagging. func (c *client) AddClusterTag(rType ResourceType, rID string, csCluster *infrav1.CloudStackCluster) error { if managedByCAPC, err := c.IsCapcManaged(rType, rID); err != nil { return err } else if managedByCAPC { ClusterTagName := generateClusterTagName(csCluster) + + // Remove old cluster tags before adding new one to prevent dual-tagging during pivot + if err := c.removeOldClusterTags(rType, rID, ClusterTagName); err != nil { + return errors.Wrapf(err, "removing old cluster tags from %s %s", rType, rID) + } + return c.AddTags(rType, rID, map[string]string{ClusterTagName: "1"}) } return nil @@ -163,6 +170,32 @@ func (c *client) DeleteTags(resourceType ResourceType, resourceID string, tagsTo return nil } +// removeOldClusterTags removes any cluster tags that don't match the current cluster UID. +// This prevents dual-tagging when a resource is moved during pivot and gets a new cluster UID. +func (c *client) removeOldClusterTags(rType ResourceType, rID string, currentClusterTagName string) error { + tags, err := c.GetTags(rType, rID) + if err != nil { + return errors.Wrapf(err, "getting tags for %s %s", rType, rID) + } + + oldClusterTags := make(map[string]string) + for tagName, tagValue := range tags { + // Find cluster tags that are NOT the current cluster's tag + if strings.HasPrefix(tagName, ClusterTagNamePrefix) && tagName != currentClusterTagName { + oldClusterTags[tagName] = tagValue + } + } + + // Delete old cluster tags if any exist + if len(oldClusterTags) > 0 { + if err := c.DeleteTags(rType, rID, oldClusterTags); err != nil { + return errors.Wrapf(err, "deleting old cluster tags %v from %s %s", oldClusterTags, rType, rID) + } + } + + return nil +} + func generateClusterTagName(csCluster *infrav1.CloudStackCluster) string { return ClusterTagNamePrefix + string(csCluster.UID) } From 4b259b437f1d00e48ded9228a3a748d2cc109342 Mon Sep 17 00:00:00 2001 From: Piyush Kumar Date: Fri, 24 Oct 2025 02:28:25 +0530 Subject: [PATCH 2/2] fixed tests --- pkg/cloud/isolated_network_test.go | 16 +++++++++++++++- pkg/cloud/tags.go | 10 +++++----- pkg/cloud/tags_test.go | 4 ++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/pkg/cloud/isolated_network_test.go b/pkg/cloud/isolated_network_test.go index 8db967d79..d79f6b336 100644 --- a/pkg/cloud/isolated_network_test.go +++ b/pkg/cloud/isolated_network_test.go @@ -105,10 +105,19 @@ var _ = ginkgo.Describe("Network", func() { Return(&csapi.CreateEgressFirewallRuleResponse{}, nil)) // Will add cluster tag once to Network and once to PublicIP. + // Each AddClusterTag calls GetTags twice (IsCapcManaged + removeOldClusterTags) createdByResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}} gomock.InOrder( + // Network: IsCapcManaged GetTags rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil), + // Network: removeOldClusterTags GetTags + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), + rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil), + // PublicIP: IsCapcManaged GetTags + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), + rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil), + // PublicIP: removeOldClusterTags GetTags rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil)) @@ -252,9 +261,14 @@ 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. + // Will add cluster tag once to PublicIP + // Each AddClusterTag calls GetTags twice (IsCapcManaged + removeOldClusterTags) createdByResponse := &csapi.ListTagsResponse{Tags: []*csapi.Tag{{Key: cloud.CreatedByCAPCTagName, Value: "1"}}} gomock.InOrder( + // PublicIP: IsCapcManaged GetTags + rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), + rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil), + // PublicIP: removeOldClusterTags GetTags rs.EXPECT().NewListTagsParams().Return(&csapi.ListTagsParams{}), rs.EXPECT().ListTags(gomock.Any()).Return(createdByResponse, nil)) diff --git a/pkg/cloud/tags.go b/pkg/cloud/tags.go index 544b3f063..3297bd8b1 100644 --- a/pkg/cloud/tags.go +++ b/pkg/cloud/tags.go @@ -71,12 +71,12 @@ func (c *client) AddClusterTag(rType ResourceType, rID string, csCluster *infrav return err } else if managedByCAPC { ClusterTagName := generateClusterTagName(csCluster) - + // Remove old cluster tags before adding new one to prevent dual-tagging during pivot if err := c.removeOldClusterTags(rType, rID, ClusterTagName); err != nil { return errors.Wrapf(err, "removing old cluster tags from %s %s", rType, rID) } - + return c.AddTags(rType, rID, map[string]string{ClusterTagName: "1"}) } return nil @@ -177,7 +177,7 @@ func (c *client) removeOldClusterTags(rType ResourceType, rID string, currentClu if err != nil { return errors.Wrapf(err, "getting tags for %s %s", rType, rID) } - + oldClusterTags := make(map[string]string) for tagName, tagValue := range tags { // Find cluster tags that are NOT the current cluster's tag @@ -185,14 +185,14 @@ func (c *client) removeOldClusterTags(rType ResourceType, rID string, currentClu oldClusterTags[tagName] = tagValue } } - + // Delete old cluster tags if any exist if len(oldClusterTags) > 0 { if err := c.DeleteTags(rType, rID, oldClusterTags); err != nil { return errors.Wrapf(err, "deleting old cluster tags %v from %s %s", oldClusterTags, rType, rID) } } - + return nil } diff --git a/pkg/cloud/tags_test.go b/pkg/cloud/tags_test.go index 4c92343a4..8f8771cb4 100644 --- a/pkg/cloud/tags_test.go +++ b/pkg/cloud/tags_test.go @@ -156,6 +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{} + // First GetTags call in IsCapcManaged + rs.EXPECT().NewListTagsParams().Return(rtlp) + rs.EXPECT().ListTags(rtlp).Return(createdByCAPCResponse, nil) + // Second GetTags call in removeOldClusterTags rs.EXPECT().NewListTagsParams().Return(rtlp) rs.EXPECT().ListTags(rtlp).Return(createdByCAPCResponse, nil) rs.EXPECT().NewCreateTagsParams(gomock.Any(), gomock.Any(), gomock.Any()).Return(ctp)