From b6ca2108181f63ee0712cd1f31921f35c5c0eff4 Mon Sep 17 00:00:00 2001 From: Anthony Wat Date: Sat, 18 May 2024 20:44:42 -0400 Subject: [PATCH 1/4] fix: Fix crash during read due to key-only tags in aws_lightsail_instance --- .changelog/37587.txt | 3 + internal/service/lightsail/generate.go | 2 +- internal/service/lightsail/instance_test.go | 98 +++++++++++++++++++++ internal/tags/key_value_tags.go | 6 +- internal/tags/key_value_tags_test.go | 9 ++ 5 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 .changelog/37587.txt diff --git a/.changelog/37587.txt b/.changelog/37587.txt new file mode 100644 index 00000000000..437883e9985 --- /dev/null +++ b/.changelog/37587.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_lightsail_instance: Fix crash when reading a resource that has a key-only tag +``` \ No newline at end of file diff --git a/internal/service/lightsail/generate.go b/internal/service/lightsail/generate.go index 71da4d3c82a..1bee40fff9d 100644 --- a/internal/service/lightsail/generate.go +++ b/internal/service/lightsail/generate.go @@ -1,7 +1,7 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -//go:generate go run ../../generate/tags/main.go -AWSSDKVersion=2 -ListTagsInIDElem=ResourceName -ServiceTagsSlice -TagInIDElem=ResourceName -UpdateTags -CreateTags +//go:generate go run ../../generate/tags/main.go -AWSSDKVersion=2 -ServiceTagsSlice -TagInIDElem=ResourceName -CreateTags -UpdateTags //go:generate go run ../../generate/servicepackage/main.go // ONLY generate directives and package declaration! Do not add anything else to this file. diff --git a/internal/service/lightsail/instance_test.go b/internal/service/lightsail/instance_test.go index 5e133d252ad..cf00f8db73a 100644 --- a/internal/service/lightsail/instance_test.go +++ b/internal/service/lightsail/instance_test.go @@ -150,8 +150,14 @@ func TestAccLightsailInstance_tags(t *testing.T) { resource.TestCheckResourceAttrSet(resourceName, "bundle_id"), resource.TestCheckResourceAttrSet(resourceName, "key_pair_name"), resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, acctest.Ct1), + resource.TestCheckResourceAttr(resourceName, "tags.Name", "tf-test"), ), }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, { Config: testAccInstanceConfig_tags2(rName), Check: resource.ComposeAggregateTestCheckFunc( @@ -161,6 +167,60 @@ func TestAccLightsailInstance_tags(t *testing.T) { resource.TestCheckResourceAttrSet(resourceName, "bundle_id"), resource.TestCheckResourceAttrSet(resourceName, "key_pair_name"), resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, acctest.Ct2), + resource.TestCheckResourceAttr(resourceName, "tags.Name", "tf-test"), + resource.TestCheckResourceAttr(resourceName, "tags.ExtraName", "tf-test"), + ), + }, + }, + }) +} + +func TestAccLightsailInstance_keyOnlyTags(t *testing.T) { + ctx := acctest.Context(t) + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_lightsail_instance.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { + acctest.PreCheck(ctx, t) + acctest.PreCheckPartitionHasService(t, strings.ToLower(lightsail.ServiceID)) + testAccPreCheck(ctx, t) + }, + ErrorCheck: acctest.ErrorCheck(t, strings.ToLower(lightsail.ServiceID)), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckInstanceDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccInstanceConfig_keyOnlyTags1(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName), + resource.TestCheckResourceAttrSet(resourceName, names.AttrAvailabilityZone), + resource.TestCheckResourceAttrSet(resourceName, "blueprint_id"), + resource.TestCheckResourceAttrSet(resourceName, "bundle_id"), + resource.TestCheckResourceAttrSet(resourceName, "key_pair_name"), + resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, acctest.Ct2), + resource.TestCheckResourceAttr(resourceName, "tags.Name", "tf-test"), + resource.TestCheckResourceAttr(resourceName, "tags.EmptyTag1", ""), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateVerify: true, + }, + { + Config: testAccInstanceConfig_keyOnlyTags2(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckInstanceExists(ctx, resourceName), + resource.TestCheckResourceAttrSet(resourceName, names.AttrAvailabilityZone), + resource.TestCheckResourceAttrSet(resourceName, "blueprint_id"), + resource.TestCheckResourceAttrSet(resourceName, "bundle_id"), + resource.TestCheckResourceAttrSet(resourceName, "key_pair_name"), + resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, acctest.Ct4), + resource.TestCheckResourceAttr(resourceName, "tags.Name", "tf-test"), + resource.TestCheckResourceAttr(resourceName, "tags.ExtraName", "tf-test"), + resource.TestCheckResourceAttr(resourceName, "tags.EmptyTag1", "NoLongerEmpty"), + resource.TestCheckResourceAttr(resourceName, "tags.EmptyTag2", ""), ), }, }, @@ -468,6 +528,44 @@ resource "aws_lightsail_instance" "test" { `, rName)) } +func testAccInstanceConfig_keyOnlyTags1(rName string) string { + return acctest.ConfigCompose( + testAccInstanceConfigBase(), + fmt.Sprintf(` +resource "aws_lightsail_instance" "test" { + name = "%s" + availability_zone = data.aws_availability_zones.available.names[0] + blueprint_id = "amazon_linux_2" + bundle_id = "nano_3_0" + + tags = { + Name = "tf-test" + EmptyTag1 = "" + } +} +`, rName)) +} + +func testAccInstanceConfig_keyOnlyTags2(rName string) string { + return acctest.ConfigCompose( + testAccInstanceConfigBase(), + fmt.Sprintf(` +resource "aws_lightsail_instance" "test" { + name = "%s" + availability_zone = data.aws_availability_zones.available.names[0] + blueprint_id = "amazon_linux_2" + bundle_id = "nano_3_0" + + tags = { + Name = "tf-test", + ExtraName = "tf-test" + EmptyTag1 = "NotEmptyAnymore" + EmptyTag2 = "" + } +} +`, rName)) +} + func testAccInstanceConfig_IPAddressType(rName string, rIPAddressType string) string { return acctest.ConfigCompose( testAccInstanceConfigBase(), diff --git a/internal/tags/key_value_tags.go b/internal/tags/key_value_tags.go index 3cbeb8a030d..82a7689c3ff 100644 --- a/internal/tags/key_value_tags.go +++ b/internal/tags/key_value_tags.go @@ -735,7 +735,11 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D result := make(map[string]string) for k, v := range t { - result[k] = v.ValueString() + if v != nil { + result[k] = v.ValueString() + } else { + result[k] = "" + } } configTags := make(map[string]configTag) diff --git a/internal/tags/key_value_tags_test.go b/internal/tags/key_value_tags_test.go index ea7981fc9ef..980a7ecd5c0 100644 --- a/internal/tags/key_value_tags_test.go +++ b/internal/tags/key_value_tags_test.go @@ -1433,6 +1433,15 @@ func TestKeyValueTagsMap(t *testing.T) { "key3": "value3", }, }, + { + name: "empty_value", + tags: New(ctx, map[string]*string{ + "key1": testStringPtr(""), + }), + want: map[string]string{ + "key1": "", + }, + }, { name: "nil_value", tags: New(ctx, map[string]*string{ From 54867a22d59342cfbf5548cd14c397edfeb5fd19 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Tue, 21 May 2024 11:16:12 -0500 Subject: [PATCH 2/4] aws_lightsail_instance: add ability to list tags --- internal/service/lightsail/instance.go | 2 +- internal/service/lightsail/instance_test.go | 2 +- .../service/lightsail/service_package_gen.go | 1 + internal/service/lightsail/tags.go | 50 +++++++++++++++++++ 4 files changed, 53 insertions(+), 2 deletions(-) create mode 100644 internal/service/lightsail/tags.go diff --git a/internal/service/lightsail/instance.go b/internal/service/lightsail/instance.go index 49a69a2fb22..461fed6228e 100644 --- a/internal/service/lightsail/instance.go +++ b/internal/service/lightsail/instance.go @@ -28,7 +28,7 @@ import ( ) // @SDKResource("aws_lightsail_instance", name="Instance") -// @Tags(identifierAttribute="id") +// @Tags(identifierAttribute="id", resourceType="Instance") func ResourceInstance() *schema.Resource { return &schema.Resource{ CreateWithoutTimeout: resourceInstanceCreate, diff --git a/internal/service/lightsail/instance_test.go b/internal/service/lightsail/instance_test.go index cf00f8db73a..ef14443d90f 100644 --- a/internal/service/lightsail/instance_test.go +++ b/internal/service/lightsail/instance_test.go @@ -219,7 +219,7 @@ func TestAccLightsailInstance_keyOnlyTags(t *testing.T) { resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, acctest.Ct4), resource.TestCheckResourceAttr(resourceName, "tags.Name", "tf-test"), resource.TestCheckResourceAttr(resourceName, "tags.ExtraName", "tf-test"), - resource.TestCheckResourceAttr(resourceName, "tags.EmptyTag1", "NoLongerEmpty"), + resource.TestCheckResourceAttr(resourceName, "tags.EmptyTag1", "NotEmptyAnymore"), resource.TestCheckResourceAttr(resourceName, "tags.EmptyTag2", ""), ), }, diff --git a/internal/service/lightsail/service_package_gen.go b/internal/service/lightsail/service_package_gen.go index b70508bcd8e..0068b575374 100644 --- a/internal/service/lightsail/service_package_gen.go +++ b/internal/service/lightsail/service_package_gen.go @@ -104,6 +104,7 @@ func (p *servicePackage) SDKResources(ctx context.Context) []*types.ServicePacka Name: "Instance", Tags: &types.ServicePackageResourceTags{ IdentifierAttribute: names.AttrID, + ResourceType: "Instance", }, }, { diff --git a/internal/service/lightsail/tags.go b/internal/service/lightsail/tags.go new file mode 100644 index 00000000000..519a43d79f9 --- /dev/null +++ b/internal/service/lightsail/tags.go @@ -0,0 +1,50 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +//go:build !generate +// +build !generate + +package lightsail + +import ( + "context" + + "github.com/aws/aws-sdk-go-v2/service/lightsail" + "github.com/hashicorp/terraform-provider-aws/internal/conns" + tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" + "github.com/hashicorp/terraform-provider-aws/internal/types/option" +) + +func (p *servicePackage) ListTags(ctx context.Context, meta any, identifier, resourceType string) error { + var ( + tags tftags.KeyValueTags + err error + ) + switch resourceType { + case "Instance": + tags, err = instanceListTags(ctx, meta.(*conns.AWSClient).LightsailClient(ctx), identifier) + + default: + return nil + } + + if err != nil { + return err + } + + if inContext, ok := tftags.FromContext(ctx); ok { + inContext.TagsOut = option.Some(tags) + } + + return nil +} + +func instanceListTags(ctx context.Context, client *lightsail.Client, id string) (tftags.KeyValueTags, error) { + out, err := FindInstanceById(ctx, client, id) + + if err != nil { + return tftags.New(ctx, nil), err + } + + return KeyValueTags(ctx, out.Tags), nil +} From 3bb8833a48a2433854eb1772664ca243040c9138 Mon Sep 17 00:00:00 2001 From: Anthony Wat Date: Tue, 21 May 2024 23:15:19 -0400 Subject: [PATCH 3/4] style: Move nil check into helper function per review comment --- internal/tags/key_value_tags.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/internal/tags/key_value_tags.go b/internal/tags/key_value_tags.go index 82a7689c3ff..68ca0e6eb6e 100644 --- a/internal/tags/key_value_tags.go +++ b/internal/tags/key_value_tags.go @@ -626,7 +626,7 @@ type TagData struct { } func (td *TagData) ValueString() string { - if td.Value == nil { + if td == nil || td.Value == nil { return "" } @@ -735,11 +735,7 @@ func (tags KeyValueTags) ResolveDuplicates(ctx context.Context, defaultConfig *D result := make(map[string]string) for k, v := range t { - if v != nil { - result[k] = v.ValueString() - } else { - result[k] = "" - } + result[k] = v.ValueString() } configTags := make(map[string]configTag) From 917564ad81e04b65c93d37ba4033f5b716f94565 Mon Sep 17 00:00:00 2001 From: Adrian Johnson Date: Fri, 24 May 2024 12:51:12 -0500 Subject: [PATCH 4/4] aws_lightsail_instance: fix test formatting --- internal/service/lightsail/instance_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/service/lightsail/instance_test.go b/internal/service/lightsail/instance_test.go index ef14443d90f..7b8184e29db 100644 --- a/internal/service/lightsail/instance_test.go +++ b/internal/service/lightsail/instance_test.go @@ -472,7 +472,7 @@ func testAccInstanceConfig_basic(rName string) string { testAccInstanceConfigBase(), fmt.Sprintf(` resource "aws_lightsail_instance" "test" { - name = "%s" + name = %[1]q availability_zone = data.aws_availability_zones.available.names[0] blueprint_id = "amazon_linux_2" bundle_id = "nano_3_0" @@ -498,7 +498,7 @@ func testAccInstanceConfig_tags1(rName string) string { testAccInstanceConfigBase(), fmt.Sprintf(` resource "aws_lightsail_instance" "test" { - name = "%s" + name = %[1]q availability_zone = data.aws_availability_zones.available.names[0] blueprint_id = "amazon_linux_2" bundle_id = "nano_3_0" @@ -515,7 +515,7 @@ func testAccInstanceConfig_tags2(rName string) string { testAccInstanceConfigBase(), fmt.Sprintf(` resource "aws_lightsail_instance" "test" { - name = "%s" + name = %[1]q availability_zone = data.aws_availability_zones.available.names[0] blueprint_id = "amazon_linux_2" bundle_id = "nano_3_0" @@ -533,7 +533,7 @@ func testAccInstanceConfig_keyOnlyTags1(rName string) string { testAccInstanceConfigBase(), fmt.Sprintf(` resource "aws_lightsail_instance" "test" { - name = "%s" + name = %[1]q availability_zone = data.aws_availability_zones.available.names[0] blueprint_id = "amazon_linux_2" bundle_id = "nano_3_0" @@ -551,7 +551,7 @@ func testAccInstanceConfig_keyOnlyTags2(rName string) string { testAccInstanceConfigBase(), fmt.Sprintf(` resource "aws_lightsail_instance" "test" { - name = "%s" + name = %[1]q availability_zone = data.aws_availability_zones.available.names[0] blueprint_id = "amazon_linux_2" bundle_id = "nano_3_0"