Merge pull request #44375 from hashicorp/f-resource-identity-intercept-fix

Update identity interceptor

Author: YakDriver

.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
+```

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

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=

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
 }
 

internal/provider/sdkv2/identity_interceptor_test.go

@@ -189,32 +189,37 @@ 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",
 			identitySpec: regionalSingleParameterizedIdentitySpec("name",
 				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)
+			}
+		})
+	}
+}

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" {}