Add wslc container cp command for tar archive upload#40835
Conversation
Implements 'wslc container cp - CONTAINER:PATH' to copy a tar archive
from stdin into a running container via Docker's PUT /containers/{id}/archive API.
Usage: tar.exe -cf - files | wslc container cp - my_container:/dest
Changes across all layers:
- IDL: Added UploadArchive to IWSLCContainer
- DockerHTTPClient: Added PutArchive method (omits Content-Length for pipes)
- WSLCContainerImpl: Relay stdin to Docker socket with SD_SEND on EOF
- ContainerService: Added CopyToContainer static method
- ContainerTasks: Added CopyToContainer task with CONTAINER:PATH parsing
- ContainerCpCommand: New command registered under 'container cp'
- Localization: Added all user-facing strings
- Tests: Added CLI parsing test cases and updated e2e command list
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR adds a new wslc container cp subcommand that uploads a tar archive from stdin into a running container by relaying stdin to Docker’s PUT /containers/{id}/archive endpoint, with supporting wiring through the CLI task/service layers and new localized help/error strings.
Changes:
- Adds a new
container cpcommand and CLI task/service method to parseCONTAINER:PATHand stream stdin into the container. - Extends the Docker HTTP client with a
PutArchiverequest helper used by the session/container implementation. - Updates tests (CLI parsing + e2e container command list) and adds localized user-facing strings for the new command.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/wslc/e2e/WSLCE2EContainerTests.cpp | Adds cp to the container subcommand help list assertions. |
| test/windows/wslc/CommandLineTestCases.h | Adds command-line parsing coverage for container cp. |
| src/windows/wslcsession/WSLCContainer.h | Declares new UploadArchive APIs on the container implementation and COM wrapper. |
| src/windows/wslcsession/WSLCContainer.cpp | Implements archive upload by relaying stdin to Docker and handling HTTP responses/errors. |
| src/windows/wslcsession/DockerHTTPClient.h | Declares PutArchive for Docker archive upload. |
| src/windows/wslcsession/DockerHTTPClient.cpp | Implements PutArchive request creation (optionally omitting Content-Length). |
| src/windows/wslc/tasks/ContainerTasks.h | Declares CopyToContainer task entrypoint. |
| src/windows/wslc/tasks/ContainerTasks.cpp | Implements CONTAINER:PATH parsing and stdin validation/size detection. |
| src/windows/wslc/services/ContainerService.h | Adds CopyToContainer service API. |
| src/windows/wslc/services/ContainerService.cpp | Wires CLI service call to COM IWSLCContainer::UploadArchive. |
| src/windows/wslc/commands/ContainerCpCommand.cpp | Adds the new container cp command and argument definitions. |
| src/windows/wslc/commands/ContainerCommand.h | Declares ContainerCpCommand. |
| src/windows/wslc/commands/ContainerCommand.cpp | Registers container cp under the container root command. |
| src/windows/service/inc/wslc.idl | Adds UploadArchive to IWSLCContainer. |
| localization/strings/en-US/Resources.resw | Adds help text and user-facing error messages for container cp. |
| HRESULT Kill([in] WSLCSignal Signal); | ||
| HRESULT Stats([out] LPSTR* Output); | ||
| HRESULT ConnectToNetwork([in] const WSLCNetworkConnectionOptions* Options); | ||
| HRESULT DisconnectFromNetwork([in] LPCSTR NetworkName); | ||
| HRESULT UploadArchive([in] WSLCHandle TarHandle, [in, string] LPCSTR DestPath, [in] ULONGLONG ContentSize); | ||
| } |
dkbennett
left a comment
There was a problem hiding this comment.
Implementation looks good, but I think this should really have at least 1 E2E test verifying the copy actually works as expected.
Can put it in with one of the other container tests so we dont create an entirely new file for it if necessary, but should have something validating we do not regress this command's functionality in the future and have confidence that it is working correctly.
| { | ||
| std::vector<std::unique_ptr<Command>> commands; | ||
| commands.push_back(std::make_unique<ContainerAttachCommand>(FullName())); | ||
| commands.push_back(std::make_unique<ContainerCpCommand>(FullName())); |
There was a problem hiding this comment.
nit: I'd really like to call this "ContainerCopyCommand" though the actual execution name could still be "cp" but that can be refactored later if it bothers me enough. It's correct though, there is no "copy" in docker but it just looks strange.
|
Just adding a note that this PR will have a conflict with #40832 due to the change from CreateSession (which is poorly named since it does an open or a create default only) to ResolveSession, depending on which one goes in first. If this one goes in first I can fix it in 40832. |
- Add 5 new CLI parsing test cases for cp in CommandLineTestCases.h - Add RunWslcWithStdinFile helper to pipe file contents to wslc stdin - Add WSLCE2EContainerCpTests with 11 e2e test methods covering: - Help output, missing arguments, invalid target formats - Stdin terminal detection, source validation - Container not found error handling - Successful tar upload to running container with exec verification - Copy to stopped container (Docker PUT /archive filesystem operation) - CreateTestTarFile builds minimal POSIX tar at runtime for tests Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Fix ContainerCpDesc to accurately describe stdin-only tar upload (was misleadingly implying bidirectional file copy) - Change PutArchive ContentLength parameter to std::optional<uint64_t> to distinguish 'unknown size' (pipe) from 'known zero size' (empty file) - WSLCContainerImpl::UploadArchive passes std::nullopt when ContentSize is 0 Note: Kept UploadArchive on IWSLCContainer (not a separate IWSLCContainer2) because wslc interfaces are internal — client and server are always deployed together from the same build, so there is no ABI compatibility concern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed code review feedback:
|
| WSLCExecutionContext context(&m_session); | ||
|
|
||
| RETURN_HR_IF(E_POINTER, DestPath == nullptr); | ||
| return CallImpl(&WSLCContainerImpl::UploadArchive, TarHandle, DestPath, ContentSize); |
| WSLCExecutionResult RunWslcWithStdinFile( | ||
| const std::wstring& commandLine, const std::filesystem::path& stdinFilePath, ElevationType elevationType = ElevationType::Elevated); |
| httpStatusCode = response.result_int(); | ||
| if (httpStatusCode != 200) | ||
| { | ||
| pendingErrorJson.emplace(); | ||
| } |
| <data name="WSLCCLI_ContainerCpLongDesc" xml:space="preserve"> | ||
| <value>Copy a tar archive from stdin into a container. Use '-' as the source to read from stdin. | ||
| Usage: wslc container cp - CONTAINER:DEST_PATH</value> | ||
| <comment>{Locked="wslc"}{Locked="container cp"}{Locked="CONTAINER:DEST_PATH"}</comment> | ||
| </data> |
| HRESULT Stats([out] LPSTR* Output); | ||
| HRESULT ConnectToNetwork([in] const WSLCNetworkConnectionOptions* Options); | ||
| HRESULT DisconnectFromNetwork([in] LPCSTR NetworkName); | ||
| HRESULT UploadArchive([in] WSLCHandle TarHandle, [in, string] LPCSTR DestPath, [in] ULONGLONG ContentSize); | ||
| } |
| HRESULT WSLCContainer::UploadArchive(WSLCHandle TarHandle, LPCSTR DestPath, ULONGLONG ContentSize) | ||
| { | ||
| WSLCExecutionContext context(&m_session); | ||
|
|
||
| RETURN_HR_IF(E_POINTER, DestPath == nullptr); | ||
| return CallImpl(&WSLCContainerImpl::UploadArchive, TarHandle, DestPath, ContentSize); | ||
| } |
| // Running without piped stdin should fail with a terminal error. | ||
| // RunWslcAndRedirectToFile gives the child a real console stdout handle, | ||
| // and since RunWslc pipes NUL to stdin, we use RunWslcAndRedirectToFile | ||
| // with no output path to get a real console for the child. | ||
| const auto result = RunWslcAndRedirectToFile(L"container cp - fakecontainer:/path"); |
| <data name="WSLCCLI_ContainerCpLongDesc" xml:space="preserve"> | ||
| <value>Copy a tar archive from stdin into a container. Use '-' as the source to read from stdin. | ||
| Usage: wslc container cp - CONTAINER:DEST_PATH</value> | ||
| <comment>{Locked="wslc"}{Locked="container cp"}{Locked="CONTAINER:DEST_PATH"}</comment> | ||
| </data> |
- Add Archive argument type in ArgumentDefinitions.h - Add --archive/-a flag to ContainerCpCommand arguments - Add WSLCCLI_ArchiveArgDescription localization string - Add 2 CLI parsing test cases for -a and --archive - Add 2 e2e tests: ArchiveFlag (-a) and ArchiveFlagLongForm (--archive) The flag is accepted for docker cp compatibility. Since the tar stream is relayed directly to Docker's PUT /archive API, uid/gid information from the source tar is always preserved (equivalent to -a behavior). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…rue/false) Add ParseBoolValue() helper to ArgumentParser that accepts true/false/1/0 (case-insensitive). Modify ProcessNamedArgument() and ProcessAliasArgument() to parse adjoined boolean values for flag-type arguments. - --flag=true and -f=true set the flag (equivalent to --flag / -f) - --flag=false and -f=false leave the flag unset - Invalid values produce FlagInvalidBooleanError - Add 8 CLI parsing unit test cases for boolean flag values - Add 5 e2e tests for -a/--archive boolean value variants - All 143 unit tests and 18 e2e tests pass Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| if (boolVal.value()) | ||
| { | ||
| AddFlag(arg.Type()); | ||
| } | ||
|
|
||
| return {}; |
| if (boolVal.value()) | ||
| { | ||
| AddFlag(firstArg->Type()); | ||
| } | ||
|
|
||
| return {}; |
| if (boolVal.value()) | ||
| { | ||
| AddFlag(nextArg->Type()); | ||
| } | ||
|
|
||
| return {}; | ||
| } |
| contentSize = static_cast<ULONGLONG>(fileSize.QuadPart); | ||
| } | ||
|
|
||
| ContainerService::CopyToContainer(session, containerId, destPath, stdinHandle, contentSize); |
| HRESULT Stats([out] LPSTR* Output); | ||
| HRESULT ConnectToNetwork([in] const WSLCNetworkConnectionOptions* Options); | ||
| HRESULT DisconnectFromNetwork([in] LPCSTR NetworkName); | ||
| HRESULT UploadArchive([in] WSLCHandle TarHandle, [in, string] LPCSTR DestPath, [in] ULONGLONG ContentSize); |
| auto url = URL::Create("/containers/{}/archive", ContainerID); | ||
| url.SetParameter("path", Path); | ||
|
|
||
| std::map<std::string, std::string> headers = {{"Content-Type", "application/x-tar"}}; | ||
| if (ContentLength.has_value()) | ||
| { | ||
| headers["Content-Length"] = std::to_string(ContentLength.value()); | ||
| } | ||
|
|
||
| return SendRequestImpl(verb::put, url, {}, headers); |
- Validate DestPath is non-empty in UploadArchive (E_INVALIDARG) - Unify CONTAINER:DEST_PATH to CONTAINER:PATH in long description - Guard FromJson with try-catch for non-JSON error responses - Fix RunWslcWithStdinFile declaration line length (>130 col) - Add comment explaining archive flag is intentional no-op for stdin tar Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| HRESULT Stats([out] LPSTR* Output); | ||
| HRESULT ConnectToNetwork([in] const WSLCNetworkConnectionOptions* Options); | ||
| HRESULT DisconnectFromNetwork([in] LPCSTR NetworkName); | ||
| HRESULT UploadArchive([in] WSLCHandle TarHandle, [in, string] LPCSTR DestPath, [in] ULONGLONG ContentSize); |
| <value>Copy a tar archive from stdin into a container. Use '-' as the source to read from stdin. | ||
| Usage: wslc container cp - CONTAINER:PATH</value> | ||
| <comment>{Locked="wslc"}{Locked="container cp"}{Locked="CONTAINER:PATH"}</comment> |
| WSLC_TEST_METHOD(WSLCE2E_Container_Cp_StdinIsTerminal) | ||
| { | ||
| // Running without piped stdin should fail with a terminal error. | ||
| // RunWslcAndRedirectToFile gives the child a real console stdout handle, | ||
| // and since RunWslc pipes NUL to stdin, we use RunWslcAndRedirectToFile | ||
| // with no output path to get a real console for the child. | ||
| const auto result = RunWslcAndRedirectToFile(L"container cp - fakecontainer:/path"); | ||
| VERIFY_IS_TRUE(result.ExitCode.has_value()); | ||
| VERIFY_ARE_EQUAL(1u, result.ExitCode.value()); | ||
| VERIFY_IS_TRUE(result.Stderr.has_value()); | ||
| VERIFY_ARE_NOT_EQUAL(0u, result.Stderr.value().size()); | ||
| } |
| std::optional<uint64_t> contentLength; | ||
| if (ContentSize > 0) | ||
| { | ||
| contentLength = ContentSize; | ||
| } | ||
|
|
||
| auto requestContext = m_dockerClient.PutArchive(m_id, DestPath, contentLength); | ||
|
|
OneBlue
left a comment
There was a problem hiding this comment.
Couple thoughts on this:
-
This change is making changes to the argument parser to add support for
-<flag>=value. I would recommend doing this in a separate change since this will affect all commands, and so we probably want to review this separately -
From my reading it's looking like we're only supporting
cpto the container (and not the other direction), and only when passing through a tar via stdin.
Given that we're planning to tag a release build soon, I would recommend waiting before merging this (otherwise users might think that we fully support cp, which can lead to confusion).
| auto source = WideToMultiByte(context.Args.Get<ArgType::Source>()); | ||
| auto target = WideToMultiByte(context.Args.Get<ArgType::Target>()); | ||
|
|
||
| // Parse CONTAINER:PATH from the target |
There was a problem hiding this comment.
cp can be used to copy files in either direction. It's looking like this change only implements the host -> container path, which is OK, but if we do that I think we should just error a "not implemented" error if the user tries to copy from container to host
| url.SetParameter("path", Path); | ||
|
|
||
| std::map<std::string, std::string> headers = {{"Content-Type", "application/x-tar"}}; | ||
| if (ContentLength.has_value()) |
There was a problem hiding this comment.
Does docker accept receiving this request without Content-Length ? When I tried this in other tar related calls, it was required
| // and since RunWslc pipes NUL to stdin, we use RunWslcAndRedirectToFile | ||
| // with no output path to get a real console for the child. | ||
| const auto result = RunWslcAndRedirectToFile(L"container cp - fakecontainer:/path"); | ||
| VERIFY_IS_TRUE(result.ExitCode.has_value()); |
There was a problem hiding this comment.
If we expect failure we should validate the error message here
There was a problem hiding this comment.
(Same for below commands)
| void CreateTestTarFile() | ||
| { | ||
| const std::string fileName = "testfile.txt"; | ||
| const std::string fileContent = "wslc-cp-test-content\n"; |
There was a problem hiding this comment.
I recommend using tar.exe instead of manually creating a tar
| return {.CommandLine = std::move(effectiveCommandLine), .Stdout = L"", .Stderr = stdErrOutput, .ExitCode = exitCode}; | ||
| } | ||
|
|
||
| WSLCExecutionResult RunWslcWithStdinFile(const std::wstring& commandLine, const std::filesystem::path& stdinFilePath, ElevationType elevationType) |
There was a problem hiding this comment.
If we want to override stdin, I would recommend adding an optional HANDLE argument to RunWslc, that way we don't need to duplicate the process creation logic
Summary
Implements \wslc container cp - CONTAINER:PATH\ to copy a tar archive from stdin into a running container via Docker's PUT /containers/{id}/archive API.
Usage
\\powershell
Pipe a tar archive into a container
tar.exe -cf - cert.pem | wslc container cp - my_container:/etc/ssl/
Pipe from any tar-producing command
Get-Content archive.tar -Raw | wslc container cp - my_container:/app/
\\
Implementation
Full stack implementation across all layers:
Key Design Decisions
Testing