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 13, 2025
1 parent 8a5ffdb commit 66bbae0
Show file tree
Hide file tree
Showing 4 changed files with 246 additions and 5 deletions.
29 changes: 29 additions & 0 deletions internal/configs/configschema/implied_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,23 @@ func (b *Block) ContainsSensitive() bool {
return false
}

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 +151,15 @@ func (o *Object) ContainsSensitive() bool {
}
return false
}

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
}
103 changes: 103 additions & 0 deletions internal/configs/configschema/marks.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,3 +140,106 @@ func (o *Object) SensitivePaths(val cty.Value, basePath cty.Path) []cty.Path {
}
return ret
}

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)
}
}

for name, attrS := range b.Attributes {
// If the attribute has no nested type, or the nested type doesn't
// contain any write only attributes, skip inspecting it
if attrS.NestedType == nil || !attrS.NestedType.ContainsWriteOnly() {
continue
}

// Create a copy of the path, with this step added, to add to our PathValueMarks slice
attrPath := copyAndExtendPath(basePath, cty.GetAttrStep{Name: name})
ret = append(ret, attrS.NestedType.WriteOnlyPaths(val.GetAttr(name), 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)
if blockV.IsNull() || !blockV.IsKnown() {
continue
}

// 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()
// Create a copy of the path, with this block instance's index
// step added, to add to our PathValueMarks slice
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
}

func (o *Object) WriteOnlyPaths(val cty.Value, basePath cty.Path) []cty.Path {
var ret []cty.Path

for name, attrS := range o.Attributes {
switch o.Nesting {
case NestingSingle, NestingGroup:
// Create a path to this attribute
attrPath := copyAndExtendPath(basePath, cty.GetAttrStep{Name: name})

if attrS.WriteOnly {
// If the entire attribute is write only, mark it so
ret = append(ret, attrPath)
} else {
// The attribute has a nested type which contains write only
// attributes, so recurse
ret = append(ret, attrS.NestedType.WriteOnlyPaths(val.GetAttr(name), attrPath)...)
}
case NestingList, NestingMap, NestingSet:
// For nested attribute types which have a non-single nesting mode,
// we add path value marks for each element of the collection
val, _ = val.Unmark() // peel off one level of marking so we can iterate
for it := val.ElementIterator(); it.Next(); {
idx, attrEV := it.Element()
attrV := attrEV.GetAttr(name)

// Create a path to this element of the attribute's collection. Note
// that the path is extended in opposite order to the iteration order
// of the loops: index into the collection, then the contained
// attribute name. This is because we have one type
// representing multiple collection elements.
attrPath := copyAndExtendPath(basePath, cty.IndexStep{Key: idx}, cty.GetAttrStep{Name: name})

if attrS.WriteOnly {
// If the entire attribute is write only, mark it so
ret = append(ret, attrPath)
} else {
ret = append(ret, attrS.NestedType.WriteOnlyPaths(attrV, 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
27 changes: 22 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(unmarkedPlannedNewVal, 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 @@ -2837,9 +2840,14 @@ func getAction(addr addrs.AbsResourceInstance, priorVal, plannedNewVal cty.Value
eqV := plannedNewVal.Equals(priorVal)
eq := eqV.IsKnown() && eqV.True()

woPaths := writeOnly.Intersection(reqRep)

switch {
case priorVal.IsNull():
action = plans.Create
case !woPaths.Empty():
action = plans.DeleteThenCreate
actionReason = plans.ResourceInstanceReplaceBecauseCannotUpdate
case eq && !matchedForceReplace:
action = plans.NoOp
case matchedForceReplace || !reqRep.Empty():
Expand Down Expand Up @@ -2880,7 +2888,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 +2933,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 66bbae0

Please sign in to comment.