From 91d73324714145cefb8148beb55a7047f65266d1 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 30 Oct 2020 12:26:22 +0100 Subject: [PATCH 01/17] add basic code for variable validation parsing * hcl2template/addrs.ParseRef will parse a reference and tell for example if we are referring to a variable and its name, for now it can only do that and in the future it improved when we need to most of it is from the TF code. This is used to tell wether a variable vas referenced in a variable validation condition; for now. * Added Validations blocks to the hcl2 Variable struct and code to parse/validate that. --- hcl2template/addrs/input_variable.go | 11 ++ hcl2template/addrs/parse_ref.go | 93 ++++++++++ hcl2template/addrs/referenceable.go | 18 ++ hcl2template/types.variables.go | 258 ++++++++++++++++++++++----- hcl2template/types.variables_test.go | 2 +- 5 files changed, 340 insertions(+), 42 deletions(-) create mode 100644 hcl2template/addrs/input_variable.go create mode 100644 hcl2template/addrs/parse_ref.go create mode 100644 hcl2template/addrs/referenceable.go diff --git a/hcl2template/addrs/input_variable.go b/hcl2template/addrs/input_variable.go new file mode 100644 index 000000000..ad4370bb8 --- /dev/null +++ b/hcl2template/addrs/input_variable.go @@ -0,0 +1,11 @@ +package addrs + +// InputVariable is the address of an input variable. +type InputVariable struct { + referenceable + Name string +} + +func (v InputVariable) String() string { + return "var." + v.Name +} diff --git a/hcl2template/addrs/parse_ref.go b/hcl2template/addrs/parse_ref.go new file mode 100644 index 000000000..ee3f238ef --- /dev/null +++ b/hcl2template/addrs/parse_ref.go @@ -0,0 +1,93 @@ +package addrs + +import ( + "fmt" + + "github.com/hashicorp/hcl/v2" +) + +// Reference describes a reference to an address with source location +// information. +type Reference struct { + Subject Referenceable + SourceRange hcl.Range + Remaining hcl.Traversal +} + +// ParseRef attempts to extract a referencable address from the prefix of the +// given traversal, which must be an absolute traversal or this function +// will panic. +// +// If no error diagnostics are returned, the returned reference includes the +// address that was extracted, the source range it was extracted from, and any +// remaining relative traversal that was not consumed as part of the +// reference. +// +// If error diagnostics are returned then the Reference value is invalid and +// must not be used. +func ParseRef(traversal hcl.Traversal) (*Reference, hcl.Diagnostics) { + ref, diags := parseRef(traversal) + + // Normalize a little to make life easier for callers. + if ref != nil { + if len(ref.Remaining) == 0 { + ref.Remaining = nil + } + } + + return ref, diags +} + +func parseRef(traversal hcl.Traversal) (*Reference, hcl.Diagnostics) { + var diags hcl.Diagnostics + + root := traversal.RootName() + rootRange := traversal[0].SourceRange() + + switch root { + + case "var": + name, rng, remain, diags := parseSingleAttrRef(traversal) + return &Reference{ + Subject: InputVariable{Name: name}, + SourceRange: rng, + Remaining: remain, + }, diags + + default: + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Unhandled reference type", + Detail: `Currently parseRef can only parse "var" references.`, + Subject: &rootRange, + }) + } + return nil, diags +} + +func parseSingleAttrRef(traversal hcl.Traversal) (string, hcl.Range, hcl.Traversal, hcl.Diagnostics) { + var diags hcl.Diagnostics + + root := traversal.RootName() + rootRange := traversal[0].SourceRange() + + if len(traversal) < 2 { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid reference", + Detail: fmt.Sprintf("The %q object cannot be accessed directly. Instead, access one of its attributes.", root), + Subject: &rootRange, + }) + return "", hcl.Range{}, nil, diags + } + if attrTrav, ok := traversal[1].(hcl.TraverseAttr); ok { + return attrTrav.Name, hcl.RangeBetween(rootRange, attrTrav.SrcRange), traversal[2:], diags + } + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid reference", + Detail: fmt.Sprintf("The %q object does not support this operation.", root), + Subject: traversal[1].SourceRange().Ptr(), + }) + return "", hcl.Range{}, nil, diags +} diff --git a/hcl2template/addrs/referenceable.go b/hcl2template/addrs/referenceable.go new file mode 100644 index 000000000..8c4925c40 --- /dev/null +++ b/hcl2template/addrs/referenceable.go @@ -0,0 +1,18 @@ +package addrs + +// Referenceable is an interface implemented by all address types that can +// appear as references in configuration language expressions. +type Referenceable interface { + referenceableSigil() + + // String produces a string representation of the address that could be + // parsed as a HCL traversal and passed to ParseRef to produce an identical + // result. + String() string +} + +type referenceable struct { +} + +func (r referenceable) referenceableSigil() { +} diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 2598eacfc..d742f44c6 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -3,15 +3,20 @@ package hcl2template import ( "fmt" "strings" + "unicode" "github.com/hashicorp/hcl/v2" "github.com/hashicorp/hcl/v2/ext/typeexpr" "github.com/hashicorp/hcl/v2/gohcl" "github.com/hashicorp/hcl/v2/hclsyntax" + "github.com/hashicorp/packer/hcl2template/addrs" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" ) +// A consistent detail message for all "not a valid identifier" diagnostics. +const badIdentifierDetail = "A name must start with a letter or underscore and may contain only letters, digits, underscores, and dashes." + // Local represents a single entry from a "locals" block in a file. // The "locals" block itself is not represented, because it serves only to // provide context for us to interpret its contents. @@ -47,6 +52,8 @@ type Variable struct { // the variable from the output stream. By replacing the text. Sensitive bool + Validations []*VariableValidation + Range hcl.Range } @@ -139,6 +146,28 @@ func (variables *Variables) decodeVariable(key string, attr *hcl.Attribute, ectx return diags } +var variableBlockSchema = &hcl.BodySchema{ + Attributes: []hcl.AttributeSchema{ + { + Name: "description", + }, + { + Name: "default", + }, + { + Name: "type", + }, + { + Name: "sensitive", + }, + }, + Blocks: []hcl.BlockHeaderSchema{ + { + Type: "validation", + }, + }, +} + // decodeVariableBlock decodes a "variables" section the way packer 1 used to func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.EvalContext) hcl.Diagnostics { if (*variables) == nil { @@ -155,51 +184,53 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval }} } - var b struct { - Description string `hcl:"description,optional"` - Sensitive bool `hcl:"sensitive,optional"` - Rest hcl.Body `hcl:",remain"` - } - diags := gohcl.DecodeBody(block.Body, nil, &b) - - if diags.HasErrors() { - return diags - } - name := block.Labels[0] - res := &Variable{ - Name: name, - Description: b.Description, - Sensitive: b.Sensitive, - Range: block.DefRange, + content, diags := block.Body.Content(variableBlockSchema) + if !hclsyntax.ValidIdentifier(name) { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid variable name", + Detail: badIdentifierDetail, + Subject: &block.LabelRanges[0], + }) } - attrs, moreDiags := b.Rest.JustAttributes() - diags = append(diags, moreDiags...) + v := &Variable{ + Name: name, + Range: block.DefRange, + } - if t, ok := attrs["type"]; ok { - delete(attrs, "type") + if attr, exists := content.Attributes["description"]; exists { + valDiags := gohcl.DecodeExpression(attr.Expr, nil, &v.Description) + diags = append(diags, valDiags...) + } + + if t, ok := content.Attributes["type"]; ok { tp, moreDiags := typeexpr.Type(t.Expr) diags = append(diags, moreDiags...) if moreDiags.HasErrors() { return diags } - res.Type = tp + v.Type = tp } - if def, ok := attrs["default"]; ok { - delete(attrs, "default") + if attr, exists := content.Attributes["sensitive"]; exists { + valDiags := gohcl.DecodeExpression(attr.Expr, nil, &v.Sensitive) + diags = append(diags, valDiags...) + } + + if def, ok := content.Attributes["default"]; ok { defaultValue, moreDiags := def.Expr.Value(ectx) diags = append(diags, moreDiags...) if moreDiags.HasErrors() { return diags } - if res.Type != cty.NilType { + if v.Type != cty.NilType { var err error - defaultValue, err = convert.Convert(defaultValue, res.Type) + defaultValue, err = convert.Convert(defaultValue, v.Type) if err != nil { diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, @@ -211,32 +242,177 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval } } - res.DefaultValue = defaultValue + v.DefaultValue = defaultValue // It's possible no type attribute was assigned so lets make sure we // have a valid type otherwise there could be issues parsing the value. - if res.Type == cty.NilType { - res.Type = res.DefaultValue.Type() + if v.Type == cty.NilType { + v.Type = v.DefaultValue.Type() } } - if len(attrs) > 0 { - keys := []string{} - for k := range attrs { - keys = append(keys, k) - } - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Unknown keys", - Detail: fmt.Sprintf("unknown variable setting(s): %s", keys), - Context: block.DefRange.Ptr(), - }) - } - (*variables)[name] = res + for _, block := range content.Blocks { + switch block.Type { + case "validation": + vv, moreDiags := decodeVariableValidationBlock(v.Name, block) + diags = append(diags, moreDiags...) + v.Validations = append(v.Validations, vv) + } + } + + (*variables)[name] = v return diags } +var variableValidationBlockSchema = &hcl.BodySchema{ + Attributes: []hcl.AttributeSchema{ + { + Name: "condition", + Required: true, + }, + { + Name: "error_message", + Required: true, + }, + }, +} + +// VariableValidation represents a configuration-defined validation rule +// for a particular input variable, given as a "validation" block inside +// a "variable" block. +type VariableValidation struct { + // Condition is an expression that refers to the variable being tested and + // contains no other references. The expression must return true to + // indicate that the value is valid or false to indicate that it is + // invalid. If the expression produces an error, that's considered a bug in + // the block defining the validation rule, not an error in the caller. + Condition hcl.Expression + + // ErrorMessage is one or more full sentences, which would need to be in + // English for consistency with the rest of the error message output but + // can in practice be in any language as long as it ends with a period. + // The message should describe what is required for the condition to return + // true in a way that would make sense to a caller of the module. + ErrorMessage string + + DeclRange hcl.Range +} + +func decodeVariableValidationBlock(varName string, block *hcl.Block) (*VariableValidation, hcl.Diagnostics) { + var diags hcl.Diagnostics + vv := &VariableValidation{ + DeclRange: block.DefRange, + } + + content, moreDiags := block.Body.Content(variableValidationBlockSchema) + diags = append(diags, moreDiags...) + + if attr, exists := content.Attributes["condition"]; exists { + vv.Condition = attr.Expr + + // The validation condition must refer to the variable itself and + // nothing else; to ensure that the variable declaration can't create + // additional edges in the dependency graph. + goodRefs := 0 + for _, traversal := range vv.Condition.Variables() { + + ref, moreDiags := addrs.ParseRef(traversal) + if !moreDiags.HasErrors() { + if addr, ok := ref.Subject.(addrs.InputVariable); ok { + if addr.Name == varName { + goodRefs++ + continue // Reference is valid + } + } + } + + // If we fall out here then the reference is invalid. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid reference in variable validation", + Detail: fmt.Sprintf("The condition for variable %q can only refer to the variable itself, using var.%s.", varName, varName), + Subject: traversal.SourceRange().Ptr(), + }) + } + if goodRefs < 1 { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid variable validation condition", + Detail: fmt.Sprintf("The condition for variable %q must refer to var.%s in order to test incoming values.", varName, varName), + Subject: attr.Expr.Range().Ptr(), + }) + } + } + + if attr, exists := content.Attributes["error_message"]; exists { + moreDiags := gohcl.DecodeExpression(attr.Expr, nil, &vv.ErrorMessage) + diags = append(diags, moreDiags...) + if !moreDiags.HasErrors() { + const errSummary = "Invalid validation error message" + switch { + case vv.ErrorMessage == "": + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: errSummary, + Detail: "An empty string is not a valid nor useful error message.", + Subject: attr.Expr.Range().Ptr(), + }) + case !looksLikeSentences(vv.ErrorMessage): + // Because we're going to include this string verbatim as part + // of a bigger error message written in our usual style in + // English, we'll require the given error message to conform + // to that. We might relax this in future if e.g. we start + // presenting these error messages in a different way, or if + // Terraform starts supporting producing error messages in + // other human languages, etc. + // For pragmatism we also allow sentences ending with + // exclamation points, but we don't mention it explicitly here + // because that's not really consistent with the Terraform UI + // writing style. + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: errSummary, + Detail: "Validation error message must be at least one full English sentence starting with an uppercase letter and ending with a period or question mark.", + Subject: attr.Expr.Range().Ptr(), + }) + } + } + } + + return vv, diags +} + +// looksLikeSentence is a simple heuristic that encourages writing error +// messages that will be presentable when included as part of a larger error +// diagnostic whose other text is written in the UI writing style. +// +// This is intentionally not a very strong validation since we're assuming that +// authors want to write good messages and might just need a nudge about +// Packer's specific style, rather than that they are going to try to work +// around these rules to write a lower-quality message. +func looksLikeSentences(s string) bool { + if len(s) < 1 { + return false + } + runes := []rune(s) // HCL guarantees that all strings are valid UTF-8 + first := runes[0] + last := runes[len(runes)-1] + + // If the first rune is a letter then it must be an uppercase letter. + // (This will only see the first rune in a multi-rune combining sequence, + // but the first rune is generally the letter if any are, and if not then + // we'll just ignore it because we're primarily expecting English messages + // right now anyway, for consistency with all of Terraform's other output.) + if unicode.IsLetter(first) && !unicode.IsUpper(first) { + return false + } + + // The string must be at least one full sentence, which implies having + // sentence-ending punctuation. + return last == '.' || last == '?' || last == '!' +} + // Prefix your environment variables with VarEnvPrefix so that Packer can see // them. const VarEnvPrefix = "PKR_VAR_" diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index b36b6e44f..d1c589d5b 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -131,7 +131,7 @@ func TestParse_variables(t *testing.T) { }, }, }, - true, false, + true, true, []packer.Build{}, false, }, From 4e08ea6a92dc2a0687c3833b44ef07f9a2895a94 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 30 Oct 2020 12:41:29 +0100 Subject: [PATCH 02/17] add a test --- .../variables/validation/valid.pkr.hcl | 9 ++++++++ hcl2template/types.variables_test.go | 22 +++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 hcl2template/testdata/variables/validation/valid.pkr.hcl diff --git a/hcl2template/testdata/variables/validation/valid.pkr.hcl b/hcl2template/testdata/variables/validation/valid.pkr.hcl new file mode 100644 index 000000000..a0bbe2cf5 --- /dev/null +++ b/hcl2template/testdata/variables/validation/valid.pkr.hcl @@ -0,0 +1,9 @@ + +variable "image_id" { + type = string + default = "ami-something-something" + validation { + condition = length(var.image_id) > 4 && substr(var.image_id, 0, 4) == "ami-" + error_message = "The image_id value must be a valid AMI id, starting with \"ami-\"." + } +} \ No newline at end of file diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index d1c589d5b..4a8f697c5 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -357,6 +357,28 @@ func TestParse_variables(t *testing.T) { }, false, }, + + {"valid validation block", + defaultParser, + parseTestArgs{"testdata/variables/validation/valid.pkr.hcl", nil, nil}, + &PackerConfig{ + Basedir: filepath.Join("testdata", "variables", "validation"), + InputVariables: Variables{ + "image_id": &Variable{ + DefaultValue: cty.StringVal("ami-something-something"), + Name: "image_id", + Validations: []*VariableValidation{ + &VariableValidation{ + ErrorMessage: `The image_id value must be a valid AMI id, starting with "ami-".`, + }, + }, + }, + }, + }, + false, false, + []packer.Build{}, + false, + }, } testParse(t, tests) } From 4d386dd806906f546def27fee11a63bcd618ecbb Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 30 Oct 2020 15:36:23 +0100 Subject: [PATCH 03/17] add length function that can work with more types --- hcl2template/function/length.go | 53 +++++++++++++++++++++++++++++++++ hcl2template/functions.go | 2 +- 2 files changed, 54 insertions(+), 1 deletion(-) create mode 100644 hcl2template/function/length.go diff --git a/hcl2template/function/length.go b/hcl2template/function/length.go new file mode 100644 index 000000000..176326ac8 --- /dev/null +++ b/hcl2template/function/length.go @@ -0,0 +1,53 @@ +package function + +import ( + "errors" + + "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/function" + "github.com/zclconf/go-cty/cty/function/stdlib" +) + +var LengthFunc = function.New(&function.Spec{ + Params: []function.Parameter{ + { + Name: "value", + Type: cty.DynamicPseudoType, + AllowDynamicType: true, + AllowUnknown: true, + }, + }, + Type: func(args []cty.Value) (cty.Type, error) { + collTy := args[0].Type() + switch { + case collTy == cty.String || collTy.IsTupleType() || collTy.IsObjectType() || collTy.IsListType() || collTy.IsMapType() || collTy.IsSetType() || collTy == cty.DynamicPseudoType: + return cty.Number, nil + default: + return cty.Number, errors.New("argument must be a string, a collection type, or a structural type") + } + }, + Impl: func(args []cty.Value, retType cty.Type) (cty.Value, error) { + coll := args[0] + collTy := args[0].Type() + switch { + case collTy == cty.DynamicPseudoType: + return cty.UnknownVal(cty.Number), nil + case collTy.IsTupleType(): + l := len(collTy.TupleElementTypes()) + return cty.NumberIntVal(int64(l)), nil + case collTy.IsObjectType(): + l := len(collTy.AttributeTypes()) + return cty.NumberIntVal(int64(l)), nil + case collTy == cty.String: + // We'll delegate to the cty stdlib strlen function here, because + // it deals with all of the complexities of tokenizing unicode + // grapheme clusters. + return stdlib.Strlen(coll) + case collTy.IsListType() || collTy.IsSetType() || collTy.IsMapType(): + return coll.Length(), nil + default: + // Should never happen, because of the checks in our Type func above + return cty.UnknownVal(cty.Number), errors.New("impossible value type for length(...)") + } + }, +}) diff --git a/hcl2template/functions.go b/hcl2template/functions.go index f91abdab7..058a3f8da 100644 --- a/hcl2template/functions.go +++ b/hcl2template/functions.go @@ -68,7 +68,7 @@ func Functions(basedir string) map[string]function.Function { "jsondecode": stdlib.JSONDecodeFunc, "jsonencode": stdlib.JSONEncodeFunc, "keys": stdlib.KeysFunc, - "length": stdlib.LengthFunc, + "length": pkrfunction.LengthFunc, "log": stdlib.LogFunc, "lookup": stdlib.LookupFunc, "lower": stdlib.LowerFunc, From 9932fd1217d612617ca4a92ebc3a69c2d8198124 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 30 Oct 2020 15:38:29 +0100 Subject: [PATCH 04/17] add Variable.validateValue func --- hcl2template/types.variables.go | 86 +++++++++++++++++++++++++++++---- 1 file changed, 77 insertions(+), 9 deletions(-) diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index d742f44c6..a5de7ca47 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -2,6 +2,7 @@ package hcl2template import ( "fmt" + "log" "strings" "unicode" @@ -67,7 +68,76 @@ func (v *Variable) GoString() string { ) } -func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { +// validateValue ensures that all of the configured custom validations for a +// variable value are passing. +// +func (v *Variable) validateValue(val cty.Value) (diags hcl.Diagnostics) { + if len(v.Validations) == 0 { + log.Printf("[TRACE] validateValue: not active for %s, so skipping", v.Name) + return nil + } + + hclCtx := &hcl.EvalContext{ + Variables: map[string]cty.Value{ + "var": cty.ObjectVal(map[string]cty.Value{ + v.Name: val, + }), + }, + Functions: Functions(""), + } + + for _, validation := range v.Validations { + const errInvalidCondition = "Invalid variable validation result" + const errInvalidValue = "Invalid value for variable" + + result, moreDiags := validation.Condition.Value(hclCtx) + diags = append(diags, moreDiags...) + if moreDiags.HasErrors() { + log.Printf("[TRACE] evalVariableValidations: %s rule %s condition expression failed: %s", v.Name, validation.DeclRange, moreDiags.Error()) + } + if !result.IsKnown() { + log.Printf("[TRACE] evalVariableValidations: %s rule %s condition value is unknown, so skipping validation for now", v.Name, validation.DeclRange) + continue // We'll wait until we've learned more, then. + } + if result.IsNull() { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: errInvalidCondition, + Detail: "Validation condition expression must return either true or false, not null.", + Subject: validation.Condition.Range().Ptr(), + Expression: validation.Condition, + EvalContext: hclCtx, + }) + continue + } + var err error + result, err = convert.Convert(result, cty.Bool) + if err != nil { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: errInvalidCondition, + Detail: fmt.Sprintf("Invalid validation condition result value: %s.", err), + Subject: validation.Condition.Range().Ptr(), + Expression: validation.Condition, + EvalContext: hclCtx, + }) + continue + } + + if result.False() { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: errInvalidValue, + Detail: fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", validation.ErrorMessage, validation.DeclRange.String()), + Subject: validation.DeclRange.Ptr(), + }) + } + } + + return diags +} + +func (v *Variable) Value() (cty.Value, hcl.Diagnostics) { for _, value := range []cty.Value{ v.CmdValue, v.VarfileValue, @@ -75,18 +145,18 @@ func (v *Variable) Value() (cty.Value, *hcl.Diagnostic) { v.DefaultValue, } { if value != cty.NilVal { - return value, nil + return value, v.validateValue(value) } } - return cty.UnknownVal(v.Type), &hcl.Diagnostic{ + return cty.UnknownVal(v.Type), hcl.Diagnostics{&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: fmt.Sprintf("Unset variable %q", v.Name), Detail: "A used variable must be set or have a default value; see " + "https://packer.io/docs/configuration/from-1.5/syntax for " + "details.", Context: v.Range.Ptr(), - } + }} } type Variables map[string]*Variable @@ -103,10 +173,8 @@ func (variables Variables) Values() (map[string]cty.Value, hcl.Diagnostics) { res := map[string]cty.Value{} var diags hcl.Diagnostics for k, v := range variables { - value, diag := v.Value() - if diag != nil { - diags = append(diags, diag) - } + value, moreDiags := v.Value() + diags = append(diags, moreDiags...) res[k] = value } return res, diags @@ -486,7 +554,7 @@ func (cfg *PackerConfig) collectInputVariableValues(env []string, files []*hcl.F }) for _, block := range content.Blocks { name := block.Labels[0] - diags = diags.Append(&hcl.Diagnostic{ + diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Variable declaration in a .pkrvar file", Detail: fmt.Sprintf("A .pkrvar file is used to assign "+ From b892414e84f1f953c7a81571499be1f62b902184 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 30 Oct 2020 15:40:31 +0100 Subject: [PATCH 05/17] add failing test case --- .../variables/validation/invalid_default.pkr.hcl | 9 +++++++++ hcl2template/testdata/variables/validation/valid.pkr.hcl | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) create mode 100644 hcl2template/testdata/variables/validation/invalid_default.pkr.hcl diff --git a/hcl2template/testdata/variables/validation/invalid_default.pkr.hcl b/hcl2template/testdata/variables/validation/invalid_default.pkr.hcl new file mode 100644 index 000000000..4fafe369f --- /dev/null +++ b/hcl2template/testdata/variables/validation/invalid_default.pkr.hcl @@ -0,0 +1,9 @@ + +variable "image_id" { + type = string + default = "potato" + validation { + condition = length(var.image_id) > 4 && substr(var.image_id, 0, 4) == "ami-" + error_message = "The image_id value must be a valid AMI id, starting with \"ami-\"." + } +} diff --git a/hcl2template/testdata/variables/validation/valid.pkr.hcl b/hcl2template/testdata/variables/validation/valid.pkr.hcl index a0bbe2cf5..e17aa7193 100644 --- a/hcl2template/testdata/variables/validation/valid.pkr.hcl +++ b/hcl2template/testdata/variables/validation/valid.pkr.hcl @@ -6,4 +6,4 @@ variable "image_id" { condition = length(var.image_id) > 4 && substr(var.image_id, 0, 4) == "ami-" error_message = "The image_id value must be a valid AMI id, starting with \"ami-\"." } -} \ No newline at end of file +} From 8de2f40a076632f97d3239114dd6ef4b66f8553d Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Fri, 30 Oct 2020 15:42:59 +0100 Subject: [PATCH 06/17] add tests for length --- hcl2template/function/length.go | 6 ++ hcl2template/function/length_test.go | 139 +++++++++++++++++++++++++++ 2 files changed, 145 insertions(+) create mode 100644 hcl2template/function/length_test.go diff --git a/hcl2template/function/length.go b/hcl2template/function/length.go index 176326ac8..c1a1e8862 100644 --- a/hcl2template/function/length.go +++ b/hcl2template/function/length.go @@ -51,3 +51,9 @@ var LengthFunc = function.New(&function.Spec{ } }, }) + +// Length returns the number of elements in the given collection or number of +// Unicode characters in the given string. +func Length(collection cty.Value) (cty.Value, error) { + return LengthFunc.Call([]cty.Value{collection}) +} diff --git a/hcl2template/function/length_test.go b/hcl2template/function/length_test.go new file mode 100644 index 000000000..5b217a00a --- /dev/null +++ b/hcl2template/function/length_test.go @@ -0,0 +1,139 @@ +package function + +import ( + "fmt" + "testing" + + "github.com/zclconf/go-cty/cty" +) + +func TestLength(t *testing.T) { + tests := []struct { + Value cty.Value + Want cty.Value + }{ + { + cty.ListValEmpty(cty.Number), + cty.NumberIntVal(0), + }, + { + cty.ListVal([]cty.Value{cty.True}), + cty.NumberIntVal(1), + }, + { + cty.ListVal([]cty.Value{cty.UnknownVal(cty.Bool)}), + cty.NumberIntVal(1), + }, + { + cty.SetValEmpty(cty.Number), + cty.NumberIntVal(0), + }, + { + cty.SetVal([]cty.Value{cty.True}), + cty.NumberIntVal(1), + }, + { + cty.MapValEmpty(cty.Bool), + cty.NumberIntVal(0), + }, + { + cty.MapVal(map[string]cty.Value{"hello": cty.True}), + cty.NumberIntVal(1), + }, + { + cty.EmptyTupleVal, + cty.NumberIntVal(0), + }, + { + cty.UnknownVal(cty.EmptyTuple), + cty.NumberIntVal(0), + }, + { + cty.TupleVal([]cty.Value{cty.True}), + cty.NumberIntVal(1), + }, + { + cty.EmptyObjectVal, + cty.NumberIntVal(0), + }, + { + cty.UnknownVal(cty.EmptyObject), + cty.NumberIntVal(0), + }, + { + cty.ObjectVal(map[string]cty.Value{"true": cty.True}), + cty.NumberIntVal(1), + }, + { + cty.UnknownVal(cty.List(cty.Bool)), + cty.UnknownVal(cty.Number), + }, + { + cty.DynamicVal, + cty.UnknownVal(cty.Number), + }, + { + cty.StringVal("hello"), + cty.NumberIntVal(5), + }, + { + cty.StringVal(""), + cty.NumberIntVal(0), + }, + { + cty.StringVal("1"), + cty.NumberIntVal(1), + }, + { + cty.StringVal("Живой Журнал"), + cty.NumberIntVal(12), + }, + { + // note that the dieresis here is intentionally a combining + // ligature. + cty.StringVal("noël"), + cty.NumberIntVal(4), + }, + { + // The Es in this string has three combining acute accents. + // This tests something that NFC-normalization cannot collapse + // into a single precombined codepoint, since otherwise we might + // be cheating and relying on the single-codepoint forms. + cty.StringVal("wé́́é́́é́́!"), + cty.NumberIntVal(5), + }, + { + // Go's normalization forms don't handle this ligature, so we + // will produce the wrong result but this is now a compatibility + // constraint and so we'll test it. + cty.StringVal("baffle"), + cty.NumberIntVal(4), + }, + { + cty.StringVal("😸😾"), + cty.NumberIntVal(2), + }, + { + cty.UnknownVal(cty.String), + cty.UnknownVal(cty.Number), + }, + { + cty.DynamicVal, + cty.UnknownVal(cty.Number), + }, + } + + for _, test := range tests { + t.Run(fmt.Sprintf("Length(%#v)", test.Value), func(t *testing.T) { + got, err := Length(test.Value) + + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + if !got.RawEquals(test.Want) { + t.Errorf("wrong result\ngot: %#v\nwant: %#v", got, test.Want) + } + }) + } +} From 6911495fc486f1bc582eceeffc7c8b80ce775520 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 2 Nov 2020 11:49:40 +0100 Subject: [PATCH 07/17] add VariableAssignment struct that help describe an input var assignment --- hcl2template/common_test.go | 75 +++--- .../validation/valid_map/definition.pkr.hcl | 17 ++ .../valid_map/invalid_value.auto.pkrvars.hcl | 7 + hcl2template/types.packer_config.go | 10 +- hcl2template/types.packer_config_test.go | 61 +++-- hcl2template/types.variables.go | 124 +++++---- hcl2template/types.variables_test.go | 246 +++++++++++------- 7 files changed, 335 insertions(+), 205 deletions(-) create mode 100644 hcl2template/testdata/variables/validation/valid_map/definition.pkr.hcl create mode 100644 hcl2template/testdata/variables/validation/valid_map/invalid_value.auto.pkrvars.hcl diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index 48eec28fa..9c77cbd7a 100644 --- a/hcl2template/common_test.go +++ b/hcl2template/common_test.go @@ -69,25 +69,7 @@ func testParse(t *testing.T, tests []parseTest) { if tt.parseWantDiagHasErrors != gotDiags.HasErrors() { t.Fatalf("Parser.parse() unexpected diagnostics HasErrors. %s", gotDiags) } - if diff := cmp.Diff(tt.parseWantCfg, gotCfg, - cmpopts.IgnoreUnexported( - PackerConfig{}, - cty.Value{}, - cty.Type{}, - Variable{}, - SourceBlock{}, - ProvisionerBlock{}, - PostProcessorBlock{}, - ), - cmpopts.IgnoreFields(PackerConfig{}, - "Cwd", // Cwd will change for every computer - ), - cmpopts.IgnoreTypes(HCL2Ref{}), - cmpopts.IgnoreTypes([]*LocalBlock{}), - cmpopts.IgnoreTypes([]hcl.Range{}), - cmpopts.IgnoreTypes(hcl.Range{}), - cmpopts.IgnoreInterfaces(struct{ hcl.Expression }{}), - cmpopts.IgnoreInterfaces(struct{ hcl.Body }{}), + if diff := cmp.Diff(tt.parseWantCfg, gotCfg, cmpOpts..., ); diff != "" { t.Fatalf("Parser.parse() wrong packer config. %s", diff) } @@ -96,11 +78,8 @@ func testParse(t *testing.T, tests []parseTest) { gotInputVar := gotCfg.InputVariables for name, value := range tt.parseWantCfg.InputVariables { if variable, ok := gotInputVar[name]; ok { - if diff := cmp.Diff(variable.DefaultValue.GoString(), value.DefaultValue.GoString()); diff != "" { - t.Fatalf("Parser.parse(): unexpected default value for %s: %s", name, diff) - } - if diff := cmp.Diff(variable.VarfileValue.GoString(), value.VarfileValue.GoString()); diff != "" { - t.Fatalf("Parser.parse(): varfile value differs for %s: %s", name, diff) + if diff := cmp.Diff(variable, value, cmpOpts...); diff != "" { + t.Fatalf("Parser.parse(): unexpected variable values %s: %s", name, diff) } } else { t.Fatalf("Parser.parse() missing input variable. %s", name) @@ -110,8 +89,8 @@ func testParse(t *testing.T, tests []parseTest) { gotLocalVar := gotCfg.LocalVariables for name, value := range tt.parseWantCfg.LocalVariables { if variable, ok := gotLocalVar[name]; ok { - if variable.DefaultValue.GoString() != value.DefaultValue.GoString() { - t.Fatalf("Parser.parse() local variable %s expected '%s' but was '%s'", name, value.DefaultValue.GoString(), variable.DefaultValue.GoString()) + if diff := cmp.Diff(variable, value, cmpOpts...); diff != "" { + t.Fatalf("Parser.parse(): unexpected variable values %s: %s", name, diff) } } else { t.Fatalf("Parser.parse() missing local variable. %s", name) @@ -127,18 +106,7 @@ func testParse(t *testing.T, tests []parseTest) { if tt.getBuildsWantDiags == (gotDiags == nil) { t.Fatalf("Parser.getBuilds() unexpected diagnostics. %s", gotDiags) } - if diff := cmp.Diff(tt.getBuildsWantBuilds, gotBuilds, - cmpopts.IgnoreUnexported( - cty.Value{}, - cty.Type{}, - packer.CoreBuild{}, - packer.CoreBuildProvisioner{}, - packer.CoreBuildPostProcessor{}, - null.Builder{}, - HCL2Provisioner{}, - HCL2PostProcessor{}, - ), - ); diff != "" { + if diff := cmp.Diff(tt.getBuildsWantBuilds, gotBuilds, cmpOpts...); diff != "" { t.Fatalf("Parser.getBuilds() wrong packer builds. %s", diff) } }) @@ -250,3 +218,34 @@ var ( }, } ) + +var cmpOpts = []cmp.Option{ + cmpopts.IgnoreUnexported( + PackerConfig{}, + cty.Value{}, + cty.Type{}, + Variable{}, + SourceBlock{}, + ProvisionerBlock{}, + PostProcessorBlock{}, + packer.CoreBuild{}, + HCL2Provisioner{}, + HCL2PostProcessor{}, + packer.CoreBuildPostProcessor{}, + packer.CoreBuildProvisioner{}, + packer.CoreBuildPostProcessor{}, + null.Builder{}, + ), + cmpopts.IgnoreFields(PackerConfig{}, + "Cwd", // Cwd will change for every os type + ), + cmpopts.IgnoreFields(VariableAssignment{}, + "Expr", // its an interface + ), + cmpopts.IgnoreTypes(HCL2Ref{}), + cmpopts.IgnoreTypes([]*LocalBlock{}), + cmpopts.IgnoreTypes([]hcl.Range{}), + cmpopts.IgnoreTypes(hcl.Range{}), + cmpopts.IgnoreInterfaces(struct{ hcl.Expression }{}), + cmpopts.IgnoreInterfaces(struct{ hcl.Body }{}), +} diff --git a/hcl2template/testdata/variables/validation/valid_map/definition.pkr.hcl b/hcl2template/testdata/variables/validation/valid_map/definition.pkr.hcl new file mode 100644 index 000000000..f095e9062 --- /dev/null +++ b/hcl2template/testdata/variables/validation/valid_map/definition.pkr.hcl @@ -0,0 +1,17 @@ + +variable "image_metadata" { + default = { + key: "value", + something: { + foo: "bar", + } + } + validation { + condition = length(var.image_metadata.key) > 4 + error_message = "The image_metadata.key field must be more than 4 runes." + } + validation { + condition = substr(var.image_metadata.something.foo, 0, 3) == "bar" + error_message = "The image_metadata.something.foo field must start with \"bar\"." + } +} diff --git a/hcl2template/testdata/variables/validation/valid_map/invalid_value.auto.pkrvars.hcl b/hcl2template/testdata/variables/validation/valid_map/invalid_value.auto.pkrvars.hcl new file mode 100644 index 000000000..9734f36f6 --- /dev/null +++ b/hcl2template/testdata/variables/validation/valid_map/invalid_value.auto.pkrvars.hcl @@ -0,0 +1,7 @@ + +image_metadata = { + key: "value", + something: { + foo: "woo", + } +} diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index c62436725..daf80a209 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -210,9 +210,13 @@ func (c *PackerConfig) evaluateLocalVariable(local *LocalBlock) hcl.Diagnostics return diags } c.LocalVariables[local.Name] = &Variable{ - Name: local.Name, - DefaultValue: value, - Type: value.Type(), + Name: local.Name, + Values: []VariableAssignment{{ + Value: value, + Expr: local.Expr, + From: "default", + }}, + Type: value.Type(), } return diags diff --git a/hcl2template/types.packer_config_test.go b/hcl2template/types.packer_config_test.go index 90498c172..992e6e80b 100644 --- a/hcl2template/types.packer_config_test.go +++ b/hcl2template/types.packer_config_test.go @@ -25,51 +25,58 @@ func TestParser_complete(t *testing.T) { Basedir: "testdata/complete", InputVariables: Variables{ "foo": &Variable{ - Name: "foo", - DefaultValue: cty.StringVal("value"), + Name: "foo", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("value")}}, }, "image_id": &Variable{ - Name: "image_id", - DefaultValue: cty.StringVal("image-id-default"), + Name: "image_id", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("image-id-default")}}, }, "port": &Variable{ - Name: "port", - DefaultValue: cty.NumberIntVal(42), + Name: "port", + Values: []VariableAssignment{{From: "default", Value: cty.NumberIntVal(42)}}, }, "availability_zone_names": &Variable{ Name: "availability_zone_names", - DefaultValue: cty.ListVal([]cty.Value{ - cty.StringVal("A"), - cty.StringVal("B"), - cty.StringVal("C"), - }), + Values: []VariableAssignment{{ + From: "default", + Value: cty.ListVal([]cty.Value{ + cty.StringVal("A"), + cty.StringVal("B"), + cty.StringVal("C"), + }), + }}, }, }, LocalVariables: Variables{ "feefoo": &Variable{ - Name: "feefoo", - DefaultValue: cty.StringVal("value_image-id-default"), + Name: "feefoo", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("value_image-id-default")}}, }, "standard_tags": &Variable{ Name: "standard_tags", - DefaultValue: cty.ObjectVal(map[string]cty.Value{ - "Component": cty.StringVal("user-service"), - "Environment": cty.StringVal("production"), - }), + Values: []VariableAssignment{{From: "default", + Value: cty.ObjectVal(map[string]cty.Value{ + "Component": cty.StringVal("user-service"), + "Environment": cty.StringVal("production"), + }), + }}, }, "abc_map": &Variable{ Name: "abc_map", - DefaultValue: cty.TupleVal([]cty.Value{ - cty.ObjectVal(map[string]cty.Value{ - "id": cty.StringVal("a"), + Values: []VariableAssignment{{From: "default", + Value: cty.TupleVal([]cty.Value{ + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("a"), + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("b"), + }), + cty.ObjectVal(map[string]cty.Value{ + "id": cty.StringVal("c"), + }), }), - cty.ObjectVal(map[string]cty.Value{ - "id": cty.StringVal("b"), - }), - cty.ObjectVal(map[string]cty.Value{ - "id": cty.StringVal("c"), - }), - }), + }}, }, }, Sources: map[SourceRef]SourceBlock{ diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index a5de7ca47..5ffe57cdf 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -26,15 +26,26 @@ type LocalBlock struct { Expr hcl.Expression } +// VariableAssignment represents a way a variable was set: the expression +// setting it and the value of that expression. It helps pinpoint were +// something was set in diagnostics. +type VariableAssignment struct { + // From tells were it was taken from, command/varfile/env/default + From string + Value cty.Value + Expr hcl.Expression +} + type Variable struct { - // CmdValue, VarfileValue, EnvValue, DefaultValue are possible values of - // the variable; The first value set from these will be the one used. If - // none is set; an error will be returned if a user tries to use the - // Variable. - CmdValue cty.Value - VarfileValue cty.Value - EnvValue cty.Value - DefaultValue cty.Value + // Values contains possible values for the variable; The last value set + // from these will be the one used. If none is set; an error will be + // returned by Value(). + Values []VariableAssignment + + // Validations contains all variables validation rules to be applied to the + // used value. Only the used value - the last value from Values - is + // validated. + Validations []*VariableValidation // Cty Type of the variable. If the default value or a collected value is // not of this type nor can be converted to this type an error diagnostic @@ -53,25 +64,23 @@ type Variable struct { // the variable from the output stream. By replacing the text. Sensitive bool - Validations []*VariableValidation - Range hcl.Range } func (v *Variable) GoString() string { - return fmt.Sprintf("{Type:%s,CmdValue:%s,VarfileValue:%s,EnvValue:%s,DefaultValue:%s}", - v.Type.GoString(), - PrintableCtyValue(v.CmdValue), - PrintableCtyValue(v.VarfileValue), - PrintableCtyValue(v.EnvValue), - PrintableCtyValue(v.DefaultValue), - ) + b := &strings.Builder{} + fmt.Fprintf(b, "{type:%s", v.Type.GoString()) + for _, vv := range v.Values { + fmt.Fprintf(b, ",%s:%s", vv.From, vv.Value) + } + fmt.Fprintf(b, "}") + return b.String() } // validateValue ensures that all of the configured custom validations for a // variable value are passing. // -func (v *Variable) validateValue(val cty.Value) (diags hcl.Diagnostics) { +func (v *Variable) validateValue(val VariableAssignment) (diags hcl.Diagnostics) { if len(v.Validations) == 0 { log.Printf("[TRACE] validateValue: not active for %s, so skipping", v.Name) return nil @@ -80,7 +89,7 @@ func (v *Variable) validateValue(val cty.Value) (diags hcl.Diagnostics) { hclCtx := &hcl.EvalContext{ Variables: map[string]cty.Value{ "var": cty.ObjectVal(map[string]cty.Value{ - v.Name: val, + v.Name: val.Value, }), }, Functions: Functions(""), @@ -88,7 +97,6 @@ func (v *Variable) validateValue(val cty.Value) (diags hcl.Diagnostics) { for _, validation := range v.Validations { const errInvalidCondition = "Invalid variable validation result" - const errInvalidValue = "Invalid value for variable" result, moreDiags := validation.Condition.Value(hclCtx) diags = append(diags, moreDiags...) @@ -125,11 +133,15 @@ func (v *Variable) validateValue(val cty.Value) (diags hcl.Diagnostics) { } if result.False() { + subj := validation.DeclRange.Ptr() + if val.Expr != nil { + subj = val.Expr.Range().Ptr() + } diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, - Summary: errInvalidValue, + Summary: fmt.Sprintf("Invalid value for %s variable", val.From), Detail: fmt.Sprintf("%s\n\nThis was checked by the validation rule at %s.", validation.ErrorMessage, validation.DeclRange.String()), - Subject: validation.DeclRange.Ptr(), + Subject: subj, }) } } @@ -137,26 +149,20 @@ func (v *Variable) validateValue(val cty.Value) (diags hcl.Diagnostics) { return diags } +// Value returns the last found value from the list of variable settings. func (v *Variable) Value() (cty.Value, hcl.Diagnostics) { - for _, value := range []cty.Value{ - v.CmdValue, - v.VarfileValue, - v.EnvValue, - v.DefaultValue, - } { - if value != cty.NilVal { - return value, v.validateValue(value) - } + if len(v.Values) == 0 { + return cty.UnknownVal(v.Type), hcl.Diagnostics{&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: fmt.Sprintf("Unset variable %q", v.Name), + Detail: "A used variable must be set or have a default value; see " + + "https://packer.io/docs/configuration/from-1.5/syntax for " + + "details.", + Context: v.Range.Ptr(), + }} } - - return cty.UnknownVal(v.Type), hcl.Diagnostics{&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: fmt.Sprintf("Unset variable %q", v.Name), - Detail: "A used variable must be set or have a default value; see " + - "https://packer.io/docs/configuration/from-1.5/syntax for " + - "details.", - Context: v.Range.Ptr(), - }} + val := v.Values[len(v.Values)-1] + return val.Value, v.validateValue(v.Values[len(v.Values)-1]) } type Variables map[string]*Variable @@ -205,10 +211,14 @@ func (variables *Variables) decodeVariable(key string, attr *hcl.Attribute, ectx } (*variables)[key] = &Variable{ - Name: key, - DefaultValue: value, - Type: value.Type(), - Range: attr.Range, + Name: key, + Values: []VariableAssignment{{ + From: "default", + Value: value, + Expr: attr.Expr, + }}, + Type: value.Type(), + Range: attr.Range, } return diags @@ -310,12 +320,16 @@ func (variables *Variables) decodeVariableBlock(block *hcl.Block, ectx *hcl.Eval } } - v.DefaultValue = defaultValue + v.Values = append(v.Values, VariableAssignment{ + From: "default", + Value: defaultValue, + Expr: def.Expr, + }) // It's possible no type attribute was assigned so lets make sure we // have a valid type otherwise there could be issues parsing the value. if v.Type == cty.NilType { - v.Type = v.DefaultValue.Type() + v.Type = defaultValue.Type() } } @@ -532,7 +546,11 @@ func (cfg *PackerConfig) collectInputVariableValues(env []string, files []*hcl.F val = cty.DynamicVal } } - variable.EnvValue = val + variable.Values = append(variable.Values, VariableAssignment{ + From: "env", + Value: val, + Expr: expr, + }) } // files will contain files found in the folder then files passed as @@ -616,7 +634,11 @@ func (cfg *PackerConfig) collectInputVariableValues(env []string, files []*hcl.F } } - variable.VarfileValue = val + variable.Values = append(variable.Values, VariableAssignment{ + From: "varfile", + Value: val, + Expr: attr.Expr, + }) } } @@ -660,7 +682,11 @@ func (cfg *PackerConfig) collectInputVariableValues(env []string, files []*hcl.F } } - variable.CmdValue = val + variable.Values = append(variable.Values, VariableAssignment{ + From: "cmd", + Value: val, + Expr: expr, + }) } return diags diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index 4a8f697c5..a6a71d99e 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -25,47 +25,59 @@ func TestParse_variables(t *testing.T) { Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ "image_name": &Variable{ - Name: "image_name", - DefaultValue: cty.StringVal("foo-image-{{user `my_secret`}}"), + Name: "image_name", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo-image-{{user `my_secret`}}")}}, }, "key": &Variable{ - Name: "key", - DefaultValue: cty.StringVal("value"), + Name: "key", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("value")}}, }, "my_secret": &Variable{ - Name: "my_secret", - DefaultValue: cty.StringVal("foo"), + Name: "my_secret", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, }, "image_id": &Variable{ - Name: "image_id", - DefaultValue: cty.StringVal("image-id-default"), + Name: "image_id", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("image-id-default")}}, }, "port": &Variable{ - Name: "port", - DefaultValue: cty.NumberIntVal(42), + Name: "port", + Values: []VariableAssignment{{From: "default", Value: cty.NumberIntVal(42)}}, }, "availability_zone_names": &Variable{ Name: "availability_zone_names", - DefaultValue: cty.ListVal([]cty.Value{ - cty.StringVal("us-west-1a"), - }), + Values: []VariableAssignment{{ + From: "default", + Value: cty.ListVal([]cty.Value{ + cty.StringVal("us-west-1a"), + }), + }}, Description: fmt.Sprintln("Describing is awesome ;D"), }, "super_secret_password": &Variable{ - Name: "super_secret_password", - Sensitive: true, - DefaultValue: cty.NullVal(cty.String), - Description: fmt.Sprintln("Handle with care plz"), + Name: "super_secret_password", + Sensitive: true, + Values: []VariableAssignment{{ + From: "default", + Value: cty.NullVal(cty.String), + }}, + Description: fmt.Sprintln("Handle with care plz"), }, }, LocalVariables: Variables{ "owner": &Variable{ - Name: "owner", - DefaultValue: cty.StringVal("Community Team"), + Name: "owner", + Values: []VariableAssignment{{ + From: "default", + Value: cty.StringVal("Community Team"), + }}, }, "service_name": &Variable{ - Name: "service_name", - DefaultValue: cty.StringVal("forum"), + Name: "service_name", + Values: []VariableAssignment{{ + From: "default", + Value: cty.StringVal("forum"), + }}, }, }, }, @@ -81,6 +93,10 @@ func TestParse_variables(t *testing.T) { InputVariables: Variables{ "boolean_value": &Variable{ Name: "boolean_value", + Values: []VariableAssignment{{ + From: "default", + Value: cty.BoolVal(false), + }}, }, }, }, @@ -96,6 +112,10 @@ func TestParse_variables(t *testing.T) { InputVariables: Variables{ "boolean_value": &Variable{ Name: "boolean_value", + Values: []VariableAssignment{{ + From: "default", + Value: cty.BoolVal(false), + }}, }, }, }, @@ -111,6 +131,10 @@ func TestParse_variables(t *testing.T) { InputVariables: Variables{ "broken_type": &Variable{ Name: "broken_type", + Values: []VariableAssignment{{ + From: "default", + Value: cty.UnknownVal(cty.DynamicPseudoType), + }}, }, }, }, @@ -126,8 +150,8 @@ func TestParse_variables(t *testing.T) { Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ "broken_variable": &Variable{ - Name: "broken_variable", - DefaultValue: cty.BoolVal(true), + Name: "broken_variable", + Values: []VariableAssignment{{From: "default", Value: cty.BoolVal(true)}}, }, }, }, @@ -196,34 +220,37 @@ func TestParse_variables(t *testing.T) { Basedir: "testdata/variables/complicated", InputVariables: Variables{ "name_prefix": &Variable{ - Name: "name_prefix", - DefaultValue: cty.StringVal("foo"), + Name: "name_prefix", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, }, }, LocalVariables: Variables{ "name_prefix": &Variable{ - Name: "name_prefix", - DefaultValue: cty.StringVal("foo"), + Name: "name_prefix", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, }, "foo": &Variable{ - Name: "foo", - DefaultValue: cty.StringVal("foo"), + Name: "foo", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, }, "bar": &Variable{ - Name: "bar", - DefaultValue: cty.StringVal("foo"), + Name: "bar", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, }, "for_var": &Variable{ - Name: "for_var", - DefaultValue: cty.StringVal("foo"), + Name: "for_var", + Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, }, "bar_var": &Variable{ Name: "bar_var", - DefaultValue: cty.TupleVal([]cty.Value{ - cty.StringVal("foo"), - cty.StringVal("foo"), - cty.StringVal("foo"), - }), + Values: []VariableAssignment{{ + From: "default", + Value: cty.TupleVal([]cty.Value{ + cty.StringVal("foo"), + cty.StringVal("foo"), + cty.StringVal("foo"), + }), + }}, }, }, }, @@ -250,9 +277,11 @@ func TestParse_variables(t *testing.T) { Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ "foo": &Variable{ - DefaultValue: cty.StringVal("bar"), - Name: "foo", - VarfileValue: cty.StringVal("wee"), + Name: "foo", + Values: []VariableAssignment{ + VariableAssignment{"default", cty.StringVal("bar"), nil}, + VariableAssignment{"varfile", cty.StringVal("wee"), nil}, + }, }, }, }, @@ -279,14 +308,14 @@ func TestParse_variables(t *testing.T) { Basedir: filepath.Join("testdata", "variables"), InputVariables: Variables{ "max_retries": &Variable{ - Name: "max_retries", - DefaultValue: cty.StringVal("1"), - Type: cty.String, + Name: "max_retries", + Values: []VariableAssignment{{"default", cty.StringVal("1"), nil}}, + Type: cty.String, }, "max_retries_int": &Variable{ - Name: "max_retries_int", - DefaultValue: cty.NumberIntVal(1), - Type: cty.Number, + Name: "max_retries_int", + Values: []VariableAssignment{{"default", cty.NumberIntVal(1), nil}}, + Type: cty.Number, }, }, Sources: map[SourceRef]SourceBlock{ @@ -365,8 +394,10 @@ func TestParse_variables(t *testing.T) { Basedir: filepath.Join("testdata", "variables", "validation"), InputVariables: Variables{ "image_id": &Variable{ - DefaultValue: cty.StringVal("ami-something-something"), - Name: "image_id", + Values: []VariableAssignment{ + {"default", cty.StringVal("ami-something-something"), nil}, + }, + Name: "image_id", Validations: []*VariableValidation{ &VariableValidation{ ErrorMessage: `The image_id value must be a valid AMI id, starting with "ami-".`, @@ -379,6 +410,28 @@ func TestParse_variables(t *testing.T) { []packer.Build{}, false, }, + + {"valid validation block - invalid default", + defaultParser, + parseTestArgs{"testdata/variables/validation/invalid_default.pkr.hcl", nil, nil}, + &PackerConfig{ + Basedir: filepath.Join("testdata", "variables", "validation"), + InputVariables: Variables{ + "image_id": &Variable{ + Values: []VariableAssignment{{"default", cty.StringVal("ami-something-something"), nil}}, + Name: "image_id", + Validations: []*VariableValidation{ + &VariableValidation{ + ErrorMessage: `The image_id value must be a valid AMI id, starting with "ami-".`, + }, + }, + }, + }, + }, + true, true, + nil, + false, + }, } testParse(t, tests) } @@ -402,8 +455,10 @@ func TestVariables_collectVariableValues(t *testing.T) { {name: "string", variables: Variables{"used_string": &Variable{ - DefaultValue: cty.StringVal("default_value"), - Type: cty.String, + Values: []VariableAssignment{ + {"default", cty.StringVal("default_value"), nil}, + }, + Type: cty.String, }}, args: args{ env: []string{`PKR_VAR_used_string=env_value`}, @@ -420,11 +475,14 @@ func TestVariables_collectVariableValues(t *testing.T) { wantDiags: false, wantVariables: Variables{ "used_string": &Variable{ - Type: cty.String, - CmdValue: cty.StringVal("cmd_value"), - VarfileValue: cty.StringVal("varfile_value"), - EnvValue: cty.StringVal("env_value"), - DefaultValue: cty.StringVal("default_value"), + Type: cty.String, + Values: []VariableAssignment{ + {"default", cty.StringVal(`"default_value"`), nil}, + {"env", cty.StringVal(`"env_value"`), nil}, + {"varfile", cty.StringVal(`"xy"`), nil}, + {"varfile", cty.StringVal(`"varfile_value"`), nil}, + {"cmd", cty.StringVal(`"cmd_value"`), nil}, + }, }, }, wantValues: map[string]cty.Value{ @@ -434,8 +492,10 @@ func TestVariables_collectVariableValues(t *testing.T) { {name: "quoted string", variables: Variables{"quoted_string": &Variable{ - DefaultValue: cty.StringVal(`"default_value"`), - Type: cty.String, + Values: []VariableAssignment{ + {"default", cty.StringVal(`"default_value"`), nil}, + }, + Type: cty.String, }}, args: args{ env: []string{`PKR_VAR_quoted_string="env_value"`}, @@ -452,11 +512,14 @@ func TestVariables_collectVariableValues(t *testing.T) { wantDiags: false, wantVariables: Variables{ "quoted_string": &Variable{ - Type: cty.String, - CmdValue: cty.StringVal(`"cmd_value"`), - VarfileValue: cty.StringVal(`"varfile_value"`), - EnvValue: cty.StringVal(`"env_value"`), - DefaultValue: cty.StringVal(`"default_value"`), + Type: cty.String, + Values: []VariableAssignment{ + {"default", cty.StringVal(`"default_value"`), nil}, + {"env", cty.StringVal(`"env_value"`), nil}, + {"varfile", cty.StringVal(`"xy"`), nil}, + {"varfile", cty.StringVal(`"varfile_value"`), nil}, + {"cmd", cty.StringVal(`"cmd_value"`), nil}, + }, }, }, wantValues: map[string]cty.Value{ @@ -466,8 +529,10 @@ func TestVariables_collectVariableValues(t *testing.T) { {name: "array of strings", variables: Variables{"used_strings": &Variable{ - DefaultValue: stringListVal("default_value_1"), - Type: cty.List(cty.String), + Values: []VariableAssignment{ + {"default", stringListVal("default_value_1"), nil}, + }, + Type: cty.List(cty.String), }}, args: args{ env: []string{`PKR_VAR_used_strings=["env_value_1", "env_value_2"]`}, @@ -484,11 +549,14 @@ func TestVariables_collectVariableValues(t *testing.T) { wantDiags: false, wantVariables: Variables{ "used_strings": &Variable{ - Type: cty.List(cty.String), - CmdValue: stringListVal("cmd_value_1"), - VarfileValue: stringListVal("varfile_value_1"), - EnvValue: stringListVal("env_value_1", "env_value_2"), - DefaultValue: stringListVal("default_value_1"), + Type: cty.List(cty.String), + Values: []VariableAssignment{ + {"default", stringListVal("default_value_1"), nil}, + {"env", stringListVal("env_value_1", "env_value_2"), nil}, + {"varfile", stringListVal("xy"), nil}, + {"varfile", stringListVal("varfile_value_1"), nil}, + {"cmd", stringListVal("cmd_value_1"), nil}, + }, }, }, wantValues: map[string]cty.Value{ @@ -498,8 +566,8 @@ func TestVariables_collectVariableValues(t *testing.T) { {name: "bool", variables: Variables{"enabled": &Variable{ - DefaultValue: cty.False, - Type: cty.Bool, + Values: []VariableAssignment{{"default", cty.False, nil}}, + Type: cty.Bool, }}, args: args{ env: []string{`PKR_VAR_enabled=true`}, @@ -515,11 +583,13 @@ func TestVariables_collectVariableValues(t *testing.T) { wantDiags: false, wantVariables: Variables{ "enabled": &Variable{ - Type: cty.Bool, - CmdValue: cty.True, - VarfileValue: cty.False, - EnvValue: cty.True, - DefaultValue: cty.False, + Type: cty.Bool, + Values: []VariableAssignment{ + {"default", cty.False, nil}, + {"env", cty.True, nil}, + {"varfile", cty.False, nil}, + {"cmd", cty.True, nil}, + }, }, }, wantValues: map[string]cty.Value{ @@ -529,8 +599,8 @@ func TestVariables_collectVariableValues(t *testing.T) { {name: "invalid env var", variables: Variables{"used_string": &Variable{ - DefaultValue: cty.StringVal("default_value"), - Type: cty.String, + Values: []VariableAssignment{{"default", cty.StringVal("default_value"), nil}}, + Type: cty.String, }}, args: args{ env: []string{`PKR_VAR_used_string`}, @@ -540,8 +610,8 @@ func TestVariables_collectVariableValues(t *testing.T) { wantDiags: false, wantVariables: Variables{ "used_string": &Variable{ - Type: cty.String, - DefaultValue: cty.StringVal("default_value"), + Type: cty.String, + Values: []VariableAssignment{{"default", cty.StringVal("default_value"), nil}}, }, }, wantValues: map[string]cty.Value{ @@ -620,8 +690,8 @@ func TestVariables_collectVariableValues(t *testing.T) { wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ - Type: cty.List(cty.String), - EnvValue: cty.DynamicVal, + Type: cty.List(cty.String), + Values: []VariableAssignment{{"env", cty.DynamicVal, nil}}, }, }, wantValues: map[string]cty.Value{ @@ -644,8 +714,8 @@ func TestVariables_collectVariableValues(t *testing.T) { wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ - Type: cty.Bool, - VarfileValue: cty.DynamicVal, + Type: cty.Bool, + Values: []VariableAssignment{{"varfile", cty.DynamicVal, nil}}, }, }, wantValues: map[string]cty.Value{ @@ -670,8 +740,8 @@ func TestVariables_collectVariableValues(t *testing.T) { wantDiagsHasError: true, wantVariables: Variables{ "used_string": &Variable{ - Type: cty.Bool, - CmdValue: cty.DynamicVal, + Type: cty.Bool, + Values: []VariableAssignment{{"cmd", cty.DynamicVal, nil}}, }, }, wantValues: map[string]cty.Value{ @@ -714,7 +784,7 @@ func TestVariables_collectVariableValues(t *testing.T) { if tt.wantDiagsHasError != gotDiags.HasErrors() { t.Fatalf("Variables.collectVariableValues() unexpected diagnostics HasErrors. %s", gotDiags) } - if diff := cmp.Diff(fmt.Sprintf("%#v", tt.wantVariables), fmt.Sprintf("%#v", tt.variables)); diff != "" { + if diff := cmp.Diff(tt.wantVariables, tt.variables, cmpOpts...); diff != "" { t.Fatalf("didn't get expected variables: %s", diff) } values := map[string]cty.Value{} From 6dd06fad144ecde83923c7b3fa8a8fc83ab15bd4 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 2 Nov 2020 15:50:38 +0100 Subject: [PATCH 08/17] add command/ tests --- command/build_test.go | 46 +++++++++++++++++++ .../hcl/validation/map/definition.pkr.hcl | 19 ++++++++ .../validation/map/invalid_value.pkrvars.hcl | 7 +++ .../validation/map/valid_value.pkrvars.hcl | 7 +++ 4 files changed, 79 insertions(+) create mode 100644 command/test-fixtures/hcl/validation/map/definition.pkr.hcl create mode 100644 command/test-fixtures/hcl/validation/map/invalid_value.pkrvars.hcl create mode 100644 command/test-fixtures/hcl/validation/map/valid_value.pkrvars.hcl diff --git a/command/build_test.go b/command/build_test.go index 9e08e6dd8..60660ff09 100644 --- a/command/build_test.go +++ b/command/build_test.go @@ -324,6 +324,52 @@ func TestBuild(t *testing.T) { }, }, }, + + { + name: "hcl - valid validation rule for default value", + args: []string{ + filepath.Join(testFixture("hcl", "validation", "map")), + }, + expectedCode: 0, + }, + + { + name: "hcl - valid setting from varfile", + args: []string{ + "-var-file", filepath.Join(testFixture("hcl", "validation", "map", "valid_value.pkrvars.hcl")), + filepath.Join(testFixture("hcl", "validation", "map")), + }, + expectedCode: 0, + }, + + { + name: "hcl - invalid setting from varfile", + args: []string{ + "-var-file", filepath.Join(testFixture("hcl", "validation", "map", "invalid_value.pkrvars.hcl")), + filepath.Join(testFixture("hcl", "validation", "map")), + }, + expectedCode: 1, + }, + + { + name: "hcl - valid cmd ( invalid varfile bypased )", + args: []string{ + "-var-file", filepath.Join(testFixture("hcl", "validation", "map", "invalid_value.pkrvars.hcl")), + "-var", `image_metadata={key = "new_value", something = { foo = "bar" }}`, + filepath.Join(testFixture("hcl", "validation", "map")), + }, + expectedCode: 0, + }, + + { + name: "hcl - invalid cmd ( valid varfile bypased )", + args: []string{ + "-var-file", filepath.Join(testFixture("hcl", "validation", "map", "valid_value.pkrvars.hcl")), + "-var", `image_metadata={key = "?", something = { foo = "wrong" }}`, + filepath.Join(testFixture("hcl", "validation", "map")), + }, + expectedCode: 1, + }, } for _, tt := range tc { diff --git a/command/test-fixtures/hcl/validation/map/definition.pkr.hcl b/command/test-fixtures/hcl/validation/map/definition.pkr.hcl new file mode 100644 index 000000000..1e60d70bd --- /dev/null +++ b/command/test-fixtures/hcl/validation/map/definition.pkr.hcl @@ -0,0 +1,19 @@ + +variable "image_metadata" { + default = { + key: "value", + something: { + foo: "bar", + } + } + validation { + condition = length(var.image_metadata.key) > 4 + error_message = "The image_metadata.key field must be more than 4 runes." + } + validation { + condition = substr(var.image_metadata.something.foo, 0, 3) == "bar" + error_message = "The image_metadata.something.foo field must start with \"bar\"." + } +} + +build {} diff --git a/command/test-fixtures/hcl/validation/map/invalid_value.pkrvars.hcl b/command/test-fixtures/hcl/validation/map/invalid_value.pkrvars.hcl new file mode 100644 index 000000000..9734f36f6 --- /dev/null +++ b/command/test-fixtures/hcl/validation/map/invalid_value.pkrvars.hcl @@ -0,0 +1,7 @@ + +image_metadata = { + key: "value", + something: { + foo: "woo", + } +} diff --git a/command/test-fixtures/hcl/validation/map/valid_value.pkrvars.hcl b/command/test-fixtures/hcl/validation/map/valid_value.pkrvars.hcl new file mode 100644 index 000000000..250fcfc8f --- /dev/null +++ b/command/test-fixtures/hcl/validation/map/valid_value.pkrvars.hcl @@ -0,0 +1,7 @@ + +image_metadata = { + key: "value", + something: { + foo: "barwoo", + } +} From 88175873e561f1b514b9a4285f285b35643005ee Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 2 Nov 2020 16:52:21 +0100 Subject: [PATCH 09/17] fix tests to actually check cty values & types --- hcl2template/common_test.go | 18 +++++++++-- hcl2template/types.packer_config_test.go | 20 ++++++++++++ hcl2template/types.variables_test.go | 40 ++++++++++++++++++++---- 3 files changed, 70 insertions(+), 8 deletions(-) diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index 9c77cbd7a..0d15e714b 100644 --- a/hcl2template/common_test.go +++ b/hcl2template/common_test.go @@ -219,11 +219,25 @@ var ( } ) +var ctyValueComparer = cmp.Comparer(func(x, y cty.Value) bool { + return x.RawEquals(y) +}) + +var ctyTypeComparer = cmp.Comparer(func(x, y cty.Type) bool { + if x == cty.NilType && y == cty.NilType { + return true + } + if x == cty.NilType || y == cty.NilType { + return false + } + return x.Equals(y) +}) + var cmpOpts = []cmp.Option{ + ctyValueComparer, + ctyTypeComparer, cmpopts.IgnoreUnexported( PackerConfig{}, - cty.Value{}, - cty.Type{}, Variable{}, SourceBlock{}, ProvisionerBlock{}, diff --git a/hcl2template/types.packer_config_test.go b/hcl2template/types.packer_config_test.go index 992e6e80b..662026da1 100644 --- a/hcl2template/types.packer_config_test.go +++ b/hcl2template/types.packer_config_test.go @@ -27,14 +27,17 @@ func TestParser_complete(t *testing.T) { "foo": &Variable{ Name: "foo", Values: []VariableAssignment{{From: "default", Value: cty.StringVal("value")}}, + Type: cty.String, }, "image_id": &Variable{ Name: "image_id", Values: []VariableAssignment{{From: "default", Value: cty.StringVal("image-id-default")}}, + Type: cty.String, }, "port": &Variable{ Name: "port", Values: []VariableAssignment{{From: "default", Value: cty.NumberIntVal(42)}}, + Type: cty.Number, }, "availability_zone_names": &Variable{ Name: "availability_zone_names", @@ -46,12 +49,14 @@ func TestParser_complete(t *testing.T) { cty.StringVal("C"), }), }}, + Type: cty.List(cty.String), }, }, LocalVariables: Variables{ "feefoo": &Variable{ Name: "feefoo", Values: []VariableAssignment{{From: "default", Value: cty.StringVal("value_image-id-default")}}, + Type: cty.String, }, "standard_tags": &Variable{ Name: "standard_tags", @@ -61,6 +66,10 @@ func TestParser_complete(t *testing.T) { "Environment": cty.StringVal("production"), }), }}, + Type: cty.Object(map[string]cty.Type{ + "Component": cty.String, + "Environment": cty.String, + }), }, "abc_map": &Variable{ Name: "abc_map", @@ -77,6 +86,17 @@ func TestParser_complete(t *testing.T) { }), }), }}, + Type: cty.Tuple([]cty.Type{ + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + cty.Object(map[string]cty.Type{ + "id": cty.String, + }), + }), }, }, Sources: map[SourceRef]SourceBlock{ diff --git a/hcl2template/types.variables_test.go b/hcl2template/types.variables_test.go index a6a71d99e..105c0b8c9 100644 --- a/hcl2template/types.variables_test.go +++ b/hcl2template/types.variables_test.go @@ -26,22 +26,27 @@ func TestParse_variables(t *testing.T) { InputVariables: Variables{ "image_name": &Variable{ Name: "image_name", + Type: cty.String, Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo-image-{{user `my_secret`}}")}}, }, "key": &Variable{ Name: "key", + Type: cty.String, Values: []VariableAssignment{{From: "default", Value: cty.StringVal("value")}}, }, "my_secret": &Variable{ Name: "my_secret", + Type: cty.String, Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, }, "image_id": &Variable{ Name: "image_id", + Type: cty.String, Values: []VariableAssignment{{From: "default", Value: cty.StringVal("image-id-default")}}, }, "port": &Variable{ Name: "port", + Type: cty.Number, Values: []VariableAssignment{{From: "default", Value: cty.NumberIntVal(42)}}, }, "availability_zone_names": &Variable{ @@ -52,6 +57,7 @@ func TestParse_variables(t *testing.T) { cty.StringVal("us-west-1a"), }), }}, + Type: cty.List(cty.String), Description: fmt.Sprintln("Describing is awesome ;D"), }, "super_secret_password": &Variable{ @@ -61,6 +67,7 @@ func TestParse_variables(t *testing.T) { From: "default", Value: cty.NullVal(cty.String), }}, + Type: cty.String, Description: fmt.Sprintln("Handle with care plz"), }, }, @@ -71,6 +78,7 @@ func TestParse_variables(t *testing.T) { From: "default", Value: cty.StringVal("Community Team"), }}, + Type: cty.String, }, "service_name": &Variable{ Name: "service_name", @@ -78,6 +86,7 @@ func TestParse_variables(t *testing.T) { From: "default", Value: cty.StringVal("forum"), }}, + Type: cty.String, }, }, }, @@ -97,6 +106,7 @@ func TestParse_variables(t *testing.T) { From: "default", Value: cty.BoolVal(false), }}, + Type: cty.Bool, }, }, }, @@ -116,6 +126,7 @@ func TestParse_variables(t *testing.T) { From: "default", Value: cty.BoolVal(false), }}, + Type: cty.Bool, }, }, }, @@ -135,6 +146,7 @@ func TestParse_variables(t *testing.T) { From: "default", Value: cty.UnknownVal(cty.DynamicPseudoType), }}, + Type: cty.List(cty.String), }, }, }, @@ -152,6 +164,7 @@ func TestParse_variables(t *testing.T) { "broken_variable": &Variable{ Name: "broken_variable", Values: []VariableAssignment{{From: "default", Value: cty.BoolVal(true)}}, + Type: cty.Bool, }, }, }, @@ -168,6 +181,7 @@ func TestParse_variables(t *testing.T) { InputVariables: Variables{ "foo": &Variable{ Name: "foo", + Type: cty.String, }, }, }, @@ -184,6 +198,7 @@ func TestParse_variables(t *testing.T) { InputVariables: Variables{ "foo": &Variable{ Name: "foo", + Type: cty.String, }, }, Sources: map[SourceRef]SourceBlock{ @@ -222,24 +237,29 @@ func TestParse_variables(t *testing.T) { "name_prefix": &Variable{ Name: "name_prefix", Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, + Type: cty.String, }, }, LocalVariables: Variables{ "name_prefix": &Variable{ Name: "name_prefix", Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, + Type: cty.String, }, "foo": &Variable{ Name: "foo", Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, + Type: cty.String, }, "bar": &Variable{ Name: "bar", Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, + Type: cty.String, }, "for_var": &Variable{ Name: "for_var", Values: []VariableAssignment{{From: "default", Value: cty.StringVal("foo")}}, + Type: cty.String, }, "bar_var": &Variable{ Name: "bar_var", @@ -251,6 +271,11 @@ func TestParse_variables(t *testing.T) { cty.StringVal("foo"), }), }}, + Type: cty.Tuple([]cty.Type{ + cty.String, + cty.String, + cty.String, + }), }, }, }, @@ -282,6 +307,7 @@ func TestParse_variables(t *testing.T) { VariableAssignment{"default", cty.StringVal("bar"), nil}, VariableAssignment{"varfile", cty.StringVal("wee"), nil}, }, + Type: cty.String, }, }, }, @@ -398,6 +424,7 @@ func TestParse_variables(t *testing.T) { {"default", cty.StringVal("ami-something-something"), nil}, }, Name: "image_id", + Type: cty.String, Validations: []*VariableValidation{ &VariableValidation{ ErrorMessage: `The image_id value must be a valid AMI id, starting with "ami-".`, @@ -418,8 +445,9 @@ func TestParse_variables(t *testing.T) { Basedir: filepath.Join("testdata", "variables", "validation"), InputVariables: Variables{ "image_id": &Variable{ - Values: []VariableAssignment{{"default", cty.StringVal("ami-something-something"), nil}}, + Values: []VariableAssignment{{"default", cty.StringVal("potato"), nil}}, Name: "image_id", + Type: cty.String, Validations: []*VariableValidation{ &VariableValidation{ ErrorMessage: `The image_id value must be a valid AMI id, starting with "ami-".`, @@ -477,11 +505,11 @@ func TestVariables_collectVariableValues(t *testing.T) { "used_string": &Variable{ Type: cty.String, Values: []VariableAssignment{ - {"default", cty.StringVal(`"default_value"`), nil}, - {"env", cty.StringVal(`"env_value"`), nil}, - {"varfile", cty.StringVal(`"xy"`), nil}, - {"varfile", cty.StringVal(`"varfile_value"`), nil}, - {"cmd", cty.StringVal(`"cmd_value"`), nil}, + {"default", cty.StringVal(`default_value`), nil}, + {"env", cty.StringVal(`env_value`), nil}, + {"varfile", cty.StringVal(`xy`), nil}, + {"varfile", cty.StringVal(`varfile_value`), nil}, + {"cmd", cty.StringVal(`cmd_value`), nil}, }, }, }, From d919fc28ab99b24ab407312598cd8057a3a9affe Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 2 Nov 2020 17:20:46 +0100 Subject: [PATCH 10/17] add doc --- .../pages/docs/from-1.5/blocks/variable.mdx | 2 + website/pages/docs/from-1.5/variables.mdx | 2 + .../from-1.5/variables/custom-validation.mdx | 43 +++++++++++++++++++ 3 files changed, 47 insertions(+) create mode 100644 website/pages/partials/from-1.5/variables/custom-validation.mdx diff --git a/website/pages/docs/from-1.5/blocks/variable.mdx b/website/pages/docs/from-1.5/blocks/variable.mdx index e7af7093a..1b423ca6f 100644 --- a/website/pages/docs/from-1.5/blocks/variable.mdx +++ b/website/pages/docs/from-1.5/blocks/variable.mdx @@ -24,6 +24,8 @@ If a default value is set, the variable is optional. Otherwise, the variable `@include 'from-1.5/variables/assignment.mdx'` +`@include 'from-1.5/variables/custom-validation.mdx'` + Example of a variable assignment from a file: `@include 'from-1.5/variables/foo-pkrvar.mdx'` diff --git a/website/pages/docs/from-1.5/variables.mdx b/website/pages/docs/from-1.5/variables.mdx index b22792667..73c5c3746 100644 --- a/website/pages/docs/from-1.5/variables.mdx +++ b/website/pages/docs/from-1.5/variables.mdx @@ -162,6 +162,8 @@ maintainers, use comments. The following sections describe these options in more detail. +`@include 'from-1.5/variables/custom-validation.mdx'` + ### Variables on the Command Line To specify individual variables on the command line, use the `-var` option when diff --git a/website/pages/partials/from-1.5/variables/custom-validation.mdx b/website/pages/partials/from-1.5/variables/custom-validation.mdx new file mode 100644 index 000000000..5a9757a0f --- /dev/null +++ b/website/pages/partials/from-1.5/variables/custom-validation.mdx @@ -0,0 +1,43 @@ +## Custom Validation Rules + +In addition to Type Constraints, you can specify arbitrary custom validation +rules for a particular variable using one or more `validation` block nested +within the corresponding `variable` block: + +```hcl +variable "image_id" { + type = string + description = "The id of the machine image (AMI) to use for the server." + + validation { + condition = length(var.image_id) > 4 && substr(var.image_id, 0, 4) == "ami-" + error_message = "The image_id value must be a valid AMI id, starting with \"ami-\"." + } +} +``` + +The `condition` argument is an expression that must use the value of the +variable to return `true` if the value is valid or `false` if it is invalid. +The expression can refer only to the variable that the condition applies to, +and _must not_ produce errors. + +If the failure of an expression is the basis of the validation decision, use +[the `can` function](./functions/can.html) to detect such errors. For example: + +```hcl +variable "image_id" { + type = string + description = "The id of the machine image (AMI) to use for the server." + + validation { + # regex(...) fails if it cannot find a match + condition = can(regex("^ami-", var.image_id)) + error_message = "The image_id value must be a valid AMI id, starting with \"ami-\"." + } +} +``` + +If `condition` evaluates to `false`, an error message including the sentences +given in `error_message` will be produced. The error message string should be +at least one full sentence explaining the constraint that failed, using a +sentence structure similar to the above examples. \ No newline at end of file From 971254928aa27b716d609c0b0b5fbc1bfbc92c19 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 2 Nov 2020 17:43:21 +0100 Subject: [PATCH 11/17] various fixes --- hcl2template/common_test.go | 3 +-- hcl2template/types.variables.go | 6 +++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index 0d15e714b..a5c8b90bd 100644 --- a/hcl2template/common_test.go +++ b/hcl2template/common_test.go @@ -69,8 +69,7 @@ func testParse(t *testing.T, tests []parseTest) { if tt.parseWantDiagHasErrors != gotDiags.HasErrors() { t.Fatalf("Parser.parse() unexpected diagnostics HasErrors. %s", gotDiags) } - if diff := cmp.Diff(tt.parseWantCfg, gotCfg, cmpOpts..., - ); diff != "" { + if diff := cmp.Diff(tt.parseWantCfg, gotCfg, cmpOpts...); diff != "" { t.Fatalf("Parser.parse() wrong packer config. %s", diff) } diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 5ffe57cdf..2f6303741 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -446,11 +446,11 @@ func decodeVariableValidationBlock(varName string, block *hcl.Block) (*VariableV // English, we'll require the given error message to conform // to that. We might relax this in future if e.g. we start // presenting these error messages in a different way, or if - // Terraform starts supporting producing error messages in + // Packer starts supporting producing error messages in // other human languages, etc. // For pragmatism we also allow sentences ending with // exclamation points, but we don't mention it explicitly here - // because that's not really consistent with the Terraform UI + // because that's not really consistent with the Packer UI // writing style. diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, @@ -485,7 +485,7 @@ func looksLikeSentences(s string) bool { // (This will only see the first rune in a multi-rune combining sequence, // but the first rune is generally the letter if any are, and if not then // we'll just ignore it because we're primarily expecting English messages - // right now anyway, for consistency with all of Terraform's other output.) + // right now anyway, for consistency with all of Packers's other output.) if unicode.IsLetter(first) && !unicode.IsUpper(first) { return false } From 20b7fd9687a7440c53b81b8c0d805751fbe7e333 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 2 Nov 2020 17:48:29 +0100 Subject: [PATCH 12/17] add hcl2template/addrs/doc.go --- hcl2template/addrs/doc.go | 11 +++++++++++ 1 file changed, 11 insertions(+) create mode 100644 hcl2template/addrs/doc.go diff --git a/hcl2template/addrs/doc.go b/hcl2template/addrs/doc.go new file mode 100644 index 000000000..13c300030 --- /dev/null +++ b/hcl2template/addrs/doc.go @@ -0,0 +1,11 @@ +// Package addrs contains types that represent "addresses", which are +// references to specific objects within a Packer configuration. +// +// All addresses have string representations based on HCL traversal syntax +// which should be used in the user-interface, and also in-memory +// representations that can be used internally. +// +// All types within this package should be treated as immutable, even if this +// is not enforced by the Go compiler. It is always an implementation error +// to modify an address object in-place after it is initially constructed. +package addrs From 2987d253355752ccb3c12d9dad1ad13fcb9f4e7c Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Mon, 2 Nov 2020 17:52:19 +0100 Subject: [PATCH 13/17] simplify tests --- hcl2template/common_test.go | 22 ++++------------------ 1 file changed, 4 insertions(+), 18 deletions(-) diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index a5c8b90bd..08f4ea10b 100644 --- a/hcl2template/common_test.go +++ b/hcl2template/common_test.go @@ -74,26 +74,12 @@ func testParse(t *testing.T, tests []parseTest) { } if gotCfg != nil && !tt.parseWantDiagHasErrors { - gotInputVar := gotCfg.InputVariables - for name, value := range tt.parseWantCfg.InputVariables { - if variable, ok := gotInputVar[name]; ok { - if diff := cmp.Diff(variable, value, cmpOpts...); diff != "" { - t.Fatalf("Parser.parse(): unexpected variable values %s: %s", name, diff) - } - } else { - t.Fatalf("Parser.parse() missing input variable. %s", name) - } + if diff := cmp.Diff(tt.parseWantCfg.InputVariables, gotCfg.InputVariables, cmpOpts...); diff != "" { + t.Fatalf("Parser.parse() unexpected input vars. %s", diff) } - gotLocalVar := gotCfg.LocalVariables - for name, value := range tt.parseWantCfg.LocalVariables { - if variable, ok := gotLocalVar[name]; ok { - if diff := cmp.Diff(variable, value, cmpOpts...); diff != "" { - t.Fatalf("Parser.parse(): unexpected variable values %s: %s", name, diff) - } - } else { - t.Fatalf("Parser.parse() missing local variable. %s", name) - } + if diff := cmp.Diff(tt.parseWantCfg.LocalVariables, gotCfg.LocalVariables, cmpOpts...); diff != "" { + t.Fatalf("Parser.parse() unexpected local vars. %s", diff) } } From 10eb32d29eaaca118cbf23cad155724b62b623dc Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Nov 2020 13:13:45 +0100 Subject: [PATCH 14/17] require less English --- hcl2template/types.variables.go | 37 +++++++++++++++------------------ 1 file changed, 17 insertions(+), 20 deletions(-) diff --git a/hcl2template/types.variables.go b/hcl2template/types.variables.go index 2f6303741..6e09893ef 100644 --- a/hcl2template/types.variables.go +++ b/hcl2template/types.variables.go @@ -371,10 +371,10 @@ type VariableValidation struct { // the block defining the validation rule, not an error in the caller. Condition hcl.Expression - // ErrorMessage is one or more full sentences, which would need to be in - // English for consistency with the rest of the error message output but - // can in practice be in any language as long as it ends with a period. - // The message should describe what is required for the condition to return + // ErrorMessage is one or more full sentences, which _should_ be in English + // for consistency with the rest of the error message output but can in + // practice be in any language as long as it ends with a period. The + // message should describe what is required for the condition to return // true in a way that would make sense to a caller of the module. ErrorMessage string @@ -442,20 +442,19 @@ func decodeVariableValidationBlock(varName string, block *hcl.Block) (*VariableV }) case !looksLikeSentences(vv.ErrorMessage): // Because we're going to include this string verbatim as part - // of a bigger error message written in our usual style in - // English, we'll require the given error message to conform - // to that. We might relax this in future if e.g. we start - // presenting these error messages in a different way, or if - // Packer starts supporting producing error messages in - // other human languages, etc. - // For pragmatism we also allow sentences ending with - // exclamation points, but we don't mention it explicitly here - // because that's not really consistent with the Packer UI - // writing style. + // of a bigger error message written in our usual style, we'll + // require the given error message to conform to that. We might + // relax this in future if e.g. we start presenting these error + // messages in a different way, or if Packer starts supporting + // producing error messages in other human languages, etc. For + // pragmatism we also allow sentences ending with exclamation + // points, but we don't mention it explicitly here because + // that's not really consistent with the Packer UI writing + // style. diags = diags.Append(&hcl.Diagnostic{ Severity: hcl.DiagError, Summary: errSummary, - Detail: "Validation error message must be at least one full English sentence starting with an uppercase letter and ending with a period or question mark.", + Detail: "Validation error message must be at least one full sentence starting with an uppercase letter ( if the alphabet permits it ) and ending with a period or question mark.", Subject: attr.Expr.Range().Ptr(), }) } @@ -481,11 +480,9 @@ func looksLikeSentences(s string) bool { first := runes[0] last := runes[len(runes)-1] - // If the first rune is a letter then it must be an uppercase letter. - // (This will only see the first rune in a multi-rune combining sequence, - // but the first rune is generally the letter if any are, and if not then - // we'll just ignore it because we're primarily expecting English messages - // right now anyway, for consistency with all of Packers's other output.) + // If the first rune is a letter then it must be an uppercase letter. To + // sorts of nudge people into writting sentences. For alphabets that don't + // have the notion of 'upper', this does nothing. if unicode.IsLetter(first) && !unicode.IsUpper(first) { return false } From fd873b8811f7d767c83a3cdce747a4da62ae6923 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Nov 2020 13:20:05 +0100 Subject: [PATCH 15/17] Referenceable: explain a bit more of the whys --- hcl2template/addrs/referenceable.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/hcl2template/addrs/referenceable.go b/hcl2template/addrs/referenceable.go index 8c4925c40..8caec1241 100644 --- a/hcl2template/addrs/referenceable.go +++ b/hcl2template/addrs/referenceable.go @@ -3,6 +3,8 @@ package addrs // Referenceable is an interface implemented by all address types that can // appear as references in configuration language expressions. type Referenceable interface { + // referenceableSigil is private to ensure that all Referenceables are + // implentented in this current package. For now this does nothing. referenceableSigil() // String produces a string representation of the address that could be @@ -11,6 +13,8 @@ type Referenceable interface { String() string } +// referenceable is an empty struct that implements Referenceable, add it to +// your Referenceable struct so that it can be recognized as such. type referenceable struct { } From 73caad492cb8c6e89d3355766a46c13b14893a86 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Nov 2020 13:31:45 +0100 Subject: [PATCH 16/17] Update custom-validation.mdx add complex example --- .../from-1.5/variables/custom-validation.mdx | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/website/pages/partials/from-1.5/variables/custom-validation.mdx b/website/pages/partials/from-1.5/variables/custom-validation.mdx index 5a9757a0f..0da062710 100644 --- a/website/pages/partials/from-1.5/variables/custom-validation.mdx +++ b/website/pages/partials/from-1.5/variables/custom-validation.mdx @@ -40,4 +40,29 @@ variable "image_id" { If `condition` evaluates to `false`, an error message including the sentences given in `error_message` will be produced. The error message string should be at least one full sentence explaining the constraint that failed, using a -sentence structure similar to the above examples. \ No newline at end of file +sentence structure similar to the above examples. + +Validation also works with more complex cases: + +```hcl +variable "image_metadata" { + + default = { + key: "value", + something: { + foo: "bar", + } + } + + validation { + condition = length(var.image_metadata.key) > 4 + error_message = "The image_metadata.key field must be more than 4 runes." + } + + validation { + condition = substr(var.image_metadata.something.foo, 0, 3) == "bar" + error_message = "The image_metadata.something.foo field must start with \"bar\"." + } + +} +``` From addd2da101d13107d01b5071d32317f390ac17b0 Mon Sep 17 00:00:00 2001 From: Adrien Delorme Date: Wed, 4 Nov 2020 15:44:21 +0100 Subject: [PATCH 17/17] add can examples --- .../partials/from-1.5/variables/custom-validation.mdx | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/website/pages/partials/from-1.5/variables/custom-validation.mdx b/website/pages/partials/from-1.5/variables/custom-validation.mdx index 0da062710..42af7e1d7 100644 --- a/website/pages/partials/from-1.5/variables/custom-validation.mdx +++ b/website/pages/partials/from-1.5/variables/custom-validation.mdx @@ -22,7 +22,7 @@ The expression can refer only to the variable that the condition applies to, and _must not_ produce errors. If the failure of an expression is the basis of the validation decision, use -[the `can` function](./functions/can.html) to detect such errors. For example: +[the `can` function](/docs/from-1.5/functions/conversion/can) to detect such errors. For example: ```hcl variable "image_id" { @@ -59,6 +59,11 @@ variable "image_metadata" { error_message = "The image_metadata.key field must be more than 4 runes." } + validation { + condition = can(var.image_metadata.something.foo) + error_message = "The image_metadata.something.foo field must exist." + } + validation { condition = substr(var.image_metadata.something.foo, 0, 3) == "bar" error_message = "The image_metadata.something.foo field must start with \"bar\"."