From 7ff07b6e0c2b1a11d82d61f6ab1e567a1df3abbc Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Fri, 12 Sep 2025 19:13:34 +0200 Subject: [PATCH] pkg/bindings: use HTTP 101 upgrade request for attach For exec and attach use an upgrade request which the server responds with HTTP 101 status. Since go 1.12 the Body can be casted to an io.Writer and then use that to write to the server. This does however not allow us to skip the ugly hack of overwriting the default dialContext() because the ReadWriterCloser on the body does not allow us to call CloseWrite() which is critical to correctly close the stdin side. So we still have to extract the underlying net.Conn for that. Using the cast response.Body is important because the underlying http lib reads from the socket to parse the header and response code of course and it is possible that it read more content than needed that is kept in its buffer but because we then only directly read from the connection it was possible we miss the first few bytes of the message. This should fix the issue and hopefully also some long standing podman-remote missing output flakes in CI. Fixes: #26951 Signed-off-by: Paul Holzinger --- pkg/bindings/containers/attach.go | 132 ++++++++++++++++-------------- 1 file changed, 69 insertions(+), 63 deletions(-) diff --git a/pkg/bindings/containers/attach.go b/pkg/bindings/containers/attach.go index 6b2aab45e9..3a03413f53 100644 --- a/pkg/bindings/containers/attach.go +++ b/pkg/bindings/containers/attach.go @@ -111,37 +111,11 @@ func Attach(ctx context.Context, nameOrID string, stdin io.Reader, stdout io.Wri }() } - headers := make(http.Header) - headers.Add("Connection", "Upgrade") - headers.Add("Upgrade", "tcp") - - var socket net.Conn - socketSet := false - dialContext := conn.Client.Transport.(*http.Transport).DialContext - t := &http.Transport{ - DialContext: func(ctx context.Context, network, address string) (net.Conn, error) { - c, err := dialContext(ctx, network, address) - if err != nil { - return nil, err - } - if !socketSet { - socket = c - socketSet = true - } - return c, err - }, - IdleConnTimeout: time.Duration(0), - } - conn.Client.Transport = t - response, err := conn.DoRequest(ctx, nil, http.MethodPost, "/containers/%s/attach", params, headers, nameOrID) + cw, socket, err := newUpgradeRequest(ctx, conn, nil, fmt.Sprintf("/containers/%s/attach", nameOrID), params) if err != nil { return err } - - if !response.IsSuccess() && !response.IsInformational() { - defer response.Body.Close() - return response.Process(nil) - } + defer socket.Close() if needTTY { winChange := make(chan os.Signal, 1) @@ -173,11 +147,7 @@ func Attach(ctx context.Context, nameOrID string, stdin io.Reader, stdout io.Wri logrus.Errorf("Failed to write input to service: %v", err) } if err == nil { - if closeWrite, ok := socket.(CloseWriter); ok { - if err := closeWrite.CloseWrite(); err != nil { - logrus.Warnf("Failed to close STDIN for writing: %v", err) - } - } + cw.CloseWrite() } stdinChan <- err }() @@ -464,33 +434,11 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar return err } - var socket net.Conn - socketSet := false - dialContext := conn.Client.Transport.(*http.Transport).DialContext - t := &http.Transport{ - DialContext: func(ctx context.Context, network, address string) (net.Conn, error) { - c, err := dialContext(ctx, network, address) - if err != nil { - return nil, err - } - if !socketSet { - socket = c - socketSet = true - } - return c, err - }, - IdleConnTimeout: time.Duration(0), - } - conn.Client.Transport = t - response, err := conn.DoRequest(ctx, bytes.NewReader(bodyJSON), http.MethodPost, "/exec/%s/start", nil, nil, sessionID) + cw, socket, err := newUpgradeRequest(ctx, conn, bytes.NewReader(bodyJSON), fmt.Sprintf("/exec/%s/start", sessionID), nil) if err != nil { return err } - defer response.Body.Close() - - if !response.IsSuccess() && !response.IsInformational() { - return response.Process(nil) - } + defer socket.Close() if needTTY { winChange := make(chan os.Signal, 1) @@ -513,12 +461,7 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar } if err == nil { - if closeWrite, ok := socket.(CloseWriter); ok { - logrus.Debugf("Closing STDIN") - if err := closeWrite.CloseWrite(); err != nil { - logrus.Warnf("Failed to close STDIN for writing: %v", err) - } - } + cw.CloseWrite() } }() } @@ -580,3 +523,66 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar } return nil } + +type closeWrite struct { + // sock is the underlying socket of the connection. + // Do not use that field directly. + sock net.Conn +} + +func (cw *closeWrite) CloseWrite() { + if closeWrite, ok := cw.sock.(CloseWriter); ok { + logrus.Debugf("Closing STDIN") + if err := closeWrite.CloseWrite(); err != nil { + logrus.Warnf("Failed to close STDIN for writing: %v", err) + } + } +} + +// newUpgradeRequest performs a new http Upgrade request, it return the closeWrite which should be used +// to close the STDIN side used and the ReadWriter which MUST be uses to write/read from the connection +// and which must closed when finished. Do not access the new.Conn in closeWrite directly. +func newUpgradeRequest(ctx context.Context, conn *bindings.Connection, body io.Reader, path string, params url.Values) (*closeWrite, io.ReadWriteCloser, error) { + headers := http.Header{ + "Connection": []string{"Upgrade"}, + "Upgrade": []string{"tcp"}, + } + + var socket net.Conn + socketSet := false + dialContext := conn.Client.Transport.(*http.Transport).DialContext + t := &http.Transport{ + DialContext: func(ctx context.Context, network, address string) (net.Conn, error) { + c, err := dialContext(ctx, network, address) + if err != nil { + return nil, err + } + if !socketSet { + socket = c + socketSet = true + } + return c, err + }, + IdleConnTimeout: time.Duration(0), + } + conn.Client.Transport = t + response, err := conn.DoRequest(ctx, body, http.MethodPost, path, params, headers) + if err != nil { + return nil, nil, err + } + + if response.StatusCode != http.StatusSwitchingProtocols { + defer response.Body.Close() + if err := response.Process(nil); err != nil { + return nil, nil, err + } + return nil, nil, fmt.Errorf("incorrect server response code %d, expected %d", response.StatusCode, http.StatusSwitchingProtocols) + } + rw, ok := response.Body.(io.ReadWriteCloser) + if !ok { + response.Body.Close() + return nil, nil, errors.New("internal error: cannot cast to http response Body to io.ReadWriteCloser") + } + + return &closeWrite{sock: socket}, rw, nil +}