summaryrefslogtreecommitdiff
path: root/pkg/api/handlers/compat
diff options
context:
space:
mode:
authorMatthew Heon <matthew.heon@pm.me>2020-08-24 11:35:01 -0400
committerMatthew Heon <matthew.heon@pm.me>2020-08-27 12:50:22 -0400
commit2ea9dac5e1d00b2820bd7156e3bea4b9fd98c1e6 (patch)
tree4251590df91107c2c0dafe12500bae05bc0c8fed /pkg/api/handlers/compat
parent7ccd821397d03ed545635de2a0b70a68ab4d46db (diff)
downloadpodman-2ea9dac5e1d00b2820bd7156e3bea4b9fd98c1e6.tar.gz
podman-2ea9dac5e1d00b2820bd7156e3bea4b9fd98c1e6.tar.bz2
podman-2ea9dac5e1d00b2820bd7156e3bea4b9fd98c1e6.zip
Send HTTP Hijack headers after successful attach
Our previous flow was to perform a hijack before passing a connection into Libpod, and then Libpod would attach to the container's attach socket and begin forwarding traffic. A problem emerges: we write the attach header as soon as the attach complete. As soon as we write the header, the client assumes that all is ready, and sends a Start request. This Start may be processed *before* we successfully finish attaching, causing us to lose output. The solution is to handle hijacking inside Libpod. Unfortunately, this requires a downright extensive refactor of the Attach and HTTP Exec StartAndAttach code. I think the result is an improvement in some places (a lot more errors will be handled with a proper HTTP error code, before the hijack occurs) but other parts, like the relocation of printing container logs, are just *bad*. Still, we need this fixed now to get CI back into good shape... Fixes #7195 Signed-off-by: Matthew Heon <matthew.heon@pm.me>
Diffstat (limited to 'pkg/api/handlers/compat')
-rw-r--r--pkg/api/handlers/compat/containers_attach.go82
-rw-r--r--pkg/api/handlers/compat/exec.go24
2 files changed, 33 insertions, 73 deletions
diff --git a/pkg/api/handlers/compat/containers_attach.go b/pkg/api/handlers/compat/containers_attach.go
index 2d63ac56d..e20d48d86 100644
--- a/pkg/api/handlers/compat/containers_attach.go
+++ b/pkg/api/handlers/compat/containers_attach.go
@@ -1,12 +1,7 @@
package compat
import (
- "bufio"
- "fmt"
- "io"
- "net"
"net/http"
- "strings"
"github.com/containers/podman/v2/libpod"
"github.com/containers/podman/v2/libpod/define"
@@ -97,75 +92,30 @@ func AttachContainer(w http.ResponseWriter, r *http.Request) {
return
}
- connection, buffer, err := AttachConnection(w, r)
- if err != nil {
- utils.InternalServerError(w, err)
- return
- }
- logrus.Debugf("Hijack for attach of container %s successful", ctr.ID())
+ idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker)
+ hijackChan := make(chan bool, 1)
// Perform HTTP attach.
// HTTPAttach will handle everything about the connection from here on
// (including closing it and writing errors to it).
- if err := ctr.HTTPAttach(connection, buffer, streams, detachKeys, nil, query.Stream, query.Logs); err != nil {
+ if err := ctr.HTTPAttach(r, w, streams, detachKeys, nil, query.Stream, query.Logs, hijackChan); err != nil {
+ hijackComplete := <-hijackChan
+
// We can't really do anything about errors anymore. HTTPAttach
// should be writing them to the connection.
logrus.Errorf("Error attaching to container %s: %v", ctr.ID(), err)
- }
- 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 {
- 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")
- }
- trackedConnection := HijackedConnection{
- Conn: connection,
- idleTracker: idleTracker,
+ if hijackComplete {
+ // We do need to tell the idle tracker that the
+ // connection has been closed, though. We can guarantee
+ // that is true after HTTPAttach exits.
+ idleTracker.TrackHijackedClosed()
+ } else {
+ // A hijack was not successfully completed. We need to
+ // report the error normally.
+ utils.InternalServerError(w, err)
+ }
}
- WriteAttachHeaders(r, trackedConnection)
-
- return trackedConnection, 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)
- }
+ logrus.Debugf("Attach for container %s completed successfully", ctr.ID())
}
diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go
index 7a62a2b58..1db950f85 100644
--- a/pkg/api/handlers/compat/exec.go
+++ b/pkg/api/handlers/compat/exec.go
@@ -10,6 +10,7 @@ import (
"github.com/containers/podman/v2/libpod/define"
"github.com/containers/podman/v2/pkg/api/handlers"
"github.com/containers/podman/v2/pkg/api/handlers/utils"
+ "github.com/containers/podman/v2/pkg/api/server/idletracker"
"github.com/containers/podman/v2/pkg/specgen/generate"
"github.com/gorilla/mux"
"github.com/pkg/errors"
@@ -173,15 +174,24 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
return
}
- connection, buffer, err := AttachConnection(w, r)
- if err != nil {
- utils.InternalServerError(w, err)
- return
- }
- logrus.Debugf("Hijack for attach of container %s exec session %s successful", sessionCtr.ID(), sessionID)
+ idleTracker := r.Context().Value("idletracker").(*idletracker.IdleTracker)
+ hijackChan := make(chan bool, 1)
+
+ if err := sessionCtr.ExecHTTPStartAndAttach(sessionID, r, w, nil, nil, nil, hijackChan); err != nil {
+ hijackComplete := <-hijackChan
- if err := sessionCtr.ExecHTTPStartAndAttach(sessionID, connection, buffer, nil, nil, nil); err != nil {
logrus.Errorf("Error attaching to container %s exec session %s: %v", sessionCtr.ID(), sessionID, err)
+
+ if hijackComplete {
+ // We do need to tell the idle tracker that the
+ // connection has been closed, though. We can guarantee
+ // that is true after HTTPAttach exits.
+ idleTracker.TrackHijackedClosed()
+ } else {
+ // A hijack was not successfully completed. We need to
+ // report the error normally.
+ utils.InternalServerError(w, err)
+ }
}
logrus.Debugf("Attach for container %s exec session %s completed successfully", sessionCtr.ID(), sessionID)