Skip to content

Commit

Permalink
plans/objchange: Don't consider refinements when validating plans
Browse files Browse the repository at this point in the history
Providers that existed prior to refinements (all of them, at the time of
writing) cannot preserve refinements sent in unknown values in the
configuration, and even if one day providers _are_ aware of refinements
there we might add new ones that existing providers don't know how to
handle.

For that reason we'll absolve providers of the responsibility of
preserving refinements from config into plan by fixing some cases where
we were incorrectly using RawEquals to compare values; that function isn't
appropriate for comparing values that might be unknown.

However, to avoid a disruptive change right now this initial fix just
strips off the refinements before comparing. Ideally this should be using
Value.Equals and handling unknown values more explicitly, but we'll save
that for a possible later improvement.

This does not include a similar exception for validating whether a final
value conforms to a plan because the plan value and the final value are
both produced by the same provider and so providers ought to be able to
be consistent with their _own_ treatment of refinements, if any.
Configuration is special because Terraform itself generates that, and so
it can potentially contain refinements that a particular provider has no
awareness of.
  • Loading branch information
apparentlymart committed May 22, 2023
1 parent 2159365 commit df9ecb2
Show file tree
Hide file tree
Showing 3 changed files with 41 additions and 4 deletions.
4 changes: 2 additions & 2 deletions internal/plans/objchange/objchange.go
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ func optionalValueNotComputable(schema *configschema.Attribute, val cty.Value) b
// values have been added. This function is only used to correlated
// configuration with possible valid prior values within sets.
func validPriorFromConfig(schema nestedSchema, prior, config cty.Value) bool {
if config.RawEquals(prior) {
if unrefinedValue(config).RawEquals(unrefinedValue(prior)) {
return true
}

Expand All @@ -446,7 +446,7 @@ func validPriorFromConfig(schema nestedSchema, prior, config cty.Value) bool {
}

// we don't need to know the schema if both are equal
if configV.RawEquals(priorV) {
if unrefinedValue(configV).RawEquals(unrefinedValue(priorV)) {
// we know they are equal, so no need to descend further
return false, nil
}
Expand Down
13 changes: 11 additions & 2 deletions internal/plans/objchange/plan_valid.go
Original file line number Diff line number Diff line change
Expand Up @@ -270,11 +270,11 @@ func assertPlannedAttrValid(name string, attrS *configschema.Attribute, priorSta
func assertPlannedValueValid(attrS *configschema.Attribute, priorV, configV, plannedV cty.Value, path cty.Path) []error {

var errs []error
if plannedV.RawEquals(configV) {
if unrefinedValue(plannedV).RawEquals(unrefinedValue(configV)) {
// This is the easy path: provider didn't change anything at all.
return errs
}
if plannedV.RawEquals(priorV) && !priorV.IsNull() && !configV.IsNull() {
if unrefinedValue(plannedV).RawEquals(unrefinedValue(priorV)) && !priorV.IsNull() && !configV.IsNull() {
// Also pretty easy: there is a prior value and the provider has
// returned it unchanged. This indicates that configV and plannedV
// are functionally equivalent and so the provider wishes to disregard
Expand Down Expand Up @@ -435,3 +435,12 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne

return errs
}

// unrefinedValue returns the given value with any unknown value refinements
// stripped away, making it a basic unknown value with only a type constraint.
func unrefinedValue(v cty.Value) cty.Value {
if !v.IsKnown() {
return cty.UnknownVal(v.Type())
}
return v
}
28 changes: 28 additions & 0 deletions internal/plans/objchange/plan_valid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1692,6 +1692,34 @@ func TestAssertPlanValid(t *testing.T) {
}),
nil,
},

"refined unknown values 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.
&configschema.Block{
Attributes: map[string]*configschema.Attribute{
"a": {
Type: cty.String,
Required: true,
},
},
},
cty.ObjectVal(map[string]cty.Value{
"a": cty.StringVal("old"),
}),
cty.ObjectVal(map[string]cty.Value{
"a": cty.UnknownVal(cty.String).RefineNotNull(),
}),
cty.ObjectVal(map[string]cty.Value{
"a": cty.UnknownVal(cty.String),
}),
nil,
},
}

for name, test := range tests {
Expand Down

0 comments on commit df9ecb2

Please sign in to comment.