Fix COPY --from=stage when running with user-namespaces #1448
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Issue was reported in moby/moby issue 34645, and relates to moby/moby PR 38599,
which addressed this issue for the classic builder.
When copying files between stages, file ownership should be preserved;
FROM busybox:latest as build
RUN touch x && chown 1:1 x
FROM busybox:latest
RUN touch y && chown 1:1 y
COPY --from=build x ./
However, when running with user namespaces enabled:
# grep dockremap /etc/sub*id
/etc/subgid:dockremap:500000:65536
/etc/subuid:dockremap:500000:65536
A build would fail, because BuildKit is trying to map the container's UID/GID
to the host:
ERROR: failed to copy file info: failed to chown /var/lib/docker/500000.500000/overlay2/mu5zpgelkig84eo9rlbqdhnn9/merged/x: Container ID 500001 cannot be mapped to a host ID
When looking at the code, I found that `(fb *Backend) Copy()` takes a `action`
argument (`pb.FileActionCopy`), one of its options is wether or not the owner
should be overridden:
// optional owner override
Owner *ChownOpt `protobuf:"bytes,3,opt,name=owner,proto3" json:"owner,omitempty"`)
That argument is passed on to `docopy()` (which is called from `(fb *Backend) Copy()`),
however inside `docopy()`, user-mapping is performed, irregardless if `Owner` was set
or not:
ch, err := mapUserToChowner(u, idmap)
if err != nil {
return err
}
opt := []copy.Opt{
func(ci *copy.CopyInfo) {
ci.Chown = ch
This patch makes that step optional, and only performs `mapUserToChowner` if
`action.Owner` is specified.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
@tonistiigi @AkihiroSuda PTAL: no idea if this is the right fix (and if we have testing in place for user-namespaces); it's just from browsing the code and suspecting this might be the cause of the problem. |
tonistiigi
reviewed
Apr 25, 2020
| if err != nil { | ||
| return err | ||
| var ch copy.Chowner | ||
| if action.Owner != nil { |
There was a problem hiding this comment.
this check here definitely isn't correct. First, action (including owner) is already parsed before calling this function and the result *copy.User passed to this function directly (with a nil check in map). Secondly, this object being nil doesn't carry any meaning as it is just a combination of two possibly nil pointers.
This issue looks to be regression from #1342
|
replaced by #1477 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue was reported in moby/moby#34645 (comment), and relates to moby/moby#38599, which addressed this issue for the classic builder.
When copying files between stages, file ownership should be preserved;
However, when running with user namespaces enabled:
# grep dockremap /etc/sub*id /etc/subgid:dockremap:500000:65536 /etc/subuid:dockremap:500000:65536A build would fail, because BuildKit is trying to map the container's UID/GID to the host:
When looking at the code, I found that
(fb *Backend) Copy()takes aactionargument (
pb.FileActionCopy), one of its options is wether or not the ownershould be overridden:
buildkit/solver/pb/ops.pb.go
Lines 1457 to 1458 in 226a5db
That argument is passed on to
docopy()(which is called from(fb *Backend) Copy()), however insidedocopy(), user-mapping is performed, irregardless ifOwnerwas set or not:buildkit/solver/llbsolver/file/backend.go
Lines 182 to 189 in 226a5db
This patch makes that step optional, and only performs
mapUserToChownerifaction.Owneris specified, hopefully addressing the issue.