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.

Author: mildwonkey

command/plan_test.go

@@ -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)
 	}
 }

configs/configschema/decoder_spec.go

@@ -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.")
 		}

configs/configschema/internal_validate.go

@@ -11,95 +11,161 @@ 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.
-					err = multierror.Append(err, fmt.Errorf("%s%s: NestingSet blocks may not contain attributes of cty.DynamicPseudoType", prefix, name))
+					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 {
-				err = multierror.Append(err, fmt.Errorf("%s%s: MinItems and MaxItems must both be 0 in NestingMap mode", prefix, name))
+				multiErr = multierror.Append(multiErr, 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))
+			multiErr = multierror.Append(multiErr, fmt.Errorf("%s%s: invalid nesting mode %s", prefix, name, blockS.Nesting))
 		}
 
 		subPrefix := prefix + name + "."
-		err = blockS.Block.internalValidate(subPrefix, err)
+		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
+					// properly hash them, and so can't support mixed types.
+					err = multierror.Append(err, fmt.Errorf("%s%s: NestingSet blocks may not contain attributes of cty.DynamicPseudoType", prefix, name))
+				}
+			}
+		case NestingMap:
+			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, 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))
+		}
 	}
 
-	return err
+	return err.ErrorOrNil()
 }