Add LCOW logpath within uVM

A common scenario for pods is to write container logs (`stdout` and
`stderr`) to a file on the host (specified by the `LogPath` and
`LogDirectory` CRI fields), and then mount that directory to another
(e.g., a monitoring or observability container) in the pod for
consumption.
For LCOW, this means logs must traverse from the uVM to the host, and
then back into the uVM via (plan9) share, which both:
 - adds unnecessary overhead; and
 - exposes data to the host.

Address this by adding annotations to mimic `LogPath` and `LogDirectory`
functionality, but within the uVM.
Since the uVM rootfs is not directly backed by a VHD (i.e., directories
are either RO or `tmpfs` backed), create a dedicated directory in the
sandbox scratch for logs, and allow that path to be mounted within a
container.

Note: containerd expects to be the end-all sink for container
logs, use `io.MultiWriter` to keep default behavior and write logs to
the specified path while also exposing them to the host.
Therefore, annotations use `TeeLog` (instead of just `Log`) to make
behavior explicit.

Add `LCOWTeeLogPath` annotation to tee container `stdin` and `stderr`
to a specified file within the uVM, relative to a dedicated log
directory in the sandbox scratch.

Add `LCOWTeeLogDirMount` annotation to mount the above log directory to
the specified path within the container.

Add a dedicated `trapsort.MultiWriter` type that writes to both an
underlying `transport.Connection` and the provided `io.Writer`.

Move `logConnection` from `stdio` to `transport`, to keep the
different `Connection` implementations together.

Add functional tests.

Streamline `test/internal/util` by making `CleanName` operation on the
test name directly (`testing.TB.Name()`), since that is the majority of
the usage.
Add `CleanString` to handle the original functionality.

Signed-off-by: Hamza El-Saawy <[email protected]>

Author: helsaawy

internal/guest/runtime/hcsv2/container.go

