Skip to content

Commit

Permalink
fix: Allow WriteOnly attributes to require replacement
Browse files Browse the repository at this point in the history
  • Loading branch information
radeksimko committed Jan 14, 2025
1 parent 8a5ffdb commit 4bcee80
Show file tree
Hide file tree
Showing 4 changed files with 241 additions and 5 deletions.
37 changes: 37 additions & 0 deletions internal/configs/configschema/implied_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
//
Expand Down Expand Up @@ -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
}
92 changes: 92 additions & 0 deletions internal/configs/configschema/write_only.go
Original file line number Diff line number Diff line change
@@ -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
}
92 changes: 92 additions & 0 deletions internal/terraform/context_plan2_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 20 additions & 5 deletions internal/terraform/node_resource_abstract_instance.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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():
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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)
}
}
Expand Down

0 comments on commit 4bcee80

Please sign in to comment.