Repository: firecracker-microvm/firecracker-containerd
Pull request #368 (2019-12-12 23:30:09)

Fix corner case in agent's taskExecID.
While debugging something else I realized the current logic for generating a
unique id from a given (taskID, execID) pair has a silly corner case where
different pairs result in the same id:
* taskID="task-task", execID="task" results in "exec-task-task-task-task"
* taskID="task", execID="task-task" also results in "exec-task-task-task-task"

This change uses the fact that the IDs are earlier validated with containerd's
identifier validation logic (which bans use of "/" in the ID) to make a unique
ID by using "/" as a separator and then doing a base64 encode (to keep the id
filesystem safe).

Signed-off-by: Erik Sipsma <sipsma@amazon.com>

I noticed this while looking into #363. I don't think it has anything to do with that issue but it's worth fixing anyways.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.



Patch:
diff --git a/agent/service.go b/agent/service.go
index 5c6747b..af02c13 100644
--- a/agent/service.go
+++ b/agent/service.go
@@ -15,13 +15,16 @@ package main
 
 import (
 	"context"
+	"encoding/base64"
 	"fmt"
 	"os"
 	"runtime/debug"
 	"sync"
 
 	"github.com/containerd/containerd/cio"
+	"github.com/containerd/containerd/identifiers"
 	"github.com/containerd/containerd/log"
+	"github.com/containerd/containerd/mount"
 	runc "github.com/containerd/containerd/runtime/v2/runc/v2"
 	"github.com/containerd/containerd/runtime/v2/shim"
 	taskAPI "github.com/containerd/containerd/runtime/v2/task"
@@ -31,7 +34,6 @@ import (
 	"github.com/sirupsen/logrus"
 	"golang.org/x/sys/unix"
 
-	"github.com/containerd/containerd/mount"
 	"github.com/firecracker-microvm/firecracker-containerd/internal/bundle"
 	"github.com/firecracker-microvm/firecracker-containerd/internal/vm"
 	"github.com/firecracker-microvm/firecracker-containerd/proto"
@@ -69,9 +71,19 @@ type TaskService struct {
 	shimCancel context.CancelFunc
 }
 
-// provides a unique string for a given (taskID, execID) pair
-func taskExecID(taskID, execID string) string {
-	return fmt.Sprintf("exec-%s-task-%s", execID, taskID)
+// TaskExecID provides a unique string for a given (taskID, execID) pair
+func TaskExecID(taskID, execID string) (string, error) {
+	err := identifiers.Validate(taskID)
+	if execID != "" {
+		err = multierror.Append(err, identifiers.Validate(execID)).ErrorOrNil()
+	}
+	if err != nil {
+		return "", err
+	}
+
+	// use "/" as a separator, which should be an illegal character for the IDs themselves
+	// after doing the above validation
+	return base64.URLEncoding.EncodeToString([]byte(fmt.Sprintf("%s/%s", taskID, execID))), nil
 }
 
 // NewTaskService creates new runc shim wrapper
@@ -117,27 +129,23 @@ func unmarshalExtraData(marshalled *types.Any) (*proto.ExtraData, error) {
 	return extraData, nil
 }
 
-func (ts *TaskService) addCleanup(taskID, execID string, cleanup func() error) {
-	id := taskExecID(taskID, execID)
-
+func (ts *TaskService) addCleanup(taskExecID string, cleanup func() error) {
 	ts.execCleanupsMu.Lock()
 	defer ts.execCleanupsMu.Unlock()
-	ts.execCleanups[id] = append(ts.execCleanups[id], cleanup)
+	ts.execCleanups[taskExecID] = append(ts.execCleanups[taskExecID], cleanup)
 }
 
-func (ts *TaskService) doCleanup(taskID, execID string) error {
-	id := taskExecID(taskID, execID)
-
+func (ts *TaskService) doCleanup(taskExecID string) error {
 	ts.execCleanupsMu.Lock()
 	defer ts.execCleanupsMu.Unlock()
 
 	var err *multierror.Error
 	// iterate in reverse order so changes are "unwound" (similar to a defer)
-	for i := len(ts.execCleanups[id]) - 1; i >= 0; i-- {
-		err = multierror.Append(err, ts.execCleanups[id][i]())
+	for i := len(ts.execCleanups[taskExecID]) - 1; i >= 0; i-- {
+		err = multierror.Append(err, ts.execCleanups[taskExecID][i]())
 	}
 
-	delete(ts.execCleanups, id)
+	delete(ts.execCleanups, taskExecID)
 	return err.ErrorOrNil()
 }
 
@@ -146,13 +154,18 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
 	defer logPanicAndDie(log.G(requestCtx))
 	taskID := req.ID
 	execID := "" // the exec ID of the initial process in a task is an empty string by containerd convention
+	// this is technically validated earlier by containerd, but is added here too for extra safety
+	taskExecID, err := TaskExecID(taskID, execID)
+	if err != nil {
+		return nil, errors.Wrap(err, "invalid task and/or exec ID")
+	}
 
 	logger := log.G(requestCtx).WithField("TaskID", taskID).WithField("ExecID", execID)
 	logger.Info("create")
 
 	defer func() {
 		if err != nil {
-			cleanupErr := ts.doCleanup(taskID, execID)
+			cleanupErr := ts.doCleanup(taskExecID)
 			if cleanupErr != nil {
 				logger.WithError(cleanupErr).Error("failed to cleanup task")
 			}
@@ -168,7 +181,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
 	req.Options = extraData.RuncOptions
 
 	bundleDir := bundle.Dir(req.Bundle)
-	ts.addCleanup(taskID, execID, func() error {
+	ts.addCleanup(taskExecID, func() error {
 		err := os.RemoveAll(bundleDir.RootPath())
 		if err != nil {
 			return errors.Wrapf(err, "failed to remove bundle path %q", bundleDir.RootPath())
@@ -184,7 +197,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
 	if !rootfsStat.IsDir() {
 		return nil, errors.Errorf("bundle's rootfs path %q is not a dir", bundleDir.RootfsPath())
 	}
-	ts.addCleanup(taskID, execID, func() error {
+	ts.addCleanup(taskExecID, func() error {
 		err := mount.UnmountAll(bundleDir.RootfsPath(), unix.MNT_DETACH)
 		if err != nil {
 			return errors.Wrapf(err, "failed to unmount bundle rootfs %q", bundleDir.RootfsPath())
@@ -203,8 +216,7 @@ func (ts *TaskService) Create(requestCtx context.Context, req *taskAPI.CreateTas
 		ioConnectorSet = vm.NewNullIOProxy()
 	} else {
 		// Override the incoming stdio FIFOs, which have paths from the host that we can't use
-		fifoName := taskExecID(taskID, execID)
-		fifoSet, err := cio.NewFIFOSetInDir(bundleDir.RootPath(), fifoName, req.Terminal)
+		fifoSet, err := cio.NewFIFOSetInDir(bundleDir.RootPath(), taskExecID, req.Terminal)
 		if err != nil {
 			err = errors.Wrap(err, "failed to open stdio FIFOs")
 			logger.WithError(err).Error()
@@ -290,6 +302,12 @@ func (ts *TaskService) Delete(requestCtx context.Context, req *taskAPI.DeleteReq
 	defer logPanicAndDie(log.G(requestCtx))
 	taskID := req.ID
 	execID := req.ExecID
+	// this is technically validated earlier by containerd, but is added here too for extra safety
+	taskExecID, err := TaskExecID(taskID, execID)
+	if err != nil {
+		return nil, errors.Wrap(err, "invalid task and/or exec ID")
+	}
+
 	log.G(requestCtx).WithFields(logrus.Fields{"id": taskID, "exec_id": execID}).Debug("delete")
 
 	resp, err := ts.taskManager.DeleteProcess(requestCtx, req, ts.runcService)
@@ -297,7 +315,7 @@ func (ts *TaskService) Delete(requestCtx context.Context, req *taskAPI.DeleteReq
 		return nil, err
 	}
 
-	err = ts.doCleanup(taskID, execID)
+	err = ts.doCleanup(taskExecID)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to cleanup task %q exec %q", taskID, execID)
 	}
@@ -390,13 +408,18 @@ func (ts *TaskService) Exec(requestCtx context.Context, req *taskAPI.ExecProcess
 
 	taskID := req.ID
 	execID := req.ExecID
+	// this is technically validated earlier by containerd, but is added here too for extra safety
+	taskExecID, err := TaskExecID(taskID, execID)
+	if err != nil {
+		return nil, errors.Wrap(err, "invalid task and/or exec ID")
+	}
 
 	logger := log.G(requestCtx).WithField("TaskID", taskID).WithField("ExecID", execID)
 	logger.Debug("exec")
 
 	defer func() {
 		if err != nil {
-			cleanupErr := ts.doCleanup(taskID, execID)
+			cleanupErr := ts.doCleanup(taskExecID)
 			if cleanupErr != nil {
 				logger.WithError(cleanupErr).Error("failed to cleanup task")
 			}
@@ -419,8 +442,7 @@ func (ts *TaskService) Exec(requestCtx context.Context, req *taskAPI.ExecProcess
 		ioConnectorSet = vm.NewNullIOProxy()
 	} else {
 		// Override the incoming stdio FIFOs, which have paths from the host that we can't use
-		fifoName := taskExecID(taskID, execID)
-		fifoSet, err := cio.NewFIFOSetInDir(bundleDir.RootPath(), fifoName, req.Terminal)
+		fifoSet, err := cio.NewFIFOSetInDir(bundleDir.RootPath(), taskExecID, req.Terminal)
 		if err != nil {
 			err = errors.Wrap(err, "failed to open stdio FIFOs")
 			logger.WithError(err).Error()
@@ -434,7 +456,7 @@ func (ts *TaskService) Exec(requestCtx context.Context, req *taskAPI.ExecProcess
 				ReadConnector:  vm.VSockAcceptConnector(extraData.StdinPort),
 				WriteConnector: vm.FIFOConnector(fifoSet.Stdin),
 			}
-			ts.addCleanup(taskID, execID, func() error {
+			ts.addCleanup(taskExecID, func() error {
 				return os.RemoveAll(req.Stdin)
 			})
 		}
@@ -446,7 +468,7 @@ func (ts *TaskService) Exec(requestCtx context.Context, req *taskAPI.ExecProcess
 				ReadConnector:  vm.FIFOConnector(fifoSet.Stdout),
 				WriteConnector: vm.VSockAcceptConnector(extraData.StdoutPort),
 			}
-			ts.addCleanup(taskID, execID, func() error {
+			ts.addCleanup(taskExecID, func() error {
 				return os.RemoveAll(req.Stdout)
 			})
 		}
@@ -458,7 +480,7 @@ func (ts *TaskService) Exec(requestCtx context.Context, req *taskAPI.ExecProcess
 				ReadConnector:  vm.FIFOConnector(fifoSet.Stderr),
 				WriteConnector: vm.VSockAcceptConnector(extraData.StderrPort),
 			}
-			ts.addCleanup(taskID, execID, func() error {
+			ts.addCleanup(taskExecID, func() error {
 				return os.RemoveAll(req.Stderr)
 			})
 		}
diff --git a/examples/Makefile b/examples/Makefile
index 2be12f7..8873f78 100644
--- a/examples/Makefile
+++ b/examples/Makefile
@@ -51,7 +51,7 @@ integ-test:
 TEST_GATEWAY?=172.16.0.1
 TEST_IP?=172.16.0.2
 TEST_SUBNET?=/24
-TEST_SS?=devmapper
+TEST_SS?=naive
 TEST_POOL?=
 testtap:
 	ip link add br0 type bridge
diff --git a/examples/taskworkflow.go b/examples/taskworkflow.go
index 5b52a18..f575987 100644
--- a/examples/taskworkflow.go
+++ b/examples/taskworkflow.go
@@ -46,11 +46,15 @@ const (
 func main() {
 	var containerCIDR = flag.String("ip", "", "ip address and subnet assigned to the container in CIDR notation. Example: -ip 172.16.0.2/24")
 	var gatewayIP = flag.String("gw", "", "gateway ip address. Example: -gw 172.16.0.1")
-	var snapshotter = flag.String("ss", "devmapper", "snapshotter")
+	var snapshotter = flag.String("ss", "", "snapshotter")
 
 	log.SetFlags(log.Ldate | log.Ltime | log.Lmicroseconds)
 	flag.Parse()
 
+	if *snapshotter == "naive" {
+		*snapshotter = "firecracker-naive"
+	}
+
 	if *containerCIDR != "" && *gatewayIP == "" {
 		log.Fatal("Incorrect usage. 'gw' needs to be specified when 'ip' is specified")
 	}
diff --git a/examples/taskworkflow.md b/examples/taskworkflow.md
index 324d2a4..641a4a5 100644
--- a/examples/taskworkflow.md
+++ b/examples/taskworkflow.md
@@ -1,6 +1,6 @@
 ### About
 The `taskworkfow.go` file contains code to pull and unpack a container image
-using the `devmapper` snaphotter, create a container and execute a task using the
+using the `naive` snaphotter, create a container and execute a task using the
 `firecracker-containerd` runtime.
 
 ### Building
diff --git a/runtime/Makefile b/runtime/Makefile
index a92b227..36fb749 100644
--- a/runtime/Makefile
+++ b/runtime/Makefile
@@ -79,7 +79,7 @@ PERF_RUNTIME_SECONDS?=600
 PERF_VM_MEMSIZE_MB?=1024
 PERF_TARGET_BANDWIDTH?=1G
 
-FICD_SNAPSHOTTER?=devmapper
+FICD_SNAPSHOTTER?=naive
 FICD_DM_POOL?=
 
 tc-redirect-tap-perf:


Test patch:
diff --git a/agent/agent_test.go b/agent/agent_test.go
new file mode 100644
index 0000000..6958a91
--- /dev/null
+++ b/agent/agent_test.go
@@ -0,0 +1,90 @@
+// Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License"). You may
+// not use this file except in compliance with the License. A copy of the
+// License is located at
+//
+//	http://aws.amazon.com/apache2.0/
+//
+// or in the "license" file accompanying this file. This file is distributed
+// on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
+// express or implied. See the License for the specific language governing
+// permissions and limitations under the License.
+
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestTaskExecID(t *testing.T) {
+	results := make(map[string]bool)
+
+	testcases := []struct {
+		taskID string
+		execID string
+	}{
+		{
+			taskID: "task-task",
+			execID: "task",
+		},
+		{
+			taskID: "task",
+			execID: "task-task",
+		},
+		{
+			taskID: "task",
+			execID: "task",
+		},
+		{
+			taskID: "task",
+			execID: "",
+		},
+	}
+	for _, testcase := range testcases {
+		id, err := TaskExecID(testcase.taskID, testcase.execID)
+		assert.NoError(t, err, "unexpected error", "valid task/exec ID %+v", testcase)
+		if results[id] {
+			assert.Fail(t, "unexpected duplicated result", "testcase %+v has duplicated result %q", testcase, id)
+		}
+		results[id] = true
+	}
+}
+
+func TestTaskExecIDFails(t *testing.T) {
+	testcases := []struct {
+		taskID string
+		execID string
+	}{
+		{
+			taskID: "ta/sk",
+			execID: "",
+		},
+		{
+			taskID: "ta/sk",
+			execID: "exec",
+		},
+		{
+			taskID: "task",
+			execID: "ex/ec",
+		},
+		{
+			taskID: "ta/sk",
+			execID: "ex/ec",
+		},
+		{
+			taskID: "task/",
+			execID: "exec/",
+		},
+		{
+			taskID: "/task",
+			execID: "/exec",
+		},
+	}
+	for _, testcase := range testcases {
+		_, err := TaskExecID(testcase.taskID, testcase.execID)
+		assert.Error(t, err, "unexpected nil error", "invalid task/exec ID %+v", testcase)
+	}
+}
diff --git a/runtime/integ_test.go b/runtime/integ_test.go
index c09081c..5c931db 100644
--- a/runtime/integ_test.go
+++ b/runtime/integ_test.go
@@ -44,11 +44,12 @@ var defaultRuntimeConfig = config.Config{
 }
 
 func defaultSnapshotterName() string {
-	if name := os.Getenv("FICD_SNAPSHOTTER"); name != "" {
-		return name
+	name := os.Getenv("FICD_SNAPSHOTTER")
+	if name == "" || name == "naive" {
+		return "firecracker-naive"
 	}
 
-	return "devmapper"
+	return name
 }
 
 func prepareIntegTest(t *testing.T, options ...func(*config.Config)) {
diff --git a/tools/docker/Dockerfile.integ-test b/tools/docker/Dockerfile.integ-test
index ed5f1a4..ee22010 100644
--- a/tools/docker/Dockerfile.integ-test
+++ b/tools/docker/Dockerfile.integ-test
@@ -1,5 +1,5 @@
 # syntax=docker/dockerfile:experimental
-# Test image that starts up containerd and the devmapper snapshotter. The default CMD will drop to a bash shell. Overrides
+# Test image that starts up containerd and the naive snapshotter. The default CMD will drop to a bash shell. Overrides
 # to CMD will be provided appended to /bin/bash -c
 FROM golang:1.13-stretch
 ENV PATH="/bin:/usr/bin:/usr/local/bin:/sbin:/usr/sbin:/usr/local/sbin:/usr/local/go/bin"
@@ -8,7 +8,7 @@ ENV DEBIAN_FRONTEND="noninteractive"
 
 ARG FIRECRACKER_TARGET=x86_64-unknown-linux-musl
 ENV FICD_LOG_DIR="/var/log/firecracker-containerd-test"
-ENV FICD_SNAPSHOTTER="devmapper"
+ENV FICD_SNAPSHOTTER="naive"
 ENV FICD_SNAPSHOTTER_OUTFILE="${FICD_LOG_DIR}/snapshotter.out"
 ENV FICD_CONTAINERD_OUTFILE="${FICD_LOG_DIR}/containerd.out"
 
@@ -26,6 +26,7 @@ RUN apt-get update && apt-get install --yes --no-install-recommends \
 RUN mkdir -p \
     /var/run/firecracker-containerd \
     /src \
+    /var/lib/firecracker-containerd/naive \
     /srv/firecracker_containerd_tests \
     ${FICD_LOG_DIR}