diff --git a/internal/terraform/context_apply_identity_test.go b/internal/terraform/context_apply_identity_test.go index d7d0a9802f54..35ec21db78d1 100644 --- a/internal/terraform/context_apply_identity_test.go +++ b/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."), }, }, diff --git a/internal/terraform/context_plan_identity_test.go b/internal/terraform/context_plan_identity_test.go index 1c6a573f7091..013735f393f1 100644 --- a/internal/terraform/context_plan_identity_test.go +++ b/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{ diff --git a/internal/terraform/node_resource_abstract.go b/internal/terraform/node_resource_abstract.go index f4787cb30ef9..90ea04f684d2 100644 --- a/internal/terraform/node_resource_abstract.go +++ b/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) diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index e15a5593c332..3a50b6bdfc49 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/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 diff --git a/internal/terraform/upgrade_resource_state.go b/internal/terraform/upgrade_resource_state.go index 5529fc557410..c40dcfb3652b 100644 --- a/internal/terraform/upgrade_resource_state.go +++ b/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()