alextab93/buildx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Use unique environment variables to separate JSON from default parsing

Browse Source 
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 <rrjjvv@yahoo.com>
This commit is contained in:
Roberto Villarreal
2025-05-09 18:36:11 -06:00
parent 1f56984626
commit 956fc0c9eb
2 changed files with 420 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files
bake/hcl_test.go
+371 -62
View File
@@ -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 {
bake/hclparser/hclparser.go
+49 -16
View File
@@ -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
}