From 9f738cc574d50d0a2accdf6f6deb30405c24a80c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 21 Apr 2017 15:08:11 -0400 Subject: [PATCH] Cleanup all the mutate + defer revert of b.runConfig in the builder Instead of mutating and reverting, just create a copy and pass the copy around. Add a unit test for builder dispatcher.run Fix two test failures Fix image history by adding a CreatedBy to commit options. Previously the createdBy field was being created by modifying a reference to the runConfig that was held from when the container was created. Fix a test that expected a trailing slash. Previously the runConfig was being modified by container create. Now that we're creating a copy of runConfig instead of sharing a reference the runConfig retains the trailing slash. Signed-off-by: Daniel Nephin --- api/types/backend/backend.go | 4 + builder/dockerfile/builder.go | 4 + builder/dockerfile/dispatchers.go | 100 +++++++++-------------- builder/dockerfile/dispatchers_test.go | 76 +++++++++++++++-- builder/dockerfile/internals.go | 42 ++++++---- builder/dockerfile/mockbackend_test.go | 27 +++++- daemon/commit.go | 9 +- image/cache/compare.go | 4 +- integration-cli/docker_cli_build_test.go | 19 ++--- 9 files changed, 183 insertions(+), 102 deletions(-) diff --git a/api/types/backend/backend.go b/api/types/backend/backend.go index 6dfd15e79a..368ad7b5ac 100644 --- a/api/types/backend/backend.go +++ b/api/types/backend/backend.go @@ -6,6 +6,7 @@ import ( "time" "github.com/docker/docker/api/types" + "github.com/docker/docker/api/types/container" ) // ContainerAttachConfig holds the streams to use when connecting to a container to view logs. @@ -97,4 +98,7 @@ type ExecProcessConfig struct { type ContainerCommitConfig struct { types.ContainerCommitConfig Changes []string + // TODO: ContainerConfig is only used by the dockerfile Builder, so remove it + // once the Builder has been updated to use a different interface + ContainerConfig *container.Config } diff --git a/builder/dockerfile/builder.go b/builder/dockerfile/builder.go index 6ed1fbdd8b..40d0cfe906 100644 --- a/builder/dockerfile/builder.go +++ b/builder/dockerfile/builder.go @@ -243,6 +243,10 @@ func (b *Builder) hasFromImage() bool { // // TODO: Remove? func BuildFromConfig(config *container.Config, changes []string) (*container.Config, error) { + if len(changes) == 0 { + return config, nil + } + b := newBuilder(context.Background(), builderOptions{}) result, err := parser.Parse(bytes.NewBufferString(strings.Join(changes, "\n"))) diff --git a/builder/dockerfile/dispatchers.go b/builder/dockerfile/dispatchers.go index 7dfecce9f2..715161270d 100644 --- a/builder/dockerfile/dispatchers.go +++ b/builder/dockerfile/dispatchers.go @@ -315,20 +315,18 @@ func workdir(req dispatchRequest) error { return nil } - cmd := req.runConfig.Cmd - comment := "WORKDIR " + req.runConfig.WorkingDir - // reset the command for cache detection - req.runConfig.Cmd = strslice.StrSlice(append(getShell(req.runConfig), "#(nop) "+comment)) - defer func(cmd strslice.StrSlice) { req.runConfig.Cmd = cmd }(cmd) + // TODO: why is this done here. This seems to be done at random places all over + // the builder + req.runConfig.Image = req.builder.image - // TODO: this should pass a copy of runConfig - if hit, err := req.builder.probeCache(req.builder.image, req.runConfig); err != nil || hit { + comment := "WORKDIR " + req.runConfig.WorkingDir + runConfigWithCommentCmd := copyRunConfig(req.runConfig, withCmdCommentString(comment)) + if hit, err := req.builder.probeCache(req.builder.image, runConfigWithCommentCmd); err != nil || hit { return err } - req.runConfig.Image = req.builder.image container, err := req.builder.docker.ContainerCreate(types.ContainerCreateConfig{ - Config: req.runConfig, + Config: runConfigWithCommentCmd, // Set a log config to override any default value set on the daemon HostConfig: &container.HostConfig{LogConfig: defaultLogConfig}, }) @@ -340,7 +338,7 @@ func workdir(req dispatchRequest) error { return err } - return req.builder.commitContainer(container.ID, copyRunConfig(req.runConfig, withCmd(cmd))) + return req.builder.commitContainer(container.ID, runConfigWithCommentCmd) } // RUN some command yo @@ -363,79 +361,57 @@ func run(req dispatchRequest) error { } args := handleJSONArgs(req.args, req.attributes) - if !req.attributes["json"] { args = append(getShell(req.runConfig), args...) } - config := &container.Config{ - Cmd: strslice.StrSlice(args), - Image: req.builder.image, + cmdFromArgs := strslice.StrSlice(args) + buildArgs := req.builder.buildArgsWithoutConfigEnv() + + saveCmd := cmdFromArgs + if len(buildArgs) > 0 { + saveCmd = prependEnvOnCmd(req.builder.buildArgs, buildArgs, cmdFromArgs) } - // stash the cmd - cmd := req.runConfig.Cmd - if len(req.runConfig.Entrypoint) == 0 && len(req.runConfig.Cmd) == 0 { - req.runConfig.Cmd = config.Cmd - } + // TODO: this was previously in b.create(), why is it necessary? + req.runConfig.Image = req.builder.image - // stash the config environment - env := req.runConfig.Env - - defer func(cmd strslice.StrSlice) { req.runConfig.Cmd = cmd }(cmd) - defer func(env []string) { req.runConfig.Env = env }(env) - - cmdBuildEnv := req.builder.buildArgsWithoutConfigEnv() - - // derive the command to use for probeCache() and to commit in this container. - // Note that we only do this if there are any build-time env vars. Also, we - // use the special argument "|#" at the start of the args array. This will - // avoid conflicts with any RUN command since commands can not - // start with | (vertical bar). The "#" (number of build envs) is there to - // help ensure proper cache matches. We don't want a RUN command - // that starts with "foo=abc" to be considered part of a build-time env var. - saveCmd := config.Cmd - if len(cmdBuildEnv) > 0 { - saveCmd = prependEnvOnCmd(req.builder.buildArgs, cmdBuildEnv, saveCmd) - } - - req.runConfig.Cmd = saveCmd - hit, err := req.builder.probeCache(req.builder.image, req.runConfig) + runConfigForCacheProbe := copyRunConfig(req.runConfig, withCmd(saveCmd)) + hit, err := req.builder.probeCache(req.builder.image, runConfigForCacheProbe) if err != nil || hit { return err } - // set Cmd manually, this is special case only for Dockerfiles - req.runConfig.Cmd = config.Cmd - // set build-time environment for 'run'. - req.runConfig.Env = append(req.runConfig.Env, cmdBuildEnv...) + runConfig := copyRunConfig(req.runConfig, + withCmd(cmdFromArgs), + withEnv(append(req.runConfig.Env, buildArgs...))) + // set config as already being escaped, this prevents double escaping on windows - req.runConfig.ArgsEscaped = true + runConfig.ArgsEscaped = true - logrus.Debugf("[BUILDER] Command to be executed: %v", req.runConfig.Cmd) + logrus.Debugf("[BUILDER] Command to be executed: %v", runConfig.Cmd) - // 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) + cID, err := req.builder.create(runConfig) if err != nil { return err } - - if err := req.builder.run(cID); err != nil { + if err := req.builder.run(cID, runConfig.Cmd); err != nil { return err } - // FIXME: this is duplicated with the defer above in this function (i think?) - // revert to original config environment and set the command string to - // have the build-time env vars in it (if any) so that future cache look-ups - // properly match it. - req.runConfig.Env = env - - req.runConfig.Cmd = saveCmd - return req.builder.commitContainer(cID, copyRunConfig(req.runConfig, withCmd(cmd))) + return req.builder.commitContainer(cID, runConfigForCacheProbe) } +// Derive the command to use for probeCache() and to commit in this container. +// Note that we only do this if there are any build-time env vars. Also, we +// use the special argument "|#" at the start of the args array. This will +// avoid conflicts with any RUN command since commands can not +// start with | (vertical bar). The "#" (number of build envs) is there to +// help ensure proper cache matches. We don't want a RUN command +// that starts with "foo=abc" to be considered part of a build-time env var. +// +// remove any unreferenced built-in args from the environment variables. +// These args are transparent so resulting image should be the same regardless +// of the value. func prependEnvOnCmd(buildArgs *buildArgs, buildArgVars []string, cmd strslice.StrSlice) strslice.StrSlice { var tmpBuildEnv []string for _, env := range buildArgVars { diff --git a/builder/dockerfile/dispatchers_test.go b/builder/dockerfile/dispatchers_test.go index 2b928e6bf6..72a82dbd91 100644 --- a/builder/dockerfile/dispatchers_test.go +++ b/builder/dockerfile/dispatchers_test.go @@ -5,7 +5,10 @@ import ( "runtime" "testing" + "bytes" + "context" "github.com/docker/docker/api/types" + "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" @@ -49,6 +52,9 @@ func newBuilderWithMockBackend() *Builder { options: &types.ImageBuildOptions{}, docker: &MockBackend{}, buildArgs: newBuildArgs(make(map[string]*string)), + tmpContainers: make(map[string]struct{}), + Stdout: new(bytes.Buffer), + clientCtx: context.Background(), disableCommit: true, } b.imageContexts = &imageContexts{b: b} @@ -409,13 +415,71 @@ func TestParseOptInterval(t *testing.T) { Value: "50ns", } _, err := parseOptInterval(flInterval) - if err == nil { - t.Fatalf("Error should be presented for interval %s", flInterval.Value) - } + testutil.ErrorContains(t, err, "cannot be less than 1ms") flInterval.Value = "1ms" _, err = parseOptInterval(flInterval) - if err != nil { - t.Fatalf("Unexpected error: %s", err.Error()) - } + require.NoError(t, err) +} + +func TestPrependEnvOnCmd(t *testing.T) { + buildArgs := newBuildArgs(nil) + buildArgs.AddArg("NO_PROXY", nil) + + args := []string{"sorted=nope", "args=not", "http_proxy=foo", "NO_PROXY=YA"} + cmd := []string{"foo", "bar"} + cmdWithEnv := prependEnvOnCmd(buildArgs, args, cmd) + expected := strslice.StrSlice([]string{ + "|3", "NO_PROXY=YA", "args=not", "sorted=nope", "foo", "bar"}) + assert.Equal(t, expected, cmdWithEnv) +} + +func TestRunWithBuildArgs(t *testing.T) { + b := newBuilderWithMockBackend() + b.buildArgs.argsFromOptions["HTTP_PROXY"] = strPtr("FOO") + b.disableCommit = false + + origCmd := strslice.StrSlice([]string{"cmd", "in", "from", "image"}) + cmdWithShell := strslice.StrSlice(append(getShell(b.runConfig), "echo foo")) + envVars := []string{"|1", "one=two"} + cachedCmd := strslice.StrSlice(append(envVars, cmdWithShell...)) + + imageCache := &mockImageCache{ + getCacheFunc: func(parentID string, cfg *container.Config) (string, error) { + // Check the runConfig.Cmd sent to probeCache() + assert.Equal(t, cachedCmd, cfg.Cmd) + return "", nil + }, + } + b.imageCache = imageCache + + mockBackend := b.docker.(*MockBackend) + mockBackend.getImageOnBuildImage = &mockImage{ + id: "abcdef", + config: &container.Config{Cmd: origCmd}, + } + mockBackend.containerCreateFunc = func(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) { + // Check the runConfig.Cmd sent to create() + assert.Equal(t, cmdWithShell, config.Config.Cmd) + assert.Contains(t, config.Config.Env, "one=two") + return container.ContainerCreateCreatedBody{ID: "12345"}, nil + } + mockBackend.commitFunc = func(cID string, cfg *backend.ContainerCommitConfig) (string, error) { + // Check the runConfig.Cmd sent to commit() + assert.Equal(t, origCmd, cfg.Config.Cmd) + assert.Equal(t, cachedCmd, cfg.ContainerConfig.Cmd) + return "", nil + } + + req := defaultDispatchReq(b, "abcdef") + require.NoError(t, from(req)) + b.buildArgs.AddArg("one", strPtr("two")) + // TODO: this can be removed with b.runConfig + req.runConfig.Cmd = origCmd + + req.args = []string{"echo foo"} + require.NoError(t, run(req)) + + // Check that runConfig.Cmd has not been modified by run + assert.Equal(t, origCmd, b.runConfig.Cmd) } diff --git a/builder/dockerfile/internals.go b/builder/dockerfile/internals.go index 1cb2eeef16..a772c615a8 100644 --- a/builder/dockerfile/internals.go +++ b/builder/dockerfile/internals.go @@ -21,7 +21,6 @@ import ( "github.com/docker/docker/api/types" "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" "github.com/docker/docker/builder/dockerfile/parser" "github.com/docker/docker/builder/remotecontext" @@ -56,10 +55,10 @@ func (b *Builder) commit(comment string) error { return err } - return b.commitContainer(id, b.runConfig) + return b.commitContainer(id, runConfigWithCommentCmd) } -func (b *Builder) commitContainer(id string, runConfig *container.Config) error { +func (b *Builder) commitContainer(id string, containerConfig *container.Config) error { if b.disableCommit { return nil } @@ -68,8 +67,9 @@ func (b *Builder) commitContainer(id string, runConfig *container.Config) error ContainerCommitConfig: types.ContainerCommitConfig{ Author: b.maintainer, Pause: true, - Config: runConfig, + Config: b.runConfig, }, + ContainerConfig: containerConfig, } // Commit the container @@ -81,7 +81,7 @@ func (b *Builder) commitContainer(id string, runConfig *container.Config) error // TODO: this function should return imageID and runConfig instead of setting // then on the builder b.image = imageID - b.imageContexts.update(imageID, runConfig) + b.imageContexts.update(imageID, b.runConfig) return nil } @@ -100,6 +100,8 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD // Work in daemon-specific filepath semantics dest := filepath.FromSlash(args[len(args)-1]) // last one is always the dest + // TODO: why is this done here. This seems to be done at random places all over + // the builder b.runConfig.Image = b.image var infos []copyInfo @@ -164,18 +166,16 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD srcHash = "multi:" + hex.EncodeToString(hasher.Sum(nil)) } - 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) - - // TODO: this should pass a copy of runConfig - if hit, err := b.probeCache(b.image, b.runConfig); err != nil || hit { + runConfigWithCommentCmd := copyRunConfig( + b.runConfig, + withCmdCommentString(fmt.Sprintf("%s %s in %s ", cmdName, srcHash, dest))) + if hit, err := b.probeCache(b.image, runConfigWithCommentCmd); err != nil || hit { return err } container, err := b.docker.ContainerCreate(types.ContainerCreateConfig{ - Config: b.runConfig, + Config: runConfigWithCommentCmd, // Set a log config to override any default value set on the daemon HostConfig: &container.HostConfig{LogConfig: defaultLogConfig}, }) @@ -196,7 +196,7 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD } } - return b.commitContainer(container.ID, copyRunConfig(b.runConfig, withCmd(cmd))) + return b.commitContainer(container.ID, runConfigWithCommentCmd) } type runConfigModifier func(*container.Config) @@ -215,12 +215,24 @@ func withCmd(cmd []string) runConfigModifier { } } +// withCmdComment sets Cmd to a nop comment string. See withCmdCommentString for +// why there are two almost identical versions of this. func withCmdComment(comment string) runConfigModifier { return func(runConfig *container.Config) { runConfig.Cmd = append(getShell(runConfig), "#(nop) ", comment) } } +// withCmdCommentString exists to maintain compatibility with older versions. +// A few instructions (workdir, copy, add) used a nop comment that is a single arg +// where as all the other instructions used a two arg comment string. This +// function implements the single arg version. +func withCmdCommentString(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 @@ -606,7 +618,7 @@ func (b *Builder) create(runConfig *container.Config) (string, error) { var errCancelled = errors.New("build cancelled") -func (b *Builder) run(cID string) (err error) { +func (b *Builder) run(cID string, cmd []string) (err error) { attached := make(chan struct{}) errCh := make(chan error) go func() { @@ -660,7 +672,7 @@ func (b *Builder) run(cID string) (err error) { } // TODO: change error type, because jsonmessage.JSONError assumes HTTP return &jsonmessage.JSONError{ - Message: fmt.Sprintf("The command '%s' returned a non-zero code: %d", strings.Join(b.runConfig.Cmd, " "), ret), + Message: fmt.Sprintf("The command '%s' returned a non-zero code: %d", strings.Join(cmd, " "), ret), Code: ret, } } diff --git a/builder/dockerfile/mockbackend_test.go b/builder/dockerfile/mockbackend_test.go index d015372a16..e8647f426c 100644 --- a/builder/dockerfile/mockbackend_test.go +++ b/builder/dockerfile/mockbackend_test.go @@ -15,13 +15,19 @@ import ( // MockBackend implements the builder.Backend interface for unit testing type MockBackend struct { - getImageOnBuildFunc func(string) (builder.Image, error) + 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 } @@ -38,6 +44,9 @@ func (m *MockBackend) ContainerAttachRaw(cID string, stdin io.ReadCloser, stdout } func (m *MockBackend) ContainerCreate(config types.ContainerCreateConfig) (container.ContainerCreateCreatedBody, error) { + if m.containerCreateFunc != nil { + return m.containerCreateFunc(config) + } return container.ContainerCreateCreatedBody{}, nil } @@ -45,7 +54,10 @@ func (m *MockBackend) ContainerRm(name string, config *types.ContainerRmConfig) return nil } -func (m *MockBackend) Commit(string, *backend.ContainerCommitConfig) (string, error) { +func (m *MockBackend) Commit(cID string, cfg *backend.ContainerCommitConfig) (string, error) { + if m.commitFunc != nil { + return m.commitFunc(cID, cfg) + } return "", nil } @@ -97,3 +109,14 @@ func (i *mockImage) ImageID() string { func (i *mockImage) RunConfig() *container.Config { return i.config } + +type mockImageCache struct { + getCacheFunc func(parentID string, cfg *container.Config) (string, error) +} + +func (mic *mockImageCache) GetCache(parentID string, cfg *container.Config) (string, error) { + if mic.getCacheFunc != nil { + return mic.getCacheFunc(parentID, cfg) + } + return "", nil +} diff --git a/daemon/commit.go b/daemon/commit.go index f3e840c7cc..e64c7d3333 100644 --- a/daemon/commit.go +++ b/daemon/commit.go @@ -129,6 +129,11 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str return "", err } + containerConfig := c.ContainerConfig + if containerConfig == nil { + containerConfig = container.Config + } + // It is not possible to commit a running container on Windows and on Solaris. if (runtime.GOOS == "windows" || runtime.GOOS == "solaris") && container.IsRunning() { return "", errors.Errorf("%+v does not support commit of a running container", runtime.GOOS) @@ -185,7 +190,7 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str h := image.History{ Author: c.Author, Created: time.Now().UTC(), - CreatedBy: strings.Join(container.Config.Cmd, " "), + CreatedBy: strings.Join(containerConfig.Cmd, " "), Comment: c.Comment, EmptyLayer: true, } @@ -204,7 +209,7 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str Architecture: runtime.GOARCH, OS: runtime.GOOS, Container: container.ID, - ContainerConfig: *container.Config, + ContainerConfig: *containerConfig, Author: c.Author, Created: h.Created, }, diff --git a/image/cache/compare.go b/image/cache/compare.go index 6abbdcd8f3..9237932463 100644 --- a/image/cache/compare.go +++ b/image/cache/compare.go @@ -1,6 +1,8 @@ package cache -import "github.com/docker/docker/api/types/container" +import ( + "github.com/docker/docker/api/types/container" +) // compare two Config struct. Do not compare the "Image" nor "Hostname" fields // If OpenStdin is set, then it differs diff --git a/integration-cli/docker_cli_build_test.go b/integration-cli/docker_cli_build_test.go index aaeb499155..5ee64cbe4b 100644 --- a/integration-cli/docker_cli_build_test.go +++ b/integration-cli/docker_cli_build_test.go @@ -345,11 +345,8 @@ ONBUILD RUN ["true"]`)) buildImageSuccessfully(c, name2, build.WithDockerfile(fmt.Sprintf(`FROM %s`, name1))) - out, _ := dockerCmd(c, "run", name2) - if !regexp.MustCompile(`(?m)^hello world`).MatchString(out) { - c.Fatalf("did not get echo output from onbuild. Got: %q", out) - } - + result := cli.DockerCmd(c, "run", name2) + result.Assert(c, icmd.Expected{Out: "hello world"}) } // FIXME(vdemeester) why we disabled cache here ? @@ -4376,12 +4373,8 @@ func (s *DockerSuite) TestBuildTimeArgHistoryExclusions(c *check.C) { if strings.Contains(out, "https_proxy") { c.Fatalf("failed to exclude proxy settings from history!") } - if !strings.Contains(out, fmt.Sprintf("%s=%s", envKey, envVal)) { - c.Fatalf("explicitly defined ARG %s is not in output", explicitProxyKey) - } - if !strings.Contains(out, fmt.Sprintf("%s=%s", envKey, envVal)) { - c.Fatalf("missing build arguments from output") - } + result.Assert(c, icmd.Expected{Out: fmt.Sprintf("%s=%s", envKey, envVal)}) + result.Assert(c, icmd.Expected{Out: fmt.Sprintf("%s=%s", explicitProxyKey, explicitProxyVal)}) cacheID := buildImage(imgName + "-two") c.Assert(origID, checker.Equals, cacheID) @@ -4576,9 +4569,7 @@ func (s *DockerSuite) TestBuildBuildTimeArgExpansion(c *check.C) { ) res := inspectField(c, imgName, "Config.WorkingDir") - if res != filepath.ToSlash(filepath.Clean(wdVal)) { - c.Fatalf("Config.WorkingDir value mismatch. Expected: %s, got: %s", filepath.ToSlash(filepath.Clean(wdVal)), res) - } + c.Check(res, check.Equals, filepath.ToSlash(wdVal)) var resArr []string inspectFieldAndUnmarshall(c, imgName, "Config.Env", &resArr)