-
Notifications
You must be signed in to change notification settings - Fork 642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove leading slash in Windows path #3304
Conversation
Waiting for the changes from Set stderr to empty string when using terminal on Windows to be available in the latest Containerd v2 release. |
Hey @TinaMor 2.0.0 rc4 just dropped and we moved nerdctl (and testing env) to it. Curious if the change in containerd got in? |
Signed-off-by: Christine Murimi <[email protected]>
ioCreator = cio.TerminalBinaryIO(u.Path, map[string]string{ | ||
parsedPath := u.Path | ||
// For Windows, remove the leading slash | ||
if (runtime.GOOS == "windows") && (strings.HasPrefix(parsedPath, "/")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right place to remove the extra slash?
From the report "binary:///C:/Program Files/nerdctl.exe?_NERDCTL_INTERNAL_LOGGING=C%3A%5CProgramData%5Cnerdctl%5C052055e3"
it seems it has 3 slashes in front of the file path: binary:///
Where is that generated and why does it have three slashes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The path is prefixed with "/" in containerd/containerd LogURIGenerator(). See the comment in LogURIGenerator().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah interesting, it is expected that url.parse returns /c:/path
golang/go@844b625
This actually feels like a bug in cio.TerminalBinaryIO
when it is checking abs path logic but I there might be some nuance that I am missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
PR Description
This PR resolves #2966
Background
On Windows, the LogURIGenerator() function in containerd/containerd fails when it checks if the
logURI
path is absolute. This is due to the leading slash in the parsed logURI, which causes the check to fail and results in an error, ultimately leading to the command's failure.The logURI parameter is a binary path formatted as a URI, for example:
When parsed with
url.Parse(logURI)
, the resultingu.Path
is:The leading slash is incorrect for Windows paths and causes issues when the path is later checked for being absolute.
Solution
To resolve this issue, the leading slash must be removed from the
u.Path
before the absolute path check is performed. This ensures the path is correctly recognized as an absolute path on Windows systems.Additional tasks
Remove Windows check in TestRunWithTtyAndDetached. 'json-file' logging for Windows implemented by PR #1468 .
nerdctl/cmd/nerdctl/container_run_test.go
Lines 461 to 464 in 74f2755