Skip to content

Commit

Permalink
service/s3: Refactor S3 Bucket Object data source and resource to use…
Browse files Browse the repository at this point in the history 
… 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)
```
  • Loading branch information
@ewbankkit
ewbankkit authored Feb 12, 2020
1 parent d6263e8 commit 4acb898
Show file tree
Hide file tree
Showing 8 changed files with 158 additions and 161 deletions.
15 changes: 8 additions & 7 deletions aws/data_source_aws_s3_bucket_object.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down
12 changes: 12 additions & 0 deletions aws/internal/keyvaluetags/key_value_tags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package keyvaluetags

import (
"fmt"
"net/url"
"strings"

"github.com/hashicorp/terraform-plugin-sdk/helper/hashcode"
Expand Down Expand Up @@ -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.
Expand Down
49 changes: 49 additions & 0 deletions aws/internal/keyvaluetags/key_value_tags_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
51 changes: 51 additions & 0 deletions aws/internal/keyvaluetags/s3_tags.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
53 changes: 32 additions & 21 deletions aws/resource_aws_s3_bucket_object.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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)),
},
Expand All @@ -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)),
Expand All @@ -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)
Expand Down
12 changes: 6 additions & 6 deletions aws/resource_aws_s3_bucket_object_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:/"),
),
},
{
Expand Down Expand Up @@ -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:/"),
),
},
{
Expand Down Expand Up @@ -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)
Expand Down
Loading

0 comments on commit 4acb898

Please sign in to comment.