From 4bcee807ff3f1bf8fb5d50de77e858ccb619b7ec Mon Sep 17 00:00:00 2001 From: Radek Simko Date: Mon, 13 Jan 2025 13:42:47 +0000 Subject: [PATCH] fix: Allow WriteOnly attributes to require replacement --- internal/configs/configschema/implied_type.go | 37 ++++++++ internal/configs/configschema/write_only.go | 92 +++++++++++++++++++ internal/terraform/context_plan2_test.go | 92 +++++++++++++++++++ .../node_resource_abstract_instance.go | 25 ++++- 4 files changed, 241 insertions(+), 5 deletions(-) create mode 100644 internal/configs/configschema/write_only.go diff --git a/internal/configs/configschema/implied_type.go b/internal/configs/configschema/implied_type.go index 90bd06da15b8..4affa6eaa8b0 100644 --- a/internal/configs/configschema/implied_type.go +++ b/internal/configs/configschema/implied_type.go @@ -59,6 +59,29 @@ func (b *Block) ContainsSensitive() bool { return false } +// ContainsWriteOnly returns true if any of the attributes of the receiving +// block or any of its descendant blocks are considered write only +// based on the declarations in the schema. +// +// Blocks themselves cannot be write only as a whole -- write only is a +// per-attribute idea. +func (b *Block) ContainsWriteOnly() bool { + for _, attrS := range b.Attributes { + if attrS.WriteOnly { + return true + } + if attrS.NestedType != nil && attrS.NestedType.ContainsWriteOnly() { + return true + } + } + for _, blockS := range b.BlockTypes { + if blockS.ContainsWriteOnly() { + return true + } + } + return false +} + // ImpliedType returns the cty.Type that would result from decoding a Block's // ImpliedType and getting the resulting AttributeType. // @@ -134,3 +157,17 @@ func (o *Object) ContainsSensitive() bool { } return false } + +// ContainsWriteOnly returns true if any of the attributes of the receiving +// Object are considered write only based on the declarations in the schema. +func (o *Object) ContainsWriteOnly() bool { + for _, attrS := range o.Attributes { + if attrS.WriteOnly { + return true + } + if attrS.NestedType != nil && attrS.NestedType.ContainsWriteOnly() { + return true + } + } + return false +} diff --git a/internal/configs/configschema/write_only.go b/internal/configs/configschema/write_only.go new file mode 100644 index 000000000000..2a44dca7fca2 --- /dev/null +++ b/internal/configs/configschema/write_only.go @@ -0,0 +1,92 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package configschema + +import ( + "fmt" + + "github.com/zclconf/go-cty/cty" +) + +// WriteOnlyPaths returns a set of paths into the given value that +// are considered write only based on the declarations in the schema. +func (b *Block) WriteOnlyPaths(val cty.Value, basePath cty.Path) []cty.Path { + var ret []cty.Path + + for name, attrS := range b.Attributes { + if attrS.WriteOnly { + attrPath := copyAndExtendPath(basePath, cty.GetAttrStep{Name: name}) + ret = append(ret, attrPath) + } + + if attrS.NestedType == nil || !attrS.NestedType.ContainsWriteOnly() { + continue + } + + attrPath := copyAndExtendPath(basePath, cty.GetAttrStep{Name: name}) + ret = append(ret, attrS.NestedType.WriteOnlyPaths(attrPath)...) + } + + // Extract from nested blocks + for name, blockS := range b.BlockTypes { + // If our block doesn't contain any write only attributes, skip inspecting it + if !blockS.Block.ContainsWriteOnly() { + continue + } + + blockV := val.GetAttr(name) + + // Create a copy of the path, with this step added, to add to our PathValueMarks slice + blockPath := copyAndExtendPath(basePath, cty.GetAttrStep{Name: name}) + + switch blockS.Nesting { + case NestingSingle, NestingGroup: + ret = append(ret, blockS.Block.WriteOnlyPaths(blockV, blockPath)...) + case NestingList, NestingMap, NestingSet: + blockV, _ = blockV.Unmark() // peel off one level of marking so we can iterate + for it := blockV.ElementIterator(); it.Next(); { + idx, blockEV := it.Element() + + blockInstancePath := copyAndExtendPath(blockPath, cty.IndexStep{Key: idx}) + morePaths := blockS.Block.WriteOnlyPaths(blockEV, blockInstancePath) + ret = append(ret, morePaths...) + } + default: + panic(fmt.Sprintf("unsupported nesting mode %s", blockS.Nesting)) + } + } + return ret +} + +// WriteOnlyPaths returns a set of paths into the given value that +// are considered write only based on the declarations in the schema. +func (o *Object) WriteOnlyPaths(basePath cty.Path) []cty.Path { + var ret []cty.Path + + for name, attrS := range o.Attributes { + // Create a path to this attribute + attrPath := copyAndExtendPath(basePath, cty.GetAttrStep{Name: name}) + + switch o.Nesting { + case NestingSingle, NestingGroup: + if attrS.WriteOnly { + ret = append(ret, attrPath) + } else { + // The attribute has a nested type which contains write only + // attributes, so recurse + ret = append(ret, attrS.NestedType.WriteOnlyPaths(attrPath)...) + } + case NestingList, NestingMap, NestingSet: + // If the attribute is iterable type we assume that + // it is write only in its entirety since we cannot + // construct indexes from a null value. + if attrS.WriteOnly { + ret = append(ret, attrPath) + } + default: + panic(fmt.Sprintf("unsupported nesting mode %s", attrS.NestedType.Nesting)) + } + } + return ret +} diff --git a/internal/terraform/context_plan2_test.go b/internal/terraform/context_plan2_test.go index e3bf573c72c8..f93c620c6022 100644 --- a/internal/terraform/context_plan2_test.go +++ b/internal/terraform/context_plan2_test.go @@ -5831,6 +5831,98 @@ resource "test_object" "obj" { } } +func TestContext2Plan_writeOnlyRequiredReplace(t *testing.T) { + m := testModuleInline(t, map[string]string{ + "main.tf": ` +resource "test_object" "obj" { + value = "changed" +} +`, + }) + p := new(testing_provider.MockProvider) + p.GetProviderSchemaResponse = &providers.GetProviderSchemaResponse{ + ResourceTypes: map[string]providers.Schema{ + "test_object": { + Block: &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "value": { + Type: cty.String, + Required: true, + WriteOnly: true, + }, + }, + }, + }, + }, + } + p.PlanResourceChangeResponse = &providers.PlanResourceChangeResponse{ + PlannedState: cty.ObjectVal(map[string]cty.Value{ + "value": cty.NullVal(cty.String), + }), + RequiresReplace: []cty.Path{ + cty.GetAttrPath("value"), + }, + } + + ctx := testContext2(t, &ContextOpts{ + Providers: map[addrs.Provider]providers.Factory{ + addrs.NewDefaultProvider("test"): testProviderFuncFixed(p), + }, + }) + + rAddr := mustResourceInstanceAddr("test_object.obj") + pAddr := mustProviderConfig(`provider["registry.terraform.io/hashicorp/test"]`) + state := states.BuildState(func(s *states.SyncState) { + s.SetResourceInstanceCurrent( + rAddr, + &states.ResourceInstanceObjectSrc{ + AttrsJSON: mustParseJson(map[string]any{ + "value": nil, + }), + Status: states.ObjectReady, + }, + pAddr) + }) + + plan, diags := ctx.Plan(m, state, nil) + assertNoErrors(t, diags) + + if plan.Changes.Empty() { + t.Fatal("unexpected empty plan") + } + expectedChanges := &plans.Changes{ + Resources: []*plans.ResourceInstanceChange{ + { + Addr: rAddr, + PrevRunAddr: rAddr, + ProviderAddr: pAddr, + Change: plans.Change{ + Action: plans.DeleteThenCreate, + Before: cty.ObjectVal(map[string]cty.Value{ + "value": cty.NullVal(cty.String), + }), + After: cty.ObjectVal(map[string]cty.Value{ + "value": cty.NullVal(cty.String), + }), + }, + ActionReason: plans.ResourceInstanceReplaceBecauseCannotUpdate, + RequiredReplace: cty.NewPathSet(cty.GetAttrPath("value")), + }, + }, + } + + schemas, schemaDiags := ctx.Schemas(m, plan.PriorState) + assertNoDiagnostics(t, schemaDiags) + + changes, err := plan.Changes.Decode(schemas) + if err != nil { + t.Fatal(err) + } + if diff := cmp.Diff(expectedChanges, changes, ctydebug.CmpOptions); diff != "" { + t.Fatalf("unexpected changes: %s", diff) + } +} + func TestContext2Plan_selfReferences(t *testing.T) { tcs := []struct { attribute string diff --git a/internal/terraform/node_resource_abstract_instance.go b/internal/terraform/node_resource_abstract_instance.go index d6a45849f53d..b0ccc621828b 100644 --- a/internal/terraform/node_resource_abstract_instance.go +++ b/internal/terraform/node_resource_abstract_instance.go @@ -1071,13 +1071,16 @@ func (n *NodeAbstractResourceInstance) plan( plannedNewVal = marks.MarkPaths(plannedNewVal, marks.Sensitive, sensitivePaths) } - reqRep, reqRepDiags := getRequiredReplaces(unmarkedPriorVal, unmarkedPlannedNewVal, resp.RequiresReplace, n.ResolvedProvider.Provider, n.Addr) + writeOnlyPaths := schema.WriteOnlyPaths(plannedNewVal, nil) + + reqRep, reqRepDiags := getRequiredReplaces(unmarkedPriorVal, unmarkedPlannedNewVal, writeOnlyPaths, resp.RequiresReplace, n.ResolvedProvider.Provider, n.Addr) diags = diags.Append(reqRepDiags) if diags.HasErrors() { return nil, nil, deferred, keyData, diags } - action, actionReason := getAction(n.Addr, unmarkedPriorVal, unmarkedPlannedNewVal, createBeforeDestroy, forceReplace, reqRep) + woPathSet := cty.NewPathSet(writeOnlyPaths...) + action, actionReason := getAction(n.Addr, unmarkedPriorVal, unmarkedPlannedNewVal, createBeforeDestroy, woPathSet, forceReplace, reqRep) if action.IsReplace() { // In this strange situation we want to produce a change object that @@ -2813,7 +2816,7 @@ func resourceInstancePrevRunAddr(ctx EvalContext, currentAddr addrs.AbsResourceI return table.OldAddr(currentAddr) } -func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value, createBeforeDestroy bool, forceReplace []addrs.AbsResourceInstance, reqRep cty.PathSet) (action plans.Action, actionReason plans.ResourceInstanceChangeActionReason) { +func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value, createBeforeDestroy bool, writeOnly cty.PathSet, forceReplace []addrs.AbsResourceInstance, reqRep cty.PathSet) (action plans.Action, actionReason plans.ResourceInstanceChangeActionReason) { // The user might also ask us to force replacing a particular resource // instance, regardless of whether the provider thinks it needs replacing. // For example, users typically do this if they learn a particular object @@ -2840,6 +2843,9 @@ func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value switch { case priorVal.IsNull(): action = plans.Create + case !writeOnly.Intersection(reqRep).Empty(): + action = plans.DeleteThenCreate + actionReason = plans.ResourceInstanceReplaceBecauseCannotUpdate case eq && !matchedForceReplace: action = plans.NoOp case matchedForceReplace || !reqRep.Empty(): @@ -2880,7 +2886,7 @@ func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value // function. This function exposes nothing about the priorVal or plannedVal // except for the paths that require replacement which can be deduced from the // type with or without marks. -func getRequiredReplaces(priorVal, plannedNewVal cty.Value, requiredReplaces []cty.Path, providerAddr tfaddr.Provider, addr addrs.AbsResourceInstance) (cty.PathSet, tfdiags.Diagnostics) { +func getRequiredReplaces(priorVal, plannedNewVal cty.Value, writeOnly []cty.Path, requiredReplaces []cty.Path, providerAddr tfaddr.Provider, addr addrs.AbsResourceInstance) (cty.PathSet, tfdiags.Diagnostics) { var diags tfdiags.Diagnostics reqRep := cty.NewPathSet() @@ -2925,7 +2931,16 @@ func getRequiredReplaces(priorVal, plannedNewVal cty.Value, requiredReplaces []c } eqV := plannedChangedVal.Equals(priorChangedVal) - if !eqV.IsKnown() || eqV.False() { + + // if attribute/path is writeOnly we have no values to compare + // but still respect the required replacement + isWriteOnly := false + for _, woPath := range writeOnly { + if path.Equals(woPath) { + isWriteOnly = true + } + } + if !eqV.IsKnown() || eqV.False() || isWriteOnly { reqRep.Add(path) } }