Skip to content

Commit

Permalink
service/ec2: Refactor Security Group data sources and resources to us…
Browse files Browse the repository at this point in the history 
…e keyvaluetags package (#11918)

Reference: #10688

Output from acceptance testing:

```
--- PASS: TestAccDataSourceAwsSecurityGroup_basic (37.66s)

--- PASS: TestAccDataSourceAwsSecurityGroups_tag (37.81s)
--- PASS: TestAccDataSourceAwsSecurityGroups_filter (38.43s)

--- PASS: TestAccAWSDefaultSecurityGroup_basic (33.65s)
--- PASS: TestAccAWSDefaultSecurityGroup_classic (10.22s)

--- PASS: TestAccAWSSecurityGroup_allowAll (64.34s)
--- PASS: TestAccAWSSecurityGroup_basic (33.11s)
--- PASS: TestAccAWSSecurityGroup_change (73.42s)
--- PASS: TestAccAWSSecurityGroup_CIDRandGroups (59.48s)
--- PASS: TestAccAWSSecurityGroup_defaultEgressClassic (70.38s)
--- PASS: TestAccAWSSecurityGroup_defaultEgressVPC (43.71s)
--- PASS: TestAccAWSSecurityGroup_drift (20.32s)
--- PASS: TestAccAWSSecurityGroup_driftComplex (38.09s)
--- PASS: TestAccAWSSecurityGroup_egressConfigMode (77.34s)
--- PASS: TestAccAWSSecurityGroup_egressWithPrefixList (62.50s)
--- PASS: TestAccAWSSecurityGroup_failWithDiffMismatch (49.74s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRulesFalse (690.01s)
--- PASS: TestAccAWSSecurityGroup_forceRevokeRulesTrue (732.92s)
--- PASS: TestAccAWSSecurityGroup_generatedName (40.25s)
--- PASS: TestAccAWSSecurityGroup_ingressConfigMode (57.14s)
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGsClassic (27.95s)
--- PASS: TestAccAWSSecurityGroup_ingressWithCidrAndSGsVPC (45.17s)
--- PASS: TestAccAWSSecurityGroup_ingressWithPrefixList (51.39s)
--- PASS: TestAccAWSSecurityGroup_invalidCIDRBlock (2.23s)
--- PASS: TestAccAWSSecurityGroup_IPRangeAndSecurityGroupWithSameRules (43.29s)
--- PASS: TestAccAWSSecurityGroup_IPRangesWithSameRules (45.55s)
--- PASS: TestAccAWSSecurityGroup_ipv4andipv6Egress (41.18s)
--- PASS: TestAccAWSSecurityGroup_ipv6 (34.86s)
--- PASS: TestAccAWSSecurityGroup_multiIngress (53.10s)
--- PASS: TestAccAWSSecurityGroup_namePrefix (53.10s)
--- PASS: TestAccAWSSecurityGroup_ruleDescription (104.82s)
--- PASS: TestAccAWSSecurityGroup_ruleGathering (65.55s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitCidrBlockExceededAppend (75.12s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAllNew (83.09s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededAppend (73.30s)
--- PASS: TestAccAWSSecurityGroup_ruleLimitExceededPrepend (74.60s)
--- PASS: TestAccAWSSecurityGroup_rulesDropOnError (73.15s)
--- PASS: TestAccAWSSecurityGroup_self (39.02s)
--- PASS: TestAccAWSSecurityGroup_sourceSecurityGroup (40.50s)
--- PASS: TestAccAWSSecurityGroup_tags (83.69s)
--- PASS: TestAccAWSSecurityGroup_vpc (53.47s)
--- PASS: TestAccAWSSecurityGroup_vpcNegOneIngress (36.25s)
--- PASS: TestAccAWSSecurityGroup_vpcProtoNumIngress (66.03s)
```
  • Loading branch information
@bflad
bflad authored Mar 6, 2020
1 parent e993cdc commit c4f54b2
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 204 deletions.
9 changes: 7 additions & 2 deletions aws/data_source_aws_security_group.go
View file
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
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
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
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
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
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

0 comments on commit c4f54b2

Please sign in to comment.