From 2e42f0aeed63a5caf38d76ceeca18b296c9451e4 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Sat, 26 Jan 2019 18:26:04 -0500 Subject: [PATCH 1/2] Include any system tags that Terraform ignores when setting S3 bucket tags. --- aws/resource_aws_cloudformation_stack.go | 74 +++++++ aws/resource_aws_s3_bucket.go | 6 +- aws/resource_aws_s3_bucket_test.go | 243 +++++++++++++++++++++++ aws/s3_tags.go | 119 +++++------ aws/s3_tags_test.go | 52 ----- 5 files changed, 370 insertions(+), 124 deletions(-) diff --git a/aws/resource_aws_cloudformation_stack.go b/aws/resource_aws_cloudformation_stack.go index 50fb922575b..3b45258762f 100644 --- a/aws/resource_aws_cloudformation_stack.go +++ b/aws/resource_aws_cloudformation_stack.go @@ -648,3 +648,77 @@ func cfStackEventIsStackDeletion(event *cloudformation.StackEvent) bool { *event.ResourceType == "AWS::CloudFormation::Stack" && event.ResourceStatusReason != nil } + +func cfStackStateRefresh(conn *cloudformation.CloudFormation, stackId string) resource.StateRefreshFunc { + return func() (interface{}, string, error) { + resp, err := conn.DescribeStacks(&cloudformation.DescribeStacksInput{ + StackName: aws.String(stackId), + }) + if err != nil { + return nil, "", fmt.Errorf("error describing CloudFormation stacks: %s", err) + } + + n := len(resp.Stacks) + switch n { + case 0: + return "", cloudformation.StackStatusDeleteComplete, nil + + case 1: + stack := resp.Stacks[0] + return stack, aws.StringValue(stack.StackStatus), nil + + default: + return nil, "", fmt.Errorf("found %d CloudFormation stacks for %s, expected 1", n, stackId) + } + } +} + +func waitForCloudFormationStackCreation(conn *cloudformation.CloudFormation, stackId string, timeout time.Duration) (string, error) { + stateConf := &resource.StateChangeConf{ + Pending: []string{ + cloudformation.StackStatusCreateInProgress, + cloudformation.StackStatusDeleteInProgress, + cloudformation.StackStatusRollbackInProgress, + }, + Target: []string{ + cloudformation.StackStatusCreateComplete, + cloudformation.StackStatusCreateFailed, + cloudformation.StackStatusDeleteComplete, + cloudformation.StackStatusDeleteFailed, + cloudformation.StackStatusRollbackComplete, + cloudformation.StackStatusRollbackFailed, + }, + Refresh: cfStackStateRefresh(conn, stackId), + Timeout: timeout, + Delay: 10 * time.Second, + MinTimeout: 5 * time.Second, + } + + v, err := stateConf.WaitForState() + if err != nil { + return "", err + } + + return aws.StringValue(v.(*cloudformation.Stack).StackStatus), nil +} + +func waitForCloudFormationStackDeletion(conn *cloudformation.CloudFormation, stackId string, timeout time.Duration) error { + stateConf := &resource.StateChangeConf{ + Pending: []string{ + cloudformation.StackStatusDeleteInProgress, + cloudformation.StackStatusRollbackInProgress, + }, + Target: []string{ + cloudformation.StackStatusDeleteComplete, + cloudformation.StackStatusDeleteFailed, + }, + Refresh: cfStackStateRefresh(conn, stackId), + Timeout: timeout, + Delay: 10 * time.Second, + MinTimeout: 5 * time.Second, + } + + _, err := stateConf.WaitForState() + + return err +} diff --git a/aws/resource_aws_s3_bucket.go b/aws/resource_aws_s3_bucket.go index b554d3d1b34..8befa7e6a11 100644 --- a/aws/resource_aws_s3_bucket.go +++ b/aws/resource_aws_s3_bucket.go @@ -675,7 +675,7 @@ func resourceAwsS3BucketCreate(d *schema.ResourceData, meta interface{}) error { func resourceAwsS3BucketUpdate(d *schema.ResourceData, meta interface{}) error { s3conn := meta.(*AWSClient).s3conn - if err := setTagsS3(s3conn, d); err != nil { + if err := setTagsS3Bucket(s3conn, d); err != nil { return fmt.Errorf("%q: %s", d.Get("bucket").(string), err) } @@ -1222,9 +1222,9 @@ func resourceAwsS3BucketRead(d *schema.ResourceData, meta interface{}) error { } } - tagSet, err := getTagSetS3(s3conn, d.Id()) + tagSet, err := getTagSetS3Bucket(s3conn, d.Id()) if err != nil { - return err + return fmt.Errorf("error getting S3 bucket tags: %s", err) } if err := d.Set("tags", tagsToMapS3(tagSet)); err != nil { diff --git a/aws/resource_aws_s3_bucket_test.go b/aws/resource_aws_s3_bucket_test.go index 4c75bee6c5a..de7517d1c93 100644 --- a/aws/resource_aws_s3_bucket_test.go +++ b/aws/resource_aws_s3_bucket_test.go @@ -14,6 +14,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" + "github.com/aws/aws-sdk-go/service/cloudformation" "github.com/aws/aws-sdk-go/service/s3" "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" @@ -238,6 +239,136 @@ func TestAccAWSS3Bucket_Bucket_EmptyString(t *testing.T) { }) } +func TestAccAWSS3Bucket_tagsWithNoSystemTags(t *testing.T) { + resourceName := "aws_s3_bucket.bucket" + rInt := acctest.RandInt() + bucketName := fmt.Sprintf("tf-test-bucket-%d", rInt) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSS3BucketDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSS3BucketConfig_withTags(bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSS3BucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "tags.%", "3"), + resource.TestCheckResourceAttr(resourceName, "tags.Key1", "AAA"), + resource.TestCheckResourceAttr(resourceName, "tags.Key2", "BBB"), + resource.TestCheckResourceAttr(resourceName, "tags.Key3", "CCC"), + ), + }, + { + Config: testAccAWSS3BucketConfig_withUpdatedTags(bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSS3BucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "tags.%", "4"), + resource.TestCheckResourceAttr(resourceName, "tags.Key2", "BBB"), + resource.TestCheckResourceAttr(resourceName, "tags.Key3", "XXX"), + resource.TestCheckResourceAttr(resourceName, "tags.Key4", "DDD"), + resource.TestCheckResourceAttr(resourceName, "tags.Key5", "EEE"), + ), + }, + { + + Config: testAccAWSS3BucketConfig_withNoTags(bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSS3BucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + ), + }, + { + Config: testAccAWSS3BucketConfig_withTags(bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSS3BucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "tags.%", "3"), + resource.TestCheckResourceAttr(resourceName, "tags.Key1", "AAA"), + resource.TestCheckResourceAttr(resourceName, "tags.Key2", "BBB"), + resource.TestCheckResourceAttr(resourceName, "tags.Key3", "CCC"), + ), + }, + }, + }) +} + +func TestAccAWSS3Bucket_tagsWithSystemTags(t *testing.T) { + resourceName := "aws_s3_bucket.bucket" + rInt := acctest.RandInt() + bucketName := fmt.Sprintf("tf-test-bucket-%d", rInt) + var stackId string + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: resource.ComposeAggregateTestCheckFunc( + testAccCheckAWSS3BucketDestroy, + func(s *terraform.State) error { + // Tear down CF stack. + conn := testAccProvider.Meta().(*AWSClient).cfconn + + req := &cloudformation.DeleteStackInput{ + StackName: aws.String(stackId), + } + + log.Printf("[DEBUG] Deleting CloudFormation stack: %#v", req) + if _, err := conn.DeleteStack(req); err != nil { + return fmt.Errorf("Error deleting CloudFormation stack: %s", err) + } + + if err := waitForCloudFormationStackDeletion(conn, stackId, 10*time.Minute); err != nil { + return fmt.Errorf("Error waiting for CloudFormation stack deletion: %s", err) + } + + return nil + }, + ), + Steps: []resource.TestStep{ + { + Config: testAccAWSS3BucketConfig_withNoTags(bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSS3BucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + testAccCheckAWSS3DestroyBucket(resourceName), + testAccCheckAWSS3BucketCreateViaCloudFormation(bucketName, &stackId), + ), + }, + { + Config: testAccAWSS3BucketConfig_withTags(bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSS3BucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "tags.%", "3"), + resource.TestCheckResourceAttr(resourceName, "tags.Key1", "AAA"), + resource.TestCheckResourceAttr(resourceName, "tags.Key2", "BBB"), + resource.TestCheckResourceAttr(resourceName, "tags.Key3", "CCC"), + testAccCheckAWSS3BucketTagKeys(resourceName, "aws:cloudformation:stack-name", "aws:cloudformation:stack-id", "aws:cloudformation:logical-id"), + ), + }, + { + Config: testAccAWSS3BucketConfig_withUpdatedTags(bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSS3BucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "tags.%", "4"), + resource.TestCheckResourceAttr(resourceName, "tags.Key2", "BBB"), + resource.TestCheckResourceAttr(resourceName, "tags.Key3", "XXX"), + resource.TestCheckResourceAttr(resourceName, "tags.Key4", "DDD"), + resource.TestCheckResourceAttr(resourceName, "tags.Key5", "EEE"), + testAccCheckAWSS3BucketTagKeys(resourceName, "aws:cloudformation:stack-name", "aws:cloudformation:stack-id", "aws:cloudformation:logical-id"), + ), + }, + { + + Config: testAccAWSS3BucketConfig_withNoTags(bucketName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSS3BucketExists(resourceName), + resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), + testAccCheckAWSS3BucketTagKeys(resourceName, "aws:cloudformation:stack-name", "aws:cloudformation:stack-id", "aws:cloudformation:logical-id"), + ), + }, + }, + }) +} + func TestAccAWSS3MultiBucket_withTags(t *testing.T) { rInt := acctest.RandInt() resource.ParallelTest(t, resource.TestCase{ @@ -1739,6 +1870,77 @@ func testAccCheckAWSS3DestroyBucket(n string) resource.TestCheckFunc { } } +// Create an S3 bucket via a CF stack so that it has system tags. +func testAccCheckAWSS3BucketCreateViaCloudFormation(n string, stackId *string) resource.TestCheckFunc { + return func(s *terraform.State) error { + conn := testAccProvider.Meta().(*AWSClient).cfconn + stackName := fmt.Sprintf("tf-acc-test-s3tags-%d", acctest.RandInt()) + templateBody := fmt.Sprintf(` + { + "Resources": { + "TfTestBucket": { + "Type": "AWS::S3::Bucket", + "Properties": { + "BucketName": "%s", + } + } + } + } + `, n) + + req := &cloudformation.CreateStackInput{ + StackName: aws.String(stackName), + TemplateBody: aws.String(templateBody), + } + + log.Printf("[DEBUG] Creating CloudFormation stack: %#v", req) + resp, err := conn.CreateStack(req) + if err != nil { + return fmt.Errorf("Error creating CloudFormation stack: %s", err) + } + + status, err := waitForCloudFormationStackCreation(conn, aws.StringValue(resp.StackId), 10*time.Minute) + if err != nil { + return fmt.Errorf("Error waiting for CloudFormation stack creation: %s", err) + } + if status != cloudformation.StackStatusCreateComplete { + return fmt.Errorf("Invalid CloudFormation stack creation status: %s", status) + } + + *stackId = aws.StringValue(resp.StackId) + return nil + } +} + +func testAccCheckAWSS3BucketTagKeys(n string, keys ...string) resource.TestCheckFunc { + return func(s *terraform.State) error { + rs := s.RootModule().Resources[n] + conn := testAccProvider.Meta().(*AWSClient).s3conn + + resp, err := conn.GetBucketTagging(&s3.GetBucketTaggingInput{ + Bucket: aws.String(rs.Primary.ID), + }) + if err != nil { + return err + } + + for _, key := range keys { + ok := false + for _, tag := range resp.TagSet { + if key == aws.StringValue(tag.Key) { + ok = true + break + } + } + if !ok { + return fmt.Errorf("Key %s not found in bucket's tag set", key) + } + } + + return nil + } +} + func testAccCheckAWSS3BucketPolicy(n string, policy string) resource.TestCheckFunc { return func(s *terraform.State) error { rs := s.RootModule().Resources[n] @@ -2094,6 +2296,47 @@ resource "aws_s3_bucket" "bucket" { `, randInt) } +func testAccAWSS3BucketConfig_withNoTags(bucketName string) string { + return fmt.Sprintf(` +resource "aws_s3_bucket" "bucket" { + bucket = %[1]q + acl = "private" + force_destroy = false +} +`, bucketName) +} + +func testAccAWSS3BucketConfig_withTags(bucketName string) string { + return fmt.Sprintf(` +resource "aws_s3_bucket" "bucket" { + bucket = %[1]q + acl = "private" + force_destroy = false + tags = { + Key1 = "AAA" + Key2 = "BBB" + Key3 = "CCC" + } +} +`, bucketName) +} + +func testAccAWSS3BucketConfig_withUpdatedTags(bucketName string) string { + return fmt.Sprintf(` +resource "aws_s3_bucket" "bucket" { + bucket = %[1]q + acl = "private" + force_destroy = false + tags = { + Key2 = "BBB" + Key3 = "XXX" + Key4 = "DDD" + Key5 = "EEE" + } +} +`, bucketName) +} + func testAccAWSS3MultiBucketConfigWithTags(randInt int) string { return fmt.Sprintf(` resource "aws_s3_bucket" "bucket1" { diff --git a/aws/s3_tags.go b/aws/s3_tags.go index 42fc6b8c0e3..1ef566086a3 100644 --- a/aws/s3_tags.go +++ b/aws/s3_tags.go @@ -1,50 +1,75 @@ package aws import ( + "fmt" "log" "regexp" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/s3" "github.com/hashicorp/terraform/helper/schema" ) +// return a slice of s3 tags associated with the given s3 bucket. Essentially +// s3.GetBucketTagging, except returns an empty slice instead of an error when +// there are no tags. +func getTagSetS3Bucket(conn *s3.S3, bucket string) ([]*s3.Tag, error) { + resp, err := conn.GetBucketTagging(&s3.GetBucketTaggingInput{ + Bucket: aws.String(bucket), + }) + if err != nil { + if isAWSErr(err, "NoSuchTagSet", "") { + return nil, nil + } + return nil, err + } + + return resp.TagSet, nil +} + // setTags is a helper to set the tags for a resource. It expects the // tags field to be named "tags" -func setTagsS3(conn *s3.S3, d *schema.ResourceData) error { +func setTagsS3Bucket(conn *s3.S3, 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 := diffTagsS3(tagsFromMapS3(o), tagsFromMapS3(n)) - // Set tags - if len(remove) > 0 { - log.Printf("[DEBUG] Removing tags: %#v", remove) - _, err := RetryOnAwsCodes([]string{"NoSuchBucket", "OperationAborted"}, func() (interface{}, error) { - return conn.DeleteBucketTagging(&s3.DeleteBucketTaggingInput{ - Bucket: aws.String(d.Get("bucket").(string)), - }) - }) - if err != nil { - return err + // Get any existing system tags. + var sysTagSet []*s3.Tag + oTagSet, err := getTagSetS3Bucket(conn, d.Get("bucket").(string)) + if err != nil { + return fmt.Errorf("error getting S3 bucket tags: %s", err) + } + for _, tag := range oTagSet { + if tagIgnoredS3(tag) { + sysTagSet = append(sysTagSet, tag) } } - if len(create) > 0 { - log.Printf("[DEBUG] Creating tags: %#v", create) + + if len(n)+len(sysTagSet) > 0 { + // The bucket's tag set must include any system tags that Terraform ignores. + nTagSet := append(tagsFromMapS3(n), sysTagSet...) + req := &s3.PutBucketTaggingInput{ Bucket: aws.String(d.Get("bucket").(string)), Tagging: &s3.Tagging{ - TagSet: create, + TagSet: nTagSet, }, } - - _, err := RetryOnAwsCodes([]string{"NoSuchBucket", "OperationAborted"}, func() (interface{}, error) { + if _, err := RetryOnAwsCodes([]string{"NoSuchBucket", "OperationAborted"}, func() (interface{}, error) { return conn.PutBucketTagging(req) - }) - if err != nil { - return err + }); err != nil { + return fmt.Errorf("error setting S3 bucket tags: %s", err) + } + } else if len(o) > 0 && len(sysTagSet) == 0 { + req := &s3.DeleteBucketTaggingInput{ + Bucket: aws.String(d.Get("bucket").(string)), + } + if _, err := RetryOnAwsCodes([]string{"NoSuchBucket", "OperationAborted"}, func() (interface{}, error) { + return conn.DeleteBucketTagging(req) + }); err != nil { + return fmt.Errorf("error deleting S3 bucket tags: %s", err) } } } @@ -78,23 +103,21 @@ func setTagsS3Object(conn *s3.S3, d *schema.ResourceData) error { // Set tags if len(o) > 0 { - _, err := conn.DeleteObjectTagging(&s3.DeleteObjectTaggingInput{ + if _, err := conn.DeleteObjectTagging(&s3.DeleteObjectTaggingInput{ Bucket: aws.String(d.Get("bucket").(string)), Key: aws.String(d.Get("key").(string)), - }) - if err != nil { + }); err != nil { return err } } if len(n) > 0 { - _, err := conn.PutObjectTagging(&s3.PutObjectTaggingInput{ + if _, err := conn.PutObjectTagging(&s3.PutObjectTaggingInput{ Bucket: aws.String(d.Get("bucket").(string)), Key: aws.String(d.Get("key").(string)), Tagging: &s3.Tagging{ TagSet: tagsFromMapS3(n), }, - }) - if err != nil { + }); err != nil { return err } } @@ -103,29 +126,6 @@ func setTagsS3Object(conn *s3.S3, d *schema.ResourceData) error { 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 diffTagsS3(oldTags, newTags []*s3.Tag) ([]*s3.Tag, []*s3.Tag) { - // First, we're creating everything we have - create := make(map[string]interface{}) - for _, t := range newTags { - create[*t.Key] = *t.Value - } - - // Build the list of what to remove - var remove []*s3.Tag - for _, t := range oldTags { - old, ok := create[*t.Key] - if !ok || old != *t.Value { - // Delete it! - remove = append(remove, t) - } - } - - return tagsFromMapS3(create), remove -} - // tagsFromMap returns the tags for the given map of data. func tagsFromMapS3(m map[string]interface{}) []*s3.Tag { result := make([]*s3.Tag, 0, len(m)) @@ -154,25 +154,6 @@ func tagsToMapS3(ts []*s3.Tag) map[string]string { return result } -// return a slice of s3 tags associated with the given s3 bucket. Essentially -// s3.GetBucketTagging, except returns an empty slice instead of an error when -// there are no tags. -func getTagSetS3(s3conn *s3.S3, bucket string) ([]*s3.Tag, error) { - request := &s3.GetBucketTaggingInput{ - Bucket: aws.String(bucket), - } - - response, err := s3conn.GetBucketTagging(request) - if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "NoSuchTagSet" { - // There is no tag set associated with the bucket. - return []*s3.Tag{}, nil - } else if err != nil { - return nil, err - } - - return response.TagSet, nil -} - // compare a tag against a list of strings and checks if it should // be ignored or not func tagIgnoredS3(t *s3.Tag) bool { diff --git a/aws/s3_tags_test.go b/aws/s3_tags_test.go index 42d5b605d04..ec089894bae 100644 --- a/aws/s3_tags_test.go +++ b/aws/s3_tags_test.go @@ -1,64 +1,12 @@ package aws import ( - "reflect" "testing" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/s3" ) -func TestDiffTagsS3(t *testing.T) { - cases := []struct { - Old, New map[string]interface{} - Create, Remove map[string]string - }{ - // Basic add/remove - { - Old: map[string]interface{}{ - "foo": "bar", - }, - New: map[string]interface{}{ - "bar": "baz", - }, - Create: map[string]string{ - "bar": "baz", - }, - Remove: map[string]string{ - "foo": "bar", - }, - }, - - // 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", - }, - }, - } - - for i, tc := range cases { - c, r := diffTagsS3(tagsFromMapS3(tc.Old), tagsFromMapS3(tc.New)) - cm := tagsToMapS3(c) - rm := tagsToMapS3(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 TestIgnoringTagsS3(t *testing.T) { var ignoredTags []*s3.Tag ignoredTags = append(ignoredTags, &s3.Tag{ From d994ad32758933aa72d724474f3a1f213e0563d8 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Thu, 29 Aug 2019 08:03:45 -0400 Subject: [PATCH 2/2] Review: Add comment to clarify acceptance test case. --- aws/resource_aws_s3_bucket_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/aws/resource_aws_s3_bucket_test.go b/aws/resource_aws_s3_bucket_test.go index de7517d1c93..58fe597692e 100644 --- a/aws/resource_aws_s3_bucket_test.go +++ b/aws/resource_aws_s3_bucket_test.go @@ -278,6 +278,7 @@ func TestAccAWSS3Bucket_tagsWithNoSystemTags(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "tags.%", "0"), ), }, + // Verify update from 0 tags. { Config: testAccAWSS3BucketConfig_withTags(bucketName), Check: resource.ComposeTestCheckFunc(