Merge pull request #5158 from jwhonce/issues/5151

Fix handler and systemd activation errors

cmd/podman/service.go

@@ -17,6 +17,7 @@ import (
 	"github.com/containers/libpod/pkg/adapter"
 	api "github.com/containers/libpod/pkg/api/server"
 	"github.com/containers/libpod/pkg/rootless"
+	"github.com/containers/libpod/pkg/systemd"
 	"github.com/containers/libpod/pkg/util"
 	"github.com/containers/libpod/pkg/varlinkapi"
 	"github.com/containers/libpod/version"
@@ -50,21 +51,52 @@ func init() {
 	serviceCommand.SetHelpTemplate(HelpTemplate())
 	serviceCommand.SetUsageTemplate(UsageTemplate())
 	flags := serviceCommand.Flags()
-	flags.Int64VarP(&serviceCommand.Timeout, "timeout", "t", 1000, "Time until the service session expires in milliseconds.  Use 0 to disable the timeout")
+	flags.Int64VarP(&serviceCommand.Timeout, "timeout", "t", 5, "Time until the service session expires in seconds.  Use 0 to disable the timeout")
 	flags.BoolVar(&serviceCommand.Varlink, "varlink", false, "Use legacy varlink service instead of REST")
 }
 
 func serviceCmd(c *cliconfig.ServiceValues) error {
-	// For V2, default to the REST socket
-	apiURI := adapter.DefaultAPIAddress
+	apiURI, err := resolveApiURI(c)
+	if err != nil {
+		return err
+	}
+
+	// Create a single runtime api consumption
+	runtime, err := libpodruntime.GetRuntimeDisableFDs(getContext(), &c.PodmanCommand)
+	if err != nil {
+		return errors.Wrapf(err, "error creating libpod runtime")
+	}
+	defer func() {
+		if err := runtime.Shutdown(false); err != nil {
+			fmt.Fprintf(os.Stderr, "Failed to shutdown libpod runtime: %v", err)
+		}
+	}()
+
+	timeout := time.Duration(c.Timeout) * time.Second
 	if c.Varlink {
-		apiURI = adapter.DefaultVarlinkAddress
+		return runVarlink(runtime, apiURI, timeout, c)
 	}
+	return runREST(runtime, apiURI, timeout)
+}
+
+func resolveApiURI(c *cliconfig.ServiceValues) (string, error) {
+	var apiURI string
 
-	if rootless.IsRootless() {
+	// When determining _*THE*_ listening endpoint --
+	// 1) User input wins always
+	// 2) systemd socket activation
+	// 3) rootless honors XDG_RUNTIME_DIR
+	// 4) if varlink -- adapter.DefaultVarlinkAddress
+	// 5) lastly adapter.DefaultAPIAddress
+
+	if len(c.InputArgs) > 0 {
+		apiURI = c.InputArgs[0]
+	} else if ok := systemd.SocketActivated(); ok {
+		apiURI = ""
+	} else if rootless.IsRootless() {
 		xdg, err := util.GetRuntimeDir()
 		if err != nil {
-			return err
+			return "", err
 		}
 		socketName := "podman.sock"
 		if c.Varlink {
@@ -74,53 +106,59 @@ func serviceCmd(c *cliconfig.ServiceValues) error {
 		if _, err := os.Stat(filepath.Dir(socketDir)); err != nil {
 			if os.IsNotExist(err) {
 				if err := os.Mkdir(filepath.Dir(socketDir), 0755); err != nil {
-					return err
+					return "", err
 				}
 			} else {
-				return err
+				return "", err
 			}
 		}
-		apiURI = fmt.Sprintf("unix:%s", socketDir)
-	}
-
-	if len(c.InputArgs) > 0 {
-		apiURI = c.InputArgs[0]
+		apiURI = "unix:" + socketDir
+	} else if c.Varlink {
+		apiURI = adapter.DefaultVarlinkAddress
+	} else {
+		// For V2, default to the REST socket
+		apiURI = adapter.DefaultAPIAddress
 	}
 
-	logrus.Infof("using API endpoint: %s", apiURI)
-
-	// Create a single runtime api consumption
-	runtime, err := libpodruntime.GetRuntimeDisableFDs(getContext(), &c.PodmanCommand)
-	if err != nil {
-		return errors.Wrapf(err, "error creating libpod runtime")
+	if "" == apiURI {
+		logrus.Info("using systemd socket activation to determine API endpoint")
+	} else {
+		logrus.Infof("using API endpoint: %s", apiURI)
 	}
-	defer runtime.DeferredShutdown(false)
-
-	timeout := time.Duration(c.Timeout) * time.Millisecond
-	if c.Varlink {
-		return runVarlink(runtime, apiURI, timeout, c)
-	}
-	return runREST(runtime, apiURI, timeout)
+	return apiURI, nil
 }
 
 func runREST(r *libpod.Runtime, uri string, timeout time.Duration) error {
 	logrus.Warn("This function is EXPERIMENTAL")
 	fmt.Println("This function is EXPERIMENTAL.")
-	fields := strings.Split(uri, ":")
-	if len(fields) == 1 {
-		return errors.Errorf("%s is an invalid socket destination", uri)
-	}
-	address := strings.Join(fields[1:], ":")
-	l, err := net.Listen(fields[0], address)
-	if err != nil {
-		return errors.Wrapf(err, "unable to create socket %s", uri)
+
+	var listener *net.Listener
+	if uri != "" {
+		fields := strings.Split(uri, ":")
+		if len(fields) == 1 {
+			return errors.Errorf("%s is an invalid socket destination", uri)
+		}
+		address := strings.Join(fields[1:], ":")
+		l, err := net.Listen(fields[0], address)
+		if err != nil {
+			return errors.Wrapf(err, "unable to create socket %s", uri)
+		}
+		defer l.Close()
+		listener = &l
 	}
-	defer l.Close()
-	server, err := api.NewServerWithSettings(r, timeout, &l)
+	server, err := api.NewServerWithSettings(r, timeout, listener)
 	if err != nil {
 		return err
 	}
-	return server.Serve()
+	defer func() {
+		if err := server.Shutdown(); err != nil {
+			fmt.Fprintf(os.Stderr, "Error when stopping service: %s", err)
+		}
+	}()
+
+	err = server.Serve()
+	logrus.Debugf("%d/%d Active connections/Total connections\n", server.ActiveConnections, server.TotalConnections)
+	return err
 }
 
 func runVarlink(r *libpod.Runtime, uri string, timeout time.Duration, c *cliconfig.ServiceValues) error {

pkg/adapter/containers.go

@@ -26,7 +26,7 @@ import (
 	"github.com/containers/libpod/libpod/image"
 	"github.com/containers/libpod/libpod/logs"
 	"github.com/containers/libpod/pkg/adapter/shortcuts"
-	"github.com/containers/libpod/pkg/systemdgen"
+	"github.com/containers/libpod/pkg/systemd/generate"
 	"github.com/containers/storage"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
@@ -1154,7 +1154,7 @@ func generateServiceName(c *cliconfig.GenerateSystemdValues, ctr *libpod.Contain
 
 // generateSystemdgenContainerInfo is a helper to generate a
 // systemdgen.ContainerInfo for `GenerateSystemd`.
-func (r *LocalRuntime) generateSystemdgenContainerInfo(c *cliconfig.GenerateSystemdValues, nameOrID string, pod *libpod.Pod) (*systemdgen.ContainerInfo, bool, error) {
+func (r *LocalRuntime) generateSystemdgenContainerInfo(c *cliconfig.GenerateSystemdValues, nameOrID string, pod *libpod.Pod) (*generate.ContainerInfo, bool, error) {
 	ctr, err := r.Runtime.LookupContainer(nameOrID)
 	if err != nil {
 		return nil, false, err
@@ -1172,7 +1172,7 @@ func (r *LocalRuntime) generateSystemdgenContainerInfo(c *cliconfig.GenerateSyst
 	}
 
 	name, serviceName := generateServiceName(c, ctr, pod)
-	info := &systemdgen.ContainerInfo{
+	info := &generate.ContainerInfo{
 		ServiceName:       serviceName,
 		ContainerName:     name,
 		RestartPolicy:     c.RestartPolicy,
@@ -1187,7 +1187,7 @@ func (r *LocalRuntime) generateSystemdgenContainerInfo(c *cliconfig.GenerateSyst
 
 // GenerateSystemd creates a unit file for a container or pod.
 func (r *LocalRuntime) GenerateSystemd(c *cliconfig.GenerateSystemdValues) (string, error) {
-	opts := systemdgen.Options{
+	opts := generate.Options{
 		Files: c.Files,
 		New:   c.New,
 	}
@@ -1196,7 +1196,7 @@ func (r *LocalRuntime) GenerateSystemd(c *cliconfig.GenerateSystemdValues) (stri
 	if info, found, err := r.generateSystemdgenContainerInfo(c, c.InputArgs[0], nil); found && err != nil {
 		return "", err
 	} else if found && err == nil {
-		return systemdgen.CreateContainerSystemdUnit(info, opts)
+		return generate.CreateContainerSystemdUnit(info, opts)
 	}
 
 	// --new does not support pods.
@@ -1242,7 +1242,7 @@ func (r *LocalRuntime) GenerateSystemd(c *cliconfig.GenerateSystemdValues) (stri
 
 	// Traverse the dependency graph and create systemdgen.ContainerInfo's for
 	// each container.
-	containerInfos := []*systemdgen.ContainerInfo{podInfo}
+	containerInfos := []*generate.ContainerInfo{podInfo}
 	for ctr, dependencies := range graph.DependencyMap() {
 		// Skip the infra container as we already generated it.
 		if ctr.ID() == infraID {
@@ -1272,7 +1272,7 @@ func (r *LocalRuntime) GenerateSystemd(c *cliconfig.GenerateSystemdValues) (stri
 		if i > 0 {
 			builder.WriteByte('\n')
 		}
-		out, err := systemdgen.CreateContainerSystemdUnit(info, opts)
+		out, err := generate.CreateContainerSystemdUnit(info, opts)
 		if err != nil {
 			return "", err
 		}

pkg/api/handlers/types.go

@@ -351,18 +351,21 @@ func ImageDataToImageInspect(ctx context.Context, l *libpodImage.Image) (*ImageI
 
 func LibpodToContainer(l *libpod.Container, infoData []define.InfoData) (*Container, error) {
 	imageId, imageName := l.Image()
-	sizeRW, err := l.RWSize()
-	if err != nil {
+
+	var (
+		err        error
+		sizeRootFs int64
+		sizeRW     int64
+		state      define.ContainerStatus
+	)
+
+	if state, err = l.State(); err != nil {
 		return nil, err
 	}
-
-	SizeRootFs, err := l.RootFsSize()
-	if err != nil {
+	if sizeRW, err = l.RWSize(); err != nil {
 		return nil, err
 	}
-
-	state, err := l.State()
-	if err != nil {
+	if sizeRootFs, err = l.RootFsSize(); err != nil {
 		return nil, err
 	}
 
@@ -375,7 +378,7 @@ func LibpodToContainer(l *libpod.Container, infoData []define.InfoData) (*Contai
 		Created:    l.CreatedTime().Unix(),
 		Ports:      nil,
 		SizeRw:     sizeRW,
-		SizeRootFs: SizeRootFs,
+		SizeRootFs: sizeRootFs,
 		Labels:     l.Labels(),
 		State:      string(state),
 		Status:     "",

pkg/api/server/handler_api.go

@@ -2,32 +2,52 @@ package server
 
 import (
 	"context"
+	"fmt"
 	"net/http"
+	"runtime"
 
+	"github.com/containers/libpod/pkg/api/handlers/utils"
 	log "github.com/sirupsen/logrus"
 )
 
 // APIHandler is a wrapper to enhance HandlerFunc's and remove redundant code
-func APIHandler(ctx context.Context, h http.HandlerFunc) http.HandlerFunc {
-	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		log.Debugf("APIHandler -- Method: %s URL: %s", r.Method, r.URL.String())
-		if err := r.ParseForm(); err != nil {
-			log.Infof("Failed Request: unable to parse form: %q", err)
-		}
+func (s *APIServer) APIHandler(h http.HandlerFunc) http.HandlerFunc {
+	return func(w http.ResponseWriter, r *http.Request) {
+		// http.Server hides panics, we want to see them and fix the cause.
+		defer func() {
+			err := recover()
+			if err != nil {
+				buf := make([]byte, 1<<20)
+				n := runtime.Stack(buf, true)
+				log.Warnf("Recovering from podman handler panic: %v, %s", err, buf[:n])
+				// Try to inform client things went south... won't work if handler already started writing response body
+				utils.InternalServerError(w, fmt.Errorf("%v", err))
+			}
+		}()
+
+		// Wrapper to hide some boiler plate
+		fn := func(w http.ResponseWriter, r *http.Request) {
+			// Connection counting, ugh.  Needed to support the sliding window for idle checking.
+			s.ConnectionCh <- EnterHandler
+			defer func() { s.ConnectionCh <- ExitHandler }()
+
+			log.Debugf("APIHandler -- Method: %s URL: %s (conn %d/%d)",
+				r.Method, r.URL.String(), s.ActiveConnections, s.TotalConnections)
 
-		// TODO: Use ConnContext when ported to go 1.13
-		c := context.WithValue(r.Context(), "decoder", ctx.Value("decoder"))
-		c = context.WithValue(c, "runtime", ctx.Value("runtime"))
-		c = context.WithValue(c, "shutdownFunc", ctx.Value("shutdownFunc"))
-		r = r.WithContext(c)
+			if err := r.ParseForm(); err != nil {
+				log.Infof("Failed Request: unable to parse form: %q", err)
+			}
 
-		h(w, r)
+			// TODO: Use r.ConnContext when ported to go 1.13
+			c := context.WithValue(r.Context(), "decoder", s.Decoder)
+			c = context.WithValue(c, "runtime", s.Runtime)
+			c = context.WithValue(c, "shutdownFunc", s.Shutdown)
+			r = r.WithContext(c)
 
-		shutdownFunc := r.Context().Value("shutdownFunc").(func() error)
-		if err := shutdownFunc(); err != nil {
-			log.Errorf("Failed to shutdown Server in APIHandler(): %s", err.Error())
+			h(w, r)
 		}
-	})
+		fn(w, r)
+	}
 }
 
 // VersionedPath prepends the version parsing code

pkg/api/server/register_auth.go

@@ -5,7 +5,7 @@ import (
 	"github.com/gorilla/mux"
 )
 
-func (s *APIServer) RegisterAuthHandlers(r *mux.Router) error {
-	r.Handle(VersionedPath("/auth"), APIHandler(s.Context, handlers.UnsupportedHandler))
+func (s *APIServer) registerAuthHandlers(r *mux.Router) error {
+	r.Handle(VersionedPath("/auth"), s.APIHandler(handlers.UnsupportedHandler))
 	return nil
 }