From 7759c8c9f32a5443970ffc11d8db14f8d3e45494 Mon Sep 17 00:00:00 2001 From: "Korhonen Sami (Samlink)" Date: Thu, 9 Jul 2020 12:09:58 +0300 Subject: Fix: Hijacking v2 endpoints to follow rfc 7230 semantics After this patch v2 hijacking endpoints, exec/start and containers/attach follow rfc 7230 specification. Connection will only be upgraded, if client specifies upgrade headers: For tcp connections: Connection: Upgrade Upgrade: tcp For unix socket connections: Connection: Upgrade Upgrade: sock There are currently no checks if upgrade type actually matches with available protocols. Implementation just protocol that client requested Signed-off-by: Sami Korhonen --- pkg/api/handlers/compat/containers_attach.go | 60 +++++++++++++++++++--------- pkg/api/handlers/compat/exec.go | 14 +------ 2 files changed, 44 insertions(+), 30 deletions(-) (limited to 'pkg/api/handlers/compat') diff --git a/pkg/api/handlers/compat/containers_attach.go b/pkg/api/handlers/compat/containers_attach.go index 325f96b40..724b54ac4 100644 --- a/pkg/api/handlers/compat/containers_attach.go +++ b/pkg/api/handlers/compat/containers_attach.go @@ -1,8 +1,12 @@ package compat import ( + "bufio" "fmt" + "io" + "net" "net/http" + "strings" "github.com/containers/libpod/v2/libpod" "github.com/containers/libpod/v2/libpod/define" @@ -12,12 +16,6 @@ import ( "github.com/sirupsen/logrus" ) -// AttachHeader is the literal header sent for upgraded/hijacked connections for -// attach, sourced from Docker at: -// https://raw.githubusercontent.com/moby/moby/b95fad8e51bd064be4f4e58a996924f343846c85/api/server/router/container/container_routes.go -// Using literally to ensure compatibility with existing clients. -const AttachHeader = "HTTP/1.1 101 UPGRADED\r\nContent-Type: application/vnd.docker.raw-stream\r\nConnection: Upgrade\r\nUpgrade: tcp\r\n\r\n" - func AttachContainer(w http.ResponseWriter, r *http.Request) { runtime := r.Context().Value("runtime").(*libpod.Runtime) decoder := r.Context().Value("decoder").(*schema.Decoder) @@ -98,21 +96,11 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) { return } - // Hijack the connection - hijacker, ok := w.(http.Hijacker) - if !ok { - utils.InternalServerError(w, errors.Errorf("unable to hijack connection")) - return - } - - connection, buffer, err := hijacker.Hijack() + connection, buffer, err := AttachConnection(w, r) if err != nil { - utils.InternalServerError(w, errors.Wrapf(err, "error hijacking connection")) + utils.InternalServerError(w, err) return } - - fmt.Fprintf(connection, AttachHeader) - logrus.Debugf("Hijack for attach of container %s successful", ctr.ID()) // Perform HTTP attach. @@ -126,3 +114,39 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) { logrus.Debugf("Attach for container %s completed successfully", ctr.ID()) } + +func AttachConnection(w http.ResponseWriter, r *http.Request) (net.Conn, *bufio.ReadWriter, error) { + // Hijack the connection + hijacker, ok := w.(http.Hijacker) + if !ok { + return nil, nil, errors.Errorf("unable to hijack connection") + } + + connection, buffer, err := hijacker.Hijack() + if err != nil { + return nil, nil, errors.Wrapf(err, "error hijacking connection") + } + + WriteAttachHeaders(r, connection) + + return connection, buffer, nil +} + +func WriteAttachHeaders(r *http.Request, connection io.Writer) { + // AttachHeader is the literal header sent for upgraded/hijacked connections for + // attach, sourced from Docker at: + // https://raw.githubusercontent.com/moby/moby/b95fad8e51bd064be4f4e58a996924f343846c85/api/server/router/container/container_routes.go + // Using literally to ensure compatibility with existing clients. + c := r.Header.Get("Connection") + proto := r.Header.Get("Upgrade") + if len(proto) == 0 || !strings.EqualFold(c, "Upgrade") { + // OK - can't upgrade if not requested or protocol is not specified + fmt.Fprintf(connection, + "HTTP/1.1 200 OK\r\nContent-Type: application/vnd.docker.raw-stream\r\n\r\n") + } else { + // Upraded + fmt.Fprintf(connection, + "HTTP/1.1 101 UPGRADED\r\nContent-Type: application/vnd.docker.raw-stream\r\nConnection: Upgrade\r\nUpgrade: %s\r\n\r\n", + proto) + } +} diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go index aee4196dd..a3b8cb573 100644 --- a/pkg/api/handlers/compat/exec.go +++ b/pkg/api/handlers/compat/exec.go @@ -173,21 +173,11 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) { return } - // Hijack the connection - hijacker, ok := w.(http.Hijacker) - if !ok { - utils.InternalServerError(w, errors.Errorf("unable to hijack connection")) - return - } - - connection, buffer, err := hijacker.Hijack() + connection, buffer, err := AttachConnection(w, r) if err != nil { - utils.InternalServerError(w, errors.Wrapf(err, "error hijacking connection")) + utils.InternalServerError(w, err) return } - - fmt.Fprintf(connection, AttachHeader) - logrus.Debugf("Hijack for attach of container %s exec session %s successful", sessionCtr.ID(), sessionID) if err := sessionCtr.ExecHTTPStartAndAttach(sessionID, connection, buffer, nil, nil, nil); err != nil { -- cgit v1.2.3-54-g00ecf From 79b005e638f110d6e8601816569827dbd0c8fdc2 Mon Sep 17 00:00:00 2001 From: "Korhonen Sami (Samlink)" Date: Thu, 9 Jul 2020 21:42:27 +0300 Subject: Fix: Correct connection counters for hijacked connections This patch fixes connection counters for v2 endpoints Idletracker was moved to a new package to prevent package cycle. Hijacking code still remains in wrong place and should be moved later to isolated package Signed-off-by: Sami Korhonen --- pkg/api/handlers/compat/containers_attach.go | 23 ++++++++- pkg/api/server/handler_api.go | 1 + pkg/api/server/idletracker/idletracker.go | 74 ++++++++++++++++++++++++++++ pkg/api/server/server.go | 72 ++++----------------------- 4 files changed, 106 insertions(+), 64 deletions(-) create mode 100644 pkg/api/server/idletracker/idletracker.go (limited to 'pkg/api/handlers/compat') diff --git a/pkg/api/handlers/compat/containers_attach.go b/pkg/api/handlers/compat/containers_attach.go index 724b54ac4..71586fca4 100644 --- a/pkg/api/handlers/compat/containers_attach.go +++ b/pkg/api/handlers/compat/containers_attach.go @@ -11,6 +11,7 @@ import ( "github.com/containers/libpod/v2/libpod" "github.com/containers/libpod/v2/libpod/define" "github.com/containers/libpod/v2/pkg/api/handlers/utils" + "github.com/containers/libpod/v2/pkg/api/server/idletracker" "github.com/gorilla/schema" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -115,7 +116,21 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) { logrus.Debugf("Attach for container %s completed successfully", ctr.ID()) } +type HijackedConnection struct { + net.Conn // Connection + idleTracker *idletracker.IdleTracker // Connection tracker +} + +func (c HijackedConnection) Close() error { + logrus.Debugf("Hijacked connection closed") + + c.idleTracker.TrackHijackedClosed() + return c.Conn.Close() +} + func AttachConnection(w http.ResponseWriter, r *http.Request) (net.Conn, *bufio.ReadWriter, error) { + idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker) + // Hijack the connection hijacker, ok := w.(http.Hijacker) if !ok { @@ -126,10 +141,14 @@ func AttachConnection(w http.ResponseWriter, r *http.Request) (net.Conn, *bufio. if err != nil { return nil, nil, errors.Wrapf(err, "error hijacking connection") } + trackedConnection := HijackedConnection{ + Conn: connection, + idleTracker: idleTracker, + } - WriteAttachHeaders(r, connection) + WriteAttachHeaders(r, trackedConnection) - return connection, buffer, nil + return trackedConnection, buffer, nil } func WriteAttachHeaders(r *http.Request, connection io.Writer) { diff --git a/pkg/api/server/handler_api.go b/pkg/api/server/handler_api.go index b0fd932ba..53fe8952b 100644 --- a/pkg/api/server/handler_api.go +++ b/pkg/api/server/handler_api.go @@ -37,6 +37,7 @@ func (s *APIServer) APIHandler(h http.HandlerFunc) http.HandlerFunc { c := context.WithValue(r.Context(), "decoder", s.Decoder) //nolint c = context.WithValue(c, "runtime", s.Runtime) //nolint c = context.WithValue(c, "shutdownFunc", s.Shutdown) //nolint + c = context.WithValue(c, "idletracker", s.idleTracker) //nolint r = r.WithContext(c) h(w, r) diff --git a/pkg/api/server/idletracker/idletracker.go b/pkg/api/server/idletracker/idletracker.go new file mode 100644 index 000000000..1ee905a99 --- /dev/null +++ b/pkg/api/server/idletracker/idletracker.go @@ -0,0 +1,74 @@ +package idletracker + +import ( + "net" + "net/http" + "sync" + "time" + + "github.com/sirupsen/logrus" +) + +type IdleTracker struct { + http map[net.Conn]struct{} + hijacked int + total int + mux sync.Mutex + timer *time.Timer + Duration time.Duration +} + +func NewIdleTracker(idle time.Duration) *IdleTracker { + return &IdleTracker{ + http: make(map[net.Conn]struct{}), + Duration: idle, + timer: time.NewTimer(idle), + } +} + +func (t *IdleTracker) ConnState(conn net.Conn, state http.ConnState) { + t.mux.Lock() + defer t.mux.Unlock() + + oldActive := t.ActiveConnections() + logrus.Debugf("IdleTracker %p:%v %d/%d connection(s)", conn, state, oldActive, t.TotalConnections()) + switch state { + case http.StateNew, http.StateActive: + t.http[conn] = struct{}{} + // stop the timer if we transitioned from idle + if oldActive == 0 { + t.timer.Stop() + } + t.total++ + case http.StateHijacked: + // hijacked connections are handled elsewhere + delete(t.http, conn) + t.hijacked++ + case http.StateIdle, http.StateClosed: + delete(t.http, conn) + // Restart the timer if we've become idle + if oldActive > 0 && len(t.http) == 0 { + t.timer.Stop() + t.timer.Reset(t.Duration) + } + } +} + +func (t *IdleTracker) TrackHijackedClosed() { + t.mux.Lock() + defer t.mux.Unlock() + + t.hijacked-- +} + +func (t *IdleTracker) ActiveConnections() int { + return len(t.http) + t.hijacked +} + +func (t *IdleTracker) TotalConnections() int { + return t.total +} + +func (t *IdleTracker) Done() <-chan time.Time { + return t.timer.C +} diff --git a/pkg/api/server/server.go b/pkg/api/server/server.go index f5b17ab68..1c6007745 100644 --- a/pkg/api/server/server.go +++ b/pkg/api/server/server.go @@ -10,12 +10,12 @@ import ( "runtime" goRuntime "runtime" "strings" - "sync" "syscall" "time" "github.com/containers/libpod/v2/libpod" "github.com/containers/libpod/v2/pkg/api/handlers" + "github.com/containers/libpod/v2/pkg/api/server/idletracker" "github.com/coreos/go-systemd/v22/activation" "github.com/gorilla/mux" "github.com/gorilla/schema" @@ -24,14 +24,14 @@ import ( ) type APIServer struct { - http.Server // The HTTP work happens here - *schema.Decoder // Decoder for Query parameters to structs - context.Context // Context to carry objects to handlers - *libpod.Runtime // Where the real work happens - net.Listener // mux for routing HTTP API calls to libpod routines - context.CancelFunc // Stop APIServer - idleTracker *IdleTracker // Track connections to support idle shutdown - pprof *http.Server // Sidecar http server for providing performance data + http.Server // The HTTP work happens here + *schema.Decoder // Decoder for Query parameters to structs + context.Context // Context to carry objects to handlers + *libpod.Runtime // Where the real work happens + net.Listener // mux for routing HTTP API calls to libpod routines + context.CancelFunc // Stop APIServer + idleTracker *idletracker.IdleTracker // Track connections to support idle shutdown + pprof *http.Server // Sidecar http server for providing performance data } // Number of seconds to wait for next request, if exceeded shutdown server @@ -68,7 +68,7 @@ func newServer(runtime *libpod.Runtime, duration time.Duration, listener *net.Li } router := mux.NewRouter().UseEncodedPath() - idle := NewIdleTracker(duration) + idle := idletracker.NewIdleTracker(duration) server := APIServer{ Server: http.Server{ @@ -231,55 +231,3 @@ func (s *APIServer) Shutdown() error { func (s *APIServer) Close() error { return s.Server.Close() } - -type IdleTracker struct { - active map[net.Conn]struct{} - total int - mux sync.Mutex - timer *time.Timer - Duration time.Duration -} - -func NewIdleTracker(idle time.Duration) *IdleTracker { - return &IdleTracker{ - active: make(map[net.Conn]struct{}), - Duration: idle, - timer: time.NewTimer(idle), - } -} - -func (t *IdleTracker) ConnState(conn net.Conn, state http.ConnState) { - t.mux.Lock() - defer t.mux.Unlock() - - oldActive := len(t.active) - logrus.Debugf("IdleTracker %p:%v %d/%d connection(s)", conn, state, t.ActiveConnections(), t.TotalConnections()) - switch state { - case http.StateNew, http.StateActive: - t.active[conn] = struct{}{} - // stop the timer if we transitioned from idle - if oldActive == 0 { - t.timer.Stop() - } - t.total++ - case http.StateIdle, http.StateClosed, http.StateHijacked: - delete(t.active, conn) - // Restart the timer if we've become idle - if oldActive > 0 && len(t.active) == 0 { - t.timer.Stop() - t.timer.Reset(t.Duration) - } - } -} - -func (t *IdleTracker) ActiveConnections() int { - return len(t.active) -} - -func (t *IdleTracker) TotalConnections() int { - return t.total -} - -func (t *IdleTracker) Done() <-chan time.Time { - return t.timer.C -} -- cgit v1.2.3-54-g00ecf From f9526cf02b76f1c006bd344d05e8a40bfe94dbe9 Mon Sep 17 00:00:00 2001 From: zhangguanzhang Date: Tue, 7 Jul 2020 23:58:37 +0800 Subject: fix API: Create container with an invalid configuration Signed-off-by: zhangguanzhang --- pkg/api/handlers/compat/containers_create.go | 33 ++++++++++++++++++++++++++-- test/apiv2/20-containers.at | 13 +++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) (limited to 'pkg/api/handlers/compat') diff --git a/pkg/api/handlers/compat/containers_create.go b/pkg/api/handlers/compat/containers_create.go index 8034a529c..031e30b7e 100644 --- a/pkg/api/handlers/compat/containers_create.go +++ b/pkg/api/handlers/compat/containers_create.go @@ -1,6 +1,7 @@ package compat import ( + "context" "encoding/json" "fmt" "net/http" @@ -40,6 +41,7 @@ func CreateContainer(w http.ResponseWriter, r *http.Request) { } if len(input.HostConfig.Links) > 0 { utils.Error(w, utils.ErrLinkNotSupport.Error(), http.StatusBadRequest, errors.Wrapf(utils.ErrLinkNotSupport, "bad parameter")) + return } newImage, err := runtime.ImageRuntime().NewFromLocal(input.Image) if err != nil { @@ -51,7 +53,7 @@ func CreateContainer(w http.ResponseWriter, r *http.Request) { utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "GetConfig()")) return } - cc, err := makeCreateConfig(containerConfig, input, newImage) + cc, err := makeCreateConfig(r.Context(), containerConfig, input, newImage) if err != nil { utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "makeCreatConfig()")) return @@ -60,7 +62,7 @@ func CreateContainer(w http.ResponseWriter, r *http.Request) { utils.CreateContainer(r.Context(), w, runtime, &cc) } -func makeCreateConfig(containerConfig *config.Config, input handlers.CreateContainerConfig, newImage *image2.Image) (createconfig.CreateConfig, error) { +func makeCreateConfig(ctx context.Context, containerConfig *config.Config, input handlers.CreateContainerConfig, newImage *image2.Image) (createconfig.CreateConfig, error) { var ( err error init bool @@ -79,6 +81,22 @@ func makeCreateConfig(containerConfig *config.Config, input handlers.CreateConta workDir = input.WorkingDir } + if len(input.Entrypoint) == 0 { + entrypointSlice, err := newImage.Entrypoint(ctx) + if err != nil { + return createconfig.CreateConfig{}, err + } + input.Entrypoint = entrypointSlice + } + + if len(input.Cmd) == 0 { + cmdSlice, err := newImage.Cmd(ctx) + if err != nil { + return createconfig.CreateConfig{}, err + } + input.Cmd = cmdSlice + } + stopTimeout := containerConfig.Engine.StopTimeout if input.StopTimeout != nil { stopTimeout = uint(*input.StopTimeout) @@ -217,5 +235,16 @@ func makeCreateConfig(containerConfig *config.Config, input handlers.CreateConta Pid: pidConfig, } + + fullCmd := append(input.Entrypoint, input.Cmd...) + if len(fullCmd) > 0 { + m.PodmanPath = fullCmd[0] + if len(fullCmd) == 1 { + m.Args = fullCmd + } else { + m.Args = fullCmd[1:] + } + } + return m, nil } diff --git a/test/apiv2/20-containers.at b/test/apiv2/20-containers.at index 60f6d97aa..ec50e659f 100644 --- a/test/apiv2/20-containers.at +++ b/test/apiv2/20-containers.at @@ -30,6 +30,19 @@ cid=$(jq -r '.[0].Id' <<<"$output") t DELETE libpod/containers/$cid 204 +# Ensure that API does not occur: Create Container creates an invalid and the container fails to start +# https://github.com/containers/libpod/issues/6799 +CNAME=testArgs +t POST libpod/containers/create?name=${CNAME} Image=${IMAGE} 201 \ + .Id~[0-9a-f]\\{64\\} +t GET libpod/containers/json?limit=1 200 \ + length=1 \ + .[0].Id~[0-9a-f]\\{64\\} +cid=$(jq -r '.[0].Id' <<<"$output") +# This step should start the container properly +t POST libpod/containers/${cid}/start '' 204 +t DELETE libpod/containers/$cid 204 + CNAME=myfoo podman run --name $CNAME $IMAGE -td top t GET libpod/containers/json?all=true 200 \ -- cgit v1.2.3-54-g00ecf From 84076bf95f485bccfd9e03108fb69a1f72b42918 Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Thu, 9 Jul 2020 13:32:20 +0200 Subject: log API: add context to allow for cancelling Add a `context.Context` to the log APIs to allow for cancelling streaming (e.g., via `podman logs -f`). This fixes issues for the remote API where some go routines of the server will continue writing and produce nothing but heat and waste CPU cycles. Signed-off-by: Valentin Rothberg --- libpod/container_api.go | 2 +- libpod/container_log.go | 23 ++++++--- libpod/container_log_linux.go | 16 ++++++- libpod/container_log_unsupported.go | 4 +- pkg/api/handlers/compat/containers_logs.go | 76 +++++++++++++++--------------- pkg/domain/infra/abi/containers.go | 2 +- pkg/varlinkapi/containers.go | 2 +- 7 files changed, 73 insertions(+), 52 deletions(-) (limited to 'pkg/api/handlers/compat') diff --git a/libpod/container_api.go b/libpod/container_api.go index d19dc651b..487f75e67 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -353,7 +353,7 @@ func (c *Container) HTTPAttach(httpCon net.Conn, httpBuf *bufio.ReadWriter, stre logOpts.WaitGroup.Wait() close(logChan) }() - if err := c.ReadLog(logOpts, logChan); err != nil { + if err := c.ReadLog(context.Background(), logOpts, logChan); err != nil { return err } logrus.Debugf("Done reading logs for container %s, %d bytes", c.ID(), logSize) diff --git a/libpod/container_log.go b/libpod/container_log.go index 97936c683..80f8e6e50 100644 --- a/libpod/container_log.go +++ b/libpod/container_log.go @@ -1,6 +1,7 @@ package libpod import ( + "context" "fmt" "os" "time" @@ -13,9 +14,9 @@ import ( ) // Log is a runtime function that can read one or more container logs. -func (r *Runtime) Log(containers []*Container, options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (r *Runtime) Log(ctx context.Context, containers []*Container, options *logs.LogOptions, logChannel chan *logs.LogLine) error { for _, ctr := range containers { - if err := ctr.ReadLog(options, logChannel); err != nil { + if err := ctr.ReadLog(ctx, options, logChannel); err != nil { return err } } @@ -23,25 +24,25 @@ func (r *Runtime) Log(containers []*Container, options *logs.LogOptions, logChan } // ReadLog reads a containers log based on the input options and returns loglines over a channel. -func (c *Container) ReadLog(options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (c *Container) ReadLog(ctx context.Context, options *logs.LogOptions, logChannel chan *logs.LogLine) error { switch c.LogDriver() { case define.NoLogging: return errors.Wrapf(define.ErrNoLogs, "this container is using the 'none' log driver, cannot read logs") case define.JournaldLogging: // TODO Skip sending logs until journald logs can be read - return c.readFromJournal(options, logChannel) + return c.readFromJournal(ctx, options, logChannel) case define.JSONLogging: // TODO provide a separate implementation of this when Conmon // has support. fallthrough case define.KubernetesLogging, "": - return c.readFromLogFile(options, logChannel) + return c.readFromLogFile(ctx, options, logChannel) default: return errors.Wrapf(define.ErrInternal, "unrecognized log driver %q, cannot read logs", c.LogDriver()) } } -func (c *Container) readFromLogFile(options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (c *Container) readFromLogFile(ctx context.Context, options *logs.LogOptions, logChannel chan *logs.LogLine) error { t, tailLog, err := logs.GetLogFile(c.LogPath(), options) if err != nil { // If the log file does not exist, this is not fatal. @@ -62,8 +63,17 @@ func (c *Container) readFromLogFile(options *logs.LogOptions, logChannel chan *l } go func() { + defer options.WaitGroup.Done() + var partial string for line := range t.Lines { + select { + case <-ctx.Done(): + // the consumer has cancelled + return + default: + // fallthrough + } nll, err := logs.NewLogLine(line.Text) if err != nil { logrus.Error(err) @@ -82,7 +92,6 @@ func (c *Container) readFromLogFile(options *logs.LogOptions, logChannel chan *l logChannel <- nll } } - options.WaitGroup.Done() }() // Check if container is still running or paused if options.Follow { diff --git a/libpod/container_log_linux.go b/libpod/container_log_linux.go index fad3bf87c..00b2039a9 100644 --- a/libpod/container_log_linux.go +++ b/libpod/container_log_linux.go @@ -4,6 +4,7 @@ package libpod import ( + "context" "fmt" "io" "math" @@ -29,7 +30,7 @@ const ( bufLen = 16384 ) -func (c *Container) readFromJournal(options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (c *Container) readFromJournal(ctx context.Context, options *logs.LogOptions, logChannel chan *logs.LogLine) error { var config journal.JournalReaderConfig if options.Tail < 0 { config.NumFromTail = math.MaxUint64 @@ -65,13 +66,24 @@ func (c *Container) readFromJournal(options *logs.LogOptions, logChannel chan *l if options.Follow { go func() { + done := make(chan bool) + until := make(chan time.Time) + go func() { + select { + case <-ctx.Done(): + until <- time.Time{} + case <-done: + // nothing to do anymore + } + }() follower := FollowBuffer{logChannel} - err := r.Follow(nil, follower) + err := r.Follow(until, follower) if err != nil { logrus.Debugf(err.Error()) } r.Close() options.WaitGroup.Done() + done <- true return }() return nil diff --git a/libpod/container_log_unsupported.go b/libpod/container_log_unsupported.go index 18882720a..f3b36619e 100644 --- a/libpod/container_log_unsupported.go +++ b/libpod/container_log_unsupported.go @@ -3,11 +3,13 @@ package libpod import ( + "context" + "github.com/containers/libpod/v2/libpod/define" "github.com/containers/libpod/v2/libpod/logs" "github.com/pkg/errors" ) -func (c *Container) readFromJournal(options *logs.LogOptions, logChannel chan *logs.LogLine) error { +func (c *Container) readFromJournal(_ context.Context, _ *logs.LogOptions, _ chan *logs.LogLine) error { return errors.Wrapf(define.ErrOSNotSupported, "Journald logging only enabled with systemd on linux") } diff --git a/pkg/api/handlers/compat/containers_logs.go b/pkg/api/handlers/compat/containers_logs.go index 8147f4d38..30ee030e8 100644 --- a/pkg/api/handlers/compat/containers_logs.go +++ b/pkg/api/handlers/compat/containers_logs.go @@ -92,7 +92,7 @@ func LogsFromContainer(w http.ResponseWriter, r *http.Request) { options.WaitGroup = &wg logChannel := make(chan *logs.LogLine, tail+1) - if err := runtime.Log([]*libpod.Container{ctnr}, options, logChannel); err != nil { + if err := runtime.Log(r.Context(), []*libpod.Container{ctnr}, options, logChannel); err != nil { utils.InternalServerError(w, errors.Wrapf(err, "Failed to obtain logs for Container '%s'", name)) return } @@ -105,50 +105,48 @@ func LogsFromContainer(w http.ResponseWriter, r *http.Request) { var frame strings.Builder header := make([]byte, 8) - for ok := true; ok; ok = query.Follow { - for line := range logChannel { - if _, found := r.URL.Query()["until"]; found { - if line.Time.After(until) { - break - } + for line := range logChannel { + if _, found := r.URL.Query()["until"]; found { + if line.Time.After(until) { + break } + } - // Reset buffer we're ready to loop again - frame.Reset() - switch line.Device { - case "stdout": - if !query.Stdout { - continue - } - header[0] = 1 - case "stderr": - if !query.Stderr { - continue - } - header[0] = 2 - default: - // Logging and moving on is the best we can do here. We may have already sent - // a Status and Content-Type to client therefore we can no longer report an error. - log.Infof("unknown Device type '%s' in log file from Container %s", line.Device, ctnr.ID()) + // Reset buffer we're ready to loop again + frame.Reset() + switch line.Device { + case "stdout": + if !query.Stdout { continue } - - if query.Timestamps { - frame.WriteString(line.Time.Format(time.RFC3339)) - frame.WriteString(" ") + header[0] = 1 + case "stderr": + if !query.Stderr { + continue } - frame.WriteString(line.Msg) + header[0] = 2 + default: + // Logging and moving on is the best we can do here. We may have already sent + // a Status and Content-Type to client therefore we can no longer report an error. + log.Infof("unknown Device type '%s' in log file from Container %s", line.Device, ctnr.ID()) + continue + } - binary.BigEndian.PutUint32(header[4:], uint32(frame.Len())) - if _, err := w.Write(header[0:8]); err != nil { - log.Errorf("unable to write log output header: %q", err) - } - if _, err := io.WriteString(w, frame.String()); err != nil { - log.Errorf("unable to write frame string: %q", err) - } - if flusher, ok := w.(http.Flusher); ok { - flusher.Flush() - } + if query.Timestamps { + frame.WriteString(line.Time.Format(time.RFC3339)) + frame.WriteString(" ") + } + frame.WriteString(line.Msg) + + binary.BigEndian.PutUint32(header[4:], uint32(frame.Len())) + if _, err := w.Write(header[0:8]); err != nil { + log.Errorf("unable to write log output header: %q", err) + } + if _, err := io.WriteString(w, frame.String()); err != nil { + log.Errorf("unable to write frame string: %q", err) + } + if flusher, ok := w.(http.Flusher); ok { + flusher.Flush() } } } diff --git a/pkg/domain/infra/abi/containers.go b/pkg/domain/infra/abi/containers.go index 596fc2cc1..8909f831d 100644 --- a/pkg/domain/infra/abi/containers.go +++ b/pkg/domain/infra/abi/containers.go @@ -924,7 +924,7 @@ func (ic *ContainerEngine) ContainerLogs(ctx context.Context, containers []strin } logChannel := make(chan *logs.LogLine, chSize) - if err := ic.Libpod.Log(ctrs, logOpts, logChannel); err != nil { + if err := ic.Libpod.Log(ctx, ctrs, logOpts, logChannel); err != nil { return err } diff --git a/pkg/varlinkapi/containers.go b/pkg/varlinkapi/containers.go index 8650ba000..07b492331 100644 --- a/pkg/varlinkapi/containers.go +++ b/pkg/varlinkapi/containers.go @@ -754,7 +754,7 @@ func (i *VarlinkAPI) GetContainersLogs(call iopodman.VarlinkCall, names []string if err != nil { return call.ReplyErrorOccurred(err.Error()) } - if err := i.Runtime.Log(containers, &options, logChannel); err != nil { + if err := i.Runtime.Log(getContext(), containers, &options, logChannel); err != nil { return err } go func() { -- cgit v1.2.3-54-g00ecf From 4d7626cc897d821bd8c931d1dde1fa0ab236f4f9 Mon Sep 17 00:00:00 2001 From: Daniel J Walsh Date: Tue, 14 Jul 2020 13:05:25 -0400 Subject: Fix handling of entrypoint If a user specifies an entrypoint of "" then we should not use the images entrypoint. Signed-off-by: Daniel J Walsh --- cmd/podman/common/specgen.go | 12 +++++------- pkg/api/handlers/compat/containers_create.go | 2 +- pkg/specgen/generate/oci.go | 2 +- test/e2e/run_entrypoint_test.go | 5 +++++ 4 files changed, 12 insertions(+), 9 deletions(-) (limited to 'pkg/api/handlers/compat') diff --git a/cmd/podman/common/specgen.go b/cmd/podman/common/specgen.go index 3722d45b2..aa8669e7a 100644 --- a/cmd/podman/common/specgen.go +++ b/cmd/podman/common/specgen.go @@ -365,9 +365,10 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *ContainerCLIOpts, args []string s.Annotations = annotations s.WorkDir = c.Workdir - entrypoint := []string{} userCommand := []string{} + var command []string if c.Entrypoint != nil { + entrypoint := []string{} if ep := *c.Entrypoint; len(ep) > 0 { // Check if entrypoint specified is json if err := json.Unmarshal([]byte(*c.Entrypoint), &entrypoint); err != nil { @@ -375,17 +376,14 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *ContainerCLIOpts, args []string } } s.Entrypoint = entrypoint + // Build the command + // If we have an entry point, it goes first + command = entrypoint } - var command []string // Include the command used to create the container. s.ContainerCreateCommand = os.Args - // Build the command - // If we have an entry point, it goes first - if c.Entrypoint != nil { - command = entrypoint - } if len(inputCommand) > 0 { // User command overrides data CMD command = append(command, inputCommand...) diff --git a/pkg/api/handlers/compat/containers_create.go b/pkg/api/handlers/compat/containers_create.go index 031e30b7e..cbee8a8b6 100644 --- a/pkg/api/handlers/compat/containers_create.go +++ b/pkg/api/handlers/compat/containers_create.go @@ -81,7 +81,7 @@ func makeCreateConfig(ctx context.Context, containerConfig *config.Config, input workDir = input.WorkingDir } - if len(input.Entrypoint) == 0 { + if input.Entrypoint == nil { entrypointSlice, err := newImage.Entrypoint(ctx) if err != nil { return createconfig.CreateConfig{}, err diff --git a/pkg/specgen/generate/oci.go b/pkg/specgen/generate/oci.go index b33fe0e25..f279aac1c 100644 --- a/pkg/specgen/generate/oci.go +++ b/pkg/specgen/generate/oci.go @@ -86,7 +86,7 @@ func makeCommand(ctx context.Context, s *specgen.SpecGenerator, img *image.Image finalCommand := []string{} entrypoint := s.Entrypoint - if len(entrypoint) == 0 && img != nil { + if entrypoint == nil && img != nil { newEntry, err := img.Entrypoint(ctx) if err != nil { return nil, err diff --git a/test/e2e/run_entrypoint_test.go b/test/e2e/run_entrypoint_test.go index c947fa863..e6604a21e 100644 --- a/test/e2e/run_entrypoint_test.go +++ b/test/e2e/run_entrypoint_test.go @@ -101,6 +101,11 @@ ENTRYPOINT ["grep", "Alpine", "/etc/os-release"] session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) Expect(session.LineInOuputStartsWith("Linux")).To(BeTrue()) + + session = podmanTest.Podman([]string{"run", "--entrypoint", "", "foobar.com/entrypoint:latest", "uname"}) + session.WaitWithDefaultTimeout() + Expect(session.ExitCode()).To(Equal(0)) + Expect(session.LineInOuputStartsWith("Linux")).To(BeTrue()) }) It("podman run user entrypoint with command overrides image entrypoint and image cmd", func() { -- cgit v1.2.3-54-g00ecf