From 956fc0c9eb3cf8cb7a39ea0be6f3a815dfcaf3af Mon Sep 17 00:00:00 2001 From: Roberto Villarreal Date: Wed, 7 May 2025 21:16:00 -0600 Subject: [PATCH] Use unique environment variables to separate JSON from default parsing The primary intent is to make JSON parsing explicitly opt-in rather than using heuristics to determine intent. With some exceptions, given bake variable `VAR`, an environment variable `VAR_JSON` must be used to provide JSON content. The value in `VAR_JSON` will be ignored when: * a bake built-in of that same name exists * a user-provided variable of that same name exists * typing (attribute `type`) is not present The first is unlikely to happen as built-ins will likely start with `BUILDX_BAKE_`, an unlikely prefix for end users. The second may be a real scenario, where users have `VAR_JSON` dedicated to accepting a string with JSON content and decoding via an HCL function. This will continue to work as-is, but can be simplified by removing the variable from their bake file (`VAR_JSON`) and applying typing (to `VAR`). Signed-off-by: Roberto Villarreal --- bake/hcl_test.go | 433 ++++++++++++++++++++++++++++++------ bake/hclparser/hclparser.go | 65 ++++-- 2 files changed, 420 insertions(+), 78 deletions(-) diff --git a/bake/hcl_test.go b/bake/hcl_test.go index 698de620d..af6dff1b2 100644 --- a/bake/hcl_test.go +++ b/bake/hcl_test.go @@ -1840,6 +1840,8 @@ func TestDefaultVarTypeWithAttrValuesEnforcement(t *testing.T) { func TestTypedVarOverrides(t *testing.T) { const unsuitableValueType = "Unsuitable value type" + const unsupportedType = "unsupported type" + const failedToParseElement = "failed to parse element" tests := []struct { name string varType string @@ -1860,17 +1862,14 @@ func TestTypedVarOverrides(t *testing.T) { 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", + name: "unquoted 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 + // 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", @@ -1890,13 +1889,6 @@ func TestTypedVarOverrides(t *testing.T) { 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", - }, { name: "proper CSV list of strings", varType: "list(string)", @@ -1904,15 +1896,6 @@ func TestTypedVarOverrides(t *testing.T) { argValue: `join("-", FOO)`, wantValue: "hi-there", }, - // pseudo-documentation that this is a scenario that might be expected to work, - // but will parse as a valid CSV in (usually) an undesirable way - { - name: "pseudo-JSON list of unquoted strings", - varType: "list(string)", - override: `[hi,there]`, - argValue: `join("-", FOO)`, - wantValue: "[hi-there]", - }, { name: "CSV of unquoted strings okay", varType: "list(string)", @@ -1920,13 +1903,6 @@ func TestTypedVarOverrides(t *testing.T) { argValue: `join("-", FOO)`, wantValue: "hi-there", }, - { - 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: "CSV list of numbers", varType: "list(number)", @@ -1935,11 +1911,12 @@ func TestTypedVarOverrides(t *testing.T) { 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: "CSV set of numbers", + varType: "set(number)", + override: "3,1,4", + // anecdotally sets are sorted but may not be guaranteed + argValue: `join("-", [for v in sort(FOO): v + 1])`, + wantValue: "2-4-5", }, { name: "CSV map of numbers", @@ -1948,22 +1925,6 @@ func TestTypedVarOverrides(t *testing.T) { argValue: `join("-", sort(values(FOO)))`, wantValue: "1-2", }, - // though a JSON payload, any failure (types in this case) defers to - // CSV parsing and its error; not ideal and could be improved - { - name: "invalid JSON map of numbers", - varType: "map(number)", - override: `{"foo": "oops", "bar": 2}`, - // in lieu of something like ErrorMatches, this is the best single phrase - wantErrorMsg: "as CSV", - }, - { - name: "JSON tuple", - varType: "tuple([number,string])", - override: `[99, "bottles"]`, - argValue: `format("%d %s", FOO[0], FOO[1])`, - wantValue: "99 bottles", - }, { name: "CSV tuple", varType: "tuple([number,string])", @@ -1971,12 +1932,6 @@ func TestTypedVarOverrides(t *testing.T) { argValue: `format("%d %s", FOO[0], FOO[1])`, wantValue: "99 bottles", }, - { - name: "JSON tuple elements with wrong type", - varType: "tuple([number,string])", - override: `[99, 100]`, - wantErrorMsg: unsuitableValueType, - }, { name: "CSV tuple elements with wrong type", varType: "tuple([number,string])", @@ -1984,11 +1939,87 @@ func TestTypedVarOverrides(t *testing.T) { wantErrorMsg: unsuitableValueType, }, { - name: "JSON object", - varType: `object({messages: list(string)})`, - override: `{"messages": ["hi", "there"]}`, - argValue: `join("-", FOO["messages"])`, - wantValue: "hi-there", + name: "invalid CSV value", + varType: "list(string)", + override: `"hello,world`, + wantErrorMsg: "from CSV", + }, + { + name: "object not supported", + varType: "object({message: string})", + override: "does not matter", + wantErrorMsg: unsupportedType, + }, + { + name: "list of non-primitives not supported", + varType: "list(list(number))", + override: "1,2", + wantErrorMsg: unsupportedType, + }, + { + name: "set of non-primitives not supported", + varType: "set(set(number))", + override: "1,2", + wantErrorMsg: unsupportedType, + }, + { + name: "tuple of non-primitives not supported", + varType: "tuple([list(number)])", + // Intentionally a different override than other similar tests; tuple is unique in that + // multiple types are involved and length matters. In the real world, it's probably more + // likely a user would accidentally omit or add an item than trying to use non-primitives, + // so the length check comes first. + override: "1", + wantErrorMsg: unsupportedType, + }, + { + name: "map of non-primitives not supported", + varType: "map(list(number))", + override: "foo:1,2", + wantErrorMsg: unsupportedType, + }, + { + name: "invalid map k/v parsing", + varType: "map(string)", + // TODO fragile; will fail in a different manner without first k/v pair + override: `a:b,foo:"bar`, + wantErrorMsg: "as CSV", + }, + { + name: "list with invalidly parsed elements", + varType: "list(number)", + override: "1,1z", + wantErrorMsg: failedToParseElement, + }, + { + name: "set with invalidly parsed elements", + varType: "set(number)", + override: "1,1z", + wantErrorMsg: failedToParseElement, + }, + { + name: "tuple with invalidly parsed elements", + varType: "tuple([number])", + override: "1z", + wantErrorMsg: failedToParseElement, + }, + { + name: "map with invalidly parsed elements", + varType: "map(number)", + override: "foo:1z", + wantErrorMsg: failedToParseElement, + }, + { + name: "map with bad value format", + varType: "map(number)", + override: "foo:1:1", + wantErrorMsg: "expected one k/v pair", + }, + { + name: "primitive with bad value format", + varType: "number", + override: "1z", + wantErrorMsg: "failed to parse", }, } @@ -2016,13 +2047,291 @@ func TestTypedVarOverrides(t *testing.T) { 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"]) + require.Equal(t, tt.wantValue, *c.Targets[0].Args["foo"]) } } }) } } +func TestTypedVarOverrides_JSON(t *testing.T) { + const unsuitableValueType = "Unsuitable value type" + 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", + }, + // no shortcuts in JSON mode + { + name: "unquoted string is error", + varType: "string", + override: "hello", + wantErrorMsg: "from JSON", + }, + { + name: "string", + 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: "list of strings", + varType: "list(string)", + override: `["hi","there"]`, + argValue: `join("-", FOO)`, + wantValue: "hi-there", + }, + { + name: "list of numbers", + varType: "list(number)", + override: "[3, 1, 4]", + argValue: `join("-", [for v in FOO: v + 1])`, + wantValue: "4-2-5", + }, + { + name: "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}`, + // in lieu of something like ErrorMatches, this is the best single phrase + wantErrorMsg: "from JSON", + }, + { + name: "tuple", + varType: "tuple([number,string])", + override: `[99, "bottles"]`, + argValue: `format("%d %s", FOO[0], FOO[1])`, + wantValue: "99 bottles", + }, + { + name: "tuple elements with wrong type", + varType: "tuple([number,string])", + override: `[99, 100]`, + wantErrorMsg: unsuitableValueType, + }, + { + 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_JSON", 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, tt.wantValue, *c.Targets[0].Args["foo"]) + } + } + }) + } +} + +func TestJSONOverridePriority(t *testing.T) { + t.Run("JSON override ignored when same user var exists", func(t *testing.T) { + dt := []byte(` + variable "FOO" { + type = list(number) + } + variable "FOO_JSON" { + type = list(number) + } + + target "default" { + args = { + foo = FOO + } + }`) + // env FOO_JSON is the CSV override of var FOO_JSON, not a JSON override of FOO + t.Setenv("FOO", "[1,2]") + t.Setenv("FOO_JSON", "[3,4]") + _, err := ParseFile(dt, "docker-bake.hcl") + require.ErrorContains(t, err, "failed to convert") + require.ErrorContains(t, err, "from CSV") + }) + + t.Run("JSON override ignored when same builtin var exists", func(t *testing.T) { + dt := []byte(` + variable "FOO" { + type = list(number) + } + + target "default" { + args = { + foo = length(FOO) + } + }`) + t.Setenv("FOO", "1,2") + t.Setenv("FOO_JSON", "[3,4,5]") + c, _, err := ParseFiles( + []File{{Name: "docker-bake.hcl", Data: dt}}, + map[string]string{"FOO_JSON": "whatever"}, + ) + require.NoError(t, err) + require.Equal(t, 1, len(c.Targets)) + require.Equal(t, "2", *c.Targets[0].Args["foo"]) + }) + + // this is implied/exercised in other tests, but repeated for completeness + t.Run("JSON override ignored if var is untyped", func(t *testing.T) { + dt := []byte(` + variable "FOO" { + default = [1, 2] + } + + target "default" { + args = { + foo = length(FOO) + } + }`) + t.Setenv("FOO_JSON", "[3,4]") + _, err := ParseFile(dt, "docker-bake.hcl") + require.ErrorContains(t, err, "unsupported type") + }) + + t.Run("override-ish variable has regular CSV override", func(t *testing.T) { + dt := []byte(` + variable "FOO_JSON" { + type = list(number) + } + + target "default" { + args = { + foo = length(FOO_JSON) + } + }`) + // despite the name, it's still CSV + t.Setenv("FOO_JSON", "10,11,12") + c, err := ParseFile(dt, "docker-bake.hcl") + require.NoError(t, err) + require.Equal(t, 1, len(c.Targets)) + require.Equal(t, "3", *c.Targets[0].Args["foo"]) + + t.Setenv("FOO_JSON", "[10,11,12]") + _, err = ParseFile(dt, "docker-bake.hcl") + require.ErrorContains(t, err, "from CSV") + }) + + t.Run("override-ish variable has own JSON override", func(t *testing.T) { + dt := []byte(` + variable "FOO_JSON" { + type = list(number) + } + + target "default" { + args = { + foo = length(FOO_JSON) + } + }`) + t.Setenv("FOO_JSON_JSON", "[4,5,6]") + c, err := ParseFile(dt, "docker-bake.hcl") + require.NoError(t, err) + require.Equal(t, 1, len(c.Targets)) + require.Equal(t, "3", *c.Targets[0].Args["foo"]) + }) + + t.Run("JSON override trumps CSV when no var name conflict", func(t *testing.T) { + dt := []byte(` + variable "FOO" { + type = list(number) + } + + target "default" { + args = { + foo = length(FOO) + } + }`) + t.Setenv("FOO", "1,2") + t.Setenv("FOO_JSON", "[3,4,5]") + c, err := ParseFile(dt, "docker-bake.hcl") + require.NoError(t, err) + require.Equal(t, 1, len(c.Targets)) + require.Equal(t, "3", *c.Targets[0].Args["foo"]) + }) + + t.Run("JSON override works with lowercase vars", func(t *testing.T) { + dt := []byte(` + variable "foo" { + type = number + } + + target "default" { + args = { + bar = foo + } + }`) + // may seem reasonable, but not supported + t.Setenv("foo_json", "9000") + c, err := ParseFile(dt, "docker-bake.hcl") + require.NoError(t, err) + require.Equal(t, 1, len(c.Targets)) + // a variable with no value has always resulted in an empty string + require.Equal(t, "", *c.Targets[0].Args["bar"]) + + t.Setenv("foo_JSON", "42") + c, err = ParseFile(dt, "docker-bake.hcl") + require.NoError(t, err) + require.Equal(t, 1, len(c.Targets)) + require.Equal(t, "42", *c.Targets[0].Args["bar"]) + }) +} + 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 13caf8e3f..5dbd0bc78 100644 --- a/bake/hclparser/hclparser.go +++ b/bake/hclparser/hclparser.go @@ -22,6 +22,8 @@ import ( ctyjson "github.com/zclconf/go-cty/cty/json" ) +const jsonEnvOverrideSuffix = "_JSON" + type Opt struct { LookupVar func(string) (string, bool) Vars map[string]string @@ -298,7 +300,7 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) { if def == nil { // 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 { + if _, ok, _ := p.valueHasOverride(name, false); !ok { vv := cty.StringVal("") v = &vv return @@ -320,32 +322,37 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) { } } + // Not entirely true... this doesn't differentiate between a user that specified 'any' + // and a user that specified nothing. But the result is the same; both are treated as strings. + typeSpecified := !varType.Equals(cty.DynamicPseudoType) + envv, hasEnv, jsonEnv := p.valueHasOverride(name, typeSpecified) _, isVar := p.vars[name] - if envv, ok := p.opt.LookupVar(name); ok && isVar { + if hasEnv && 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.IsListType(), varType.IsSetType(), varType.IsTupleType(), varType.IsMapType(): // typing explicitly specified - // since CSV is being treated as the officially supported way, throw away (for now) any JSON errors - // in favor of CSV behavior and leave it the user to figure it out if they intended JSON + case typeSpecified && jsonEnv: vv, err = ctyjson.Unmarshal([]byte(envv), varType) if err != nil { - vv, err = valueFromCSV(name, envv, varType) - if err != nil { - return errors.Wrapf(err, "failed to convert variable %s", name) - } + return errors.Wrapf(err, "failed to convert variable %s from JSON", name) } - case !varType.Equals(cty.DynamicPseudoType): // typing was explicitly specified - vv, err = ctyjson.Unmarshal([]byte(envv), varType) + case supportedCSVType(varType): // typing explicitly specified for selected complex types + vv, err = valueFromCSV(name, envv, varType) if err != nil { - return errors.Wrapf(err, "failed to convert %s as required %s", name, varType.FriendlyName()) + return errors.Wrapf(err, "failed to convert variable %s from CSV", name) } + case typeSpecified && varType.IsPrimitiveType(): + vv, err = convertPrimitive(name, envv, varType) + if err != nil { + return err + } + case typeSpecified: + // e.g., an 'object' not provided as JSON (which can't be expressed in the default CSV format) + return errors.Errorf("unsupported type %s for variable %s", varType.FriendlyName(), name) case def == nil: // no default from which to infer typing vv = cty.StringVal(envv) case vv.Type().Equals(cty.DynamicPseudoType): vv = cty.StringVal(envv) - case vv.Type().Equals(cty.Bool), vv.Type().Equals(cty.String), vv.Type().Equals(cty.Number): + case vv.Type().IsPrimitiveType(): vv, err = convertPrimitive(name, envv, vv.Type()) if err != nil { return err @@ -358,6 +365,27 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) { return nil } +// valueHasOverride returns a possible override value if one was specified, and whether it should +// be treated as a JSON value. +// +// A plain/CSV override is the default; this consolidates the logic around how a JSON-specific override +// is specified and when it will be honored when there are naming conflicts or ambiguity. +func (p *parser) valueHasOverride(name string, favorJSON bool) (string, bool, bool) { + jsonEnv := false + envv, hasEnv := p.opt.LookupVar(name) + if !hasEnv || favorJSON { + jsonVarName := name + jsonEnvOverrideSuffix + _, builtin := p.opt.Vars[jsonVarName] + if _, ok := p.vars[jsonVarName]; !ok && !builtin { + if j, ok := p.opt.LookupVar(jsonVarName); ok { + envv = j + hasEnv, jsonEnv = true, true + } + } + } + return envv, hasEnv, jsonEnv +} + // resolveBlock force evaluates a block, storing the result in the parser. If a // target schema is provided, only the attributes and blocks present in the // schema will be evaluated. @@ -982,6 +1010,11 @@ func convertPrimitive(name, value string, target cty.Type) (cty.Value, error) { } } +// supportedCSVType reports whether the given cty.Type might be convertible from a CSV string via valueFromCSV. +func supportedCSVType(t cty.Type) bool { + return t.IsListType() || t.IsSetType() || t.IsTupleType() || t.IsMapType() +} + // valueFromCSV takes CSV value and converts it to cty.Type. // // This currently supports conversion to cty.List and cty.Set. @@ -1060,7 +1093,7 @@ func valueFromCSV(name, value string, target cty.Type) (cty.Value, error) { } v, err := convertPrimitive(name, kvSlice[1], target.ElementType()) if err != nil { - return cty.NilVal, errors.Wrapf(err, "failed to parse value from type %s", target.FriendlyName()) + return cty.NilVal, errors.Wrapf(err, "failed to parse element from type %s", target.FriendlyName()) } m[kvSlice[0]] = v }