From a1520ea1b26a097ccabf59713dd6d11f58b38647 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Thu, 30 Mar 2023 11:50:28 +0100 Subject: [PATCH] bake: additional validation for matrixes This adds the following constraints to the new features: - Explicit renaming with the `name` property is *only* permitted when used with the `matrix` property. - Group does not support either `name` or `matrix` (we may choose to relax this constraint over time). - All generated names must be unique. Signed-off-by: Justin Chadwell --- bake/bake.go | 68 ++++++++++++++++++++- bake/hcl_test.go | 65 +++++++++++--------- bake/hclparser/hclparser.go | 115 +++++++++++++++++------------------- 3 files changed, 157 insertions(+), 91 deletions(-) diff --git a/bake/bake.go b/bake/bake.go index 3760dae4..60e158b7 100644 --- a/bake/bake.go +++ b/bake/bake.go @@ -24,6 +24,7 @@ import ( "github.com/moby/buildkit/session/auth/authprovider" "github.com/pkg/errors" "github.com/zclconf/go-cty/cty" + "github.com/zclconf/go-cty/cty/convert" ) var ( @@ -612,6 +613,11 @@ type Target struct { linked bool } +var _ hclparser.WithEvalContexts = &Target{} +var _ hclparser.WithGetName = &Target{} +var _ hclparser.WithEvalContexts = &Group{} +var _ hclparser.WithGetName = &Group{} + func (t *Target) normalize() { t.Attest = removeDupes(t.Attest) t.Tags = removeDupes(t.Tags) @@ -795,7 +801,20 @@ func (t *Target) AddOverrides(overrides map[string]Override) error { return nil } -func (t *Target) EvalContexts(ectx *hcl.EvalContext, block *hcl.Block, loadDeps func(hcl.Expression) hcl.Diagnostics) ([]*hcl.EvalContext, error) { +func (g *Group) GetEvalContexts(ectx *hcl.EvalContext, block *hcl.Block, loadDeps func(hcl.Expression) hcl.Diagnostics) ([]*hcl.EvalContext, error) { + content, _, err := block.Body.PartialContent(&hcl.BodySchema{ + Attributes: []hcl.AttributeSchema{{Name: "matrix"}}, + }) + if err != nil { + return nil, err + } + if _, ok := content.Attributes["matrix"]; ok { + return nil, errors.Errorf("matrix is not supported for groups") + } + return []*hcl.EvalContext{ectx}, nil +} + +func (t *Target) GetEvalContexts(ectx *hcl.EvalContext, block *hcl.Block, loadDeps func(hcl.Expression) hcl.Diagnostics) ([]*hcl.EvalContext, error) { content, _, err := block.Body.PartialContent(&hcl.BodySchema{ Attributes: []hcl.AttributeSchema{{Name: "matrix"}}, }) @@ -843,6 +862,53 @@ func (t *Target) EvalContexts(ectx *hcl.EvalContext, block *hcl.Block, loadDeps return ectxs, nil } +func (g *Group) GetName(ectx *hcl.EvalContext, block *hcl.Block, loadDeps func(hcl.Expression) hcl.Diagnostics) (string, error) { + content, _, diags := block.Body.PartialContent(&hcl.BodySchema{ + Attributes: []hcl.AttributeSchema{{Name: "name"}, {Name: "matrix"}}, + }) + if diags != nil { + return "", diags + } + + if _, ok := content.Attributes["name"]; ok { + return "", errors.Errorf("name is not supported for groups") + } + if _, ok := content.Attributes["matrix"]; ok { + return "", errors.Errorf("matrix is not supported for groups") + } + return block.Labels[0], nil +} + +func (t *Target) GetName(ectx *hcl.EvalContext, block *hcl.Block, loadDeps func(hcl.Expression) hcl.Diagnostics) (string, error) { + content, _, diags := block.Body.PartialContent(&hcl.BodySchema{ + Attributes: []hcl.AttributeSchema{{Name: "name"}, {Name: "matrix"}}, + }) + if diags != nil { + return "", diags + } + + attr, ok := content.Attributes["name"] + if !ok { + return block.Labels[0], nil + } + if _, ok := content.Attributes["matrix"]; !ok { + return "", errors.Errorf("name requires matrix") + } + if diags := loadDeps(attr.Expr); diags.HasErrors() { + return "", diags + } + value, diags := attr.Expr.Value(ectx) + if diags != nil { + return "", diags + } + + value, err := convert.Convert(value, cty.String) + if err != nil { + return "", err + } + return value.AsString(), nil +} + func TargetsToBuildOpt(m map[string]*Target, inp *Input) (map[string]build.Options, error) { m2 := make(map[string]build.Options, len(m)) for k, v := range m { diff --git a/bake/hcl_test.go b/bake/hcl_test.go index f84172c5..5010984c 100644 --- a/bake/hcl_test.go +++ b/bake/hcl_test.go @@ -660,16 +660,8 @@ func TestHCLRenameTarget(t *testing.T) { } `) - c, err := ParseFile(dt, "docker-bake.hcl") - require.NoError(t, err) - - require.Equal(t, 1, len(c.Targets)) - require.Equal(t, "xyz", c.Targets[0].Name) - require.Equal(t, "foo", *c.Targets[0].Dockerfile) - - require.Equal(t, 1, len(c.Groups)) - require.Equal(t, "abc", c.Groups[0].Name) - require.Equal(t, []string{"xyz"}, c.Groups[0].Targets) + _, err := ParseFile(dt, "docker-bake.hcl") + require.ErrorContains(t, err, "requires matrix") } func TestHCLRenameGroup(t *testing.T) { @@ -678,31 +670,28 @@ func TestHCLRenameGroup(t *testing.T) { name = "bar" targets = ["x", "y"] } - - target "x" { - } - target "y" { - } `) - c, err := ParseFile(dt, "docker-bake.hcl") - require.NoError(t, err) + _, err := ParseFile(dt, "docker-bake.hcl") + require.ErrorContains(t, err, "not supported") - require.Equal(t, 2, len(c.Targets)) - require.Equal(t, "x", c.Targets[0].Name) - require.Equal(t, "y", c.Targets[1].Name) + dt = []byte(` + group "foo" { + matrix = { + name = ["x", "y"] + } + } + `) - require.Equal(t, 2, len(c.Groups)) - require.Equal(t, "bar", c.Groups[0].Name) - require.Equal(t, []string{"x", "y"}, c.Groups[0].Targets) - require.Equal(t, "foo", c.Groups[1].Name) - require.Equal(t, []string{"bar"}, c.Groups[1].Targets) + _, err = ParseFile(dt, "docker-bake.hcl") + require.ErrorContains(t, err, "not supported") } func TestHCLRenameTargetAttrs(t *testing.T) { dt := []byte(` target "abc" { name = "xyz" + matrix = {} dockerfile = "foo" } @@ -726,6 +715,7 @@ func TestHCLRenameTargetAttrs(t *testing.T) { target "abc" { name = "xyz" + matrix = {} dockerfile = "foo" } `) @@ -741,6 +731,7 @@ func TestHCLRenameTargetAttrs(t *testing.T) { dt = []byte(` target "abc" { name = "xyz" + matrix = {} dockerfile = "foo" } @@ -750,7 +741,7 @@ func TestHCLRenameTargetAttrs(t *testing.T) { `) _, err = ParseFile(dt, "docker-bake.hcl") - require.Error(t, err) + require.ErrorContains(t, err, "abc") dt = []byte(` target "def" { @@ -759,23 +750,26 @@ func TestHCLRenameTargetAttrs(t *testing.T) { target "abc" { name = "xyz" + matrix = {} dockerfile = "foo" } `) _, err = ParseFile(dt, "docker-bake.hcl") - require.Error(t, err) + require.ErrorContains(t, err, "abc") } func TestHCLRenameSplit(t *testing.T) { dt := []byte(` target "x" { name = "y" + matrix = {} dockerfile = "foo" } target "x" { name = "z" + matrix = {} dockerfile = "bar" } `) @@ -798,6 +792,7 @@ func TestHCLRenameMultiFile(t *testing.T) { dt := []byte(` target "foo" { name = "bar" + matrix = {} dockerfile = "x" } `) @@ -979,6 +974,20 @@ func TestHCLMatrixMultipleTargets(t *testing.T) { } } +func TestHCLMatrixDuplicateNames(t *testing.T) { + dt := []byte(` + target "default" { + matrix = { + foo = ["a", "b"] + } + name = "c" + } + `) + + _, err := ParseFile(dt, "docker-bake.hcl") + require.Error(t, err) +} + func TestHCLMatrixArgs(t *testing.T) { dt := []byte(` a = 1 @@ -1033,7 +1042,7 @@ func TestHCLMatrixArgsOverride(t *testing.T) { require.Equal(t, ptrstr("33"), c.Targets[2].Args["foo"]) } -func TestHCLMatrixErrors(t *testing.T) { +func TestHCLMatrixBadTypes(t *testing.T) { dt := []byte(` target "default" { matrix = "test" diff --git a/bake/hclparser/hclparser.go b/bake/hclparser/hclparser.go index 04e11630..f29c4a8a 100644 --- a/bake/hclparser/hclparser.go +++ b/bake/hclparser/hclparser.go @@ -54,6 +54,7 @@ type parser struct { blocks map[string]map[string][]*hcl.Block blockValues map[*hcl.Block][]reflect.Value blockEvalCtx map[*hcl.Block][]*hcl.EvalContext + blockNames map[*hcl.Block][]string blockTypes map[string]reflect.Type ectx *hcl.EvalContext @@ -64,6 +65,14 @@ type parser struct { doneB map[uint64]map[string]struct{} } +type WithEvalContexts interface { + GetEvalContexts(base *hcl.EvalContext, block *hcl.Block, loadDeps func(hcl.Expression) hcl.Diagnostics) ([]*hcl.EvalContext, error) +} + +type WithGetName interface { + GetName(ectx *hcl.EvalContext, block *hcl.Block, loadDeps func(hcl.Expression) hcl.Diagnostics) (string, error) +} + var errUndefined = errors.New("undefined") func (p *parser) loadDeps(ectx *hcl.EvalContext, exp hcl.Expression, exclude map[string]struct{}, allowMissing bool) hcl.Diagnostics { @@ -306,6 +315,11 @@ func (p *parser) resolveValue(ectx *hcl.EvalContext, name string) (err error) { // target schema is provided, only the attributes and blocks present in the // schema will be evaluated. func (p *parser) resolveBlock(block *hcl.Block, target *hcl.BodySchema) (err error) { + // prepare the variable map for this type + if _, ok := p.ectx.Variables[block.Type]; !ok { + p.ectx.Variables[block.Type] = cty.MapValEmpty(cty.Map(cty.String)) + } + // prepare the output destination and evaluation context t, ok := p.blockTypes[block.Type] if !ok { @@ -317,11 +331,8 @@ func (p *parser) resolveBlock(block *hcl.Block, target *hcl.BodySchema) (err err outputs = prev ectxs = p.blockEvalCtx[block] } else { - type ectxI interface { - EvalContexts(base *hcl.EvalContext, block *hcl.Block, loadDeps func(hcl.Expression) hcl.Diagnostics) ([]*hcl.EvalContext, error) - } - if v, ok := reflect.New(t).Interface().(ectxI); ok { - ectxs, err = v.EvalContexts(p.ectx, block, func(expr hcl.Expression) hcl.Diagnostics { + if v, ok := reflect.New(t).Interface().(WithEvalContexts); ok { + ectxs, err = v.GetEvalContexts(p.ectx, block, func(expr hcl.Expression) hcl.Diagnostics { return p.loadDeps(p.ectx, expr, nil, true) }) if err != nil { @@ -345,12 +356,9 @@ func (p *parser) resolveBlock(block *hcl.Block, target *hcl.BodySchema) (err err for i, output := range outputs { target := target ectx := ectxs[i] - name, _ := getName(output) - if name == "" { - name = block.Labels[0] - } - if err := p.opt.ValidateLabel(name); err != nil { - return err + name := block.Labels[0] + if names, ok := p.blockNames[block]; ok { + name = names[i] } if _, ok := p.doneB[key(block, ectx)]; !ok { @@ -415,9 +423,6 @@ func (p *parser) resolveBlock(block *hcl.Block, target *hcl.BodySchema) (err err // load dependencies from all targeted properties schema, _ := gohcl.ImpliedBodySchema(reflect.New(t).Interface()) - if nameKey, ok := getNameKey(output); ok { - schema.Attributes = append(schema.Attributes, hcl.AttributeSchema{Name: nameKey}) - } content, _, diag := body().PartialContent(schema) if diag.HasErrors() { return diag @@ -440,22 +445,6 @@ func (p *parser) resolveBlock(block *hcl.Block, target *hcl.BodySchema) (err err if diag.HasErrors() { return diag } - if nameKey, ok := getNameKey(output); ok { - for k, v := range content.Attributes { - if k == nameKey { - var name2 string - diag = gohcl.DecodeExpression(v.Expr, ectx, &name) - if diag.HasErrors() { - return diag - } - if name2 != "" { - name = name2 - } - break - } - } - } - setName(output, name) // mark all targeted properties as done for _, a := range content.Attributes { @@ -499,35 +488,49 @@ func (p *parser) resolveBlock(block *hcl.Block, target *hcl.BodySchema) (err err // resolveBlockNames returns the names of the block, calling resolveBlock to // evaluate any label fields to correctly resolve the name. func (p *parser) resolveBlockNames(block *hcl.Block) ([]string, error) { - t, ok := p.blockTypes[block.Type] - if !ok { - return nil, errors.Errorf("internal error: unknown block type %s", block.Type) + if names, ok := p.blockNames[block]; ok { + return names, nil } - nameKey, ok := getNameKey(reflect.New(t)) - if ok { - target := &hcl.BodySchema{ - Attributes: []hcl.AttributeSchema{{Name: nameKey}}, - Blocks: []hcl.BlockHeaderSchema{{Type: nameKey}}, - } - if err := p.resolveBlock(block, target); err != nil { + if err := p.resolveBlock(block, &hcl.BodySchema{}); err != nil { + return nil, err + } + + names := make([]string, 0, len(p.blockValues[block])) + for i, val := range p.blockValues[block] { + ectx := p.blockEvalCtx[block][i] + + name := block.Labels[0] + if err := p.opt.ValidateLabel(name); err != nil { return nil, err } - } else { - if err := p.resolveBlock(block, &hcl.BodySchema{}); err != nil { - return nil, err + + if v, ok := val.Interface().(WithGetName); ok { + var err error + name, err = v.GetName(ectx, block, func(expr hcl.Expression) hcl.Diagnostics { + return p.loadDeps(ectx, expr, nil, true) + }) + if err != nil { + return nil, err + } + if err := p.opt.ValidateLabel(name); err != nil { + return nil, err + } } + + setName(val, name) + names = append(names, name) } - names := make([]string, 0, len(p.blockValues[block])) - for _, prev := range p.blockValues[block] { - name, ok := getName(prev) - if !ok { - return nil, errors.New("internal error: failed to get name") + found := map[string]struct{}{} + for _, name := range names { + if _, ok := found[name]; ok { + return nil, errors.Errorf("duplicate name %q", name) } - names = append(names, name) + found[name] = struct{}{} } + p.blockNames[block] = names return names, nil } @@ -570,6 +573,7 @@ func Parse(b hcl.Body, opt Opt, val interface{}) (map[string]map[string][]string blocks: map[string]map[string][]*hcl.Block{}, blockValues: map[*hcl.Block][]reflect.Value{}, blockEvalCtx: map[*hcl.Block][]*hcl.EvalContext{}, + blockNames: map[*hcl.Block][]string{}, blockTypes: map[string]reflect.Type{}, ectx: &hcl.EvalContext{ Variables: map[string]cty.Value{}, @@ -837,19 +841,6 @@ func getName(v reflect.Value) (string, bool) { return "", false } -func getNameKey(v reflect.Value) (string, bool) { - numFields := v.Elem().Type().NumField() - for i := 0; i < numFields; i++ { - parts := strings.Split(v.Elem().Type().Field(i).Tag.Get("hcl"), ",") - for _, t := range parts[1:] { - if t == "label" { - return parts[0], true - } - } - } - return "", false -} - func getNameIndex(v reflect.Value) (int, bool) { numFields := v.Elem().Type().NumField() for i := 0; i < numFields; i++ {