Fix LogURIGenerator on Windows

Checking / is not the right way to distinguish an absolute path in
Windows.

Fixes #5786.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>

cio/io.go

@@ -18,7 +18,6 @@ package cio
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"io"
 	"net/url"
@@ -302,14 +301,21 @@ func LogFile(path string) Creator {
 // LogURIGenerator is the helper to generate log uri with specific scheme.
 func LogURIGenerator(scheme string, path string, args map[string]string) (*url.URL, error) {
 	path = filepath.Clean(path)
-	if !strings.HasPrefix(path, "/") {
+	if !filepath.IsAbs(path) {
-		return nil, errors.New("absolute path needed")
+		return nil, fmt.Errorf("%q must be absolute", path)
 	}
 
-	uri := &url.URL{
+	// Without adding / here, C:\foo\bar.txt will become file://C:/foo/bar.txt
-		Scheme: scheme,
+	// which is invalid. The path must have three slashes.
-		Path:   path,
+	//
+	// https://learn.microsoft.com/en-us/archive/blogs/ie/file-uris-in-windows
+	// > In the case of a local Windows file path, there is no hostname,
+	// > and thus another slash and the path immediately follow.
+	p := filepath.ToSlash(path)
+	if !strings.HasPrefix(path, "/") {
+		p = "/" + p
 	}
+	uri := &url.URL{Scheme: scheme, Path: p}
 
 	if len(args) == 0 {
 		return uri, nil

cio/io_test.go

@@ -1,6 +1,3 @@
-//go:build !windows
-// +build !windows
-
 /*
    Copyright The containerd Authors.
 
@@ -20,152 +17,44 @@
 package cio
 
 import (
-	"bytes"
+	"fmt"
-	"context"
-	"io"
 	"net/url"
-	"os"
-	"path/filepath"
 	"runtime"
-	"strings"
-	"syscall"
 	"testing"
 
-	"github.com/containerd/fifo"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
-func assertHasPrefix(t *testing.T, s, prefix string) {
+var (
-	t.Helper()
+	prefix    string
-	if !strings.HasPrefix(s, prefix) {
+	urlPrefix string
-		t.Fatalf("expected %s to start with %s", s, prefix)
+)
-	}
-}
 
-func TestNewFIFOSetInDir(t *testing.T) {
+func init() {
 	if runtime.GOOS == "windows" {
-		t.Skip("NewFIFOSetInDir has different behaviour on windows")
+		prefix = "C:"
+		urlPrefix = "/C:"
 	}
-
-	root := t.TempDir()
-
-	fifos, err := NewFIFOSetInDir(root, "theid", true)
-	assert.NoError(t, err)
-
-	dir := filepath.Dir(fifos.Stdin)
-	assertHasPrefix(t, dir, root)
-	expected := &FIFOSet{
-		Config: Config{
-			Stdin:    filepath.Join(dir, "theid-stdin"),
-			Stdout:   filepath.Join(dir, "theid-stdout"),
-			Stderr:   filepath.Join(dir, "theid-stderr"),
-			Terminal: true,
-		},
-	}
-
-	assert.Equal(t, fifos.Config, expected.Config)
-
-	files, err := os.ReadDir(root)
-	assert.NoError(t, err)
-	assert.Len(t, files, 1)
-
-	assert.Nil(t, fifos.Close())
-	files, err = os.ReadDir(root)
-	assert.NoError(t, err)
-	assert.Len(t, files, 0)
-}
-
-func TestNewAttach(t *testing.T) {
-	if runtime.GOOS == "windows" {
-		t.Skip("setupFIFOProducers not yet implemented on windows")
-	}
-	var (
-		expectedStdin  = "this is the stdin"
-		expectedStdout = "this is the stdout"
-		expectedStderr = "this is the stderr"
-		stdin          = bytes.NewBufferString(expectedStdin)
-		stdout         = new(bytes.Buffer)
-		stderr         = new(bytes.Buffer)
-	)
-
-	withBytesBuffers := func(streams *Streams) {
-		*streams = Streams{Stdin: stdin, Stdout: stdout, Stderr: stderr}
-	}
-	attacher := NewAttach(withBytesBuffers)
-
-	fifos, err := NewFIFOSetInDir("", "theid", false)
-	assert.NoError(t, err)
-
-	attachedFifos, err := attacher(fifos)
-	assert.NoError(t, err)
-	defer attachedFifos.Close()
-
-	producers := setupFIFOProducers(t, attachedFifos.Config())
-	initProducers(t, producers, expectedStdout, expectedStderr)
-
-	actualStdin, err := io.ReadAll(producers.Stdin)
-	assert.NoError(t, err)
-
-	attachedFifos.Wait()
-	attachedFifos.Cancel()
-	assert.Nil(t, attachedFifos.Close())
-
-	assert.Equal(t, expectedStdout, stdout.String())
-	assert.Equal(t, expectedStderr, stderr.String())
-	assert.Equal(t, expectedStdin, string(actualStdin))
-}
-
-type producers struct {
-	Stdin  io.ReadCloser
-	Stdout io.WriteCloser
-	Stderr io.WriteCloser
-}
-
-func setupFIFOProducers(t *testing.T, fifos Config) producers {
-	var (
-		err   error
-		pipes producers
-		ctx   = context.Background()
-	)
-
-	pipes.Stdin, err = fifo.OpenFifo(ctx, fifos.Stdin, syscall.O_RDONLY, 0)
-	assert.NoError(t, err)
-
-	pipes.Stdout, err = fifo.OpenFifo(ctx, fifos.Stdout, syscall.O_WRONLY, 0)
-	assert.NoError(t, err)
-
-	pipes.Stderr, err = fifo.OpenFifo(ctx, fifos.Stderr, syscall.O_WRONLY, 0)
-	assert.NoError(t, err)
-
-	return pipes
-}
-
-func initProducers(t *testing.T, producers producers, stdout, stderr string) {
-	_, err := producers.Stdout.Write([]byte(stdout))
-	assert.NoError(t, err)
-	assert.Nil(t, producers.Stdout.Close())
-
-	_, err = producers.Stderr.Write([]byte(stderr))
-	assert.NoError(t, err)
-	assert.Nil(t, producers.Stderr.Close())
 }
 
 func TestBinaryIOArgs(t *testing.T) {
-	res, err := BinaryIO("/file.bin", map[string]string{"id": "1"})("")
+	res, err := BinaryIO(prefix+"/file.bin", map[string]string{"id": "1"})("")
-	assert.NoError(t, err)
+	require.NoError(t, err)
-	assert.Equal(t, "binary:///file.bin?id=1", res.Config().Stdout)
+	expected := fmt.Sprintf("binary://%s/file.bin?id=1", urlPrefix)
-	assert.Equal(t, "binary:///file.bin?id=1", res.Config().Stderr)
+	assert.Equal(t, expected, res.Config().Stdout)
+	assert.Equal(t, expected, res.Config().Stderr)
 }
 
 func TestBinaryIOAbsolutePath(t *testing.T) {
-	res, err := BinaryIO("/full/path/bin", nil)("!")
+	res, err := BinaryIO(prefix+"/full/path/bin", nil)("!")
-	assert.NoError(t, err)
+	require.NoError(t, err)
 
 	// Test parse back
 	parsed, err := url.Parse(res.Config().Stdout)
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	assert.Equal(t, "binary", parsed.Scheme)
-	assert.Equal(t, "/full/path/bin", parsed.Path)
+	assert.Equal(t, urlPrefix+"/full/path/bin", parsed.Path)
 }
 
 func TestBinaryIOFailOnRelativePath(t *testing.T) {
@@ -174,16 +63,17 @@ func TestBinaryIOFailOnRelativePath(t *testing.T) {
 }
 
 func TestLogFileAbsolutePath(t *testing.T) {
-	res, err := LogFile("/full/path/file.txt")("!")
+	res, err := LogFile(prefix + "/full/path/file.txt")("!")
-	assert.NoError(t, err)
+	require.NoError(t, err)
-	assert.Equal(t, "file:///full/path/file.txt", res.Config().Stdout)
+	expected := fmt.Sprintf("file://%s/full/path/file.txt", urlPrefix)
-	assert.Equal(t, "file:///full/path/file.txt", res.Config().Stderr)
+	assert.Equal(t, expected, res.Config().Stdout)
+	assert.Equal(t, expected, res.Config().Stderr)
 
 	// Test parse back
 	parsed, err := url.Parse(res.Config().Stdout)
 	assert.NoError(t, err)
 	assert.Equal(t, "file", parsed.Scheme)
-	assert.Equal(t, "/full/path/file.txt", parsed.Path)
+	assert.Equal(t, urlPrefix+"/full/path/file.txt", parsed.Path)
 }
 
 func TestLogFileFailOnRelativePath(t *testing.T) {

cio/io_unix_test.go

@@ -20,10 +20,16 @@
 package cio
 
 import (
+	"bytes"
 	"context"
+	"io"
+	"os"
 	"path/filepath"
+	"strings"
+	"syscall"
 	"testing"
 
+	"github.com/containerd/fifo"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -95,3 +101,111 @@ func TestOpenFifosWithTerminal(t *testing.T) {
 		}
 	}
 }
+
+func assertHasPrefix(t *testing.T, s, prefix string) {
+	t.Helper()
+	if !strings.HasPrefix(s, prefix) {
+		t.Fatalf("expected %s to start with %s", s, prefix)
+	}
+}
+
+func TestNewFIFOSetInDir(t *testing.T) {
+	root := t.TempDir()
+
+	fifos, err := NewFIFOSetInDir(root, "theid", true)
+	assert.NoError(t, err)
+
+	dir := filepath.Dir(fifos.Stdin)
+	assertHasPrefix(t, dir, root)
+	expected := &FIFOSet{
+		Config: Config{
+			Stdin:    filepath.Join(dir, "theid-stdin"),
+			Stdout:   filepath.Join(dir, "theid-stdout"),
+			Stderr:   filepath.Join(dir, "theid-stderr"),
+			Terminal: true,
+		},
+	}
+
+	assert.Equal(t, fifos.Config, expected.Config)
+
+	files, err := os.ReadDir(root)
+	assert.NoError(t, err)
+	assert.Len(t, files, 1)
+
+	assert.Nil(t, fifos.Close())
+	files, err = os.ReadDir(root)
+	assert.NoError(t, err)
+	assert.Len(t, files, 0)
+}
+
+func TestNewAttach(t *testing.T) {
+	var (
+		expectedStdin  = "this is the stdin"
+		expectedStdout = "this is the stdout"
+		expectedStderr = "this is the stderr"
+		stdin          = bytes.NewBufferString(expectedStdin)
+		stdout         = new(bytes.Buffer)
+		stderr         = new(bytes.Buffer)
+	)
+
+	withBytesBuffers := func(streams *Streams) {
+		*streams = Streams{Stdin: stdin, Stdout: stdout, Stderr: stderr}
+	}
+	attacher := NewAttach(withBytesBuffers)
+
+	fifos, err := NewFIFOSetInDir("", "theid", false)
+	assert.NoError(t, err)
+
+	attachedFifos, err := attacher(fifos)
+	assert.NoError(t, err)
+	defer attachedFifos.Close()
+
+	producers := setupFIFOProducers(t, attachedFifos.Config())
+	initProducers(t, producers, expectedStdout, expectedStderr)
+
+	actualStdin, err := io.ReadAll(producers.Stdin)
+	assert.NoError(t, err)
+
+	attachedFifos.Wait()
+	attachedFifos.Cancel()
+	assert.Nil(t, attachedFifos.Close())
+
+	assert.Equal(t, expectedStdout, stdout.String())
+	assert.Equal(t, expectedStderr, stderr.String())
+	assert.Equal(t, expectedStdin, string(actualStdin))
+}
+
+type producers struct {
+	Stdin  io.ReadCloser
+	Stdout io.WriteCloser
+	Stderr io.WriteCloser
+}
+
+func setupFIFOProducers(t *testing.T, fifos Config) producers {
+	var (
+		err   error
+		pipes producers
+		ctx   = context.Background()
+	)
+
+	pipes.Stdin, err = fifo.OpenFifo(ctx, fifos.Stdin, syscall.O_RDONLY, 0)
+	assert.NoError(t, err)
+
+	pipes.Stdout, err = fifo.OpenFifo(ctx, fifos.Stdout, syscall.O_WRONLY, 0)
+	assert.NoError(t, err)
+
+	pipes.Stderr, err = fifo.OpenFifo(ctx, fifos.Stderr, syscall.O_WRONLY, 0)
+	assert.NoError(t, err)
+
+	return pipes
+}
+
+func initProducers(t *testing.T, producers producers, stdout, stderr string) {
+	_, err := producers.Stdout.Write([]byte(stdout))
+	assert.NoError(t, err)
+	assert.Nil(t, producers.Stdout.Close())
+
+	_, err = producers.Stderr.Write([]byte(stderr))
+	assert.NoError(t, err)
+	assert.Nil(t, producers.Stderr.Close())
+}

cio/io_windows_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestNewFifoSetInDir_NoTerminal(t *testing.T) {
@@ -45,3 +46,19 @@ func TestNewFifoSetInDir_Terminal(t *testing.T) {
 	assert.NotEmpty(t, set.Stdout, "FIFOSet.Stdout should be set")
 	assert.Empty(t, set.Stderr, "FIFOSet.Stderr should not be set")
 }
+
+func TestLogFileBackslash(t *testing.T) {
+	testcases := []struct {
+		path string
+	}{
+		{`C:/foo/bar.log`},
+		{`C:\foo\bar.log`},
+	}
+	for _, tc := range testcases {
+		f := LogFile(tc.path)
+		res, err := f("unused")
+		require.NoError(t, err)
+		assert.Equal(t, res.Config().Stdout, res.Config().Stderr)
+		assert.Equal(t, "file:///C:/foo/bar.log", res.Config().Stdout)
+	}
+}