Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix environ substitutions in `docker commit --change ...` #35582

Merged
merged 1 commit into from Dec 21, 2017

Conversation

@asottile
Copy link
Contributor

commented Nov 22, 2017

The building machinery was being handed an uninitialized container
Config. This changes it to use the target container's Config.

Resolves #30538

- How to verify it

before

# cid=$(docker create ubuntu:xenial) && \
>     docker commit --change 'ENV PATH=/wat:$PATH' "$cid" test > /dev/null && \
>     docker rm "$cid" > /dev/null && \
>     docker inspect test --format='{{.Config.Env}}'
[PATH=/wat:]

after

# cid=$(docker create ubuntu:xenial) && \
>    docker commit --change 'ENV PATH=/wat:$PATH' "$cid" test > /dev/null && \
>    docker rm "$cid" > /dev/null && \
>    docker inspect test --format='{{.Config.Env}}'
[PATH=/wat:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin]

- Description for the changelog
Fix environ substitutions in docker commit --change ...

- A picture of a cute animal (not mandatory but encouraged)

@GordonTheTurtle

This comment has been minimized.

Copy link

commented Nov 22, 2017

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "use_base_containers_config" git@github.com:asottile/moby.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2017

An aside: while working on this I noticed that 4 (!) different Config objects are in scope inside Commit:

  • container.Config: (probably the only useful one)
  • newConfig: BuildFromConfig return value (should probably be used the rest of the function?)
  • c.ContainerConfig: ???
  • c.Config: ???

lmk if you want me to attempt to clean this up as part of this change as well :)

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

My initial patch was:

diff --git a/daemon/commit.go b/daemon/commit.go
index 0053132..3482af9 100644
--- a/daemon/commit.go
+++ b/daemon/commit.go
@@ -149,7 +149,7 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str
                defer daemon.containerUnpause(container)
        }
 
-       newConfig, err := dockerfile.BuildFromConfig(c.Config, c.Changes)
+       newConfig, err := dockerfile.BuildFromConfig(container.Config, c.Changes)
        if err != nil {
                return "", err
        }

The current patch imo is a little lower impact :)

@asottile asottile requested review from dnephin and vdemeester as code owners Nov 28, 2017

