Skip to content
Open
Show file tree
Hide file tree
Changes from 6 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
10 changes: 9 additions & 1 deletion localization/strings/en-US/Resources.resw
Original file line number Diff line number Diff line change
Expand Up @@ -2118,6 +2118,10 @@ For privacy information about this product please visit https://aka.ms/privacy.<
<value>Failed to open '{}': {}</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name="MessageWslcFailedToWriteFile" xml:space="preserve">
<value>Failed to write to '{}': {}</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
</data>
<data name = "MessageWslcBuildFileNotFound" xml:space = "preserve" >
<value>No Containerfile or Dockerfile found in '{}'</value>
<comment>{FixedPlaceholder="{}"}Command line arguments, file names and string inserts should not be translated</comment>
Expand Down Expand Up @@ -2426,7 +2430,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 +2495,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
43 changes: 41 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,40 @@ 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();
if (error == ERROR_FILE_EXISTS || error == ERROR_ALREADY_EXISTS)
{
THROW_HR_WITH_USER_ERROR(HRESULT_FROM_WIN32(error), Localization::WSLCCLI_CIDFileAlreadyExistsError(*cidFilePath));
}

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::MessageWslcFailedToWriteFile(*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 +330,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 +346,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 +357,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
35 changes: 35 additions & 0 deletions test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,40 @@ 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.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 +1266,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