Repository: firecracker-microvm/firecracker-containerd (go)

Issue:
Shorten the firecracker unix socket path
Firecracker containerd constructs the firecracker socket path using pattern `/var/run/firecracker-containerd/$NAMESPACE/$VMID/firecracker.sock`, however, Linux has a limit on the length of socket path, that means if I am using uuid for both namespace and vmid, the firecracker socket path will be 122 characters long which is beyond the limit. 

Containerd used to have this problem to create shim socket, they fixed the issue in this [PR](https://github.com/containerd/containerd/pull/3046) where they do sha256 on the path, is it possible for firecracker containerd to do the same?

Thanks,
Haikuo

Pull request:
Specify unix socket paths as relative paths.
This limits their length which prevents us from exceeding the max socket
length limit imposed by Linux.

Closes #264

Signed-off-by: Erik Sipsma <sipsma@amazon.com>

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/firecracker-control/local.go b/firecracker-control/local.go
index 24e934b..74d233a 100644
--- a/firecracker-control/local.go
+++ b/firecracker-control/local.go
@@ -280,6 +280,12 @@ func (s *local) newShim(ns, vmID, containerdAddress string, shimSocket *net.Unix
 		return nil, err
 	}
 
+	// note: The working dir of the shim has an effect on the length of the path
+	// needed to specify various unix sockets that the shim uses to communicate
+	// with the firecracker VMM and guest agent within. The length of that path
+	// has a relatively low limit (usually 108 chars), so modifying the working
+	// dir should be done with caution. See internal/vm/dir.go for the path
+	// definitions.
 	cmd.Dir = shimDir.RootPath()
 
 	shimSocketFile, err := shimSocket.File()
diff --git a/internal/vm/dir.go b/internal/vm/dir.go
index 8281790..00fc041 100644
--- a/internal/vm/dir.go
+++ b/internal/vm/dir.go
@@ -43,7 +43,12 @@ func ShimDir(namespace, vmID string) (Dir, error) {
 		return "", errors.Wrap(err, "invalid vm id")
 	}
 
-	return Dir(filepath.Join(varRunDir, namespace, vmID)), nil
+	resolvedVarRunDir, err := filepath.EvalSymlinks(varRunDir)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed evaluating any symlinks in path %q", varRunDir)
+	}
+
+	return Dir(filepath.Join(resolvedVarRunDir, namespace, vmID)), nil
 }
 
 // Dir represents the root of a firecracker-containerd VM directory, which
@@ -83,12 +88,24 @@ func (d Dir) FirecrackerSockPath() string {
 	return filepath.Join(d.RootPath(), internal.FirecrackerSockName)
 }
 
+// FirecrackerSockRelPath returns the path to FirecrackerSockPath relative to the
+// current working directory
+func (d Dir) FirecrackerSockRelPath() (string, error) {
+	return relPathTo(d.FirecrackerSockPath())
+}
+
 // FirecrackerVSockPath returns the path to the vsock unix socket that the runtime uses
 // to communicate with the VM agent.
 func (d Dir) FirecrackerVSockPath() string {
 	return filepath.Join(d.RootPath(), internal.FirecrackerVSockName)
 }
 
+// FirecrackerVSockRelPath returns the path to FirecrackerVSockPath relative to the
+// current working directory
+func (d Dir) FirecrackerVSockRelPath() (string, error) {
+	return relPathTo(d.FirecrackerVSockPath())
+}
+
 // FirecrackerLogFifoPath returns the path to the FIFO at which the firecracker VMM writes
 // its logs
 func (d Dir) FirecrackerLogFifoPath() string {
@@ -157,3 +174,17 @@ func createSymlink(oldPath, newPath string, errMsgName string) error {
 
 	return nil
 }
+
+func relPathTo(absPath string) (string, error) {
+	cwd, err := os.Getwd()
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to get current working directory")
+	}
+
+	relPath, err := filepath.Rel(cwd, absPath)
+	if err != nil {
+		return "", errors.Wrapf(err, "failed to get relative path from %q to %q", cwd, absPath)
+	}
+
+	return relPath, nil
+}
diff --git a/runtime/service.go b/runtime/service.go
index 7dca209..a180183 100644
--- a/runtime/service.go
+++ b/runtime/service.go
@@ -463,9 +463,19 @@ func (s *service) createVM(requestCtx context.Context, request *proto.CreateVMRe
 		return errors.Wrapf(err, "failed to build VM configuration")
 	}
 
+	relSockPath, err := s.shimDir.FirecrackerSockRelPath()
+	if err != nil {
+		return errors.Wrapf(err, "failed to get relative path to firecracker api socket")
+	}
+
+	relVSockPath, err := s.shimDir.FirecrackerVSockRelPath()
+	if err != nil {
+		return errors.Wrapf(err, "failed to get relative path to firecracker vsock")
+	}
+
 	cmd := firecracker.VMCommandBuilder{}.
 		WithBin(s.config.FirecrackerBinaryPath).
