Refactor S3 Bucket Object data source and resource to use keyvaluetags package

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
 }

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.

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

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
+}

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)

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)