diff --git a/.changelog/44375.txt b/.changelog/44375.txt new file mode 100644 index 000000000000..416d45ce6ae7 --- /dev/null +++ b/.changelog/44375.txt @@ -0,0 +1,10 @@ +```release-note:note +provider: This release contains both internal provider fixes and a Terraform Plugin SDK V2 update related to a [regression](https://github.com/hashicorp/terraform-provider-aws/issues/44366) which may impact resources that support resource identity +``` + +```release-note:bug +provider: Fix `Missing Resource Identity After Update` errors for non-refreshed and failed updates +``` +```release-note:bug +provider: Fix `Unexpected Identity Change` errors when fully-null identity values in state are updated to valid values +``` diff --git a/go.mod b/go.mod index 6a6d69cdb6a9..050b0dba7bee 100644 --- a/go.mod +++ b/go.mod @@ -299,7 +299,7 @@ require ( github.com/hashicorp/terraform-plugin-go v0.29.0 github.com/hashicorp/terraform-plugin-log v0.9.0 github.com/hashicorp/terraform-plugin-mux v0.21.0 - github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.0 + github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.1 github.com/hashicorp/terraform-plugin-testing v1.14.0-beta.1 github.com/jaswdr/faker/v2 v2.8.0 github.com/jmespath/go-jmespath v0.4.0 diff --git a/go.sum b/go.sum index f290a0d72fe4..b15fd01692b3 100644 --- a/go.sum +++ b/go.sum @@ -683,8 +683,8 @@ github.com/hashicorp/terraform-plugin-go v0.29.0 h1:1nXKl/nSpaYIUBU1IG/EsDOX0vv+ github.com/hashicorp/terraform-plugin-go v0.29.0/go.mod h1:vYZbIyvxyy0FWSmDHChCqKvI40cFTDGSb3D8D70i9GM= github.com/hashicorp/terraform-plugin-mux v0.21.0 h1:QsEYnzSD2c3zT8zUrUGqaFGhV/Z8zRUlU7FY3ZPJFfw= github.com/hashicorp/terraform-plugin-mux v0.21.0/go.mod h1:Qpt8+6AD7NmL0DS7ASkN0EXpDQ2J/FnnIgeUr1tzr5A= -github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.0 h1:PQP7Crrc7t/ozj+P9x0/lsTzGNy3lVppH8zAJylofaE= -github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.0/go.mod h1:GQhpKVvvuwzD79e8/NZ+xzj+ZpWovdPAe8nfV/skwNU= +github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.1 h1:mlAq/OrMlg04IuJT7NpefI1wwtdpWudnEmjuQs04t/4= +github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.1/go.mod h1:GQhpKVvvuwzD79e8/NZ+xzj+ZpWovdPAe8nfV/skwNU= github.com/hashicorp/terraform-plugin-testing v1.14.0-beta.1 h1:caWmY2Fv/KgDAXU7IVjcBDfIdmr/n6VRYhCLxNmlaXs= github.com/hashicorp/terraform-plugin-testing v1.14.0-beta.1/go.mod h1:jVm3pD9uQAT0X2RSEdcqjju2bCGv5f73DGZFU4v7EAU= github.com/hashicorp/terraform-registry-address v0.4.0 h1:S1yCGomj30Sao4l5BMPjTGZmCNzuv7/GDTDX99E9gTk= diff --git a/internal/provider/sdkv2/identity_interceptor.go b/internal/provider/sdkv2/identity_interceptor.go index e9b5cbe984ed..a3c838280c82 100644 --- a/internal/provider/sdkv2/identity_interceptor.go +++ b/internal/provider/sdkv2/identity_interceptor.go @@ -29,7 +29,7 @@ func (r identityInterceptor) run(ctx context.Context, opts crudInterceptorOption case After: switch why { case Create, Read, Update: - if why == Update && !(r.identitySpec.IsMutable && r.identitySpec.IsSetOnUpdate) { + if why == Update && !(r.identitySpec.IsMutable && r.identitySpec.IsSetOnUpdate) && !identityIsFullyNull(d, r.identitySpec) { break } if d.Id() == "" { @@ -63,11 +63,65 @@ func (r identityInterceptor) run(ctx context.Context, opts crudInterceptorOption } } } + case OnError: + switch why { + case Update: + if identityIsFullyNull(d, r.identitySpec) { + if d.Id() == "" { + break + } + identity, err := d.Identity() + if err != nil { + return sdkdiag.AppendFromErr(diags, err) + } + + for _, attr := range r.identitySpec.Attributes { + switch attr.Name() { + case names.AttrAccountID: + if err := identity.Set(attr.Name(), awsClient.AccountID(ctx)); err != nil { + return sdkdiag.AppendFromErr(diags, err) + } + + case names.AttrRegion: + if err := identity.Set(attr.Name(), awsClient.Region(ctx)); err != nil { + return sdkdiag.AppendFromErr(diags, err) + } + + default: + val, ok := getAttributeOk(d, attr.ResourceAttributeName()) + if !ok { + continue + } + if err := identity.Set(attr.Name(), val); err != nil { + return sdkdiag.AppendFromErr(diags, err) + } + } + } + } + } } return diags } +// identityIsFullyNull returns true if a resource supports identity and +// all attributes are set to null values +func identityIsFullyNull(d schemaResourceData, identitySpec *inttypes.Identity) bool { + identity, err := d.Identity() + if err != nil { + return false + } + + for _, attr := range identitySpec.Attributes { + value := identity.Get(attr.Name()) + if value != "" { + return false + } + } + + return true +} + func getAttributeOk(d schemaResourceData, name string) (string, bool) { if name == "id" { return d.Id(), true @@ -81,17 +135,13 @@ func getAttributeOk(d schemaResourceData, name string) (string, bool) { func newIdentityInterceptor(identitySpec *inttypes.Identity) interceptorInvocation { interceptor := interceptorInvocation{ - when: After, - why: Create | Read, + when: After | OnError, + why: Create | Read | Update, interceptor: identityInterceptor{ identitySpec: identitySpec, }, } - if identitySpec.IsMutable && identitySpec.IsSetOnUpdate { - interceptor.why |= Update - } - return interceptor } diff --git a/internal/provider/sdkv2/identity_interceptor_test.go b/internal/provider/sdkv2/identity_interceptor_test.go index 2c0a47e3dce3..eba5ca6a9651 100644 --- a/internal/provider/sdkv2/identity_interceptor_test.go +++ b/internal/provider/sdkv2/identity_interceptor_test.go @@ -189,25 +189,29 @@ func TestIdentityInterceptor_Update(t *testing.T) { attrName string identitySpec inttypes.Identity ExpectIdentity bool + Description string }{ - "not mutable": { + "not mutable - fresh resource": { attrName: "name", identitySpec: regionalSingleParameterizedIdentitySpec("name"), - ExpectIdentity: false, + ExpectIdentity: true, + Description: "Immutable identity with all null attributes should get populated (bug fix scenario)", }, "v6.0 SDK fix": { attrName: "name", identitySpec: regionalSingleParameterizedIdentitySpec("name", inttypes.WithV6_0SDKv2Fix(), ), - ExpectIdentity: false, + ExpectIdentity: true, + Description: "Mutable identity (v6.0 SDK fix) should always get populated on Update", }, "identity fix": { attrName: "name", identitySpec: regionalSingleParameterizedIdentitySpec("name", inttypes.WithIdentityFix(), ), - ExpectIdentity: false, + ExpectIdentity: true, + Description: "Mutable identity (identity fix) should always get populated on Update", }, "mutable": { attrName: "name", @@ -215,6 +219,7 @@ func TestIdentityInterceptor_Update(t *testing.T) { inttypes.WithMutableIdentity(), ), ExpectIdentity: true, + Description: "Explicitly mutable identity should always get populated on Update", }, } @@ -317,3 +322,82 @@ func (c mockClient) ValidateInContextRegionInPartition(ctx context.Context) erro func (c mockClient) AwsConfig(context.Context) aws.Config { // nosemgrep:ci.aws-in-func-name panic("not implemented") //lintignore:R009 } + +func TestIdentityIsFullyNull(t *testing.T) { + t.Parallel() + + identitySpec := &inttypes.Identity{ + Attributes: []inttypes.IdentityAttribute{ + inttypes.StringIdentityAttribute(names.AttrAccountID, false), + inttypes.StringIdentityAttribute(names.AttrRegion, false), + inttypes.StringIdentityAttribute(names.AttrBucket, true), + }, + } + + testCases := map[string]struct { + identityValues map[string]string + expectNull bool + description string + }{ + "all_null": { + identityValues: map[string]string{}, + expectNull: true, + description: "All attributes null should return true", + }, + "some_null": { + identityValues: map[string]string{ + names.AttrAccountID: "123456789012", + // region and bucket remain null + }, + expectNull: false, + description: "Some attributes set should return false", + }, + "all_set": { + identityValues: map[string]string{ + names.AttrAccountID: "123456789012", + names.AttrRegion: "us-west-2", // lintignore:AWSAT003 + names.AttrBucket: "test-bucket", + }, + expectNull: false, + description: "All attributes set should return false", + }, + "empty_string_values": { + identityValues: map[string]string{ + names.AttrAccountID: "", + names.AttrRegion: "", + names.AttrBucket: "", + }, + expectNull: true, + description: "Empty string values should be treated as null", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + resourceSchema := map[string]*schema.Schema{ + names.AttrBucket: {Type: schema.TypeString, Required: true}, + } + identitySchema := identity.NewIdentitySchema(*identitySpec) + d := schema.TestResourceDataWithIdentityRaw(t, resourceSchema, identitySchema, nil) + d.SetId("test-id") + + identity, err := d.Identity() + if err != nil { + t.Fatalf("unexpected error getting identity: %v", err) + } + for attrName, value := range tc.identityValues { + if err := identity.Set(attrName, value); err != nil { + t.Fatalf("unexpected error setting %s in identity: %v", attrName, err) + } + } + + result := identityIsFullyNull(d, identitySpec) + if result != tc.expectNull { + t.Errorf("%s: expected identityIsFullyNull to return %v, got %v", + tc.description, tc.expectNull, result) + } + }) + } +} diff --git a/internal/service/iam/role_test.go b/internal/service/iam/role_test.go index 4f84101404dc..dbba273a64e0 100644 --- a/internal/service/iam/role_test.go +++ b/internal/service/iam/role_test.go @@ -972,6 +972,82 @@ func TestAccIAMRole_ManagedPolicy_outOfBandAdditionRemovedEmpty(t *testing.T) { }) } +func TestAccIAMRole_Identity_ExistingResource_NoRefresh(t *testing.T) { + ctx := acctest.Context(t) + var conf awstypes.Role + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_iam_role.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, names.IAMServiceID), + CheckDestroy: testAccCheckRoleDestroy(ctx), + AdditionalCLIOptions: &resource.AdditionalCLIOptions{ + Plan: resource.PlanOptions{ + NoRefresh: true, + }, + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.100.0", + }, + }, + Config: testAccRoleConfig_basic(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckRoleExists(ctx, resourceName, &conf), + ), + }, + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccRoleConfig_description(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckRoleExists(ctx, resourceName, &conf), + ), + }, + }, + }) +} + +func TestAccIAMRole_Identity_ExistingResource_NoRefreshFailure(t *testing.T) { + ctx := acctest.Context(t) + var conf awstypes.Role + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_iam_role.test" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, names.IAMServiceID), + CheckDestroy: testAccCheckRoleDestroy(ctx), + AdditionalCLIOptions: &resource.AdditionalCLIOptions{ + Plan: resource.PlanOptions{ + NoRefresh: true, + }, + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.100.0", + }, + }, + Config: testAccRoleConfig_basic(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckRoleExists(ctx, resourceName, &conf), + ), + }, + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccRoleConfig_invalidAssumeRolePolicy(rName), + ExpectError: regexache.MustCompile(`MalformedPolicyDocument: Unknown field invalid`), + }, + }, + }) +} + func testAccCheckRoleDestroy(ctx context.Context) resource.TestCheckFunc { return func(s *terraform.State) error { conn := acctest.Provider.Meta().(*conns.AWSClient).IAMClient(ctx) @@ -1260,6 +1336,21 @@ resource "aws_iam_role" "test" { `, rName) } +func testAccRoleConfig_invalidAssumeRolePolicy(rName string) string { + return fmt.Sprintf(` +data "aws_service_principal" "ec2" { + service_name = "ec2" +} + +resource "aws_iam_role" "test" { + name = %[1]q + path = "/" + + assume_role_policy = "{\"invalid\":true}" +} +`, rName) +} + func testAccRoleConfig_diffs(rName, tags string) string { return fmt.Sprintf(` data "aws_partition" "current" {} diff --git a/internal/service/s3/object_test.go b/internal/service/s3/object_test.go index fa28a4d875eb..a8826d25400b 100644 --- a/internal/service/s3/object_test.go +++ b/internal/service/s3/object_test.go @@ -2119,6 +2119,45 @@ func TestAccS3Object_basicUpgrade(t *testing.T) { }) } +func TestAccS3Object_Identity_ExistingResource_NoRefresh(t *testing.T) { + ctx := acctest.Context(t) + var obj s3.GetObjectOutput + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + resourceName := "aws_s3_object.object" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, names.S3ServiceID), + CheckDestroy: testAccCheckObjectDestroy(ctx), + AdditionalCLIOptions: &resource.AdditionalCLIOptions{ + Plan: resource.PlanOptions{ + NoRefresh: true, + }, + }, + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.100.0", + }, + }, + Config: testAccObjectConfig_basic(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckObjectExists(ctx, resourceName, &obj), + ), + }, + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccObjectConfig_content(rName, "updated content"), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckObjectExists(ctx, resourceName, &obj), + ), + }, + }, + }) +} + func testAccCheckObjectVersionIDDiffers(first, second *s3.GetObjectOutput) resource.TestCheckFunc { return func(s *terraform.State) error { if aws.ToString(first.VersionId) == aws.ToString(second.VersionId) { diff --git a/tools/tfsdk2fw/go.mod b/tools/tfsdk2fw/go.mod index 9da7ce5369f9..0a199d7dd334 100644 --- a/tools/tfsdk2fw/go.mod +++ b/tools/tfsdk2fw/go.mod @@ -3,7 +3,7 @@ module github.com/hashicorp/terraform-provider-aws/tools/tfsdk2fw go 1.24.6 require ( - github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.0 + github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.1 github.com/hashicorp/terraform-provider-aws v1.60.1-0.20220322001452-8f7a597d0c24 ) diff --git a/tools/tfsdk2fw/go.sum b/tools/tfsdk2fw/go.sum index d2d462a11cd7..8a83fcd34eba 100644 --- a/tools/tfsdk2fw/go.sum +++ b/tools/tfsdk2fw/go.sum @@ -681,8 +681,8 @@ github.com/hashicorp/terraform-plugin-go v0.29.0 h1:1nXKl/nSpaYIUBU1IG/EsDOX0vv+ github.com/hashicorp/terraform-plugin-go v0.29.0/go.mod h1:vYZbIyvxyy0FWSmDHChCqKvI40cFTDGSb3D8D70i9GM= github.com/hashicorp/terraform-plugin-mux v0.21.0 h1:QsEYnzSD2c3zT8zUrUGqaFGhV/Z8zRUlU7FY3ZPJFfw= github.com/hashicorp/terraform-plugin-mux v0.21.0/go.mod h1:Qpt8+6AD7NmL0DS7ASkN0EXpDQ2J/FnnIgeUr1tzr5A= -github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.0 h1:PQP7Crrc7t/ozj+P9x0/lsTzGNy3lVppH8zAJylofaE= -github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.0/go.mod h1:GQhpKVvvuwzD79e8/NZ+xzj+ZpWovdPAe8nfV/skwNU= +github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.1 h1:mlAq/OrMlg04IuJT7NpefI1wwtdpWudnEmjuQs04t/4= +github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.1/go.mod h1:GQhpKVvvuwzD79e8/NZ+xzj+ZpWovdPAe8nfV/skwNU= github.com/hashicorp/terraform-plugin-testing v1.14.0-beta.1 h1:caWmY2Fv/KgDAXU7IVjcBDfIdmr/n6VRYhCLxNmlaXs= github.com/hashicorp/terraform-plugin-testing v1.14.0-beta.1/go.mod h1:jVm3pD9uQAT0X2RSEdcqjju2bCGv5f73DGZFU4v7EAU= github.com/hashicorp/terraform-registry-address v0.4.0 h1:S1yCGomj30Sao4l5BMPjTGZmCNzuv7/GDTDX99E9gTk=