From dd5e25553766e48303c5e4c4c04210920411629a Mon Sep 17 00:00:00 2001 From: Jacob MacElroy Date: Wed, 27 Oct 2021 16:32:36 -0600 Subject: [PATCH 1/5] Handling parsing of multiple scopes combined in a single string. It is possible for challenge headers to contain multiple scopes in a single string. This change ensures that this case is handled when parsing the scopes by splitting out scopes combined in a single string. Signed-off-by: Jacob MacElroy (cherry picked from commit 5279e683a54a7f0ae7352dd8ab5c28806d68a8eb) --- util/resolver/authorizer.go | 39 +++++++++++++----------- util/resolver/authorizer_test.go | 51 ++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 18 deletions(-) create mode 100644 util/resolver/authorizer_test.go diff --git a/util/resolver/authorizer.go b/util/resolver/authorizer.go index 2766f2157e07..66f410a6e687 100644 --- a/util/resolver/authorizer.go +++ b/util/resolver/authorizer.go @@ -434,25 +434,28 @@ type scopes map[string]map[string]struct{} func parseScopes(s []string) scopes { // https://docs.docker.com/registry/spec/auth/scope/ m := map[string]map[string]struct{}{} - for _, scope := range s { - parts := strings.SplitN(scope, ":", 3) - names := []string{parts[0]} - if len(parts) > 1 { - names = append(names, parts[1]) - } - var actions []string - if len(parts) == 3 { - actions = append(actions, strings.Split(parts[2], ",")...) - } - name := strings.Join(names, ":") - ma, ok := m[name] - if !ok { - ma = map[string]struct{}{} - m[name] = ma - } + for _, scopeStr := range s { + // The scopeStr may have strings that contain multiple scopes separated by a space. + for _, scope := range strings.Split(scopeStr, " ") { + parts := strings.SplitN(scope, ":", 3) + names := []string{parts[0]} + if len(parts) > 1 { + names = append(names, parts[1]) + } + var actions []string + if len(parts) == 3 { + actions = append(actions, strings.Split(parts[2], ",")...) + } + name := strings.Join(names, ":") + ma, ok := m[name] + if !ok { + ma = map[string]struct{}{} + m[name] = ma + } - for _, a := range actions { - ma[a] = struct{}{} + for _, a := range actions { + ma[a] = struct{}{} + } } } return m diff --git a/util/resolver/authorizer_test.go b/util/resolver/authorizer_test.go new file mode 100644 index 000000000000..7c162a8490cb --- /dev/null +++ b/util/resolver/authorizer_test.go @@ -0,0 +1,51 @@ +package resolver + +import ( + "reflect" + "testing" +) + +func TestParseScopes(t *testing.T) { + for _, tc := range []struct { + name string + input []string + expected scopes + }{ + { + name: "SeparateStrings", + input: []string{ + "repository:foo/bar:pull", + "repository:foo/baz:pull,push", + }, + expected: map[string]map[string]struct{}{ + "repository:foo/bar": { + "pull": struct{}{}, + }, + "repository:foo/baz": { + "pull": struct{}{}, + "push": struct{}{}, + }, + }, + }, + { + name: "CombinedStrings", + input: []string{"repository:foo/bar:pull repository:foo/baz:pull,push"}, + expected: map[string]map[string]struct{}{ + "repository:foo/bar": { + "pull": struct{}{}, + }, + "repository:foo/baz": { + "pull": struct{}{}, + "push": struct{}{}, + }, + }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + parsed := parseScopes(tc.input) + if !reflect.DeepEqual(parsed, tc.expected) { + t.Fatalf("expected %v, got %v", tc.expected, parsed) + } + }) + } +} From 34abeb35aa5691bdbfd1d6a2d44387520f127715 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 6 Oct 2021 22:46:37 -0700 Subject: [PATCH 2/5] limited: fix possible deadlock when pushhandler calls fetcher Signed-off-by: Tonis Tiigi (cherry picked from commit 7153f5a9bbb796b5c7f32b4c3d431c3f3789a02f) --- util/resolver/limited/group.go | 84 ++++++++++------------------------ 1 file changed, 25 insertions(+), 59 deletions(-) diff --git a/util/resolver/limited/group.go b/util/resolver/limited/group.go index 5789dbdcdfb2..31e8f5e0f8f9 100644 --- a/util/resolver/limited/group.go +++ b/util/resolver/limited/group.go @@ -11,12 +11,15 @@ import ( "github.com/containerd/containerd/images" "github.com/containerd/containerd/remotes" "github.com/docker/distribution/reference" - "github.com/opencontainers/go-digest" ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" "golang.org/x/sync/semaphore" ) +type contextKeyT string + +var contextKey = contextKeyT("buildkit/util/resolver/limited") + var Default = New(4) type Group struct { @@ -30,7 +33,13 @@ type req struct { ref string } -func (r *req) acquire(ctx context.Context, desc ocispec.Descriptor) (func(), error) { +func (r *req) acquire(ctx context.Context, desc ocispec.Descriptor) (context.Context, func(), error) { + if v := ctx.Value(contextKey); v != nil { + return ctx, func() {}, nil + } + + ctx = context.WithValue(ctx, contextKey, struct{}{}) + // json request get one additional connection highPriority := strings.HasSuffix(desc.MediaType, "+json") @@ -46,16 +55,16 @@ func (r *req) acquire(ctx context.Context, desc ocispec.Descriptor) (func(), err r.g.mu.Unlock() if !highPriority { if err := s[0].Acquire(ctx, 1); err != nil { - return nil, err + return ctx, nil, err } } if err := s[1].Acquire(ctx, 1); err != nil { if !highPriority { s[0].Release(1) } - return nil, err + return ctx, nil, err } - return func() { + return ctx, func() { s[1].Release(1) if !highPriority { s[0].Release(1) @@ -78,60 +87,17 @@ func (g *Group) WrapFetcher(f remotes.Fetcher, ref string) remotes.Fetcher { return &fetcher{Fetcher: f, req: g.req(ref)} } -func (g *Group) WrapPusher(p remotes.Pusher, ref string) remotes.Pusher { - return &pusher{Pusher: p, req: g.req(ref)} -} - -type pusher struct { - remotes.Pusher - req *req -} - -func (p *pusher) Push(ctx context.Context, desc ocispec.Descriptor) (content.Writer, error) { - release, err := p.req.acquire(ctx, desc) - if err != nil { - return nil, err - } - w, err := p.Pusher.Push(ctx, desc) - if err != nil { - release() - return nil, err - } - ww := &writer{Writer: w} - closer := func() { - if !ww.closed { - logrus.Warnf("writer not closed cleanly: %s", desc.Digest) +func (g *Group) PushHandler(pusher remotes.Pusher, provider content.Provider, ref string) images.HandlerFunc { + ph := remotes.PushHandler(pusher, provider) + req := g.req(ref) + return func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) { + ctx, release, err := req.acquire(ctx, desc) + if err != nil { + return nil, err } - release() + defer release() + return ph(ctx, desc) } - ww.release = closer - runtime.SetFinalizer(ww, func(rc *writer) { - rc.close() - }) - return ww, nil -} - -type writer struct { - content.Writer - once sync.Once - release func() - closed bool -} - -func (w *writer) Close() error { - w.closed = true - w.close() - return w.Writer.Close() -} - -func (w *writer) Commit(ctx context.Context, size int64, expected digest.Digest, opts ...content.Opt) error { - w.closed = true - w.close() - return w.Writer.Commit(ctx, size, expected, opts...) -} - -func (w *writer) close() { - w.once.Do(w.release) } type fetcher struct { @@ -140,7 +106,7 @@ type fetcher struct { } func (f *fetcher) Fetch(ctx context.Context, desc ocispec.Descriptor) (io.ReadCloser, error) { - release, err := f.req.acquire(ctx, desc) + ctx, release, err := f.req.acquire(ctx, desc) if err != nil { return nil, err } @@ -196,7 +162,7 @@ func FetchHandler(ingester content.Ingester, fetcher remotes.Fetcher, ref string } func PushHandler(pusher remotes.Pusher, provider content.Provider, ref string) images.HandlerFunc { - return remotes.PushHandler(Default.WrapPusher(pusher, ref), provider) + return Default.PushHandler(pusher, provider, ref) } func domain(ref string) string { From f86b8ebd8816d02556db333b589b6cf22b8a54e4 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Thu, 28 Oct 2021 20:30:29 -0700 Subject: [PATCH 3/5] gha: fix handling removed blobs on reexport Signed-off-by: Tonis Tiigi (cherry picked from commit 962c287b1b651b05a5cb8070632ed4425eb7056d) --- cache/remotecache/export.go | 2 +- cache/remotecache/gha/gha.go | 2 +- cache/remotecache/inline/inline.go | 6 +++--- cache/remotecache/v1/chains.go | 5 +++-- cache/remotecache/v1/chains_test.go | 9 +++++---- cache/remotecache/v1/utils.go | 22 +++++++++++++++++----- solver/llbsolver/solver.go | 4 ++-- 7 files changed, 32 insertions(+), 18 deletions(-) diff --git a/cache/remotecache/export.go b/cache/remotecache/export.go index ded6d5cb9871..50a33946d766 100644 --- a/cache/remotecache/export.go +++ b/cache/remotecache/export.go @@ -68,7 +68,7 @@ func NewExporter(ingester content.Ingester, ref string, oci bool) Exporter { func (ce *contentCacheExporter) Finalize(ctx context.Context) (map[string]string, error) { res := make(map[string]string) - config, descs, err := ce.chains.Marshal() + config, descs, err := ce.chains.Marshal(ctx) if err != nil { return nil, err } diff --git a/cache/remotecache/gha/gha.go b/cache/remotecache/gha/gha.go index 9e56c82f997b..f267c59b3881 100644 --- a/cache/remotecache/gha/gha.go +++ b/cache/remotecache/gha/gha.go @@ -106,7 +106,7 @@ func (ce *exporter) indexKey() string { func (ce *exporter) Finalize(ctx context.Context) (map[string]string, error) { // res := make(map[string]string) - config, descs, err := ce.chains.Marshal() + config, descs, err := ce.chains.Marshal(ctx) if err != nil { return nil, err } diff --git a/cache/remotecache/inline/inline.go b/cache/remotecache/inline/inline.go index 449c7c0fb51f..1d1c1180b32a 100644 --- a/cache/remotecache/inline/inline.go +++ b/cache/remotecache/inline/inline.go @@ -38,8 +38,8 @@ func (ce *exporter) reset() { ce.chains = cc } -func (ce *exporter) ExportForLayers(layers []digest.Digest) ([]byte, error) { - config, descs, err := ce.chains.Marshal() +func (ce *exporter) ExportForLayers(ctx context.Context, layers []digest.Digest) ([]byte, error) { + config, descs, err := ce.chains.Marshal(ctx) if err != nil { return nil, err } @@ -63,7 +63,7 @@ func (ce *exporter) ExportForLayers(layers []digest.Digest) ([]byte, error) { return nil, err } - cfg, _, err := cc.Marshal() + cfg, _, err := cc.Marshal(ctx) if err != nil { return nil, err } diff --git a/cache/remotecache/v1/chains.go b/cache/remotecache/v1/chains.go index 166971de17d1..e885f66d2308 100644 --- a/cache/remotecache/v1/chains.go +++ b/cache/remotecache/v1/chains.go @@ -1,6 +1,7 @@ package cacheimport import ( + "context" "strings" "sync" "time" @@ -75,7 +76,7 @@ func (c *CacheChains) normalize() error { return nil } -func (c *CacheChains) Marshal() (*CacheConfig, DescriptorProvider, error) { +func (c *CacheChains) Marshal(ctx context.Context) (*CacheConfig, DescriptorProvider, error) { if err := c.normalize(); err != nil { return nil, nil, err } @@ -87,7 +88,7 @@ func (c *CacheChains) Marshal() (*CacheConfig, DescriptorProvider, error) { } for _, it := range c.items { - if err := marshalItem(it, st); err != nil { + if err := marshalItem(ctx, it, st); err != nil { return nil, nil, err } } diff --git a/cache/remotecache/v1/chains_test.go b/cache/remotecache/v1/chains_test.go index b811f968a9c6..172cb4ccdc6c 100644 --- a/cache/remotecache/v1/chains_test.go +++ b/cache/remotecache/v1/chains_test.go @@ -1,6 +1,7 @@ package cacheimport import ( + "context" "encoding/json" "testing" "time" @@ -33,7 +34,7 @@ func TestSimpleMarshal(t *testing.T) { addRecords() - cfg, _, err := cc.Marshal() + cfg, _, err := cc.Marshal(context.TODO()) require.NoError(t, err) require.Equal(t, len(cfg.Layers), 2) @@ -65,7 +66,7 @@ func TestSimpleMarshal(t *testing.T) { // adding same info again doesn't produce anything extra addRecords() - cfg2, descPairs, err := cc.Marshal() + cfg2, descPairs, err := cc.Marshal(context.TODO()) require.NoError(t, err) require.EqualValues(t, cfg, cfg2) @@ -78,13 +79,13 @@ func TestSimpleMarshal(t *testing.T) { err = Parse(dt, descPairs, newChains) require.NoError(t, err) - cfg3, _, err := cc.Marshal() + cfg3, _, err := cc.Marshal(context.TODO()) require.NoError(t, err) require.EqualValues(t, cfg, cfg3) // add extra item cc.Add(outputKey(dgst("bay"), 0)) - cfg, _, err = cc.Marshal() + cfg, _, err = cc.Marshal(context.TODO()) require.NoError(t, err) require.Equal(t, len(cfg.Layers), 2) diff --git a/cache/remotecache/v1/utils.go b/cache/remotecache/v1/utils.go index 889064a6c175..5ff0c4a796be 100644 --- a/cache/remotecache/v1/utils.go +++ b/cache/remotecache/v1/utils.go @@ -1,12 +1,14 @@ package cacheimport import ( + "context" "fmt" "sort" "github.com/moby/buildkit/exporter/containerimage/exptypes" "github.com/moby/buildkit/solver" digest "github.com/opencontainers/go-digest" + ocispecs "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -277,17 +279,27 @@ type marshalState struct { recordsByItem map[*item]int } -func marshalRemote(r *solver.Remote, state *marshalState) string { +func marshalRemote(ctx context.Context, r *solver.Remote, state *marshalState) string { if len(r.Descriptors) == 0 { return "" } + + if cd, ok := r.Provider.(interface { + CheckDescriptor(context.Context, ocispecs.Descriptor) error + }); ok && len(r.Descriptors) > 0 { + for _, d := range r.Descriptors { + if cd.CheckDescriptor(ctx, d) != nil { + return "" + } + } + } var parentID string if len(r.Descriptors) > 1 { r2 := &solver.Remote{ Descriptors: r.Descriptors[:len(r.Descriptors)-1], Provider: r.Provider, } - parentID = marshalRemote(r2, state) + parentID = marshalRemote(ctx, r2, state) } desc := r.Descriptors[len(r.Descriptors)-1] @@ -318,7 +330,7 @@ func marshalRemote(r *solver.Remote, state *marshalState) string { return id } -func marshalItem(it *item, state *marshalState) error { +func marshalItem(ctx context.Context, it *item, state *marshalState) error { if _, ok := state.recordsByItem[it]; ok { return nil } @@ -330,7 +342,7 @@ func marshalItem(it *item, state *marshalState) error { for i, m := range it.links { for l := range m { - if err := marshalItem(l.src, state); err != nil { + if err := marshalItem(ctx, l.src, state); err != nil { return err } idx, ok := state.recordsByItem[l.src] @@ -345,7 +357,7 @@ func marshalItem(it *item, state *marshalState) error { } if it.result != nil { - id := marshalRemote(it.result, state) + id := marshalRemote(ctx, it.result, state) if id != "" { idx, ok := state.chainsByID[id] if !ok { diff --git a/solver/llbsolver/solver.go b/solver/llbsolver/solver.go index ff38f46f9db3..adf3f70bc843 100644 --- a/solver/llbsolver/solver.go +++ b/solver/llbsolver/solver.go @@ -267,7 +267,7 @@ func (s *Solver) Solve(ctx context.Context, id string, sessionID string, req fro func inlineCache(ctx context.Context, e remotecache.Exporter, res solver.CachedResult, g session.Group) ([]byte, error) { if efl, ok := e.(interface { - ExportForLayers([]digest.Digest) ([]byte, error) + ExportForLayers(context.Context, []digest.Digest) ([]byte, error) }); ok { workerRef, ok := res.Sys().(*worker.WorkerRef) if !ok { @@ -292,7 +292,7 @@ func inlineCache(ctx context.Context, e remotecache.Exporter, res solver.CachedR return nil, err } - return efl.ExportForLayers(digests) + return efl.ExportForLayers(ctx, digests) } return nil, nil } From 6c21d1e0f27adc1f8aeb27f7506964e50531cad1 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Fri, 8 Oct 2021 19:51:11 -0700 Subject: [PATCH 4/5] solver: fix exporters unsafely sharing records Signed-off-by: Tonis Tiigi (cherry picked from commit 4cec7a064cc0a0a6f7756481ab66f8b07b006d6d) --- solver/exporter.go | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/solver/exporter.go b/solver/exporter.go index 26ca2fb9291a..3f983327429a 100644 --- a/solver/exporter.go +++ b/solver/exporter.go @@ -11,7 +11,6 @@ type exporter struct { records []*CacheRecord record *CacheRecord - res []CacheExporterRecord edge *edge // for secondaryExporters override *bool } @@ -52,9 +51,10 @@ func addBacklinks(t CacheExporterTarget, rec CacheExporterRecord, cm *cacheManag return rec, nil } -type backlinkT struct{} +type contextT string -var backlinkKey = backlinkT{} +var backlinkKey = contextT("solver/exporter/backlinks") +var resKey = contextT("solver/exporter/res") func (e *exporter) ExportTo(ctx context.Context, t CacheExporterTarget, opt CacheExportOpt) ([]CacheExporterRecord, error) { var bkm map[string]CacheExporterRecord @@ -66,8 +66,16 @@ func (e *exporter) ExportTo(ctx context.Context, t CacheExporterTarget, opt Cach bkm = bk.(map[string]CacheExporterRecord) } + var res map[*exporter][]CacheExporterRecord + if r := ctx.Value(resKey); r == nil { + res = map[*exporter][]CacheExporterRecord{} + ctx = context.WithValue(ctx, resKey, res) + } else { + res = r.(map[*exporter][]CacheExporterRecord) + } + if t.Visited(e) { - return e.res, nil + return res[e], nil } t.Visit(e) @@ -180,9 +188,9 @@ func (e *exporter) ExportTo(ctx context.Context, t CacheExporterTarget, opt Cach } } - e.res = allRec + res[e] = allRec - return e.res, nil + return allRec, nil } func getBestResult(records []*CacheRecord) *CacheRecord { From dfc83c0271b081111bc1ad3ee35fa7dce1aafe36 Mon Sep 17 00:00:00 2001 From: CrazyMax Date: Tue, 2 Nov 2021 18:02:27 +0100 Subject: [PATCH 5/5] ci: fix git protocol Signed-off-by: CrazyMax (cherry picked from commit b434e7314b9e8fcd260701efa3b0be850b24efa7) --- README.md | 2 +- hack/util | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 590b435365b2..576eb8fd6431 100644 --- a/README.md +++ b/README.md @@ -194,7 +194,7 @@ buildctl build \ buildctl build \ --frontend gateway.v0 \ --opt source=docker/dockerfile \ - --opt context=git://github.com/moby/moby \ + --opt context=https://github.com/moby/moby.git \ --opt build-arg:APT_MIRROR=cdn-fastly.deb.debian.org ``` diff --git a/hack/util b/hack/util index 60e85264d92b..e449e698f85d 100755 --- a/hack/util +++ b/hack/util @@ -46,7 +46,7 @@ cacheRefFrom="" cacheRefTo="" currentref="" if [ "$GITHUB_ACTIONS" = "true" ]; then - currentref="git://github.com/$GITHUB_REPOSITORY#$GITHUB_REF" + currentref="https://github.com/$GITHUB_REPOSITORY.git#$GITHUB_REF" cacheType="local" cacheRefFrom="$CACHEDIR_FROM" cacheRefTo="$CACHEDIR_TO"