Skip to content

Drop duplicate instance tags, if exists #78

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Oct 23, 2018
Merged

Drop duplicate instance tags, if exists #78

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions pkg/cloud/aws/actuators/machine/actuator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,21 @@ type ActuatorParams struct {
AwsClientBuilder awsclient.AwsClientBuilderFuncType
}

// Scan machine tags, and return a deduped tags list
func removeDuplicatedTags(tags []*ec2.Tag) []*ec2.Tag {
m := make(map[string]bool)
result := []*ec2.Tag{}

// look for duplicates
for _, entry := range tags {
if _, value := m[*entry.Key]; !value {
m[*entry.Key] = true
result = append(result, entry)
}
}
return result
}

// NewActuator returns a new AWS Actuator
func NewActuator(params ActuatorParams) (*Actuator, error) {
actuator := &Actuator{
Expand Down Expand Up @@ -339,16 +354,16 @@ func (a *Actuator) CreateMachine(cluster *clusterv1.Cluster, machine *clusterv1.
return nil, err
}
// Add tags to the created machine
tagList := []*ec2.Tag{}
rawTagList := []*ec2.Tag{}
for _, tag := range machineProviderConfig.Tags {
tagList = append(tagList, &ec2.Tag{Key: aws.String(tag.Name), Value: aws.String(tag.Value)})
rawTagList = append(rawTagList, &ec2.Tag{Key: aws.String(tag.Name), Value: aws.String(tag.Value)})
}
tagList = append(tagList, []*ec2.Tag{
rawTagList = append(rawTagList, []*ec2.Tag{
Copy link
Member

@ingvagabund ingvagabund Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we just put all the tags under a map and then check if the key already exists?

tagMap := make(map[string]struct{})
for _, tag := range machineProviderConfig.Tags {
 		tagMap[tag.Name] = struct{}{}
}
if _, exists := tagMap["clusterid"]; !exists {
 		tagList = append(tagList, {Key: aws.String("clusterid"), Value: aws.String(clusterID)}}
}
...

Copy link
Member

@enxebre enxebre Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

personally I like current implementation. It prevents also from injecting duplications @frobware wdyt?

Copy link

@frobware frobware Oct 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kind of prefer the de-dupe phase. It's easier to rationalise. I have a bunch of tags, then a last action is to de-dupe them.

{Key: aws.String("clusterid"), Value: aws.String(clusterID)},
{Key: aws.String("kubernetes.io/cluster/" + clusterID), Value: aws.String("owned")},
{Key: aws.String("Name"), Value: aws.String(machine.Name)},
}...)

tagList := removeDuplicatedTags(rawTagList)
tagInstance := &ec2.TagSpecification{
ResourceType: aws.String("instance"),
Tags: tagList,
Expand Down
47 changes: 47 additions & 0 deletions pkg/cloud/aws/actuators/machine/actuator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"errors"
"fmt"
"reflect"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -292,3 +293,49 @@ func mockTerminateInstances(mockAWSClient *mockaws.MockClient) {
mockAWSClient.EXPECT().TerminateInstances(gomock.Any()).Return(
&ec2.TerminateInstancesOutput{}, nil)
}

func TestRemoveDuplicatedTags(t *testing.T) {
cases := []struct {
tagList []*ec2.Tag
expected []*ec2.Tag
}{
{
// empty tags
tagList: []*ec2.Tag{},
expected: []*ec2.Tag{},
},
{
// no duplicate tags
tagList: []*ec2.Tag{
Copy link

@frobware frobware Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the empty case (i.e., empty slices).

{Key: aws.String("clusterID"), Value: aws.String("test-ClusterIDValue")},
{Key: aws.String("clusterName"), Value: aws.String("test-ClusterNameValue")},
},
expected: []*ec2.Tag{
{Key: aws.String("clusterID"), Value: aws.String("test-ClusterIDValue")},
{Key: aws.String("clusterName"), Value: aws.String("test-ClusterNameValue")},
},
},
{
// multiple duplicate tags
tagList: []*ec2.Tag{
{Key: aws.String("clusterID"), Value: aws.String("test-ClusterIDValue")},
{Key: aws.String("clusterName"), Value: aws.String("test-ClusterNameValue")},
{Key: aws.String("clusterSize"), Value: aws.String("test-ClusterSizeValue")},
{Key: aws.String("clusterName"), Value: aws.String("test-ClusterNameDuplicatedValue")},
{Key: aws.String("clusterSize"), Value: aws.String("test-ClusterSizeDuplicatedValue")},
},
expected: []*ec2.Tag{
{Key: aws.String("clusterID"), Value: aws.String("test-ClusterIDValue")},
{Key: aws.String("clusterName"), Value: aws.String("test-ClusterNameValue")},
{Key: aws.String("clusterSize"), Value: aws.String("test-ClusterSizeValue")},
},
},
}

for i, c := range cases {
actual := removeDuplicatedTags(c.tagList)
if !reflect.DeepEqual(c.expected, actual) {
t.Errorf("test #%d: expected %+v, got %+v", i, c.expected, actual)
}
}
}