Skip to content

Commit

Permalink
Fix LogURIGenerator on Windows
Browse files Browse the repository at this point in the history 
Checking / is not the right way to distinguish an absolute path in
Windows.

Fixes #5786.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
  • Loading branch information
@kzys
kzys committed Sep 19, 2022
1 parent 290ef2b commit bf26140
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 140 deletions.
18 changes: 12 additions & 6 deletions cio/io.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package cio

import (
"context"
"errors"
"fmt"
"io"
"net/url"
Expand Down Expand Up @@ -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, "/") {
return nil, errors.New("absolute path needed")
if !filepath.IsAbs(path) {
return nil, fmt.Errorf("%q must be absolute", path)
}

uri := &url.URL{
Scheme: scheme,
Path: path,
// Without adding / here, C:\foo\bar.txt will become file://C:/foo/bar.txt
// which is invalid. The path must have three slashes.
//
// 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
Expand Down
158 changes: 24 additions & 134 deletions cio/io_test.go
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
//go:build !windows
// +build !windows

/*
Copyright The containerd Authors.
Expand All @@ -20,152 +17,44 @@
package cio

import (
"bytes"
"context"
"io"
"fmt"
"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) {
t.Helper()
if !strings.HasPrefix(s, prefix) {
t.Fatalf("expected %s to start with %s", s, prefix)
}
}
var (
prefix string
urlPrefix string
)

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"})("")
assert.NoError(t, err)
assert.Equal(t, "binary:///file.bin?id=1", res.Config().Stdout)
assert.Equal(t, "binary:///file.bin?id=1", res.Config().Stderr)
res, err := BinaryIO(prefix+"/file.bin", map[string]string{"id": "1"})("")
require.NoError(t, err)
expected := fmt.Sprintf("binary://%s/file.bin?id=1", urlPrefix)
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)("!")
assert.NoError(t, err)
res, err := BinaryIO(prefix+"/full/path/bin", nil)("!")
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) {
Expand All @@ -174,16 +63,17 @@ func TestBinaryIOFailOnRelativePath(t *testing.T) {
}

func TestLogFileAbsolutePath(t *testing.T) {
res, err := LogFile("/full/path/file.txt")("!")
assert.NoError(t, err)
assert.Equal(t, "file:///full/path/file.txt", res.Config().Stdout)
assert.Equal(t, "file:///full/path/file.txt", res.Config().Stderr)
res, err := LogFile(prefix + "/full/path/file.txt")("!")
require.NoError(t, err)
expected := fmt.Sprintf("file://%s/full/path/file.txt", urlPrefix)
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) {
Expand Down
114 changes: 114 additions & 0 deletions cio/io_unix_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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())
}
17 changes: 17 additions & 0 deletions cio/io_windows_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)

func TestNewFifoSetInDir_NoTerminal(t *testing.T) {
Expand All @@ -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)
}
}

0 comments on commit bf26140

Please sign in to comment.