-		WithSocketPath(s.shimDir.FirecrackerSockPath()).
+		WithSocketPath(relSockPath).
 		Build(s.shimCtx) // shimCtx so the VM process is only killed when the shim shuts down
 
 	// use shimCtx so the VM is killed when the shim shuts down
@@ -480,7 +490,7 @@ func (s *service) createVM(requestCtx context.Context, request *proto.CreateVMRe
 	}
 
 	s.logger.Info("calling agent")
-	conn, err := vm.VSockDial(requestCtx, s.logger, s.shimDir.FirecrackerVSockPath(), defaultVsockPort)
+	conn, err := vm.VSockDial(requestCtx, s.logger, relVSockPath, defaultVsockPort)
 	if err != nil {
 		return errors.Wrapf(err, "failed to dial the VM over vsock")
 	}
@@ -537,7 +547,7 @@ func (s *service) GetVMInfo(requestCtx context.Context, request *proto.GetVMInfo
 
 	return &proto.GetVMInfoResponse{
 		VMID:            s.vmID,
-		SocketPath:      s.machineConfig.SocketPath,
+		SocketPath:      s.shimDir.FirecrackerSockPath(),
 		LogFifoPath:     s.machineConfig.LogFifo,
 		MetricsFifoPath: s.machineConfig.MetricsFifo,
 	}, nil
@@ -565,10 +575,20 @@ func (s *service) SetVMMetadata(requestCtx context.Context, request *proto.SetVM
 }
 
 func (s *service) buildVMConfiguration(req *proto.CreateVMRequest) (*firecracker.Config, error) {
+	relSockPath, err := s.shimDir.FirecrackerSockRelPath()
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to get relative path to firecracker api socket")
+	}
+
+	relVSockPath, err := s.shimDir.FirecrackerVSockRelPath()
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to get relative path to firecracker vsock")
+	}
+
 	cfg := firecracker.Config{
-		SocketPath: s.shimDir.FirecrackerSockPath(),
+		SocketPath: relSockPath,
 		VsockDevices: []firecracker.VsockDevice{{
-			Path: s.shimDir.FirecrackerVSockPath(),
+			Path: relVSockPath,
 			ID:   "agent_api",
 		}},
 		LogFifo:     s.shimDir.FirecrackerLogFifoPath(),
@@ -711,6 +731,11 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask
 		return nil, err
 	}
 
