summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorOpenShift Merge Robot <openshift-merge-robot@users.noreply.github.com>2021-06-16 15:29:34 -0400
committerGitHub <noreply@github.com>2021-06-16 15:29:34 -0400
commit2509a81c343314fa452a0215d05e9d74ab4ec15c (patch)
tree18252074d42b6adfa4021e89a80d6c2f399f0a34
parentb3f61ec38cda171caf8e8624a9ec875ba85bc20d (diff)
parent666f555aa52b9f82da9d6ca64cf11e4f5e1e78e8 (diff)
downloadpodman-2509a81c343314fa452a0215d05e9d74ab4ec15c.tar.gz
podman-2509a81c343314fa452a0215d05e9d74ab4ec15c.tar.bz2
podman-2509a81c343314fa452a0215d05e9d74ab4ec15c.zip
Merge pull request #10683 from Luap99/exec-resize
Fix resize race with podman exec -it
-rw-r--r--libpod/container_exec.go16
-rw-r--r--libpod/oci.go7
-rw-r--r--libpod/oci_attach_linux.go13
-rw-r--r--libpod/oci_conmon_exec_linux.go19
-rw-r--r--libpod/oci_missing.go5
-rw-r--r--pkg/api/handlers/compat/exec.go10
-rw-r--r--pkg/api/handlers/compat/resize.go2
-rw-r--r--pkg/api/handlers/types.go6
-rw-r--r--pkg/api/server/register_exec.go10
-rw-r--r--pkg/bindings/containers/attach.go24
-rw-r--r--pkg/domain/infra/abi/terminal/terminal_linux.go4
-rw-r--r--test/system/450-interactive.bats11
12 files changed, 96 insertions, 31 deletions
diff --git a/libpod/container_exec.go b/libpod/container_exec.go
index 737bf74ad..5d4bcb422 100644
--- a/libpod/container_exec.go
+++ b/libpod/container_exec.go
@@ -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
}
diff --git a/libpod/oci.go b/libpod/oci.go
index 1f2c7dd71..c92d9a077 100644
--- a/libpod/oci.go
+++ b/libpod/oci.go
@@ -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)
diff --git a/libpod/oci_attach_linux.go b/libpod/oci_attach_linux.go
index b5040de3e..de435b58a 100644
--- a/libpod/oci_attach_linux.go
+++ b/libpod/oci_attach_linux.go
@@ -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 {
diff --git a/libpod/oci_conmon_exec_linux.go b/libpod/oci_conmon_exec_linux.go
index 76338b86c..09d3d1833 100644
--- a/libpod/oci_conmon_exec_linux.go
+++ b/libpod/oci_conmon_exec_linux.go
@@ -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 {
diff --git a/libpod/oci_missing.go b/libpod/oci_missing.go
index 10526f368..fcf2ffca8 100644
--- a/libpod/oci_missing.go
+++ b/libpod/oci_missing.go
@@ -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()
}
diff --git a/pkg/api/handlers/compat/exec.go b/pkg/api/handlers/compat/exec.go
index 1b7b884e0..77e62c112 100644
--- a/pkg/api/handlers/compat/exec.go
+++ b/pkg/api/handlers/compat/exec.go
@@ -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
diff --git a/pkg/api/handlers/compat/resize.go b/pkg/api/handlers/compat/resize.go
index f65e313fc..844fb74c4 100644
--- a/pkg/api/handlers/compat/resize.go
+++ b/pkg/api/handlers/compat/resize.go
@@ -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
}
diff --git a/pkg/api/handlers/types.go b/pkg/api/handlers/types.go
index f94c9a1f5..ee157cb56 100644
--- a/pkg/api/handlers/types.go
+++ b/pkg/api/handlers/types.go
@@ -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) {
diff --git a/pkg/api/server/register_exec.go b/pkg/api/server/register_exec.go
index 3716ef6a2..e353d714c 100644
--- a/pkg/api/server/register_exec.go
+++ b/pkg/api/server/register_exec.go
@@ -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:
diff --git a/pkg/bindings/containers/attach.go b/pkg/bindings/containers/attach.go
index adef1e7c8..cc12c8ab7 100644
--- a/pkg/bindings/containers/attach.go
+++ b/pkg/bindings/containers/attach.go
@@ -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
diff --git a/pkg/domain/infra/abi/terminal/terminal_linux.go b/pkg/domain/infra/abi/terminal/terminal_linux.go
index ab71f8f6f..09c0f802d 100644
--- a/pkg/domain/infra/abi/terminal/terminal_linux.go
+++ b/pkg/domain/infra/abi/terminal/terminal_linux.go
@@ -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)
}
diff --git a/test/system/450-interactive.bats b/test/system/450-interactive.bats
index a2db39492..53925b3c8 100644
--- a/test/system/450-interactive.bats
+++ b/test/system/450-interactive.bats
@@ -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
}