@@ -125,7 +125,7 @@ func (s *DockerSuite) TestCommitChange(c *check.C) {
prefix = strings.ToUpper(prefix) // Force C: as that's how WORKDIR is normalized on Windows
expected := map[string]string{
"Config.ExposedPorts": "map[8080/tcp:{}]",
"Config.Env": "[DEBUG=true test=1 PATH=/foo]",
"Config.Env": "[PATH=/foo DEBUG=true test=1]",

This comment has been minimized.

Copy link
@asottile

asottile Nov 28, 2017

Author Contributor

I was a bit puzzled why this changed until I thought more about it. Now that the base configuration environ ([PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin]) is being used to seed the new environ -- --change PATH=/foo will override the beginning element and then the others will append.

imageID, _ := dockerCmd(c, "commit",
"--change", "ENV PATH=/usr/bin:$PATH",
"test2", "test-img-2")
imageID = strings.TrimSpace(imageID)

This comment has been minimized.

Copy link
@asottile

asottile Nov 28, 2017

Author Contributor

I decided to commit twice to make this test less dependent on the details of busybox (though I doubt that busybox's PATH is going to change any time soon)

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

I don't mean to be short but I find this a bit ridiculous:

03:59:21 The following new tests were added to integration-cli:
03:59:21 
03:59:21 +func (s *DockerSuite) TestCommitInheritEnv(c *check.C) {
03:59:21 
03:59:21 integration-cli is deprecated. Please add an API integration test to
03:59:21 ./integration/COMPONENT/. See ./TESTING.md for more details.
03:59:21 
03:59:21 Build step 'Execute shell' marked build as failure

There's 55kloc of integration-cli tests:

$ git ls-files -- integration-cli/ | xargs wc -l | tail -1
  55358 total

and a mere 2kloc of integration tests:

$ git ls-files -- integration/ | xargs wc -l | tail -1
 2051 total

There's no examples of anything even remotely related to what I'm touching in commit in either unit tests (there's no daemon/commit_test.go even!) or integration tests -- adding an entire testsuite for this (imo quite complicated interaction between components needing an integration test) seems very out of scope.

I get that the effort is in the direction of eliminating integration-cli, but there's a ton of friction to even start working in that direction! Honestly my first gut on seeing that error message was, wellp I'll just delete the test then.

CC @dnephin

// Ensure environ substitutions in commit use the container's image config
if len(c.Config.Env) == 0 {
c.Config.Env = make([]string, len(container.Config.Env))
copy(c.Config.Env, container.Config.Env)

This comment has been minimized.

Copy link
@dnephin

dnephin Nov 28, 2017

Member

I'm not sure why c.Config even exists. It seems like it should be using container.Config as the base, but it's been this way for a while.

This fix looks right when used with the CLI, because the CLI never sends a c.Config in the request body, but there could be API consumers sending a request body. This is a great example of why testing using the CLI is a bad idea.

If someone is sending a Config in the request body I guess it's on them to use the correct Config.

This comment has been minimized.

Copy link
@asottile

asottile Nov 28, 2017

Author Contributor

I think the actual semantics I want is to replace this if statement here to something like:

if c == nil {
    c = copyRunConfig(GetContainer(cname).Config)
}

but the current encapsulation doesn't allow me to do this (perhaps intentionally?)

This comment has been minimized.

Copy link
@dnephin

dnephin Nov 28, 2017

Member

You shouldn't need copy because GetContainer() should hopefully give you a copy already.

GetContainer() could be exposed in the Backend interface. That should probably work

This comment has been minimized.

Copy link
@asottile

asottile Nov 28, 2017

Author Contributor

from what I can tell, it gives me the live Container and not a copy :(

Where would I look to extend the Backend interface?

This comment has been minimized.

Copy link
@dnephin

dnephin Nov 28, 2017

Member

https://github.com/moby/moby/blob/c307e0ce49d63d1b1f479560955dca3956f005ec/api/server/router/image/backend.go

The methods exposed by backend all return api/types not internal types, so I guess you'll need to use Daemon.ContainerInspectCurrent() instead of GetContainer(), but that should still give you the container.Config, and that one would be a copy.

@dnephin

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

I can help you with the tests. The test would go into integration/image/commit_test.go. I would expect it to do something like this:

package image

import (
	"context"
	"testing"

	"github.com/docker/docker/api/types"
	"github.com/docker/docker/api/types/container"
	"github.com/docker/docker/integration/util/request"
	"github.com/stretchr/testify/assert"
	"github.com/stretchr/testify/require"
)

func TestCommit(t *testing.T) {
	defer setupTest(t)()
	client := request.NewAPIClient(t)
	ctx := context.Background()

	createResp, err := client.ContainerCreate(ctx, &container.Config{Image: "busybox"}, nil, nil, "")
	require.NoError(t, err)

	commitResp, err := client.ContainerCommit(ctx, createResp.ID, types.ContainerCommitOptions{
		Changes:   []string{"ENV PATH=/new/path:$PATH"},
		Reference: "test-commit-image",
	})
	require.NoError(t, err)

	image, _, err := client.ImageInspectWithRaw(ctx, commitResp.ID)
	require.NoError(t, err)
	expected := types.ImageInspect{
		Config: &container.Config{
			Env: []string{"PATH=..."},
			// ...
		},
		// ...
	}
	assert.Equal(t, expected, image)
}
@dnephin

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

Re: "why are there so many config objects in Commit?" it looks like a refactor in #17762 made some changes that haven't aged well. I think the API postCommit() should only accept either a container name or a request.Body, and error if both are set. Then Commit() shouldn't need to merge anything.

c.ContainerConfig is required by the builder, it's a separate field in the Image.

Commit could definitely use some cleanup.

@asottile asottile requested a review from tonistiigi as a code owner Nov 28, 2017

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

@dnephin I've updated my patch

I still needed the copyRunConfig in builder/dockerfile/builder.go otherwise this test fails. This to me means two things:

  • we're not getting a copy
  • BuildFromConfig both mutates the first parameter and returns it.
----------------------------------------------------------------------
FAIL: docker_cli_commit_test.go:146: DockerSuite.TestCommitChangeLabels

docker_cli_commit_test.go:156:
    // check that container labels didn't change
    c.Assert(inspectField(c, "test", "Config.Labels"), checker.Equals, "map[some:label]")
... obtained string = "map[some:label2]"
... expected string = "map[some:label]"


----------------------------------------------------------------------
@dnephin

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

There's a golint error, but otherwise I think this looks good.

With this change I think MergeConfigs can be removed. Mind trying to remove that and see if the tests are happy?

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

cool, fixed the lint error, I've pushed these in separate commits and if the tests are happy I can squash.

EDIT: I'm spoiled by pre-commit heh.

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 28, 2017

yeah removing MergeConfigs fails some tests:

----------------------------------------------------------------------
FAIL: docker_api_containers_test.go:509: DockerSuite.TestContainerAPICommitWithLabelInConfig

docker_api_containers_test.go:535:
    c.Assert(cmd, checker.Equals, "[/bin/sh -c touch /test]", check.Commentf("got wrong Cmd from commit: %q", cmd))
... obtained string = "[]"
... expected string = "[/bin/sh -c touch /test]"
... got wrong Cmd from commit: "[]"

going to leave that bit out for now

Here's the patch I had for that:

commit 0a7f13ee420eed24aac1356149f37967918bf32b
Author: Anthony Sottile <asottile@umich.edu>
Date:   Tue Nov 28 13:17:54 2017 -0800

    Remove MergeConfigs
    
    Signed-off-by: Anthony Sottile <asottile@umich.edu>

diff --git a/api/server/router/image/image_routes.go b/api/server/router/image/image_routes.go
index e2a50db..ecd1db5 100644
--- a/api/server/router/image/image_routes.go
+++ b/api/server/router/image/image_routes.go
@@ -55,13 +55,12 @@ func (s *imageRouter) postCommit(ctx context.Context, w http.ResponseWriter, r *
 
        commitCfg := &backend.ContainerCommitConfig{
                ContainerCommitConfig: types.ContainerCommitConfig{
-                       Pause:        pause,
-                       Repo:         r.Form.Get("repo"),
-                       Tag:          r.Form.Get("tag"),
-                       Author:       r.Form.Get("author"),
-                       Comment:      r.Form.Get("comment"),
-                       Config:       c,
-                       MergeConfigs: true,
+                       Pause:   pause,
+                       Repo:    r.Form.Get("repo"),
+                       Tag:     r.Form.Get("tag"),
+                       Author:  r.Form.Get("author"),
+                       Comment: r.Form.Get("comment"),
+                       Config:  c,
                },
                Changes: r.Form["changes"],
        }
diff --git a/api/types/configs.go b/api/types/configs.go
index 20c19f2..9b561ed 100644
--- a/api/types/configs.go
+++ b/api/types/configs.go
@@ -33,9 +33,7 @@ type ContainerCommitConfig struct {
        Tag     string
        Author  string
        Comment string
-       // merge container config into commit config before commit
-       MergeConfigs bool
-       Config       *container.Config
+       Config  *container.Config
 }
 
 // ExecConfig is a small subset of the Config struct that holds the configuration
diff --git a/daemon/commit.go b/daemon/commit.go
index 0053132..1e3a555 100644
--- a/daemon/commit.go
+++ b/daemon/commit.go
@@ -154,12 +154,6 @@ func (daemon *Daemon) Commit(name string, c *backend.ContainerCommitConfig) (str
                return "", err
        }
 
-       if c.MergeConfigs {
-               if err := merge(newConfig, container.Config); err != nil {
-                       return "", err
-               }
-       }
-
        rwTar, err := daemon.exportContainerRw(container)
        if err != nil {
                return "", err
@dnephin

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

Thanks for trying.

WindowsRS1 failures looks related

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

Indeed, the windows failures would imply that busybox on windows doesn't have PATH set by default... strange. Any ideas how to debug this and/or fix the test? My gut is to if windows: ... else: ... but I'd like to verify my assumption before doing so.

@dnephin

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

Sounds right, if/else is what we do everywhere else, so should be fine here too

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 29, 2017

Added a branch, windows is passing now!

@asottile

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2017

@dnephin seems like powerpc flaked, is there a way to restart that build?

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Nov 30, 2017

I restarted PowerPC 👍

@dnephin
Copy link
Member

left a comment

LGTM

@vdemeester
Copy link
Member

left a comment

LGTM 🐸

@vdemeester

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

@@ -47,7 +46,11 @@ func (s *imageRouter) postCommit(ctx context.Context, w http.ResponseWriter, r *
return err
}
if c == nil {
c = &container.Config{}
containerJSON, err := s.backend.ContainerInspectCurrent(cname, false)

This comment has been minimized.

Copy link
@tonistiigi

tonistiigi Dec 11, 2017

Member

Why is this in builder pkg and not in daemon.commit ?

This comment has been minimized.

Copy link
@dnephin

dnephin Dec 11, 2017

Member

Do you mean the api package?

daemon.Commit() is overloaded right now, it's trying to share too much when the two consumers are not that similar. The parts that are not similar need to be pulled out

This comment has been minimized.

Copy link
@tonistiigi

tonistiigi Dec 11, 2017

Member

Sorry, yes, api. Shouldn't MergeConfigs mean the same what these lines are doing(when empty config is passed)?

This comment has been minimized.

Copy link
@dnephin

dnephin Dec 11, 2017

Member

My question is why would MergeConfigs even need to exist? It seems like a hack to support a weird code structure (it was added in a refactor, not as a feature).

This comment has been minimized.

Copy link
@tonistiigi

tonistiigi Dec 11, 2017

Member

My understanding is that commits from builder do not merge while commits from commit do. If you would want to refactor it so that they don't call the same function with different parameters(you comment about it being overloaded) that may be ok, but makes it even weirder to have parts of the same functionality split in different packages.

This comment has been minimized.

Copy link
@tonistiigi

tonistiigi Dec 19, 2017

Member

Do you agree with my assessment of the 3 cases?

2 and 3 could happen together. But that isn't really related to this PR as this is fixing an issue in existing code, not refactoring the internal go api.

This comment has been minimized.

Copy link
@tonistiigi

tonistiigi Dec 20, 2017

Member

To be clear, my proposed fixed is

if c.MergeConfigs && c.Config== nil {
  c.Config = container.Config
}

before BuildConfig in the daemon, that avoids adding the extra method.

This comment has been minimized.

Copy link
@dnephin

dnephin Dec 20, 2017

Member

Thanks. While I still think this makes Commit() worse, I agree it does keep the exported interface cleaner. Since it may be a while before we can properly cleanup the commit api backend, I guess this is a better option for now.

@asottile do you mind making that change? It should remove the need to change the Backend interface.

Sorry for all the changes on this.

This comment has been minimized.

Copy link
@asottile

asottile Dec 20, 2017

Author Contributor

That was actually my initial patch but I believe it broke tests. I can try it again.

This comment has been minimized.

Copy link
@asottile

asottile Dec 20, 2017

Author Contributor

seems to work now, probably because of the code I added to avoid the mutation inside BuildFromConfig

Fix environ substitutions in `docker commit --change ...`
The building machinery was being handed an uninitialized container
Config.  This changes it to use the target container's Config.

Resolves #30538

Signed-off-by: Anthony Sottile <asottile@umich.edu>
@dnephin
Copy link
Member

left a comment

LGTM

@tonistiigi
Copy link
Member

left a comment

@asottile Thanks!

LGTM (if green)

@tonistiigi

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

z failure unrelated (other PRs hanging same way).

@tonistiigi tonistiigi merged commit a5de79b into moby:master Dec 21, 2017

5 of 6 checks passed

z Jenkins build Docker-PRs-s390x 7473 has encountered an error
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38514 has succeeded
Details
janky Jenkins build Docker-PRs 47250 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7630 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18762 has succeeded
Details
@thaJeztah

This comment has been minimized.

Copy link
Member

commented Dec 21, 2017

Thank you @asottile !

@asottile asottile deleted the asottile:use_base_containers_config branch Dec 21, 2017

SamirTalwar added a commit to prodo-ai/plz that referenced this pull request Jan 26, 2018
ml-pytorch: Set the PATH in a script, not with ENV.
This is required because setting the PATH or PYTHONPATH self-referentially when
building the Docker image doesn't work with Packer.  This is because it uses
`docker commit --change`, which appears to have a bug in its handling of
environment variables.

More information can be found at:
moby/moby#35582
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.