Skip to content

Commit

Permalink
plans/objchange: Support nested unknowns in our unrefinedValue shim
Browse files Browse the repository at this point in the history
Several parts of the objchange logic incorrectly use cty.Value.RawEquals
for value comparison, instead of more appropriate comparison methods like
cty.Value.Equals or c.Value.Range().Includes. That makes them incorrectly
consider two unknown values with the same type but different refinements
as always non-equal, rather than evaluating based on the overlap between
the refinements (if any).

As a short-term fix for that we previously added this unrefinedValue shim
that just strips away the refinements for comparison, thus allowing
callers to continue using RawEquals as long as they've already taken care
of all of the other things that can make that go wrong, such as value
marks.

Unfortunately the shim was too simplistic and only supported direct
unknown values. Unknown values with refinements can also appear nested
inside known container values such as collections, so the shim needs to
recursively un-refine the entire data structure in that case.

This is still intended only as a temporary fix until we have time to
revisit all of the callers and make them use cty's own logic for
comparison. Using cty's own logic will make the results more precise,
because e.g. it can notice if two unknown strings have different known
prefixes and therefore cannot possibly be equal despite not being fully
known. For now this shim will accept any pair of unknown values of the
same type as equal, regardless of refinement.
  • Loading branch information
apparentlymart committed Jun 22, 2023
1 parent cbebace commit d49e991
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 4 deletions.
18 changes: 14 additions & 4 deletions internal/plans/objchange/plan_valid.go
Original file line number Diff line number Diff line change
Expand Up @@ -465,9 +465,19 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne

// unrefinedValue returns the given value with any unknown value refinements
// stripped away, making it a basic unknown value with only a type constraint.
//
// This function also considers unknown values nested inside a known container
// such as a collection, which unfortunately makes it relatively expensive
// for large data structures. Over time we should transition away from using
// this trick and prefer to use cty's Equals and value range APIs instead of
// of using Value.RawEquals, which is primarily intended for unit test code
// rather than real application use.
func unrefinedValue(v cty.Value) cty.Value {
if !v.IsKnown() {
return cty.UnknownVal(v.Type())
}
return v
ret, _ := cty.Transform(v, func(p cty.Path, v cty.Value) (cty.Value, error) {
if !v.IsKnown() {
return cty.UnknownVal(v.Type()), nil
}
return v, nil
})
return ret
}
45 changes: 45 additions & 0 deletions internal/plans/objchange/plan_valid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1830,6 +1830,51 @@ func TestAssertPlanValid(t *testing.T) {
nil,
},

"refined unknown values in collection elements can become less refined": {
// Providers often can't preserve refinements through the provider
// wire protocol: although we do have a defined serialization for
// it, most providers were written before there was any such
// thing as refinements, and in future there might be new
// refinements that even refinement-aware providers don't know
// how to preserve, so we allow them to get dropped here as
// a concession to backward-compatibility.
//
// This is intending to approximate something like this:
//
// resource "null_resource" "hello" {
// triggers = {
// key = uuid()
// }
// }
//
// ...under the assumption that the null_resource implementation
// cannot preserve the not-null refinement that the uuid function
// generates.
//
// https://github.com/hashicorp/terraform/issues/33385
&configschema.Block{
Attributes: map[string]*configschema.Attribute{
"m": {
Type: cty.Map(cty.String),
},
},
},
cty.NullVal(cty.Object(map[string]cty.Type{
"m": cty.Map(cty.String),
})),
cty.ObjectVal(map[string]cty.Value{
"m": cty.MapVal(map[string]cty.Value{
"key": cty.UnknownVal(cty.String).RefineNotNull(),
}),
}),
cty.ObjectVal(map[string]cty.Value{
"m": cty.MapVal(map[string]cty.Value{
"key": cty.UnknownVal(cty.String),
}),
}),
nil,
},

"nested set values can contain computed unknown": {
&configschema.Block{
Attributes: map[string]*configschema.Attribute{
Expand Down

0 comments on commit d49e991

Please sign in to comment.