Merge pull request #3356 from mxpv/binary-io-path

fuweid · web-flow · commit 111b082e2042 · 2019-06-20T10:25:47.000+08:00
BinaryIO/LogFile creator bug fixing
diff --git a/cio/io.go b/cio/io.go
@@ -18,10 +18,13 @@ package cio
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"net/url"
 	"os"
+	"path/filepath"
+	"strings"
 	"sync"
 
 	"github.com/containerd/containerd/defaults"
@@ -242,17 +245,24 @@ func LogURI(uri *url.URL) Creator {
 // BinaryIO forwards container STDOUT|STDERR directly to a logging binary
 func BinaryIO(binary string, args map[string]string) Creator {
 	return func(_ string) (IO, error) {
+		binary = filepath.Clean(binary)
+		if !strings.HasPrefix(binary, "/") {
+			return nil, errors.New("absolute path needed")
+		}
 		uri := &url.URL{
 			Scheme: "binary",
-			Host:   binary,
+			Path:   binary,
 		}
+		q := uri.Query()
 		for k, v := range args {
-			uri.Query().Set(k, v)
+			q.Set(k, v)
 		}
+		uri.RawQuery = q.Encode()
+		res := uri.String()
 		return &logURI{
 			config: Config{
-				Stdout: uri.String(),
-				Stderr: uri.String(),
+				Stdout: res,
+				Stderr: res,
 			},
 		}, nil
 	}
@@ -262,14 +272,19 @@ func BinaryIO(binary string, args map[string]string) Creator {
 // If the log file already exists, the logs will be appended to the file.
 func LogFile(path string) Creator {
 	return func(_ string) (IO, error) {
+		path = filepath.Clean(path)
+		if !strings.HasPrefix(path, "/") {
+			return nil, errors.New("absolute path needed")
+		}
 		uri := &url.URL{
 			Scheme: "file",
-			Host:   path,
+			Path:   path,
 		}
+		res := uri.String()
 		return &logURI{
 			config: Config{
-				Stdout: uri.String(),
-				Stderr: uri.String(),
+				Stdout: res,
+				Stderr: res,
 			},
 		}, nil
 	}
diff --git a/cio/io_test.go b/cio/io_test.go
@@ -23,6 +23,7 @@ import (
 	"context"
 	"io"
 	"io/ioutil"
+	"net/url"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -153,3 +154,44 @@ func initProducers(t *testing.T, producers producers, stdout, stderr string) {
 	assert.NilError(t, err)
 	assert.NilError(t, producers.Stderr.Close())
 }
+
+func TestBinaryIOArgs(t *testing.T) {
+	res, err := BinaryIO("/file.bin", map[string]string{"id": "1"})("")
+	assert.NilError(t, err)
+	assert.Equal(t, "binary:///file.bin?id=1", res.Config().Stdout)
+	assert.Equal(t, "binary:///file.bin?id=1", res.Config().Stderr)
+}
+
+func TestBinaryIOAbsolutePath(t *testing.T) {
+	res, err := BinaryIO("/full/path/bin", nil)("!")
+	assert.NilError(t, err)
+
+	// Test parse back
+	parsed, err := url.Parse(res.Config().Stdout)
+	assert.NilError(t, err)
+	assert.Equal(t, "binary", parsed.Scheme)
+	assert.Equal(t, "/full/path/bin", parsed.Path)
+}
+
+func TestBinaryIOFailOnRelativePath(t *testing.T) {
+	_, err := BinaryIO("./bin", nil)("!")
+	assert.Error(t, err, "absolute path needed")
+}
+
+func TestLogFileAbsolutePath(t *testing.T) {
+	res, err := LogFile("/full/path/file.txt")("!")
+	assert.NilError(t, err)
+	assert.Equal(t, "file:///full/path/file.txt", res.Config().Stdout)
+	assert.Equal(t, "file:///full/path/file.txt", res.Config().Stderr)
+
+	// Test parse back
+	parsed, err := url.Parse(res.Config().Stdout)
+	assert.NilError(t, err)
+	assert.Equal(t, "file", parsed.Scheme)
+	assert.Equal(t, "/full/path/file.txt", parsed.Path)
+}
+
+func TestLogFileFailOnRelativePath(t *testing.T) {
+	_, err := LogFile("./file.txt")("!")
+	assert.Error(t, err, "absolute path needed")
+}
diff --git a/runtime/v1/linux/proc/io.go b/runtime/v1/linux/proc/io.go
@@ -103,11 +103,11 @@ func createIO(ctx context.Context, id string, ioUID, ioGID int, stdio proc.Stdio
 	case "binary":
 		pio.io, err = NewBinaryIO(ctx, id, u)
 	case "file":
-		if err := os.MkdirAll(filepath.Dir(u.Host), 0755); err != nil {
+		if err := os.MkdirAll(filepath.Dir(u.Path), 0755); err != nil {
 			return nil, err
 		}
 		var f *os.File
-		f, err = os.OpenFile(u.Host, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+		f, err = os.OpenFile(u.Path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
 		if err != nil {
 			return nil, err
 		}
@@ -265,7 +265,7 @@ func NewBinaryIO(ctx context.Context, id string, uri *url.URL) (runc.IO, error)
 		}
 	}
 	ctx, cancel := context.WithCancel(ctx)
-	cmd := exec.CommandContext(ctx, uri.Host, args...)
+	cmd := exec.CommandContext(ctx, uri.Path, args...)
 	cmd.Env = append(cmd.Env,
 		"CONTAINER_ID="+id,
 		"CONTAINER_NAMESPACE="+ns,

Original file line number	Diff line number	Diff line change
`@@ -103,11 +103,11 @@ func createIO(ctx context.Context, id string, ioUID, ioGID int, stdio proc.Stdio`
`103`	`103`	`case "binary":`
`104`	`104`	`pio.io, err = NewBinaryIO(ctx, id, u)`
`105`	`105`	`case "file":`
`106`		`- if err := os.MkdirAll(filepath.Dir(u.Host), 0755); err != nil {`
	`106`	`+ if err := os.MkdirAll(filepath.Dir(u.Path), 0755); err != nil {`
`107`	`107`	`return nil, err`
`108`	`108`	`}`
`109`	`109`	`var f *os.File`
`110`		`- f, err = os.OpenFile(u.Host, os.O_APPEND\|os.O_CREATE\|os.O_WRONLY, 0644)`
	`110`	`+ f, err = os.OpenFile(u.Path, os.O_APPEND\|os.O_CREATE\|os.O_WRONLY, 0644)`
`111`	`111`	`if err != nil {`
`112`	`112`	`return nil, err`
`113`	`113`	`}`
`@@ -265,7 +265,7 @@ func NewBinaryIO(ctx context.Context, id string, uri *url.URL) (runc.IO, error)`
`265`	`265`	`}`
`266`	`266`	`}`
`267`	`267`	`ctx, cancel := context.WithCancel(ctx)`
`268`		`- cmd := exec.CommandContext(ctx, uri.Host, args...)`
	`268`	`+ cmd := exec.CommandContext(ctx, uri.Path, args...)`
`269`	`269`	`cmd.Env = append(cmd.Env,`
`270`	`270`	`"CONTAINER_ID="+id,`
`271`	`271`	`"CONTAINER_NAMESPACE="+ns,`