From 682286e1846cbe0efb2bfb700af691ce0f90d93a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 26 Jul 2019 14:36:19 -0700 Subject: [PATCH 1/3] test for Required win MinItems > 1 --- builtin/providers/test/provider.go | 1 + .../providers/test/resource_required_min.go | 68 +++++++++++++++++++ .../test/resource_required_min_test.go | 66 ++++++++++++++++++ 3 files changed, 135 insertions(+) create mode 100644 builtin/providers/test/resource_required_min.go create mode 100644 builtin/providers/test/resource_required_min_test.go diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index 9d4dc94be..4afae201a 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -37,6 +37,7 @@ func Provider() terraform.ResourceProvider { "test_resource_config_mode": testResourceConfigMode(), "test_resource_nested_id": testResourceNestedId(), "test_undeleteable": testResourceUndeleteable(), + "test_resource_required_min": testResourceRequiredMin(), }, DataSourcesMap: map[string]*schema.Resource{ "test_data_source": testDataSource(), diff --git a/builtin/providers/test/resource_required_min.go b/builtin/providers/test/resource_required_min.go new file mode 100644 index 000000000..413d4c51d --- /dev/null +++ b/builtin/providers/test/resource_required_min.go @@ -0,0 +1,68 @@ +package test + +import ( + "github.com/hashicorp/terraform/helper/schema" +) + +func testResourceRequiredMin() *schema.Resource { + return &schema.Resource{ + Create: testResourceRequiredMinCreate, + Read: testResourceRequiredMinRead, + Update: testResourceRequiredMinUpdate, + Delete: testResourceRequiredMinDelete, + + CustomizeDiff: func(d *schema.ResourceDiff, _ interface{}) error { + if d.HasChange("dependent_list") { + d.SetNewComputed("computed_list") + } + return nil + }, + + Schema: map[string]*schema.Schema{ + "min_items": { + Type: schema.TypeList, + Optional: true, + MinItems: 2, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "val": { + Type: schema.TypeString, + Required: true, + }, + }, + }, + }, + "required_min_items": { + Type: schema.TypeList, + Required: true, + MinItems: 2, + Elem: &schema.Resource{ + Schema: map[string]*schema.Schema{ + "val": { + Type: schema.TypeString, + Required: true, + }, + }, + }, + }, + }, + } +} + +func testResourceRequiredMinCreate(d *schema.ResourceData, meta interface{}) error { + d.SetId("testId") + return testResourceRequiredMinRead(d, meta) +} + +func testResourceRequiredMinRead(d *schema.ResourceData, meta interface{}) error { + return nil +} + +func testResourceRequiredMinUpdate(d *schema.ResourceData, meta interface{}) error { + return testResourceRequiredMinRead(d, meta) +} + +func testResourceRequiredMinDelete(d *schema.ResourceData, meta interface{}) error { + d.SetId("") + return nil +} diff --git a/builtin/providers/test/resource_required_min_test.go b/builtin/providers/test/resource_required_min_test.go new file mode 100644 index 000000000..180c28ccf --- /dev/null +++ b/builtin/providers/test/resource_required_min_test.go @@ -0,0 +1,66 @@ +package test + +import ( + "regexp" + "strings" + "testing" + + "github.com/hashicorp/terraform/helper/resource" +) + +func TestResource_dynamicRequiredMinItems(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: ` +resource "test_resource_required_min" "a" { +} +`, + ExpectError: regexp.MustCompile(`"required_min_items" blocks are required`), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list" "a" { + dependent_list { + val = "a" + } +} + +resource "test_resource_required_min" "b" { + dynamic "required_min_items" { + for_each = test_resource_list.a.computed_list + content { + val = required_min_items.value + } + } +} + `), + ExpectError: regexp.MustCompile(`required_min_items: attribute supports 2 item as a minimum`), + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource_list" "c" { + dependent_list { + val = "a" + } + + dependent_list { + val = "b" + } +} + +resource "test_resource_required_min" "b" { + dynamic "required_min_items" { + for_each = test_resource_list.c.computed_list + content { + val = required_min_items.value + } + } +} + `), + }, + }, + }) +} From 67dbd6d3458cf3c67bba8d0f7770c15d7fd9644f Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 26 Jul 2019 14:43:11 -0700 Subject: [PATCH 2/3] don't check MinItems with unknowns in blocks If a block was defined via "dynamic", there will be only one block value until the expansion is known. Since we can't detect dynamic blocks at this point, don't verify MinItems while there are unknown values in the config. The decoder spec can also only check for existence of a block, so limit the check to 0 or 1. --- configs/configschema/coerce_value.go | 10 +++++-- configs/configschema/coerce_value_test.go | 35 ++++++++++++++++++++++- configs/configschema/decoder_spec.go | 14 +++++++-- configs/configschema/decoder_spec_test.go | 27 +++++++++++++++++ 4 files changed, 80 insertions(+), 6 deletions(-) diff --git a/configs/configschema/coerce_value.go b/configs/configschema/coerce_value.go index e59f58d8e..7996c383a 100644 --- a/configs/configschema/coerce_value.go +++ b/configs/configschema/coerce_value.go @@ -113,7 +113,10 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("must be a list") } l := coll.LengthInt() - if l < blockS.MinItems { + + // Assume that if there are unknowns this could have come from + // a dynamic block, and we can't validate MinItems yet. + if l < blockS.MinItems && coll.IsWhollyKnown() { return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("insufficient items for attribute %q; must have at least %d", typeName, blockS.MinItems) } if l > blockS.MaxItems && blockS.MaxItems > 0 { @@ -161,7 +164,10 @@ func (b *Block) coerceValue(in cty.Value, path cty.Path) (cty.Value, error) { return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("must be a set") } l := coll.LengthInt() - if l < blockS.MinItems { + + // Assume that if there are unknowns this could have come from + // a dynamic block, and we can't validate MinItems yet. + if l < blockS.MinItems && coll.IsWhollyKnown() { return cty.UnknownVal(b.ImpliedType()), path.NewErrorf("insufficient items for attribute %q; must have at least %d", typeName, blockS.MinItems) } if l > blockS.MaxItems && blockS.MaxItems > 0 { diff --git a/configs/configschema/coerce_value_test.go b/configs/configschema/coerce_value_test.go index 3286751a3..7fd07856d 100644 --- a/configs/configschema/coerce_value_test.go +++ b/configs/configschema/coerce_value_test.go @@ -331,7 +331,7 @@ func TestCoerceValue(t *testing.T) { "foo": { Block: Block{}, Nesting: NestingList, - MinItems: 1, + MinItems: 2, }, }, }, @@ -345,6 +345,39 @@ func TestCoerceValue(t *testing.T) { }), "", }, + "unknowns in nested list": { + &Block{ + BlockTypes: map[string]*NestedBlock{ + "foo": { + Block: Block{ + Attributes: map[string]*Attribute{ + "attr": { + Type: cty.String, + Required: true, + }, + }, + }, + Nesting: NestingList, + MinItems: 2, + }, + }, + }, + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "attr": cty.UnknownVal(cty.String), + }), + }), + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "attr": cty.UnknownVal(cty.String), + }), + }), + }), + "", + }, "unknown nested set": { &Block{ Attributes: map[string]*Attribute{ diff --git a/configs/configschema/decoder_spec.go b/configs/configschema/decoder_spec.go index d8f41eabc..e748dd20d 100644 --- a/configs/configschema/decoder_spec.go +++ b/configs/configschema/decoder_spec.go @@ -33,6 +33,14 @@ func (b *Block) DecoderSpec() hcldec.Spec { childSpec := blockS.Block.DecoderSpec() + // We can only validate 0 or 1 for MinItems, because a dynamic block + // may satisfy any number of min items while only having a single + // block in the config. + minItems := 0 + if blockS.MinItems > 1 { + minItems = 1 + } + switch blockS.Nesting { case NestingSingle, NestingGroup: ret[name] = &hcldec.BlockSpec{ @@ -57,14 +65,14 @@ func (b *Block) DecoderSpec() hcldec.Spec { ret[name] = &hcldec.BlockTupleSpec{ TypeName: name, Nested: childSpec, - MinItems: blockS.MinItems, + MinItems: minItems, MaxItems: blockS.MaxItems, } } else { ret[name] = &hcldec.BlockListSpec{ TypeName: name, Nested: childSpec, - MinItems: blockS.MinItems, + MinItems: minItems, MaxItems: blockS.MaxItems, } } @@ -77,7 +85,7 @@ func (b *Block) DecoderSpec() hcldec.Spec { ret[name] = &hcldec.BlockSetSpec{ TypeName: name, Nested: childSpec, - MinItems: blockS.MinItems, + MinItems: minItems, MaxItems: blockS.MaxItems, } case NestingMap: diff --git a/configs/configschema/decoder_spec_test.go b/configs/configschema/decoder_spec_test.go index b779f2c35..d6f6a6a15 100644 --- a/configs/configschema/decoder_spec_test.go +++ b/configs/configschema/decoder_spec_test.go @@ -356,6 +356,33 @@ func TestBlockDecoderSpec(t *testing.T) { }), 1, // too many "foo" blocks }, + // dynamic blocks may fulfill MinItems, but there is only one block to + // decode. + "required MinItems": { + &Block{ + BlockTypes: map[string]*NestedBlock{ + "foo": { + Nesting: NestingList, + Block: Block{}, + MinItems: 2, + }, + }, + }, + hcltest.MockBody(&hcl.BodyContent{ + Blocks: hcl.Blocks{ + &hcl.Block{ + Type: "foo", + Body: hcl.EmptyBody(), + }, + }, + }), + cty.ObjectVal(map[string]cty.Value{ + "foo": cty.ListVal([]cty.Value{ + cty.EmptyObjectVal, + }), + }), + 0, + }, "extraneous attribute": { &Block{}, hcltest.MockBody(&hcl.BodyContent{ From 4bed030d40ad9f0d7e6ab612a7433f528c864f81 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 26 Jul 2019 14:50:58 -0700 Subject: [PATCH 3/3] don't validate MinItems with unknowns in a block If there are unknowns, the block may have come from a dynamic declaration, and we can't validate MinItems. Once the blocks are expanded, we will get the full config for validation without any unknown values. --- helper/schema/schema.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 931622963..5f16672d3 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -1456,6 +1456,15 @@ func (m schemaMap) validateList( "%s: should be a list", k)} } + // We can't validate list length if this came from a dynamic block. + // Since there's no way to determine if something was from a dynamic block + // at this point, we're going to skip validation in the new protocol if + // there are any unknowns. Validate will eventually be called again once + // all values are known. + if isProto5() && !isWhollyKnown(raw) { + return nil, nil + } + // Validate length if schema.MaxItems > 0 && rawV.Len() > schema.MaxItems { return nil, []error{fmt.Errorf(