query: validate identity did not change when schema version does not change

internal/terraform/context_apply_identity_test.go

@@ -43,6 +43,7 @@ func TestContext2Apply_identity(t *testing.T) {
 				"id": cty.BoolVal(false),
 			}),
 			expectDiagnostics: tfdiags.Diagnostics{
+				tfdiags.Sourceless(tfdiags.Error, "Provider produced invalid identity", "Provider \"registry.terraform.io/hashicorp/test\" returned a different identity from the one in state with the same version for test_resource.test. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
 				tfdiags.Sourceless(tfdiags.Error, "Provider produced an identity that doesn't match the schema", "Provider \"registry.terraform.io/hashicorp/test\" returned an identity for test_resource.test that doesn't match the identity schema: .id: string required, but received bool. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
 			},
 		},

internal/terraform/context_plan_identity_test.go

@@ -153,7 +153,6 @@ func TestContext2Plan_resource_identity_refresh(t *testing.T) {
 			ExpectedError:                       fmt.Errorf("failed to upgrade resource identity: provider was unable to do so"),
 		},
 		"identity sent to provider differs from returned one": {
-			// We don't throw an error here, because there are resource types with mutable identities
 			StoredIdentitySchemaVersion: 0,
 			StoredIdentityJSON:          []byte(`{"id": "foo"}`),
 			IdentitySchema: providers.IdentitySchema{
@@ -171,9 +170,7 @@ func TestContext2Plan_resource_identity_refresh(t *testing.T) {
 			IdentityData: cty.ObjectVal(map[string]cty.Value{
 				"id": cty.StringVal("bar"),
 			}),
-			ExpectedIdentity: cty.ObjectVal(map[string]cty.Value{
-				"id": cty.StringVal("bar"),
-			}),
+			ExpectedError: fmt.Errorf("Provider produced invalid identity: Provider \"registry.terraform.io/hashicorp/aws\" returned a different identity from the one in state with the same version for aws_instance.web. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
 		},
 		"identity with unknowns": {
 			IdentitySchema: providers.IdentitySchema{
@@ -538,6 +535,7 @@ func TestContext2Plan_resource_identity_plan(t *testing.T) {
 
 		"update - changing identity": {
 			// We don't throw an error here, because there are resource types with mutable identities
+			// but when they change, the identity schema version must have been incremented.
 			prevRunState: states.BuildState(func(s *states.SyncState) {
 				s.SetResourceInstanceCurrent(
 					addrs.Resource{
@@ -557,9 +555,13 @@ func TestContext2Plan_resource_identity_plan(t *testing.T) {
 					},
 				)
 			}),
+			identitySchemaVersion: 1,
 			plannedIdentity: cty.ObjectVal(map[string]cty.Value{
 				"id": cty.StringVal("foo"),
 			}),
+			upgradedIdentity: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.StringVal("foo"),
+			}),
 
 			expectedIdentity: cty.ObjectVal(map[string]cty.Value{
 				"id": cty.StringVal("foo"),
@@ -626,6 +628,32 @@ func TestContext2Plan_resource_identity_plan(t *testing.T) {
 				tfdiags.Sourceless(tfdiags.Error, "Resource instance managed by newer provider version", "The current state of test_resource.test was created by a newer provider version than is currently selected. Upgrade the test provider to work with this state."),
 			},
 		},
+		"update - updating identity to null": {
+			prevRunState: states.BuildState(func(s *states.SyncState) {
+				s.SetResourceInstanceCurrent(
+					addrs.Resource{
+						Mode: addrs.ManagedResourceMode,
+						Type: "test_resource",
+						Name: "test",
+					}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+					&states.ResourceInstanceObjectSrc{
+						Status:                states.ObjectReady,
+						AttrsJSON:             []byte(`{"id":"bar"}`),
+						IdentitySchemaVersion: 0,
+						IdentityJSON:          []byte(`{"id":"bar"}`),
+					},
+					addrs.AbsProviderConfig{
+						Provider: addrs.NewDefaultProvider("test"),
+						Module:   addrs.RootModule,
+					},
+				)
+			}),
+			identitySchemaVersion: 1,
+			upgradedIdentity:      cty.NilVal,
+			expectDiagnostics: tfdiags.Diagnostics{
+				tfdiags.Sourceless(tfdiags.Error, "Invalid resource identity upgrade", "The test provider upgraded the identity for test_resource.test from a previous version, but produced an invalid result: The returned state is null."),
+			},
+		},
 
 		"read and update": {
 			prevRunState: states.BuildState(func(s *states.SyncState) {
@@ -724,6 +752,33 @@ func TestContext2Plan_resource_identity_plan(t *testing.T) {
 				"id": cty.String,
 			})),
 		},
+		"update - identity changed but schemaversion did not": {
+			prevRunState: states.BuildState(func(s *states.SyncState) {
+				s.SetResourceInstanceCurrent(
+					addrs.Resource{
+						Mode: addrs.ManagedResourceMode,
+						Type: "test_resource",
+						Name: "test",
+					}.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance),
+					&states.ResourceInstanceObjectSrc{
+						Status:                states.ObjectReady,
+						AttrsJSON:             []byte(`{"id":"bar"}`),
+						IdentitySchemaVersion: 0,
+						IdentityJSON:          []byte(`{"id":"foo"}`),
+					},
+					addrs.AbsProviderConfig{
+						Provider: addrs.NewDefaultProvider("test"),
+						Module:   addrs.RootModule,
+					},
+				)
+			}),
+			plannedIdentity: cty.ObjectVal(map[string]cty.Value{
+				"id": cty.StringVal("bar"),
+			}),
+			expectDiagnostics: tfdiags.Diagnostics{
+				tfdiags.Sourceless(tfdiags.Error, "Provider produced invalid identity", "Provider \"registry.terraform.io/hashicorp/test\" returned a different identity from the one in state with the same version for test_resource.test. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker."),
+			},
+		},
 	} {
 		t.Run(name, func(t *testing.T) {
 			m := testModuleInline(t, map[string]string{

internal/terraform/node_resource_abstract.go

@@ -522,12 +522,6 @@ func (n *NodeAbstractResource) readResourceInstanceState(ctx EvalContext, addr a
 		return nil, diags
 	}
 
-	src, upgradeDiags = upgradeResourceIdentity(addr, provider, src, schema)
-	diags = diags.Append(upgradeDiags)
-	if diags.HasErrors() {
-		return nil, diags
-	}
-
 	obj, err := src.Decode(schema)
 	if err != nil {
 		diags = diags.Append(err)
@@ -579,12 +573,6 @@ func (n *NodeAbstractResource) readResourceInstanceStateDeposed(ctx EvalContext,
 		return nil, diags
 	}
 
-	src, upgradeDiags = upgradeResourceIdentity(addr, provider, src, schema)
-	diags = diags.Append(upgradeDiags)
-	if diags.HasErrors() {
-		return nil, diags
-	}
-
 	obj, err := src.Decode(schema)
 	if err != nil {
 		diags = diags.Append(err)

internal/terraform/node_resource_abstract_instance.go

@@ -471,7 +471,7 @@ func (n *NodeAbstractResourceInstance) planDestroy(ctx EvalContext, currentState
 
 		if !resp.PlannedIdentity.IsNull() {
 			// Destroying is an operation where we allow identity changes.
-			diags = diags.Append(n.validateIdentityKnown(resp.PlannedIdentity))
+			diags = diags.Append(n.validateIdentityKnown(currentState.Identity, resp.PlannedIdentity))
 			diags = diags.Append(n.validateIdentity(resp.PlannedIdentity, schema.Identity))
 		}
 
@@ -685,7 +685,7 @@ func (n *NodeAbstractResourceInstance) refresh(ctx EvalContext, deposedKey state
 		}
 
 		if !resp.Identity.IsNull() {
-			diags = diags.Append(n.validateIdentityKnown(resp.Identity))
+			diags = diags.Append(n.validateIdentityKnown(state.Identity, resp.Identity))
 			diags = diags.Append(n.validateIdentity(resp.Identity, schema.Identity))
 		}
 		if resp.Deferred != nil {
@@ -1136,7 +1136,7 @@ func (n *NodeAbstractResourceInstance) plan(
 
 	if !plannedIdentity.IsNull() {
 		if !action.IsReplace() && action != plans.Create {
-			diags = diags.Append(n.validateIdentityKnown(plannedIdentity))
+			diags = diags.Append(n.validateIdentityKnown(priorIdentity, plannedIdentity))
 		}
 
 		diags = diags.Append(n.validateIdentity(plannedIdentity, schema.Identity))
@@ -2658,7 +2658,7 @@ func (n *NodeAbstractResourceInstance) apply(
 		})
 
 		if !resp.NewIdentity.IsNull() {
-			diags = diags.Append(n.validateIdentityKnown(resp.NewIdentity))
+			diags = diags.Append(n.validateIdentityKnown(change.AfterIdentity, resp.NewIdentity))
 			diags = diags.Append(n.validateIdentity(resp.NewIdentity, schema.Identity))
 		}
 	}
@@ -2911,7 +2911,7 @@ func (n *NodeAbstractResourceInstance) prevRunAddr(ctx EvalContext) addrs.AbsRes
 	return resourceInstancePrevRunAddr(ctx, n.Addr)
 }
 
-func (n *NodeAbstractResourceInstance) validateIdentityKnown(newIdentity cty.Value) (diags tfdiags.Diagnostics) {
+func (n *NodeAbstractResourceInstance) validateIdentityKnown(priorIdentity, newIdentity cty.Value) (diags tfdiags.Diagnostics) {
 	if !newIdentity.IsWhollyKnown() {
 		diags = diags.Append(tfdiags.Sourceless(
 			tfdiags.Error,
@@ -2921,6 +2921,21 @@ func (n *NodeAbstractResourceInstance) validateIdentityKnown(newIdentity cty.Val
 				n.ResolvedProvider.Provider, n.Addr,
 			),
 		))
+		return
+	}
+
+	// We already performed an upgrade to the priorIdentity before we got here.
+	// Any changes to the identity should have been made in the `UpgradeResourceIdentity` RPC, not when planning / reading.
+	nonEmpty := priorIdentity != cty.NilVal && !newIdentity.IsNull()
+	if nonEmpty && priorIdentity.Equals(newIdentity).False() {
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Provider produced invalid identity",
+			fmt.Sprintf(
+				"Provider %q returned a different identity from the one in state with the same version for %s. \n\nThis is a bug in the provider, which should be reported in the provider's own issue tracker.",
+				n.ResolvedProvider.Provider, n.Addr,
+			),
+		))
 	}
 
 	return diags

internal/terraform/upgrade_resource_state.go

@@ -149,6 +149,13 @@ func upgradeResourceState(addr addrs.AbsResourceInstance, provider providers.Int
 			fmt.Sprintf("Failed to encode state for %s after resource schema upgrade: %s.", addr, tfdiags.FormatError(err)),
 		))
 	}
+
+	new, upgradeDiags := upgradeResourceIdentity(addr, provider, new, currentSchema)
+	diags = diags.Append(upgradeDiags)
+	if diags.HasErrors() {
+		return nil, diags
+	}
+
 	return new, diags
 }
 
@@ -201,6 +208,15 @@ func upgradeResourceIdentity(addr addrs.AbsResourceInstance, provider providers.
 		return nil, diags
 	}
 
+	if resp.UpgradedIdentity.IsNull() {
+		diags = diags.Append(tfdiags.Sourceless(
+			tfdiags.Error,
+			"Invalid resource identity upgrade",
+			fmt.Sprintf("The %s provider upgraded the identity for %s from a previous version, but produced an invalid result: The returned state is null.", providerType, addr),
+		))
+		return nil, diags
+	}
+
 	newIdentity := resp.UpgradedIdentity
 	newType := newIdentity.Type()
 	currentType := currentSchema.Identity.ImpliedType()