Fix windows path handling in podman cp

[danegsta]

This PR fixes a handful of Windows path bugs in podman container cp that prevented copying files from (or to) a Windows host.

• The Windows drive letter was being treated as a container ID/name, which is fixed by using filepath.IsAbs to determine if a given path is an absolute path on the system (on Windows, this is true for paths starting with a single drive letter and a colon).
• The code used / as the root folder path, even on Windows (should be <drive letter>:\ (i.e. C:\), which caused copier to reject the given path. This was fixed by depending on the behavior of filepath.VolumeName and os.PathSeparator to build the correct root path for a given OS.
• There was logic that was hardcoded to use / as the path separator rather than os.PathSeparator which caused path normalization to fail. Fixed by using the os specific separator.
• There was a bug (now fixed) in the copier package in containers/buildah that would cause an infinite loop when trying to process files from a Windows path. I updated the vendored dependency to the latest version of buildah.
• Some methods in cp.go used filepath for parsing paths within containers, which causes issues on Windows. Switched to using path for processing paths within containers to force Linux style conventions.

I also refactored the e2e test coverage that was added for podman container cp (moved the existing test from basic_test.go to cp_test.go alongside the podman machine cp test coverage) and added a few new test cases.

Does this PR introduce a user-facing change?

None

[packit-as-a-service]

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

[danegsta]

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

I’d accidentally downgraded github.com/containers/common when rebasing my work on main; fixed the issue in my latest push.

[l0rd]

@danegsta thank you! I believe changes to go.mod and the vendor folder should not be part of this PR.

[Luap99]

@flouthoc new buildah tests seems broken when run with podman build, can you take a look?

[Luap99]

@danegsta thank you! I believe changes to go.mod and the vendor folder should not be part of this PR.

AFAICT this depends on the buildah bump due the bug fix in the copier package so the bump here should be fine.
That said generally I prefer if the vendor bump is a separate commit to make the actual podman changes easier to review.

[danegsta]

@danegsta thank you! I believe changes to go.mod and the vendor folder should not be part of this PR.

AFAICT this depends on the buildah bump due the bug fix in the copier package so the bump here should be fine. That said generally I prefer if the vendor bump is a separate commit to make the actual podman changes easier to review.

Yes, the bug fix in the copier package needs to be picked up via upgraded buildah version before the fixes to the cp command can be made (otherwise with the original version of buildah the command will get stuck in an infinite loop while trying to resolve the files to be copied).

Would you prefer an entirely separate PR for the buildah version update or should I restructure this PR into two separate commits?

[flouthoc]

@Luap99 The new test which I added at buildah containers/buildah#6087 expects 128 to be the exit code but it seems buildah-tests.diff overrides it with 125 here https://github.com/containers/podman/blob/main/test/buildah-bud/buildah-tests.diff#L118

I can add another hack in buildah-tests.diff for this test or I can fix in buildah to return 125 in case of error, fixing this at buildah end is more suitable i guess. WDYT ?

[Luap99]

I can add another hack in buildah-tests.diff for this test or I can fix in buildah to return 125 in case of error, fixing this at buildah end is more suitable i guess. WDYT ?

I don't think 128 is a particular great exit code. 125 seems to better for consistency with other errors so I would think this in buildah if that is easy.

[Luap99]

@flouthoc Note there is also a second issue:

[+0848s] not ok 426 build-with-timestamp-applies-to-oci-archive
[+0848s] # (from function `die' in file tests/helpers.bash, line 523,
[+0848s] #  from function `run_buildah' in file tests/helpers.bash, line 510,
[+0848s] #  in test file tests/bud.bats, line 7494)
[+0848s] #   `run_buildah build -f <(echo 'FROM scratch') --tag=oci-archive:${outpath}.a --timestamp 0' failed
[+0848s] # /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.39.1-0.20250326204000-898fbb2d25c6/tests /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.39.1-0.20250326204000-898fbb2d25c6
[+0848s] # # /var/tmp/go/src/github.com/containers/podman/bin/podman --root /var/tmp/buildah_tests.gseiis/root --runroot /var/tmp/buildah_tests.gseiis/runroot --storage-driver vfs --registries-conf /var/tmp/go/src/github.com/containers/podman/test-buildah-v1.39.1-0.20250326204000-898fbb2d25c6/tests/registries.conf system service [...] unix:///run/podman/podman-426.sock
[+0848s] # # podman-remote build --force-rm=false --layers=false -f /dev/fd/63 --tag=oci-archive:/var/tmp/buildah_tests.gseiis/timestamp-oci.tar.a --timestamp 0
[+0848s] # time="2025-03-27T04:59:20-05:00" level=error msg="1 error occurred:\n\t* readlink /proc/180077/fd/8: no such file or directory\n\n\n"
[+0848s] # Error: Post "http://d/v5.5.0/libpod/build?compatvolumes=0&dockerfile=%5B%2263%22%5D&httpproxy=1&identitylabel=1&idmappingoptions=%7B%22HostUIDMapping%22%3Atrue%2C%22HostGIDMapping%22%3Atrue%2C%22UIDMap%22%3A%5B%5D%2C%22GIDMap%22%3A%5B%5D%2C%22AutoUserNs%22%3Afalse%2C%22AutoUserNsOpts%22%3A%7B%22Size%22%3A0%2C%22InitialSize%22%3A0%2C%22PasswdFile%22%3A%22%22%2C%22GroupFile%22%3A%22%22%2C%22AdditionalUIDMappings%22%3Anull%2C%22AdditionalGIDMappings%22%3Anull%7D%7D&isolation=0&jobs=1&networkmode=0&nsoptions=%5B%7B%22Name%22%3A%22user%22%2C%22Host%22%3Atrue%2C%22Path%22%3A%22%22%7D%5D&omithistory=0&output=oci-archive%3A%2Fvar%2Ftmp%2Fbuildah_tests.gseiis%2Ftimestamp-oci.tar.a&outputformat=application%2Fvnd.oci.image.manifest.v1%2Bjson&pullpolicy=missing&retry=3&retry-delay=2s&rm=1&seccomp=%2Fusr%2Fshare%2Fcontainers%2Fseccomp.json&shmsize=204800&t=oci-archive%3A%2Fvar%2Ftmp%2Fbuildah_tests.gseiis%2Ftimestamp-oci.tar.a&timestamp=0": io: read/write on closed pipe
[+0848s] # [ rc=125 (** EXPECTED 0 **) ]
[+0848s] # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
[+0848s] # #| FAIL: exit code is 125; expected 0
[+0848s] # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[+0848s] # teardown: stopping podman server 180008

[danegsta]

@Luap99 it sounds like additional changes need to make it into buildah before it can be updated. Seems like it probably makes more sense to have a separate PR for updating buildah.

I’ll break this PR up into separate commits for the podman and buildah changes (so that it can be reviewed in the meantime), but once buildah is updated independently I’ll undo my go.mod and vendor changes and rebase off of main.

[Luap99]

but once buildah is updated independently I’ll undo my go.mod and vendor changes and rebase off of main.

Sounds good.

[flouthoc]

@flouthoc new buildah tests seems broken when run with podman build, can you take a look?

Created a PR for first one containers/buildah#6092

and for second test build-with-timestamp-applies-to-oci-archive needs to be skipped at remote because compat API does not support tags to oci-archive , infact API does not knows yet how to handle cases where no tags is produced.

[danegsta]

Looks like #14862 should be fixed by this PR.

[danegsta]

@flouthoc looks like containers/buildah#6092 was merged; I've opened #25756 to bump the vendored buildah version to the latest commit from main.

I'll rebase this PR with only the cp command changes once that PR gets merged.

[danegsta]

Rebased on main with just the podman cp fixes now that buildah has been updated.

[danegsta]

Rebased on latest main

[danegsta]

/retest

[openshift-ci]

@danegsta: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[danegsta]

@Luap99 did you have any other concerns with the PR? I wanted to make sure this doesn't fall off the radar.

There's one failing test, but it seems like it should be a flaky test as I've rebased a couple times and it seems that a different random test fails each time the tests restart (also the current failing test doesn't look like it should be hitting any of the code changed in this PR).

[Luap99]

Some comments on the test, sorry I should have looked closer before.

Overall I don't use windows so I am not the best person to review/understand the code but it makes sense to me at least. @l0rd Can you do a review here?

[Luap99]

LGTM, thank you
@l0rd PTAL

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danegsta, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Luap99]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[l0rd]

@danegsta one test is failing on both WSL and Hyper-V:

STEP: copy from host to container by name - C:/Users/Administrator/AppData/Local/cirrus-ci-build/repo[/pkg/machine/e2e/cp_test.go:110](https://github.com/containers/podman/blob/24d7eaf6879059239b17ee593015ee1e3cb4aea9/pkg/machine/e2e/cp_test.go#L110) @ 04/08/25 18:32:23.41
  C> podman.exe -r cp Z:\ginkgo1275841403\foo.txt podman-cp-test:/tmp/rename.txt
  Error: "/tmp/rename.txt" could not be found on container podman-cp-test: no such file or directory

  [FAILED] Expected
      <int>: 125
  to match exit code:
      <int>: 0
  In [It] at: C:/Users/Administrator/AppData/Local/cirrus-ci-build/repo[/pkg/machine/e2e/cp_test.go:114](https://github.com/containers/podman/blob/24d7eaf6879059239b17ee593015ee1e3cb4aea9/pkg/machine/e2e/cp_test.go#L114) @ 04/08/25 18:32:23.691

  Full Stack Trace
    github.com/containers/podman/v5/pkg/machine/e2e_test.init.func8.1()
    	C:/Users/Administrator/AppData/Local/cirrus-ci-build/repo[/pkg/machine/e2e/cp_test.go:114](https://github.com/containers/podman/blob/24d7eaf6879059239b17ee593015ee1e3cb4aea9/pkg/machine/e2e/cp_test.go#L114) +0x1f22

I don't think this is a bug introduced by this PR though. You can either file a new issue and update this PR to skip that particular test on Windows or try to fix it in this PR.

You can reproduce the error running the tests locally on Windows:

./winmake localmachine "cp_test.go"

[l0rd]

There's one failing test, but it seems like it should be a flaky test as I've rebased a couple times and it seems that a different random test fails each time the tests restart (also the current failing test doesn't look like it should be hitting any of the code changed in this PR).

Yes this is annoying. I have been there too. Don't hesitate to ask us to re-run a specific test when it happens again.

[danegsta]

I don't think this is a bug introduced by this PR though. You can either file a new issue and update this PR to skip that particular test on Windows or try to fix it in this PR.

This is what I get for updating my test cases for a Windows focused PR on a Mac. The new failing test was a pretty easy fix; containerParentDir was using filepath to parse container specific paths when it should probably be using path to force Linux style path handling.

Edit: just took a closer look and this seems to be a problem in a few other places in cp.go. I'm going to try and fix them and add additional test coverage for container to container copy.

[l0rd]

Tests are green. Great job, @danegsta.

About your last change: it's usually preferable to use path/filepath rather than path for manipulating file paths. However container paths must be consistent across OSes (using forward slashes, even when the code is run on Windows) and using path should be okay in this case. cc @Luap99

/lgtm
/hold

[l0rd]

/unhold