Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion localization/strings/en-US/Resources.resw
Original file line number Diff line number Diff line change
Expand Up @@ -2426,7 +2426,7 @@ On first run, creates the file with all settings commented out at their defaults
<value>Working directory inside the container</value>
</data>
<data name="WSLCCLI_CIDFileArgDescription" xml:space="preserve">
<value>Write the container ID to the provided path.</value>
<value>Write the container ID to the provided path</value>
</data>
<data name="WSLCCLI_DNSArgDescription" xml:space="preserve">
<value>IP address of the DNS nameserver in resolv.conf</value>
Expand Down Expand Up @@ -2491,6 +2491,10 @@ On first run, creates the file with all settings commented out at their defaults
<value>Invalid {} value: {} is out of valid range ({}-{}).</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="WSLCCLI_CIDFileAlreadyExistsError" xml:space="preserve">
<value>CID file '{}' already exists</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="WSLCCLI_ImageNotFoundPulling" xml:space="preserve">
<value>Image '{}' not found, pulling</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
Expand Down
2 changes: 1 addition & 1 deletion src/windows/wslc/arguments/ArgumentDefinitions.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ Module Name:
_(All, "all", L"a", Kind::Flag, Localization::WSLCCLI_AllArgDescription()) \
_(Attach, "attach", L"a", Kind::Flag, Localization::WSLCCLI_AttachArgDescription()) \
_(BuildArg, "build-arg", NO_ALIAS, Kind::Value, Localization::WSLCCLI_BuildArgDescription()) \
/*_(CIDFile, "cidfile", NO_ALIAS, Kind::Value, Localization::WSLCCLI_CIDFileArgDescription())*/ \
_(CIDFile, "cidfile", NO_ALIAS, Kind::Value, Localization::WSLCCLI_CIDFileArgDescription()) \
_(Command, "command", NO_ALIAS, Kind::Positional, Localization::WSLCCLI_CommandArgDescription()) \
_(ContainerId, "container-id", NO_ALIAS, Kind::Positional, Localization::WSLCCLI_ContainerIdArgDescription()) \
_(Force, "force", L"f", Kind::Flag, Localization::WSLCCLI_ForceArgDescription()) \
Expand Down
21 changes: 21 additions & 0 deletions src/windows/wslc/arguments/ArgumentValidation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,12 @@ void Argument::Validate(const ArgMap& execArgs) const
break;
}

case ArgType::CIDFile:
{
validation::ValidateCidFile(execArgs.Get<ArgType::CIDFile>());
break;
}

default:
break;
}
Expand Down Expand Up @@ -97,6 +103,21 @@ void ValidateVolumeMount(const std::vector<std::wstring>& values)
}
}

void ValidateCidFile(const std::wstring& cidFile)
{
std::error_code ec;
if (std::filesystem::exists(std::filesystem::path{cidFile}, ec))
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recommend manually checking if it the file exists since that creates a potential race condition with the CreateFile() call later.

{
THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(ERROR_ALREADY_EXISTS), Localization::WSLCCLI_CIDFileAlreadyExistsError(cidFile));
}

if (ec)
{
const auto errorMessage = MultiByteToWide(ec.message());
THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(ec.value()), Localization::MessageWslcFailedToOpenFile(cidFile, errorMessage));
}
}

