diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 84383d1ee8..05cb6ab6e7 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -73,7 +73,7 @@ func env(req dispatchRequest) error { } } - return req.builder.commit("", req.runConfig.Cmd, commitMessage.String()) + return req.builder.commit(commitMessage.String()) } // MAINTAINER some text @@ -90,7 +90,7 @@ func maintainer(req dispatchRequest) error { maintainer := req.args[0] req.builder.maintainer = maintainer - return req.builder.commit("", req.runConfig.Cmd, "MAINTAINER "+maintainer) + return req.builder.commit("MAINTAINER " + maintainer) } // LABEL some json data describing the image @@ -130,7 +130,7 @@ func label(req dispatchRequest) error { req.runConfig.Labels[req.args[j]] = req.args[j+1] j++ } - return req.builder.commit("", req.runConfig.Cmd, commitStr) + return req.builder.commit(commitStr) } // ADD foo /path @@ -281,7 +281,7 @@ func onbuild(req dispatchRequest) error { original := regexp.MustCompile(`(?i)^\s*ONBUILD\s*`).ReplaceAllString(req.original, "") req.runConfig.OnBuild = append(req.runConfig.OnBuild, original) - return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("ONBUILD %s", original)) + return req.builder.commit("ONBUILD " + original) } // WORKDIR /tmp @@ -321,10 +321,9 @@ func workdir(req dispatchRequest) error { req.runConfig.Cmd = strslice.StrSlice(append(getShell(req.runConfig), "#(nop) "+comment)) defer func(cmd strslice.StrSlice) { req.runConfig.Cmd = cmd }(cmd) - if hit, err := req.builder.probeCache(); err != nil { + // TODO: this should pass a copy of runConfig + if hit, err := req.builder.probeCache(req.builder.image, req.runConfig); err != nil || hit { return err - } else if hit { - return nil } req.runConfig.Image = req.builder.image @@ -341,7 +340,7 @@ func workdir(req dispatchRequest) error { return err } - return req.builder.commit(container.ID, cmd, comment) + return req.builder.commitContainer(container.ID, copyRunConfig(req.runConfig, withCmd(cmd))) } // RUN some command yo @@ -402,13 +401,10 @@ func run(req dispatchRequest) error { } req.runConfig.Cmd = saveCmd - hit, err := req.builder.probeCache() - if err != nil { + hit, err := req.builder.probeCache(req.builder.image, req.runConfig) + if err != nil || hit { return err } - if hit { - return nil - } // set Cmd manually, this is special case only for Dockerfiles req.runConfig.Cmd = config.Cmd @@ -419,7 +415,11 @@ func run(req dispatchRequest) error { logrus.Debugf("[BUILDER] Command to be executed: %v", req.runConfig.Cmd) - cID, err := req.builder.create() + // TODO: this was previously in b.create(), why is it necessary? + req.builder.runConfig.Image = req.builder.image + + // TODO: should pass a copy of runConfig + cID, err := req.builder.create(req.runConfig) if err != nil { return err } @@ -451,7 +451,7 @@ func run(req dispatchRequest) error { saveCmd = strslice.StrSlice(append(tmpEnv, saveCmd...)) } req.runConfig.Cmd = saveCmd - return req.builder.commit(cID, cmd, "run") + return req.builder.commitContainer(cID, copyRunConfig(req.runConfig, withCmd(cmd))) } // CMD foo @@ -474,7 +474,7 @@ func cmd(req dispatchRequest) error { // set config as already being escaped, this prevents double escaping on windows req.runConfig.ArgsEscaped = true - if err := req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("CMD %q", cmdSlice)); err != nil { + if err := req.builder.commit(fmt.Sprintf("CMD %q", cmdSlice)); err != nil { return err } @@ -590,7 +590,7 @@ func healthcheck(req dispatchRequest) error { req.runConfig.Healthcheck = &healthcheck } - return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("HEALTHCHECK %q", req.runConfig.Healthcheck)) + return req.builder.commit(fmt.Sprintf("HEALTHCHECK %q", req.runConfig.Healthcheck)) } // ENTRYPOINT /usr/sbin/nginx @@ -626,11 +626,7 @@ func entrypoint(req dispatchRequest) error { req.runConfig.Cmd = nil } - if err := req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("ENTRYPOINT %q", req.runConfig.Entrypoint)); err != nil { - return err - } - - return nil + return req.builder.commit(fmt.Sprintf("ENTRYPOINT %q", req.runConfig.Entrypoint)) } // EXPOSE 6667/tcp 7000/tcp @@ -671,7 +667,7 @@ func expose(req dispatchRequest) error { i++ } sort.Strings(portList) - return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("EXPOSE %s", strings.Join(portList, " "))) + return req.builder.commit("EXPOSE " + strings.Join(portList, " ")) } // USER foo @@ -689,7 +685,7 @@ func user(req dispatchRequest) error { } req.runConfig.User = req.args[0] - return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("USER %v", req.args)) + return req.builder.commit(fmt.Sprintf("USER %v", req.args)) } // VOLUME /foo @@ -715,10 +711,7 @@ func volume(req dispatchRequest) error { } req.runConfig.Volumes[v] = struct{}{} } - if err := req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("VOLUME %v", req.args)); err != nil { - return err - } - return nil + return req.builder.commit(fmt.Sprintf("VOLUME %v", req.args)) } // STOPSIGNAL signal @@ -736,7 +729,7 @@ func stopSignal(req dispatchRequest) error { } req.runConfig.StopSignal = sig - return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("STOPSIGNAL %v", req.args)) + return req.builder.commit(fmt.Sprintf("STOPSIGNAL %v", req.args)) } // ARG name[=value] @@ -786,7 +779,7 @@ func arg(req dispatchRequest) error { req.builder.buildArgs.AddMetaArg(name, value) return nil } - return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("ARG %s", arg)) + return req.builder.commit("ARG " + arg) } // SHELL powershell -command @@ -808,7 +801,7 @@ func shell(req dispatchRequest) error { // SHELL powershell -command - not JSON return errNotJSON("SHELL", req.original) } - return req.builder.commit("", req.runConfig.Cmd, fmt.Sprintf("SHELL %v", shellSlice)) + return req.builder.commit(fmt.Sprintf("SHELL %v", shellSlice)) } func errAtLeastOneArgument(command string) error { @@ -831,15 +824,6 @@ func errTooManyArguments(command string) error { return fmt.Errorf("Bad input to %s, too many arguments", command) } -// getShell is a helper function which gets the right shell for prefixing the -// shell-form of RUN, ENTRYPOINT and CMD instructions -func getShell(c *container.Config) []string { - if 0 == len(c.Shell) { - return append([]string{}, defaultShell[:]...) - } - return append([]string{}, c.Shell[:]...) -} - // 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) diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index e6dc40d173..1cb2eeef16 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -36,41 +36,39 @@ import ( "github.com/pkg/errors" ) -func (b *Builder) commit(id string, autoCmd strslice.StrSlice, comment string) error { +func (b *Builder) commit(comment string) error { if b.disableCommit { return nil } if !b.hasFromImage() { return errors.New("Please provide a source image with `from` prior to commit") } + // TODO: why is this set here? b.runConfig.Image = b.image - if id == "" { - cmd := b.runConfig.Cmd - b.runConfig.Cmd = strslice.StrSlice(append(getShell(b.runConfig), "#(nop) ", comment)) - defer func(cmd strslice.StrSlice) { b.runConfig.Cmd = cmd }(cmd) - - hit, err := b.probeCache() - if err != nil { - return err - } else if hit { - return nil - } - id, err = b.create() - if err != nil { - return err - } + runConfigWithCommentCmd := copyRunConfig(b.runConfig, withCmdComment(comment)) + hit, err := b.probeCache(b.image, runConfigWithCommentCmd) + if err != nil || hit { + return err + } + id, err := b.create(runConfigWithCommentCmd) + if err != nil { + return err } - // Note: Actually copy the struct - autoConfig := *b.runConfig - autoConfig.Cmd = autoCmd + return b.commitContainer(id, b.runConfig) +} + +func (b *Builder) commitContainer(id string, runConfig *container.Config) error { + if b.disableCommit { + return nil + } commitCfg := &backend.ContainerCommitConfig{ ContainerCommitConfig: types.ContainerCommitConfig{ Author: b.maintainer, Pause: true, - Config: &autoConfig, + Config: runConfig, }, } @@ -80,8 +78,10 @@ func (b *Builder) commit(id string, autoCmd strslice.StrSlice, comment string) e return err } + // TODO: this function should return imageID and runConfig instead of setting + // then on the builder b.image = imageID - b.imageContexts.update(imageID, &autoConfig) + b.imageContexts.update(imageID, runConfig) return nil } @@ -148,11 +148,9 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD // For backwards compat, if there's just one info then use it as the // cache look-up string, otherwise hash 'em all into one var srcHash string - var origPaths string if len(infos) == 1 { info := infos[0] - origPaths = info.path srcHash = info.hash } else { var hashs []string @@ -164,17 +162,16 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD hasher := sha256.New() hasher.Write([]byte(strings.Join(hashs, ","))) srcHash = "multi:" + hex.EncodeToString(hasher.Sum(nil)) - origPaths = strings.Join(origs, " ") } cmd := b.runConfig.Cmd + // TODO: should this have been using origPaths instead of srcHash in the comment? b.runConfig.Cmd = strslice.StrSlice(append(getShell(b.runConfig), fmt.Sprintf("#(nop) %s %s in %s ", cmdName, srcHash, dest))) defer func(cmd strslice.StrSlice) { b.runConfig.Cmd = cmd }(cmd) - if hit, err := b.probeCache(); err != nil { + // TODO: this should pass a copy of runConfig + if hit, err := b.probeCache(b.image, b.runConfig); err != nil || hit { return err - } else if hit { - return nil } container, err := b.docker.ContainerCreate(types.ContainerCreateConfig{ @@ -187,8 +184,6 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD } b.tmpContainers[container.ID] = struct{}{} - comment := fmt.Sprintf("%s %s in %s", cmdName, origPaths, dest) - // Twiddle the destination when it's a relative path - meaning, make it // relative to the WORKINGDIR if dest, err = normaliseDest(cmdName, b.runConfig.WorkingDir, dest); err != nil { @@ -201,7 +196,44 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD } } - return b.commit(container.ID, cmd, comment) + return b.commitContainer(container.ID, copyRunConfig(b.runConfig, withCmd(cmd))) +} + +type runConfigModifier func(*container.Config) + +func copyRunConfig(runConfig *container.Config, modifiers ...runConfigModifier) *container.Config { + copy := *runConfig + for _, modifier := range modifiers { + modifier(©) + } + return © +} + +func withCmd(cmd []string) runConfigModifier { + return func(runConfig *container.Config) { + runConfig.Cmd = cmd + } +} + +func withCmdComment(comment string) runConfigModifier { + return func(runConfig *container.Config) { + runConfig.Cmd = append(getShell(runConfig), "#(nop) ", comment) + } +} + +func withEnv(env []string) runConfigModifier { + return func(runConfig *container.Config) { + runConfig.Env = env + } +} + +// getShell is a helper function which gets the right shell for prefixing the +// shell-form of RUN, ENTRYPOINT and CMD instructions +func getShell(c *container.Config) []string { + if 0 == len(c.Shell) { + return append([]string{}, defaultShell[:]...) + } + return append([]string{}, c.Shell[:]...) } func (b *Builder) download(srcURL string) (remote builder.Source, p string, err error) { @@ -498,35 +530,33 @@ func (b *Builder) processImageFrom(img builder.Image) error { // If an image is found, probeCache returns `(true, nil)`. // If no image is found, it returns `(false, nil)`. // If there is any error, it returns `(false, err)`. -func (b *Builder) probeCache() (bool, error) { +func (b *Builder) probeCache(imageID string, runConfig *container.Config) (bool, error) { c := b.imageCache if c == nil || b.options.NoCache || b.cacheBusted { return false, nil } - cache, err := c.GetCache(b.image, b.runConfig) + cache, err := c.GetCache(imageID, runConfig) if err != nil { return false, err } if len(cache) == 0 { - logrus.Debugf("[BUILDER] Cache miss: %s", b.runConfig.Cmd) + logrus.Debugf("[BUILDER] Cache miss: %s", runConfig.Cmd) b.cacheBusted = true return false, nil } fmt.Fprint(b.Stdout, " ---> Using cache\n") - logrus.Debugf("[BUILDER] Use cached version: %s", b.runConfig.Cmd) + logrus.Debugf("[BUILDER] Use cached version: %s", runConfig.Cmd) b.image = string(cache) - b.imageContexts.update(b.image, b.runConfig) + b.imageContexts.update(b.image, runConfig) return true, nil } -func (b *Builder) create() (string, error) { +func (b *Builder) create(runConfig *container.Config) (string, error) { if !b.hasFromImage() { return "", errors.New("Please provide a source image with `from` prior to run") } - b.runConfig.Image = b.image - resources := container.Resources{ CgroupParent: b.options.CgroupParent, CPUShares: b.options.CPUShares, @@ -551,11 +581,9 @@ func (b *Builder) create() (string, error) { ExtraHosts: b.options.ExtraHosts, } - config := *b.runConfig - // Create the container c, err := b.docker.ContainerCreate(types.ContainerCreateConfig{ - Config: b.runConfig, + Config: runConfig, HostConfig: hostConfig, }) if err != nil { @@ -569,7 +597,7 @@ func (b *Builder) create() (string, error) { fmt.Fprintf(b.Stdout, " ---> Running in %s\n", stringid.TruncateID(c.ID)) // override the entry point that may have been picked up from the base image - if err := b.docker.ContainerUpdateCmdOnBuild(c.ID, config.Cmd); err != nil { + if err := b.docker.ContainerUpdateCmdOnBuild(c.ID, runConfig.Cmd); err != nil { return "", err } diff --git a/builder/dockerfile/internals_test.go b/builder/dockerfile/internals_test.go index f5bee6da94..3119576788 100644 --- a/builder/dockerfile/internals_test.go +++ b/builder/dockerfile/internals_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/docker/docker/api/types/container" "github.com/docker/docker/builder" "github.com/docker/docker/builder/remotecontext" "github.com/docker/docker/pkg/archive" @@ -71,3 +72,54 @@ func readAndCheckDockerfile(t *testing.T, testName, contextDir, dockerfilePath, _, _, err = remotecontext.Detect(context.Background(), "", dockerfilePath, tarStream, nil) assert.EqualError(t, err, expectedError) } + +func TestCopyRunConfig(t *testing.T) { + defaultEnv := []string{"foo=1"} + defaultCmd := []string{"old"} + + var testcases = []struct { + doc string + modifiers []runConfigModifier + expected *container.Config + }{ + { + doc: "Set the command", + modifiers: []runConfigModifier{withCmd([]string{"new"})}, + expected: &container.Config{ + Cmd: []string{"new"}, + Env: defaultEnv, + }, + }, + { + doc: "Set the command to a comment", + modifiers: []runConfigModifier{withCmdComment("comment")}, + expected: &container.Config{ + Cmd: append(defaultShell, "#(nop) ", "comment"), + Env: defaultEnv, + }, + }, + { + doc: "Set the command and env", + modifiers: []runConfigModifier{ + withCmd([]string{"new"}), + withEnv([]string{"one", "two"}), + }, + expected: &container.Config{ + Cmd: []string{"new"}, + Env: []string{"one", "two"}, + }, + }, + } + + for _, testcase := range testcases { + runConfig := &container.Config{ + Cmd: defaultCmd, + Env: defaultEnv, + } + runConfigCopy := copyRunConfig(runConfig, testcase.modifiers...) + assert.Equal(t, testcase.expected, runConfigCopy, testcase.doc) + // Assert the original was not modified + assert.NotEqual(t, runConfig, runConfigCopy, testcase.doc) + } + +}