From 2651af04b74dc66bdda96b0596f771efba579a8d Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Oct 2025 11:04:42 -0400 Subject: [PATCH 1/5] Fix framework identity interceptor --- .../framework/identity_interceptor.go | 135 ++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/internal/provider/framework/identity_interceptor.go b/internal/provider/framework/identity_interceptor.go index 2048d3630360..9a5bc3aed03b 100644 --- a/internal/provider/framework/identity_interceptor.go +++ b/internal/provider/framework/identity_interceptor.go @@ -9,6 +9,8 @@ import ( "github.com/hashicorp/terraform-plugin-framework/attr" "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource" + "github.com/hashicorp/terraform-plugin-framework/tfsdk" + "github.com/hashicorp/terraform-plugin-framework/types" inttypes "github.com/hashicorp/terraform-provider-aws/internal/types" "github.com/hashicorp/terraform-provider-aws/names" ) @@ -56,6 +58,41 @@ func (r identityInterceptor) create(ctx context.Context, opts interceptorOptions } } } + case OnError: + identity := response.Identity + if identity == nil { + break + } + + if identityIsFullyNull(ctx, identity, r.attributes) { + for _, att := range r.attributes { + switch att.Name() { + case names.AttrAccountID: + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), awsClient.AccountID(ctx))...) + if opts.response.Diagnostics.HasError() { + return + } + + case names.AttrRegion: + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), awsClient.Region(ctx))...) + if opts.response.Diagnostics.HasError() { + return + } + + default: + var attrVal attr.Value + opts.response.Diagnostics.Append(response.State.GetAttribute(ctx, path.Root(att.ResourceAttributeName()), &attrVal)...) + if opts.response.Diagnostics.HasError() { + return + } + + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), attrVal)...) + if opts.response.Diagnostics.HasError() { + return + } + } + } + } } } @@ -103,11 +140,109 @@ func (r identityInterceptor) read(ctx context.Context, opts interceptorOptions[r } func (r identityInterceptor) update(ctx context.Context, opts interceptorOptions[resource.UpdateRequest, resource.UpdateResponse]) { + awsClient := opts.c + + switch response, when := opts.response, opts.when; when { + case After: + if response.State.Raw.IsNull() { + break + } + identity := response.Identity + if identity == nil { + break + } + + for _, att := range r.attributes { + switch att.Name() { + case names.AttrAccountID: + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), awsClient.AccountID(ctx))...) + if opts.response.Diagnostics.HasError() { + return + } + + case names.AttrRegion: + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), awsClient.Region(ctx))...) + if opts.response.Diagnostics.HasError() { + return + } + + default: + var attrVal attr.Value + opts.response.Diagnostics.Append(response.State.GetAttribute(ctx, path.Root(att.ResourceAttributeName()), &attrVal)...) + if opts.response.Diagnostics.HasError() { + return + } + + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), attrVal)...) + if opts.response.Diagnostics.HasError() { + return + } + } + } + case OnError: + if response.State.Raw.IsNull() { + break + } + identity := response.Identity + if identity == nil { + break + } + + if identityIsFullyNull(ctx, identity, r.attributes) { + for _, att := range r.attributes { + switch att.Name() { + case names.AttrAccountID: + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), awsClient.AccountID(ctx))...) + if opts.response.Diagnostics.HasError() { + return + } + + case names.AttrRegion: + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), awsClient.Region(ctx))...) + if opts.response.Diagnostics.HasError() { + return + } + + default: + var attrVal attr.Value + opts.response.Diagnostics.Append(response.State.GetAttribute(ctx, path.Root(att.ResourceAttributeName()), &attrVal)...) + if opts.response.Diagnostics.HasError() { + return + } + + opts.response.Diagnostics.Append(identity.SetAttribute(ctx, path.Root(att.Name()), attrVal)...) + if opts.response.Diagnostics.HasError() { + return + } + } + } + } + } } func (r identityInterceptor) delete(ctx context.Context, opts interceptorOptions[resource.DeleteRequest, resource.DeleteResponse]) { } +// identityIsFullyNull returns true if a resource supports identity and +// all attributes are set to null values +func identityIsFullyNull(ctx context.Context, identity *tfsdk.ResourceIdentity, attributes []inttypes.IdentityAttribute) bool { + if identity == nil { + return true + } + + for _, attr := range attributes { + var attrVal types.String + if diags := identity.GetAttribute(ctx, path.Root(attr.Name()), &attrVal); diags.HasError() { + return false + } + if !attrVal.IsNull() && attrVal.ValueString() != "" { + return false + } + } + + return true +} + func newIdentityInterceptor(attributes []inttypes.IdentityAttribute) identityInterceptor { return identityInterceptor{ attributes: attributes, From 73b027404886a3d313f62126cc8fd6e007b2d72a Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 1 Oct 2025 11:17:03 -0400 Subject: [PATCH 2/5] Add changelog --- .changelog/44518.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/44518.txt diff --git a/.changelog/44518.txt b/.changelog/44518.txt new file mode 100644 index 000000000000..ef134d25a620 --- /dev/null +++ b/.changelog/44518.txt @@ -0,0 +1,3 @@ +```release-note:bug +provider: Fix `Missing Resource Identity After Create` errors for framework resources +``` \ No newline at end of file From ab11f940d3ac70ff1179cdadf8fbaf4316c73139 Mon Sep 17 00:00:00 2001 From: Dirk Avery <31492422+YakDriver@users.noreply.github.com> Date: Wed, 8 Oct 2025 10:20:02 -0400 Subject: [PATCH 3/5] Update .changelog/44518.txt Co-authored-by: Jared Baker --- .changelog/44518.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.changelog/44518.txt b/.changelog/44518.txt index ef134d25a620..aed0c87ec78f 100644 --- a/.changelog/44518.txt +++ b/.changelog/44518.txt @@ -1,3 +1,6 @@ ```release-note:bug -provider: Fix `Missing Resource Identity After Create` errors for framework resources +provider: Fix `Missing Resource Identity After Update` errors for non-refreshed and failed updates of Plugin Framework based resources +``` +```release-note:bug +provider: Fix `Unexpected Identity Change` errors when fully-null identity values in state are updated to valid values for Plugin Framework based resources ``` \ No newline at end of file From ae4b153db3abdd70ae5ff9f59f0c8cd62e018e67 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 8 Oct 2025 10:41:12 -0400 Subject: [PATCH 4/5] Add unit tests --- .../framework/identity_interceptor_test.go | 93 +++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/internal/provider/framework/identity_interceptor_test.go b/internal/provider/framework/identity_interceptor_test.go index bd53b564d578..67fcece3fa4c 100644 --- a/internal/provider/framework/identity_interceptor_test.go +++ b/internal/provider/framework/identity_interceptor_test.go @@ -269,3 +269,96 @@ 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() + + attributes := []inttypes.IdentityAttribute{ + inttypes.StringIdentityAttribute("account_id", false), + inttypes.StringIdentityAttribute("region", false), + inttypes.StringIdentityAttribute("bucket", 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{ + "account_id": "123456789012", + // region and bucket remain null + }, + expectNull: false, + description: "Some attributes set should return false", + }, + "all_set": { + identityValues: map[string]string{ + "account_id": "123456789012", + "region": "us-west-2", // lintignore:AWSAT003 + "bucket": "test-bucket", + }, + expectNull: false, + description: "All attributes set should return false", + }, + "empty_string_values": { + identityValues: map[string]string{ + "account_id": "", + "region": "", + "bucket": "", + }, + expectNull: true, + description: "Empty string values should be treated as null", + }, + "nil_identity": { + identityValues: nil, // This will result in nil identity + expectNull: true, + description: "Nil identity should return true", + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + + // Create identity schema + identitySchema := &identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "account_id": identityschema.StringAttribute{}, + "region": identityschema.StringAttribute{}, + "bucket": identityschema.StringAttribute{}, + }, + } + + var identity *tfsdk.ResourceIdentity + + // Handle nil identity case + if tc.identityValues == nil { + identity = nil + } else { + // Create identity with values + identity = emtpyIdentityFromSchema(ctx, identitySchema) + for attrName, value := range tc.identityValues { + if value != "" { + diags := identity.SetAttribute(ctx, path.Root(attrName), value) + if diags.HasError() { + t.Fatalf("unexpected error setting %s in identity: %s", attrName, fwdiag.DiagnosticsError(diags)) + } + } + } + } + + result := identityIsFullyNull(ctx, identity, attributes) + if result != tc.expectNull { + t.Errorf("%s: expected identityIsFullyNull to return %v, got %v", + tc.description, tc.expectNull, result) + } + }) + } +} From 4252f6480bfcd04f4bb79f6076256a194705da89 Mon Sep 17 00:00:00 2001 From: Dirk Avery Date: Wed, 8 Oct 2025 13:10:05 -0400 Subject: [PATCH 5/5] Cleaner test structure, reduce complexity --- .../framework/identity_interceptor_test.go | 87 ++++++++++--------- 1 file changed, 44 insertions(+), 43 deletions(-) diff --git a/internal/provider/framework/identity_interceptor_test.go b/internal/provider/framework/identity_interceptor_test.go index 67fcece3fa4c..26c3206cf1da 100644 --- a/internal/provider/framework/identity_interceptor_test.go +++ b/internal/provider/framework/identity_interceptor_test.go @@ -279,46 +279,74 @@ func TestIdentityIsFullyNull(t *testing.T) { inttypes.StringIdentityAttribute("bucket", true), } + // Create identity schema once for all test cases + identitySchema := &identityschema.Schema{ + Attributes: map[string]identityschema.Attribute{ + "account_id": identityschema.StringAttribute{}, + "region": identityschema.StringAttribute{}, + "bucket": identityschema.StringAttribute{}, + }, + } + + ctx := context.Background() + + // Helper function to create identity with values + createIdentityWithValues := func(values map[string]string) *tfsdk.ResourceIdentity { + if values == nil { + return nil + } + identity := emtpyIdentityFromSchema(ctx, identitySchema) + for attrName, value := range values { + if value != "" { + diags := identity.SetAttribute(ctx, path.Root(attrName), value) + if diags.HasError() { + t.Fatalf("unexpected error setting %s in identity: %s", attrName, fwdiag.DiagnosticsError(diags)) + } + } + } + return identity + } + testCases := map[string]struct { - identityValues map[string]string - expectNull bool - description string + identity *tfsdk.ResourceIdentity + expectNull bool + description string }{ "all_null": { - identityValues: map[string]string{}, - expectNull: true, - description: "All attributes null should return true", + identity: createIdentityWithValues(map[string]string{}), + expectNull: true, + description: "All attributes null should return true", }, "some_null": { - identityValues: map[string]string{ + identity: createIdentityWithValues(map[string]string{ "account_id": "123456789012", // region and bucket remain null - }, + }), expectNull: false, description: "Some attributes set should return false", }, "all_set": { - identityValues: map[string]string{ + identity: createIdentityWithValues(map[string]string{ "account_id": "123456789012", "region": "us-west-2", // lintignore:AWSAT003 "bucket": "test-bucket", - }, + }), expectNull: false, description: "All attributes set should return false", }, "empty_string_values": { - identityValues: map[string]string{ + identity: createIdentityWithValues(map[string]string{ "account_id": "", "region": "", "bucket": "", - }, + }), expectNull: true, description: "Empty string values should be treated as null", }, "nil_identity": { - identityValues: nil, // This will result in nil identity - expectNull: true, - description: "Nil identity should return true", + identity: createIdentityWithValues(nil), + expectNull: true, + description: "Nil identity should return true", }, } @@ -327,34 +355,7 @@ func TestIdentityIsFullyNull(t *testing.T) { t.Parallel() ctx := context.Background() - // Create identity schema - identitySchema := &identityschema.Schema{ - Attributes: map[string]identityschema.Attribute{ - "account_id": identityschema.StringAttribute{}, - "region": identityschema.StringAttribute{}, - "bucket": identityschema.StringAttribute{}, - }, - } - - var identity *tfsdk.ResourceIdentity - - // Handle nil identity case - if tc.identityValues == nil { - identity = nil - } else { - // Create identity with values - identity = emtpyIdentityFromSchema(ctx, identitySchema) - for attrName, value := range tc.identityValues { - if value != "" { - diags := identity.SetAttribute(ctx, path.Root(attrName), value) - if diags.HasError() { - t.Fatalf("unexpected error setting %s in identity: %s", attrName, fwdiag.DiagnosticsError(diags)) - } - } - } - } - - result := identityIsFullyNull(ctx, identity, attributes) + result := identityIsFullyNull(ctx, tc.identity, attributes) if result != tc.expectNull { t.Errorf("%s: expected identityIsFullyNull to return %v, got %v", tc.description, tc.expectNull, result)