Merge pull request firecracker-microvm#96 from superfly/jailer-improv…

…ements

Improves and fixes jailer

.gitignore

@@ -1,3 +1,4 @@
 firecracker
+jailer
 firecracker-*
 vmlinux

CHANGELOG.md

@@ -1,5 +1,6 @@
 # [ Unreleased ]
 
+* Fixes a bug where fifos were not working properly with jailer enabled (#96)
 * Fixes bug where context was not being used at all during startVM (#86)
 * Updates the jailer's socket path to point to the unix socket in the jailer's workspace (#86)
 * Fixes bug where default socketpath would always be used when not using jailer (#84).

handlers.go

@@ -22,6 +22,7 @@ import (
 const (
 	StartVMMHandlerName                = "fcinit.StartVMM"
 	BootstrapLoggingHandlerName        = "fcinit.BootstrapLogging"
+	CreateLogFilesHandlerName          = "fcinit.CreateLogFilesHandler"
 	CreateMachineHandlerName           = "fcinit.CreateMachine"
 	CreateBootSourceHandlerName        = "fcinit.CreateBootSource"
 	AttachDrivesHandlerName            = "fcinit.AttachDrives"
@@ -107,6 +108,29 @@ var StartVMMHandler = Handler{
 	},
 }
 
+// CreateLogFilesHandler is a named handler that will create the fifo log files
+var CreateLogFilesHandler = Handler{
+	Name: CreateLogFilesHandlerName,
+	Fn: func(ctx context.Context, m *Machine) error {
+		logFifoPath := m.cfg.LogFifo
+		metricsFifoPath := m.cfg.MetricsFifo
+
+		if len(logFifoPath) == 0 || len(metricsFifoPath) == 0 {
+			// logging is disabled
+			return nil
+		}
+
+		if err := createFifos(logFifoPath, metricsFifoPath); err != nil {
+			m.logger.Errorf("Unable to set up logging: %s", err)
+			return err
+		}
+
+		m.logger.Debug("Created metrics and logging fifos.")
+
+		return nil
+	},
+}
+
 // BootstrapLoggingHandler is a named handler that will set up fifo logging of
 // firecracker process.
 var BootstrapLoggingHandler = Handler{
@@ -180,6 +204,7 @@ func NewSetMetadataHandler(metadata interface{}) Handler {
 
 var defaultFcInitHandlerList = HandlerList{}.Append(
 	StartVMMHandler,
+	CreateLogFilesHandler,
 	BootstrapLoggingHandler,
 	CreateMachineHandler,
 	CreateBootSourceHandler,

jailer.go

@@ -309,12 +309,7 @@ func jail(ctx context.Context, m *Machine, cfg *Config) error {
 		stderr = os.Stderr
 	}
 
-	stdin := cfg.JailerCfg.Stdin
-	if stdin == nil {
-		stdin = os.Stdin
-	}
-
-	m.cmd = NewJailerCommandBuilder().
+	builder := NewJailerCommandBuilder().
 		WithID(cfg.JailerCfg.ID).
 		WithUID(*cfg.JailerCfg.UID).
 		WithGID(*cfg.JailerCfg.GID).
@@ -324,9 +319,13 @@ func jail(ctx context.Context, m *Machine, cfg *Config) error {
 		WithDaemonize(cfg.JailerCfg.Daemonize).
 		WithSeccompLevel(cfg.JailerCfg.SeccompLevel).
 		WithStdout(stdout).
-		WithStderr(stderr).
-		WithStdin(stdin).
-		Build(ctx)
+		WithStderr(stderr)
+
+	if stdin := cfg.JailerCfg.Stdin; stdin != nil {
+		builder = builder.WithStdin(stdin)
+	}
+
+	m.cmd = builder.Build(ctx)
 
 	if err := cfg.JailerCfg.ChrootStrategy.AdaptHandlers(&m.Handlers); err != nil {
 		return err
@@ -375,6 +374,29 @@ func LinkFilesHandler(rootfs, kernelImageFileName string) Handler {
 			}
 
 			m.cfg.KernelImagePath = kernelImageFileName
+
+			for _, fifoPath := range []*string{&m.cfg.LogFifo, &m.cfg.MetricsFifo} {
+				if fifoPath == nil || *fifoPath == "" {
+					continue
+				}
+
+				fileName := filepath.Base(*fifoPath)
+				if err := linkFileToRootFS(
+					m.cfg.JailerCfg,
+					filepath.Join(rootfs, fileName),
+					*fifoPath,
+				); err != nil {
+					return err
+				}
+
+				if err := os.Chown(filepath.Join(rootfs, fileName), *m.cfg.JailerCfg.UID, *m.cfg.JailerCfg.GID); err != nil {
+					return err
+				}
+
+				// update fifoPath as jailer works relative to the chroot dir
+				*fifoPath = fileName
+			}
+
 			return nil
 		},
 	}
@@ -395,18 +417,18 @@ func NewNaiveChrootStrategy(rootfs, kernelImagePath string) NaiveChrootStrategy
 	}
 }
 
-// ErrCreateMachineHandlerMissing occurs when the CreateMachineHandler is not
-// present in FcInit.
-var ErrCreateMachineHandlerMissing = fmt.Errorf("%s is missing from FcInit's list", CreateMachineHandlerName)
+// ErrRequiredHandlerMissing occurs when a required handler is not present in
+// the handler list.
+var ErrRequiredHandlerMissing = fmt.Errorf("required handler is missing from FcInit's list")
 
 // AdaptHandlers will inject the LinkFilesHandler into the handler list.
 func (s NaiveChrootStrategy) AdaptHandlers(handlers *Handlers) error {
-	if !handlers.FcInit.Has(CreateMachineHandlerName) {
-		return ErrCreateMachineHandlerMissing
+	if !handlers.FcInit.Has(CreateLogFilesHandlerName) {
+		return ErrRequiredHandlerMissing
 	}
 
 	handlers.FcInit = handlers.FcInit.AppendAfter(
-		CreateMachineHandlerName,
+		CreateLogFilesHandlerName,
 		LinkFilesHandler(filepath.Join(s.Rootfs, rootfsFolderName), filepath.Base(s.KernelImagePath)),
 	)
 

jailer_test.go

@@ -111,7 +111,7 @@ func TestJailerBuilder(t *testing.T) {
 func TestJail(t *testing.T) {
 	m := &Machine{
 		Handlers: Handlers{
-			FcInit: HandlerList{}.Append(CreateMachineHandler),
+			FcInit: defaultFcInitHandlerList,
 		},
 	}
 	cfg := &Config{

machine.go

@@ -417,13 +417,6 @@ func (m *Machine) setupLogging(ctx context.Context) error {
 		return nil
 	}
 
-	if err := createFifos(m.cfg.LogFifo, m.cfg.MetricsFifo); err != nil {
-		m.logger.Errorf("Unable to set up logging: %s", err)
-		return err
-	}
-
-	m.logger.Debug("Created metrics and logging fifos.")
-
 	l := models.Logger{
 		LogFifo:       String(m.cfg.LogFifo),
 		Level:         String(m.cfg.LogLevel),

machine_test.go

@@ -141,8 +141,8 @@ func TestJailerMicroVMExecution(t *testing.T) {
 	os.MkdirAll(jailerTestPath, 0777)
 
 	socketPath := filepath.Join(jailerTestPath, "firecracker", "TestJailerMicroVMExecution.socket")
-	logFifo := filepath.Join(testDataPath, "firecracker.log")
-	metricsFifo := filepath.Join(testDataPath, "firecracker-metrics")
+	logFifo := filepath.Join(tmpDir, "firecracker.log")
+	metricsFifo := filepath.Join(tmpDir, "firecracker-metrics")
 	defer func() {
 		os.Remove(socketPath)
 		os.Remove(logFifo)