Skip to content

Commit

Permalink
Merge pull request #35710 from hashicorp/b-s3-empty-tag
Browse files Browse the repository at this point in the history 
Allow empty values for tags for S3 resources
  • Loading branch information
@gdavison
gdavison committed Mar 4, 2024
2 parents b0b511e + 232cd5c commit 8e42385
Show file tree
Hide file tree
Showing 24 changed files with 2,011 additions and 419 deletions.
27 changes: 27 additions & 0 deletions .changelog/35710.txt
View file
@@ -0,0 +1,27 @@
```release-note:bug
resource/aws_s3_bucket: Tags with empty values no longer remove all tags.
```

```release-note:bug
resource/aws_s3_bucket_object: Tags with empty values no longer remove all tags.
```

```release-note:bug
resource/aws_s3_object: Tags with empty values no longer remove all tags.
```

```release-note:bug
resource/aws_s3_object_copy: Tags with empty values no longer remove all tags.
```

```release-note:enhancement
resource/aws_s3_bucket_object: Adds attribute `arn`.
```

```release-note:enhancement
resource/aws_s3_object: Adds attribute `arn`.
```

```release-note:enhancement
resource/aws_s3_object_copy: Adds attribute `arn`.
```
101 changes: 101 additions & 0 deletions internal/service/ec2/vpc_test.go
View file
Expand Up @@ -574,6 +574,107 @@ func TestAccVPC_DefaultTagsProviderAndResource_moveDuplicateTags(t *testing.T) {
})
}

