Make fsNode handle correctly consecutive reads and writes (#3820)

* Make fsNode handle correctly consecutive reads and writes

* Check for directories in ReadFile and add some error checks

* Update comment

* Improved docs and added better test

* Move test into its own file protected by built constraint

* Use manual test since iotest.TestReader is only available in Go 1.16

api/filesys/fsnode.go

@@ -6,6 +6,7 @@ package filesys
 import (
 	"bytes"
 	"fmt"
+	"io"
 	"log"
 	"os"
 	"path/filepath"
@@ -37,9 +38,9 @@ type fsNode struct {
 	// if this node is a file, this is the content.
 	content []byte
 
-	// if this node is a file, this tracks whether or
-	// not it is "open".
-	open bool
+	// if offset is not nil the file is open and it tracks
+	// the current file offset.
+	offset *int
 }
 
 // MakeEmptyDirInMemory returns an empty directory.
@@ -119,6 +120,9 @@ func (n *fsNode) addFile(name string, c []byte) (result *fsNode, err error) {
 	result, ok := parent.dir[fileName]
 	if ok {
 		// File already exists; overwrite it.
+		if result.offset != nil {
+			return nil, fmt.Errorf("cannot add already opened file '%s'", n.Path())
+		}
 		result.content = c
 		return result, nil
 	}
@@ -133,7 +137,12 @@ func (n *fsNode) addFile(name string, c []byte) (result *fsNode, err error) {
 // Create implements FileSystem.
 // Create makes an empty file.
 func (n *fsNode) Create(path string) (result File, err error) {
-	return n.AddFile(path, []byte{})
+	f, err := n.AddFile(path, nil)
+	if err != nil {
+		return f, err
+	}
+	f.offset = new(int)
+	return f, nil
 }
 
 // WriteFile implements FileSystem.
@@ -154,6 +163,7 @@ func (n *fsNode) AddFile(
 }
 
 func (n *fsNode) addDir(path string) (result *fsNode, err error) {
+
 	parent := n
 	dName, subDirName := mySplit(path)
 	if dName != "" {
@@ -348,7 +358,17 @@ func (n *fsNode) Size() int64 {
 }
 
 // Open implements FileSystem.
-// Open opens the node for reading (just marks it).
+// Open opens the node in read-write mode and sets the offset its start.
+// Writing right after opening the file will replace the original content
+// and move the offset forward, as with a file opened with O_RDWR | O_CREATE.
+//
+// As an example, let's consider a file with content "content":
+// - open: sets offset to start, content is "content"
+// - write "@": offset increases by one, the content is now "@ontent"
+// - read the rest: since offset is 1, the read operation returns "ontent"
+// - write "$": offset is at EOF, so "$" is appended and content is now "@ontent$"
+// - read the rest: returns 0 bytes and EOF
+// - close: the content is still "@ontent$"
 func (n *fsNode) Open(path string) (File, error) {
 	result, err := n.Find(path)
 	if err != nil {
@@ -357,13 +377,19 @@ func (n *fsNode) Open(path string) (File, error) {
 	if result == nil {
 		return nil, fmt.Errorf("cannot find '%s' to open it", path)
 	}
-	result.open = true
+	if result.offset != nil {
+		return nil, fmt.Errorf("cannot open previously opened file '%s'", path)
+	}
+	result.offset = new(int)
 	return result, nil
 }
 
 // Close marks the node closed.
 func (n *fsNode) Close() error {
-	n.open = false
+	if n.offset == nil {
+		return fmt.Errorf("cannot close already closed file '%s'", n.Path())
+	}
+	n.offset = nil
 	return nil
 }
 
@@ -376,9 +402,12 @@ func (n *fsNode) ReadFile(path string) (c []byte, err error) {
 	if result == nil {
 		return nil, fmt.Errorf("cannot find '%s' to read it", path)
 	}
+	if result.isNodeADir() {
+		return nil, fmt.Errorf("cannot read content from non-file '%s'", n.Path())
+	}
 	c = make([]byte, len(result.content))
-	_, err = result.Read(c)
-	return c, err
+	copy(c, result.content)
+	return c, nil
 }
 
 // Read returns the content of the file node.
@@ -387,7 +416,19 @@ func (n *fsNode) Read(d []byte) (c int, err error) {
 		return 0, fmt.Errorf(
 			"cannot read content from non-file '%s'", n.Path())
 	}
-	return copy(d, n.content), nil
+	if n.offset == nil {
+		return 0, fmt.Errorf("cannot read from closed file '%s'", n.Path())
+	}
+
+	rest := n.content[*n.offset:]
+	if len(d) < len(rest) {
+		rest = rest[:len(d)]
+	} else {
+		err = io.EOF
+	}
+	copy(d, rest)
+	*n.offset += len(rest)
+	return len(rest), err
 }
 
 // Write saves the contents of the argument to the file node.
@@ -396,8 +437,12 @@ func (n *fsNode) Write(p []byte) (c int, err error) {
 		return 0, fmt.Errorf(
 			"cannot write content to non-file '%s'", n.Path())
 	}
-	n.content = make([]byte, len(p))
-	return copy(n.content, p), nil
+	if n.offset == nil {
+		return 0, fmt.Errorf("cannot write to closed file '%s'", n.Path())
+	}
+	n.content = append(n.content[:*n.offset], p...)
+	*n.offset = len(n.content)
+	return len(p), nil
 }
 
 // ContentMatches returns true if v matches fake file's content.

api/filesys/fsnode_test.go

@@ -5,6 +5,9 @@ package filesys
 
 import (
 	"fmt"
+	"io"
+	"io/ioutil"
+	"math/rand"
 	"os"
 	"path/filepath"
 	"sort"
@@ -91,7 +94,6 @@ func TestMakeFsInMemory(t *testing.T) {
 func runBasicOperations(
 	t *testing.T, tName string, isFSysRooted bool,
 	cases []pathCase, fSys FileSystem) {
-	buff := make([]byte, 500)
 	for _, c := range cases {
 		err := fSys.WriteFile(c.arg, []byte(content))
 		if c.errStr != "" {
@@ -128,26 +130,40 @@ func runBasicOperations(
 		if fi.Name() != c.name {
 			t.Fatalf("%s; expected name '%s', got '%s'", c.what, c.name, fi.Name())
 		}
-		count, err := f.Read(buff)
+		buff, err := ioutil.ReadAll(f)
 		if err != nil {
 			t.Fatalf("%s; unexpected error: %v", c.what, err)
 		}
-		if string(buff[:count]) != content {
+		if string(buff) != content {
 			t.Fatalf("%s; unexpected buff '%s'", c.what, buff)
 		}
-		count, err = f.Write([]byte(shortContent))
+		count, err := f.Write([]byte(shortContent))
 		if err != nil {
 			t.Fatalf("%s; unexpected error: %v", c.what, err)
 		}
 		if count != len(shortContent) {
 			t.Fatalf("%s; unexpected count: %d", c.what, len(shortContent))
 		}
+		if err := f.Close(); err != nil {
+			t.Fatalf("%s; unexpected error: %v", c.what, err)
+		}
+		stuff, err = fSys.ReadFile(c.path)
+		if err != nil {
+			t.Fatalf("%s; unexpected error: %v", c.what, err)
+		}
+		both := content + shortContent
+		if string(stuff) != both {
+			t.Fatalf("%s; unexpected content '%s', expected '%s'", c.what, stuff, both)
+		}
+		if err := fSys.WriteFile(c.path, []byte(shortContent)); err != nil {
+			t.Fatalf("%s; unexpected error: %v", c.what, err)
+		}
 		stuff, err = fSys.ReadFile(c.path)
 		if err != nil {
 			t.Fatalf("%s; unexpected error: %v", c.what, err)
 		}
 		if string(stuff) != shortContent {
-			t.Fatalf("%s; unexpected content '%s'", c.what, stuff)
+			t.Fatalf("%s; unexpected content '%s', expected '%s'", c.what, stuff, shortContent)
 		}
 	}
 
@@ -589,6 +605,7 @@ func TestGlob(t *testing.T) {
 }
 
 func assertEqualStringSlices(t *testing.T, expected, actual []string, message string) {
+	t.Helper()
 	if len(expected) != len(actual) {
 		t.Fatalf(
 			"%s; unequal sizes; len(expected)=%d, len(actual)=%d\n%+v\n%+v\n",
@@ -786,3 +803,52 @@ func TestCleanedAbs(t *testing.T) {
 		}
 	}
 }
+
+func TestFileOps(t *testing.T) {
+	const path = "foo.txt"
+	content := strings.Repeat("longest content", 100)
+
+	fs := MakeFsInMemory()
+	f, err := fs.Create(path)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if _, err := fs.Open(path); err == nil {
+		t.Fatalf("expected already opened error, got nil")
+	}
+	if _, err := fmt.Fprint(f, content); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if err := f.Close(); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if err := f.Close(); err == nil {
+		t.Fatalf("expected already closed error, got nil")
+	}
+
+	f, err = fs.Open(path)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	defer f.Close()
+
+	for {
+		buf := make([]byte, rand.Intn(10))
+		n, err := f.Read(buf)
+		if err != nil && err != io.EOF {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if content[:n] != string(buf[:n]) {
+			t.Fatalf("unexpected read: expected %q got %q", content[:n], buf[:n])
+		}
+		content = content[n:]
+		if err != io.EOF {
+			continue
+		}
+		if len(content) == 0 {
+			break
+		}
+		t.Fatalf("unexpected EOF: remaining %d bytes", len(content))
+	}
+}

api/testutils/kusttest/harness.go

@@ -42,27 +42,36 @@ func (th Harness) GetFSys() filesys.FileSystem {
 }
 
 func (th Harness) WriteK(path string, content string) {
-	th.fSys.WriteFile(
+	err := th.fSys.WriteFile(
 		filepath.Join(
 			path,
 			konfig.DefaultKustomizationFileName()), []byte(`
 apiVersion: kustomize.config.k8s.io/v1beta1
 kind: Kustomization
 `+content))
+	if err != nil {
+		th.t.Fatalf("unexpected error while writing Kustomization to %s: %v", path, err)
+	}
 }
 
 func (th Harness) WriteC(path string, content string) {
-	th.fSys.WriteFile(
+	err := th.fSys.WriteFile(
 		filepath.Join(
 			path,
 			konfig.DefaultKustomizationFileName()), []byte(`
 apiVersion: kustomize.config.k8s.io/v1alpha1
 kind: Component
 `+content))
+	if err != nil {
+		th.t.Fatalf("unexpected error while writing Component to %s: %v", path, err)
+	}
 }
 
 func (th Harness) WriteF(path string, content string) {
-	th.fSys.WriteFile(path, []byte(content))
+	err := th.fSys.WriteFile(path, []byte(content))
+	if err != nil {
+		th.t.Fatalf("unexpected error while writing file to %s: %v", path, err)
+	}
 }
 
 func (th Harness) MakeDefaultOptions() krusty.Options {