Skip to content

Commit 201f829

Browse files
author
Joshua Reed
committed
Fixup tagging tests for created by.
1 parent 38eab63 commit 201f829

File tree

3 files changed

+69
-87
lines changed

3 files changed

+69
-87
lines changed

‎pkg/cloud/network.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,10 @@ func (c *client) CreateIsolatedNetwork(csCluster *infrav1.CloudStackCluster) (re
139139
if err != nil {
140140
return err
141141
}
142-
if err := c.AddClusterTag(ResourceTypeNetwork, zoneStatus.Network.ID, csCluster, addCreatedByTag); err != nil {
142+
if err := c.AddClusterTag(ResourceTypeNetwork, zoneStatus.Network.ID, csCluster); err != nil {
143+
return err
144+
}
145+
if err := c.AddCreatedByCAPCTag(ResourceTypeNetwork, zoneStatus.Network.ID); err != nil {
143146
return err
144147
}
145148

@@ -247,9 +250,8 @@ func (c *client) AssociatePublicIPAddress(csCluster *infrav1.CloudStackCluster)
247250
csCluster.Spec.ControlPlaneEndpoint.Host = publicAddress.Ipaddress
248251
csCluster.Status.PublicIPID = publicAddress.Id
249252

250-
if publicAddress.Allocated != "" {
251-
// Address already allocated to network. Allocated is a timestamp -- not a boolean.
252-
return c.AddClusterTag(ResourceTypeIPAddress, publicAddress.Id, csCluster, false)
253+
if publicAddress.Allocated != "" { // Address already allocated. Allocated is a timestamp -- not a boolean.
254+
return c.AddClusterTag(ResourceTypeIPAddress, publicAddress.Id, csCluster)
253255
} // Address not yet allocated. Allocate now.
254256

255257
// Public IP found, but not yet allocated to network.

‎pkg/cloud/tags.go

Lines changed: 41 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,16 @@ package cloud
1919
import (
2020
"strings"
2121

22+
"github.com/hashicorp/go-multierror"
23+
2224
infrav1 "github.com/aws/cluster-api-provider-cloudstack/api/v1beta1"
25+
"github.com/pkg/errors"
2326
)
2427

2528
type TagIface interface {
26-
AddClusterTag(ResourceType, string, *infrav1.CloudStackCluster, bool) error
29+
AddClusterTag(ResourceType, string, *infrav1.CloudStackCluster) error
2730
DeleteClusterTag(ResourceType, string, *infrav1.CloudStackCluster) error
31+
AddCreatedByCAPCTag(ResourceType, string) error
2832
DeleteCreatedByCAPCTag(ResourceType, string) error
2933
DoClusterTagsAllowDisposal(ResourceType, string) (bool, error)
3034
AddTags(ResourceType, string, map[string]string) error
@@ -41,62 +45,37 @@ const (
4145
ResourceTypeIPAddress ResourceType = "PublicIpAddress"
4246
)
4347

44-
// AddClusterTag adds cluster-related tags to a resource. One tag indicates that the resource is used by a given
45-
// cluster. The other tag, if applied, indicates that CAPC created the resource and may dispose of it later.
46-
func (c *client) AddClusterTag(
47-
resourceType ResourceType,
48-
resourceID string,
49-
csCluster *infrav1.CloudStackCluster,
50-
addCreatedByCAPCTag bool,
51-
) error {
52-
53-
clusterTagName := generateClusterTagName(csCluster)
54-
newTags := map[string]string{}
55-
56-
existingTags, err := c.GetTags(resourceType, resourceID)
57-
if err != nil {
48+
// ignoreAlreadyPresentErrors returns nil if the error is an already present tag error.
49+
func ignoreAlreadyPresentErrors(err error, rType ResourceType, rID string) error {
50+
matchSubString := strings.ToLower("already on " + string(rType) + " with id " + rID)
51+
if err != nil && !strings.Contains(strings.ToLower(err.Error()), matchSubString) {
5852
return err
5953
}
60-
61-
if existingTags[clusterTagName] == "" {
62-
newTags[clusterTagName] = "1"
63-
}
64-
65-
if len(newTags) > 0 {
66-
return c.AddTags(resourceType, resourceID, newTags)
67-
}
68-
6954
return nil
7055
}
7156

72-
// DeleteClusterTag deletes the tag that associates the resource with a given cluster.
73-
func (c *client) DeleteClusterTag(resourceType ResourceType, resourceID string, csCluster *infrav1.CloudStackCluster) error {
74-
tags, err := c.GetTags(resourceType, resourceID)
75-
if err != nil {
76-
return err
77-
}
57+
// AddClusterTag adds cluster tag to a resource. This tag indicates the resource is used by a given the cluster.
58+
func (c *client) AddClusterTag(rType ResourceType, rID string, csCluster *infrav1.CloudStackCluster) error {
59+
clusterTagName := generateClusterTagName(csCluster)
60+
return c.AddTags(rType, rID, map[string]string{clusterTagName: "1"})
61+
}
7862

63+
// DeleteClusterTag deletes the tag that associates the resource with a given cluster.
64+
func (c *client) DeleteClusterTag(rType ResourceType, rID string, csCluster *infrav1.CloudStackCluster) error {
7965
clusterTagName := generateClusterTagName(csCluster)
80-
if tagValue := tags[clusterTagName]; tagValue != "" {
81-
return c.DeleteTags(resourceType, resourceID, map[string]string{clusterTagName: tagValue})
82-
}
66+
return c.DeleteTags(rType, rID, map[string]string{clusterTagName: "1"})
67+
}
8368

84-
return nil
69+
// AddCreatedByCAPCTag deletes the tag that indicates that the resource was created by CAPC. This is useful when a
70+
// resource is disassociated instead of deleted. That way the tag won't cause confusion if the resource is reused later.
71+
func (c *client) AddCreatedByCAPCTag(rType ResourceType, rID string) error {
72+
return c.AddTags(rType, rID, map[string]string{createdByCAPCTagName: "1"})
8573
}
8674

8775
// DeleteCreatedByCAPCTag deletes the tag that indicates that the resource was created by CAPC. This is useful when a
8876
// resource is disassociated instead of deleted. That way the tag won't cause confusion if the resource is reused later.
89-
func (c *client) DeleteCreatedByCAPCTag(resourceType ResourceType, resourceID string) error {
90-
tags, err := c.GetTags(resourceType, resourceID)
91-
if err != nil {
92-
return err
93-
}
94-
95-
if tagValue := tags[createdByCAPCTagName]; tagValue != "" {
96-
return c.DeleteTags(resourceType, resourceID, map[string]string{createdByCAPCTagName: tagValue})
97-
}
98-
99-
return nil
77+
func (c *client) DeleteCreatedByCAPCTag(rType ResourceType, rID string) error {
78+
return c.DeleteTags(rType, rID, map[string]string{createdByCAPCTagName: "1"})
10079
}
10180

10281
// DoClusterTagsAllowDisposal checks to see if the resource is in a state that makes it eligible for disposal. CAPC can
@@ -121,7 +100,7 @@ func (c *client) DoClusterTagsAllowDisposal(resourceType ResourceType, resourceI
121100
func (c *client) AddTags(resourceType ResourceType, resourceID string, tags map[string]string) error {
122101
p := c.cs.Resourcetags.NewCreateTagsParams([]string{resourceID}, string(resourceType), tags)
123102
_, err := c.cs.Resourcetags.CreateTags(p)
124-
return err
103+
return ignoreAlreadyPresentErrors(err, resourceType, resourceID)
125104
}
126105

127106
// GetTags gets all of a resource's tags.
@@ -141,13 +120,23 @@ func (c *client) GetTags(resourceType ResourceType, resourceID string) (map[stri
141120
return tags, nil
142121
}
143122

144-
// DeleteTags deletes the given tags from a resource. If the tags don't exist, or if the values don't match, it will
145-
// result in an error.
123+
// DeleteTags deletes the given tags from a resource.
124+
// Ignores errors if the tag is not present.
146125
func (c *client) DeleteTags(resourceType ResourceType, resourceID string, tagsToDelete map[string]string) error {
147-
p := c.cs.Resourcetags.NewDeleteTagsParams([]string{resourceID}, string(resourceType))
148-
p.SetTags(tagsToDelete)
149-
_, err := c.cs.Resourcetags.DeleteTags(p)
150-
return err
126+
for tagkey, tagval := range tagsToDelete {
127+
p := c.cs.Resourcetags.NewDeleteTagsParams([]string{resourceID}, string(resourceType))
128+
p.SetTags(tagsToDelete)
129+
if _, err1 := c.cs.Resourcetags.DeleteTags(p); err1 != nil { // Error in deletion attempt. Check for tag.
130+
currTag := map[string]string{tagkey: tagval}
131+
if tags, err2 := c.GetTags(resourceType, resourceID); len(tags) != 0 {
132+
if _, foundTag := tags[tagkey]; foundTag {
133+
return errors.Wrapf(multierror.Append(err1, err2),
134+
"could not remove tag %s from %s with ID %s", currTag, resourceType, resourceID)
135+
}
136+
}
137+
}
138+
}
139+
return nil
151140
}
152141

153142
func generateClusterTagName(csCluster *infrav1.CloudStackCluster) string {

‎pkg/cloud/tags_test.go

Lines changed: 22 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -64,11 +64,11 @@ var _ = Describe("Tag Unit Tests", func() {
6464
})
6565

6666
It("returns an error when you delete a tag that doesn't exist", func() {
67-
Ω(client.DeleteTags(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.Tags)).ShouldNot(Succeed())
67+
Ω(client.DeleteTags(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.Tags)).Should(Succeed())
6868
})
6969

7070
It("adds the tags for a cluster (resource created by CAPC)", func() {
71-
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, true)).
71+
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).
7272
Should(Succeed())
7373

7474
// Verify tags
@@ -79,12 +79,12 @@ var _ = Describe("Tag Unit Tests", func() {
7979
})
8080

8181
It("does not fail when the cluster tags are added twice", func() {
82-
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, true)).Should(Succeed())
83-
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, true)).Should(Succeed())
82+
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
83+
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
8484
})
8585

8686
It("adds the tags for a cluster (resource NOT created by CAPC)", func() {
87-
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, false)).Should(Succeed())
87+
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
8888

8989
// Verify tags
9090
tags, err := client.GetTags(cloud.ResourceTypeNetwork, dummies.Net1.ID)
@@ -94,29 +94,19 @@ var _ = Describe("Tag Unit Tests", func() {
9494
})
9595

9696
It("deletes a cluster tag", func() {
97-
_ = client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, true)
97+
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
9898
Ω(client.DeleteClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
9999

100-
// Verify tags
101-
tags, err := client.GetTags(cloud.ResourceTypeNetwork, dummies.Net1.ID)
102-
Ω(err).Should(BeNil())
103-
Ω(tags[dummies.CSClusterTagKey]).Should(Equal(""))
100+
Ω(client.GetTags(cloud.ResourceTypeNetwork, dummies.Net1.ID)).ShouldNot(HaveKey(dummies.CSClusterTagKey))
104101
})
105102

106-
// TODO add back in w/created by tag implementation.
107-
// It("deletes a CAPC created tag", func() {
108-
// _ = client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, true)
109-
// Ω(client.DeleteCreatedByCAPCTag(cloud.ResourceTypeNetwork, dummies.Net1.ID)).Should(Succeed())
110-
111-
// // Verify tags
112-
// tags, err := client.GetTags(cloud.ResourceTypeNetwork, dummies.Net1.ID)
113-
// Ω(err).Should(BeNil())
114-
// Ω(tags[dummies.CreatedByCapcKey]).Should(Equal(""))
115-
// Ω(tags[dummies.CSClusterTagKey]).Should(Equal("1"))
116-
// })
103+
It("adds and deletes a created by capc tag", func() {
104+
Ω(client.AddCreatedByCAPCTag(cloud.ResourceTypeNetwork, dummies.Net1.ID)).Should(Succeed())
105+
Ω(client.DeleteCreatedByCAPCTag(cloud.ResourceTypeNetwork, dummies.Net1.ID)).Should(Succeed())
106+
})
117107

118108
It("does not fail when cluster and CAPC created tags are deleted twice", func() {
119-
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, true)).Should(Succeed())
109+
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
120110
Ω(client.DeleteClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
121111
Ω(client.DeleteClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
122112
Ω(client.DeleteCreatedByCAPCTag(cloud.ResourceTypeNetwork, dummies.Net1.ID)).Should(Succeed())
@@ -130,19 +120,20 @@ var _ = Describe("Tag Unit Tests", func() {
130120
})
131121

132122
It("does not allow a resource to be deleted when there is a cluster tag", func() {
133-
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, true)).Should(Succeed())
123+
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
134124
tagsAllowDisposal, err := client.DoClusterTagsAllowDisposal(cloud.ResourceTypeNetwork, dummies.Net1.ID)
135125
Ω(err).Should(BeNil())
136126
Ω(tagsAllowDisposal).Should(BeFalse())
137127
})
138128

139-
// TODO add back in w/created by tag implementation.
140-
// It("does allow a resource to be deleted when there are no cluster tags and there is a CAPC created tag", func() {
141-
// _ = client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster, true)
142-
// _ = client.DeleteClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)
143-
// tagsAllowDisposal, err := client.DoClusterTagsAllowDisposal(cloud.ResourceTypeNetwork, dummies.Net1.ID)
144-
// Ω(err).Should(BeNil())
145-
// Ω(tagsAllowDisposal).Should(BeTrue())
146-
// })
129+
It("does allow a resource to be deleted when there are no cluster tags and there is a CAPC created tag", func() {
130+
Ω(client.AddClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
131+
Ω(client.AddCreatedByCAPCTag(cloud.ResourceTypeNetwork, dummies.Net1.ID)).Should(Succeed())
132+
Ω(client.DeleteClusterTag(cloud.ResourceTypeNetwork, dummies.Net1.ID, dummies.CSCluster)).Should(Succeed())
133+
134+
tagsAllowDisposal, err := client.DoClusterTagsAllowDisposal(cloud.ResourceTypeNetwork, dummies.Net1.ID)
135+
Ω(err).Should(BeNil())
136+
Ω(tagsAllowDisposal).Should(BeTrue())
137+
})
147138
})
148139
})

0 commit comments

Comments
 (0)