search

docker / app

Make your Docker Compose applications reusable, and share them on Docker Hub
Apache License 2.0
1.57k stars 177 forks source link

Use standard archive/tar instead of docker's untar implementation #435

Closed ulyssessouza closed 5 years ago

ulyssessouza commented 5 years ago

!!! Note that this PR is implemented based in another !!! The only 2 exclusive files on this PR are:

- What I did Substitute docker's untar package with a standard golang implementation

- How I did it Adapted an implementation of untar to fit our needs and use it in TestPackInvocationImageContext

- How to verify it Tests on packing_test.go passing without the need of running inside a container (just with make test and not make -f docker.Makefile test)

- Description for the changelog Substitute docker's untar package with a standard golang implementation

- A picture of a cute animal kitty_orig

ulyssessouza commented 5 years ago

@vdemeester The long story is that:

What are the reasons behind moving away form github.com/docker/docker/pkg/archive ?

The idea is to be able to run this test with just a make test and not make -f docker.Makefile test the first runs in the host and the second will be executed inside a container. The problem of using github.com/docker/docker/pkg/archive here is that it assumes it's inside a container. Like when assuming that infos about UID and GID are located in /etc/subuid and /etc/subgid (as in idtools.go:L33-34)

If it's related to lchown, I'm pretty sure using TarOptions field NoLchown should work.

That was my first guess. The problem here is that according to idtools.go:L55 it not chown the exisiting files, but will systematically do it for the new ones. The TarOptions::NoLchown is just used in archive.go:L651 that will prevent the os.Lchown in archive.go:L655 to be executed, but not the firstly described.

If not, I still believe it's better to fix/contribute upstream (it's even in the same docker org…) that duplicate this code.

My concern with this is that from what it looks, github.com/docker/docker/pkg/archive was built to be used in a different environment, which seems to be inside a container where it can read /etc/sub{uid|gid} to retrieve the user and group. Without this assumption, it should retrieve the username and group for different OSs, and also docker-app is compiled without CGO, so even a user.Current() work. So all of this makes me think its not buggy, but just thought to be used in a different context.

Given that, one possibility of contribution for the upstream would be add a new func in idtools.go which is similar to MkdirAllAndChownNew (given that it's a public func I cannot just change it's signature) that tells mkdirAs (having as side-effect the change of it's signature) to not chown even the new files. That would work.

Talking with @silvin-lubecki, he came with a simplier solution, that consists in just iterate through the entries in the archive, instead of really extracting and iterating through it. This solution seems to be cleaner for our needs.

thaJeztah commented 5 years ago

We should probably be careful replacing this package; there may be special cases for backward compatibility in this package @tonistiigi @dmcgowan

ulyssessouza commented 5 years ago

@vdemeester And what about now? I just pushed a simple solution described in the last paragraph of last message.

silvin-lubecki commented 5 years ago

The build fails

[Build Invocation image] Failed in branch Build Invocation image
[Validate] (84/87) Failed to write github.com/containerd/containerd@v1.1.2
[Validate] (85/87) Failed to write k8s.io/client-go@kubernetes-1.11.1
[Validate] (86/87) Failed to write github.com/coreos/etcd@v3.3.8
[Binaries] GOOS=darwin CGO_ENABLED=0 go build -tags="" -ldflags="-s -w -X github.com/docker/app/internal.GitCommit=9d03e79 -X github.com/docker/app/internal.Version=9d03e790c7 -X github.com/docker/app/internal.Experimental=off -X github.com/docker/app/internal.BuildTime=2018-12-28T12:18:51Z" -o bin/docker-app-darwin ./cmd/docker-app
[Validate] (87/87) Failed to write k8s.io/kubernetes@v1.11.1
[Validate] grouped write of manifest, lock and vendor: error while writing out vendor tree: failed to write dep tree: failed to export github.com/deis/duffle: 
[Validate]  (1) failed to list versions for ssh://git@github.com/simonferquel/duffle.git: fatal: cannot run ssh: No such file or directory
[Validate] fatal: unable to fork
[Validate] : exit status 128
[Validate] 
[Validate] make: *** [Makefile:93: vendor] Error 1
[Validate] docker.Makefile:106: recipe for target 'vendor' failed
[Validate] make: *** [vendor] Error 2
GordonTheTurtle commented 5 years ago

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 "std-untar" git@github.com:ulyssessouza/app.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842357813216
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

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

codecov[bot] commented 5 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (cnab-preview@133a2c6). Click here to learn what that means. The diff coverage is 48.42%.

Impacted file tree graph

@@               Coverage Diff               @@
##             cnab-preview     #435   +/-   ##
===============================================
  Coverage                ?   59.04%           
===============================================
  Files                   ?       56           
  Lines                   ?     2803           
  Branches                ?        0           
===============================================
  Hits                    ?     1655           
  Misses                  ?      931           
  Partials                ?      217
Impacted Files Coverage Δ
cmd/docker-app/init.go 100% <ø> (ø)
internal/names.go 100% <ø> (ø)
types/metadata/metadata.go 100% <ø> (ø)
types/parameters/opts.go 100% <ø> (ø)
cmd/docker-app/image-add.go 0% <0%> (ø)
cmd/docker-app/cnab.go 0% <0%> (ø)
cmd/docker-app/validate.go 88.88% <100%> (ø)
internal/renderer/renderer.go 100% <100%> (ø)
internal/packager/parameter.go 100% <100%> (ø)
loader/loader.go 79.24% <100%> (ø)
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 133a2c6...6c84a2b. Read the comment docs.

ulyssessouza commented 5 years ago

Closing it since it has been merged by #443