Skip to content
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

Jump to bottom

service/ec2: Refactor Security Group data sources and resources to use keyvaluetags package #11918

Merged
merged 3 commits into from
Mar 6, 2020
Merged

service/ec2: Refactor Security Group data sources and resources to use keyvaluetags package #11918

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d73726e
service/ec2: Refactor Security Group data sources and resources to us…
bflad Feb 5, 2020
81dc92e
Merge branch 'master' into td-ec2-security-groups-keyvaluetags
bflad Mar 6, 2020
ddde357
service/ec2: Remove now unused setTags and diffTags functions
bflad Mar 6, 2020
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
9 changes: 7 additions & 2 deletions aws/data_source_aws_security_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/aws/aws-sdk-go/aws/arn"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
)

func dataSourceAwsSecurityGroup() *schema.Resource {
Expand Down Expand Up @@ -63,7 +64,7 @@ func dataSourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) er
},
)
req.Filters = append(req.Filters, buildEC2TagFilterList(
tagsFromMap(d.Get("tags").(map[string]interface{})),
keyvaluetags.New(d.Get("tags").(map[string]interface{})).Ec2Tags(),
)...)
req.Filters = append(req.Filters, buildEC2CustomFilterList(
d.Get("filter").(*schema.Set),
Expand Down Expand Up @@ -91,7 +92,11 @@ func dataSourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) er
d.Set("name", sg.GroupName)
d.Set("description", sg.Description)
d.Set("vpc_id", sg.VpcId)
d.Set("tags", tagsToMap(sg.Tags))

if err := d.Set("tags", keyvaluetags.Ec2KeyValueTags(sg.Tags).IgnoreAws().Map()); err != nil {
return fmt.Errorf("error setting tags: %s", err)
}

arn := arn.ARN{
Partition: meta.(*AWSClient).partition,
Service: "ec2",
Expand Down
3 changes: 2 additions & 1 deletion aws/data_source_aws_security_groups.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
)

func dataSourceAwsSecurityGroups() *schema.Resource {
Expand Down Expand Up @@ -49,7 +50,7 @@ func dataSourceAwsSecurityGroupsRead(d *schema.ResourceData, meta interface{}) e
}
if tagsOk {
req.Filters = append(req.Filters, buildEC2TagFilterList(
tagsFromMap(tags.(map[string]interface{})),
keyvaluetags.New(tags.(map[string]interface{})).Ec2Tags(),
)...)
}

Expand Down
7 changes: 5 additions & 2 deletions aws/resource_aws_default_security_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
)

func resourceAwsDefaultSecurityGroup() *schema.Resource {
Expand Down Expand Up @@ -90,8 +91,10 @@ func resourceAwsDefaultSecurityGroupCreate(d *schema.ResourceData, meta interfac

log.Printf("[INFO] Default Security Group ID: %s", d.Id())

if err := setTags(conn, d); err != nil {
return err
if v := d.Get("tags").(map[string]interface{}); len(v) > 0 {
if err := keyvaluetags.Ec2UpdateTags(conn, d.Id(), nil, v); err != nil {
return fmt.Errorf("error adding EC2 Default Security Group (%s) tags: %s", d.Id(), err)
}
}

if err := revokeDefaultSecurityGroupRules(meta, g); err != nil {
Expand Down
21 changes: 14 additions & 7 deletions aws/resource_aws_security_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/naming"
)

Expand Down Expand Up @@ -268,8 +269,10 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er
d.Id(), err)
}

if err := setTags(conn, d); err != nil {
return err
if v := d.Get("tags").(map[string]interface{}); len(v) > 0 {
if err := keyvaluetags.Ec2UpdateTags(conn, d.Id(), nil, v); err != nil {
return fmt.Errorf("error adding EC2 Security Group (%s) tags: %s", d.Id(), err)
}
}

// AWS defaults all Security Groups to have an ALLOW ALL egress rule. Here we
Expand Down Expand Up @@ -389,7 +392,10 @@ func resourceAwsSecurityGroupRead(d *schema.ResourceData, meta interface{}) erro
log.Printf("[WARN] Error setting Egress rule set for (%s): %s", d.Id(), err)
}

d.Set("tags", tagsToMap(sg.Tags))
if err := d.Set("tags", keyvaluetags.Ec2KeyValueTags(sg.Tags).IgnoreAws().Map()); err != nil {
return fmt.Errorf("error setting tags: %s", err)
}

return nil
}

Expand Down Expand Up @@ -427,11 +433,12 @@ func resourceAwsSecurityGroupUpdate(d *schema.ResourceData, meta interface{}) er
}
}

