From d394e48a8dcbfdf1767858366bd6e5875ca0dfa5 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Tue, 28 Jan 2025 22:22:34 -0800 Subject: [PATCH] dockerfile: avoid too eager loading of unreachable named contextes Signed-off-by: Tonis Tiigi --- frontend/dockerfile/dockerfile2llb/convert.go | 129 ++++++++++-------- frontend/dockerfile/dockerfile_test.go | 46 +++++++ frontend/dockerui/config.go | 13 +- frontend/dockerui/namedcontext.go | 98 ++++++++----- frontend/gateway/gateway.go | 14 +- 5 files changed, 199 insertions(+), 101 deletions(-) diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go index 88e5943ebdcc..c084e3991ee3 100644 --- a/frontend/dockerfile/dockerfile2llb/convert.go +++ b/frontend/dockerfile/dockerfile2llb/convert.go @@ -206,17 +206,17 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS return nil, errors.Errorf("Client and MainContext cannot both be provided") } - namedContext := func(ctx context.Context, name string, copt dockerui.ContextOpt) (*llb.State, *dockerspec.DockerOCIImage, error) { + namedContext := func(name string, copt dockerui.ContextOpt) (*dockerui.NamedContext, error) { if opt.Client == nil { - return nil, nil, nil + return nil, nil } if !strings.EqualFold(name, "scratch") && !strings.EqualFold(name, "context") { if copt.Platform == nil { copt.Platform = opt.TargetPlatform } - return opt.Client.NamedContext(ctx, name, copt) + return opt.Client.NamedContext(name, copt) } - return nil, nil, nil + return nil, nil } lint, err := newRuleLinter(dt, &opt) @@ -349,7 +349,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS } if st.Name != "" { - s, img, err := namedContext(ctx, st.Name, dockerui.ContextOpt{ + nc, err := namedContext(st.Name, dockerui.ContextOpt{ Platform: ds.platform, ResolveMode: opt.ImageResolveMode.String(), AsyncLocalOpts: ds.asyncLocalOpts, @@ -357,25 +357,8 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS if err != nil { return nil, err } - if s != nil { - ds.dispatched = true - ds.state = *s - if img != nil { - // timestamps are inherited as-is, regardless to SOURCE_DATE_EPOCH - // https://github.com/moby/buildkit/issues/4614 - ds.image = *img - if img.Architecture != "" && img.OS != "" { - ds.platform = &ocispecs.Platform{ - OS: img.OS, - Architecture: img.Architecture, - Variant: img.Variant, - OSVersion: img.OSVersion, - } - if img.OSFeatures != nil { - ds.platform.OSFeatures = append([]string{}, img.OSFeatures...) - } - } - } + if nc != nil { + ds.namedContext = nc allDispatchStates.addState(ds) ds.base = nil // reset base set by addState continue @@ -467,7 +450,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS // resolve image config for every stage if d.base == nil && !d.dispatched && !d.resolved { d.resolved = reachable // avoid re-resolving if called again after onbuild - if d.stage.BaseName == emptyImageName { + if d.stage.BaseName == emptyImageName && d.namedContext == nil { d.state = llb.Scratch() d.image = emptyImage(platformOpt.targetPlatform) d.platform = &platformOpt.targetPlatform @@ -499,25 +482,58 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS d.stage.BaseName = reference.TagNameOnly(ref).String() var isScratch bool - st, img, err := namedContext(ctx, d.stage.BaseName, dockerui.ContextOpt{ - ResolveMode: opt.ImageResolveMode.String(), - Platform: platform, - AsyncLocalOpts: d.asyncLocalOpts, - }) - if err != nil { - return err - } - if st != nil { - if img != nil { - d.image = *img - } else { - d.image = emptyImage(platformOpt.targetPlatform) - } - d.state = st.Platform(*platform) - d.platform = platform - return nil - } if reachable { + // stage was named context + if d.namedContext != nil { + st, img, err := d.namedContext.Load(ctx) + if err != nil { + return err + } + d.dispatched = true + d.state = *st + if img != nil { + // timestamps are inherited as-is, regardless to SOURCE_DATE_EPOCH + // https://github.com/moby/buildkit/issues/4614 + d.image = *img + if img.Architecture != "" && img.OS != "" { + d.platform = &ocispecs.Platform{ + OS: img.OS, + Architecture: img.Architecture, + Variant: img.Variant, + OSVersion: img.OSVersion, + } + if img.OSFeatures != nil { + d.platform.OSFeatures = append([]string{}, img.OSFeatures...) + } + } + } + return nil + } + + // check if base is named context + nc, err := namedContext(d.stage.BaseName, dockerui.ContextOpt{ + ResolveMode: opt.ImageResolveMode.String(), + Platform: platform, + AsyncLocalOpts: d.asyncLocalOpts, + }) + if err != nil { + return err + } + if nc != nil { + st, img, err := nc.Load(ctx) + if err != nil { + return err + } + if img != nil { + d.image = *img + } else { + d.image = emptyImage(platformOpt.targetPlatform) + } + d.state = st.Platform(*platform) + d.platform = platform + return nil + } + prefix := "[" if opt.MultiPlatformRequested && platform != nil { prefix += platforms.FormatAll(*platform) + " " @@ -1015,19 +1031,20 @@ func dispatch(d *dispatchState, cmd command, opt dispatchOpt) error { } type dispatchState struct { - opt dispatchOpt - state llb.State - image dockerspec.DockerOCIImage - platform *ocispecs.Platform - stage instructions.Stage - base *dispatchState - baseImg *dockerspec.DockerOCIImage // immutable, unlike image - dispatched bool - resolved bool // resolved is set to true if base image has been resolved - onBuildInit bool - deps map[*dispatchState]instructions.Command - buildArgs []instructions.KeyValuePairOptional - commands []command + opt dispatchOpt + state llb.State + image dockerspec.DockerOCIImage + namedContext *dockerui.NamedContext + platform *ocispecs.Platform + stage instructions.Stage + base *dispatchState + baseImg *dockerspec.DockerOCIImage // immutable, unlike image + dispatched bool + resolved bool // resolved is set to true if base image has been resolved + onBuildInit bool + deps map[*dispatchState]instructions.Command + buildArgs []instructions.KeyValuePairOptional + commands []command // ctxPaths marks the paths this dispatchState uses from the build context. ctxPaths map[string]struct{} // paths marks the paths that are used by this dispatchState. diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go index 33cafc9e1b1c..c1195dbbb503 100644 --- a/frontend/dockerfile/dockerfile_test.go +++ b/frontend/dockerfile/dockerfile_test.go @@ -205,6 +205,7 @@ var allTests = integration.TestFuncs( testFrontendDeduplicateSources, testDuplicateLayersProvenance, testSourcePolicyWithNamedContext, + testEagerNamedContextLookup, testEmptyStringArgInEnv, testInvalidJSONCommands, testHistoryError, @@ -9167,6 +9168,51 @@ func testSourcePolicyWithNamedContext(t *testing.T, sb integration.Sandbox) { require.Equal(t, "foo", string(dt)) } +// testEagerNamedContextLookup tests that named context are not loaded if +// they are not used by current build. +func testEagerNamedContextLookup(t *testing.T, sb integration.Sandbox) { + integration.SkipOnPlatform(t, "windows") + + f := getFrontend(t, sb) + c, err := client.New(sb.Context(), sb.Address()) + require.NoError(t, err) + defer c.Close() + + dockerfile := []byte(` +FROM broken AS notused + +FROM alpine AS base +RUN echo "base" > /foo + +FROM busybox AS otherstage + +FROM scratch +COPY --from=base /foo /foo + `) + + dir := integration.Tmpdir(t, fstest.CreateFile("Dockerfile", dockerfile, 0600)) + + out := t.TempDir() + _, err = f.Solve(sb.Context(), c, client.SolveOpt{ + Exports: []client.ExportEntry{ + {Type: client.ExporterLocal, OutputDir: out}, + }, + FrontendAttrs: map[string]string{ + "context:notused": "docker-image://docker.io/library/nosuchimage:latest", + "context:busybox": "docker-image://docker.io/library/dontexist:latest", + }, + LocalMounts: map[string]fsutil.FS{ + dockerui.DefaultLocalNameDockerfile: dir, + dockerui.DefaultLocalNameContext: dir, + }, + }, nil) + require.NoError(t, err) + + dt, err := os.ReadFile(filepath.Join(out, "foo")) + require.NoError(t, err) + require.Equal(t, "base\n", string(dt)) +} + func testBaseImagePlatformMismatch(t *testing.T, sb integration.Sandbox) { integration.SkipOnPlatform(t, "windows") workers.CheckFeatureCompat(t, sb, workers.FeatureDirectPush) diff --git a/frontend/dockerui/config.go b/frontend/dockerui/config.go index a880c886ee84..368b57080c65 100644 --- a/frontend/dockerui/config.go +++ b/frontend/dockerui/config.go @@ -18,7 +18,6 @@ import ( "github.com/moby/buildkit/frontend/gateway/client" "github.com/moby/buildkit/solver/pb" "github.com/moby/buildkit/util/flightcontrol" - dockerspec "github.com/moby/docker-image-spec/specs-go/v1" "github.com/moby/patternmatcher/ignorefile" digest "github.com/opencontainers/go-digest" ocispecs "github.com/opencontainers/image-spec/specs-go/v1" @@ -452,10 +451,10 @@ func (bc *Client) MainContext(ctx context.Context, opts ...llb.LocalOption) (*ll return &st, nil } -func (bc *Client) NamedContext(ctx context.Context, name string, opt ContextOpt) (*llb.State, *dockerspec.DockerOCIImage, error) { +func (bc *Client) NamedContext(name string, opt ContextOpt) (*NamedContext, error) { named, err := reference.ParseNormalizedNamed(name) if err != nil { - return nil, nil, errors.Wrapf(err, "invalid context name %s", name) + return nil, errors.Wrapf(err, "invalid context name %s", name) } name = strings.TrimSuffix(reference.FamiliarString(named), ":latest") @@ -464,11 +463,11 @@ func (bc *Client) NamedContext(ctx context.Context, name string, opt ContextOpt) pp = *opt.Platform } pname := name + "::" + platforms.FormatAll(platforms.Normalize(pp)) - st, img, err := bc.namedContext(ctx, name, pname, opt) - if err != nil || st != nil { - return st, img, err + nc, err := bc.namedContext(name, pname, opt) + if err != nil || nc != nil { + return nc, err } - return bc.namedContext(ctx, name, name, opt) + return bc.namedContext(name, name, opt) } func (bc *Client) IsNoCache(name string) bool { diff --git a/frontend/dockerui/namedcontext.go b/frontend/dockerui/namedcontext.go index 3bf7343e4e13..9598d0b2d9bc 100644 --- a/frontend/dockerui/namedcontext.go +++ b/frontend/dockerui/namedcontext.go @@ -26,31 +26,51 @@ const ( maxContextRecursion = 10 ) -func (bc *Client) namedContext(ctx context.Context, name string, nameWithPlatform string, opt ContextOpt) (*llb.State, *dockerspec.DockerOCIImage, error) { - return bc.namedContextRecursive(ctx, name, nameWithPlatform, opt, 0) +type NamedContext struct { + input string + bc *Client + name string + nameWithPlatform string + opt ContextOpt } -func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWithPlatform string, opt ContextOpt, count int) (*llb.State, *dockerspec.DockerOCIImage, error) { +func (bc *Client) namedContext(name string, nameWithPlatform string, opt ContextOpt) (*NamedContext, error) { opts := bc.bopts.Opts contextKey := contextPrefix + nameWithPlatform v, ok := opts[contextKey] if !ok { - return nil, nil, nil + return nil, nil } + return &NamedContext{ + input: v, + bc: bc, + name: name, + nameWithPlatform: nameWithPlatform, + opt: opt, + }, nil +} + +func (nc *NamedContext) Load(ctx context.Context) (*llb.State, *dockerspec.DockerOCIImage, error) { + return nc.load(ctx, 0) +} + +func (nc *NamedContext) load(ctx context.Context, count int) (*llb.State, *dockerspec.DockerOCIImage, error) { + opt := nc.opt if count > maxContextRecursion { - return nil, nil, errors.New("context recursion limit exceeded; this may indicate a cycle in the provided source policies: " + v) + return nil, nil, errors.New("context recursion limit exceeded; this may indicate a cycle in the provided source policies: " + nc.input) } - vv := strings.SplitN(v, ":", 2) + vv := strings.SplitN(nc.input, ":", 2) if len(vv) != 2 { - return nil, nil, errors.Errorf("invalid context specifier %s for %s", v, nameWithPlatform) + return nil, nil, errors.Errorf("invalid context specifier %s for %s", nc.input, nc.nameWithPlatform) } // allow git@ without protocol for SSH URLs for backwards compatibility if strings.HasPrefix(vv[0], "git@") { vv[0] = "git" } + switch vv[0] { case "docker-image": ref := strings.TrimPrefix(vv[1], "//") @@ -60,7 +80,7 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi } imgOpt := []llb.ImageOption{ - llb.WithCustomName("[context " + nameWithPlatform + "] " + ref), + llb.WithCustomName("[context " + nc.nameWithPlatform + "] " + ref), } if opt.Platform != nil { imgOpt = append(imgOpt, llb.Platform(*opt.Platform)) @@ -73,8 +93,8 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi named = reference.TagNameOnly(named) - ref, dgst, data, err := bc.client.ResolveImageConfig(ctx, named.String(), sourceresolver.Opt{ - LogName: fmt.Sprintf("[context %s] load metadata for %s", nameWithPlatform, ref), + ref, dgst, data, err := nc.bc.client.ResolveImageConfig(ctx, named.String(), sourceresolver.Opt{ + LogName: fmt.Sprintf("[context %s] load metadata for %s", nc.nameWithPlatform, ref), Platform: opt.Platform, ImageOpt: &sourceresolver.ResolveImageOpt{ ResolveMode: opt.ResolveMode, @@ -88,8 +108,16 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi return nil, nil, errors.Errorf("could not parse ref: %s", e.Updated) } - bc.bopts.Opts[contextKey] = before + ":" + after - return bc.namedContextRecursive(ctx, name, nameWithPlatform, opt, count+1) + nc.bc.bopts.Opts[contextPrefix+nc.nameWithPlatform] = before + ":" + after + + ncnew, err := nc.bc.namedContext(nc.name, nc.nameWithPlatform, nc.opt) + if err != nil { + return nil, nil, err + } + if ncnew == nil { + return nil, nil, nil + } + return ncnew.load(ctx, count+1) } return nil, nil, err } @@ -110,15 +138,15 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi } return &st, &img, nil case "git": - st, ok := DetectGitContext(v, true) + st, ok := DetectGitContext(nc.input, true) if !ok { - return nil, nil, errors.Errorf("invalid git context %s", v) + return nil, nil, errors.Errorf("invalid git context %s", nc.input) } return st, nil, nil case "http", "https": - st, ok := DetectGitContext(v, true) + st, ok := DetectGitContext(nc.input, true) if !ok { - httpst := llb.HTTP(v, llb.WithCustomName("[context "+nameWithPlatform+"] "+v)) + httpst := llb.HTTP(nc.input, llb.WithCustomName("[context "+nc.nameWithPlatform+"] "+nc.input)) st = &httpst } return st, nil, nil @@ -139,21 +167,21 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi // for the dummy ref primarily used in log messages, we can use the // original name, since the store key may not be significant - dummyRef, err := reference.ParseNormalizedNamed(name) + dummyRef, err := reference.ParseNormalizedNamed(nc.name) if err != nil { - return nil, nil, errors.Wrapf(err, "could not parse oci-layout reference %q", name) + return nil, nil, errors.Wrapf(err, "could not parse oci-layout reference %q", nc.name) } dummyRef, err = reference.WithDigest(dummyRef, dgstd.Digest()) if err != nil { - return nil, nil, errors.Wrapf(err, "could not wrap %q with digest", name) + return nil, nil, errors.Wrapf(err, "could not wrap %q with digest", nc.name) } - _, dgst, data, err := bc.client.ResolveImageConfig(ctx, dummyRef.String(), sourceresolver.Opt{ - LogName: fmt.Sprintf("[context %s] load metadata for %s", nameWithPlatform, dummyRef.String()), + _, dgst, data, err := nc.bc.client.ResolveImageConfig(ctx, dummyRef.String(), sourceresolver.Opt{ + LogName: fmt.Sprintf("[context %s] load metadata for %s", nc.nameWithPlatform, dummyRef.String()), Platform: opt.Platform, OCILayoutOpt: &sourceresolver.ResolveOCILayoutOpt{ Store: sourceresolver.ResolveImageConfigOptStore{ - SessionID: bc.bopts.SessionID, + SessionID: nc.bc.bopts.SessionID, StoreID: named.Name(), }, }, @@ -168,8 +196,8 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi } ociOpt := []llb.OCILayoutOption{ - llb.WithCustomName("[context " + nameWithPlatform + "] OCI load from client"), - llb.OCIStore(bc.bopts.SessionID, named.Name()), + llb.WithCustomName("[context " + nc.nameWithPlatform + "] OCI load from client"), + llb.OCIStore(nc.bc.bopts.SessionID, named.Name()), } if opt.Platform != nil { ociOpt = append(ociOpt, llb.Platform(*opt.Platform)) @@ -187,22 +215,22 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi } return &st, &img, nil case "local": - sessionID := bc.bopts.SessionID - if v, ok := bc.localsSessionIDs[vv[1]]; ok { + sessionID := nc.bc.bopts.SessionID + if v, ok := nc.bc.localsSessionIDs[vv[1]]; ok { sessionID = v } st := llb.Local(vv[1], llb.SessionID(sessionID), llb.FollowPaths([]string{DefaultDockerignoreName}), - llb.SharedKeyHint("context:"+nameWithPlatform+"-"+DefaultDockerignoreName), - llb.WithCustomName("[context "+nameWithPlatform+"] load "+DefaultDockerignoreName), + llb.SharedKeyHint("context:"+nc.nameWithPlatform+"-"+DefaultDockerignoreName), + llb.WithCustomName("[context "+nc.nameWithPlatform+"] load "+DefaultDockerignoreName), llb.Differ(llb.DiffNone, false), ) def, err := st.Marshal(ctx) if err != nil { return nil, nil, err } - res, err := bc.client.Solve(ctx, client.SolveRequest{ + res, err := nc.bc.client.Solve(ctx, client.SolveRequest{ Evaluate: true, Definition: def.ToPB(), }) @@ -229,7 +257,7 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi localOutput := &asyncLocalOutput{ name: vv[1], - nameWithPlatform: nameWithPlatform, + nameWithPlatform: nc.nameWithPlatform, sessionID: sessionID, excludes: excludes, extraOpts: opt.AsyncLocalOpts, @@ -237,15 +265,15 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi st = llb.NewState(localOutput) return &st, nil, nil case "input": - inputs, err := bc.client.Inputs(ctx) + inputs, err := nc.bc.client.Inputs(ctx) if err != nil { return nil, nil, err } st, ok := inputs[vv[1]] if !ok { - return nil, nil, errors.Errorf("invalid input %s for %s", vv[1], nameWithPlatform) + return nil, nil, errors.Errorf("invalid input %s for %s", vv[1], nc.nameWithPlatform) } - md, ok := opts[inputMetadataPrefix+vv[1]] + md, ok := nc.bc.bopts.Opts[inputMetadataPrefix+vv[1]] if ok { m := make(map[string][]byte) if err := json.Unmarshal([]byte(md), &m); err != nil { @@ -258,14 +286,14 @@ func (bc *Client) namedContextRecursive(ctx context.Context, name string, nameWi return nil, nil, err } if err := json.Unmarshal(dtic, &img); err != nil { - return nil, nil, errors.Wrapf(err, "failed to parse image config for %s", nameWithPlatform) + return nil, nil, errors.Wrapf(err, "failed to parse image config for %s", nc.nameWithPlatform) } } return &st, img, nil } return &st, nil, nil default: - return nil, nil, errors.Errorf("unsupported context source %s for %s", vv[0], nameWithPlatform) + return nil, nil, errors.Errorf("unsupported context source %s for %s", vv[0], nc.nameWithPlatform) } } diff --git a/frontend/gateway/gateway.go b/frontend/gateway/gateway.go index 324455f9761f..19edcd84f10e 100644 --- a/frontend/gateway/gateway.go +++ b/frontend/gateway/gateway.go @@ -191,14 +191,22 @@ func (gf *gatewayFrontend) Solve(ctx context.Context, llbBridge frontend.Fronten if err != nil { return nil, err } - st, dockerImage, err := dc.NamedContext(ctx, source, dockerui.ContextOpt{ + nc, err := dc.NamedContext(source, dockerui.ContextOpt{ CaptureDigest: &mfstDigest, }) if err != nil { return nil, err } - if dockerImage != nil { - img = *dockerImage + var st *llb.State + if nc != nil { + var dockerImage *dockerspec.DockerOCIImage + st, dockerImage, err = nc.Load(ctx) + if err != nil { + return nil, err + } + if dockerImage != nil { + img = *dockerImage + } } if st == nil { sourceRef, err := reference.ParseNormalizedNamed(source)