From 4c439b099f0ca886255f063c4cbcf0613a298e08 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 15 Mar 2023 16:55:26 -0700 Subject: [PATCH] plans/objchange: Don't consider refinements when validating plans 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. --- internal/plans/objchange/objchange.go | 4 +-- internal/plans/objchange/plan_valid.go | 13 ++++++-- internal/plans/objchange/plan_valid_test.go | 34 +++++++++++++++++++-- 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/internal/plans/objchange/objchange.go b/internal/plans/objchange/objchange.go index f806174f0c..f21cd9b6b2 100644 --- a/internal/plans/objchange/objchange.go +++ b/internal/plans/objchange/objchange.go @@ -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 } @@ -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 } diff --git a/internal/plans/objchange/plan_valid.go b/internal/plans/objchange/plan_valid.go index 6e8941fa02..ad7c051783 100644 --- a/internal/plans/objchange/plan_valid.go +++ b/internal/plans/objchange/plan_valid.go @@ -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 @@ -463,3 +463,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 +} diff --git a/internal/plans/objchange/plan_valid_test.go b/internal/plans/objchange/plan_valid_test.go index 316bf56133..655c6e22f7 100644 --- a/internal/plans/objchange/plan_valid_test.go +++ b/internal/plans/objchange/plan_valid_test.go @@ -1796,11 +1796,39 @@ func TestAssertPlanValid(t *testing.T) { )), }), []string{ - `.set: count in plan (cty.UnknownVal(cty.Number)) disagrees with count in config (cty.NumberIntVal(1))`, - `.list: count in plan (cty.UnknownVal(cty.Number)) disagrees with count in config (cty.NumberIntVal(1))`, - `.map: count in plan (cty.UnknownVal(cty.Number)) disagrees with count in config (cty.NumberIntVal(1))`, + `.set: count in plan (cty.UnknownVal(cty.Number).Refine().NotNull().NumberLowerBound(cty.NumberIntVal(0), true).NumberUpperBound(cty.NumberIntVal(9.223372036854775807e+18), true).NewValue()) disagrees with count in config (cty.NumberIntVal(1))`, + `.list: count in plan (cty.UnknownVal(cty.Number).Refine().NotNull().NumberLowerBound(cty.NumberIntVal(0), true).NumberUpperBound(cty.NumberIntVal(9.223372036854775807e+18), true).NewValue()) disagrees with count in config (cty.NumberIntVal(1))`, + `.map: count in plan (cty.UnknownVal(cty.Number).Refine().NotNull().NumberLowerBound(cty.NumberIntVal(0), true).NumberUpperBound(cty.NumberIntVal(9.223372036854775807e+18), true).NewValue()) disagrees with count in config (cty.NumberIntVal(1))`, }, }, + + "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 {