Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY in stream mode #549

shouze · 2017-09-22T09:25:12Z

- What I did

Fix of moby/moby#32816 for stream mode. This is a follow up of #513. Also related to tonistiigi/fsutil#10 & moby/buildkit#126

- How I did it

By resetting uid/gid to 0/0 in the build context in stream mode.

- How to verify it

go test -v ./cli/command/image -run TestRunBuildResetsUidAndGidInContextWithStreamMode

- Description for the changelog

Reset uid/gid to 0 in build context to fix cache busting issues on
ADD/COPY in stream mode

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

shouze · 2017-09-22T09:35:55Z

@tonistiigi I currently have this failure, any idea why? I admit I've still not looked for, just written the test as I was believing it should be implemented. Also I can merge TestRunBuildResetsUidAndGidInContextWithStreamMode & TestRunBuildResetsUidAndGidInContext in a next move and manage stream option as a test case, WDYT?

=== RUN   TestRunBuildResetsUidAndGidInContextWithStreamMode
--- FAIL: TestRunBuildResetsUidAndGidInContextWithStreamMode (0.00s)
        Error Trace:    build_test.go:52
	Error:      	Received unexpected error:
	            	open Dockerfile: no such file or directory
	            	failed to open Dockerfile
	            	github.com/docker/cli/cli/command/image.runBuild
	            		/go/src/github.com/docker/cli/cli/command/image/build.go:267
	            	github.com/docker/cli/cli/command/image.TestRunBuildResetsUidAndGidInContextWithStreamMode
	            		/go/src/github.com/docker/cli/cli/command/image/build_test.go:51
	            	testing.tRunner
	            		/usr/local/go/src/testing/testing.go:657
	            	runtime.goexit
	            		/usr/local/go/src/runtime/asm_amd64.s:2197

dnephin

I think they make sense as separate test cases for now.

You might try rebasing on #294 to see if the bug still exists.

I suspect you'll need to provide another function to the fake for the client session endpoint, but I don't think that's related to the error you're hitting now.

dnephin · 2017-09-22T15:11:40Z

cli/command/image/build_session.go

