Correctly handle ignored tags for S3 Bucket and Object resources

aws/internal/keyvaluetags/key_value_tags.go

@@ -11,6 +11,7 @@ import (
 	"net/url"
 	"reflect"
 	"regexp"
+	"sort"
 	"strings"
 
 	"github.com/terraform-providers/terraform-provider-aws/aws/internal/hashcode"
@@ -380,6 +381,25 @@ func (tags KeyValueTags) Hash() int {
 	return hash
 }
 
+// String returns the default string representation of the KeyValueTags.
+func (tags KeyValueTags) String() string {
+	var builder strings.Builder
+
+	keys := tags.Keys()
+	sort.Strings(keys)
+
+	builder.WriteString("map[")
+	for i, k := range keys {
+		if i > 0 {
+			builder.WriteString(" ")
+		}
+		fmt.Fprintf(&builder, "%s:%s", k, tags[k].String())
+	}
+	builder.WriteString("]")
+
+	return builder.String()
+}
+
 // UrlEncode returns the KeyValueTags encoded as URL Query parameters.
 func (tags KeyValueTags) UrlEncode() string {
 	values := url.Values{}

aws/internal/keyvaluetags/key_value_tags_test.go

@@ -1859,6 +1859,55 @@ func TestToSnakeCase(t *testing.T) {
 	}
 }
 
+func TestKeyValueTagsString(t *testing.T) {
+	testCases := []struct {
+		name string
+		tags KeyValueTags
+		want string
+	}{
+		{
+			name: "empty",
+			tags: New(map[string]string{}),
+			want: "map[]",
+		},
+		{
+			name: "no value",
+			tags: New(map[string]*string{
+				"key1": nil,
+			}),
+			want: "map[key1:]",
+		},
+		{
+			name: "single",
+			tags: New(map[string]string{
+				"key1": "value1",
+			}),
+			want: "map[key1:TagData{Value: value1}]",
+		},
+		{
+			name: "multiple",
+			tags: New(map[string]string{
+				"key1": "value1",
+				"key3": "value3",
+				"key2": "value2",
+				"key5": "value5",
+				"key4": "value4",
+			}),
+			want: "map[key1:TagData{Value: value1} key2:TagData{Value: value2} key3:TagData{Value: value3} key4:TagData{Value: value4} key5:TagData{Value: value5}]",
+		},
+	}
+
+	for _, testCase := range testCases {
+		t.Run(testCase.name, func(t *testing.T) {
+			got := testCase.tags.String()
+
+			if got != testCase.want {
+				t.Errorf("unexpected string value: %q", got)
+			}
+		})
+	}
+}
+
 func testKeyValueTagsVerifyKeys(t *testing.T, got []string, want []string) {
 	for _, g := range got {
 		found := false

aws/internal/keyvaluetags/s3_tags.go

@@ -8,6 +8,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/s3"
 	"github.com/hashicorp/aws-sdk-go-base/tfawserr"
+	tfs3 "github.com/terraform-providers/terraform-provider-aws/aws/internal/service/s3"
 )
 
 // Custom S3 tag service update functions using the same format as generated code.
@@ -24,7 +25,7 @@ func S3BucketListTags(conn *s3.S3, identifier string) (KeyValueTags, error) {
 	// S3 API Reference (https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetBucketTagging.html)
 	// lists the special error as NoSuchTagSetError, however the existing logic used NoSuchTagSet
 	// and the AWS Go SDK has neither as a constant.
-	if tfawserr.ErrCodeEquals(err, "NoSuchTagSet") {
+	if tfawserr.ErrCodeEquals(err, tfs3.NoSuchTagSet) {
 		return New(nil), nil
 	}
 
@@ -41,20 +42,20 @@ func S3BucketUpdateTags(conn *s3.S3, identifier string, oldTagsMap interface{},
 	oldTags := New(oldTagsMap)
 	newTags := New(newTagsMap)
 
-	// We need to also consider any existing system tags.
+	// We need to also consider any existing ignored tags.
 	allTags, err := S3BucketListTags(conn, identifier)
 
 	if err != nil {
 		return fmt.Errorf("error listing resource tags (%s): %w", identifier, err)
 	}
 
-	sysTags := allTags.Removed(allTags.IgnoreAws())
+	ignoredTags := allTags.Ignore(oldTags).Ignore(newTags)
 
-	if len(newTags)+len(sysTags) > 0 {
+	if len(newTags)+len(ignoredTags) > 0 {
 		input := &s3.PutBucketTaggingInput{
 			Bucket: aws.String(identifier),
 			Tagging: &s3.Tagging{
-				TagSet: newTags.Merge(sysTags).S3Tags(),
+				TagSet: newTags.Merge(ignoredTags).S3Tags(),
 			},
 		}
 
@@ -63,7 +64,7 @@ func S3BucketUpdateTags(conn *s3.S3, identifier string, oldTagsMap interface{},
 		if err != nil {
 			return fmt.Errorf("error setting resource tags (%s): %w", identifier, err)
 		}
-	} else if len(oldTags) > 0 && len(sysTags) == 0 {
+	} else if len(oldTags) > 0 && len(ignoredTags) == 0 {
 		input := &s3.DeleteBucketTaggingInput{
 			Bucket: aws.String(identifier),
 		}
@@ -99,12 +100,21 @@ func S3ObjectUpdateTags(conn *s3.S3, bucket, key string, oldTagsMap interface{},
 	oldTags := New(oldTagsMap)
 	newTags := New(newTagsMap)
 
-	if len(newTags) > 0 {
+	// We need to also consider any existing ignored tags.
+	allTags, err := S3ObjectListTags(conn, bucket, key)
+
+	if err != nil {
+		return fmt.Errorf("error listing resource tags (%s/%s): %w", bucket, key, err)
+	}
+
+	ignoredTags := allTags.Ignore(oldTags).Ignore(newTags)
+
+	if len(newTags)+len(ignoredTags) > 0 {
 		input := &s3.PutObjectTaggingInput{
 			Bucket: aws.String(bucket),
 			Key:    aws.String(key),
 			Tagging: &s3.Tagging{
-				TagSet: newTags.IgnoreAws().S3Tags(),
+				TagSet: newTags.Merge(ignoredTags).IgnoreAws().S3Tags(),
 			},
 		}
 
@@ -113,7 +123,7 @@ func S3ObjectUpdateTags(conn *s3.S3, bucket, key string, oldTagsMap interface{},
 		if err != nil {
 			return fmt.Errorf("error setting resource tags (%s/%s): %w", bucket, key, err)
 		}
-	} else if len(oldTags) > 0 {
+	} else if len(oldTags) > 0 && len(ignoredTags) == 0 {
 		input := &s3.DeleteObjectTaggingInput{
 			Bucket: aws.String(bucket),
 			Key:    aws.String(key),

aws/internal/service/s3/errors.go

@@ -0,0 +1,5 @@
+package s3
+
+const (
+	NoSuchTagSet = "NoSuchTagSet"
+)

aws/resource_aws_s3_bucket_object_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+	"github.com/terraform-providers/terraform-provider-aws/aws/internal/keyvaluetags"
 )
 
 func init() {
@@ -954,6 +955,56 @@ func TestAccAWSS3BucketObject_ObjectLockRetentionStartWithSet(t *testing.T) {
 	})
 }
 
+func TestAccAWSS3BucketObject_ignoreTags(t *testing.T) {
+	var obj s3.GetObjectOutput
+	resourceName := "aws_s3_bucket_object.object"
+	rInt := acctest.RandInt()
+	key := "test-key"
+
+	resource.ParallelTest(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSS3BucketObjectDestroy,
+		Steps: []resource.TestStep{
+			{
+				PreConfig: func() {},
+				Config: composeConfig(
+					testAccProviderConfigIgnoreTagsKeyPrefixes1("ignorekey"),
+					testAccAWSS3BucketObjectConfig_withNoTags(rInt, key, "stuff")),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSS3BucketObjectExists(resourceName, &obj),
+					testAccCheckAWSS3BucketObjectBody(&obj, "stuff"),
+					testAccCheckAWSS3BucketObjectUpdateTags(resourceName, nil, map[string]string{"ignorekey1": "ignorevalue1"}),
+					resource.TestCheckResourceAttr(resourceName, "tags.%", "0"),
+					testAccCheckAWSS3BucketObjectCheckTags(resourceName, map[string]string{
+						"ignorekey1": "ignorevalue1",
+					}),
+				),
+			},
+			{
+				PreConfig: func() {},
+				Config: composeConfig(
+					testAccProviderConfigIgnoreTagsKeyPrefixes1("ignorekey"),
+					testAccAWSS3BucketObjectConfig_withTags(rInt, key, "stuff")),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSS3BucketObjectExists(resourceName, &obj),
+					testAccCheckAWSS3BucketObjectBody(&obj, "stuff"),
+					resource.TestCheckResourceAttr(resourceName, "tags.%", "3"),
+					resource.TestCheckResourceAttr(resourceName, "tags.Key1", "AAA"),
+					resource.TestCheckResourceAttr(resourceName, "tags.Key2", "BBB"),
+					resource.TestCheckResourceAttr(resourceName, "tags.Key3", "CCC"),
+					testAccCheckAWSS3BucketObjectCheckTags(resourceName, map[string]string{
+						"ignorekey1": "ignorevalue1",
+						"Key1":       "AAA",
+						"Key2":       "BBB",
+						"Key3":       "CCC",
+					}),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckAWSS3BucketObjectVersionIdDiffers(first, second *s3.GetObjectOutput) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		if first.VersionId == nil {
@@ -1155,6 +1206,34 @@ func testAccAWSS3BucketObjectCreateTempFile(t *testing.T, data string) string {
 	return filename
 }
 
+func testAccCheckAWSS3BucketObjectUpdateTags(n string, oldTags, newTags map[string]string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		rs := s.RootModule().Resources[n]
+		conn := testAccProvider.Meta().(*AWSClient).s3conn
+
+		return keyvaluetags.S3ObjectUpdateTags(conn, rs.Primary.Attributes["bucket"], rs.Primary.Attributes["key"], oldTags, newTags)
+	}
+}
+
+func testAccCheckAWSS3BucketObjectCheckTags(n string, expectedTags map[string]string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		rs := s.RootModule().Resources[n]
+		conn := testAccProvider.Meta().(*AWSClient).s3conn
+
+		got, err := keyvaluetags.S3ObjectListTags(conn, rs.Primary.Attributes["bucket"], rs.Primary.Attributes["key"])
+		if err != nil {
+			return err
+		}
+
+		want := keyvaluetags.New(expectedTags)
+		if !reflect.DeepEqual(want, got) {
+			return fmt.Errorf("Incorrect tags, want: %v got: %v", want, got)
+		}
+
+		return nil
+	}
+}
+
 func testAccAWSS3BucketObjectConfigBasic(bucket, key string) string {
 	return fmt.Sprintf(`
 resource "aws_s3_bucket_object" "object" {