+	relVSockPath, err := s.shimDir.FirecrackerVSockRelPath()
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to get relative path to firecracker vsock")
+	}
+
 	var ioConnectorSet vm.IOProxy
 
 	if vm.IsAgentOnlyIO(request.Stdout, logger) {
@@ -720,14 +745,14 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask
 		if request.Stdin != "" {
 			stdinConnectorPair = &vm.IOConnectorPair{
 				ReadConnector:  vm.FIFOConnector(request.Stdin),
-				WriteConnector: vm.VSockDialConnector(s.shimDir.FirecrackerVSockPath(), extraData.StdinPort),
+				WriteConnector: vm.VSockDialConnector(relVSockPath, extraData.StdinPort),
 			}
 		}
 
 		var stdoutConnectorPair *vm.IOConnectorPair
 		if request.Stdout != "" {
 			stdoutConnectorPair = &vm.IOConnectorPair{
-				ReadConnector:  vm.VSockDialConnector(s.shimDir.FirecrackerVSockPath(), extraData.StdoutPort),
+				ReadConnector:  vm.VSockDialConnector(relVSockPath, extraData.StdoutPort),
 				WriteConnector: vm.FIFOConnector(request.Stdout),
 			}
 		}
@@ -735,7 +760,7 @@ func (s *service) Create(requestCtx context.Context, request *taskAPI.CreateTask
 		var stderrConnectorPair *vm.IOConnectorPair
 		if request.Stderr != "" {
 			stderrConnectorPair = &vm.IOConnectorPair{
-				ReadConnector:  vm.VSockDialConnector(s.shimDir.FirecrackerVSockPath(), extraData.StderrPort),
+				ReadConnector:  vm.VSockDialConnector(relVSockPath, extraData.StderrPort),
 				WriteConnector: vm.FIFOConnector(request.Stderr),
 			}
 		}
@@ -821,6 +846,11 @@ func (s *service) Exec(requestCtx context.Context, req *taskAPI.ExecProcessReque
 		return nil, err
 	}
 
+	relVSockPath, err := s.shimDir.FirecrackerVSockRelPath()
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to get relative path to firecracker vsock")
+	}
+
 	var ioConnectorSet vm.IOProxy
 
 	if vm.IsAgentOnlyIO(req.Stdout, logger) {
@@ -830,14 +860,14 @@ func (s *service) Exec(requestCtx context.Context, req *taskAPI.ExecProcessReque
 		if req.Stdin != "" {
 			stdinConnectorPair = &vm.IOConnectorPair{
 				ReadConnector:  vm.FIFOConnector(req.Stdin),
-				WriteConnector: vm.VSockDialConnector(s.shimDir.FirecrackerVSockPath(), extraData.StdinPort),
+				WriteConnector: vm.VSockDialConnector(relVSockPath, extraData.StdinPort),
 			}
 		}
 
 		var stdoutConnectorPair *vm.IOConnectorPair
 		if req.Stdout != "" {
 			stdoutConnectorPair = &vm.IOConnectorPair{
-				ReadConnector:  vm.VSockDialConnector(s.shimDir.FirecrackerVSockPath(), extraData.StdoutPort),
+				ReadConnector:  vm.VSockDialConnector(relVSockPath, extraData.StdoutPort),
 				WriteConnector: vm.FIFOConnector(req.Stdout),
 			}
 		}
@@ -845,7 +875,7 @@ func (s *service) Exec(requestCtx context.Context, req *taskAPI.ExecProcessReque
 		var stderrConnectorPair *vm.IOConnectorPair
 		if req.Stderr != "" {
 			stderrConnectorPair = &vm.IOConnectorPair{
-				ReadConnector:  vm.VSockDialConnector(s.shimDir.FirecrackerVSockPath(), extraData.StderrPort),
+				ReadConnector:  vm.VSockDialConnector(relVSockPath, extraData.StderrPort),
 				WriteConnector: vm.FIFOConnector(req.Stderr),
 			}
 		}


Test patch:
diff --git a/internal/vm/dir_test.go b/internal/vm/dir_test.go
index 7cdef40..4672865 100644
--- a/internal/vm/dir_test.go
+++ b/internal/vm/dir_test.go
@@ -40,9 +40,9 @@ func TestShimDir(t *testing.T) {
 		{name: "id with ?", ns: "test", id: "?", outErr: `invalid vm id: identifier "?" must match`},
 		{name: "id with *", ns: "test", id: "*", outErr: `invalid vm id: identifier "*" must match`},
 		{name: "id with ,", ns: "test", id: ",", outErr: `invalid vm id: identifier "," must match`},
-		{name: "valid", ns: "ns", id: "1", outDir: "/var/run/firecracker-containerd/ns/1"},
-		{name: "valid with dashes", ns: "test-123", id: "123-456", outDir: "/var/run/firecracker-containerd/test-123/123-456"},
-		{name: "valid with dots", ns: "test.123", id: "123.456", outDir: "/var/run/firecracker-containerd/test.123/123.456"},
+		{name: "valid", ns: "ns", id: "1", outDir: "/run/firecracker-containerd/ns/1"},
+		{name: "valid with dashes", ns: "test-123", id: "123-456", outDir: "/run/firecracker-containerd/test-123/123-456"},
+		{name: "valid with dots", ns: "test.123", id: "123.456", outDir: "/run/firecracker-containerd/test.123/123.456"},
 	}
 
 	for _, tc := range tests {
diff --git a/runtime/service_integ_test.go b/runtime/service_integ_test.go
index ed6b3ee..0153817 100644
--- a/runtime/service_integ_test.go
+++ b/runtime/service_integ_test.go
@@ -45,6 +45,7 @@ import (
 
 	_ "github.com/firecracker-microvm/firecracker-containerd/firecracker-control"
 	"github.com/firecracker-microvm/firecracker-containerd/internal"
+	"github.com/firecracker-microvm/firecracker-containerd/internal/vm"
 	"github.com/firecracker-microvm/firecracker-containerd/proto"
 	fccontrol "github.com/firecracker-microvm/firecracker-containerd/proto/service/fccontrol/ttrpc"
 	"github.com/firecracker-microvm/firecracker-containerd/runtime/firecrackeroci"
@@ -61,7 +62,7 @@ const (
 
 	defaultVMRootfsPath = "/var/lib/firecracker-containerd/runtime/default-rootfs.img"
 	defaultVMNetDevName = "eth0"
-	varRunDir           = "/var/run/firecracker-containerd"
+	varRunDir           = "/run/firecracker-containerd"
 )
 
 // Images are presumed by the isolated tests to have already been pulled
@@ -448,6 +449,53 @@ func TestMultipleVMs_Isolated(t *testing.T) {
 
 	vmWg.Wait()
 }
+
+func TestLongUnixSocketPath_Isolated(t *testing.T) {
+	// Verify that if the absolute path of the Firecracker unix sockets are longer
+	// than the max length enforced by the kernel (UNIX_PATH_MAX, usually 108), we
+	// don't fail (due to the internal implementation using relative paths).
+	// We do this by using the max VMID len (76 chars), which in combination with the
+	// default location we store state results in a path like
+	// "/run/firecracker-containerd/default/<vmID>" (with len 112).
+	internal.RequiresIsolation(t)
+	const maxUnixSockLen = 108
+	vmID := strings.Repeat("x", 76)
+
+	ctx := namespaces.WithNamespace(context.Background(), "default")
+
+	pluginClient, err := ttrpcutil.NewClient(containerdSockPath + ".ttrpc")
+	require.NoError(t, err, "failed to create ttrpc client")
+
+	fcClient := fccontrol.NewFirecrackerClient(pluginClient.Client())
+	_, err = fcClient.CreateVM(ctx, &proto.CreateVMRequest{
+		VMID: vmID,
+		RootDrive: &proto.FirecrackerDrive{
+			PathOnHost:   defaultVMRootfsPath,
+			IsReadOnly:   false,
+			IsRootDevice: true,
+		},
+		NetworkInterfaces: []*proto.FirecrackerNetworkInterface{},
+	})
+	require.NoError(t, err, "failed to create VM")
+
+	// double-check that the sockets are at the expected path and that their absolute
+	// length exceeds 108 bytes
+	shimDir, err := vm.ShimDir("default", vmID)
+	require.NoError(t, err, "failed to get shim dir")
+
+	_, err = os.Stat(shimDir.FirecrackerSockPath())
+	require.NoError(t, err, "failed to stat firecracker socket path")
+	if len(shimDir.FirecrackerSockPath()) <= maxUnixSockLen {
+		assert.Failf(t, "firecracker sock absolute path %q is not greater than max unix socket path length", shimDir.FirecrackerSockPath())
+	}
+
+	_, err = os.Stat(shimDir.FirecrackerVSockPath())
+	require.NoError(t, err, "failed to stat firecracker vsock path")
+	if len(shimDir.FirecrackerVSockPath()) <= maxUnixSockLen {
+		assert.Failf(t, "firecracker vsock absolute path %q is not greater than max unix socket path length", shimDir.FirecrackerVSockPath())
+	}
+}
+
 func TestStubBlockDevices_Isolated(t *testing.T) {
 	internal.RequiresIsolation(t)
 	const vmID = 0
diff --git a/runtime/service_test.go b/runtime/service_test.go
index 49a7f53..7122e0d 100644
--- a/runtime/service_test.go
+++ b/runtime/service_test.go
@@ -191,10 +191,17 @@ func TestBuildVMConfiguration(t *testing.T) {
 			defer os.RemoveAll(tempDir)
 
 			svc.shimDir = vm.Dir(tempDir)
+
+			relSockPath, err := svc.shimDir.FirecrackerSockRelPath()
+			require.NoError(t, err, "failed to get firecracker sock rel path")
+
+			relVSockPath, err := svc.shimDir.FirecrackerVSockRelPath()
+			require.NoError(t, err, "failed to get firecracker vsock rel path")
+
 			// For values that remain constant between tests, they are written here
-			tc.expectedCfg.SocketPath = svc.shimDir.FirecrackerSockPath()
+			tc.expectedCfg.SocketPath = relSockPath
 			tc.expectedCfg.VsockDevices = []firecracker.VsockDevice{{
-				Path: svc.shimDir.FirecrackerVSockPath(),
+				Path: relVSockPath,
 				ID:   "agent_api",
 			}}
 			tc.expectedCfg.LogFifo = svc.shimDir.FirecrackerLogFifoPath()
@@ -260,7 +267,10 @@ func TestDebugConfig(t *testing.T) {
 		},
 	}
 
-	path, err := ioutil.TempDir("./", "TestDebugConfig")
+	cwd, err := os.Getwd()
+	require.NoError(t, err, "failed to get working dir")
+
+	path, err := ioutil.TempDir(cwd, "TestDebugConfig")
 	assert.NoError(t, err, "failed to create temp directory")
 
 	defer os.RemoveAll(path)
@@ -270,7 +280,9 @@ func TestDebugConfig(t *testing.T) {
 		stubDrivePath := filepath.Join(path, fmt.Sprintf("%d", i))
 		err := os.MkdirAll(stubDrivePath, os.ModePerm)
 		assert.NoError(t, err, "failed to create stub drive path")
+
 		c.service.shimDir = vm.Dir(stubDrivePath)
+
 		req := proto.CreateVMRequest{}
 
 		cfg, err := c.service.buildVMConfiguration(&req)