From 7ca6be82854bae12b4bece168ac44dfb2d745e2d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 30 Aug 2021 13:35:47 -0400 Subject: [PATCH 1/4] correctly verify planned nested object values The validation for nested object types with computed attributes was using the incorrect function call. --- internal/plans/objchange/plan_valid.go | 2 +- internal/plans/objchange/plan_valid_test.go | 213 ++++++++++++++++++++ 2 files changed, 214 insertions(+), 1 deletion(-) diff --git a/internal/plans/objchange/plan_valid.go b/internal/plans/objchange/plan_valid.go index 3555e02ed..e26979e84 100644 --- a/internal/plans/objchange/plan_valid.go +++ b/internal/plans/objchange/plan_valid.go @@ -424,7 +424,7 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne if !prior.IsNull() && prior.HasIndex(idx).True() { priorEV = prior.Index(idx) } - moreErrs := assertPlannedObjectValid(schema, priorEV, configEV, plannedEV, path) + moreErrs := assertPlannedAttrsValid(schema.Attributes, priorEV, configEV, plannedEV, path) errs = append(errs, moreErrs...) } for it := config.ElementIterator(); it.Next(); { diff --git a/internal/plans/objchange/plan_valid_test.go b/internal/plans/objchange/plan_valid_test.go index a7166fe7b..783e7e15f 100644 --- a/internal/plans/objchange/plan_valid_test.go +++ b/internal/plans/objchange/plan_valid_test.go @@ -1222,6 +1222,137 @@ func TestAssertPlanValid(t *testing.T) { }), []string{`.bloop: planned value cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"blop":cty.StringVal("ok")})}) for a non-computed attribute`}, }, + "computed in nested objects": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "map": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingMap, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Computed: true, + }, + }, + }, + }, + // When an object has dynamic attrs, the map may be + // handled as an object. + "map_as_obj": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingMap, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Computed: true, + }, + }, + }, + }, + "list": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingList, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Computed: true, + }, + }, + }, + }, + "set": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingSet, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Computed: true, + }, + }, + }, + }, + "single": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingSingle, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.DynamicPseudoType, + Computed: true, + }, + }, + }, + }, + }, + }, + cty.NullVal(cty.Object(map[string]cty.Type{ + "map": cty.Map(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + "map_as_obj": cty.Map(cty.Object(map[string]cty.Type{ + "name": cty.DynamicPseudoType, + })), + "list": cty.List(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + "set": cty.Set(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + "single": cty.Object(map[string]cty.Type{ + "name": cty.String, + }), + })), + cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "one": cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + }), + "map_as_obj": cty.ObjectVal(map[string]cty.Value{ + "one": cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.DynamicPseudoType), + }), + }), + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + }), + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + }), + "single": cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "one": cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + }), + "map_as_obj": cty.ObjectVal(map[string]cty.Value{ + "one": cty.ObjectVal(map[string]cty.Value{ + "name": cty.StringVal("computed"), + }), + }), + "list": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + }), + "set": cty.SetVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + }), + "single": cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + }), + }), + nil, + }, } for name, test := range tests { @@ -1256,3 +1387,85 @@ func TestAssertPlanValid(t *testing.T) { }) } } + +func TestAssertPlanValidTEST(t *testing.T) { + tests := map[string]struct { + Schema *configschema.Block + Prior cty.Value + Config cty.Value + Planned cty.Value + WantErrs []string + }{ + "computed in map": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "items": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingMap, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + Computed: true, + Optional: true, + }, + }, + }, + Required: true, + }, + }, + }, + cty.NullVal(cty.Object(map[string]cty.Type{ + "items": cty.Map(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + })), + cty.ObjectVal(map[string]cty.Value{ + "items": cty.MapVal(map[string]cty.Value{ + "one": cty.ObjectVal(map[string]cty.Value{ + "name": cty.NullVal(cty.String), + //"name": cty.StringVal("computed"), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "items": cty.MapVal(map[string]cty.Value{ + "one": cty.ObjectVal(map[string]cty.Value{ + "name": cty.StringVal("computed"), + }), + }), + }), + nil, + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + errs := AssertPlanValid(test.Schema, test.Prior, test.Config, test.Planned) + + wantErrs := make(map[string]struct{}) + gotErrs := make(map[string]struct{}) + for _, err := range errs { + gotErrs[tfdiags.FormatError(err)] = struct{}{} + } + for _, msg := range test.WantErrs { + wantErrs[msg] = struct{}{} + } + + t.Logf( + "\nprior: %sconfig: %splanned: %s", + dump.Value(test.Planned), + dump.Value(test.Config), + dump.Value(test.Planned), + ) + for msg := range wantErrs { + if _, ok := gotErrs[msg]; !ok { + t.Errorf("missing expected error: %s", msg) + } + } + for msg := range gotErrs { + if _, ok := wantErrs[msg]; !ok { + t.Errorf("unexpected extra error: %s", msg) + } + } + }) + } +} From 903797084a361680f4c00f4b9439e7b7454f220a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 30 Aug 2021 13:53:49 -0400 Subject: [PATCH 2/4] objects vs maps in nested object types When using NestedMap objects, unify the codepath for both maps and objects as they may be interchangeable. --- internal/plans/objchange/plan_valid.go | 97 ++++++++------------- internal/plans/objchange/plan_valid_test.go | 2 +- 2 files changed, 37 insertions(+), 62 deletions(-) diff --git a/internal/plans/objchange/plan_valid.go b/internal/plans/objchange/plan_valid.go index e26979e84..5bcfe8dd5 100644 --- a/internal/plans/objchange/plan_valid.go +++ b/internal/plans/objchange/plan_valid.go @@ -368,71 +368,46 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne case configschema.NestingMap: // A NestingMap might either be a map or an object, depending on - // whether there are dynamically-typed attributes inside, but - // that's decided statically and so all values will have the same - // kind. - if planned.Type().IsObjectType() { - plannedAtys := planned.Type().AttributeTypes() - configAtys := config.Type().AttributeTypes() - for k := range plannedAtys { - if _, ok := configAtys[k]; !ok { - errs = append(errs, path.NewErrorf("block key %q from plan is not present in config", k)) - continue - } - path := append(path, cty.GetAttrStep{Name: k}) + // whether there are dynamically-typed attributes inside, so we will + // break these down to maps to handle them both in the same manner. + plannedVals := map[string]cty.Value{} + configVals := map[string]cty.Value{} + priorVals := map[string]cty.Value{} - plannedEV := planned.GetAttr(k) - if !plannedEV.IsKnown() { - errs = append(errs, path.NewErrorf("element representing nested block must not be unknown itself; set nested attribute values to unknown instead")) - continue - } - configEV := config.GetAttr(k) - priorEV := cty.NullVal(schema.ImpliedType()) - if !prior.IsNull() && prior.Type().HasAttribute(k) { - priorEV = prior.GetAttr(k) - } - moreErrs := assertPlannedAttrsValid(schema.Attributes, priorEV, configEV, plannedEV, path) - errs = append(errs, moreErrs...) + if !planned.IsNull() { + plannedVals = planned.AsValueMap() + } + if !config.IsNull() { + configVals = config.AsValueMap() + } + if !prior.IsNull() { + priorVals = prior.AsValueMap() + } + + for k, plannedEV := range plannedVals { + configEV, ok := configVals[k] + if !ok { + errs = append(errs, path.NewErrorf("block key %q from plan is not present in config", k)) + continue } - for k := range configAtys { - if _, ok := plannedAtys[k]; !ok { - errs = append(errs, path.NewErrorf("block key %q from config is not present in plan", k)) - continue - } + path := append(path, cty.GetAttrStep{Name: k}) + + if !plannedEV.IsKnown() { + errs = append(errs, path.NewErrorf("element representing nested block must not be unknown itself; set nested attribute values to unknown instead")) + continue } - } else { - plannedL := planned.LengthInt() - configL := config.LengthInt() - if plannedL != configL { - errs = append(errs, path.NewErrorf("block count in plan (%d) disagrees with count in config (%d)", plannedL, configL)) - return errs + + priorEV, ok := priorVals[k] + if !ok { + priorEV = cty.NullVal(schema.ImpliedType()) } - for it := planned.ElementIterator(); it.Next(); { - idx, plannedEV := it.Element() - path := append(path, cty.IndexStep{Key: idx}) - if !plannedEV.IsKnown() { - errs = append(errs, path.NewErrorf("element representing nested block must not be unknown itself; set nested attribute values to unknown instead")) - continue - } - k := idx.AsString() - if !config.HasIndex(idx).True() { - errs = append(errs, path.NewErrorf("block key %q from plan is not present in config", k)) - continue - } - configEV := config.Index(idx) - priorEV := cty.NullVal(schema.ImpliedType()) - if !prior.IsNull() && prior.HasIndex(idx).True() { - priorEV = prior.Index(idx) - } - moreErrs := assertPlannedAttrsValid(schema.Attributes, priorEV, configEV, plannedEV, path) - errs = append(errs, moreErrs...) - } - for it := config.ElementIterator(); it.Next(); { - idx, _ := it.Element() - if !planned.HasIndex(idx).True() { - errs = append(errs, path.NewErrorf("block key %q from config is not present in plan", idx.AsString())) - continue - } + moreErrs := assertPlannedAttrsValid(schema.Attributes, priorEV, configEV, plannedEV, path) + errs = append(errs, moreErrs...) + } + for k := range configVals { + if _, ok := plannedVals[k]; !ok { + errs = append(errs, path.NewErrorf("block key %q from config is not present in plan", k)) + continue } } diff --git a/internal/plans/objchange/plan_valid_test.go b/internal/plans/objchange/plan_valid_test.go index 783e7e15f..28150002b 100644 --- a/internal/plans/objchange/plan_valid_test.go +++ b/internal/plans/objchange/plan_valid_test.go @@ -1307,7 +1307,7 @@ func TestAssertPlanValid(t *testing.T) { "name": cty.NullVal(cty.String), }), }), - "map_as_obj": cty.ObjectVal(map[string]cty.Value{ + "map_as_obj": cty.MapVal(map[string]cty.Value{ "one": cty.ObjectVal(map[string]cty.Value{ "name": cty.NullVal(cty.DynamicPseudoType), }), From ea68d79ea20b23e0bd3627f1e8609733093e4155 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Mon, 30 Aug 2021 14:12:47 -0400 Subject: [PATCH 3/4] nested object values can be computed While blocks were not allowed to be computed by the provider, nested objects can be. Remove the errors regarding blocks and verify unknown values are valid. --- internal/plans/objchange/plan_valid.go | 53 ++++------ internal/plans/objchange/plan_valid_test.go | 101 +++++++++++++++++++- 2 files changed, 120 insertions(+), 34 deletions(-) diff --git a/internal/plans/objchange/plan_valid.go b/internal/plans/objchange/plan_valid.go index 5bcfe8dd5..2706a6c52 100644 --- a/internal/plans/objchange/plan_valid.go +++ b/internal/plans/objchange/plan_valid.go @@ -39,11 +39,11 @@ func AssertPlanValid(schema *configschema.Block, priorState, config, plannedStat func assertPlanValid(schema *configschema.Block, priorState, config, plannedState cty.Value, path cty.Path) []error { var errs []error if plannedState.IsNull() && !config.IsNull() { - errs = append(errs, path.NewErrorf("planned for absense but config wants existence")) + errs = append(errs, path.NewErrorf("planned for absence but config wants existence")) return errs } if config.IsNull() && !plannedState.IsNull() { - errs = append(errs, path.NewErrorf("planned for existence but config wants absense")) + errs = append(errs, path.NewErrorf("planned for existence but config wants absence")) return errs } if plannedState.IsNull() { @@ -286,6 +286,11 @@ func assertPlannedValueValid(attrS *configschema.Attribute, priorV, configV, pla } return errs } + } else { + if attrS.Computed { + errs = append(errs, path.NewErrorf("configuration present for computed attribute")) + return errs + } } // If this attribute has a NestedType, validate the nested object @@ -317,11 +322,11 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne var errs []error if planned.IsNull() && !config.IsNull() { - errs = append(errs, path.NewErrorf("planned for absense but config wants existence")) + errs = append(errs, path.NewErrorf("planned for absence but config wants existence")) return errs } if config.IsNull() && !planned.IsNull() { - errs = append(errs, path.NewErrorf("planned for existence but config wants absense")) + errs = append(errs, path.NewErrorf("planned for existence but config wants absence")) return errs } if planned.IsNull() { @@ -349,10 +354,6 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne for it := planned.ElementIterator(); it.Next(); { idx, plannedEV := it.Element() path := append(path, cty.IndexStep{Key: idx}) - if !plannedEV.IsKnown() { - errs = append(errs, path.NewErrorf("element representing nested block must not be unknown itself; set nested attribute values to unknown instead")) - continue - } if !config.HasIndex(idx).True() { continue // should never happen since we checked the lengths above } @@ -387,16 +388,11 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne for k, plannedEV := range plannedVals { configEV, ok := configVals[k] if !ok { - errs = append(errs, path.NewErrorf("block key %q from plan is not present in config", k)) + errs = append(errs, path.NewErrorf("map key %q from plan is not present in config", k)) continue } path := append(path, cty.GetAttrStep{Name: k}) - if !plannedEV.IsKnown() { - errs = append(errs, path.NewErrorf("element representing nested block must not be unknown itself; set nested attribute values to unknown instead")) - continue - } - priorEV, ok := priorVals[k] if !ok { priorEV = cty.NullVal(schema.ImpliedType()) @@ -406,31 +402,22 @@ func assertPlannedObjectValid(schema *configschema.Object, prior, config, planne } for k := range configVals { if _, ok := plannedVals[k]; !ok { - errs = append(errs, path.NewErrorf("block key %q from config is not present in plan", k)) + errs = append(errs, path.NewErrorf("map key %q from config is not present in plan", k)) continue } } case configschema.NestingSet: - // Because set elements have no identifier with which to correlate - // them, we can't robustly validate the plan for a nested block - // backed by a set, and so unfortunately we need to just trust the - // provider to do the right thing. :( - // - // (In principle we could correlate elements by matching the - // subset of attributes explicitly set in config, except for the - // special diff suppression rule which allows for there to be a - // planned value that is constructed by mixing part of a prior - // value with part of a config value, creating an entirely new - // element that is not present in either prior nor config.) - for it := planned.ElementIterator(); it.Next(); { - idx, plannedEV := it.Element() - path := append(path, cty.IndexStep{Key: idx}) - if !plannedEV.IsKnown() { - errs = append(errs, path.NewErrorf("element representing nested block must not be unknown itself; set nested attribute values to unknown instead")) - continue - } + plannedL := planned.LengthInt() + configL := config.LengthInt() + if plannedL != configL { + errs = append(errs, path.NewErrorf("count in plan (%d) disagrees with count in config (%d)", plannedL, configL)) + return errs } + // Because set elements have no identifier with which to correlate + // them, we can't robustly validate the plan for a nested object + // backed by a set, and so unfortunately we need to just trust the + // provider to do the right thing. } return errs diff --git a/internal/plans/objchange/plan_valid_test.go b/internal/plans/objchange/plan_valid_test.go index 28150002b..dedb3958b 100644 --- a/internal/plans/objchange/plan_valid_test.go +++ b/internal/plans/objchange/plan_valid_test.go @@ -1222,7 +1222,7 @@ func TestAssertPlanValid(t *testing.T) { }), []string{`.bloop: planned value cty.ListVal([]cty.Value{cty.ObjectVal(map[string]cty.Value{"blop":cty.StringVal("ok")})}) for a non-computed attribute`}, }, - "computed in nested objects": { + "computed within nested objects": { &configschema.Block{ Attributes: map[string]*configschema.Attribute{ "map": { @@ -1353,6 +1353,105 @@ func TestAssertPlanValid(t *testing.T) { }), nil, }, + "computed nested objects": { + &configschema.Block{ + Attributes: map[string]*configschema.Attribute{ + "map": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingMap, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + }, + }, + }, + Computed: true, + }, + "list": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingList, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + }, + }, + }, + Computed: true, + }, + "set": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingSet, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.String, + }, + }, + }, + Computed: true, + }, + "single": { + NestedType: &configschema.Object{ + Nesting: configschema.NestingSingle, + Attributes: map[string]*configschema.Attribute{ + "name": { + Type: cty.DynamicPseudoType, + }, + }, + }, + Computed: true, + }, + }, + }, + cty.NullVal(cty.Object(map[string]cty.Type{ + "map": cty.Map(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + "list": cty.List(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + "set": cty.Set(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + "single": cty.Object(map[string]cty.Type{ + "name": cty.String, + }), + })), + cty.ObjectVal(map[string]cty.Value{ + "map": cty.NullVal(cty.Map(cty.Object(map[string]cty.Type{ + "name": cty.String, + }))), + "list": cty.NullVal(cty.List(cty.Object(map[string]cty.Type{ + "name": cty.String, + }))), + "set": cty.NullVal(cty.Set(cty.Object(map[string]cty.Type{ + "name": cty.String, + }))), + "single": cty.NullVal(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + }), + cty.ObjectVal(map[string]cty.Value{ + "map": cty.MapVal(map[string]cty.Value{ + "one": cty.UnknownVal(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + }), + "list": cty.ListVal([]cty.Value{ + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + }), + "set": cty.SetVal([]cty.Value{ + cty.UnknownVal(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + }), + "single": cty.UnknownVal(cty.Object(map[string]cty.Type{ + "name": cty.String, + })), + }), + nil, + }, } for name, test := range tests { From f195ce7fd42975d30d48b7d5758537690aea01b6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 31 Aug 2021 11:17:32 -0400 Subject: [PATCH 4/4] remove temp test --- internal/plans/objchange/plan_valid_test.go | 82 --------------------- 1 file changed, 82 deletions(-) diff --git a/internal/plans/objchange/plan_valid_test.go b/internal/plans/objchange/plan_valid_test.go index dedb3958b..834d10463 100644 --- a/internal/plans/objchange/plan_valid_test.go +++ b/internal/plans/objchange/plan_valid_test.go @@ -1486,85 +1486,3 @@ func TestAssertPlanValid(t *testing.T) { }) } } - -func TestAssertPlanValidTEST(t *testing.T) { - tests := map[string]struct { - Schema *configschema.Block - Prior cty.Value - Config cty.Value - Planned cty.Value - WantErrs []string - }{ - "computed in map": { - &configschema.Block{ - Attributes: map[string]*configschema.Attribute{ - "items": { - NestedType: &configschema.Object{ - Nesting: configschema.NestingMap, - Attributes: map[string]*configschema.Attribute{ - "name": { - Type: cty.String, - Computed: true, - Optional: true, - }, - }, - }, - Required: true, - }, - }, - }, - cty.NullVal(cty.Object(map[string]cty.Type{ - "items": cty.Map(cty.Object(map[string]cty.Type{ - "name": cty.String, - })), - })), - cty.ObjectVal(map[string]cty.Value{ - "items": cty.MapVal(map[string]cty.Value{ - "one": cty.ObjectVal(map[string]cty.Value{ - "name": cty.NullVal(cty.String), - //"name": cty.StringVal("computed"), - }), - }), - }), - cty.ObjectVal(map[string]cty.Value{ - "items": cty.MapVal(map[string]cty.Value{ - "one": cty.ObjectVal(map[string]cty.Value{ - "name": cty.StringVal("computed"), - }), - }), - }), - nil, - }, - } - for name, test := range tests { - t.Run(name, func(t *testing.T) { - errs := AssertPlanValid(test.Schema, test.Prior, test.Config, test.Planned) - - wantErrs := make(map[string]struct{}) - gotErrs := make(map[string]struct{}) - for _, err := range errs { - gotErrs[tfdiags.FormatError(err)] = struct{}{} - } - for _, msg := range test.WantErrs { - wantErrs[msg] = struct{}{} - } - - t.Logf( - "\nprior: %sconfig: %splanned: %s", - dump.Value(test.Planned), - dump.Value(test.Config), - dump.Value(test.Planned), - ) - for msg := range wantErrs { - if _, ok := gotErrs[msg]; !ok { - t.Errorf("missing expected error: %s", msg) - } - } - for msg := range gotErrs { - if _, ok := wantErrs[msg]; !ok { - t.Errorf("unexpected extra error: %s", msg) - } - } - }) - } -}