+	workdirProvider := filesync.NewFSSyncProvider([]filesync.SyncedDir{
+		{Dir: contextDir},
+		{Excludes: excludes},
+		{Map: func(s *fsutil.Stat) bool {


Shouldn't all of these be in a single SyncedDir instead of 3 separate ones?

I'm alsonot seeing a Map field in master,so we should wait for moby/buildkit#126 to merge and vendor in master instead of the PR branch.

Yes.. only one you're right 😊 . And yes Map is part of moby/buildkit#126 this PR needs to use master of course before merge.

shouze · 2017-09-25T15:42:55Z

@dnephin I've rebased with #294 and the bug remains. I still need to figure out why I have this Dockerfile error in stream mode. I guess I don't reproduce the way that stream mode currently works in the test, will investigate further.

shouze · 2017-09-25T17:07:38Z

@tonistiigi @dnephin ok so I've tooked a look further and... the Dockerfile error is related to:

cli/cli/command/image/build.go

Lines 262 to 270 in bd6e175

    
           // if streaming and dockerfile was not from stdin then read from file 
        
           // to the same reader that is usually stdin 
        
           if options.stream && dockerfileCtx == nil { 
        
           	dockerfileCtx, err = os.Open(relDockerfile) 
        
           	if err != nil { 
        
           		return errors.Wrapf(err, "failed to open %s", relDockerfile) 
        
           	} 
        
           	defer dockerfileCtx.Close() 
        
           }

BTW, If I comment this statement or if I use a Dockerfile from stdin, then I get this error from the fakeCli:

=== RUN   TestRunBuildResetsUidAndGidInContextWithStreamMode
--- FAIL: TestRunBuildResetsUidAndGidInContextWithStreamMode (0.00s)
        Error Trace:    build_test.go:31
	            	client_test.go:121
	            	build.go:390
	            	build_test.go:51
	Error:      	Received unexpected error:
	            	Empty archive
FAIL

@tonistiigi looks like it's ok as stream mode maybe don't produce a tar archive in fact?

if I dump some interesting fakeImage args in my test:

context is nil
options:
- SessionID & RemoteContext remains undefined

Maybe because there's an async build process here with the session which is dialed, it there a way to wait against this async process?

dnephin · 2017-09-25T17:23:48Z

I think it is expected that stream doesn't produce an archive, it would send files using the stream.

We need to fake the client lib parts for the session call as well I think, but I'm not sure if that's related to the error.

shouze · 2017-09-25T17:30:01Z

Yes from what I've read stream mode uses diffcopy (was supporting a tarstream mode in the past but removed). Ok will look around to fake client lib parts for the session but yes for the original error about the Dockerfile I still don't know, will probably find on my way. Thank you.

shouze · 2017-09-25T17:39:19Z

Not that easy BTW as I probably have to fake the build_session... but in the same time.... my patch is on addDirToSession in build_session! So... maybe it worth instead to write a unit test for this addDirToSession only in build_session?

dnephin · 2017-09-26T14:41:28Z

A unit test for addDirToSession seems appropriate.

(and fsutil) Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

This is a follow up of #513 for stream mode. Fixes cache busting issues in the build context Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

shouze · 2017-09-26T16:51:00Z

cli/command/image/build_session_test.go

+	require.NoError(t, err)
+
+	var s *session.Session
+	s, err = session.NewSession(filepath.Base(contextDir), sharedKey)


@dnephin @tonistiigi ok I guess the best is to mock/fake the session here instead of using the true one. @tonistiigi does it worth that builkit provides a session faker at some point? I can put it in the cli ATM.

Having the CLI depend on a type Session interface{...} instead of the struct would be great, then it should be easy for us to fake.

A fake as part of the CLI testing code sounds fine to me.

Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

codecov-io · 2017-09-26T16:51:42Z

Codecov Report

Merging #549 into master will increase coverage by 0.17%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #549      +/-   ##
==========================================
+ Coverage   49.49%   49.66%   +0.17%     
==========================================
  Files         208      208              
  Lines       17153    17164      +11     
==========================================
+ Hits         8490     8525      +35     
+ Misses       8231     8203      -28     
- Partials      432      436       +4

thaJeztah · 2017-09-29T21:10:32Z

this needs a rebase now, @shouze 😢

shouze · 2017-09-30T05:15:09Z

@thaJestah, yes and even if green I need to introduce the session interface and to fake it prior to be able to write the good test.

I’ll be back on this soon.

thaJeztah · 2017-10-01T12:36:30Z

No worries, thanks! Let us know if you need help 👍

thaJeztah · 2017-10-31T12:13:19Z

friendly nudge @shouze 🤗

vdemeester · 2018-02-16T09:40:29Z

@shouze @dnephin @thaJeztah as I understand it's a bit tied to #294 and other build/build session enhancement. Given the activity, should we close this one and consolidate the work of all this in one PR ? ⚓️

dnephin · 2018-02-16T16:16:39Z

I think this is a bug fix not a cleanup.

Needs a rebase, I think we've updated the dependencies already, so maybe the PR will be smaller after the rebase.

dnephin · 2018-02-16T16:17:59Z

But I guess there was no response from @shouze since the ping in Oct, so we can close for now.

If you're still interested in contributing this fix please let us know and we can re-open the PR.

tonistiigi · 2018-02-16T18:40:42Z

@thaJeztah @dnephin It is a big problem that we can't get these trivial fixes that are essentially a single line fix in anymore. The vendored component is already unit tested. With previous repository layout it would have been trivial to add an integration test that checks the actual feature (btw I think we still don't have a test for that although the original issue in moby is closed - we only have some tests that run internal functions with mock wrappers in a different repo). Atm we only have a caller that doesn't really have any functionality at all and we expect to mock all the functionality around that caller. It is not reasonable nor will improve quality.

dnephin · 2018-02-16T18:52:30Z

I agree it's an issue. Which is why we need to merge #294 to fix it.

Atm we only have a caller that doesn't really have any functionality at all and we expect to mock all the functionality around that caller.

What is the caller?

tonistiigi · 2018-02-16T18:58:01Z

What is the caller?

Code that only takes configuration passed by user and calls to other packages with public go/http API, then takes the response from these calls and shows to the user.

dnephin · 2018-02-16T19:23:29Z

That sounds like functionality to me.

Build also does a lot more than that.

thaJeztah · 2018-02-17T02:14:05Z

Is someone able to carry this?

shouze · 2018-02-20T18:30:48Z

@dnephin @tonistiigi @thaJeztah @vdemeester sorry guys I was on holiday past 3 weeks, Yes this PR should be closed anyway, as I wasn't able to push a decent test, this was not trivial to test against buildkit the way runBuild was done.

koreno · 2020-07-29T14:48:32Z

What's the latest on this issue - is there a workaround/solution to cache busting due to ownership change?
I'm currently on docker 19.3

GordonTheTurtle added the status/0-triage label Sep 22, 2017

vdemeester added status/2-code-review and removed status/0-triage labels Sep 22, 2017

vdemeester requested review from dnephin, simonferquel and tonistiigi September 22, 2017 09:27

shouze mentioned this pull request Sep 22, 2017

vendor: update fsutil moby/buildkit#126

Merged

dnephin reviewed Sep 22, 2017

View reviewed changes

GordonTheTurtle added dco/no and removed dco/no labels Sep 25, 2017

shouze added 4 commits September 26, 2017 18:11

vendor: update buildkit

1f66bf2

(and fsutil) Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

build: update session usage after vendor

5b58a95

Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

build: reset uid/gid to 0 in stream mode

49c38d0

This is a follow up of #513 for stream mode. Fixes cache busting issues in the build context Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

build: fix the SyncedDir

b57dd17

Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

GordonTheTurtle added the dco/no label Sep 26, 2017

shouze commented Sep 26, 2017

View reviewed changes

build: test addDirToSession

34a6abe

Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>

GordonTheTurtle removed the dco/no label Sep 26, 2017

docker deleted a comment from GordonTheTurtle Feb 16, 2018

dnephin closed this Feb 16, 2018

Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY in stream mode #549

Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY in stream mode #549

Uh oh!

Conversation

shouze commented Sep 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shouze commented Sep 22, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin Sep 22, 2017

Choose a reason for hiding this comment

Uh oh!

shouze Sep 25, 2017

Choose a reason for hiding this comment

Uh oh!

shouze commented Sep 25, 2017

Uh oh!

shouze commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Sep 25, 2017

Uh oh!

shouze commented Sep 25, 2017

Uh oh!

shouze commented Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Sep 26, 2017

Uh oh!

shouze Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Sep 26, 2017

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Sep 29, 2017

Uh oh!

shouze commented Sep 30, 2017

Uh oh!

thaJeztah commented Oct 1, 2017

Uh oh!

thaJeztah commented Oct 31, 2017

Uh oh!

vdemeester commented Feb 16, 2018

Uh oh!

dnephin commented Feb 16, 2018

Uh oh!

dnephin commented Feb 16, 2018

Uh oh!

tonistiigi commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnephin commented Feb 16, 2018

Uh oh!

tonistiigi commented Feb 16, 2018

Uh oh!

dnephin commented Feb 16, 2018

Uh oh!

thaJeztah commented Feb 17, 2018

Uh oh!

shouze commented Feb 20, 2018

Uh oh!

koreno commented Jul 29, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

shouze commented Sep 22, 2017 •

edited

Loading

shouze commented Sep 25, 2017 •

edited

Loading

shouze commented Sep 25, 2017 •

edited

Loading

codecov-io commented Sep 26, 2017 •

edited

Loading

tonistiigi commented Feb 16, 2018 •

edited

Loading