Fix cp tar stream support for stdin and stdout - #1947
Conversation
|
It looks like this implementation is acting more as a wrapper around the existing One of the main reasons I'd want streaming tar support in There are also implications on path length limitations in the tar files. Because the contents are fully serialized/deserialized on the host machine in a temp folder, contents with a valid path length in the archive would fail to copy due to exceeding the maximum path length when extracted under a temp folder. This is definitely more of an edge case, but still one that could trip up users. Ideally the tar streaming implementation would provide as close to 1:1 parity with Docker and Podman behavior for the same feature as possible. |
Thanks for the detailed feedback, this makes sense and I agree. Rework target checklist
|
The first pass wrapped the existing path based cp: it shelled out to /usr/bin/tar, unpacked the stdin archive into a host temp directory, then copied the extracted files in, and did the reverse on the way out. That gave up the two things tar streaming is for. Ownership and mode came from whatever the host filesystem allowed rather than from the tar headers, and every entry had to fit the host's path length limits under a temp directory even when its path was valid inside the archive. Hand the descriptor down instead. `cp -` now passes the CLI's stdin or stdout through both XPC hops on a new archiveFd key, using the same file handle passing that dial and logs already rely on, to LinuxContainer's stream based copyIn(archive:)/copyOut(to:). The bytes go straight to the guest over vsock and the guest extracts them as root, so the ownership, mode and symlink targets in the headers are applied verbatim and nothing is unpacked on the host. Path traversal is rejected during extraction, where the archive is actually read, rather than by pre-scanning entry names on the host. Each hop closes the descriptor it owns and duplicates for the outgoing message. Setting a file handle on an XPC message closes the descriptor it is given, and the container lookups on the receiving side can throw before the handle is ever forwarded, which otherwise leaked an fd in the apiserver on every copy to a stopped container. Behavior now follows docker and podman: output is uncompressed tar named relative to the source's parent so the source's basename is the top level entry, for files as well as directories, and input may be uncompressed or gzip, bzip2 or xz compressed. The integration tests assert metadata rather than content: a stream carrying uid, gid and modes the host could not reproduce unprivileged, a traversal entry that must not escape, entry naming on the way out, and a round trip. Requires the stream based copy API from apple/containerization#812; the containerization pin still needs bumping once that lands.
164fef8 to
75fa939
Compare
Resolves the import collision in TestCLICopyCommand and adopts the warmup image API the tests were moved to on main: - keep both ContainerTestSupport and ContainerizationArchive imports; the former is where the fixture moved, the latter is used to build tar streams with ownership and modes the host cannot reproduce unprivileged - WarmupImage.alpine320.rawValue replaces f.copyWarmupImage(ContainerFixture.warmupImages[0]) main also moved the containerization pin from 0.37.0 to 0.40.1.
283a657 to
99cf494
Compare
|
@danegsta You were right on all three counts, and reworking it surfaced two more parity gaps I'd otherwise have shipped. No more host staging. The Path-traversal rejection also moved into the guest extractor, where the archive is actually parsed, rather than pre-scanning entry names on the host. Two further parity gaps found while doing this, both fixed in the containerization PR:
Still not 1:1. Measured against this branch, not inferred:
These all live in the guest extractor. mtime, xattrs and hardlinks are outside the initial scope listed in #1908, so I've left them for now — happy to pick any of them up here or as follow-ups, whichever you prefer. The one I'd least want to leave is the silent skip: dropping a hardlink while exiting 0 is worse than failing, and erroring there is a small change. Verification: 42/42 in Ordering: this depends on apple/containerization#812 for the stream API. CI here stays red until that lands and the pin moves off |
Summary
Add
docker cp -/podman cp -style tar streaming tocontainer cp, using-as a stdin/stdout endpoint. Closes #1908.Why
The point of tar streaming is exact control over ownership and mode: the tar headers, not host filesystem metadata, decide what lands in the container. That makes
cp -usable as a dynamic image layer — injecting certs and config with the right uid/gid/mode — which is the use case in #1908.Approach
The descriptor is passed down, not the contents.
cp -sends the CLI's stdin or stdout through both XPC hops on a newarchiveFdkey, reusing the file-handle passing thatdialandlogsalready rely on, intoLinuxContainer's stream-basedcopyIn(archive:)/copyOut(to:). The bytes reach the guest over vsock and the guest extracts them as root.Consequences:
tarbeing present locally.An earlier revision of this PR unpacked to a host temp dir and shelled out to
/usr/bin/tar; that gave up all four properties and was reworked after review.Behaviour
container cp CONTAINER:/path -writes an uncompressed tar to stdout, entries named relative to the source's parent so the source's basename is the top-level entry, for files as well as directories.container cp - CONTAINER:/pathreads a tar from stdin and extracts it into the destination. Uncompressed, gzip, bzip2 and xz inputs are all accepted.Known deviations from docker
Measured, not inferred. All are in the guest extractor:
lchowncp - CONTAINER:/nonexistentmtime, xattrs and hardlinks are outside the initial scope in #1908. Flagging rather than hiding them.
Testing
TestCLICopyCommand: 42/42, run against a locally builtvminitdand initfs. Six new tests assert metadata rather than content:statinside the container..entry that must not escape the destinationcp - /local/pathis rejectedDependency
Requires the stream copy API from apple/containerization#812. CI will fail here until that lands and
scVersionmoves off0.40.1.