From 2ca98378bcf7fe9fa696fbf2d7f6b78eb64fc949 Mon Sep 17 00:00:00 2001 From: Jhon Honce Date: Mon, 10 Feb 2020 14:24:10 -0700 Subject: [PATCH] Fix handler and systemd activation errors On panic from handler: log warning and stack trace, report InternalServerError to client When using `podman system service` make determining the listening endpoint deterministic. // 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 Fixes #5150 Fixes #5151 Signed-off-by: Jhon Honce --- cmd/podman/service.go | 59 ++++++++++++------- pkg/adapter/containers.go | 14 ++--- pkg/api/handlers/types.go | 19 +++--- pkg/api/server/handler_api.go | 49 ++++++++++----- pkg/api/server/server.go | 1 + pkg/systemd/activation.go | 40 +++++++++++++ pkg/{systemdgen => systemd}/systemdgen.go | 2 +- .../systemdgen_test.go | 2 +- 8 files changed, 133 insertions(+), 53 deletions(-) create mode 100644 pkg/systemd/activation.go rename pkg/{systemdgen => systemd}/systemdgen.go (99%) rename pkg/{systemdgen => systemd}/systemdgen_test.go (99%) diff --git a/cmd/podman/service.go b/cmd/podman/service.go index 4978b5d5195a..f4a6bdb7e302 100644 --- a/cmd/podman/service.go +++ b/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" @@ -55,13 +56,20 @@ func init() { } func serviceCmd(c *cliconfig.ServiceValues) error { - // For V2, default to the REST socket - apiURI := adapter.DefaultAPIAddress - if c.Varlink { - apiURI = adapter.DefaultVarlinkAddress - } + var apiURI string + + // 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 rootless.IsRootless() { + 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 @@ -80,15 +88,20 @@ func serviceCmd(c *cliconfig.ServiceValues) error { return err } } - apiURI = fmt.Sprintf("unix:%s", socketDir) + apiURI = "unix:" + socketDir + } else if c.Varlink { + apiURI = adapter.DefaultVarlinkAddress + } else { + // For V2, default to the REST socket + apiURI = adapter.DefaultAPIAddress } - if len(c.InputArgs) > 0 { - apiURI = c.InputArgs[0] + if "" == apiURI { + logrus.Info("using systemd socket activation to determine API endpoint") + } else { + logrus.Infof("using API endpoint: %s", apiURI) } - logrus.Infof("using API endpoint: %s", apiURI) - // Create a single runtime api consumption runtime, err := libpodruntime.GetRuntimeDisableFDs(getContext(), &c.PodmanCommand) if err != nil { @@ -106,17 +119,21 @@ func serviceCmd(c *cliconfig.ServiceValues) error { 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 l *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() } - defer l.Close() - server, err := api.NewServerWithSettings(r, timeout, &l) + server, err := api.NewServerWithSettings(r, timeout, l) if err != nil { return err } diff --git a/pkg/adapter/containers.go b/pkg/adapter/containers.go index f66999ffa7c7..15bea02e4f72 100644 --- a/pkg/adapter/containers.go +++ b/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" "github.com/containers/storage" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -1142,7 +1142,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) (*systemd.ContainerInfo, bool, error) { ctr, err := r.Runtime.LookupContainer(nameOrID) if err != nil { return nil, false, err @@ -1160,7 +1160,7 @@ func (r *LocalRuntime) generateSystemdgenContainerInfo(c *cliconfig.GenerateSyst } name, serviceName := generateServiceName(c, ctr, pod) - info := &systemdgen.ContainerInfo{ + info := &systemd.ContainerInfo{ ServiceName: serviceName, ContainerName: name, RestartPolicy: c.RestartPolicy, @@ -1175,7 +1175,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 := systemd.Options{ Files: c.Files, New: c.New, } @@ -1184,7 +1184,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 systemd.CreateContainerSystemdUnit(info, opts) } // --new does not support pods. @@ -1230,7 +1230,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 := []*systemd.ContainerInfo{podInfo} for ctr, dependencies := range graph.DependencyMap() { // Skip the infra container as we already generated it. if ctr.ID() == infraID { @@ -1260,7 +1260,7 @@ func (r *LocalRuntime) GenerateSystemd(c *cliconfig.GenerateSystemdValues) (stri if i > 0 { builder.WriteByte('\n') } - out, err := systemdgen.CreateContainerSystemdUnit(info, opts) + out, err := systemd.CreateContainerSystemdUnit(info, opts) if err != nil { return "", err } diff --git a/pkg/api/handlers/types.go b/pkg/api/handlers/types.go index 6169adb18a94..9871427754df 100644 --- a/pkg/api/handlers/types.go +++ b/pkg/api/handlers/types.go @@ -354,21 +354,24 @@ 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 { - return nil, err - } - SizeRootFs, err := l.RootFsSize() - if err != nil { - return nil, err - } + var ( + sizeRW int64 + SizeRootFs int64 + err error + ) state, err := l.State() if err != nil { return nil, err } + // FIXME Why do the *Size() calls cause a panic I cannot catch on other states? + if state == define.ContainerStateRunning { + sizeRW, _ = l.RWSize() + SizeRootFs, _ = l.RootFsSize() + } + return &Container{docker.Container{ ID: l.ID(), Names: []string{fmt.Sprintf("/%s", l.Name())}, diff --git a/pkg/api/server/handler_api.go b/pkg/api/server/handler_api.go index 4b93998ee797..e3653ee3de65 100644 --- a/pkg/api/server/handler_api.go +++ b/pkg/api/server/handler_api.go @@ -2,32 +2,51 @@ 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) - } + return func(w http.ResponseWriter, r *http.Request) { + defer func() { + log.Debugf("Recovering APIHandler -- Method: %s URL: %s", r.Method, r.URL.String()) + + err := recover() + if err != nil { + buf := make([]byte, 1<<20) + n := runtime.Stack(buf, true) + log.Warnf("panic: %v, %s", err, buf[:n]) + utils.InternalServerError(w, fmt.Errorf("%v", err)) + } + }() + + fn := 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) + } - // 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) + // 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) - h(w, r) + h(w, r) - shutdownFunc := r.Context().Value("shutdownFunc").(func() error) - if err := shutdownFunc(); err != nil { - log.Errorf("Failed to shutdown Server in APIHandler(): %s", err.Error()) + shutdownFunc := r.Context().Value("shutdownFunc").(func() error) + if err := shutdownFunc(); err != nil { + log.Errorf("Failed to shutdown Server in APIHandler(): %s", err.Error()) + } } - }) + fn(w, r) + } } // VersionedPath prepends the version parsing code diff --git a/pkg/api/server/server.go b/pkg/api/server/server.go index 7bb0f5481999..3f43251d73e7 100644 --- a/pkg/api/server/server.go +++ b/pkg/api/server/server.go @@ -64,6 +64,7 @@ func newServer(runtime *libpod.Runtime, duration time.Duration, listener *net.Li } router := mux.NewRouter().UseEncodedPath() + server := APIServer{ Server: http.Server{ Handler: router, diff --git a/pkg/systemd/activation.go b/pkg/systemd/activation.go new file mode 100644 index 000000000000..705d56277bcc --- /dev/null +++ b/pkg/systemd/activation.go @@ -0,0 +1,40 @@ +package systemd + +import ( + "os" + "strconv" + "strings" +) + +// Determine if podman is running under the socket activation protocol +func SocketActivated() (int, bool) { + pid, pid_found := os.LookupEnv("LISTEN_PID") + fds, fds_found := os.LookupEnv("LISTEN_FDS") + fdnames, fdnames_found := os.LookupEnv("LISTEN_FDNAMES") + + if !(pid_found && fds_found && fdnames_found) { + return 0, false + } + + p, err := strconv.Atoi(pid) + if err != nil || p != os.Getpid() { + return 0, false + } + + nfds, err := strconv.Atoi(fds) + if err != nil || nfds < 1 { + return 0, false + } + + if nfds > 1 { + names := strings.Split(fdnames, ":") + for i, n := range names { + if strings.Contains(n, "podman") { + return 3 + i, true + } + } + } + + // First available file descriptor is always 3. + return 3, true +} diff --git a/pkg/systemdgen/systemdgen.go b/pkg/systemd/systemdgen.go similarity index 99% rename from pkg/systemdgen/systemdgen.go rename to pkg/systemd/systemdgen.go index 26b3b3756630..d4ad32c1fe29 100644 --- a/pkg/systemdgen/systemdgen.go +++ b/pkg/systemd/systemdgen.go @@ -1,4 +1,4 @@ -package systemdgen +package systemd import ( "bytes" diff --git a/pkg/systemdgen/systemdgen_test.go b/pkg/systemd/systemdgen_test.go similarity index 99% rename from pkg/systemdgen/systemdgen_test.go rename to pkg/systemd/systemdgen_test.go index ee2429407768..5189345536f7 100644 --- a/pkg/systemdgen/systemdgen_test.go +++ b/pkg/systemd/systemdgen_test.go @@ -1,4 +1,4 @@ -package systemdgen +package systemd import ( "testing"