if !d.IsNewResource() {
if err := setTags(conn, d); err != nil {
return err
if d.HasChange("tags") && !d.IsNewResource() {
o, n := d.GetChange("tags")

if err := keyvaluetags.Ec2UpdateTags(conn, d.Id(), o, n); err != nil {
return fmt.Errorf("error updating EC2 Security Group (%s) tags: %s", d.Id(), err)
}
d.SetPartial("tags")
}

return resourceAwsSecurityGroupRead(d, meta)
Expand Down
108 changes: 0 additions & 108 deletions aws/tags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,9 @@ package aws
import (
"log"
"regexp"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws"
"github.com/aws/aws-sdk-go/aws/awserr"
"github.com/aws/aws-sdk-go/service/ec2"
"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
)
Expand Down Expand Up @@ -39,110 +35,6 @@ func tagsSchemaForceNew() *schema.Schema {
}
}

// setTags is a helper to set the tags for a resource. It expects the
// tags field to be named "tags"
func setTags(conn *ec2.EC2, d *schema.ResourceData) error {
if d.HasChange("tags") {
oraw, nraw := d.GetChange("tags")
o := oraw.(map[string]interface{})
n := nraw.(map[string]interface{})
create, remove := diffTags(tagsFromMap(o), tagsFromMap(n))

// Set tags
if len(remove) > 0 {
err := resource.Retry(5*time.Minute, func() *resource.RetryError {
log.Printf("[DEBUG] Removing tags: %#v from %s", remove, d.Id())
_, err := conn.DeleteTags(&ec2.DeleteTagsInput{
Resources: []*string{aws.String(d.Id())},
Tags: remove,
})
if err != nil {
ec2err, ok := err.(awserr.Error)
if ok && strings.Contains(ec2err.Code(), ".NotFound") {
return resource.RetryableError(err) // retry
}
return resource.NonRetryableError(err)
}
return nil
})
if err != nil {
// Retry without time bounds for EC2 throttling
if isResourceTimeoutError(err) {
log.Printf("[DEBUG] Removing tags: %#v from %s", remove, d.Id())
_, err := conn.DeleteTags(&ec2.DeleteTagsInput{
Resources: []*string{aws.String(d.Id())},
Tags: remove,
})
if err != nil {
return err
}
} else {
return err
}
}
}
if len(create) > 0 {
err := resource.Retry(5*time.Minute, func() *resource.RetryError {
log.Printf("[DEBUG] Creating tags: %s for %s", create, d.Id())
_, err := conn.CreateTags(&ec2.CreateTagsInput{
Resources: []*string{aws.String(d.Id())},
Tags: create,
})
if err != nil {
ec2err, ok := err.(awserr.Error)
if ok && strings.Contains(ec2err.Code(), ".NotFound") {
return resource.RetryableError(err) // retry
}
return resource.NonRetryableError(err)
}
return nil
})
if err != nil {
// Retry without time bounds for EC2 throttling
if isResourceTimeoutError(err) {
log.Printf("[DEBUG] Creating tags: %s for %s", create, d.Id())
_, err := conn.CreateTags(&ec2.CreateTagsInput{
Resources: []*string{aws.String(d.Id())},
Tags: create,
})
if err != nil {
return err
}
} else {
return err
}
}
}
}

return nil
}

// diffTags takes our tags locally and the ones remotely and returns
// the set of tags that must be created, and the set of tags that must
// be destroyed.
func diffTags(oldTags, newTags []*ec2.Tag) ([]*ec2.Tag, []*ec2.Tag) {
// First, we're creating everything we have
create := make(map[string]interface{})
for _, t := range newTags {
create[aws.StringValue(t.Key)] = aws.StringValue(t.Value)
}

// Build the list of what to remove
var remove []*ec2.Tag
for _, t := range oldTags {
old, ok := create[aws.StringValue(t.Key)]
if !ok || old != aws.StringValue(t.Value) {
remove = append(remove, t)
} else if ok {
// already present so remove from new
delete(create, aws.StringValue(t.Key))
}
}

return tagsFromMap(create), remove
}

// tagsFromMap returns the tags for the given map of data.
func tagsFromMap(m map[string]interface{}) []*ec2.Tag {
result := make([]*ec2.Tag, 0, len(m))
Expand Down
84 changes: 0 additions & 84 deletions aws/tags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package aws

import (
"fmt"
"reflect"
"testing"

"github.com/aws/aws-sdk-go/aws"
Expand All @@ -11,89 +10,6 @@ import (
"github.com/hashicorp/terraform-plugin-sdk/terraform"
)

func TestDiffTags(t *testing.T) {
cases := []struct {
Old, New map[string]interface{}
Create, Remove map[string]string
}{
// Add
{
Old: map[string]interface{}{
"foo": "bar",
},
New: map[string]interface{}{
"foo": "bar",
"bar": "baz",
},
Create: map[string]string{
"bar": "baz",
},
Remove: map[string]string{},
},

// Modify
{
Old: map[string]interface{}{
"foo": "bar",
},
New: map[string]interface{}{
"foo": "baz",
},
Create: map[string]string{
"foo": "baz",
},
Remove: map[string]string{
"foo": "bar",
},
},

// Overlap
{
Old: map[string]interface{}{
"foo": "bar",
"hello": "world",
},
New: map[string]interface{}{
"foo": "baz",
"hello": "world",
},
Create: map[string]string{
"foo": "baz",
},
Remove: map[string]string{
"foo": "bar",
},
},

// Remove
{
Old: map[string]interface{}{
"foo": "bar",
"bar": "baz",
},
New: map[string]interface{}{
"foo": "bar",
},
Create: map[string]string{},
Remove: map[string]string{
"bar": "baz",
},
},
}

for i, tc := range cases {
c, r := diffTags(tagsFromMap(tc.Old), tagsFromMap(tc.New))
cm := tagsToMap(c)
rm := tagsToMap(r)
if !reflect.DeepEqual(cm, tc.Create) {
t.Fatalf("%d: bad create: %#v", i, cm)
}
if !reflect.DeepEqual(rm, tc.Remove) {
t.Fatalf("%d: bad remove: %#v", i, rm)
}
}
}

func TestIgnoringTags(t *testing.T) {
var ignoredTags []*ec2.Tag
ignoredTags = append(ignoredTags, &ec2.Tag{
Expand Down