@@ -45,8 +45,10 @@ const (
 )
 
 type Container struct {
-	id    string
-	vsock transport.Transport
+	id string
+
+	vsock   transport.Transport
+	logFile *os.File
 
 	spec          *oci.Spec
 	ociBundlePath string
@@ -77,10 +79,17 @@ type Container struct {
 
 func (c *Container) Start(ctx context.Context, conSettings stdio.ConnectionSettings) (int, error) {
 	log.G(ctx).WithField(logfields.ContainerID, c.id).Info("opengcs::Container::Start")
-	stdioSet, err := stdio.Connect(c.vsock, conSettings)
+
+	// only use the logfile for the init process, since we don't want to tee stdio of execs
+	t := c.vsock
+	if c.logFile != nil {
+		t = transport.NewMultiWriter(c.vsock, c.logFile)
+	}
+	stdioSet, err := stdio.Connect(t, conSettings)
 	if err != nil {
 		return -1, err
 	}
+
 	if c.initProcess.spec.Terminal {
 		ttyr := c.container.Tty()
 		ttyr.ReplaceConnectionSet(stdioSet)
@@ -180,12 +189,24 @@ func (c *Container) GetAllProcessPids(ctx context.Context) ([]int, error) {
 
 // Kill sends 'signal' to the container process.
 func (c *Container) Kill(ctx context.Context, signal syscall.Signal) error {
-	log.G(ctx).WithField(logfields.ContainerID, c.id).Info("opengcs::Container::Kill")
+	entity := log.G(ctx).WithField(logfields.ContainerID, c.id)
+	entity.Info("opengcs::Container::Kill")
 	err := c.container.Kill(signal)
 	if err != nil {
 		return err
 	}
+
 	c.setExitType(signal)
+	if c.logFile != nil {
+		if err = c.logFile.Close(); err != nil {
+			entity.WithFields(logrus.Fields{
+				logrus.ErrorKey: err,
+				logfields.Path:  c.logFile.Name(),
+			}).Warn("failed to close log file")
+		}
+		c.logFile = nil
+	}
+
 	return nil
 }
 

internal/guest/runtime/hcsv2/uvm.go

@@ -22,6 +22,13 @@ import (
 
 	"github.com/Microsoft/cosesign1go/pkg/cosesign1"
 	didx509resolver "github.com/Microsoft/didx509go/pkg/did-x509-resolver"
+	cgroup1stats "github.com/containerd/cgroups/v3/cgroup1/stats"
+	"github.com/mattn/go-shellwords"
+	"github.com/opencontainers/runtime-spec/specs-go"
+	"github.com/pkg/errors"
+	"github.com/sirupsen/logrus"
+	"golang.org/x/sys/unix"
+
 	"github.com/Microsoft/hcsshim/internal/bridgeutils/gcserr"
 	"github.com/Microsoft/hcsshim/internal/debug"
 	"github.com/Microsoft/hcsshim/internal/guest/policy"
@@ -37,18 +44,13 @@ import (
 	"github.com/Microsoft/hcsshim/internal/guest/storage/scsi"
 	"github.com/Microsoft/hcsshim/internal/guest/transport"
 	"github.com/Microsoft/hcsshim/internal/log"
+	"github.com/Microsoft/hcsshim/internal/logfields"
 	"github.com/Microsoft/hcsshim/internal/oci"
 	"github.com/Microsoft/hcsshim/internal/protocol/guestrequest"
 	"github.com/Microsoft/hcsshim/internal/protocol/guestresource"
 	"github.com/Microsoft/hcsshim/internal/verity"
 	"github.com/Microsoft/hcsshim/pkg/annotations"
 	"github.com/Microsoft/hcsshim/pkg/securitypolicy"
-	cgroup1stats "github.com/containerd/cgroups/v3/cgroup1/stats"
-	"github.com/mattn/go-shellwords"
-	"github.com/opencontainers/runtime-spec/specs-go"
-	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
-	"golang.org/x/sys/unix"
 )
 
 // UVMContainerID is the ContainerID that will be sent on any prot.MessageBase
@@ -299,6 +301,17 @@ func setupSandboxHugePageMountsPath(id string) error {
 	return storage.MountRShared(mountPath)
 }
 
+func setupSandboxLogDir(id string) error {
+	mountPath := specGuest.SandboxLogsDir(id)
+	if err := mkdirAllModePerm(mountPath); err != nil {
+		return errors.Wrapf(err, "failed to create sandbox logs dir in sandbox %v", id)
+	}
+	return nil
+}
+
+// TODO: unify workload and standalone logic for non-sandbox features (e.g., block devices, huge pages, uVM mounts)
+// TODO(go1.24): use [os.Root] instead of `!strings.HasPrefix(<path>, <root>)`
+
 func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VMHostedContainerSettingsV2) (_ *Container, err error) {
 	criType, isCRI := settings.OCISpecification.Annotations[annotations.KubernetesContainerType]
 	c := &Container{
@@ -354,6 +367,9 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM
 			if err = setupSandboxHugePageMountsPath(id); err != nil {
 				return nil, err
 			}
+			if err = setupSandboxLogDir(id); err != nil {
+				return nil, err
+			}
 
 			if err := policy.ExtendPolicyWithNetworkingMounts(id, h.securityPolicyEnforcer, settings.OCISpecification); err != nil {
 				return nil, err
@@ -405,6 +421,17 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM
 		}
 	}
 
+	// don't specialize tee logs (both files and mounts) just for workload containers
+	// add log directory mount before enforcing (mount) policy
+	if logDirMount := settings.OCISpecification.Annotations[annotations.LCOWTeeLogDirMount]; logDirMount != "" {
+		settings.OCISpecification.Mounts = append(settings.OCISpecification.Mounts, specs.Mount{
+			Destination: logDirMount,
+			Type:        "bind",
+			Source:      specGuest.SandboxLogsDir(sandboxID),
+			Options:     []string{"bind"},
+		})
+	}
+
 	user, groups, umask, err := h.securityPolicyEnforcer.GetUserInfo(settings.OCISpecification.Process, settings.OCISpecification.Root.Path)
 	if err != nil {
 		return nil, err
@@ -441,6 +468,31 @@ func (h *Host) CreateContainer(ctx context.Context, id string, settings *prot.VM
 		c.vsock = h.devNullTransport
 	}
 
+	if logPath := settings.OCISpecification.Annotations[annotations.LCOWTeeLogPath]; logPath != "" {
+		if !allowStdio {
+			return nil, errors.Errorf("teeing container stdio to log path %q denied due to policy not allowing stdio access", logPath)
+		}
+
+		logPath = specGuest.SandboxLogPath(sandboxID, logPath)
+		// verify the logpath is still under the correct directory
+		if !strings.HasPrefix(logPath, specGuest.SandboxLogsDir(sandboxID)) {
+			return nil, errors.Errorf("log path %v is not within sandbox's log dir", logPath)
+		}
+
+		log.G(ctx).WithFields(logrus.Fields{
+			logfields.Path:        logPath,
+			logfields.ContainerID: id,
+		}).Debug("creating container log file in uVM")
+		dir := filepath.Dir(logPath)
+		if err := mkdirAllModePerm(dir); err != nil {
+			return nil, errors.Wrapf(err, "failed to create log file parent directory: %s", dir)
+		}
+		// don't use [os.Create] since that truncates an existing file, which is not desired
+		if c.logFile, err = os.OpenFile(logPath, os.O_RDWR|os.O_CREATE, 0666); err != nil {
+			return nil, errors.Wrapf(err, "failed to create log file: %s", logPath)
+		}
+	}
+
 	if envToKeep != nil {
 		settings.OCISpecification.Process.Env = []string(envToKeep)
 	}

internal/guest/spec/spec.go

@@ -89,20 +89,30 @@ func HugePagesMountsDir(sandboxID string) string {
 	return filepath.Join(SandboxRootDir(sandboxID), "hugepages")
 }
 
-// SandboxMountSource returns sandbox mount path inside UVM
+// SandboxMountSource returns sandbox mount path inside UVM.
 func SandboxMountSource(sandboxID, path string) string {
 	mountsDir := SandboxMountsDir(sandboxID)
 	subPath := strings.TrimPrefix(path, guestpath.SandboxMountPrefix)
 	return filepath.Join(mountsDir, subPath)
 }
 
-// HugePagesMountSource returns hugepages mount path inside UVM
+// HugePagesMountSource returns hugepages mount path inside UVM.
 func HugePagesMountSource(sandboxID, path string) string {
 	mountsDir := HugePagesMountsDir(sandboxID)
 	subPath := strings.TrimPrefix(path, guestpath.HugePagesMountPrefix)
 	return filepath.Join(mountsDir, subPath)
 }
 
+// SandboxLogsDir returns the logs directory inside the UVM for forwarding container stdio to.
+func SandboxLogsDir(sandboxID string) string {
+	return filepath.Join(SandboxRootDir(sandboxID), "logs")
+}
+
+// SandboxLogPath returns the log path inside the UVM.
+func SandboxLogPath(sandboxID, path string) string {
+	return filepath.Join(SandboxLogsDir(sandboxID), path)
+}
+
 // GetNetworkNamespaceID returns the `ToLower` of
 // `spec.Windows.Network.NetworkNamespace` or `""`.
 func GetNetworkNamespaceID(spec *oci.Spec) string {

internal/guest/stdio/connection.go

@@ -4,11 +4,9 @@
 package stdio
 
 import (
-	"os"
+	"github.com/pkg/errors"
 
 	"github.com/Microsoft/hcsshim/internal/guest/transport"
-	"github.com/pkg/errors"
-	"github.com/sirupsen/logrus"
 )
 
 // ConnectionSettings describe the stdin, stdout, stderr ports to connect the
@@ -19,49 +17,6 @@ type ConnectionSettings struct {
 	StdErr *uint32
 }
 
-type logConnection struct {
-	con  transport.Connection
-	port uint32
-}
-
-func (lc *logConnection) Read(b []byte) (int, error) {
-	return lc.con.Read(b)
-}
-
-func (lc *logConnection) Write(b []byte) (int, error) {
-	return lc.con.Write(b)
-}
-
-func (lc *logConnection) Close() error {
-	logrus.WithFields(logrus.Fields{
-		"port": lc.port,
-	}).Debug("opengcs::logConnection::Close - closing connection")
-
-	return lc.con.Close()
-}
-
-func (lc *logConnection) CloseRead() error {
-	logrus.WithFields(logrus.Fields{
-		"port": lc.port,
-	}).Debug("opengcs::logConnection::Close - closing read connection")
-
-	return lc.con.CloseRead()
-}
-
-func (lc *logConnection) CloseWrite() error {
-	logrus.WithFields(logrus.Fields{
-		"port": lc.port,
-	}).Debug("opengcs::logConnection::Close - closing write connection")
-
-	return lc.con.CloseWrite()
-}
-
-func (lc *logConnection) File() (*os.File, error) {
-	return lc.con.File()
-}
-
-var _ = (transport.Connection)(&logConnection{})
-
 // Connect returns new transport.Connection instances, one for each stdio pipe
 // to be used. If CreateStd*Pipe for a given pipe is false, the given Connection
 // is set to nil.
@@ -77,30 +32,21 @@ func Connect(tport transport.Transport, settings ConnectionSettings) (_ *Connect
 		if err != nil {
 			return nil, errors.Wrap(err, "failed creating stdin Connection")
 		}
-		connSet.In = &logConnection{
-			con:  c,
-			port: *settings.StdIn,
-		}
+		connSet.In = transport.NewLogConnection(c, *settings.StdIn)
 	}
 	if settings.StdOut != nil {
 		c, err := tport.Dial(*settings.StdOut)
 		if err != nil {
 			return nil, errors.Wrap(err, "failed creating stdout Connection")
 		}
-		connSet.Out = &logConnection{
-			con:  c,
-			port: *settings.StdOut,
-		}
+		connSet.Out = transport.NewLogConnection(c, *settings.StdOut)
 	}
 	if settings.StdErr != nil {
 		c, err := tport.Dial(*settings.StdErr)
 		if err != nil {
 			return nil, errors.Wrap(err, "failed creating stderr Connection")
 		}
-		connSet.Err = &logConnection{
-			con:  c,
-			port: *settings.StdErr,
-		}
+		connSet.Err = transport.NewLogConnection(c, *settings.StdErr)
 	}
 	return connSet, nil
 }

internal/guest/transport/log.go

@@ -0,0 +1,38 @@
+//go:build linux
+
+package transport
+
+import (
+	"github.com/sirupsen/logrus"
+)
+
+// logConnection wraps the underlying [Connection] and logs the Close*() operations with
+// the connection's port number.
+type logConnection struct {
+	Connection
+	entry *logrus.Entry
+}
+
+var _ Connection = &logConnection{}
+
+func NewLogConnection(c Connection, port uint32) Connection {
+	return &logConnection{c, logrus.WithField("port", port)}
+}
+
+func (c *logConnection) Close() error {
+	c.entry.Debug("opengcs::logConnection::Close - closing connection")
+
+	return c.Connection.Close()
+}
+
+func (c *logConnection) CloseRead() error {
+	c.entry.Debug("opengcs::logConnection::Close - closing read connection")
+
+	return c.Connection.CloseRead()
+}
+
+func (c *logConnection) CloseWrite() error {
+	c.entry.Debug("opengcs::logConnection::Close - closing write connection")
+
+	return c.Connection.CloseWrite()
+}

internal/guest/transport/multiwriter.go

@@ -0,0 +1,45 @@
+//go:build linux
+
+package transport
+
+import (
+	"fmt"
+	"io"
+)
+
+// multiWriter writes to both the underlying connection and the [io.Writer], w, via [io.multiWriter].
+type multiWriter struct {
+	t Transport
+	w io.Writer
+}
+
+var _ Transport = &multiWriter{}
+
+func NewMultiWriter(t Transport, w io.Writer) Transport {
+	return &multiWriter{t, w}
+}
+
+// Dial accepts a vsock socket port number as configuration, and
+// returns an unconnected VsockConnection struct.
+func (t *multiWriter) Dial(port uint32) (Connection, error) {
+	if t == nil || t.w == nil || t.t == nil {
+		return nil, fmt.Errorf("invalid transpot")
+	}
+
+	conn, err := t.t.Dial(port)
+	if err != nil {
+		return nil, fmt.Errorf("multiwriter base transport dial: %w", err)
+	}
+	return &multiWriterConnection{conn, io.MultiWriter(conn, t.w)}, nil
+}
+
+type multiWriterConnection struct {
+	Connection
+	multi io.Writer
+}
+
+var _ Connection = &multiWriterConnection{}
+
+func (c *multiWriterConnection) Write(buf []byte) (int, error) {
+	return c.multi.Write(buf)
+}

internal/guest/transport/transport.go

@@ -5,6 +5,10 @@ import (
 	"os"
 )
 
+// TODO: specialized [Transport] and [Connection] for [io.Reader]/[io.Writer] instead of both,
+// so either stdin or stdout/stderr can be specialized without affecting the other.
+// i.e., don't use [multiWriter] for stdin.
+
 // Transport is the interface defining a method of transporting data in a
 // connection-like way.
 // Examples of a Transport implementation could be: