From 01b4a2279822e6bbaaf3fef2753ef2f6d273a53a Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 5 May 2017 13:05:25 -0400 Subject: [PATCH] Refactor interaction between dispatcher.from and dispatchState Signed-off-by: Daniel Nephin Upstream-commit: ab3a037a5b77220d0524ce2b17105e1daae39425 Component: engine --- components/engine/builder/builder.go | 2 +- .../engine/builder/dockerfile/dispatchers.go | 73 +++++++++++++++---- .../builder/dockerfile/dispatchers_test.go | 4 +- .../engine/builder/dockerfile/evaluator.go | 53 +++++++++----- .../engine/builder/dockerfile/imagecontext.go | 9 ++- .../engine/builder/dockerfile/internals.go | 69 ------------------ .../builder/dockerfile/mockbackend_test.go | 2 +- components/engine/daemon/build.go | 26 +++---- .../integration-cli/docker_cli_build_test.go | 2 +- 9 files changed, 117 insertions(+), 123 deletions(-) diff --git a/components/engine/builder/builder.go b/components/engine/builder/builder.go index 359eb89b24..f990635933 100644 --- a/components/engine/builder/builder.go +++ b/components/engine/builder/builder.go @@ -89,5 +89,5 @@ type Image interface { // ReleaseableLayer is an image layer that can be mounted and released type ReleaseableLayer interface { Release() error - Mount() (string, error) + Mount(string) (string, error) } diff --git a/components/engine/builder/dockerfile/dispatchers.go b/components/engine/builder/dockerfile/dispatchers.go index da8b181637..83c2a6f247 100644 --- a/components/engine/builder/dockerfile/dispatchers.go +++ b/components/engine/builder/dockerfile/dispatchers.go @@ -23,6 +23,7 @@ import ( "github.com/docker/docker/api/types/backend" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/strslice" + "github.com/docker/docker/builder/dockerfile/parser" "github.com/docker/docker/pkg/signal" "github.com/docker/go-connections/nat" "github.com/pkg/errors" @@ -196,24 +197,21 @@ func from(req dispatchRequest) error { } req.builder.resetImageCache() - req.state.noBaseImage = false - req.state.stageName = stageName image, err := req.builder.getFromImage(req.shlex, req.args[0]) if err != nil { return err } - if image == nil { - req.state.imageID = "" - req.state.noBaseImage = true - image = newImageMount(nil, nil) - } if err := req.builder.imageContexts.add(stageName, image); err != nil { return err } - req.state.baseImage = image - + req.state.beginStage(stageName, image) req.builder.buildArgs.ResetAllowed() - return req.builder.processImageFrom(req.state, image) + if image.ImageID() == "" { + // Typically this means they used "FROM scratch" + return nil + } + + return processOnBuild(req) } func parseBuildStageName(args []string) (string, error) { @@ -243,11 +241,7 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error } 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 + return im, nil } // Windows cannot support a container with no base image. @@ -255,7 +249,7 @@ func (b *Builder) getFromImage(shlex *ShellLex, name string) (*imageMount, error if runtime.GOOS == "windows" { return nil, errors.New("Windows does not support FROM scratch") } - return nil, nil + return newImageMount(nil, nil), nil } return b.getImage(name) } @@ -272,6 +266,53 @@ func (b *Builder) getImage(name string) (*imageMount, error) { return newImageMount(image, layer), 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 // // ONBUILD triggers run when the image is used in a FROM statement. diff --git a/components/engine/builder/dockerfile/dispatchers_test.go b/components/engine/builder/dockerfile/dispatchers_test.go index 5b8a23bb6c..507f72bdcf 100644 --- a/components/engine/builder/dockerfile/dispatchers_test.go +++ b/components/engine/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" @@ -192,8 +193,9 @@ 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) { diff --git a/components/engine/builder/dockerfile/evaluator.go b/components/engine/builder/dockerfile/evaluator.go index 8402af9087..6fc2c6a6cd 100644 --- a/components/engine/builder/dockerfile/evaluator.go +++ b/components/engine/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 ` 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) { diff --git a/components/engine/builder/dockerfile/imagecontext.go b/components/engine/builder/dockerfile/imagecontext.go index 571a20116e..e0a25084ee 100644 --- a/components/engine/builder/dockerfile/imagecontext.go +++ b/components/engine/builder/dockerfile/imagecontext.go @@ -45,9 +45,9 @@ func (ic *imageContexts) update(imageID string, runConfig *container.Config) { func (ic *imageContexts) validate(i int) error { if i < 0 || i >= len(ic.list)-1 { if i == len(ic.list)-1 { - return errors.Errorf("%d refers to current build stage", i) + return errors.New("refers to current build stage") } - return errors.Errorf("index out of bounds") + return errors.New("index out of bounds") } return nil } @@ -122,7 +122,7 @@ func (im *imageMount) context() (builder.Source, error) { if im.id == "" || im.layer == nil { return nil, errors.Errorf("empty context") } - mountPath, err := im.layer.Mount() + mountPath, err := im.layer.Mount(im.id) if err != nil { return nil, errors.Wrapf(err, "failed to mount %s", im.id) } @@ -136,6 +136,9 @@ func (im *imageMount) context() (builder.Source, error) { } func (im *imageMount) unmount() error { + 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.id) } diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index b70a61b8e6..5affd1d5f9 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/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" @@ -480,74 +479,6 @@ func (b *Builder) calcCopyInfo(cmdName, origPath string, allowLocalDecompression 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)`. diff --git a/components/engine/builder/dockerfile/mockbackend_test.go b/components/engine/builder/dockerfile/mockbackend_test.go index a874dfb550..4f4a59d545 100644 --- a/components/engine/builder/dockerfile/mockbackend_test.go +++ b/components/engine/builder/dockerfile/mockbackend_test.go @@ -112,6 +112,6 @@ func (l *mockLayer) Release() error { return nil } -func (l *mockLayer) Mount() (string, error) { +func (l *mockLayer) Mount(_ string) (string, error) { return "mountPath", nil } diff --git a/components/engine/daemon/build.go b/components/engine/daemon/build.go index 44640efee5..806f05c392 100644 --- a/components/engine/daemon/build.go +++ b/components/engine/daemon/build.go @@ -17,7 +17,7 @@ import ( type releaseableLayer struct { rwLayer layer.RWLayer release func(layer.RWLayer) error - mount func() (layer.RWLayer, error) + mount func(string) (layer.RWLayer, error) } func (rl *releaseableLayer) Release() error { @@ -28,10 +28,9 @@ func (rl *releaseableLayer) Release() error { return rl.release(rl.rwLayer) } -func (rl *releaseableLayer) Mount() (string, error) { +func (rl *releaseableLayer) Mount(imageID string) (string, error) { var err error - // daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil) - rl.rwLayer, err = rl.mount() + rl.rwLayer, err = rl.mount(imageID) if err != nil { return "", errors.Wrap(err, "failed to create rwlayer") } @@ -47,8 +46,12 @@ func (rl *releaseableLayer) Mount() (string, error) { return mountPath, err } -func (daemon *Daemon) getReleasableLayerForImage(img *image.Image) (*releaseableLayer, error) { - mountFunc := func() (layer.RWLayer, error) { +func (daemon *Daemon) getReleasableLayerForImage() *releaseableLayer { + mountFunc := func(imageID string) (layer.RWLayer, error) { + img, err := daemon.GetImage(imageID) + if err != nil { + return nil, err + } mountID := stringid.GenerateRandomID() return daemon.layerStore.CreateRWLayer(mountID, img.RootFS.ChainID(), nil) } @@ -59,7 +62,7 @@ func (daemon *Daemon) getReleasableLayerForImage(img *image.Image) (*releaseable return err } - return &releaseableLayer{mount: mountFunc, release: releaseFunc}, nil + return &releaseableLayer{mount: mountFunc, release: releaseFunc} } // TODO: could this use the regular daemon PullImage ? @@ -94,15 +97,10 @@ func (daemon *Daemon) GetImageAndLayer(ctx context.Context, refOrID string, opts image, _ := daemon.GetImage(refOrID) // TODO: shouldn't we error out if error is different from "not found" ? if image != nil { - layer, err := daemon.getReleasableLayerForImage(image) - return image, layer, err + return image, daemon.getReleasableLayerForImage(), nil } } image, err := daemon.pullForBuilder(ctx, refOrID, opts.AuthConfig, opts.Output) - if err != nil { - return nil, nil, err - } - layer, err := daemon.getReleasableLayerForImage(image) - return image, layer, err + return image, daemon.getReleasableLayerForImage(), err } diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go index fff95d66ef..99d6e785d7 100644 --- a/components/engine/integration-cli/docker_cli_build_test.go +++ b/components/engine/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: `