From f97b63f878ade3ebb8abff855830403d5cc8fa3d Mon Sep 17 00:00:00 2001
From: Tolga Ceylan <tolga.ceylan@gmail.com>
Date: Fri, 1 Jun 2018 09:25:51 -0700
Subject: [PATCH] fn: fixup temp dir read/write permissions if tmp fs size is
 not set. (#1024)

When TmpFsSize is not set in a route, docker fails to create a /tmp
mount that is writable. Forcing docker to explicitly to this if
read-only root directory is enabled (default).
---
 api/agent/agent_test.go            | 103 ++++++++++++++++++++++++++++-
 api/agent/drivers/docker/docker.go |  12 ++--
 2 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/api/agent/agent_test.go b/api/agent/agent_test.go
index 0bf6aafda..6d58018a8 100644
--- a/api/agent/agent_test.go
+++ b/api/agent/agent_test.go
@@ -632,10 +632,106 @@ func TestGetCallReturnsResourceImpossibility(t *testing.T) {
 	}
 }
 
-func TestTmpFsSize(t *testing.T) {
-	// TODO it may be a good idea to mock out the http server and use a real
-	// response writer with sync, and also test that this works with async + log
+//
+// Tmp directory should be RW by default.
+//
+func TestTmpFsRW(t *testing.T) {
+	appName := "myapp"
+	path := "/hello"
+	url := "http://127.0.0.1:8080/r/" + appName + path
 
+	app := &models.App{Name: appName}
+	app.SetDefaults()
+	// we need to load in app & route so that FromRequest works
+	ds := datastore.NewMockInit(
+		[]*models.App{app},
+		[]*models.Route{
+			{
+				Path:        path,
+				AppID:       app.ID,
+				Image:       "fnproject/fn-test-utils",
+				Type:        "sync",
+				Format:      "http", // this _is_ the test
+				Timeout:     5,
+				IdleTimeout: 10,
+				Memory:      64,
+			},
+		},
+	)
+
+	a := New(NewDirectDataAccess(ds, ds, new(mqs.Mock)))
+	defer checkClose(t, a)
+
+	// Here we tell fn-test-utils to read file /proc/mounts and create a /tmp/salsa of 4MB
+	bodOne := `{"readFile":"/proc/mounts", "createFile":"/tmp/salsa", "createFileSize": 4194304, "isDebug": true}`
+
+	req, err := http.NewRequest("GET", url, &dummyReader{Reader: strings.NewReader(bodOne)})
+	if err != nil {
+		t.Fatal("unexpected error building request", err)
+	}
+
+	var out bytes.Buffer
+	callI, err := a.GetCall(FromRequest(a, app, path, req), WithWriter(&out))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = a.Submit(callI)
+	if err != nil {
+		t.Error("submit should not error:", err)
+	}
+
+	// we're using http format so this will have written a whole http request
+	res, err := http.ReadResponse(bufio.NewReader(&out), nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+
+	// Let's fetch read output and write results. See fn-test-utils AppResponse struct (data field)
+	var resp struct {
+		R struct {
+			MountsRead string `json:"/proc/mounts.read_output"`
+			CreateFile string `json:"/tmp/salsa.create_error"`
+		} `json:"data"`
+	}
+
+	json.NewDecoder(res.Body).Decode(&resp)
+
+	// Let's check what mounts are on...
+	mounts := strings.Split(resp.R.MountsRead, "\n")
+	isFound := false
+	isRootFound := false
+	for _, mnt := range mounts {
+		tokens := strings.Split(mnt, " ")
+		if len(tokens) < 3 {
+			continue
+		}
+
+		point := tokens[1]
+		opts := tokens[3]
+
+		// tmp dir with RW and no other options (size, inodes, etc.)
+		if point == "/tmp" && opts == "rw,nosuid,nodev,noexec,relatime" {
+			// good
+			isFound = true
+		} else if point == "/" && strings.HasPrefix(opts, "ro,") {
+			// Read-only root, good...
+			isRootFound = true
+		}
+	}
+
+	if !isFound || !isRootFound {
+		t.Fatal(`didn't get proper mounts for /tmp or /, got /proc/mounts content of:\n`, resp.R.MountsRead)
+	}
+
+	// write file should not have failed...
+	if resp.R.CreateFile != "" {
+		t.Fatal(`limited tmpfs should generate fs full error, but got output: `, resp.R.CreateFile)
+	}
+}
+
+func TestTmpFsSize(t *testing.T) {
 	appName := "myapp"
 	path := "/hello"
 	url := "http://127.0.0.1:8080/r/" + appName + path
@@ -719,6 +815,7 @@ func TestTmpFsSize(t *testing.T) {
 		point := tokens[1]
 		opts := tokens[3]
 
+		// rw tmp dir with size and inode limits applied.
 		if point == "/tmp" && opts == "rw,nosuid,nodev,noexec,relatime,size=1024k,nr_inodes=1024" {
 			// good
 			isFound = true
diff --git a/api/agent/drivers/docker/docker.go b/api/agent/drivers/docker/docker.go
index 8c484de38..a3e4097c2 100644
--- a/api/agent/drivers/docker/docker.go
+++ b/api/agent/drivers/docker/docker.go
@@ -219,7 +219,7 @@ func (drv *DockerDriver) configureFs(log logrus.FieldLogger, container *docker.C
 }
 
 func (drv *DockerDriver) configureTmpFs(log logrus.FieldLogger, container *docker.CreateContainerOptions, task drivers.ContainerTask) {
-	if task.TmpFsSize() == 0 {
+	if task.TmpFsSize() == 0 && !drv.conf.EnableReadOnlyRootFs {
 		return
 	}
 
@@ -228,10 +228,12 @@ func (drv *DockerDriver) configureTmpFs(log logrus.FieldLogger, container *docke
 	}
 
 	var tmpFsOption string
-	if drv.conf.MaxTmpFsInodes != 0 {
-		tmpFsOption = fmt.Sprintf("size=%dm,nr_inodes=%d", task.TmpFsSize(), drv.conf.MaxTmpFsInodes)
-	} else {
-		tmpFsOption = fmt.Sprintf("size=%dm", task.TmpFsSize())
+	if task.TmpFsSize() != 0 {
+		if drv.conf.MaxTmpFsInodes != 0 {
+			tmpFsOption = fmt.Sprintf("size=%dm,nr_inodes=%d", task.TmpFsSize(), drv.conf.MaxTmpFsInodes)
+		} else {
+			tmpFsOption = fmt.Sprintf("size=%dm", task.TmpFsSize())
+		}
 	}
 	target := "/tmp"