containers
/
podman
mirror of https://github.com/containers/podman.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #10683 from Luap99/exec-resize 

Fix resize race with podman exec -it
This commit is contained in:
OpenShift Merge Robot 2021-06-16 15:29:34 -04:00 committed by GitHub
parent b3f61ec38c 666f555aa5
commit 2509a81c34
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
12 changed files with 96 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
libpod/container_exec.go
View File

@ -277,9 +277,10 @@ func (c *Container) ExecStart(sessionID string) error {
}
// ExecStartAndAttach starts and attaches to an exec session in a container.
// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
// TODO: Should we include detach keys in the signature to allow override?
// TODO: How do we handle AttachStdin/AttachStdout/AttachStderr?
func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachStreams) error {
func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachStreams, newSize *define.TerminalSize) error {
if !c.batched {
c.lock.Lock()
defer c.lock.Unlock()
@ -310,7 +311,7 @@ func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachS
return err
}
pid, attachChan, err := c.ociRuntime.ExecContainer(c, session.ID(), opts, streams)
pid, attachChan, err := c.ociRuntime.ExecContainer(c, session.ID(), opts, streams, newSize)
if err != nil {
return err
}
@ -373,7 +374,9 @@ func (c *Container) ExecStartAndAttach(sessionID string, streams *define.AttachS
}
// ExecHTTPStartAndAttach starts and performs an HTTP attach to an exec session.
func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, detachKeys *string, cancel <-chan bool, hijackDone chan<- bool) error {
// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w http.ResponseWriter,
streams *HTTPAttachStreams, detachKeys *string, cancel <-chan bool, hijackDone chan<- bool, newSize *define.TerminalSize) error {
// TODO: How do we combine streams with the default streams set in the exec session?
// Ensure that we don't leak a goroutine here
@ -431,7 +434,7 @@ func (c *Container) ExecHTTPStartAndAttach(sessionID string, r *http.Request, w
close(holdConnOpen)
}()
pid, attachChan, err := c.ociRuntime.ExecContainerHTTP(c, session.ID(), execOpts, r, w, streams, cancel, hijackDone, holdConnOpen)
pid, attachChan, err := c.ociRuntime.ExecContainerHTTP(c, session.ID(), execOpts, r, w, streams, cancel, hijackDone, holdConnOpen, newSize)
if err != nil {
session.State = define.ExecStateStopped
session.ExitCode = define.TranslateExecErrorToExitCode(define.ExecErrorCodeGeneric, err)
@ -719,7 +722,10 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi
// API there.
// TODO: Refactor so this is closed here, before we remove the exec
// session.
var size *define.TerminalSize
if resize != nil {
s := <-resize
size = &s
go func() {
logrus.Debugf("Sending resize events to exec session %s", sessionID)
for resizeRequest := range resize {
@ -737,7 +743,7 @@ func (c *Container) Exec(config *ExecConfig, streams *define.AttachStreams, resi
}()
}
if err := c.ExecStartAndAttach(sessionID, streams); err != nil {
if err := c.ExecStartAndAttach(sessionID, streams, size); err != nil {
return -1, err
}

7
libpod/oci.go
View File

@ -72,13 +72,16 @@ type OCIRuntime interface {
// has completed, as one might expect. The attach session will remain
// running, in a goroutine that will return via the chan error in the
// return signature.
ExecContainer(ctr *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams) (int, chan error, error)
// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
ExecContainer(ctr *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams, newSize *define.TerminalSize) (int, chan error, error)
// ExecContainerHTTP executes a command in a running container and
// attaches its standard streams to a provided hijacked HTTP session.
// Maintains the same invariants as ExecContainer (returns on session
// start, with a goroutine running in the background to handle attach).
// The HTTP attach itself maintains the same invariants as HTTPAttach.
ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool) (int, chan error, error)
// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, r *http.Request, w http.ResponseWriter,
streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, newSize *define.TerminalSize) (int, chan error, error)
// ExecContainerDetached executes a command in a running container, but
// does not attach to it. Returns the PID of the exec session and an
// error (if starting the exec session failed)

13
libpod/oci_attach_linux.go
View File

@ -94,17 +94,18 @@ func (c *Container) attach(streams *define.AttachStreams, keys string, resize <-
// this ensures attachToExec gets all of the output of the called process
// conmon will then send the exit code of the exec process, or an error in the exec session
// startFd must be the input side of the fd.
// newSize resizes the tty to this size before the process is started, must be nil if the exec session has no tty
// conmon will wait to start the exec session until the parent process has setup the console socket.
// Once attachToExec successfully attaches to the console socket, the child conmon process responsible for calling runtime exec
// will read from the output side of start fd, thus learning to start the child process.
// Thus, the order goes as follow:
// 1. conmon parent process sets up its console socket. sends on attachFd
// 2. attachToExec attaches to the console socket after reading on attachFd
// 2. attachToExec attaches to the console socket after reading on attachFd and resizes the tty
// 3. child waits on startFd for attachToExec to attach to said console socket
// 4. attachToExec sends on startFd, signalling it has attached to the socket and child is ready to go
// 5. child receives on startFd, runs the runtime exec command
// attachToExec is responsible for closing startFd and attachFd
func (c *Container) attachToExec(streams *define.AttachStreams, keys *string, sessionID string, startFd, attachFd *os.File) error {
func (c *Container) attachToExec(streams *define.AttachStreams, keys *string, sessionID string, startFd, attachFd *os.File, newSize *define.TerminalSize) error {
if !streams.AttachOutput && !streams.AttachError && !streams.AttachInput {
return errors.Wrapf(define.ErrInvalidArg, "must provide at least one stream to attach to")
}
@ -137,6 +138,14 @@ func (c *Container) attachToExec(streams *define.AttachStreams, keys *string, se
return err
}
// resize before we start the container process
if newSize != nil {
err = c.ociRuntime.ExecAttachResize(c, sessionID, *newSize)
if err != nil {
logrus.Warn("resize failed", err)
}
}
// 2: then attach
conn, err := openUnixSocket(sockPath)
if err != nil {

19
libpod/oci_conmon_exec_linux.go
View File

@ -25,7 +25,7 @@ import (
)
// ExecContainer executes a command in a running container
func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams) (int, chan error, error) {
func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams, newSize *define.TerminalSize) (int, chan error, error) {
if options == nil {
return -1, nil, errors.Wrapf(define.ErrInvalidArg, "must provide an ExecOptions struct to ExecContainer")
}
@ -68,7 +68,7 @@ func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options
attachChan := make(chan error)
go func() {
// attachToExec is responsible for closing pipes
attachChan <- c.attachToExec(streams, options.DetachKeys, sessionID, pipes.startPipe, pipes.attachPipe)
attachChan <- c.attachToExec(streams, options.DetachKeys, sessionID, pipes.startPipe, pipes.attachPipe, newSize)
close(attachChan)
}()
@ -83,7 +83,8 @@ func (r *ConmonOCIRuntime) ExecContainer(c *Container, sessionID string, options
// ExecContainerHTTP executes a new command in an existing container and
// forwards its standard streams over an attach
func (r *ConmonOCIRuntime) ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, req *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool) (int, chan error, error) {
func (r *ConmonOCIRuntime) ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, req *http.Request, w http.ResponseWriter,
streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, newSize *define.TerminalSize) (int, chan error, error) {
if streams != nil {
if !streams.Stdin && !streams.Stdout && !streams.Stderr {
return -1, nil, errors.Wrapf(define.ErrInvalidArg, "must provide at least one stream to attach to")
@ -133,7 +134,7 @@ func (r *ConmonOCIRuntime) ExecContainerHTTP(ctr *Container, sessionID string, o
conmonPipeDataChan := make(chan conmonPipeData)
go func() {
// attachToExec is responsible for closing pipes
attachChan <- attachExecHTTP(ctr, sessionID, req, w, streams, pipes, detachKeys, options.Terminal, cancel, hijackDone, holdConnOpen, execCmd, conmonPipeDataChan, ociLog)
attachChan <- attachExecHTTP(ctr, sessionID, req, w, streams, pipes, detachKeys, options.Terminal, cancel, hijackDone, holdConnOpen, execCmd, conmonPipeDataChan, ociLog, newSize)
close(attachChan)
}()
@ -486,7 +487,7 @@ func (r *ConmonOCIRuntime) startExec(c *Container, sessionID string, options *Ex
}
// Attach to a container over HTTP
func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, pipes *execPipes, detachKeys []byte, isTerminal bool, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, execCmd *exec.Cmd, conmonPipeDataChan chan<- conmonPipeData, ociLog string) (deferredErr error) {
func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, pipes *execPipes, detachKeys []byte, isTerminal bool, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, execCmd *exec.Cmd, conmonPipeDataChan chan<- conmonPipeData, ociLog string, newSize *define.TerminalSize) (deferredErr error) {
// NOTE: As you may notice, the attach code is quite complex.
// Many things happen concurrently and yet are interdependent.
// If you ever change this function, make sure to write to the
@ -524,6 +525,14 @@ func attachExecHTTP(c *Container, sessionID string, r *http.Request, w http.Resp
return err
}
// resize before we start the container process
if newSize != nil {
err = c.ociRuntime.ExecAttachResize(c, sessionID, *newSize)
if err != nil {
logrus.Warn("resize failed", err)
}
}
// 2: then attach
conn, err := openUnixSocket(sockPath)
if err != nil {

5
libpod/oci_missing.go
View File

@ -119,12 +119,13 @@ func (r *MissingRuntime) AttachResize(ctr *Container, newSize define.TerminalSiz
}
// ExecContainer is not available as the runtime is missing
func (r *MissingRuntime) ExecContainer(ctr *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams) (int, chan error, error) {
func (r *MissingRuntime) ExecContainer(ctr *Container, sessionID string, options *ExecOptions, streams *define.AttachStreams, newSize *define.TerminalSize) (int, chan error, error) {
return -1, nil, r.printError()
}
// ExecContainerHTTP is not available as the runtime is missing
func (r *MissingRuntime) ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, req *http.Request, w http.ResponseWriter, streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool) (int, chan error, error) {
func (r *MissingRuntime) ExecContainerHTTP(ctr *Container, sessionID string, options *ExecOptions, req *http.Request, w http.ResponseWriter,
streams *HTTPAttachStreams, cancel <-chan bool, hijackDone chan<- bool, holdConnOpen <-chan bool, newSize *define.TerminalSize) (int, chan error, error) {
return -1, nil, r.printError()
}

10
pkg/api/handlers/compat/exec.go
View File

@ -178,8 +178,16 @@ func ExecStartHandler(w http.ResponseWriter, r *http.Request) {
logrus.Error(errors.Wrapf(e, "error attaching to container %s exec session %s", sessionCtr.ID(), sessionID))
}
var size *define.TerminalSize
if bodyParams.Tty && (bodyParams.Height > 0 || bodyParams.Width > 0) {
size = &define.TerminalSize{
Height: bodyParams.Height,
Width: bodyParams.Width,
}
}
hijackChan := make(chan bool, 1)
err = sessionCtr.ExecHTTPStartAndAttach(sessionID, r, w, nil, nil, nil, hijackChan)
err = sessionCtr.ExecHTTPStartAndAttach(sessionID, r, w, nil, nil, nil, hijackChan, size)
if <-hijackChan {
// If connection was Hijacked, we have to signal it's being closed

2
pkg/api/handlers/compat/resize.go
View File

@ -73,7 +73,7 @@ func ResizeTTY(w http.ResponseWriter, r *http.Request) {
return
}
if err := ctnr.ExecResize(name, sz); err != nil {
if errors.Cause(err) != define.ErrCtrStateInvalid || !query.IgnoreNotRunning {
if errors.Cause(err) != define.ErrExecSessionStateInvalid || !query.IgnoreNotRunning {
utils.InternalServerError(w, errors.Wrapf(err, "cannot resize session"))
return
}

6
pkg/api/handlers/types.go
View File

@ -157,8 +157,10 @@ type ExecCreateResponse struct {
}
type ExecStartConfig struct {
Detach bool `json:"Detach"`
Tty bool `json:"Tty"`
Detach bool `json:"Detach"`
Tty bool `json:"Tty"`
Height uint16 `json:"h"`
Width uint16 `json:"w"`
}
func ImageToImageSummary(l *libimage.Image) (*entities.ImageSummary, error) {

10
pkg/api/server/register_exec.go
View File

@ -269,10 +269,16 @@ func (s *APIServer) registerExecHandlers(r *mux.Router) error {
// properties:
// Detach:
// type: boolean
// description: Detach from the command. Not presently supported.
// description: Detach from the command.
// Tty:
// type: boolean
// description: Allocate a pseudo-TTY. Presently ignored.
// description: Allocate a pseudo-TTY.
// h:
// type: integer
// description: Height of the TTY session in characters. Tty must be set to true to use it.
// w:
// type: integer
// description: Width of the TTY session in characters. Tty must be set to true to use it.
// produces:
// - application/json
// responses:

24
pkg/bindings/containers/attach.go
View File

@ -343,7 +343,7 @@ func attachHandleResize(ctx, winCtx context.Context, winChange chan os.Signal, i
resizeErr = ResizeContainerTTY(ctx, id, new(ResizeTTYOptions).WithHeight(h).WithWidth(w))
}
if resizeErr != nil {
logrus.Warnf("failed to resize TTY: %v", resizeErr)
logrus.Infof("failed to resize TTY: %v", resizeErr)
}
}
@ -408,6 +408,17 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar
// If we are in TTY mode, we need to set raw mode for the terminal.
// TODO: Share all of this with Attach() for containers.
needTTY := terminalFile != nil && terminal.IsTerminal(int(terminalFile.Fd())) && isTerm
body := struct {
Detach bool `json:"Detach"`
TTY bool `json:"Tty"`
Height uint16 `json:"h"`
Width uint16 `json:"w"`
}{
Detach: false,
TTY: needTTY,
}
if needTTY {
state, err := setRawTerminal(terminalFile)
if err != nil {
@ -419,13 +430,14 @@ func ExecStartAndAttach(ctx context.Context, sessionID string, options *ExecStar
}
logrus.SetFormatter(&logrus.TextFormatter{})
}()
w, h, err := terminal.GetSize(int(terminalFile.Fd()))
if err != nil {
logrus.Warnf("failed to obtain TTY size: %v", err)
}
body.Width = uint16(w)
body.Height = uint16(h)
}
body := struct {
Detach bool `json:"Detach"`
}{
Detach: false,
}
bodyJSON, err := json.Marshal(body)
if err != nil {
return err

4
pkg/domain/infra/abi/terminal/terminal_linux.go
View File

@ -15,12 +15,13 @@ import (
// ExecAttachCtr execs and attaches to a container
func ExecAttachCtr(ctx context.Context, ctr *libpod.Container, execConfig *libpod.ExecConfig, streams *define.AttachStreams) (int, error) {
resize := make(chan define.TerminalSize)
var resize chan define.TerminalSize
haveTerminal := terminal.IsTerminal(int(os.Stdin.Fd()))
// Check if we are attached to a terminal. If we are, generate resize
// events, and set the terminal to raw mode
if haveTerminal && execConfig.Terminal {
resize = make(chan define.TerminalSize)
cancel, oldTermState, err := handleTerminalAttach(ctx, resize)
if err != nil {
return -1, err
@ -32,7 +33,6 @@ func ExecAttachCtr(ctx context.Context, ctr *libpod.Container, execConfig *libpo
}
}()
}
return ctr.Exec(execConfig, streams, resize)
}

11
test/system/450-interactive.bats
View File

@ -57,7 +57,16 @@ function teardown() {
# ...and make sure stty under podman reads that.
run_podman run -it --name mystty $IMAGE stty size <$PODMAN_TEST_PTY
is "$output" "$rows $cols" "stty under podman reads the correct dimensions"
is "$output" "$rows $cols" "stty under podman run reads the correct dimensions"
run_podman rm -f mystty
# check that the same works for podman exec
run_podman run -d --name mystty $IMAGE top
run_podman exec -it mystty stty size <$PODMAN_TEST_PTY
is "$output" "$rows $cols" "stty under podman exec reads the correct dimensions"
run_podman rm -f mystty
}