From 621ab1c0af05cdef816039c6b7055a70707e5015 Mon Sep 17 00:00:00 2001 From: Justin Chadwell Date: Thu, 10 Nov 2022 12:19:58 +0000 Subject: [PATCH] cache: error on duplicate sets of cache options Previously, we attempted to gracefully resolve duplicate cache export options, however, we should explicitly error - a client has made an error if the same options appear twice. If we really want de-duplication, then clients such as buildx should implement it there. Signed-off-by: Justin Chadwell --- control/control.go | 59 ++++++++++++++++--- .../util_test.go => control/control_test.go | 55 ++++++----------- solver/llbsolver/util/util.go | 44 -------------- 3 files changed, 71 insertions(+), 87 deletions(-) rename solver/llbsolver/util/util_test.go => control/control_test.go (58%) delete mode 100644 solver/llbsolver/util/util.go diff --git a/control/control.go b/control/control.go index 718dfd282f74..a2edd6de0a66 100644 --- a/control/control.go +++ b/control/control.go @@ -2,11 +2,13 @@ package control import ( "context" + "fmt" "sync" "sync/atomic" "time" "github.com/docker/distribution/reference" + "github.com/mitchellh/hashstructure/v2" controlapi "github.com/moby/buildkit/api/services/control" apitypes "github.com/moby/buildkit/api/types" "github.com/moby/buildkit/cache/remotecache" @@ -21,7 +23,6 @@ import ( "github.com/moby/buildkit/solver" "github.com/moby/buildkit/solver/llbsolver" "github.com/moby/buildkit/solver/llbsolver/proc" - solverutil "github.com/moby/buildkit/solver/llbsolver/util" "github.com/moby/buildkit/solver/pb" "github.com/moby/buildkit/util/bklog" "github.com/moby/buildkit/util/imageutil" @@ -293,14 +294,17 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (* } } - var cacheImports []frontend.CacheOptionsEntry - - var cacheExporters []llbsolver.RemoteCacheExporter - exportCacheOptEntries, err := solverutil.DedupCacheOptions(req.Cache.Exports) - if err != nil { + if c, err := findDuplicateCacheOptions(req.Cache.Exports); err != nil { return nil, err + } else if c != nil { + types := []string{} + for _, c := range c { + types = append(types, c.Type) + } + return nil, errors.Errorf("duplicate cache exports %s", types) } - for _, e := range exportCacheOptEntries { + var cacheExporters []llbsolver.RemoteCacheExporter + for _, e := range req.Cache.Exports { cacheExporterFunc, ok := c.opt.ResolveCacheExporterFuncs[e.Type] if !ok { return nil, errors.Errorf("unknown cache exporter: %q", e.Type) @@ -317,6 +321,8 @@ func (c *Controller) Solve(ctx context.Context, req *controlapi.SolveRequest) (* } cacheExporters = append(cacheExporters, exp) } + + var cacheImports []frontend.CacheOptionsEntry for _, im := range req.Cache.Imports { cacheImports = append(cacheImports, frontend.CacheOptionsEntry{ Type: im.Type, @@ -562,3 +568,42 @@ func toPBBuildkitVersion(in client.BuildkitVersion) *apitypes.BuildkitVersion { Revision: in.Revision, } } + +func findDuplicateCacheOptions(cacheOpts []*controlapi.CacheOptionsEntry) ([]*controlapi.CacheOptionsEntry, error) { + seen := map[string]*controlapi.CacheOptionsEntry{} + duplicate := map[string]struct{}{} + for _, opt := range cacheOpts { + k, err := cacheOptKey(*opt) + if err != nil { + return nil, err + } + if _, ok := seen[k]; ok { + duplicate[k] = struct{}{} + } + seen[k] = opt + } + + var duplicates []*controlapi.CacheOptionsEntry + for k := range duplicate { + duplicates = append(duplicates, seen[k]) + } + return duplicates, nil +} + +func cacheOptKey(opt controlapi.CacheOptionsEntry) (string, error) { + if opt.Type == "registry" && opt.Attrs["ref"] != "" { + return opt.Attrs["ref"], nil + } + var rawOpt = struct { + Type string + Attrs map[string]string + }{ + Type: opt.Type, + Attrs: opt.Attrs, + } + hash, err := hashstructure.Hash(rawOpt, hashstructure.FormatV2, nil) + if err != nil { + return "", err + } + return fmt.Sprint(opt.Type, ":", hash), nil +} diff --git a/solver/llbsolver/util/util_test.go b/control/control_test.go similarity index 58% rename from solver/llbsolver/util/util_test.go rename to control/control_test.go index 42f836b0bcfa..9955ca549ace 100644 --- a/solver/llbsolver/util/util_test.go +++ b/control/control_test.go @@ -1,4 +1,4 @@ -package util +package control import ( "testing" @@ -7,21 +7,15 @@ import ( "github.com/stretchr/testify/require" ) -func TestDedupCacheOptions(t *testing.T) { +func TestDuplicateCacheOptions(t *testing.T) { var testCases = []struct { name string opts []*controlapi.CacheOptionsEntry - expected []*controlapi.CacheOptionsEntry + expected []uint }{ { - name: "deduplicates opts", + name: "avoids unique opts", opts: []*controlapi.CacheOptionsEntry{ - { - Type: "registry", - Attrs: map[string]string{ - "ref": "example.com/ref:v1.0.0", - }, - }, { Type: "registry", Attrs: map[string]string{ @@ -34,30 +28,18 @@ func TestDedupCacheOptions(t *testing.T) { "dest": "/path/for/export", }, }, + }, + expected: nil, + }, + { + name: "finds duplicate opts", + opts: []*controlapi.CacheOptionsEntry{ { - Type: "local", - Attrs: map[string]string{ - "dest": "/path/for/export", - }, - }, - { - Type: "azblob", - Attrs: map[string]string{ - "account-url": "url", - "blobs_prefix": "prefix", - "name": "name", - }, - }, - { - Type: "azblob", + Type: "registry", Attrs: map[string]string{ - "account-url": "url", - "blobs_prefix": "prefix", - "name": "name", + "ref": "example.com/ref:v1.0.0", }, }, - }, - expected: []*controlapi.CacheOptionsEntry{ { Type: "registry", Attrs: map[string]string{ @@ -71,22 +53,23 @@ func TestDedupCacheOptions(t *testing.T) { }, }, { - Type: "azblob", + Type: "local", Attrs: map[string]string{ - "account-url": "url", - "blobs_prefix": "prefix", - "name": "name", + "dest": "/path/for/export", }, }, }, + expected: []uint{0, 2}, }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - opts, err := DedupCacheOptions(tc.opts) + p, err := findDuplicateCacheOptions(tc.opts) require.NoError(t, err) - require.ElementsMatch(t, opts, tc.expected) + for i, j := range tc.expected { + require.Equal(t, p[i], tc.opts[j]) + } }) } } diff --git a/solver/llbsolver/util/util.go b/solver/llbsolver/util/util.go deleted file mode 100644 index 7529538475c9..000000000000 --- a/solver/llbsolver/util/util.go +++ /dev/null @@ -1,44 +0,0 @@ -package util - -import ( - "fmt" - - "github.com/mitchellh/hashstructure/v2" - - controlapi "github.com/moby/buildkit/api/services/control" -) - -// DedupCacheOptions removes duplicates from cacheOpts -func DedupCacheOptions(cacheOpts []*controlapi.CacheOptionsEntry) (result []*controlapi.CacheOptionsEntry, err error) { - seen := map[string]*controlapi.CacheOptionsEntry{} - for _, opt := range cacheOpts { - key, err := cacheOptKey(*opt) - if err != nil { - return nil, err - } - seen[key] = opt - } - result = make([]*controlapi.CacheOptionsEntry, 0, len(seen)) - for _, opt := range seen { - result = append(result, opt) - } - return result, nil -} - -func cacheOptKey(opt controlapi.CacheOptionsEntry) (string, error) { - if opt.Type == "registry" && opt.Attrs["ref"] != "" { - return opt.Attrs["ref"], nil - } - var rawOpt = struct { - Type string - Attrs map[string]string - }{ - Type: opt.Type, - Attrs: opt.Attrs, - } - hash, err := hashstructure.Hash(rawOpt, hashstructure.FormatV2, nil) - if err != nil { - return "", err - } - return fmt.Sprint(opt.Type, ":", hash), nil -}