PR feedback #1

Signed-off-by: Daniel Canter <dcanter@microsoft.com>

cmd/ncproxy/main.go

@@ -1,30 +1,7 @@
 package main
 
 import (
-	"flag"
-
-	"github.com/Microsoft/hcsshim/cmd/ncproxy/nodenetsvc"
-	"github.com/Microsoft/hcsshim/internal/computeagent"
 	"github.com/sirupsen/logrus"
-	"google.golang.org/grpc"
-)
-
-type nodeNetSvcConn struct {
-	client   nodenetsvc.NodeNetworkServiceClient
-	addr     string
-	grpcConn *grpc.ClientConn
-}
-
-var (
-	configPath    = flag.String("config", "", "Path to JSON configuration file.")
-	registerSvc   = flag.Bool("register-service", false, "Register ncproxy as a Windows service")
-	unregisterSvc = flag.Bool("unregister-service", false, "Unregister ncproxy as a Windows service")
-	runSvc        = flag.Bool("run-service", false, "Run ncproxy as a Windows service")
-	// Global mapping of network namespace ID to shim compute agent ttrpc service.
-	containerIDToShim = make(map[string]computeagent.ComputeAgentService)
-	// Global object representing the connection to the node network service that
-	// ncproxy will be talking to.
-	nodeNetSvcClient *nodeNetSvcConn
 )
 
 func main() {

cmd/ncproxy/run.go

@@ -6,11 +6,13 @@ import (
 	"fmt"
 	"os"
 	"os/signal"
+	"path/filepath"
 	"syscall"
 	"time"
 
 	"github.com/Microsoft/go-winio/pkg/etwlogrus"
 	"github.com/Microsoft/hcsshim/cmd/ncproxy/nodenetsvc"
+	"github.com/Microsoft/hcsshim/internal/computeagent"
 	"github.com/Microsoft/hcsshim/internal/log"
 	"github.com/Microsoft/hcsshim/internal/oc"
 	"github.com/pkg/errors"
@@ -20,16 +22,42 @@ import (
 	"google.golang.org/grpc"
 )
 
+type nodeNetSvcConn struct {
+	client   nodenetsvc.NodeNetworkServiceClient
+	addr     string
+	grpcConn *grpc.ClientConn
+}
+
+var (
+	// Global mapping of network namespace ID to shim compute agent ttrpc service.
+	containerIDToShim = make(map[string]computeagent.ComputeAgentService)
+	// Global object representing the connection to the node network service that
+	// ncproxy will be talking to.
+	nodeNetSvcClient *nodeNetSvcConn
+)
+
+var (
+	configPath    = flag.String("config", "", "Path to JSON configuration file.")
+	registerSvc   = flag.Bool("register-service", false, "Register ncproxy as a Windows service")
+	unregisterSvc = flag.Bool("unregister-service", false, "Unregister ncproxy as a Windows service")
+	runSvc        = flag.Bool("run-service", false, "Run ncproxy as a Windows service")
+)
+
 // Run ncproxy
 func run() error {
 	flag.Parse()
-	// Stop if we are registering or unregistering against Windows SCM.
-	stop, err := registerUnregisterService()
-	if err != nil {
-		return err
+
+	// For both unregistering and registering the service we need to exit out (even on success). -register-service will register
+	// ncproxy's commandline to launch with the -run-service flag set.
+	if *unregisterSvc {
+		if *registerSvc {
+			return errors.New("-register-service and -unregister-service cannot be used together")
+		}
+		return unregisterService()
 	}
-	if stop {
-		return nil
+
+	if *registerSvc {
+		return registerService()
 	}
 
 	// Provider ID: cf9f01fe-87b3-568d-ecef-9f54b7c5ff70
@@ -93,7 +121,7 @@ func run() error {
 	}).Info("starting ncproxy")
 
 	sigChan := make(chan os.Signal, 1)
-	signal.Notify(sigChan, syscall.SIGINT)
+	signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM)
 	defer signal.Stop(sigChan)
 
 	// Create new server and then register NetworkConfigProxyServices.
@@ -110,8 +138,23 @@ func run() error {
 	serveErr := make(chan error, 1)
 
 	// Launch as a Windows Service if necessary
-	if err := launchService(server, serveErr); err != nil {
-		return err
+	if *runSvc {
+		// Setup panic log file
+		panicLog := filepath.Join(filepath.Dir(os.Args[0]), "ncproxy-panic.log")
+		if err := initPanicFile(panicLog); err != nil {
+			return err
+		}
+
+		f, err := os.OpenFile(panicLog, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644)
+		if err != nil {
+			return errors.Wrapf(err, "open log file %q")
+		}
+		logrus.SetOutput(f)
+		defer f.Close()
+
+		if err := launchService(serveErr); err != nil {
+			return err
+		}
 	}
 
 	server.serve(ctx, ttrpcListener, grpcListener, serveErr)

cmd/ncproxy/service.go

@@ -1,13 +1,13 @@
 package main
 
 import (
+	"log"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"time"
 	"unsafe"
 
-	"github.com/pkg/errors"
 	"golang.org/x/sys/windows"
 	"golang.org/x/sys/windows/svc"
 	"golang.org/x/sys/windows/svc/debug"
@@ -16,9 +16,13 @@ import (
 
 const serviceName = "ncproxy"
 
+var (
+	panicFile *os.File
+	oldStderr windows.Handle
+)
+
 type handler struct {
 	fromsvc  chan error
-	s        *server
 	serveErr chan error
 }
 
@@ -51,6 +55,61 @@ const (
 	serviceConfigFailureActions = 2
 )
 
+func initPanicFile(path string) error {
+	panicFile, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
+	if err != nil {
+		return err
+	}
+
+	st, err := panicFile.Stat()
+	if err != nil {
+		return err
+	}
+
+	// If there are contents in the file already, move the file out of the way
+	// and replace it.
+	if st.Size() > 0 {
+		panicFile.Close()
+		os.Rename(path, path+".old")
+		panicFile, err = os.Create(path)
+		if err != nil {
+			return err
+		}
+	}
+
+	// Update STD_ERROR_HANDLE to point to the panic file so that Go writes to
+	// it when it panics. Remember the old stderr to restore it before removing
+	// the panic file.
+	sh := uint32(windows.STD_ERROR_HANDLE)
+	h, err := windows.GetStdHandle(sh)
+	if err != nil {
+		return err
+	}
+	oldStderr = h
+
+	if err := windows.SetStdHandle(sh, windows.Handle(panicFile.Fd())); err != nil {
+		return err
+	}
+
+	// Reset os.Stderr to the panic file (so fmt.Fprintf(os.Stderr,...) actually gets redirected)
+	os.Stderr = os.NewFile(panicFile.Fd(), "/dev/stderr")
+
+	// Force threads that panic to write to stderr (the panicFile handle now).
+	log.SetOutput(os.Stderr)
+	return nil
+}
+
+func removePanicFile() {
+	if st, err := panicFile.Stat(); err == nil {
+		if st.Size() == 0 {
+			sh := uint32(windows.STD_ERROR_HANDLE)
+			_ = windows.SetStdHandle(sh, oldStderr)
+			_ = panicFile.Close()
+			_ = os.Remove(panicFile.Name())
+		}
+	}
+}
+
 func registerService() error {
 	p, err := getServicePath()
 	if err != nil {
@@ -75,7 +134,7 @@ func registerService() error {
 	// Configure the service to launch with the arguments that were just passed.
 	args := []string{"-run-service"}
 	for _, a := range os.Args[1:] {
-		if a != "-register-service" && a != "-unregister-service" {
+		if a != "-register-service" {
 			args = append(args, a)
 		}
 	}
@@ -113,36 +172,14 @@ func unregisterService() error {
 	return s.Delete()
 }
 
-// Returns an indication to stop on successful SCM operation, and an error. For either register or unregister
-// return true so we can exit ncproxy and start it up as a Windows service.
-func registerUnregisterService() (bool, error) {
-	if *unregisterSvc {
-		if *registerSvc {
-			return true, errors.New("-register-service and -unregister-service cannot be used together")
-		}
-		return true, unregisterService()
-	}
-
-	if *registerSvc {
-		return true, registerService()
-	}
-
-	return false, nil
-}
-
 // launchService is the entry point for running ncproxy under SCM.
-func launchService(s *server, serveErr chan error) error {
-	if !*runSvc {
-		return nil
-	}
-
+func launchService(serveErr chan error) error {
 	h := &handler{
 		fromsvc:  make(chan error),
-		s:        s,
 		serveErr: serveErr,
 	}
 
-	interactive, err := svc.IsAnInteractiveSession() // nolint:staticcheck
+	interactive, err := svc.IsAnInteractiveSession()
 	if err != nil {
 		return err
 	}
@@ -178,6 +215,7 @@ Loop:
 		}
 	}
 
+	removePanicFile()
 	h.serveErr <- nil
 	return false, 0
 }