ResiLien
/
terraform
2
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

terraform: validate providers' schemas during NewContext (#28124) 

* checkpoint save: update InternalValidate tests to compare exact error

* configschema: extract and extend attribute validation

This commit adds an attribute-specific InternalValidate which was extracted directly from the block.InternalValidate logic and extended to verify any NestedTypes inside an Attribute. Only one error message changed, since it is now valid to have a cty.NilType for Attribute.Type as long as NestedType is set.

* terraform: validate provider schema's during NewContext

We haven't been able to guarantee that providers are validating their own schemas using (some version of) InternalValidate since providers were split out of the main codebase. This PR adds a call to InternalValidate when provider schemas are initially loaded by NewContext, which required a few other changes:

InternalValidate's handling of errors vs multierrors was a little weird - before this PR, it was occasionally returning a non-nil error which only stated "0 errors occurred" - so I addressed that in InternalValidate. I then tested this with a configuration that was using all of our most popular providers, and found that at least on provider had some invalid attribute names, so I commented that particular validation out. Adding that in would be a breaking change which we would have to coordinate with enablement and providers and (especially in this case) make sure it's well communicated to external provider developers.

I ran a few very unscientific tests comparing the timing with and without this validation, and it appeared to only cause a sub-second increase.

* refactor validate error message to closer match the sdk's message

* better error message

* tweak error message: move the instruction to run init to the end of the message, after the specific error.
This commit is contained in:
Kristin Laemmert 2021-03-22 13:17:50 -04:00 committed by GitHub
parent 77562d9b57
commit b9138f4465
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 230 additions and 101 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
command/plan_test.go
View File

@ -940,7 +940,7 @@ func TestPlan_init_required(t *testing.T) {
t.Fatalf("expected error, got success")
}
got := output.Stderr()
if !strings.Contains(got, `Plugin reinitialization required. Please run "terraform init".`) {
if !strings.Contains(got, `Error: Could not load plugin`) {
t.Fatal("wrong error message in output:", got)
}
}

30
configs/configschema/decoder_spec.go
View File