func TestAccVPC_Tags_EmptyTag_OnCreate(t *testing.T) {
ctx := acctest.Context(t)
var vpc awstypes.Vpc
resourceName := "aws_vpc.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckVPCDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCConfig_tags1("key1", ""),
Check: resource.ComposeAggregateTestCheckFunc(
acctest.CheckVPCExistsV2(ctx, resourceName, &vpc),
resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", ""),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccVPC_Tags_EmptyTag_OnUpdate_Add(t *testing.T) {
ctx := acctest.Context(t)
var vpc awstypes.Vpc
resourceName := "aws_vpc.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckVPCDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCConfig_tags1("key1", "value1"),
Check: resource.ComposeAggregateTestCheckFunc(
acctest.CheckVPCExistsV2(ctx, resourceName, &vpc),
resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"),
),
},
{
Config: testAccVPCConfig_tags2("key1", "value1", "key2", ""),
Check: resource.ComposeAggregateTestCheckFunc(
acctest.CheckVPCExistsV2(ctx, resourceName, &vpc),
resource.TestCheckResourceAttr(resourceName, "tags.%", "2"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"),
resource.TestCheckResourceAttr(resourceName, "tags.key2", ""),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccVPC_Tags_EmptyTag_OnUpdate_Replace(t *testing.T) {
ctx := acctest.Context(t)
var vpc awstypes.Vpc
resourceName := "aws_vpc.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.EC2),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckVPCDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccVPCConfig_tags1("key1", "value1"),
Check: resource.ComposeAggregateTestCheckFunc(
acctest.CheckVPCExistsV2(ctx, resourceName, &vpc),
resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"),
),
},
{
Config: testAccVPCConfig_tags1("key1", ""),
Check: resource.ComposeAggregateTestCheckFunc(
acctest.CheckVPCExistsV2(ctx, resourceName, &vpc),
resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", ""),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

// TestAccVPC_DynamicResourceTagsMergedWithLocals_ignoreChanges ensures computed "tags_all"
// attributes are correctly determined when the provider-level default_tags block
// is left unused and resource tags (merged with local.tags) are only known at apply time,
Expand Down
104 changes: 104 additions & 0 deletions internal/service/elbv2/load_balancer_test.go
View file
Expand Up @@ -372,6 +372,110 @@ func TestAccELBV2LoadBalancer_tags(t *testing.T) {
})
}

func TestAccELBV2LoadBalancer_Tags_EmptyTag_OnCreate(t *testing.T) {
ctx := acctest.Context(t)
var conf elbv2.LoadBalancer
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
resourceName := "aws_lb.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.ELBV2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckLoadBalancerDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccLoadBalancerConfig_tags1(rName, "key1", ""),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckLoadBalancerExists(ctx, resourceName, &conf),
resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", ""),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccELBV2LoadBalancer_Tags_EmptyTag_OnUpdate_Add(t *testing.T) {
ctx := acctest.Context(t)
var conf elbv2.LoadBalancer
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
resourceName := "aws_lb.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.ELBV2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckLoadBalancerDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccLoadBalancerConfig_tags1(rName, "key1", "value1"),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckLoadBalancerExists(ctx, resourceName, &conf),
resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"),
),
},
{
Config: testAccLoadBalancerConfig_tags2(rName, "key1", "value1", "key2", ""),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckLoadBalancerExists(ctx, resourceName, &conf),
resource.TestCheckResourceAttr(resourceName, "tags.%", "2"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"),
resource.TestCheckResourceAttr(resourceName, "tags.key2", ""),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccELBV2LoadBalancer_Tags_EmptyTag_OnUpdate_Replace(t *testing.T) {
ctx := acctest.Context(t)
var conf elbv2.LoadBalancer
rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix)
resourceName := "aws_lb.test"

resource.ParallelTest(t, resource.TestCase{
PreCheck: func() { acctest.PreCheck(ctx, t) },
ErrorCheck: acctest.ErrorCheck(t, names.ELBV2ServiceID),
ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories,
CheckDestroy: testAccCheckLoadBalancerDestroy(ctx),
Steps: []resource.TestStep{
{
Config: testAccLoadBalancerConfig_tags1(rName, "key1", "value1"),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckLoadBalancerExists(ctx, resourceName, &conf),
resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", "value1"),
),
},
{
Config: testAccLoadBalancerConfig_tags1(rName, "key1", ""),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckLoadBalancerExists(ctx, resourceName, &conf),
resource.TestCheckResourceAttr(resourceName, "tags.%", "1"),
resource.TestCheckResourceAttr(resourceName, "tags.key1", ""),
),
},
{
ResourceName: resourceName,
ImportState: true,
ImportStateVerify: true,
},
},
})
}

func TestAccELBV2LoadBalancer_ipv6SubnetMapping(t *testing.T) {
ctx := acctest.Context(t)
var conf elbv2.LoadBalancer
Expand Down
46 changes: 6 additions & 40 deletions internal/service/s3/bucket.go
View file
Expand Up @@ -44,11 +44,11 @@ import (
const (
// General timeout for S3 bucket changes to propagate.
// See https://docs.aws.amazon.com/AmazonS3/latest/userguide/Welcome.html#ConsistencyModel.
bucketPropagationTimeout = 2 * time.Minute // nosemgrep:ci.s3-in-const-name, ci.s3-in-var-name
bucketPropagationTimeout = 2 * time.Minute
)

// @SDKResource("aws_s3_bucket", name="Bucket")
// @Tags
// @Tags(identifierAttribute="bucket", resourceType="Bucket")
func resourceBucket() *schema.Resource {
return &schema.Resource{
CreateWithoutTimeout: resourceBucketCreate,
Expand Down Expand Up @@ -773,6 +773,10 @@ func resourceBucketCreate(ctx context.Context, d *schema.ResourceData, meta inte
return sdkdiag.AppendErrorf(diags, "waiting for S3 Bucket (%s) create: %s", d.Id(), err)
}

if err := bucketCreateTags(ctx, conn, d.Id(), getTagsIn(ctx)); err != nil {
return sdkdiag.AppendErrorf(diags, "setting S3 Bucket (%s) tags: %s", d.Id(), err)
}

return append(diags, resourceBucketUpdate(ctx, d, meta)...)
}

Expand Down Expand Up @@ -1137,27 +1141,6 @@ func resourceBucketRead(ctx context.Context, d *schema.ResourceData, meta interf
d.Set("website_endpoint", endpoint)
}

//
// Bucket Tags.
//
tags, err := retryWhenNoSuchBucketError(ctx, d.Timeout(schema.TimeoutRead), func() (tftags.KeyValueTags, error) {
return bucketListTags(ctx, conn, d.Id())
})

if !d.IsNewResource() && tfawserr.ErrCodeEquals(err, errCodeNoSuchBucket) {
log.Printf("[WARN] S3 Bucket (%s) not found, removing from state", d.Id())
d.SetId("")
return diags
}

switch {
case err == nil:
setTagsOut(ctx, Tags(tags))
case tfawserr.ErrCodeEquals(err, errCodeMethodNotAllowed, errCodeNotImplemented, errCodeXNotImplemented):
default:
return sdkdiag.AppendErrorf(diags, "listing tags for S3 Bucket (%s): %s", d.Id(), err)
}

return diags
}

Expand Down Expand Up @@ -1556,23 +1539,6 @@ func resourceBucketUpdate(ctx context.Context, d *schema.ResourceData, meta inte
}
}

//
// Bucket Tags.
//
if d.HasChange("tags_all") {
o, n := d.GetChange("tags_all")

// Retry due to S3 eventual consistency.
_, err := tfresource.RetryWhenAWSErrCodeEquals(ctx, d.Timeout(schema.TimeoutUpdate), func() (interface{}, error) {
terr := bucketUpdateTags(ctx, conn, d.Id(), o, n)
return nil, terr
}, errCodeNoSuchBucket)

if err != nil {
return sdkdiag.AppendErrorf(diags, "updating S3 Bucket (%s) tags: %s", d.Id(), err)
}
}

return append(diags, resourceBucketRead(ctx, d, meta)...)
}

Expand Down

0 comments on commit 8e42385

Please sign in to comment.