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

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

Merged
merged 1 commit into from Sep 13, 2017

Conversation

Projects
None yet
6 participants
@shouze
Contributor

shouze commented Sep 8, 2017

- What I did

Fix of moby/moby#32816

- How I did it

By resetting uid/gid to 0/0 in the build context. Prior to that, made possible by adding chownOpts in pkg/archive tarWithOption moby/moby#34590

- How to verify it

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

- Description for the changelog

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

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

133-insolite-25

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 8, 2017

Codecov Report

Merging #513 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   49.06%   49.06%   +<.01%     
==========================================
  Files         200      200              
  Lines       16407    16408       +1     
==========================================
+ Hits         8050     8051       +1     
  Misses       7938     7938              
  Partials      419      419

codecov-io commented Sep 8, 2017

Codecov Report

Merging #513 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #513      +/-   ##
==========================================
+ Coverage   49.06%   49.06%   +<.01%     
==========================================
  Files         200      200              
  Lines       16407    16408       +1     
==========================================
+ Hits         8050     8051       +1     
  Misses       7938     7938              
  Partials      419      419
@shouze

This comment has been minimized.

Show comment
Hide comment
@shouze

shouze Sep 8, 2017

Contributor

@tonistiigi I need to write the test but looks like not so trivial to me ATM as buildCtx is internal concept of runBuild(). Still need to read the code. I also want to address --stream, will I need to make changes inside buildkit?

Contributor

shouze commented Sep 8, 2017

@tonistiigi I need to write the test but looks like not so trivial to me ATM as buildCtx is internal concept of runBuild(). Still need to read the code. I also want to address --stream, will I need to make changes inside buildkit?

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Sep 8, 2017

Member

@dnephin may have some tips for the test. For --stream I need to fix moby/moby#34708 first.

Member

tonistiigi commented Sep 8, 2017

@dnephin may have some tips for the test. For --stream I need to fix moby/moby#34708 first.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 8, 2017

Collaborator

@tonistiigi this would be really easy to test if we merged #294 , it would be a unit test on createBuildContextFromLocalDir()

Collaborator

dnephin commented Sep 8, 2017

@tonistiigi this would be really easy to test if we merged #294 , it would be a unit test on createBuildContextFromLocalDir()

@shouze shouze changed the title from Reset build ctx uid/gid to 0/0 on ADD/COPY to Reset uid and gid to 0 in the build context to avoid cache busting issues on ADD/COPY Sep 9, 2017

@shouze shouze changed the title from Reset uid and gid to 0 in the build context to avoid cache busting issues on ADD/COPY to Reset uid/gid to 0 in build context to fix cache busting issues on ADD/COPY Sep 9, 2017

@shouze

This comment has been minimized.

Show comment
Hide comment
@shouze

shouze Sep 9, 2017

Contributor

@tonistiigi @dnephin ok I've added the test, ready for a review.

@tonistiigi ok so about --stream maybe I can make a dedicated PR as it's still an experimental feature while this PR fixes a legacy issue. WDYT?

Contributor

shouze commented Sep 9, 2017

@tonistiigi @dnephin ok I've added the test, ready for a review.

@tonistiigi ok so about --stream maybe I can make a dedicated PR as it's still an experimental feature while this PR fixes a legacy issue. WDYT?

@shouze

This comment has been minimized.

Show comment
Hide comment
@shouze

shouze Sep 10, 2017

Contributor

@tonistiigi I've started moby/moby#34708 in moby/moby#34797 but looks like it's probably just more than an update, I'll take a look as a fsutil upgrade seems required too.

Contributor

shouze commented Sep 10, 2017

@tonistiigi I've started moby/moby#34708 in moby/moby#34797 but looks like it's probably just more than an update, I'll take a look as a fsutil upgrade seems required too.

@dnephin

Thanks! The test is looking good, but I have a couple suggestions for making it more readable.

The gotestyourself/fs helpers are new, so they weren't used on the existing test. I'll have to go back and update it to use the new functions.

Show outdated Hide outdated cli/command/image/build_test.go Outdated
Show outdated Hide outdated cli/command/image/build_test.go Outdated
Show outdated Hide outdated cli/command/image/build_test.go Outdated
@shouze

This comment has been minimized.

Show comment
Hide comment
@shouze

shouze Sep 12, 2017

Contributor

@dnephin ready for a new review, thx for gotestyourself tips, it reduces a lot the noise when we read the test 👍

I can upgrade/modernize the other tests in another PR if you want.

Contributor

shouze commented Sep 12, 2017

@dnephin ready for a new review, thx for gotestyourself tips, it reduces a lot the noise when we read the test 👍

I can upgrade/modernize the other tests in another PR if you want.

@dnephin

The test looks great, Thanks!

LGTM

Show outdated Hide outdated cli/command/image/build_test.go Outdated
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Sep 12, 2017

Member

LGTM

@shouze Needs rebase

Member

tonistiigi commented Sep 12, 2017

LGTM

@shouze Needs rebase

Reset idPair during build to avoid cache busting
Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>
@shouze

This comment has been minimized.

Show comment
Hide comment
@shouze

shouze Sep 13, 2017

Contributor

@tonistiigi thx, rebased!

Contributor

shouze commented Sep 13, 2017

@tonistiigi thx, rebased!

@shouze

This comment has been minimized.

Show comment
Hide comment
@shouze

shouze Sep 13, 2017

Contributor

@dnephin @tonistiigi I guess that after the merge a documentation upgrade would be required?

Contributor

shouze commented Sep 13, 2017

@dnephin @tonistiigi I guess that after the merge a documentation upgrade would be required?

@dnephin dnephin merged commit 7b77ab5 into docker:master Sep 13, 2017

8 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 50%)
Details
codecov/project 49.06% (+<.01%) compared to b604132
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.10.0 milestone Sep 13, 2017

@shouze shouze deleted the shouze:reset-id-pair-during-build-to-avoid-cache-busting branch Sep 13, 2017

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 14, 2017

Member

/cc @mstanleyjones for documentation ^^

Member

thaJeztah commented Sep 14, 2017

/cc @mstanleyjones for documentation ^^

shouze added a commit to shouze/cli that referenced this pull request Sep 22, 2017

build: reset uid/gid to 0 in stream mode
This is a follow up of docker#513 for stream mode.
Fixes cache busting issues in the build context

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

shouze added a commit to shouze/cli that referenced this pull request Sep 25, 2017

build: reset uid/gid to 0 in stream mode
This is a follow up of docker#513 for stream mode.
Fixes cache busting issues in the build context

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

shouze added a commit to shouze/cli that referenced this pull request Sep 26, 2017

build: reset uid/gid to 0 in stream mode
This is a follow up of docker#513 for stream mode.
Fixes cache busting issues in the build context

Signed-off-by: Sébastien HOUZÉ <cto@verylastroom.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment