From 2e2780e1f740ccaab9e84bfba6582c4ec1baa35f Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Wed, 18 Mar 2020 23:30:09 -0400 Subject: [PATCH 1/3] helper/schema: Additional validation for schema attribute references Reference: https://github.com/hashicorp/terraform-plugin-sdk/issues/71 --- helper/schema/schema.go | 36 ++++++++++++++++++++++++++-------- helper/schema/schema_test.go | 38 +++++++++++++++++++++++++++++------- 2 files changed, 59 insertions(+), 15 deletions(-) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 881b2ebba2..78cfb546c7 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -770,7 +770,7 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro } if len(v.ConflictsWith) > 0 { - err := checkKeysAgainstSchemaFlags(k, v.ConflictsWith, topSchemaMap) + err := checkKeysAgainstSchemaFlags(k, v.ConflictsWith, topSchemaMap, v) if err != nil { return fmt.Errorf("ConflictsWith: %+v", err) } @@ -784,14 +784,14 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro } if len(v.ExactlyOneOf) > 0 { - err := checkKeysAgainstSchemaFlags(k, v.ExactlyOneOf, topSchemaMap) + err := checkKeysAgainstSchemaFlags(k, v.ExactlyOneOf, topSchemaMap, v) if err != nil { return fmt.Errorf("ExactlyOneOf: %+v", err) } } if len(v.AtLeastOneOf) > 0 { - err := checkKeysAgainstSchemaFlags(k, v.AtLeastOneOf, topSchemaMap) + err := checkKeysAgainstSchemaFlags(k, v.AtLeastOneOf, topSchemaMap, v) if err != nil { return fmt.Errorf("AtLeastOneOf: %+v", err) } @@ -860,14 +860,20 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro return nil } -func checkKeysAgainstSchemaFlags(k string, keys []string, topSchemaMap schemaMap) error { +func checkKeysAgainstSchemaFlags(k string, keys []string, topSchemaMap schemaMap, self *Schema) error { for _, key := range keys { parts := strings.Split(key, ".") sm := topSchemaMap var target *Schema for _, part := range parts { - // Skip index fields - if _, err := strconv.Atoi(part); err == nil { + // Skip index fields if 0 + partInt, err := strconv.Atoi(part) + + if err == nil { + if partInt != 0 { + return fmt.Errorf("%s configuration block reference (%s) can only use the .0. index for TypeList and MaxItems: 1 configuration blocks", k, key) + } + continue } @@ -876,13 +882,27 @@ func checkKeysAgainstSchemaFlags(k string, keys []string, topSchemaMap schemaMap return fmt.Errorf("%s references unknown attribute (%s) at part (%s)", k, key, part) } - if subResource, ok := target.Elem.(*Resource); ok { - sm = schemaMap(subResource.Schema) + subResource, ok := target.Elem.(*Resource) + + if !ok { + continue + } + + if target.Type == TypeSet || target.MaxItems != 1 { + return fmt.Errorf("%s configuration block reference (%s) can only be used with TypeList and MaxItems: 1 configuration blocks", k, key) } + + sm = schemaMap(subResource.Schema) } + if target == nil { return fmt.Errorf("%s cannot find target attribute (%s), sm: %#v", k, key, sm) } + + if target == self { + return fmt.Errorf("%s cannot reference self (%s)", k, key) + } + if target.Required { return fmt.Errorf("%s cannot contain Required attribute (%s)", k, key) } diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 2f21f536cc..87d1603255 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3611,7 +3611,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { }, }, }, - false, // TODO: This should return an error as it will always error during runtime + true, }, "ConflictsWith list index syntax with list configuration block existing attribute": { @@ -3619,6 +3619,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "config_block_attr": &Schema{ Type: TypeList, Optional: true, + MaxItems: 1, Elem: &Resource{ Schema: map[string]*Schema{ "nested_attr": &Schema{ @@ -3660,6 +3661,29 @@ func TestSchemaMap_InternalValidate(t *testing.T) { true, }, + "ConflictsWith list index syntax with list configuration block missing MaxItems": { + map[string]*Schema{ + "config_block_attr": &Schema{ + Type: TypeList, + Optional: true, + Elem: &Resource{ + Schema: map[string]*Schema{ + "nested_attr": &Schema{ + Type: TypeString, + Optional: true, + }, + }, + }, + }, + "test": &Schema{ + Type: TypeBool, + Optional: true, + ConflictsWith: []string{"config_block_attr.0.missing_attr"}, + }, + }, + true, + }, + "ConflictsWith list index syntax with set configuration block existing attribute": { map[string]*Schema{ "config_block_attr": &Schema{ @@ -3680,7 +3704,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { ConflictsWith: []string{"config_block_attr.0.nested_attr"}, }, }, - false, // TODO: This should return an error as sets cannot be indexed + true, }, "ConflictsWith list index syntax with set configuration block missing attribute": { @@ -3726,7 +3750,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { ConflictsWith: []string{"config_block_attr.nested_attr"}, }, }, - false, // TODO: This should return an error as validateConflictingAttributes will never error + true, }, "ConflictsWith map key syntax with list configuration block self reference": { @@ -3745,7 +3769,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { }, }, }, - false, // TODO: This should return an error as validateConflictingAttributes will never error + true, }, "ConflictsWith map key syntax with set configuration block existing attribute": { @@ -3768,7 +3792,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { ConflictsWith: []string{"config_block_attr.nested_attr"}, }, }, - false, // TODO: This should return an error as validateConflictingAttributes will never error and sets cannot be indexed + true, }, "ConflictsWith map key syntax with set configuration block self reference": { @@ -3787,7 +3811,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { }, }, }, - false, // TODO: This should return an error as validateConflictingAttributes will never error and sets cannot be indexed + true, }, "ConflictsWith map key syntax with map attribute": { @@ -3830,7 +3854,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { ConflictsWith: []string{"test"}, }, }, - false, // TODO: This should return an error as it will always error during runtime + true, }, "Sub-resource invalid": { From 2fd0ba0290e56c26df78a96e46b791a5a55e350d Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Wed, 18 Mar 2020 23:40:58 -0400 Subject: [PATCH 2/3] tests/helper/schema: gofmt -s -w --- helper/schema/schema_test.go | 64 ++++++++++++++++++------------------ 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index 87d1603255..688c96c60a 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -3597,12 +3597,12 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith list index syntax with self reference": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeList, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, ConflictsWith: []string{"config_block_attr.0.nested_attr"}, @@ -3616,20 +3616,20 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith list index syntax with list configuration block existing attribute": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeList, Optional: true, MaxItems: 1, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, }, }, }, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"config_block_attr.0.nested_attr"}, @@ -3640,19 +3640,19 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith list index syntax with list configuration block missing attribute": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeList, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, }, }, }, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"config_block_attr.0.missing_attr"}, @@ -3663,19 +3663,19 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith list index syntax with list configuration block missing MaxItems": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeList, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, }, }, }, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"config_block_attr.0.missing_attr"}, @@ -3686,19 +3686,19 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith list index syntax with set configuration block existing attribute": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeSet, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, }, }, }, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"config_block_attr.0.nested_attr"}, @@ -3709,19 +3709,19 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith list index syntax with set configuration block missing attribute": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeSet, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, }, }, }, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"config_block_attr.0.missing_attr"}, @@ -3732,19 +3732,19 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith map key syntax with list configuration block existing attribute": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeList, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, }, }, }, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"config_block_attr.nested_attr"}, @@ -3755,12 +3755,12 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith map key syntax with list configuration block self reference": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeList, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, ConflictsWith: []string{"config_block_attr.nested_attr"}, @@ -3774,19 +3774,19 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith map key syntax with set configuration block existing attribute": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeSet, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, }, }, }, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"config_block_attr.nested_attr"}, @@ -3797,12 +3797,12 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith map key syntax with set configuration block self reference": { map[string]*Schema{ - "config_block_attr": &Schema{ + "config_block_attr": { Type: TypeSet, Optional: true, Elem: &Resource{ Schema: map[string]*Schema{ - "nested_attr": &Schema{ + "nested_attr": { Type: TypeString, Optional: true, ConflictsWith: []string{"config_block_attr.nested_attr"}, @@ -3816,12 +3816,12 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith map key syntax with map attribute": { map[string]*Schema{ - "map_attr": &Schema{ + "map_attr": { Type: TypeMap, Optional: true, Elem: &Schema{Type: TypeString}, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"map_attr.some_key"}, @@ -3832,12 +3832,12 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith string syntax with map attribute": { map[string]*Schema{ - "map_attr": &Schema{ + "map_attr": { Type: TypeMap, Optional: true, Elem: &Schema{Type: TypeString}, }, - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"map_attr"}, @@ -3848,7 +3848,7 @@ func TestSchemaMap_InternalValidate(t *testing.T) { "ConflictsWith string syntax with self reference": { map[string]*Schema{ - "test": &Schema{ + "test": { Type: TypeBool, Optional: true, ConflictsWith: []string{"test"}, From a69dc35e86bcc6ded9c681174708cce49112afa7 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 23 Apr 2020 20:37:31 -0400 Subject: [PATCH 3/3] helper/schema: Add missing argument from rebase --- helper/schema/schema.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 78cfb546c7..138980c298 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -777,7 +777,7 @@ func (m schemaMap) internalValidate(topSchemaMap schemaMap, attrsOnly bool) erro } if len(v.RequiredWith) > 0 { - err := checkKeysAgainstSchemaFlags(k, v.RequiredWith, topSchemaMap) + err := checkKeysAgainstSchemaFlags(k, v.RequiredWith, topSchemaMap, v) if err != nil { return fmt.Errorf("RequiredWith: %+v", err) }