From 4acb8988a74c281d2261284e2f2583222fdfeff1 Mon Sep 17 00:00:00 2001 From: Kit Ewbank Date: Wed, 12 Feb 2020 13:49:11 -0500 Subject: [PATCH] service/s3: Refactor S3 Bucket Object data source and resource to use keyvaluetags package (#11964) Output from acceptance testing: ``` --- PASS: TestAccAWSS3BucketObject_acl (31.06s) --- PASS: TestAccAWSS3BucketObject_content (22.64s) --- PASS: TestAccAWSS3BucketObject_contentBase64 (21.80s) --- PASS: TestAccAWSS3BucketObject_empty (22.31s) --- PASS: TestAccAWSS3BucketObject_etagEncryption (21.89s) --- PASS: TestAccAWSS3BucketObject_kms (36.14s) --- PASS: TestAccAWSS3BucketObject_metadata (30.03s) --- PASS: TestAccAWSS3BucketObject_noNameNoKey (7.02s) --- PASS: TestAccAWSS3BucketObject_NonVersioned (22.05s) --- PASS: TestAccAWSS3BucketObject_ObjectLockLegalHoldStartWithNone (31.88s) --- PASS: TestAccAWSS3BucketObject_ObjectLockLegalHoldStartWithOn (23.42s) --- PASS: TestAccAWSS3BucketObject_ObjectLockRetentionStartWithNone (29.92s) --- PASS: TestAccAWSS3BucketObject_ObjectLockRetentionStartWithSet (35.34s) --- PASS: TestAccAWSS3BucketObject_source (21.47s) --- PASS: TestAccAWSS3BucketObject_sse (15.41s) --- PASS: TestAccAWSS3BucketObject_storageClass (42.50s) --- PASS: TestAccAWSS3BucketObject_tags (36.27s) --- PASS: TestAccAWSS3BucketObject_tagsLeadingSlash (38.08s) --- PASS: TestAccAWSS3BucketObject_updates (30.28s) --- PASS: TestAccAWSS3BucketObject_updateSameFile (29.88s) --- PASS: TestAccAWSS3BucketObject_updatesWithVersioning (26.85s) --- PASS: TestAccAWSS3BucketObject_withContentCharacteristics (21.15s) --- PASS: TestAccDataSourceAWSS3BucketObject_allParams (34.39s) --- PASS: TestAccDataSourceAWSS3BucketObject_basic (36.34s) --- PASS: TestAccDataSourceAWSS3BucketObject_kmsEncrypted (55.18s) --- PASS: TestAccDataSourceAWSS3BucketObject_LeadingSlash (36.58s) --- PASS: TestAccDataSourceAWSS3BucketObject_MultipleSlashes (37.72s) --- PASS: TestAccDataSourceAWSS3BucketObject_ObjectLockLegalHoldOff (37.37s) --- PASS: TestAccDataSourceAWSS3BucketObject_ObjectLockLegalHoldOn (37.99s) --- PASS: TestAccDataSourceAWSS3BucketObject_readableBody (35.87s) --- PASS: TestAccDataSourceAWSS3BucketObjects_all (40.93s) --- PASS: TestAccDataSourceAWSS3BucketObjects_basic (38.14s) --- PASS: TestAccDataSourceAWSS3BucketObjects_encoded (41.15s) --- PASS: TestAccDataSourceAWSS3BucketObjects_fetchOwner (40.57s) --- PASS: TestAccDataSourceAWSS3BucketObjects_maxKeys (40.56s) --- PASS: TestAccDataSourceAWSS3BucketObjects_prefixes (41.50s) --- PASS: TestAccDataSourceAWSS3BucketObjects_startAfter (39.73s) ``` --- aws/data_source_aws_s3_bucket_object.go | 15 +-- aws/internal/keyvaluetags/key_value_tags.go | 12 +++ .../keyvaluetags/key_value_tags_test.go | 49 +++++++++ aws/internal/keyvaluetags/s3_tags.go | 51 +++++++++ aws/resource_aws_s3_bucket_object.go | 53 +++++---- aws/resource_aws_s3_bucket_object_test.go | 12 +-- aws/s3_tags.go | 102 ------------------ aws/s3_tags_test.go | 25 ----- 8 files changed, 158 insertions(+), 161 deletions(-) delete mode 100644 aws/s3_tags.go delete mode 100644 aws/s3_tags_test.go diff --git a/aws/data_source_aws_s3_bucket_object.go b/aws/data_source_aws_s3_bucket_object.go index bcb1814387d2..cad280b2f44b 100644 --- a/aws/data_source_aws_s3_bucket_object.go +++ b/aws/data_source_aws_s3_bucket_object.go @@ -11,6 +11,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/s3" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" ) func dataSourceAwsS3BucketObject() *schema.Resource { @@ -218,15 +219,15 @@ func dataSourceAwsS3BucketObjectRead(d *schema.ResourceData, meta interface{}) e uniqueId, contentType) } - tagResp, err := conn.GetObjectTagging( - &s3.GetObjectTaggingInput{ - Bucket: aws.String(bucket), - Key: aws.String(key), - }) + tags, err := keyvaluetags.S3ObjectListTags(conn, bucket, key) + if err != nil { - return err + return fmt.Errorf("error listing tags for S3 Bucket (%s) Object (%s): %s", bucket, key, err) + } + + if err := d.Set("tags", tags.IgnoreAws().Map()); err != nil { + return fmt.Errorf("error setting tags: %s", err) } - d.Set("tags", tagsToMapS3(tagResp.TagSet)) return nil } diff --git a/aws/internal/keyvaluetags/key_value_tags.go b/aws/internal/keyvaluetags/key_value_tags.go index 9a6ef2998e01..76d319808521 100644 --- a/aws/internal/keyvaluetags/key_value_tags.go +++ b/aws/internal/keyvaluetags/key_value_tags.go @@ -6,6 +6,7 @@ package keyvaluetags import ( "fmt" + "net/url" "strings" "github.com/hashicorp/terraform-plugin-sdk/helper/hashcode" @@ -224,6 +225,17 @@ func (tags KeyValueTags) Hash() int { return hash } +// UrlEncode returns the KeyValueTags encoded as URL Query parameters. +func (tags KeyValueTags) UrlEncode() string { + values := url.Values{} + + for k, v := range tags { + values.Add(k, *v) + } + + return values.Encode() +} + // New creates KeyValueTags from common Terraform Provider SDK types. // Supports map[string]string, map[string]*string, map[string]interface{}, and []interface{}. // When passed []interface{}, all elements are treated as keys and assigned nil values. diff --git a/aws/internal/keyvaluetags/key_value_tags_test.go b/aws/internal/keyvaluetags/key_value_tags_test.go index 3db38c25f312..00010ab1d97c 100644 --- a/aws/internal/keyvaluetags/key_value_tags_test.go +++ b/aws/internal/keyvaluetags/key_value_tags_test.go @@ -939,6 +939,55 @@ func TestKeyValueTagsHash(t *testing.T) { } } +func TestKeyValueTagsUrlEncode(t *testing.T) { + testCases := []struct { + name string + tags KeyValueTags + want string + }{ + { + name: "empty", + tags: New(map[string]string{}), + want: "", + }, + { + name: "single", + tags: New(map[string]string{ + "key1": "value1", + }), + want: "key1=value1", + }, + { + name: "multiple", + tags: New(map[string]string{ + "key1": "value1", + "key2": "value2", + "key3": "value3", + }), + want: "key1=value1&key2=value2&key3=value3", + }, + { + name: "multiple_with_encoded", + tags: New(map[string]string{ + "key1": "value 1", + "key@2": "value+:2", + "key3": "value3", + }), + want: "key1=value+1&key3=value3&key%402=value%2B%3A2", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + got := testCase.tags.UrlEncode() + + if got != testCase.want { + t.Errorf("unexpected URL encoded value: %q", got) + } + }) + } +} + func testKeyValueTagsVerifyKeys(t *testing.T, got []string, want []string) { for _, g := range got { found := false diff --git a/aws/internal/keyvaluetags/s3_tags.go b/aws/internal/keyvaluetags/s3_tags.go index fccea37ccd85..0766c9034b42 100644 --- a/aws/internal/keyvaluetags/s3_tags.go +++ b/aws/internal/keyvaluetags/s3_tags.go @@ -77,3 +77,54 @@ func S3BucketUpdateTags(conn *s3.S3, identifier string, oldTagsMap interface{}, return nil } + +// S3ObjectListTags lists S3 object tags. +func S3ObjectListTags(conn *s3.S3, bucket, key string) (KeyValueTags, error) { + input := &s3.GetObjectTaggingInput{ + Bucket: aws.String(bucket), + Key: aws.String(key), + } + + output, err := conn.GetObjectTagging(input) + + if err != nil { + return New(nil), err + } + + return S3KeyValueTags(output.TagSet), nil +} + +// S3ObjectUpdateTags updates S3 object tags. +func S3ObjectUpdateTags(conn *s3.S3, bucket, key string, oldTagsMap interface{}, newTagsMap interface{}) error { + oldTags := New(oldTagsMap) + newTags := New(newTagsMap) + + if len(newTags) > 0 { + input := &s3.PutObjectTaggingInput{ + Bucket: aws.String(bucket), + Key: aws.String(key), + Tagging: &s3.Tagging{ + TagSet: newTags.IgnoreAws().S3Tags(), + }, + } + + _, err := conn.PutObjectTagging(input) + + if err != nil { + return fmt.Errorf("error setting resource tags (%s/%s): %w", bucket, key, err) + } + } else if len(oldTags) > 0 { + input := &s3.DeleteObjectTaggingInput{ + Bucket: aws.String(bucket), + Key: aws.String(key), + } + + _, err := conn.DeleteObjectTagging(input) + + if err != nil { + return fmt.Errorf("error deleting resource tags (%s/%s): %w", bucket, key, err) + } + } + + return nil +} diff --git a/aws/resource_aws_s3_bucket_object.go b/aws/resource_aws_s3_bucket_object.go index 4043b7c6c560..eca82d8a0238 100644 --- a/aws/resource_aws_s3_bucket_object.go +++ b/aws/resource_aws_s3_bucket_object.go @@ -6,19 +6,18 @@ import ( "fmt" "io" "log" - "net/url" "os" "strings" "time" - "github.com/hashicorp/terraform-plugin-sdk/helper/schema" - "github.com/hashicorp/terraform-plugin-sdk/helper/validation" - "github.com/mitchellh/go-homedir" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/kms" "github.com/aws/aws-sdk-go/service/s3" + "github.com/hashicorp/terraform-plugin-sdk/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/helper/validation" + "github.com/mitchellh/go-homedir" + "github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags" ) func resourceAwsS3BucketObject() *schema.Resource { @@ -281,13 +280,9 @@ func resourceAwsS3BucketObjectPut(d *schema.ResourceData, meta interface{}) erro putInput.ServerSideEncryption = aws.String(s3.ServerSideEncryptionAwsKms) } - if v, ok := d.GetOk("tags"); ok { + if v := d.Get("tags").(map[string]interface{}); len(v) > 0 { // The tag-set must be encoded as URL Query parameters. - values := url.Values{} - for k, v := range v.(map[string]interface{}) { - values.Add(k, v.(string)) - } - putInput.Tagging = aws.String(values.Encode()) + putInput.Tagging = aws.String(keyvaluetags.New(v).IgnoreAws().UrlEncode()) } if v, ok := d.GetOk("website_redirect"); ok { @@ -390,8 +385,17 @@ func resourceAwsS3BucketObjectRead(d *schema.ResourceData, meta interface{}) err d.Set("storage_class", resp.StorageClass) } - if err := getTagsS3Object(s3conn, d); err != nil { - return fmt.Errorf("error getting S3 object tags (bucket: %s, key: %s): %s", bucket, key, err) + // Retry due to S3 eventual consistency + tags, err := retryOnAwsCode(s3.ErrCodeNoSuchBucket, func() (interface{}, error) { + return keyvaluetags.S3ObjectListTags(s3conn, bucket, key) + }) + + if err != nil { + return fmt.Errorf("error listing tags for S3 Bucket (%s) Object (%s): %s", bucket, key, err) + } + + if err := d.Set("tags", tags.(keyvaluetags.KeyValueTags).IgnoreAws().Map()); err != nil { + return fmt.Errorf("error setting tags: %s", err) } return nil @@ -422,10 +426,13 @@ func resourceAwsS3BucketObjectUpdate(d *schema.ResourceData, meta interface{}) e conn := meta.(*AWSClient).s3conn + bucket := d.Get("bucket").(string) + key := d.Get("key").(string) + if d.HasChange("acl") { _, err := conn.PutObjectAcl(&s3.PutObjectAclInput{ - Bucket: aws.String(d.Get("bucket").(string)), - Key: aws.String(d.Get("key").(string)), + Bucket: aws.String(bucket), + Key: aws.String(key), ACL: aws.String(d.Get("acl").(string)), }) if err != nil { @@ -435,8 +442,8 @@ func resourceAwsS3BucketObjectUpdate(d *schema.ResourceData, meta interface{}) e if d.HasChange("object_lock_legal_hold_status") { _, err := conn.PutObjectLegalHold(&s3.PutObjectLegalHoldInput{ - Bucket: aws.String(d.Get("bucket").(string)), - Key: aws.String(d.Get("key").(string)), + Bucket: aws.String(bucket), + Key: aws.String(key), LegalHold: &s3.ObjectLockLegalHold{ Status: aws.String(d.Get("object_lock_legal_hold_status").(string)), }, @@ -448,8 +455,8 @@ func resourceAwsS3BucketObjectUpdate(d *schema.ResourceData, meta interface{}) e if d.HasChange("object_lock_mode") || d.HasChange("object_lock_retain_until_date") { req := &s3.PutObjectRetentionInput{ - Bucket: aws.String(d.Get("bucket").(string)), - Key: aws.String(d.Get("key").(string)), + Bucket: aws.String(bucket), + Key: aws.String(key), Retention: &s3.ObjectLockRetention{ Mode: aws.String(d.Get("object_lock_mode").(string)), RetainUntilDate: expandS3ObjectLockRetainUntilDate(d.Get("object_lock_retain_until_date").(string)), @@ -472,8 +479,12 @@ func resourceAwsS3BucketObjectUpdate(d *schema.ResourceData, meta interface{}) e } } - if err := setTagsS3Object(conn, d); err != nil { - return fmt.Errorf("error setting S3 object tags: %s", err) + if d.HasChange("tags") { + o, n := d.GetChange("tags") + + if err := keyvaluetags.S3ObjectUpdateTags(conn, bucket, key, o, n); err != nil { + return fmt.Errorf("error updating tags: %s", err) + } } return resourceAwsS3BucketObjectRead(d, meta) diff --git a/aws/resource_aws_s3_bucket_object_test.go b/aws/resource_aws_s3_bucket_object_test.go index 47bb7f9f749d..5248c3b6f95c 100644 --- a/aws/resource_aws_s3_bucket_object_test.go +++ b/aws/resource_aws_s3_bucket_object_test.go @@ -630,9 +630,9 @@ func TestAccAWSS3BucketObject_tags(t *testing.T) { testAccCheckAWSS3BucketObjectBody(&obj2, "stuff"), resource.TestCheckResourceAttr(resourceName, "tags.%", "4"), resource.TestCheckResourceAttr(resourceName, "tags.Key2", "BBB"), - resource.TestCheckResourceAttr(resourceName, "tags.Key3", "XXX"), + resource.TestCheckResourceAttr(resourceName, "tags.Key3", "X X"), resource.TestCheckResourceAttr(resourceName, "tags.Key4", "DDD"), - resource.TestCheckResourceAttr(resourceName, "tags.Key5", "EEE"), + resource.TestCheckResourceAttr(resourceName, "tags.Key5", "E:/"), ), }, { @@ -694,9 +694,9 @@ func TestAccAWSS3BucketObject_tagsLeadingSlash(t *testing.T) { testAccCheckAWSS3BucketObjectBody(&obj2, "stuff"), resource.TestCheckResourceAttr(resourceName, "tags.%", "4"), resource.TestCheckResourceAttr(resourceName, "tags.Key2", "BBB"), - resource.TestCheckResourceAttr(resourceName, "tags.Key3", "XXX"), + resource.TestCheckResourceAttr(resourceName, "tags.Key3", "X X"), resource.TestCheckResourceAttr(resourceName, "tags.Key4", "DDD"), - resource.TestCheckResourceAttr(resourceName, "tags.Key5", "EEE"), + resource.TestCheckResourceAttr(resourceName, "tags.Key5", "E:/"), ), }, { @@ -1341,9 +1341,9 @@ resource "aws_s3_bucket_object" "object" { tags = { Key2 = "BBB" - Key3 = "XXX" + Key3 = "X X" Key4 = "DDD" - Key5 = "EEE" + Key5 = "E:/" } } `, randInt, key, content) diff --git a/aws/s3_tags.go b/aws/s3_tags.go deleted file mode 100644 index bd44de6163fb..000000000000 --- a/aws/s3_tags.go +++ /dev/null @@ -1,102 +0,0 @@ -package aws - -import ( - "log" - "regexp" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/s3" - "github.com/hashicorp/terraform-plugin-sdk/helper/schema" -) - -func getTagsS3Object(conn *s3.S3, d *schema.ResourceData) error { - resp, err := retryOnAwsCode(s3.ErrCodeNoSuchKey, func() (interface{}, error) { - return conn.GetObjectTagging(&s3.GetObjectTaggingInput{ - Bucket: aws.String(d.Get("bucket").(string)), - Key: aws.String(d.Get("key").(string)), - }) - }) - if err != nil { - return err - } - - if err := d.Set("tags", tagsToMapS3(resp.(*s3.GetObjectTaggingOutput).TagSet)); err != nil { - return err - } - - return nil -} - -func setTagsS3Object(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{}) - - // Set tags - if len(o) > 0 { - if _, err := conn.DeleteObjectTagging(&s3.DeleteObjectTaggingInput{ - Bucket: aws.String(d.Get("bucket").(string)), - Key: aws.String(d.Get("key").(string)), - }); err != nil { - return err - } - } - if len(n) > 0 { - 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), - }, - }); err != nil { - return err - } - } - } - - return nil -} - -// 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)) - for k, v := range m { - t := &s3.Tag{ - Key: aws.String(k), - Value: aws.String(v.(string)), - } - if !tagIgnoredS3(t) { - result = append(result, t) - } - } - - return result -} - -// tagsToMap turns the list of tags into a map. -func tagsToMapS3(ts []*s3.Tag) map[string]string { - result := make(map[string]string) - for _, t := range ts { - if !tagIgnoredS3(t) { - result[*t.Key] = *t.Value - } - } - - return result -} - -// compare a tag against a list of strings and checks if it should -// be ignored or not -func tagIgnoredS3(t *s3.Tag) bool { - filter := []string{"^aws:"} - for _, v := range filter { - log.Printf("[DEBUG] Matching %v with %v\n", v, *t.Key) - r, _ := regexp.MatchString(v, *t.Key) - if r { - log.Printf("[DEBUG] Found AWS specific tag %s (val: %s), ignoring.\n", *t.Key, *t.Value) - return true - } - } - return false -} diff --git a/aws/s3_tags_test.go b/aws/s3_tags_test.go deleted file mode 100644 index ec089894bae4..000000000000 --- a/aws/s3_tags_test.go +++ /dev/null @@ -1,25 +0,0 @@ -package aws - -import ( - "testing" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/s3" -) - -func TestIgnoringTagsS3(t *testing.T) { - var ignoredTags []*s3.Tag - ignoredTags = append(ignoredTags, &s3.Tag{ - Key: aws.String("aws:cloudformation:logical-id"), - Value: aws.String("foo"), - }) - ignoredTags = append(ignoredTags, &s3.Tag{ - Key: aws.String("aws:foo:bar"), - Value: aws.String("baz"), - }) - for _, tag := range ignoredTags { - if !tagIgnoredS3(tag) { - t.Fatalf("Tag %v with value %v not ignored, but should be!", *tag.Key, *tag.Value) - } - } -}