uvmboot and gcs.test bug fix (#1966)

Fix bugs:
 - Using `boot-files-path` flag name instead of value
 - Explicitly passing open door policy instead of empty string

Functional gcs tests also passed in encoded open door policy string,
which is no longer necessary.

Remove unnecessary else blocks.

Pass context through calls.
Use `log.G(ctx)` instead of `logrus`.

Rename variables `cmd` and `scsi` to avoid overshadowing package names.

Signed-off-by: Hamza El-Saawy <hamzaelsaawy@microsoft.com>

internal/tools/uvmboot/lcow.go

@@ -4,18 +4,17 @@ package main
 
 import (
 	"context"
-	"fmt"
 	"io"
 	"os"
 	"strings"
 
+	"github.com/containerd/console"
+	"github.com/urfave/cli"
+
 	"github.com/Microsoft/hcsshim/internal/cmd"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/memory"
 	"github.com/Microsoft/hcsshim/internal/uvm"
-	"github.com/Microsoft/hcsshim/pkg/securitypolicy"
-	"github.com/containerd/console"
-	"github.com/urfave/cli"
 )
 
 const (
@@ -145,11 +144,7 @@ var lcowCommand = cli.Command{
 				return err
 			}
 
-			if err := runLCOW(ctx, options, c); err != nil {
-				return err
-			}
-
-			return nil
+			return runLCOW(ctx, options, c)
 		})
 
 		return nil
