diff --git a/bake/hcl_test.go b/bake/hcl_test.go index 5f899544f..120e28aae 100644 --- a/bake/hcl_test.go +++ b/bake/hcl_test.go @@ -1,6 +1,7 @@ package bake import ( + "fmt" "reflect" "regexp" "testing" @@ -1645,6 +1646,328 @@ func TestHCLIndexOfFunc(t *testing.T) { require.Empty(t, c.Targets[1].Tags[1]) } +func TestVarTypingSpec(t *testing.T) { + templ := ` + variable "FOO" { + type = %s + } + target "default" { + }` + + // not exhaustive, but the common ones + for _, s := range []string{ + "bool", "number", "string", "any", + "list(string)", "set(string)", "tuple([string, number])", + } { + dt := fmt.Sprintf(templ, s) + _, err := ParseFile([]byte(dt), "docker-bake.hcl") + require.NoError(t, err) + } + + for _, s := range []string{ + "boolean", // no synonyms/aliases + "BOOL", // case matters + `lower("bool")`, // must be literals + } { + dt := fmt.Sprintf(templ, s) + _, err := ParseFile([]byte(dt), "docker-bake.hcl") + require.ErrorContains(t, err, "not a valid type") + } +} + +func TestDefaultVarTypeEnforcement(t *testing.T) { + // To help prove a given default doesn't just pass the type check, but *is* that type, + // we use argValue to provide an expression that would work only on that type. + tests := []struct { + name string + varType string + varDefault any + argValue string + wantValue string + wantError bool + }{ + { + name: "number (happy)", + varType: "number", + varDefault: 99, + argValue: "FOO + 1", + wantValue: "100", + }, + { + name: "numeric string compatible with number", + varType: "number", + varDefault: `"99"`, + argValue: "FOO + 1", + wantValue: "100", + }, + { + name: "boolean (happy)", + varType: "bool", + varDefault: true, + argValue: "and(FOO, true)", + wantValue: "true", + }, + { + name: "numeric boolean compatible with boolean", + varType: "bool", + varDefault: `"true"`, + argValue: "and(FOO, true)", + wantValue: "true", + }, + // should be representative of flagrant primitive type mismatches; not worth listing all possibilities? + { + name: "non-numeric string default incompatible with number", + varType: "number", + varDefault: `"oops"`, + wantError: true, + }, + { + name: "list of numbers (happy)", + varType: "list(number)", + varDefault: "[2,3]", + argValue: `join("", [for v in FOO: v + 1])`, + wantValue: "34", + }, + { + name: "list of numbers with numeric strings okay", + varType: "list(number)", + varDefault: `["2","3"]`, + argValue: `join("", [for v in FOO: v + 1])`, + wantValue: "34", + }, + // represent flagrant mismatches for list types + { + name: "non-numeric strings in numeric list rejected", + varType: "list(number)", + varDefault: `["oops"]`, + wantError: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + argValue := tt.argValue + if argValue == "" { + argValue = "FOO" + } + dt := fmt.Sprintf(` + variable "FOO" { + type = %s + default = %v + } + + target "default" { + args = { + foo = %s + } + }`, tt.varType, tt.varDefault, argValue) + c, err := ParseFile([]byte(dt), "docker-bake.hcl") + if tt.wantError { + require.ErrorContains(t, err, "invalid type") + } else { + require.NoError(t, err) + if tt.wantValue != "" { + require.Equal(t, 1, len(c.Targets)) + require.Equal(t, ptrstr(tt.wantValue), c.Targets[0].Args["foo"]) + } + } + }) + } +} + +func TestDefaultVarTypeWithAttrValuesEnforcement(t *testing.T) { + tests := []struct { + name string + attrValue any + varType string + wantError bool + }{ + { + name: "attribute literal which matches var type", + attrValue: `"hello"`, + varType: "string", + }, + { + name: "attribute literal which coerces to var type", + attrValue: `"99"`, + varType: "number", + }, + { + name: "mismatch", + attrValue: 99, + varType: "bool", + wantError: true, + }, + { + name: "attribute correctly typed via function", + attrValue: `split(",", "1,2,3")`, + varType: "list(number)", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dt := fmt.Sprintf(` + BAR = %v + variable "FOO" { + type = %s + default = BAR + } + + target "default" { + }`, tt.attrValue, tt.varType) + _, err := ParseFile([]byte(dt), "docker-bake.hcl") + if tt.wantError { + require.ErrorContains(t, err, "invalid type") + require.ErrorContains(t, err, "FOO default value") + } else { + require.NoError(t, err) + } + }) + } +} + +func TestTypedVarOverrides(t *testing.T) { + const convertFailure = "failed to convert FOO" + tests := []struct { + name string + varType string + override string + argValue string + wantValue string + wantErrorMsg string + }{ + { + name: "boolean", + varType: "bool", + override: "true", + wantValue: "true", + }, + { + name: "number", + varType: "number", + override: "99", + wantValue: "99", + }, + // this breaks the rule about needing proper JSON as it would violate + // the principle of least surprise and hinder usability + { + name: "enquoted string accepted", + varType: "string", + override: "hello", + wantValue: "hello", + }, + // similar to above, an environment variable with a quoted string would + // most likely be intended to be a string whose first and last characters + // are quotes + { + name: "quoted string keeps quotes in value", + varType: "string", + override: `"hello"`, + wantValue: `"hello"`, + }, + { + name: "any", + varType: "any", + override: "[1,2]", + wantValue: "[1,2]", + }, + { + name: "any never convert to complex types", + varType: "any", + override: "[1,2]", + argValue: "length(FOO)", + wantErrorMsg: "collection must be a list", + }, + { + name: "proper JSON list of strings", + varType: "list(string)", + override: `["hi","there"]`, + argValue: `join("-", FOO)`, + wantValue: "hi-there", + }, + // not that this *should* be an error, but pseudo-documentation that this is + // a scenario that might be expected to work, but doesn't (yet) for simplicity + { + name: "JSON list of unquoted strings not okay", + varType: "list(string)", + override: `[hi,there]`, + wantErrorMsg: convertFailure, + }, + // ditto above + { + name: "CSV of quoted strings not okay", + varType: "list(string)", + override: `"hi","there"`, + wantErrorMsg: convertFailure, + }, + // ditto above + { + name: "CSV of unquoted strings not okay", + varType: "list(string)", + override: `hi,there`, + wantErrorMsg: convertFailure, + }, + { + name: "JSON list of numbers", + varType: "list(number)", + override: "[3, 1, 4]", + argValue: `join("-", [for v in FOO: v + 1])`, + wantValue: "4-2-5", + }, + { + name: "JSON map of numbers", + varType: "map(number)", + override: `{"foo": 1, "bar": 2}`, + argValue: `join("-", sort(values(FOO)))`, + wantValue: "1-2", + }, + { + name: "invalid JSON map of numbers", + varType: "map(number)", + override: `{"foo": "oops", "bar": 2}`, + wantErrorMsg: convertFailure, + }, + { + name: "JSON object", + varType: `object({messages: list(string)})`, + override: `{"messages": ["hi", "there"]}`, + argValue: `join("-", FOO["messages"])`, + wantValue: "hi-there", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + argValue := tt.argValue + if argValue == "" { + argValue = "FOO" + } + dt := fmt.Sprintf(` + variable "FOO" { + type = %s + } + + target "default" { + args = { + foo = %s + } + }`, tt.varType, argValue) + t.Setenv("FOO", tt.override) + c, err := ParseFile([]byte(dt), "docker-bake.hcl") + if tt.wantErrorMsg != "" { + require.ErrorContains(t, err, tt.wantErrorMsg) + } else { + require.NoError(t, err) + if tt.wantValue != "" { + require.Equal(t, 1, len(c.Targets)) + require.Equal(t, ptrstr(tt.wantValue), c.Targets[0].Args["foo"]) + } + } + }) + } +} + func ptrstr(s any) *string { var n *string if reflect.ValueOf(s).Kind() == reflect.String { diff --git a/bake/hclparser/hclparser.go b/bake/hclparser/hclparser.go index 1628e7c0d..2ded19216 100644 --- a/bake/hclparser/hclparser.go +++ b/bake/hclparser/hclparser.go @@ -14,9 +14,11 @@ import ( "github.com/docker/buildx/bake/hclparser/gohcl" "github.com/docker/buildx/util/userfunc" "github.com/hashicorp/hcl/v2" + "github.com/hashicorp/hcl/v2/ext/typeexpr" "github.com/pkg/errors" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" + ctyjson "github.com/zclconf/go-cty/cty/json" ) type Opt struct { @@ -27,6 +29,7 @@ type Opt struct { type variable struct { Name string `json:"-" hcl:"name,label"` + Type hcl.Expression `json:"type,omitempty" hcl:"type,optional"` Default *hcl.Attribute `json:"default,omitempty" hcl:"default,optional"` Description string `json:"description,omitempty" hcl:"description,optional"` Validations []*variableValidation `json:"validation,omitempty" hcl:"validation,block"` @@ -267,38 +270,68 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) { } }() + // built-in vars aren't intended to be overridden and are statically typed as strings; + // no sense sending them through type checks or waiting to return them + if val, ok := p.opt.Vars[name]; ok { + vv := cty.StringVal(val) + v = &vv + return + } + + var diags hcl.Diagnostics + varType := cty.DynamicPseudoType def, ok := p.attrs[name] - if _, builtin := p.opt.Vars[name]; !ok && !builtin { + if !ok { vr, ok := p.vars[name] if !ok { return errors.Wrapf(errUndefined{}, "variable %q does not exist", name) } def = vr.Default ectx = p.ectx + varType, diags = typeConstraint(vr.Type) + if diags.HasErrors() { + return diags + } } if def == nil { - val, ok := p.opt.Vars[name] - if !ok { - val, _ = p.opt.LookupVar(name) + // lack of specified value is considered to have an empty string value, + // but any overrides get type checked + if _, ok := p.opt.LookupVar(name); !ok { + vv := cty.StringVal("") + v = &vv + return } - vv := cty.StringVal(val) - v = &vv - return } - if diags := p.loadDeps(ectx, def.Expr, nil, true); diags.HasErrors() { - return diags - } - vv, diags := def.Expr.Value(ectx) - if diags.HasErrors() { - return diags + var vv cty.Value + if def != nil { + if diags := p.loadDeps(ectx, def.Expr, nil, true); diags.HasErrors() { + return diags + } + vv, diags = def.Expr.Value(ectx) + if diags.HasErrors() { + return diags + } + vv, err = convert.Convert(vv, varType) + if err != nil { + return errors.Wrapf(err, "invalid type %s for variable %s default value", varType.FriendlyName(), name) + } } _, isVar := p.vars[name] if envv, ok := p.opt.LookupVar(name); ok && isVar { switch { + case varType.Equals(cty.String): // don't parse as JSON; users don't expect to have to quote strings + vv = cty.StringVal(envv) + case !varType.Equals(cty.DynamicPseudoType): // typing was explicitly specified + vv, err = ctyjson.Unmarshal([]byte(envv), varType) + if err != nil { + return errors.Wrapf(err, "failed to convert %s as required %s", name, varType.FriendlyName()) + } + case def == nil: // no default from which to infer typing + vv = cty.StringVal(envv) case vv.Type().Equals(cty.Bool): b, err := strconv.ParseBool(envv) if err != nil { @@ -317,7 +350,6 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) { } vv = cty.NumberVal(big.NewFloat(n)) default: - // TODO: support lists with csv values return errors.Errorf("unsupported type %s for variable %s", vv.Type().FriendlyName(), name) } } @@ -907,6 +939,23 @@ func Parse(b hcl.Body, opt Opt, val any) (*ParseMeta, hcl.Diagnostics) { }, nil } +// typeConstraint wraps typeexpr.TypeConstraint to differentiate between errors in the +// specification and errors due to being cty.NullVal (not provided). +func typeConstraint(expr hcl.Expression) (cty.Type, hcl.Diagnostics) { + t, diag := typeexpr.TypeConstraint(expr) + if !diag.HasErrors() { + return t, diag + } + // if had errors, it could be because the expression is 'nil', i.e., unspecified + if v, err := expr.Value(nil); err == nil { + if v.IsNull() { + return cty.DynamicPseudoType, nil + } + } + // even if the evaluation resulted in error, the original (error) diagnostics are likely more useful + return t, diag +} + // wrapErrorDiagnostic wraps an error into a hcl.Diagnostics object. // If the error is already an hcl.Diagnostics object, it is returned as is. func wrapErrorDiagnostic(message string, err error, subject *hcl.Range, context *hcl.Range) hcl.Diagnostics {