@ -92,10 +92,10 @@ func (b *Block) DecoderSpec() hcldec.Spec {
for name, blockS := range b.BlockTypes {
if _, exists := ret[name]; exists {
// This indicates an invalid schema, since it's not valid to
// define both an attribute and a block type of the same name.
// However, we don't raise this here since it's checked by
// InternalValidate.
// This indicates an invalid schema, since it's not valid to define
// both an attribute and a block type of the same name. We assume
// that the provider has already used something like
// InternalValidate to validate their schema.
continue
}
@ -104,7 +104,7 @@ func (b *Block) DecoderSpec() hcldec.Spec {
// 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. We cannot validate MaxItems because a
// configuration may have any number of dynamic blocks
// configuration may have any number of dynamic blocks.
minItems := 0
if blockS.MinItems > 1 {
minItems = 1
@ -145,10 +145,12 @@ func (b *Block) DecoderSpec() hcldec.Spec {
}
case NestingSet:
// We forbid dynamically-typed attributes inside NestingSet in
// InternalValidate, so we don't do anything special to handle
// that here. (There is no set analog to tuple and object types,
// because cty's set implementation depends on knowing the static
// type in order to properly compute its internal hashes.)
// InternalValidate, so we don't do anything special to handle that
// here. (There is no set analog to tuple and object types, because
// cty's set implementation depends on knowing the static type in
// order to properly compute its internal hashes.) We assume that
// the provider has already used something like InternalValidate to
// validate their schema.
ret[name] = &hcldec.BlockSetSpec{
TypeName: name,
Nested: childSpec,
@ -174,7 +176,8 @@ func (b *Block) DecoderSpec() hcldec.Spec {
}
default:
// Invalid nesting type is just ignored. It's checked by
// InternalValidate.
// InternalValidate. We assume that the provider has already used
// something like InternalValidate to validate their schema.
continue
}
}
@ -190,9 +193,10 @@ func (a *Attribute) decoderSpec(name string) hcldec.Spec {
}
if a.NestedType != nil {
// FIXME: a panic() is a bad UX. Fix this, probably by extending
// InternalValidate() to check Attribute schemas as well and calling it
// when we get the schema from the provider in Context().
// FIXME: a panic() is a bad UX. InternalValidate() can check Attribute
// schemas as well so a fix might be to call it when we get the schema
// from the provider in Context(). Since this could be a breaking
// change, we'd need to communicate well before adding that call.
if a.Type != cty.NilType {
panic("Invalid attribute schema: NestedType and Type cannot both be set. This is a bug in the provider.")
}

142
configs/configschema/internal_validate.go
View File

@ -11,77 +11,139 @@ import (
var validName = regexp.MustCompile(`^[a-z0-9_]+$`)
// InternalValidate returns an error if the receiving block and its child
// schema definitions have any consistencies with the documented rules for
// valid schema.
// InternalValidate returns an error if the receiving block and its child schema
// definitions have any inconsistencies with the documented rules for valid
// schema.
//
// This is intended to be used within unit tests to detect when a given
// schema is invalid.
// This can be used within unit tests to detect when a given schema is invalid,
// and is run when terraform loads provider schemas during NewContext.
func (b *Block) InternalValidate() error {
if b == nil {
return fmt.Errorf("top-level block schema is nil")
}
return b.internalValidate("", nil)
return b.internalValidate("")
}
func (b *Block) internalValidate(prefix string, err error) error {
func (b *Block) internalValidate(prefix string) error {
var multiErr *multierror.Error
for name, attrS := range b.Attributes {
if attrS == nil {
err = multierror.Append(err, fmt.Errorf("%s%s: attribute schema is nil", prefix, name))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: attribute schema is nil", prefix, name))
continue
}
if !validName.MatchString(name) {
err = multierror.Append(err, fmt.Errorf("%s%s: name may contain only lowercase letters, digits and underscores", prefix, name))
}
if !attrS.Optional && !attrS.Required && !attrS.Computed {
err = multierror.Append(err, fmt.Errorf("%s%s: must set Optional, Required or Computed", prefix, name))
}
if attrS.Optional && attrS.Required {
err = multierror.Append(err, fmt.Errorf("%s%s: cannot set both Optional and Required", prefix, name))
}
if attrS.Computed && attrS.Required {
err = multierror.Append(err, fmt.Errorf("%s%s: cannot set both Computed and Required", prefix, name))
}
if attrS.Type == cty.NilType {
err = multierror.Append(err, fmt.Errorf("%s%s: Type must be set to something other than cty.NilType", prefix, name))
}
multiErr = multierror.Append(multiErr, attrS.internalValidate(name, prefix))
}
for name, blockS := range b.BlockTypes {
if blockS == nil {
err = multierror.Append(err, fmt.Errorf("%s%s: block schema is nil", prefix, name))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: block schema is nil", prefix, name))
continue
}
if _, isAttr := b.Attributes[name]; isAttr {
err = multierror.Append(err, fmt.Errorf("%s%s: name defined as both attribute and child block type", prefix, name))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: name defined as both attribute and child block type", prefix, name))
} else if !validName.MatchString(name) {
err = multierror.Append(err, fmt.Errorf("%s%s: name may contain only lowercase letters, digits and underscores", prefix, name))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: name may contain only lowercase letters, digits and underscores", prefix, name))
}
if blockS.MinItems < 0 || blockS.MaxItems < 0 {
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems and MaxItems must both be greater than zero", prefix, name))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: MinItems and MaxItems must both be greater than zero", prefix, name))
}
switch blockS.Nesting {
case NestingSingle:
switch {
case blockS.MinItems != blockS.MaxItems:
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems and MaxItems must match in NestingSingle mode", prefix, name))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: MinItems and MaxItems must match in NestingSingle mode", prefix, name))
case blockS.MinItems < 0 || blockS.MinItems > 1:
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems and MaxItems must be set to either 0 or 1 in NestingSingle mode", prefix, name))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: MinItems and MaxItems must be set to either 0 or 1 in NestingSingle mode", prefix, name))
}
case NestingGroup:
if blockS.MinItems != 0 || blockS.MaxItems != 0 {
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems and MaxItems cannot be used in NestingGroup mode", prefix, name))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: MinItems and MaxItems cannot be used in NestingGroup mode", prefix, name))
}
case NestingList, NestingSet:
if blockS.MinItems > blockS.MaxItems && blockS.MaxItems != 0 {
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems must be less than or equal to MaxItems in %s mode", prefix, name, blockS.Nesting))
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: MinItems must be less than or equal to MaxItems in %s mode", prefix, name, blockS.Nesting))
}
if blockS.Nesting == NestingSet {
ety := blockS.Block.ImpliedType()
if ety.HasDynamicTypes() {
// This is not permitted because the HCL (cty) set implementation
// needs to know the exact type of set elements in order to
// properly hash them, and so can't support mixed types.
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: NestingSet blocks may not contain attributes of cty.DynamicPseudoType", prefix, name))
}
}
case NestingMap:
if blockS.MinItems != 0 || blockS.MaxItems != 0 {
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: MinItems and MaxItems must both be 0 in NestingMap mode", prefix, name))
}
default:
multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: invalid nesting mode %s", prefix, name, blockS.Nesting))
}
subPrefix := prefix + name + "."
multiErr = multierror.Append(multiErr, blockS.Block.internalValidate(subPrefix))
}
return multiErr.ErrorOrNil()
}
// InternalValidate returns an error if the receiving attribute and its child
// schema definitions have any inconsistencies with the documented rules for
// valid schema.
func (a *Attribute) InternalValidate(name string) error {
if a == nil {
return fmt.Errorf("attribute schema is nil")
}
return a.internalValidate(name, "")
}
func (a *Attribute) internalValidate(name, prefix string) error {
var err *multierror.Error
/* FIXME: this validation breaks certain existing providers and cannot be enforced without coordination.
if !validName.MatchString(name) {
err = multierror.Append(err, fmt.Errorf("%s%s: name may contain only lowercase letters, digits and underscores", prefix, name))
}
*/
if !a.Optional && !a.Required && !a.Computed {
err = multierror.Append(err, fmt.Errorf("%s%s: must set Optional, Required or Computed", prefix, name))
}
if a.Optional && a.Required {
err = multierror.Append(err, fmt.Errorf("%s%s: cannot set both Optional and Required", prefix, name))
}
if a.Computed && a.Required {
err = multierror.Append(err, fmt.Errorf("%s%s: cannot set both Computed and Required", prefix, name))
}
if a.Type == cty.NilType && a.NestedType == nil {
err = multierror.Append(err, fmt.Errorf("%s%s: either Type or NestedType must be defined", prefix, name))
}
if a.Type != cty.NilType {
if a.NestedType != nil {
err = multierror.Append(fmt.Errorf("%s: Type and NestedType cannot both be set", name))
}
}
if a.NestedType != nil {
switch a.NestedType.Nesting {
case NestingSingle:
switch {
case a.NestedType.MinItems != a.NestedType.MaxItems:
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems and MaxItems must match in NestingSingle mode", prefix, name))
case a.NestedType.MinItems < 0 || a.NestedType.MinItems > 1:
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems and MaxItems must be set to either 0 or 1 in NestingSingle mode", prefix, name))
}
case NestingList, NestingSet:
if a.NestedType.MinItems > a.NestedType.MaxItems && a.NestedType.MaxItems != 0 {
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems must be less than or equal to MaxItems in %s mode", prefix, name, a.NestedType.Nesting))
}
if a.NestedType.Nesting == NestingSet {
ety := a.NestedType.ImpliedType()
if ety.HasDynamicTypes() {
// This is not permitted because the HCL (cty) set implementation
// needs to know the exact type of set elements in order to
@ -90,16 +152,20 @@ func (b *Block) internalValidate(prefix string, err error) error {
}
}
case NestingMap:
if blockS.MinItems != 0 || blockS.MaxItems != 0 {
if a.NestedType.MinItems != 0 || a.NestedType.MaxItems != 0 {
err = multierror.Append(err, fmt.Errorf("%s%s: MinItems and MaxItems must both be 0 in NestingMap mode", prefix, name))
}
default:
err = multierror.Append(err, fmt.Errorf("%s%s: invalid nesting mode %s", prefix, name, blockS.Nesting))
err = multierror.Append(err, fmt.Errorf("%s%s: invalid nesting mode %s", prefix, name, a.NestedType.Nesting))
}
for name, attrS := range a.NestedType.Attributes {
if attrS == nil {
err = multierror.Append(err, fmt.Errorf("%s%s: attribute schema is nil", prefix, name))
continue
}
err = multierror.Append(err, attrS.internalValidate(name, prefix))
}
subPrefix := prefix + name + "."
err = blockS.Block.internalValidate(subPrefix, err)
}
return err
return err.ErrorOrNil()
}

138
configs/configschema/internal_validate_test.go
View File

@ -10,172 +10,208 @@ import (
func TestBlockInternalValidate(t *testing.T) {
tests := map[string]struct {
Block *Block
ErrCount int
Block *Block
Errs []string
}{
"empty": {
&Block{},
0,
[]string{},
},
"valid": {
&Block{
Attributes: map[string]*Attribute{
"foo": &Attribute{
"foo": {
Type: cty.String,
Required: true,
},
"bar": &Attribute{
"bar": {
Type: cty.String,
Optional: true,
},
"baz": &Attribute{
"baz": {
Type: cty.String,
Computed: true,
},
"baz_maybe": &Attribute{
"baz_maybe": {
Type: cty.String,
Optional: true,
Computed: true,
},
},
BlockTypes: map[string]*NestedBlock{
"single": &NestedBlock{
"single": {
Nesting: NestingSingle,
Block: Block{},
},
"single_required": &NestedBlock{
"single_required": {
Nesting: NestingSingle,
Block: Block{},
MinItems: 1,
MaxItems: 1,
},
"list": &NestedBlock{
"list": {
Nesting: NestingList,
Block: Block{},
},
"list_required": &NestedBlock{
"list_required": {
Nesting: NestingList,
Block: Block{},
MinItems: 1,
},
"set": &NestedBlock{
"set": {
Nesting: NestingSet,
Block: Block{},
},
"set_required": &NestedBlock{
"set_required": {
Nesting: NestingSet,
Block: Block{},
MinItems: 1,
},
"map": &NestedBlock{
"map": {
Nesting: NestingMap,
Block: Block{},
},
},
},
0,
[]string{},
},
"attribute with no flags set": {
&Block{
Attributes: map[string]*Attribute{
"foo": &Attribute{
"foo": {
Type: cty.String,
},
},
},
1, // must set one of the flags
[]string{"foo: must set Optional, Required or Computed"},
},
"attribute required and optional": {
&Block{
Attributes: map[string]*Attribute{
"foo": &Attribute{
"foo": {
Type: cty.String,
Required: true,
Optional: true,
},
},
},
1, // both required and optional
[]string{"foo: cannot set both Optional and Required"},
},
"attribute required and computed": {
&Block{
Attributes: map[string]*Attribute{
"foo": &Attribute{
"foo": {
Type: cty.String,
Required: true,
Computed: true,
},
},
},
1, // both required and computed
[]string{"foo: cannot set both Computed and Required"},
},
"attribute optional and computed": {
&Block{
Attributes: map[string]*Attribute{
"foo": &Attribute{
"foo": {
Type: cty.String,
Optional: true,
Computed: true,
},
},
},
0,
[]string{},
},
"attribute with missing type": {
&Block{
Attributes: map[string]*Attribute{
"foo": &Attribute{
"foo": {
Optional: true,
},
},
},
1, // Type must be set
[]string{"foo: either Type or NestedType must be defined"},
},
"attribute with invalid name": {
/* FIXME: This caused errors when applied to existing providers (oci)
and cannot be enforced without coordination.
"attribute with invalid name": {&Block{Attributes:
map[string]*Attribute{"fooBar": {Type: cty.String, Optional:
true,
},
},
},
[]string{"fooBar: name may contain only lowercase letters, digits and underscores"},
},
*/
"attribute with invalid NestedType nesting": {
&Block{
Attributes: map[string]*Attribute{
"fooBar": &Attribute{
Type: cty.String,
"foo": {
NestedType: &Object{
Nesting: NestingSingle,
MinItems: 10,
MaxItems: 10,
},
Optional: true,
},
},
},
1, // name may not contain uppercase letters
[]string{"foo: MinItems and MaxItems must be set to either 0 or 1 in NestingSingle mode"},
},
"attribute with invalid NestedType attribute": {
&Block{
Attributes: map[string]*Attribute{
"foo": {
NestedType: &Object{
Nesting: NestingSingle,
Attributes: map[string]*Attribute{
"foo": {
Type: cty.String,
Required: true,
Optional: true,
},
},
},
Optional: true,
},
},
},
[]string{"foo: cannot set both Optional and Required"},
},
"block type with invalid name": {
&Block{
BlockTypes: map[string]*NestedBlock{
"fooBar": &NestedBlock{
"fooBar": {
Nesting: NestingSingle,
},
},
},
1, // name may not contain uppercase letters
[]string{"fooBar: name may contain only lowercase letters, digits and underscores"},
},
"colliding names": {
&Block{
Attributes: map[string]*Attribute{
"foo": &Attribute{
"foo": {
Type: cty.String,
Optional: true,
},
},
BlockTypes: map[string]*NestedBlock{
"foo": &NestedBlock{
"foo": {
Nesting: NestingSingle,
},
},
},
1, // "foo" is defined as both attribute and block type
[]string{"foo: name defined as both attribute and child block type"},
},
"nested block with badness": {
&Block{
BlockTypes: map[string]*NestedBlock{
"bad": &NestedBlock{
"bad": {
Nesting: NestingSingle,
Block: Block{
Attributes: map[string]*Attribute{
"nested_bad": &Attribute{
"nested_bad": {
Type: cty.String,
Required: true,
Optional: true,
@ -185,16 +221,16 @@ func TestBlockInternalValidate(t *testing.T) {
},
},
},
1, // nested_bad is both required and optional
[]string{"bad.nested_bad: cannot set both Optional and Required"},
},
"nested list block with dynamically-typed attribute": {
&Block{
BlockTypes: map[string]*NestedBlock{
"bad": &NestedBlock{
"bad": {
Nesting: NestingList,
Block: Block{
Attributes: map[string]*Attribute{
"nested_bad": &Attribute{
"nested_bad": {
Type: cty.DynamicPseudoType,
Optional: true,
},
@ -203,16 +239,16 @@ func TestBlockInternalValidate(t *testing.T) {
},
},
},
0,
[]string{},
},
"nested set block with dynamically-typed attribute": {
&Block{
BlockTypes: map[string]*NestedBlock{
"bad": &NestedBlock{
"bad": {
Nesting: NestingSet,
Block: Block{
Attributes: map[string]*Attribute{
"nested_bad": &Attribute{
"nested_bad": {
Type: cty.DynamicPseudoType,
Optional: true,
},
@ -221,11 +257,11 @@ func TestBlockInternalValidate(t *testing.T) {
},
},
},
1, // NestingSet blocks may not contain attributes of cty.DynamicPseudoType
[]string{"bad: NestingSet blocks may not contain attributes of cty.DynamicPseudoType"},
},
"nil": {
nil,
1, // block is nil
[]string{"top-level block schema is nil"},
},
"nil attr": {
&Block{
@ -233,7 +269,7 @@ func TestBlockInternalValidate(t *testing.T) {
"bad": nil,
},
},
1, // attribute schema is nil
[]string{"bad: attribute schema is nil"},
},
"nil block type": {
&Block{
@ -241,18 +277,26 @@ func TestBlockInternalValidate(t *testing.T) {
"bad": nil,
},
},
1, // block schema is nil
[]string{"bad: block schema is nil"},
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
errs := multierrorErrors(test.Block.InternalValidate())
if got, want := len(errs), test.ErrCount; got != want {
if got, want := len(errs), len(test.Errs); got != want {
t.Errorf("wrong number of errors %d; want %d", got, want)
for _, err := range errs {
t.Logf("- %s", err.Error())
}
} else {
if len(errs) > 0 {
for i := range errs {
if errs[i].Error() != test.Errs[i] {
t.Errorf("wrong error: got %s, want %s", errs[i].Error(), test.Errs[i])
}
}
}
}
})
}

5
terraform/resource_provider.go
View File

@ -1,8 +1,6 @@
package terraform
const errPluginInit = `
Plugin reinitialization required. Please run "terraform init".
Plugins are external binaries that Terraform uses to access and manipulate
resources. The configuration provided requires plugins which can't be located,
don't satisfy the version constraints, or are otherwise incompatible.
@ -12,4 +10,7 @@ configuration, including providers used in child modules. To see the
requirements and constraints, run "terraform providers".
%s
Plugin reinitialization required. Please address the above error(s) and run
"terraform init".
`

14
terraform/schemas.go
View File

@ -142,6 +142,9 @@ func loadProviderSchemas(schemas map[addrs.Provider]*ProviderSchema, config *con
}
for t, r := range resp.ResourceTypes {
if err := r.Block.InternalValidate(); err != nil {
diags = diags.Append(fmt.Errorf(errProviderSchemaInvalid, name, "resource", t, err))
}
s.ResourceTypes[t] = r.Block
s.ResourceTypeSchemaVersions[t] = uint64(r.Version)
if r.Version < 0 {
@ -152,6 +155,9 @@ func loadProviderSchemas(schemas map[addrs.Provider]*ProviderSchema, config *con
}
for t, d := range resp.DataSources {
if err := d.Block.InternalValidate(); err != nil {
diags = diags.Append(fmt.Errorf(errProviderSchemaInvalid, name, "data source", t, err))
}
s.DataSources[t] = d.Block
if d.Version < 0 {
// We're not using the version numbers here yet, but we'll check
@ -274,3 +280,11 @@ func (ps *ProviderSchema) SchemaForResourceType(mode addrs.ResourceMode, typeNam
func (ps *ProviderSchema) SchemaForResourceAddr(addr addrs.Resource) (schema *configschema.Block, version uint64) {
return ps.SchemaForResourceType(addr.Mode, addr.Type)
}
const errProviderSchemaInvalid = `
Internal validation of the provider failed! This is always a bug with the
provider itself, and not a user issue. Please report this bug to the
maintainers of the %q provider:
%s %s: %s
`