Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Fixing ‘docker cp’ to allow new target file name in a host symlinked directory #31993
‘docker cp’ fails with error ‘not a directory’ if a file is copied from a container to a host symlinked directory with a new target filename.
The fix is to use os.Stat instead of os.Lstat since os.Stat follows the linked path.
Fixes Issue #31816
Signed-off-by: Douglas Curtis firstname.lastname@example.org
- What I did
I changed os.Lstat to os.Stat in CopyInfoDestinationPath.
- How I did it
- How to verify it
After fixing the issue the test case passed as expected.
Manual functional test:
- Description for the changelog
Added isDirectory method to detect symlink directories in CopyInfoDestinationPath to enable copying to a new target file name located in a symlinked host directory.
This looks fine to me. AFAICT this is only used by the docker CLI for
I wonder if it makes sense to just move this over to the cli repo since this function is only using exported functions/types anyway.
@tonistiigi I see you self-assigned this. Does this LGTY?
Unfortunately rebasing will not be sufficient as first the archive package would need to be updated here. Then the CLI updated, then have the CLI commit updated here... but we aren't updating the CLI commit anymore (it's frozen and should not be updated). The functionality is in the docker CLI and can't really be tested here. Also I think why I think it might make sense to move this function out of archive and into the CLI repo... But in any case, regardless of where this function lives, it can be tested with a unit test.…
On Mon, Jun 19, 2017 at 8:42 AM, Douglas Curtis ***@***.***> wrote: I'm a new contributor and still getting used to the processes. Should I rebase my branch and fix tests as needed? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#31993 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAwxZhZicV-bPv7Inv-SO3atripjiiNXks5sFpb8gaJpZM4MkrSZ> .
-- - Brian Goff
Please sign your commits following these rules:
$ git clone -b "issue-31816" email@example.com:dccurtis/docker.git somewhere $ cd somewhere $ git rebase -i HEAD~842353533496 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.
@thaJeztah Thanks, I think your test suggestion looks like a good start but first check my understanding of what’s happened since my original PR. I think this will have to go into the docker/cli repo since the moby/moby CopyInfoDestinationPath seems to be dead code.
Let me explain:
I manually tested the same os.Lstat -> os.Stat change in docker/cli and it fixes the problem.
As for me declaring the moby/moby copy.go logic dead code. I added asserts all over the moby/moby copy.go file and all of the existing ‘cp’ tests pass. This is what lead me to discover the docker/cli repo in the first place. So I don’t know this with 100% confidence but it seems pretty obvious that its not running in the integration-cli tests anymore.
So if this is how things are working today; I think to solve this and cleanup the code we would have to:
Anyway, let me know what you think and I can start picking off some of these items. Also, any news on the security assessment that you mentioned earlier?
Ah, I see the confusion. Let me try to explain (in a rather schizophrenic way, as I happen to be both a maintainer for the Moby project, and employed by Docker
What happened with the CLI split, is that the codebase for the CLI is indeed in a separate repository. The Docker CLI is not part of the Moby project, just like many other tools that use the remote API.
All existing integration tests that use the Docker CLI are kept. However, the version of the Docker CLI that is used to run them is fixated to the version that was used at the time that the Docker CLI moved to its own repository.
Basically, those tests are used to verify that what worked keeps working, and that new features that are added, removed or changed in this repository, don't break those tests. It's possible changes break newer versions of the Docker CLI, or break something that isn't tested; it's Docker's responsibility to run integration tests with newer versions of the CLI, and if something broke, to report it here (or provide a fix). Given that those breaking changes would be either due to a change in the API, or due to a change in behavior of a package, testing for that can all be covered by API tests, or unit tests of the package
The code from this repository is still used
The Docker CLI uses various packages that are in this repository as a dependency (as well as many other packages from other repositories). You can find all dependencies in the
For example, the vendored version of the file that's being changed in this pull request can be found in vendor/github.com/docker/docker/pkg/archive/copy.go.
Files are vendored (not downloaded at compile time) so that builds are always reproducible, and don't depend on repositories we don't control (repositories may go down, someone can do a "force push" and mess things up, and repositories may move
A tool (LK4D4/vndr) is used to "vendor" those dependencies that. Vendoring involves fetching the exact git-commit (or tag) of the dependency as specified in the vendor.conf configuration file. After that, the tool analyzes all import statements, and removes all files that are not used, including test files (as described earlier, making sure a package is tested is a responsiblity of the package author / repository).
If you look at the
So how to get this fix into the Docker CLI?
First, the PR needs to be finished
Once the PR is merged, a "vendor bump" PR can be opened against the Docker CLI repository to bring the actual changes into the Docker CLI. A vendor PR basically is;
Hope this helps!
Sep 19, 2017
6 checks passed
@thaJeztah Can you give me a pointer on how to resolve this error that I'm getting with vndr? Thanks!