From bc1b5f958976b69d75d899351bae6d84e493fa54 Mon Sep 17 00:00:00 2001 From: Ivan Orlov Date: Thu, 4 Jan 2024 07:41:44 +0000 Subject: [PATCH] Check if supplied value matches module variable type --- pkg/config/expand.go | 30 +++++++++++++++++++++- pkg/config/expression_test.go | 1 + pkg/config/validator_test.go | 6 +++++ pkg/config/yaml.go | 37 +++++++++++++++++---------- pkg/config/yaml_test.go | 7 ++--- pkg/modulereader/metadata.go | 1 - pkg/modulewriter/modulewriter_test.go | 4 +-- pkg/validators/validators_test.go | 24 +++++++++-------- 8 files changed, 79 insertions(+), 31 deletions(-) diff --git a/pkg/config/expand.go b/pkg/config/expand.go index cd57bc9e5e..e1750cb183 100644 --- a/pkg/config/expand.go +++ b/pkg/config/expand.go @@ -23,6 +23,7 @@ import ( "github.com/agext/levenshtein" "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/convert" "golang.org/x/exp/maps" "golang.org/x/exp/slices" ) @@ -87,11 +88,38 @@ func validateModuleInputs(mp modulePath, m Module, bp Blueprint) error { continue } - // TODO: Check set value and input dtypes convertability + errs.At(ip, checkInputValueMatchesType(m.Settings.Get(input.Name), input, bp)) } return errs.OrNil() } +func attemptEvalModuleInput(val cty.Value, bp Blueprint) (cty.Value, bool) { + v, err := evalValue(val, bp) + // there could be a legitimate reasons for it. + // e.g. use of modules output or unsupported (by ghpc) functions + // TODO: + // * substitute module outputs with an UnknownValue + // * skip if uses functions with side-effects, e.g. `file` + // * add implementation of all pure terraform functions + // * add positive selection for eval-errors to bubble up + return v, err == nil +} + +func checkInputValueMatchesType(val cty.Value, input modulereader.VarInfo, bp Blueprint) error { + v, ok := attemptEvalModuleInput(val, bp) + if !ok || input.Type == cty.NilType { + return nil // skip, can do nothing + } + // cty does panic on some edge cases, e.g. (cty.NilVal) + // we don't anticipate any of those, but just in case, catch panic and swallow it + defer func() { recover() }() + // TODO: consider returning error (not panic) or logging warning + if _, err := convert.Convert(v, input.Type); err != nil { + return fmt.Errorf("unsuitable value for %q: %w", input.Name, err) + } + return nil +} + func (dc *DeploymentConfig) expandBackends() { // 1. DEFAULT: use TerraformBackend configuration (if supplied) in each // resource group diff --git a/pkg/config/expression_test.go b/pkg/config/expression_test.go index be6206a1ee..74703a67ff 100644 --- a/pkg/config/expression_test.go +++ b/pkg/config/expression_test.go @@ -154,6 +154,7 @@ func TestTokensForValueNoLiteral(t *testing.T) { "ba": cty.NumberIntVal(56), })}), "pony.zebra": cty.NilVal, + "zanzibar": cty.NullVal(cty.DynamicPseudoType), }) want := hclwrite.NewEmptyFile() want.Body().AppendUnstructuredTokens(hclwrite.TokensForValue(val)) diff --git a/pkg/config/validator_test.go b/pkg/config/validator_test.go index 95d6e5498a..c79df42096 100644 --- a/pkg/config/validator_test.go +++ b/pkg/config/validator_test.go @@ -37,6 +37,12 @@ func (s *zeroSuite) TestValidateVars(c *C) { c.Check(validateVars(vars), NotNil) } + { // Fail: Null value + vars := Dict{base} + vars.Set("fork", cty.NullVal(cty.String)) + c.Check(validateVars(vars), NotNil) + } + { // Fail: labels not a map vars := Dict{base} vars.Set("labels", cty.StringVal("a_string")) diff --git a/pkg/config/yaml.go b/pkg/config/yaml.go index c3d912335b..4d0fde2d7b 100644 --- a/pkg/config/yaml.go +++ b/pkg/config/yaml.go @@ -219,14 +219,23 @@ func (ms *ModuleIDs) UnmarshalYAML(n *yaml.Node) error { // YamlValue is wrapper around cty.Value to handle YAML unmarshal. type YamlValue struct { - v cty.Value + v cty.Value // do not use this field directly, use Wrap() and Unwrap() instead } // Unwrap returns wrapped cty.Value. func (y YamlValue) Unwrap() cty.Value { + if y.v == cty.NilVal { + // we can't use 0-value of cty.Value (NilVal) + // instead it should be a proper null(any) value + return cty.NullVal(cty.DynamicPseudoType) + } return y.v } +func (y *YamlValue) Wrap(v cty.Value) { + y.v = v +} + // UnmarshalYAML implements custom YAML unmarshaling. func (y *YamlValue) UnmarshalYAML(n *yaml.Node) error { var err error @@ -252,24 +261,26 @@ func (y *YamlValue) unmarshalScalar(n *yaml.Node) error { if err != nil { return fmt.Errorf("line %d: %w", n.Line, err) } - if y.v, err = gocty.ToCtyValue(s, ty); err != nil { + v, err := gocty.ToCtyValue(s, ty) + if err != nil { return err } + y.Wrap(v) - if l, is := IsYamlExpressionLiteral(y.v); is { // HCL literal + if l, is := IsYamlExpressionLiteral(y.Unwrap()); is { // HCL literal var e Expression if e, err = ParseExpression(l); err != nil { // TODO: point to exact location within expression, see Diagnostic.Subject return fmt.Errorf("line %d: %w", n.Line, err) } - y.v = e.AsValue() - } else if y.v.Type() == cty.String && hasVariable(y.v.AsString()) { // "simple" variable - e, err := SimpleVarToExpression(y.v.AsString()) + y.Wrap(e.AsValue()) + } else if y.Unwrap().Type() == cty.String && hasVariable(y.Unwrap().AsString()) { // "simple" variable + e, err := SimpleVarToExpression(y.Unwrap().AsString()) if err != nil { // TODO: point to exact location within expression, see Diagnostic.Subject return fmt.Errorf("line %d: %w", n.Line, err) } - y.v = e.AsValue() + y.Wrap(e.AsValue()) } return nil } @@ -281,9 +292,9 @@ func (y *YamlValue) unmarshalObject(n *yaml.Node) error { } mv := map[string]cty.Value{} for k, y := range my { - mv[k] = y.v + mv[k] = y.Unwrap() } - y.v = cty.ObjectVal(mv) + y.Wrap(cty.ObjectVal(mv)) return nil } @@ -294,9 +305,9 @@ func (y *YamlValue) unmarshalTuple(n *yaml.Node) error { } lv := []cty.Value{} for _, y := range ly { - lv = append(lv, y.v) + lv = append(lv, y.Unwrap()) } - y.v = cty.TupleVal(lv) + y.Wrap(cty.TupleVal(lv)) return nil } @@ -306,11 +317,11 @@ func (d *Dict) UnmarshalYAML(n *yaml.Node) error { if err := n.Decode(&v); err != nil { return err } - ty := v.v.Type() + ty := v.Unwrap().Type() if !ty.IsObjectType() { return fmt.Errorf("line %d: must be a mapping, got %s", n.Line, ty.FriendlyName()) } - for k, w := range v.v.AsValueMap() { + for k, w := range v.Unwrap().AsValueMap() { d.Set(k, w) } return nil diff --git a/pkg/config/yaml_test.go b/pkg/config/yaml_test.go index f65f4d6cc3..75aa8242c7 100644 --- a/pkg/config/yaml_test.go +++ b/pkg/config/yaml_test.go @@ -337,10 +337,11 @@ b: null c: ~ d: "null" ` + anyNull := cty.NullVal(cty.DynamicPseudoType) want := cty.ObjectVal(map[string]cty.Value{ - "a": cty.NilVal, - "b": cty.NilVal, - "c": cty.NilVal, + "a": anyNull, + "b": anyNull, + "c": anyNull, "d": cty.StringVal("null"), }) diff --git a/pkg/modulereader/metadata.go b/pkg/modulereader/metadata.go index 3748d3872d..15069f6c11 100644 --- a/pkg/modulereader/metadata.go +++ b/pkg/modulereader/metadata.go @@ -54,7 +54,6 @@ type MetadataGhpc struct { func GetMetadata(source string) (Metadata, error) { var err error var data []byte - // TODO: use bpmetadata.UnmarshalMetadata, it performs some additional checks filePath := filepath.Join(source, "metadata.yaml") switch { diff --git a/pkg/modulewriter/modulewriter_test.go b/pkg/modulewriter/modulewriter_test.go index 1d63ac989b..39e599ccc4 100644 --- a/pkg/modulewriter/modulewriter_test.go +++ b/pkg/modulewriter/modulewriter_test.go @@ -58,8 +58,8 @@ func (s *MySuite) getDeploymentConfigForTest() config.DeploymentConfig { Kind: config.TerraformKind, ID: "testModule", Settings: config.NewDict(map[string]cty.Value{ - "deployment_name": cty.NilVal, - "project_id": cty.NilVal, + "deployment_name": cty.NullVal(cty.String), + "project_id": cty.NullVal(cty.String), }), Outputs: []modulereader.OutputInfo{ { diff --git a/pkg/validators/validators_test.go b/pkg/validators/validators_test.go index d9389f19a6..16d46ca519 100644 --- a/pkg/validators/validators_test.go +++ b/pkg/validators/validators_test.go @@ -32,37 +32,39 @@ func Test(t *testing.T) { } func (s *MySuite) TestCheckInputs(c *C) { + dummy := cty.NullVal(cty.String) + { // OK: Inputs is equal to required inputs without regard to ordering i := config.NewDict(map[string]cty.Value{ - "in0": cty.NilVal, - "in1": cty.NilVal}) + "in0": dummy, + "in1": dummy}) c.Check(checkInputs(i, []string{"in0", "in1"}), IsNil) c.Check(checkInputs(i, []string{"in1", "in0"}), IsNil) } { // FAIL: inputs are a proper subset of required inputs i := config.NewDict(map[string]cty.Value{ - "in0": cty.NilVal, - "in1": cty.NilVal}) + "in0": dummy, + "in1": dummy}) err := checkInputs(i, []string{"in0", "in1", "in2"}) c.Check(err, NotNil) } { // FAIL: inputs intersect with required inputs but are not a proper subset i := config.NewDict(map[string]cty.Value{ - "in0": cty.NilVal, - "in1": cty.NilVal, - "in3": cty.NilVal}) + "in0": dummy, + "in1": dummy, + "in3": dummy}) err := checkInputs(i, []string{"in0", "in1", "in2"}) c.Check(err, NotNil) } { // FAIL inputs are a proper superset of required inputs i := config.NewDict(map[string]cty.Value{ - "in0": cty.NilVal, - "in1": cty.NilVal, - "in2": cty.NilVal, - "in3": cty.NilVal}) + "in0": dummy, + "in1": dummy, + "in2": dummy, + "in3": dummy}) err := checkInputs(i, []string{"in0", "in1", "in2"}) c.Check(err, ErrorMatches, "only 3 inputs \\[in0 in1 in2\\] should be provided") }