Merge pull request #33054 from dnephin/refactor-builder-named-contexts-interface

[Builder] Expose GetImage interface for builder

api/types/backend/build.go

@@ -22,3 +22,10 @@ type BuildConfig struct {
 	ProgressWriter ProgressWriter
 	Options        *types.ImageBuildOptions
 }
+
+// GetImageAndLayerOptions are the options supported by GetImageAndReleasableLayer
+type GetImageAndLayerOptions struct {
+	ForcePull  bool
+	AuthConfig map[string]types.AuthConfig
+	Output     io.Writer
+}

builder/builder.go

@@ -34,12 +34,8 @@ type Source interface {
 
 // Backend abstracts calls to a Docker Daemon.
 type Backend interface {
-	// TODO: use digest reference instead of name
+	ImageBackend
 
-	// GetImageOnBuild looks up a Docker image referenced by `name`.
-	GetImageOnBuild(name string) (Image, error)
-	// PullOnBuild tells Docker to pull image referenced by `name`.
-	PullOnBuild(ctx context.Context, name string, authConfigs map[string]types.AuthConfig, output io.Writer) (Image, error)
 	// ContainerAttachRaw attaches to container.
 	ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool, attached chan struct{}) error
 	// ContainerCreate creates a new Docker container and returns potential warnings
@@ -62,15 +58,11 @@ type Backend interface {
 	// TODO: extract in the builder instead of passing `decompress`
 	// TODO: use containerd/fs.changestream instead as a source
 	CopyOnBuild(containerID string, destPath string, srcRoot string, srcPath string, decompress bool) error
-
-	// MountImage returns mounted path with rootfs of an image.
-	MountImage(name string) (string, func() error, error)
 }
 
-// Image represents a Docker image used by the builder.
-type Image interface {
-	ImageID() string
-	RunConfig() *container.Config
+// ImageBackend are the interface methods required from an image component
+type ImageBackend interface {
+	GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (Image, ReleaseableLayer, error)
 }
 
 // Result is the output produced by a Builder
@@ -92,3 +84,15 @@ type ImageCache interface {
 	// and runconfig equals `cfg`. A cache miss is expected to return an empty ID and a nil error.
 	GetCache(parentID string, cfg *container.Config) (imageID string, err error)
 }
+
+// Image represents a Docker image used by the builder.
+type Image interface {
+	ImageID() string
+	RunConfig() *container.Config
+}
+
+// ReleaseableLayer is an image layer that can be mounted and released
+type ReleaseableLayer interface {
+	Release() error
+	Mount() (string, error)
+}

builder/dockerfile/builder.go

@@ -45,7 +45,10 @@ type BuildManager struct {
 
 // NewBuildManager creates a BuildManager
 func NewBuildManager(b builder.Backend) *BuildManager {
-	return &BuildManager{backend: b, pathCache: &syncmap.Map{}}
+	return &BuildManager{
+		backend:   b,
+		pathCache: &syncmap.Map{},
+	}
 }
 
 // Build starts a new build from a BuildConfig
@@ -99,11 +102,12 @@ type Builder struct {
 	clientCtx context.Context
 
 	tmpContainers map[string]struct{}
-	imageContexts *imageContexts // helper for storing contexts from builds
+	buildStages   *buildStages
 	disableCommit bool
 	cacheBusted   bool
 	buildArgs     *buildArgs
 	imageCache    builder.ImageCache
+	imageSources  *imageSources
 }
 
 // newBuilder creates a new Dockerfile builder from an optional dockerfile and a Options.
@@ -122,8 +126,9 @@ func newBuilder(clientCtx context.Context, options builderOptions) *Builder {
 		docker:        options.Backend,
 		tmpContainers: map[string]struct{}{},
 		buildArgs:     newBuildArgs(config.BuildArgs),
+		buildStages:   newBuildStages(),
+		imageSources:  newImageSources(clientCtx, options),
 	}
-	b.imageContexts = &imageContexts{b: b, cache: options.PathCache}
 	return b
 }
 
@@ -137,7 +142,7 @@ func (b *Builder) resetImageCache() {
 // Build runs the Dockerfile builder by parsing the Dockerfile and executing
 // the instructions from the file.
 func (b *Builder) build(source builder.Source, dockerfile *parser.Result) (*builder.Result, error) {
-	defer b.imageContexts.unmount()
+	defer b.imageSources.Unmount()
 
 	// TODO: Remove source field from Builder
 	b.source = source

builder/dockerfile/dispatchers.go

@@ -23,6 +23,7 @@ import (
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/api/types/strslice"
 	"github.com/docker/docker/builder"
+	"github.com/docker/docker/builder/dockerfile/parser"
 	"github.com/docker/docker/pkg/signal"
 	"github.com/docker/go-connections/nat"
 	"github.com/pkg/errors"
@@ -160,23 +161,34 @@ func dispatchCopy(req dispatchRequest) error {
 	}
 
 	flFrom := req.flags.AddString("from", "")
-
 	if err := req.flags.Parse(); err != nil {
 		return err
 	}
 
-	var im *imageMount
-	if flFrom.IsUsed() {
-		var err error
-		im, err = req.builder.imageContexts.get(flFrom.Value)
-		if err != nil {
-			return err
-		}
+	im, err := req.builder.getImageMount(flFrom)
+	if err != nil {
+		return errors.Wrapf(err, "invalid from flag value %s", flFrom.Value)
 	}
-
 	return req.builder.runContextCommand(req, false, false, "COPY", im)
 }
 
+func (b *Builder) getImageMount(fromFlag *Flag) (*imageMount, error) {
+	if !fromFlag.IsUsed() {
+		// TODO: this could return the source in the default case as well?
+		return nil, nil
+	}
+
+	imageRefOrID := fromFlag.Value
+	stage, err := b.buildStages.get(fromFlag.Value)
+	if err != nil {
+		return nil, err
+	}
+	if stage != nil {
+		imageRefOrID = stage.ImageID()
+	}
+	return b.imageSources.Get(imageRefOrID)
+}
+
 // FROM imagename[:tag | @digest] [AS build-stage-name]
 //
 func from(req dispatchRequest) error {
@@ -190,27 +202,21 @@ func from(req dispatchRequest) error {
 	}
 
 	req.builder.resetImageCache()
-	req.state.noBaseImage = false
-	req.state.stageName = stageName
-	if _, err := req.builder.imageContexts.add(stageName); err != nil {
+	image, err := req.builder.getFromImage(req.shlex, req.args[0])
+	if err != nil {
 		return err
 	}
-
-	image, err := req.builder.getFromImage(req.state, req.shlex, req.args[0])
-	if err != nil {
+	if err := req.builder.buildStages.add(stageName, image); err != nil {
 		return err
 	}
-	switch image {
-	case nil:
-		req.state.imageID = ""
-		req.state.noBaseImage = true
-	default:
-		req.builder.imageContexts.update(image.ImageID(), image.RunConfig())
+	req.state.beginStage(stageName, image)
+	req.builder.buildArgs.ResetAllowed()
+	if image.ImageID() == "" {
+		// Typically this means they used "FROM scratch"
+		return nil
 	}
-	req.state.baseImage = image
 
-	req.builder.buildArgs.ResetAllowed()
-	return req.builder.processImageFrom(req.state, image)
+	return processOnBuild(req)
 }
 
 func parseBuildStageName(args []string) (string, error) {
@@ -228,7 +234,12 @@ func parseBuildStageName(args []string) (string, error) {
 	return stageName, nil
 }
 
-func (b *Builder) getFromImage(dispatchState *dispatchState, shlex *ShellLex, name string) (builder.Image, error) {
+// scratchImage is used as a token for the empty base image. It uses buildStage
+// as a convenient implementation of builder.Image, but is not actually a
+// buildStage.
+var scratchImage builder.Image = &buildStage{}
+
+func (b *Builder) getFromImage(shlex *ShellLex, name string) (builder.Image, error) {
 	substitutionArgs := []string{}
 	for key, value := range b.buildArgs.GetAllMeta() {
 		substitutionArgs = append(substitutionArgs, key+"="+value)
@@ -239,12 +250,8 @@ func (b *Builder) getFromImage(dispatchState *dispatchState, shlex *ShellLex, na
 		return nil, err
 	}
 
-	if im, ok := b.imageContexts.byName[name]; ok {
-		if len(im.ImageID()) > 0 {
-			return im, nil
-		}
-		// FROM scratch does not have an ImageID
-		return nil, nil
+	if im, ok := b.buildStages.getByName(name); ok {
+		return im, nil
 	}
 
 	// Windows cannot support a container with no base image.
@@ -252,9 +259,60 @@ func (b *Builder) getFromImage(dispatchState *dispatchState, shlex *ShellLex, na
 		if runtime.GOOS == "windows" {
 			return nil, errors.New("Windows does not support FROM scratch")
 		}
-		return nil, nil
+		return scratchImage, nil
 	}
-	return pullOrGetImage(b, name)
+	imageMount, err := b.imageSources.Get(name)
+	if err != nil {
+		return nil, err
+	}
+	return imageMount.Image(), nil
+}
+
+func processOnBuild(req dispatchRequest) error {
+	dispatchState := req.state
+	// Process ONBUILD triggers if they exist
+	if nTriggers := len(dispatchState.runConfig.OnBuild); nTriggers != 0 {
+		word := "trigger"
+		if nTriggers > 1 {
+			word = "triggers"
+		}
+		fmt.Fprintf(req.builder.Stderr, "# Executing %d build %s...\n", nTriggers, word)
+	}
+
+	// Copy the ONBUILD triggers, and remove them from the config, since the config will be committed.
+	onBuildTriggers := dispatchState.runConfig.OnBuild
+	dispatchState.runConfig.OnBuild = []string{}
+
+	// Reset stdin settings as all build actions run without stdin
+	dispatchState.runConfig.OpenStdin = false
+	dispatchState.runConfig.StdinOnce = false
+
+	// parse the ONBUILD triggers by invoking the parser
+	for _, step := range onBuildTriggers {
+		dockerfile, err := parser.Parse(strings.NewReader(step))
+		if err != nil {
+			return err
+		}
+
+		for _, n := range dockerfile.AST.Children {
+			if err := checkDispatch(n); err != nil {
+				return err
+			}
+
+			upperCasedCmd := strings.ToUpper(n.Value)
+			switch upperCasedCmd {
+			case "ONBUILD":
+				return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
+			case "MAINTAINER", "FROM":
+				return errors.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd)
+			}
+		}
+
+		if _, err := dispatchFromDockerfile(req.builder, dockerfile, dispatchState); err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 // ONBUILD RUN echo yo
@@ -801,29 +859,3 @@ func errBlankCommandNames(command string) error {
 func errTooManyArguments(command string) error {
 	return fmt.Errorf("Bad input to %s, too many arguments", command)
 }
-
-// mountByRef creates an imageMount from a reference. pulling the image if needed.
-func mountByRef(b *Builder, name string) (*imageMount, error) {
-	image, err := pullOrGetImage(b, name)
-	if err != nil {
-		return nil, err
-	}
-	im := b.imageContexts.newImageMount(image.ImageID())
-	return im, nil
-}
-
-func pullOrGetImage(b *Builder, name string) (builder.Image, error) {
-	var image builder.Image
-	if !b.options.PullParent {
-		image, _ = b.docker.GetImageOnBuild(name)
-		// TODO: shouldn't we error out if error is different from "not found" ?
-	}
-	if image == nil {
-		var err error
-		image, err = b.docker.PullOnBuild(b.clientCtx, name, b.options.AuthConfigs, b.Output)
-		if err != nil {
-			return nil, err
-		}
-	}
-	return image, nil
-}

builder/dockerfile/dispatchers_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/docker/docker/api/types/strslice"
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/dockerfile/parser"
+	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/pkg/testutil"
 	"github.com/docker/go-connections/nat"
 	"github.com/stretchr/testify/assert"
@@ -47,16 +48,22 @@ func defaultDispatchReq(builder *Builder, args ...string) dispatchRequest {
 }
 
 func newBuilderWithMockBackend() *Builder {
+	mockBackend := &MockBackend{}
+	ctx := context.Background()
 	b := &Builder{
 		options:       &types.ImageBuildOptions{},
-		docker:        &MockBackend{},
+		docker:        mockBackend,
 		buildArgs:     newBuildArgs(make(map[string]*string)),
 		tmpContainers: make(map[string]struct{}),
 		Stdout:        new(bytes.Buffer),
-		clientCtx:     context.Background(),
+		clientCtx:     ctx,
 		disableCommit: true,
+		imageSources: newImageSources(ctx, builderOptions{
+			Options: &types.ImageBuildOptions{},
+			Backend: mockBackend,
+		}),
+		buildStages: newBuildStages(),
 	}
-	b.imageContexts = &imageContexts{b: b}
 	return b
 }
 
@@ -191,19 +198,20 @@ func TestFromScratch(t *testing.T) {
 	}
 
 	require.NoError(t, err)
+	assert.True(t, req.state.hasFromImage())
 	assert.Equal(t, "", req.state.imageID)
-	assert.Equal(t, true, req.state.noBaseImage)
+	assert.Equal(t, []string{"PATH=" + system.DefaultPathEnv}, req.state.runConfig.Env)
 }
 
 func TestFromWithArg(t *testing.T) {
 	tag, expected := ":sometag", "expectedthisid"
 
-	getImage := func(name string) (builder.Image, error) {
+	getImage := func(name string) (builder.Image, builder.ReleaseableLayer, error) {
 		assert.Equal(t, "alpine"+tag, name)
-		return &mockImage{id: "expectedthisid"}, nil
+		return &mockImage{id: "expectedthisid"}, nil, nil
 	}
 	b := newBuilderWithMockBackend()
-	b.docker.(*MockBackend).getImageOnBuildFunc = getImage
+	b.docker.(*MockBackend).getImageFunc = getImage
 
 	require.NoError(t, arg(defaultDispatchReq(b, "THETAG="+tag)))
 	req := defaultDispatchReq(b, "alpine${THETAG}")
@@ -219,12 +227,12 @@ func TestFromWithArg(t *testing.T) {
 func TestFromWithUndefinedArg(t *testing.T) {
 	tag, expected := "sometag", "expectedthisid"
 
-	getImage := func(name string) (builder.Image, error) {
+	getImage := func(name string) (builder.Image, builder.ReleaseableLayer, error) {
 		assert.Equal(t, "alpine", name)
-		return &mockImage{id: "expectedthisid"}, nil
+		return &mockImage{id: "expectedthisid"}, nil, nil
 	}
 	b := newBuilderWithMockBackend()
-	b.docker.(*MockBackend).getImageOnBuildFunc = getImage
+	b.docker.(*MockBackend).getImageFunc = getImage
 	b.options.BuildArgs = map[string]*string{"THETAG": &tag}
 
 	req := defaultDispatchReq(b, "alpine${THETAG}")
@@ -474,9 +482,11 @@ func TestRunWithBuildArgs(t *testing.T) {
 	b.imageCache = imageCache
 
 	mockBackend := b.docker.(*MockBackend)
-	mockBackend.getImageOnBuildImage = &mockImage{
-		id:     "abcdef",
-		config: &container.Config{Cmd: origCmd},
+	mockBackend.getImageFunc = func(_ string) (builder.Image, builder.ReleaseableLayer, error) {
+		return &mockImage{
+			id:     "abcdef",
+			config: &container.Config{Cmd: origCmd},
+		}, nil, nil
 	}
 	mockBackend.containerCreateFunc = func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) {
 		// Check the runConfig.Cmd sent to create()

builder/dockerfile/evaluator.go

@@ -28,6 +28,7 @@ import (
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/dockerfile/command"
 	"github.com/docker/docker/builder/dockerfile/parser"
+	"github.com/docker/docker/pkg/system"
 	"github.com/docker/docker/runconfig/opts"
 	"github.com/pkg/errors"
 )
@@ -184,37 +185,55 @@ type dispatchOptions struct {
 
 // dispatchState is a data object which is modified by dispatchers
 type dispatchState struct {
-	runConfig   *container.Config
-	maintainer  string
-	cmdSet      bool
-	noBaseImage bool
-	imageID     string
-	baseImage   builder.Image
-	stageName   string
+	runConfig  *container.Config
+	maintainer string
+	cmdSet     bool
+	imageID    string
+	baseImage  builder.Image
+	stageName  string
 }
 
 func newDispatchState() *dispatchState {
 	return &dispatchState{runConfig: &container.Config{}}
 }
 
-func (r *dispatchState) updateRunConfig() {
-	r.runConfig.Image = r.imageID
+func (s *dispatchState) updateRunConfig() {
+	s.runConfig.Image = s.imageID
 }
 
 // hasFromImage returns true if the builder has processed a `FROM <image>` line
-func (r *dispatchState) hasFromImage() bool {
-	return r.imageID != "" || r.noBaseImage
+func (s *dispatchState) hasFromImage() bool {
+	return s.imageID != "" || (s.baseImage != nil && s.baseImage.ImageID() == "")
 }
 
-func (r *dispatchState) runConfigEnvMapping() map[string]string {
-	return opts.ConvertKVStringsToMap(r.runConfig.Env)
-}
-
-func (r *dispatchState) isCurrentStage(target string) bool {
+func (s *dispatchState) isCurrentStage(target string) bool {
 	if target == "" {
 		return false
 	}
-	return strings.EqualFold(r.stageName, target)
+	return strings.EqualFold(s.stageName, target)
+}
+
+func (s *dispatchState) beginStage(stageName string, image builder.Image) {
+	s.stageName = stageName
+	s.imageID = image.ImageID()
+
+	if image.RunConfig() != nil {
+		s.runConfig = image.RunConfig()
+	}
+	s.baseImage = image
+	s.setDefaultPath()
+}
+
+// Add the default PATH to runConfig.ENV if one exists for the platform and there
+// is no PATH set. Note that windows won't have one as it's set by HCS
+func (s *dispatchState) setDefaultPath() {
+	if system.DefaultPathEnv == "" {
+		return
+	}
+	envMap := opts.ConvertKVStringsToMap(s.runConfig.Env)
+	if _, ok := envMap["PATH"]; !ok {
+		s.runConfig.Env = append(s.runConfig.Env, "PATH="+system.DefaultPathEnv)
+	}
 }
 
 func handleOnBuildNode(ast *parser.Node, msg *bytes.Buffer) (*parser.Node, []string, error) {

builder/dockerfile/imagecontext.go

@@ -5,10 +5,12 @@ import (
 	"strings"
 
 	"github.com/Sirupsen/logrus"
+	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/builder"
 	"github.com/docker/docker/builder/remotecontext"
 	"github.com/pkg/errors"
+	"golang.org/x/net/context"
 )
 
 type pathCache interface {
@@ -16,80 +18,128 @@ type pathCache interface {
 	Store(key, value interface{})
 }
 
-// imageContexts is a helper for stacking up built image rootfs and reusing
-// them as contexts
-type imageContexts struct {
-	b              *Builder
-	list           []*imageMount // indexed list of stages
-	implicitMounts []*imageMount // implicitly mounted images
-	byName         map[string]*imageMount
-	cache          pathCache
+type buildStage struct {
+	id     string
+	config *container.Config
 }
 
-func (ic *imageContexts) newImageMount(id string) *imageMount {
-	return &imageMount{ic: ic, id: id}
+func newBuildStageFromImage(image builder.Image) *buildStage {
+	return &buildStage{id: image.ImageID(), config: image.RunConfig()}
 }
 
-func (ic *imageContexts) add(name string) (*imageMount, error) {
-	im := &imageMount{ic: ic}
-	if len(name) > 0 {
-		if ic.byName == nil {
-			ic.byName = make(map[string]*imageMount)
-		}
-		if _, ok := ic.byName[name]; ok {
-			return nil, errors.Errorf("duplicate name %s", name)
+func (b *buildStage) ImageID() string {
+	return b.id
+}
+
+func (b *buildStage) RunConfig() *container.Config {
+	return b.config
+}
+
+func (b *buildStage) update(imageID string, runConfig *container.Config) {
+	b.id = imageID
+	b.config = runConfig
+}
+
+var _ builder.Image = &buildStage{}
+
+// buildStages tracks each stage of a build so they can be retrieved by index
+// or by name.
+type buildStages struct {
+	sequence []*buildStage
+	byName   map[string]*buildStage
+}
+
+func newBuildStages() *buildStages {
+	return &buildStages{byName: make(map[string]*buildStage)}
+}
+
+func (s *buildStages) getByName(name string) (builder.Image, bool) {
+	stage, ok := s.byName[strings.ToLower(name)]
+	return stage, ok
+}
+
+func (s *buildStages) get(indexOrName string) (builder.Image, error) {
+	index, err := strconv.Atoi(indexOrName)
+	if err == nil {
+		if err := s.validateIndex(index); err != nil {
+			return nil, err
 		}
-		ic.byName[name] = im
+		return s.sequence[index], nil
 	}
-	ic.list = append(ic.list, im)
-	return im, nil
+	if im, ok := s.byName[strings.ToLower(indexOrName)]; ok {
+		return im, nil
+	}
+	return nil, nil
 }
 
-func (ic *imageContexts) update(imageID string, runConfig *container.Config) {
-	ic.list[len(ic.list)-1].id = imageID
-	ic.list[len(ic.list)-1].runConfig = runConfig
+func (s *buildStages) validateIndex(i int) error {
+	if i < 0 || i >= len(s.sequence)-1 {
+		if i == len(s.sequence)-1 {
+			return errors.New("refers to current build stage")
+		}
+		return errors.New("index out of bounds")
+	}
+	return nil
 }
 
-func (ic *imageContexts) validate(i int) error {
-	if i < 0 || i >= len(ic.list)-1 {
-		var extraMsg string
-		if i == len(ic.list)-1 {
-			extraMsg = " refers current build block"
+func (s *buildStages) add(name string, image builder.Image) error {
+	stage := newBuildStageFromImage(image)
+	name = strings.ToLower(name)
+	if len(name) > 0 {
+		if _, ok := s.byName[name]; ok {
+			return errors.Errorf("duplicate name %s", name)
 		}
-		return errors.Errorf("invalid from flag value %d%s", i, extraMsg)
+		s.byName[name] = stage
 	}
+	s.sequence = append(s.sequence, stage)
 	return nil
 }
 
-func (ic *imageContexts) get(indexOrName string) (*imageMount, error) {
-	index, err := strconv.Atoi(indexOrName)
-	if err == nil {
-		if err := ic.validate(index); err != nil {
-			return nil, err
-		}
-		return ic.list[index], nil
+func (s *buildStages) update(imageID string, runConfig *container.Config) {
+	s.sequence[len(s.sequence)-1].update(imageID, runConfig)
+}
+
+type getAndMountFunc func(string) (builder.Image, builder.ReleaseableLayer, error)
+
+// imageSources mounts images and provides a cache for mounted images. It tracks
+// all images so they can be unmounted at the end of the build.
+type imageSources struct {
+	byImageID map[string]*imageMount
+	getImage  getAndMountFunc
+	cache     pathCache // TODO: remove
+}
+
+func newImageSources(ctx context.Context, options builderOptions) *imageSources {
+	getAndMount := func(idOrRef string) (builder.Image, builder.ReleaseableLayer, error) {
+		return options.Backend.GetImageAndReleasableLayer(ctx, idOrRef, backend.GetImageAndLayerOptions{
+			ForcePull:  options.Options.PullParent,
+			AuthConfig: options.Options.AuthConfigs,
+			Output:     options.ProgressWriter.Output,
+		})
 	}
-	if im, ok := ic.byName[strings.ToLower(indexOrName)]; ok {
+
+	return &imageSources{
+		byImageID: make(map[string]*imageMount),
+		getImage:  getAndMount,
+	}
+}
+
+func (m *imageSources) Get(idOrRef string) (*imageMount, error) {
+	if im, ok := m.byImageID[idOrRef]; ok {
 		return im, nil
 	}
-	im, err := mountByRef(ic.b, indexOrName)
+
+	image, layer, err := m.getImage(idOrRef)
 	if err != nil {
-		return nil, errors.Wrapf(err, "invalid from flag value %s", indexOrName)
+		return nil, err
 	}
-	ic.implicitMounts = append(ic.implicitMounts, im)
+	im := newImageMount(image, layer)
+	m.byImageID[image.ImageID()] = im
 	return im, nil
 }
 
-func (ic *imageContexts) unmount() (retErr error) {
-	for _, iml := range append([][]*imageMount{}, ic.list, ic.implicitMounts) {
-		for _, im := range iml {
-			if err := im.unmount(); err != nil {
-				logrus.Error(err)
-				retErr = err
-			}
-		}
-	}
-	for _, im := range ic.byName {
+func (m *imageSources) Unmount() (retErr error) {
+	for _, im := range m.byImageID {
 		if err := im.unmount(); err != nil {
 			logrus.Error(err)
 			retErr = err
@@ -98,64 +148,63 @@ func (ic *imageContexts) unmount() (retErr error) {
 	return
 }
 
-func (ic *imageContexts) getCache(id, path string) (interface{}, bool) {
-	if ic.cache != nil {
+// TODO: remove getCache/setCache from imageSources
+func (m *imageSources) getCache(id, path string) (interface{}, bool) {
+	if m.cache != nil {
 		if id == "" {
 			return nil, false
 		}
-		return ic.cache.Load(id + path)
+		return m.cache.Load(id + path)
 	}
 	return nil, false
 }
 
-func (ic *imageContexts) setCache(id, path string, v interface{}) {
-	if ic.cache != nil {
-		ic.cache.Store(id+path, v)
+func (m *imageSources) setCache(id, path string, v interface{}) {
+	if m.cache != nil {
+		m.cache.Store(id+path, v)
 	}
 }
 
-// imageMount is a reference for getting access to a buildcontext that is backed
-// by an existing image
+// imageMount is a reference to an image that can be used as a builder.Source
 type imageMount struct {
-	id        string
-	source    builder.Source
-	release   func() error
-	ic        *imageContexts
-	runConfig *container.Config
+	image  builder.Image
+	source builder.Source
+	layer  builder.ReleaseableLayer
 }
 
-func (im *imageMount) context() (builder.Source, error) {
+func newImageMount(image builder.Image, layer builder.ReleaseableLayer) *imageMount {
+	im := &imageMount{image: image, layer: layer}
+	return im
+}
+
+func (im *imageMount) Source() (builder.Source, error) {
 	if im.source == nil {
-		if im.id == "" {
-			return nil, errors.Errorf("could not copy from empty context")
+		if im.layer == nil {
+			return nil, errors.Errorf("empty context")
 		}
-		p, release, err := im.ic.b.docker.MountImage(im.id)
+		mountPath, err := im.layer.Mount()
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to mount %s", im.id)
+			return nil, errors.Wrapf(err, "failed to mount %s", im.image.ImageID())
 		}
-		source, err := remotecontext.NewLazyContext(p)
+		source, err := remotecontext.NewLazyContext(mountPath)
 		if err != nil {
-			return nil, errors.Wrapf(err, "failed to create lazycontext for %s", p)
+			return nil, errors.Wrapf(err, "failed to create lazycontext for %s", mountPath)
 		}
-		im.release = release
 		im.source = source
 	}
 	return im.source, nil
 }
 
 func (im *imageMount) unmount() error {
-	if im.release != nil {
-		if err := im.release(); err != nil {
-			return errors.Wrapf(err, "failed to unmount previous build image %s", im.id)
-		}
-		im.release = nil
+	if im.layer == nil {
+		return nil
+	}
+	if err := im.layer.Release(); err != nil {
+		return errors.Wrapf(err, "failed to unmount previous build image %s", im.image.ImageID())
 	}
 	return nil
 }
 
-func (im *imageMount) ImageID() string {
-	return im.id
-}
-func (im *imageMount) RunConfig() *container.Config {
-	return im.runConfig
+func (im *imageMount) Image() builder.Image {
+	return im.image
 }

builder/dockerfile/internals.go

@@ -22,7 +22,6 @@ import (
 	"github.com/docker/docker/api/types/backend"
 	"github.com/docker/docker/api/types/container"
 	"github.com/docker/docker/builder"
-	"github.com/docker/docker/builder/dockerfile/parser"
 	"github.com/docker/docker/builder/remotecontext"
 	"github.com/docker/docker/pkg/httputils"
 	"github.com/docker/docker/pkg/ioutils"
@@ -79,7 +78,7 @@ func (b *Builder) commitContainer(dispatchState *dispatchState, id string, conta
 	}
 
 	dispatchState.imageID = imageID
-	b.imageContexts.update(imageID, dispatchState.runConfig)
+	b.buildStages.update(imageID, dispatchState.runConfig)
 	return nil
 }
 
@@ -370,9 +369,9 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression
 	source := b.source
 	var err error
 	if imageSource != nil {
-		source, err = imageSource.context()
+		source, err = imageSource.Source()
 		if err != nil {
-			return nil, err
+			return nil, errors.Wrapf(err, "failed to copy")
 		}
 	}
 
@@ -428,7 +427,7 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression
 
 	if imageSource != nil {
 		// fast-cache based on imageID
-		if h, ok := b.imageContexts.getCache(imageSource.id, origPath); ok {
+		if h, ok := b.imageSources.getCache(imageSource.Image().ImageID(), origPath); ok {
 			copyInfos[0].hash = h.(string)
 			return copyInfos, nil
 		}
@@ -474,80 +473,12 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression
 	hasher.Write([]byte(strings.Join(subfiles, ",")))
 	copyInfos[0].hash = "dir:" + hex.EncodeToString(hasher.Sum(nil))
 	if imageSource != nil {
-		b.imageContexts.setCache(imageSource.id, origPath, copyInfos[0].hash)
+		b.imageSources.setCache(imageSource.Image().ImageID(), origPath, copyInfos[0].hash)
 	}
 
 	return copyInfos, nil
 }
 
-func (b *Builder) processImageFrom(dispatchState *dispatchState, img builder.Image) error {
-	if img != nil {
-		dispatchState.imageID = img.ImageID()
-
-		if img.RunConfig() != nil {
-			dispatchState.runConfig = img.RunConfig()
-		}
-	}
-
-	// Check to see if we have a default PATH, note that windows won't
-	// have one as it's set by HCS
-	if system.DefaultPathEnv != "" {
-		if _, ok := dispatchState.runConfigEnvMapping()["PATH"]; !ok {
-			dispatchState.runConfig.Env = append(dispatchState.runConfig.Env,
-				"PATH="+system.DefaultPathEnv)
-		}
-	}
-
-	if img == nil {
-		// Typically this means they used "FROM scratch"
-		return nil
-	}
-
-	// Process ONBUILD triggers if they exist
-	if nTriggers := len(dispatchState.runConfig.OnBuild); nTriggers != 0 {
-		word := "trigger"
-		if nTriggers > 1 {
-			word = "triggers"
-		}
-		fmt.Fprintf(b.Stderr, "# Executing %d build %s...\n", nTriggers, word)
-	}
-
-	// Copy the ONBUILD triggers, and remove them from the config, since the config will be committed.
-	onBuildTriggers := dispatchState.runConfig.OnBuild
-	dispatchState.runConfig.OnBuild = []string{}
-
-	// Reset stdin settings as all build actions run without stdin
-	dispatchState.runConfig.OpenStdin = false
-	dispatchState.runConfig.StdinOnce = false
-
-	// parse the ONBUILD triggers by invoking the parser
-	for _, step := range onBuildTriggers {
-		dockerfile, err := parser.Parse(strings.NewReader(step))
-		if err != nil {
-			return err
-		}
-
-		for _, n := range dockerfile.AST.Children {
-			if err := checkDispatch(n); err != nil {
-				return err
-			}
-
-			upperCasedCmd := strings.ToUpper(n.Value)
-			switch upperCasedCmd {
-			case "ONBUILD":
-				return errors.New("Chaining ONBUILD via `ONBUILD ONBUILD` isn't allowed")
-			case "MAINTAINER", "FROM":
-				return errors.Errorf("%s isn't allowed as an ONBUILD trigger", upperCasedCmd)
-			}
-		}
-
-		if _, err := dispatchFromDockerfile(b, dockerfile, dispatchState); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
 // probeCache checks if cache match can be found for current build instruction.
 // If an image is found, probeCache returns `(true, nil)`.
 // If no image is found, it returns `(false, nil)`.
@@ -570,7 +501,7 @@ func (b *Builder) probeCache(dispatchState *dispatchState, runConfig *container.
 	fmt.Fprint(b.Stdout, " ---> Using cache\n")
 	logrus.Debugf("[BUILDER] Use cached version: %s", runConfig.Cmd)
 	dispatchState.imageID = string(cache)
-	b.imageContexts.update(dispatchState.imageID, runConfig)
+	b.buildStages.update(dispatchState.imageID, runConfig)
 
 	return true, nil
 }

builder/dockerfile/mockbackend_test.go

@@ -15,30 +15,15 @@ import (
 
 // MockBackend implements the builder.Backend interface for unit testing
 type MockBackend struct {
-	getImageOnBuildFunc  func(string) (builder.Image, error)
-	getImageOnBuildImage *mockImage
-	containerCreateFunc  func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error)
-	commitFunc           func(string, *backend.ContainerCommitConfig) (string, error)
-}
-
-func (m *MockBackend) GetImageOnBuild(name string) (builder.Image, error) {
-	if m.getImageOnBuildFunc != nil {
-		return m.getImageOnBuildFunc(name)
-	}
-	if m.getImageOnBuildImage != nil {
-		return m.getImageOnBuildImage, nil
-	}
-	return &mockImage{id: "theid"}, nil
+	containerCreateFunc func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error)
+	commitFunc          func(string, *backend.ContainerCommitConfig) (string, error)
+	getImageFunc        func(string) (builder.Image, builder.ReleaseableLayer, error)
 }
 
 func (m *MockBackend) TagImageWithReference(image.ID, reference.Named) error {
 	return nil
 }
 
-func (m *MockBackend) PullOnBuild(ctx context.Context, name string, authConfigs map[string]types.AuthConfig, output io.Writer) (builder.Image, error) {
-	return nil, nil
-}
-
 func (m *MockBackend) ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout, stderr io.Writer, stream bool, attached chan struct{}) error {
 	return nil
 }
@@ -81,16 +66,12 @@ func (m *MockBackend) CopyOnBuild(containerID string, destPath string, srcRoot s
 	return nil
 }
 
-func (m *MockBackend) HasExperimental() bool {
-	return false
-}
-
-func (m *MockBackend) SquashImage(from string, to string) (string, error) {
-	return "", nil
-}
+func (m *MockBackend) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) {
+	if m.getImageFunc != nil {
+		return m.getImageFunc(refOrID)
+	}
 
-func (m *MockBackend) MountImage(name string) (string, func() error, error) {
-	return "", func() error { return nil }, nil
+	return &mockImage{id: "theid"}, &mockLayer{}, nil
 }
 
 type mockImage struct {
@@ -116,3 +97,13 @@ func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config) (str
 	}
 	return "", nil
 }
+
+type mockLayer struct{}
+
+func (l *mockLayer) Release() error {
+	return nil
+}
+
+func (l *mockLayer) Mount() (string, error) {
+	return "mountPath", nil
+}

daemon/archive.go

@@ -8,12 +8,10 @@ import (
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/container"
-	"github.com/docker/docker/layer"
 	"github.com/docker/docker/pkg/archive"
 	"github.com/docker/docker/pkg/chrootarchive"
 	"github.com/docker/docker/pkg/idtools"
 	"github.com/docker/docker/pkg/ioutils"
-	"github.com/docker/docker/pkg/stringid"
 	"github.com/docker/docker/pkg/symlink"
 	"github.com/docker/docker/pkg/system"
 	"github.com/pkg/errors"
@@ -470,34 +468,3 @@ func (daemon *Daemon) CopyOnBuild(cID, destPath, srcRoot, srcPath string, decomp
 
 	return fixPermissions(fullSrcPath, destPath, rootUID, rootGID, destExists)
 }
-
-// MountImage returns mounted path with rootfs of an image.
-func (daemon *Daemon) MountImage(name string) (string, func() error, error) {
-	img, err := daemon.GetImage(name)
-	if err != nil {
-		return "", nil, errors.Wrapf(err, "no such image: %s", name)
-	}
-
-	mountID := stringid.GenerateRandomID()
-	rwLayer, err := daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil)
-	if err != nil {
-		return "", nil, errors.Wrap(err, "failed to create rwlayer")
-	}
-
-	mountPath, err := rwLayer.Mount("")
-	if err != nil {
-		metadata, releaseErr := daemon.layerStore.ReleaseRWLayer(rwLayer)
-		if releaseErr != nil {
-			err = errors.Wrapf(err, "failed to release rwlayer: %s", releaseErr.Error())
-		}
-		layer.LogReleaseMetadata(metadata)
-		return "", nil, errors.Wrap(err, "failed to mount rwlayer")
-	}
-
-	return mountPath, func() error {
-		rwLayer.Unmount()
-		metadata, err := daemon.layerStore.ReleaseRWLayer(rwLayer)
-		layer.LogReleaseMetadata(metadata)
-		return err
-	}, nil
-}

daemon/build.go

@@ -0,0 +1,122 @@
+package daemon
+
+import (
+	"github.com/Sirupsen/logrus"
+	"github.com/docker/distribution/reference"
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/backend"
+	"github.com/docker/docker/builder"
+	"github.com/docker/docker/image"
+	"github.com/docker/docker/layer"
+	"github.com/docker/docker/pkg/stringid"
+	"github.com/docker/docker/registry"
+	"github.com/pkg/errors"
+	"golang.org/x/net/context"
+	"io"
+)
+
+type releaseableLayer struct {
+	layerStore layer.Store
+	roLayer    layer.Layer
+	rwLayer    layer.RWLayer
+}
+
+func (rl *releaseableLayer) Mount() (string, error) {
+	if rl.roLayer == nil {
+		return "", errors.New("can not mount an image with no root FS")
+	}
+	var err error
+	mountID := stringid.GenerateRandomID()
+	rl.rwLayer, err = rl.layerStore.CreateRWLayer(mountID, rl.roLayer.ChainID(), nil)
+	if err != nil {
+		return "", errors.Wrap(err, "failed to create rwlayer")
+	}
+
+	return rl.rwLayer.Mount("")
+}
+
+func (rl *releaseableLayer) Release() error {
+	rl.releaseRWLayer()
+	return rl.releaseROLayer()
+}
+
+func (rl *releaseableLayer) releaseRWLayer() error {
+	if rl.rwLayer == nil {
+		return nil
+	}
+	metadata, err := rl.layerStore.ReleaseRWLayer(rl.rwLayer)
+	layer.LogReleaseMetadata(metadata)
+	if err != nil {
+		logrus.Errorf("Failed to release RWLayer: %s", err)
+	}
+	return err
+}
+
+func (rl *releaseableLayer) releaseROLayer() error {
+	if rl.roLayer == nil {
+		return nil
+	}
+	metadata, err := rl.layerStore.Release(rl.roLayer)
+	layer.LogReleaseMetadata(metadata)
+	return err
+}
+
+func newReleasableLayerForImage(img *image.Image, layerStore layer.Store) (builder.ReleaseableLayer, error) {
+	if img.RootFS.ChainID() == "" {
+		return nil, nil
+	}
+	// Hold a reference to the image layer so that it can't be removed before
+	// it is released
+	roLayer, err := layerStore.Get(img.RootFS.ChainID())
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to get layer for image %s", img.ImageID())
+	}
+	return &releaseableLayer{layerStore: layerStore, roLayer: roLayer}, nil
+}
+
+// TODO: could this use the regular daemon PullImage ?
+func (daemon *Daemon) pullForBuilder(ctx context.Context, name string, authConfigs map[string]types.AuthConfig, output io.Writer) (*image.Image, error) {
+	ref, err := reference.ParseNormalizedNamed(name)
+	if err != nil {
+		return nil, err
+	}
+	ref = reference.TagNameOnly(ref)
+
+	pullRegistryAuth := &types.AuthConfig{}
+	if len(authConfigs) > 0 {
+		// The request came with a full auth config, use it
+		repoInfo, err := daemon.RegistryService.ResolveRepository(ref)
+		if err != nil {
+			return nil, err
+		}
+
+		resolvedConfig := registry.ResolveAuthConfig(authConfigs, repoInfo.Index)
+		pullRegistryAuth = &resolvedConfig
+	}
+
+	if err := daemon.pullImageWithReference(ctx, ref, nil, pullRegistryAuth, output); err != nil {
+		return nil, err
+	}
+	return daemon.GetImage(name)
+}
+
+// GetImageAndReleasableLayer returns an image and releaseable layer for a reference or ID.
+// Every call to GetImageAndReleasableLayer MUST call releasableLayer.Release() to prevent
+// leaking of layers.
+func (daemon *Daemon) GetImageAndReleasableLayer(ctx context.Context, refOrID string, opts backend.GetImageAndLayerOptions) (builder.Image, builder.ReleaseableLayer, error) {
+	if !opts.ForcePull {
+		image, _ := daemon.GetImage(refOrID)
+		// TODO: shouldn't we error out if error is different from "not found" ?
+		if image != nil {
+			layer, err := newReleasableLayerForImage(image, daemon.layerStore)
+			return image, layer, err
+		}
+	}
+
+	image, err := daemon.pullForBuilder(ctx, refOrID, opts.AuthConfig, opts.Output)
+	if err != nil {
+		return nil, nil, err
+	}
+	layer, err := newReleasableLayerForImage(image, daemon.layerStore)
+	return image, layer, err
+}

daemon/image.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 
 	"github.com/docker/distribution/reference"
-	"github.com/docker/docker/builder"
 	"github.com/docker/docker/image"
 	"github.com/docker/docker/pkg/stringid"
 )
@@ -75,12 +74,3 @@ func (daemon *Daemon) GetImage(refOrID string) (*image.Image, error) {
 	}
 	return daemon.imageStore.Get(imgID)
 }
-
-// GetImageOnBuild looks up a Docker image referenced by `name`.
-func (daemon *Daemon) GetImageOnBuild(name string) (builder.Image, error) {
-	img, err := daemon.GetImage(name)
-	if err != nil {
-		return nil, err
-	}
-	return img, nil
-}

daemon/image_pull.go

@@ -7,7 +7,6 @@ import (
 	dist "github.com/docker/distribution"
 	"github.com/docker/distribution/reference"
 	"github.com/docker/docker/api/types"
-	"github.com/docker/docker/builder"
 	"github.com/docker/docker/distribution"
 	progressutils "github.com/docker/docker/distribution/utils"
 	"github.com/docker/docker/pkg/progress"
@@ -46,35 +45,6 @@ func (daemon *Daemon) PullImage(ctx context.Context, image, tag string, metaHead
 	return daemon.pullImageWithReference(ctx, ref, metaHeaders, authConfig, outStream)
 }
 
-// PullOnBuild tells Docker to pull image referenced by `name`.
-func (daemon *Daemon) PullOnBuild(ctx context.Context, name string, authConfigs map[string]types.AuthConfig, output io.Writer) (builder.Image, error) {
-	ref, err := reference.ParseNormalizedNamed(name)
-	if err != nil {
-		return nil, err
-	}
-	ref = reference.TagNameOnly(ref)
-
-	pullRegistryAuth := &types.AuthConfig{}
-	if len(authConfigs) > 0 {
-		// The request came with a full auth config file, we prefer to use that
-		repoInfo, err := daemon.RegistryService.ResolveRepository(ref)
-		if err != nil {
-			return nil, err
-		}
-
-		resolvedConfig := registry.ResolveAuthConfig(
-			authConfigs,
-			repoInfo.Index,
-		)
-		pullRegistryAuth = &resolvedConfig
-	}
-
-	if err := daemon.pullImageWithReference(ctx, ref, nil, pullRegistryAuth, output); err != nil {
-		return nil, err
-	}
-	return daemon.GetImage(name)
-}
-
 func (daemon *Daemon) pullImageWithReference(ctx context.Context, ref reference.Named, metaHeaders map[string][]string, authConfig *types.AuthConfig, outStream io.Writer) error {
 	// Include a buffer so that slow client connections don't affect
 	// transfer performance.

integration-cli/docker_cli_build_test.go

@@ -5946,7 +5946,7 @@ func (s *DockerSuite) TestBuildCopyFromPreviousRootFSErrors(c *check.C) {
 			dockerfile: `
 		FROM busybox
 		COPY --from=0 foo bar`,
-			expectedError: "invalid from flag value 0 refers current build block",
+			expectedError: "invalid from flag value 0: refers to current build stage",
 		},
 		{
 			dockerfile: `