@@ -167,7 +162,7 @@ func createLCOWOptions(ctx context.Context, c *cli.Context, id string) (*uvm.Opt
 
 	// boot
 	if c.IsSet(bootFilesPathArgName) {
-		options.UpdateBootFilesPath(ctx, bootFilesPathArgName)
+		options.UpdateBootFilesPath(ctx, c.String(bootFilesPathArgName))
 	}
 
 	// kernel
@@ -246,13 +241,7 @@ func createLCOWOptions(ctx context.Context, c *cli.Context, id string) (*uvm.Opt
 		options.DisableTimeSyncService = true
 	}
 
-	// default to open door security policy to allow resource modifications in
-	// non-snp uvmboot scenarios.
-	openPolicy, err := securitypolicy.NewOpenDoorPolicy().EncodeToString()
-	if err != nil {
-		return nil, fmt.Errorf("failed to encode open door policy: %s", err)
-	}
-	options.SecurityPolicy = openPolicy
+	// empty policy string defaults to open door
 	if c.IsSet(securityPolicyArgName) {
 		options.SecurityPolicy = c.String(securityPolicyArgName)
 	}
@@ -273,7 +262,9 @@ func runLCOW(ctx context.Context, options *uvm.OptionsLCOW, c *cli.Context) erro
 	if err != nil {
 		return err
 	}
-	defer vm.Close()
+	defer func() {
+		_ = vm.CloseCtx(ctx)
+	}()
 
 	if err := vm.Start(ctx); err != nil {
 		return err
@@ -292,43 +283,44 @@ func runLCOW(ctx context.Context, options *uvm.OptionsLCOW, c *cli.Context) erro
 	}
 
 	if options.UseGuestConnection {
-		if err := execViaGcs(vm, c); err != nil {
+		if err := execViaGCS(ctx, vm, c); err != nil {
 			return err
 		}
 		_ = vm.Terminate(ctx)
-		_ = vm.Wait()
+		_ = vm.WaitCtx(ctx)
 
 		return vm.ExitError()
 	}
 
-	return vm.Wait()
+	return vm.WaitCtx(ctx)
 }
 
-func execViaGcs(vm *uvm.UtilityVM, c *cli.Context) error {
-	cmd := cmd.Command(vm, "/bin/sh", "-c", c.String(execCommandLineArgName))
-	cmd.Log = log.L.Dup()
+func execViaGCS(ctx context.Context, vm *uvm.UtilityVM, cCtx *cli.Context) error {
+	c := cmd.CommandContext(ctx, vm, "/bin/sh", "-c", cCtx.String(execCommandLineArgName))
+	c.Log = log.L.Dup()
 	if lcowUseTerminal {
-		cmd.Spec.Terminal = true
-		cmd.Stdin = os.Stdin
-		cmd.Stdout = os.Stdout
+		c.Spec.Terminal = true
+		c.Stdin = os.Stdin
+		c.Stdout = os.Stdout
 		con, err := console.ConsoleFromFile(os.Stdin)
-		if err == nil {
-			err = con.SetRaw()
-			if err != nil {
+		if err != nil {
+			log.G(ctx).WithError(err).Warn("could not create console from stdin")
+		} else {
+			if err := con.SetRaw(); err != nil {
 				return err
 			}
 			defer func() {
 				_ = con.Reset()
 			}()
 		}
-	} else if c.String(outputHandlingArgName) == "stdout" {
-		if c.Bool(forwardStdoutArgName) {
-			cmd.Stdout = os.Stdout
+	} else if cCtx.String(outputHandlingArgName) == "stdout" {
+		if cCtx.Bool(forwardStdoutArgName) {
+			c.Stdout = os.Stdout
 		}
-		if c.Bool(forwardStderrArgName) {
-			cmd.Stderr = os.Stdout // match non-GCS behavior and forward to stdout
+		if cCtx.Bool(forwardStderrArgName) {
+			c.Stderr = os.Stdout // match non-GCS behavior and forward to stdout
 		}
 	}
 
-	return cmd.Run()
+	return c.Run()
 }

internal/tools/uvmboot/main.go

@@ -9,10 +9,11 @@ import (
 	"sync"
 	"time"
 
-	"github.com/Microsoft/hcsshim/internal/uvm"
-	"github.com/Microsoft/hcsshim/internal/winapi"
 	"github.com/sirupsen/logrus"
 	"github.com/urfave/cli"
+
+	"github.com/Microsoft/hcsshim/internal/uvm"
+	"github.com/Microsoft/hcsshim/internal/winapi"
 )
 
 const (

internal/tools/uvmboot/mounts.go

@@ -7,18 +7,20 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/Microsoft/hcsshim/internal/uvm"
-	"github.com/Microsoft/hcsshim/internal/uvm/scsi"
 	"github.com/sirupsen/logrus"
 	"github.com/urfave/cli"
+
+	"github.com/Microsoft/hcsshim/internal/log"
+	"github.com/Microsoft/hcsshim/internal/uvm"
+	"github.com/Microsoft/hcsshim/internal/uvm/scsi"
 )
 
 func mountSCSI(ctx context.Context, c *cli.Context, vm *uvm.UtilityVM) error {
-	for _, m := range parseMounts(c, scsiMountsArgName) {
+	for _, m := range parseMounts(ctx, c, scsiMountsArgName) {
 		if m.guest != "" {
 			return fmt.Errorf("scsi mount %s: guest path must be empty", m.host)
 		}
-		scsi, err := vm.SCSIManager.AddVirtualDisk(
+		mount, err := vm.SCSIManager.AddVirtualDisk(
 			ctx,
 			m.host,
 			!m.writable,
@@ -27,13 +29,12 @@ func mountSCSI(ctx context.Context, c *cli.Context, vm *uvm.UtilityVM) error {
 		)
 		if err != nil {
 			return fmt.Errorf("could not mount disk %s: %w", m.host, err)
-		} else {
-			logrus.WithFields(logrus.Fields{
-				"host":     m.host,
-				"guest":    scsi.GuestPath(),
-				"writable": m.writable,
-			}).Info("Mounted SCSI disk")
 		}
+		log.G(ctx).WithFields(logrus.Fields{
+			"host":     m.host,
+			"guest":    mount.GuestPath(),
+			"writable": m.writable,
+		}).Info("Mounted SCSI disk")
 	}
 
 	return nil
@@ -49,20 +50,19 @@ func shareFiles(ctx context.Context, c *cli.Context, vm *uvm.UtilityVM) error {
 }
 
 func shareFilesLCOW(ctx context.Context, c *cli.Context, vm *uvm.UtilityVM) error {
-	for _, s := range parseMounts(c, shareFilesArgName) {
+	for _, s := range parseMounts(ctx, c, shareFilesArgName) {
 		if s.guest == "" {
 			return fmt.Errorf("file shares %q has invalid quest destination: %q", s.host, s.guest)
 		}
 
 		if err := vm.Share(ctx, s.host, s.guest, !s.writable); err != nil {
 			return fmt.Errorf("could not share file or directory %s: %w", s.host, err)
-		} else {
-			logrus.WithFields(logrus.Fields{
-				"host":     s.host,
-				"guest":    s.guest,
-				"writable": s.writable,
-			}).Debug("Shared path")
 		}
+		log.G(ctx).WithFields(logrus.Fields{
+			"host":     s.host,
+			"guest":    s.guest,
+			"writable": s.writable,
+		}).Debug("Shared path")
 	}
 
 	return nil
@@ -86,13 +86,13 @@ type mount struct {
 	writable bool
 }
 
-// parseMounts parses the mounts stored under the cli StringSlice argument, `n`
-func parseMounts(c *cli.Context, n string) []mount {
+// parseMounts parses the mounts stored under the cli StringSlice argument, `n`.
+func parseMounts(ctx context.Context, c *cli.Context, n string) []mount {
 	if c.IsSet(n) {
 		ss := c.StringSlice(n)
 		ms := make([]mount, 0, len(ss))
 		for _, s := range ss {
-			logrus.Debugf("parsing %q", s)
+			log.G(ctx).Debugf("parsing %q", s)
 
 			if m, err := mountFromString(s); err == nil {
 				ms = append(ms, m)

internal/tools/uvmboot/wcow.go

@@ -11,11 +11,12 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/containerd/console"
+	"github.com/urfave/cli"
+
 	"github.com/Microsoft/hcsshim/internal/cmd"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/uvm"
-	"github.com/containerd/console"
-	"github.com/urfave/cli"
 )
 
 var (
@@ -121,8 +122,8 @@ var wcowCommand = cli.Command{
 }
 
 func getLayers(imageName string) ([]string, error) {
-	cmd := exec.Command("docker", "inspect", imageName, "-f", `"{{.GraphDriver.Data.dir}}"`)
-	out, err := cmd.Output()
+	c := exec.Command("docker", "inspect", imageName, "-f", `"{{.GraphDriver.Data.dir}}"`)
+	out, err := c.Output()
 	if err != nil {
 		return nil, fmt.Errorf("failed to find layers for %s", imageName)
 	}
@@ -143,7 +144,7 @@ func getLayerChain(layerFolder string) ([]string, error) {
 	var layerChain []string
 	err = json.Unmarshal(content, &layerChain)
 	if err != nil {
-		return nil, fmt.Errorf("failed to unmarshal layerchain: %s", err)
+		return nil, fmt.Errorf("failed to unmarshal layerchain: %w", err)
 	}
 	return layerChain, nil
 }

test/gcs/main_test.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	"flag"
 	"fmt"
-	"log"
 	"os"
 	"path/filepath"
 	"strings"
@@ -61,15 +60,6 @@ var (
 	)
 )
 
-var securityPolicy string
-
-func init() {
-	var err error
-	if securityPolicy, err = securitypolicy.NewOpenDoorPolicy().EncodeToString(); err != nil {
-		log.Fatal("could not encode open door policy to string: %w", err)
-	}
-}
-
 func TestMain(m *testing.M) {
 	flag.Parse()
 
@@ -176,11 +166,11 @@ func getHost(_ context.Context, tb testing.TB, rt runtime.Runtime) *hcsv2.Host {
 }
 
 func getHostErr(rt runtime.Runtime, tp transport.Transport) (*hcsv2.Host, error) {
-	h := hcsv2.NewHost(rt, tp, &securitypolicy.ClosedDoorSecurityPolicyEnforcer{}, os.Stdout)
-	cOpts := &guestresource.LCOWConfidentialOptions{
-		EncodedSecurityPolicy: securityPolicy,
-	}
-	if err := h.SetConfidentialUVMOptions(context.Background(), cOpts); err != nil {
+	h := hcsv2.NewHost(rt, tp, &securitypolicy.OpenDoorSecurityPolicyEnforcer{}, os.Stdout)
+	if err := h.SetConfidentialUVMOptions(
+		context.Background(),
+		&guestresource.LCOWConfidentialOptions{},
+	); err != nil {
 		return nil, fmt.Errorf("could not set host security policy: %w", err)
 	}