// Convert string to WSLCSignal enum - accepts either signal name (e.g., "SIGKILL") or number (e.g., "9")
WSLCSignal GetWSLCSignalFromString(const std::wstring& input, const std::wstring& argName)
{
Expand Down
2 changes: 2 additions & 0 deletions src/windows/wslc/arguments/ArgumentValidation.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,6 @@ FormatType GetFormatTypeFromString(const std::wstring& input, const std::wstring

void ValidateVolumeMount(const std::vector<std::wstring>& values);

void ValidateCidFile(const std::wstring& cidFile);

} // namespace wsl::windows::wslc::validation
2 changes: 1 addition & 1 deletion src/windows/wslc/commands/ContainerCreateCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ std::vector<Argument> ContainerCreateCommand::GetArguments() const
Argument::Create(ArgType::ImageId, true),
Argument::Create(ArgType::Command),
Argument::Create(ArgType::ForwardArgs),
// Argument::Create(ArgType::CIDFile),
Argument::Create(ArgType::CIDFile),
// Argument::Create(ArgType::DNS),
// Argument::Create(ArgType::DNSDomain),
// Argument::Create(ArgType::DNSOption),
Expand Down
2 changes: 1 addition & 1 deletion src/windows/wslc/commands/ContainerRunCommand.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ std::vector<Argument> ContainerRunCommand::GetArguments() const
Argument::Create(ArgType::ImageId, true),
Argument::Create(ArgType::Command),
Argument::Create(ArgType::ForwardArgs),
// Argument::Create(ArgType::CIDFile),
Argument::Create(ArgType::CIDFile),
Argument::Create(ArgType::Detach),
// Argument::Create(ArgType::DNS),
// Argument::Create(ArgType::DNSDomain),
Expand Down
1 change: 1 addition & 0 deletions src/windows/wslc/services/ContainerModel.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ struct ContainerOptions
std::vector<std::string> Entrypoint;
std::optional<std::string> User{};
std::vector<std::string> Tmpfs;
std::optional<std::wstring> CidFile{};
};

struct CreateContainerResult
Expand Down
38 changes: 36 additions & 2 deletions src/windows/wslc/services/ContainerService.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ Module Name:
#include <wslutil.h>
#include <WSLCProcessLauncher.h>
#include <CommandLine.h>
#include <filesystem>
#include <fstream>
#include <unordered_map>
#include <wslc.h>

Expand All @@ -37,6 +39,35 @@ static void SetContainerArguments(WSLCProcessOptions& options, std::vector<const
options.CommandLine = {.Values = argsStorage.data(), .Count = static_cast<ULONG>(argsStorage.size())};
}

static void WriteContainerIdToFile(const std::optional<std::wstring>& cidFilePath, const std::string& containerId)
{
if (!cidFilePath.has_value())
{
return;
}

const auto path = std::filesystem::path(cidFilePath.value());
HANDLE file = ::CreateFileW(path.c_str(), GENERIC_WRITE, 0, nullptr, CREATE_NEW, FILE_ATTRIBUTE_NORMAL, nullptr);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend using:

 auto file = wil::try_create_new_file(path.c_str(), GENERIC_WRITE);

That way we don't manually have to close the handle

if (file == INVALID_HANDLE_VALUE)
{
const auto error = ::GetLastError();
const auto errorMessage = wsl::shared::string::MultiByteToWide(std::system_category().message(error));
THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(error), Localization::MessageWslcFailedToOpenFile(*cidFilePath, errorMessage));
}

DWORD bytesWritten{};
const bool writeSuccess = ::WriteFile(file, containerId.data(), static_cast<DWORD>(containerId.size()), &bytesWritten, nullptr) != FALSE;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: We can simplify this to:

THROW_IF_WIN32_BOOL_FALSE(WriteFile(file, containerId.data(), static_cast<DWORD>(containerId.size()), &bytesWritten, nullptr);

I think it would be OK not to have specialized errors for write errors here, since they're very unlikely since we successfully opened the handle at this point

const HRESULT closeResult = ::CloseHandle(file) ? S_OK : HRESULT_FROM_WIN32(GetLastError());
if (!writeSuccess || bytesWritten != containerId.size())
{
const auto error = writeSuccess ? ERROR_WRITE_FAULT : ::GetLastError();
const auto errorMessage = wsl::shared::string::MultiByteToWide(std::system_category().message(error));
THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(error), Localization::MessageWslcFailedToOpenFile(*cidFilePath, errorMessage));
}

THROW_IF_FAILED(closeResult);
}

static wsl::windows::common::RunningWSLCContainer CreateInternal(Session& session, const std::string& image, const ContainerOptions& options)
{
auto processFlags = WSLCProcessFlagsNone;
Expand Down Expand Up @@ -294,6 +325,10 @@ int ContainerService::Run(Session& session, const std::string& image, ContainerO
runningContainer.SetDeleteOnClose(false);
auto& container = runningContainer.Get();

WSLCContainerId containerId{};
THROW_IF_FAILED(container.GetId(containerId));
WriteContainerIdToFile(runOptions.CidFile, containerId);

// Start the created container
WSLCContainerStartFlags startFlags{};
WI_SetFlagIf(startFlags, WSLCContainerStartFlagsAttach, !runOptions.Detach);
Expand All @@ -306,8 +341,6 @@ int ContainerService::Run(Session& session, const std::string& image, ContainerO
return consoleService.AttachToCurrentConsole(runningContainer.GetInitProcess());
}

WSLCContainerId containerId{};
THROW_IF_FAILED(container.GetId(containerId));
PrintMessage(L"%hs", stdout, containerId);
return 0;
}
Expand All @@ -319,6 +352,7 @@ CreateContainerResult ContainerService::Create(Session& session, const std::stri
auto& container = runningContainer.Get();
WSLCContainerId id{};
THROW_IF_FAILED(container.GetId(id));
WriteContainerIdToFile(runOptions.CidFile, id);
return {.Id = id};
}

Expand Down
5 changes: 5 additions & 0 deletions src/windows/wslc/tasks/ContainerTasks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,11 @@ void SetContainerOptionsFromArgs(CLIExecutionContext& context)
{
ContainerOptions options;

if (context.Args.Contains(ArgType::CIDFile))
{
options.CidFile = context.Args.Get<ArgType::CIDFile>();
}

if (context.Args.Contains(ArgType::Name))
{
options.Name = WideToMultiByte(context.Args.Get<ArgType::Name>());
Expand Down
2 changes: 2 additions & 0 deletions test/windows/wslc/CommandLineTestCases.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ COMMAND_LINE_TEST_CASE(L"container list --format badformat", L"list", false)
COMMAND_LINE_TEST_CASE(L"run ubuntu", L"run", true)
COMMAND_LINE_TEST_CASE(L"container run ubuntu bash -c 'echo Hello World'", L"run", true)
COMMAND_LINE_TEST_CASE(L"container run ubuntu", L"run", true)
COMMAND_LINE_TEST_CASE(L"container run --cidfile C:\\temp\\cidfile ubuntu", L"run", true)
COMMAND_LINE_TEST_CASE(L"container run -it --name foo ubuntu", L"run", true)
COMMAND_LINE_TEST_CASE(L"container run --rm -it --name foo ubuntu", L"run", true)
COMMAND_LINE_TEST_CASE(L"stop", L"stop", true)
Expand All @@ -70,6 +71,7 @@ COMMAND_LINE_TEST_CASE(L"container start --attach cont", L"start", true)
COMMAND_LINE_TEST_CASE(L"container start -a cont", L"start", true)
COMMAND_LINE_TEST_CASE(L"create ubuntu:latest", L"create", true)
COMMAND_LINE_TEST_CASE(L"container create --name foo ubuntu", L"create", true)
COMMAND_LINE_TEST_CASE(L"container create --cidfile C:\\temp\\cidfile --name foo ubuntu", L"create", true)
COMMAND_LINE_TEST_CASE(L"exec cont1 echo Hello", L"exec", true)
COMMAND_LINE_TEST_CASE(L"exec cont1", L"exec", false) // Missing required command argument
COMMAND_LINE_TEST_CASE(L"container exec -it cont1 sh -c \"echo a && echo b\"", L"exec", true) // docker exec example
Expand Down
36 changes: 36 additions & 0 deletions test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,41 @@ class WSLCE2EContainerCreateTests
VerifyContainerIsListed(containerId, L"created");
}

TEST_METHOD(WSLCE2E_Container_Create_CIDFile_Valid)
{
WSL2_TEST_ONLY();

// Prepare a CID file path that does not exist
const auto cidFilePath = wsl::windows::common::filesystem::GetTempFilename();
VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str()));
auto deleteCidFile = wil::scope_exit([&]() { VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); });

auto result = RunWslc(std::format(
L"container create --cidfile \"{}\" --name {} {}", EscapePath(cidFilePath.wstring()), WslcContainerName, DebianImage.NameAndTag()));
result.Verify({.Stderr = L"", .ExitCode = S_OK});

const auto containerId = result.GetStdoutOneLine();
VERIFY_IS_TRUE(std::filesystem::exists(cidFilePath));
VERIFY_ARE_EQUAL(containerId, ReadFileContent(cidFilePath.wstring()));
}

TEST_METHOD(WSLCE2E_Container_Create_CIDFile_AlreadyExists)
{
WSL2_TEST_ONLY();

const auto cidFilePath = wsl::windows::common::filesystem::GetTempFilename();
auto deleteCidFile = wil::scope_exit([&]() { VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); });

auto result = RunWslc(std::format(
L"container create --cidfile \"{}\" --name {} {}", EscapePath(cidFilePath.wstring()), WslcContainerName, DebianImage.NameAndTag()));
result.Dump();
result.Verify(
{.Stderr = std::format(L"CID file '{}' already exists\r\nError code: ERROR_ALREADY_EXISTS\r\n", EscapePath(cidFilePath.wstring())),
.ExitCode = 1});

VerifyContainerIsNotListed(WslcContainerName);
}

TEST_METHOD(WSLCE2E_Container_Create_DuplicateContainerName)
{
WSL2_TEST_ONLY();
Expand Down Expand Up @@ -1232,6 +1267,7 @@ class WSLCE2EContainerCreateTests
{
std::wstringstream options;
options << L"The following options are available:\r\n" //
<< L" --cidfile Write the container ID to the provided path\r\n"
<< L" --entrypoint Specifies the container init process executable\r\n"
<< L" -e,--env Key=Value pairs for environment variables\r\n"
<< L" --env-file File containing key=value pairs of env variables\r\n"
Expand Down
38 changes: 38 additions & 0 deletions test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,43 @@ class WSLCE2EContainerRunTests
VerifyContainerIsListed(WslcContainerName, L"exited");
}

TEST_METHOD(WSLCE2E_Container_Run_CIDFile_Valid)
{
WSL2_TEST_ONLY();

// Prepare a CID file path that does not exist
const auto cidFilePath = wsl::windows::common::filesystem::GetTempFilename();
VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str()));
auto deleteCidFile = wil::scope_exit([&]() { VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); });

auto result = RunWslc(std::format(
L"container run -d --cidfile \"{}\" --name {} {} sleep infinity",
EscapePath(cidFilePath.wstring()),
WslcContainerName,
DebianImage.NameAndTag()));
result.Verify({.Stderr = L"", .ExitCode = 0});

const auto containerId = result.GetStdoutOneLine();
VERIFY_IS_TRUE(std::filesystem::exists(cidFilePath));
VERIFY_ARE_EQUAL(containerId, ReadFileContent(cidFilePath.wstring()));
}

TEST_METHOD(WSLCE2E_Container_Run_CIDFile_AlreadyExists)
{
WSL2_TEST_ONLY();

const auto cidFilePath = wsl::windows::common::filesystem::GetTempFilename();
auto deleteCidFile = wil::scope_exit([&]() { VERIFY_IS_TRUE(DeleteFileW(cidFilePath.c_str())); });

auto result = RunWslc(std::format(
L"container run --cidfile \"{}\" --name {} {}", EscapePath(cidFilePath.wstring()), WslcContainerName, DebianImage.NameAndTag()));
result.Verify(
{.Stderr = std::format(L"CID file '{}' already exists\r\nError code: ERROR_ALREADY_EXISTS\r\n", EscapePath(cidFilePath.wstring())),
.ExitCode = 1});

VerifyContainerIsNotListed(WslcContainerName);
}

TEST_METHOD(WSLCE2E_Container_Run_Entrypoint)
{
WSL2_TEST_ONLY();
Expand Down Expand Up @@ -247,6 +284,7 @@ class WSLCE2EContainerRunTests
{
std::wstringstream options;
options << L"The following options are available:\r\n"
<< L" --cidfile Write the container ID to the provided path\r\n"
<< L" -d,--detach Run container in detached mode\r\n"
<< L" --entrypoint Specifies the container init process executable\r\n"
<< L" -e,--env Key=Value pairs for environment variables\r\n"
Expand Down
Loading