From 28cb1307393a2a6a0d1600955e17cd585e1aa7b8 Mon Sep 17 00:00:00 2001 From: Daniel Banck Date: Tue, 5 Aug 2025 12:00:31 +0200 Subject: [PATCH] Fix resource identity being dropped from state in certain cases (#37396) * Test case for keeping identity on destroy error * Add identity to resource instance object on error * Add changelog * Test case for keeping identity on update without changes * Add identity to resource instance object on udpate --- .changes/v1.13/BUG FIXES-20250804-162137.yaml | 5 + internal/terraform/context_apply2_test.go | 189 ++++++++++++++++++ internal/terraform/context_test.go | 1 + .../node_resource_abstract_instance.go | 2 + 4 files changed, 197 insertions(+) create mode 100644 .changes/v1.13/BUG FIXES-20250804-162137.yaml diff --git a/.changes/v1.13/BUG FIXES-20250804-162137.yaml b/.changes/v1.13/BUG FIXES-20250804-162137.yaml new file mode 100644 index 0000000000..2a9dbd238f --- /dev/null +++ b/.changes/v1.13/BUG FIXES-20250804-162137.yaml @@ -0,0 +1,5 @@ +kind: BUG FIXES +body: Fixes resource identity being dropped from state in certain cases +time: 2025-08-04T16:21:37.590435+02:00 +custom: + Issue: "37396" diff --git a/internal/terraform/context_apply2_test.go b/internal/terraform/context_apply2_test.go index c29d7531f8..098288715d 100644 --- a/internal/terraform/context_apply2_test.go +++ b/internal/terraform/context_apply2_test.go @@ -4082,3 +4082,192 @@ func TestContext2Apply_excludeListResources(t *testing.T) { t.Fatal("expected error") } } + +func TestContext2Apply_errorDestroyWithIdentity(t *testing.T) { + m := testModule(t, "empty") + p := testProvider("test") + + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&providerSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Optional: true}, + }, + }, + }, + IdentityTypes: map[string]*configschema.Object{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Required: true, + }, + }, + Nesting: configschema.NestingSingle, + }, + }, + }) + p.PlanResourceChangeFn = func(req providers.PlanResourceChangeRequest) providers.PlanResourceChangeResponse { + // Should actually be called for this test, because Terraform Core + // constructs the plan for a destroy operation itself. + return providers.PlanResourceChangeResponse{ + PlannedState: req.ProposedNewState, + } + } + value := cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("baz"), + }) + identity := cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("baz"), + }) + p.ApplyResourceChangeFn = func(req providers.ApplyResourceChangeRequest) providers.ApplyResourceChangeResponse { + // The apply (in this case, a destroy) always fails, so we can verify + // that the object stays in the state after a destroy fails even though + // we aren't returning a new state object here. + return providers.ApplyResourceChangeResponse{ + NewState: value, + NewIdentity: identity, + Diagnostics: tfdiags.Diagnostics(nil).Append(fmt.Errorf("failed")), + } + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + state := states.BuildState(func(ss *states.SyncState) { + ss.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":"baz"}`), + IdentityJSON: []byte(`{"id":"baz"}`), + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + }) + plan, diags := ctx.Plan(m, state, DefaultPlanOpts) + tfdiags.AssertNoErrors(t, diags) + + state, diags = ctx.Apply(plan, m, nil) + if !diags.HasErrors() { + t.Fatal("should have error") + } + + schema := p.GetProviderSchemaResponse.ResourceTypes["test_resource"] + resourceInstanceStateSrc := state.Modules[""].Resources["test_resource.test"].Instance(addrs.NoKey).Current + resourceInstanceState, err := resourceInstanceStateSrc.Decode(schema) + if err != nil { + t.Fatalf("failed to decode resource instance state: %s", err) + } + + if !resourceInstanceState.Value.RawEquals(value) { + t.Fatalf("expected value to still be present in state, but got: %s", resourceInstanceState.Value.GoString()) + } + if !resourceInstanceState.Identity.RawEquals(identity) { + t.Fatalf("expected identity to still be present in state, but got: %s", resourceInstanceState.Identity.GoString()) + } +} + +func TestContext2Apply_SensitivityChangeWithIdentity(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +variable "sensitive_var" { + default = "hello" + sensitive = true +} + +resource "test_resource" "foo" { + value = var.sensitive_var +}`, + }) + + p := testProvider("test") + p.GetProviderSchemaResponse = getProviderSchemaResponseFromProviderSchema(&providerSchema{ + ResourceTypes: map[string]*configschema.Block{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": {Type: cty.String, Computed: true}, + "value": {Type: cty.String, Optional: true}, + "sensitive_value": {Type: cty.String, Sensitive: true, Optional: true}, + }, + }, + }, + IdentityTypes: map[string]*configschema.Object{ + "test_resource": { + Attributes: map[string]*configschema.Attribute{ + "id": { + Type: cty.String, + Required: true, + }, + }, + Nesting: configschema.NestingSingle, + }, + }, + }) + p.PlanResourceChangeFn = testDiffFn + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + addrs.Resource{ + Mode: addrs.ManagedResourceMode, + Type: "test_resource", + Name: "foo", + }.Instance(addrs.NoKey).Absolute(addrs.RootModuleInstance), + &states.ResourceInstanceObjectSrc{ + Status: states.ObjectReady, + AttrsJSON: []byte(`{"id":"foo", "value":"hello"}`), + IdentityJSON: []byte(`{"id":"baz"}`), + // No AttrSensitivePaths present + }, + addrs.AbsProviderConfig{ + Provider: addrs.NewDefaultProvider("test"), + Module: addrs.RootModule, + }, + ) + }) + + plan, diags := ctx.Plan(m, state, SimplePlanOpts(plans.NormalMode, testInputValuesUnset(m.Module.Variables))) + tfdiags.AssertNoErrors(t, diags) + + addr := mustResourceInstanceAddr("test_resource.foo") + + state, diags = ctx.Apply(plan, m, nil) + tfdiags.AssertNoErrors(t, diags) + + fooState := state.ResourceInstance(addr) + + if len(fooState.Current.AttrSensitivePaths) != 2 { + t.Fatalf("wrong number of sensitive paths, expected 2, got, %v", len(fooState.Current.AttrSensitivePaths)) + } + + for _, path := range fooState.Current.AttrSensitivePaths { + switch { + case path.Equals(cty.GetAttrPath("value")): + case path.Equals(cty.GetAttrPath("sensitive_value")): + default: + t.Errorf("unexpected sensitive path: %#v", path) + return + } + } + + expectedIdentity := `{"id":"baz"}` + if string(fooState.Current.IdentityJSON) != expectedIdentity { + t.Fatalf("missing identity in state, got %q", fooState.Current.IdentityJSON) + } +} diff --git a/internal/terraform/context_test.go b/internal/terraform/context_test.go index 99075894df..520b39705e 100644 --- a/internal/terraform/context_test.go +++ b/internal/terraform/context_test.go @@ -443,6 +443,7 @@ func testDiffFn(req providers.PlanResourceChangeRequest) (resp providers.PlanRes } resp.PlannedState = cty.ObjectVal(planned) + resp.PlannedIdentity = req.PriorIdentity return } diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index 816928c0a8..c6f1930103 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -2626,6 +2626,7 @@ func (n *NodeAbstractResourceInstance) apply( Private: state.Private, Status: state.Status, Value: change.After, + Identity: change.AfterIdentity, } return newState, diags } @@ -2883,6 +2884,7 @@ func (n *NodeAbstractResourceInstance) apply( Value: newVal, Private: resp.Private, CreateBeforeDestroy: createBeforeDestroy, + Identity: resp.NewIdentity, } // if the resource was being deleted, the dependencies are not going to