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 <dnephin@docker.com>

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
 }

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")))

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 {

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)
 }

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,
 		}
 	}

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
+}

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,
 		},

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

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)