From 2a474c88c9ffc7221b09513ad4db8720ca7661cb Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 25 Jun 2019 13:50:30 -0400 Subject: Finish up remote exec implementation Signed-off-by: Peter Hunt --- pkg/adapter/containers_remote.go | 216 ++++++++++++++++++++++----------------- 1 file changed, 124 insertions(+), 92 deletions(-) (limited to 'pkg/adapter/containers_remote.go') diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go index 6b9fc8ee7..665821bb6 100644 --- a/pkg/adapter/containers_remote.go +++ b/pkg/adapter/containers_remote.go @@ -3,6 +3,7 @@ package adapter import ( + "bufio" "context" "encoding/json" "fmt" @@ -555,93 +556,6 @@ func (r *LocalRuntime) Ps(c *cliconfig.PsValues, opts shared.PsOptions) ([]share return psContainers, nil } -func (r *LocalRuntime) attach(ctx context.Context, stdin, stdout *os.File, cid string, start bool, detachKeys string) (chan error, error) { - var ( - oldTermState *term.State - ) - errChan := make(chan error) - spec, err := r.Spec(cid) - if err != nil { - return nil, err - } - resize := make(chan remotecommand.TerminalSize, 5) - 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 && spec.Process.Terminal { - logrus.Debugf("Handling terminal attach") - - subCtx, cancel := context.WithCancel(ctx) - defer cancel() - - resizeTty(subCtx, resize) - oldTermState, err = term.SaveState(os.Stdin.Fd()) - if err != nil { - return nil, errors.Wrapf(err, "unable to save terminal state") - } - - logrus.SetFormatter(&RawTtyFormatter{}) - term.SetRawTerminal(os.Stdin.Fd()) - - } - // TODO add detach keys support - reply, err := iopodman.Attach().Send(r.Conn, varlink.Upgrade, cid, detachKeys, start) - if err != nil { - restoreTerminal(oldTermState) - return nil, err - } - - // See if the server accepts the upgraded connection or returns an error - _, err = reply() - - if err != nil { - restoreTerminal(oldTermState) - return nil, err - } - - // These are the varlink sockets - reader := r.Conn.Reader - writer := r.Conn.Writer - - // These are the special writers that encode input from the client. - varlinkStdinWriter := virtwriter.NewVirtWriteCloser(writer, virtwriter.ToStdin) - varlinkResizeWriter := virtwriter.NewVirtWriteCloser(writer, virtwriter.TerminalResize) - - go func() { - // Read from the wire and direct to stdout or stderr - err := virtwriter.Reader(reader, stdout, os.Stderr, nil, nil) - defer restoreTerminal(oldTermState) - errChan <- err - }() - - go func() { - for termResize := range resize { - b, err := json.Marshal(termResize) - if err != nil { - defer restoreTerminal(oldTermState) - errChan <- err - } - _, err = varlinkResizeWriter.Write(b) - if err != nil { - defer restoreTerminal(oldTermState) - errChan <- err - } - } - }() - - // Takes stdinput and sends it over the wire after being encoded - go func() { - if _, err := io.Copy(varlinkStdinWriter, stdin); err != nil { - defer restoreTerminal(oldTermState) - errChan <- err - } - - }() - return errChan, nil - -} - // Attach to a remote terminal func (r *LocalRuntime) Attach(ctx context.Context, c *cliconfig.AttachValues) error { ctr, err := r.LookupContainer(c.InputArgs[0]) @@ -796,6 +710,50 @@ func (r *LocalRuntime) Start(ctx context.Context, c *cliconfig.StartValues, sigP return exitCode, finalErr } +func (r *LocalRuntime) attach(ctx context.Context, stdin, stdout *os.File, cid string, start bool, detachKeys string) (chan error, error) { + var ( + oldTermState *term.State + ) + spec, err := r.Spec(cid) + if err != nil { + return nil, err + } + resize := make(chan remotecommand.TerminalSize, 5) + 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 && spec.Process.Terminal { + cancel, oldTermState, err := handleTerminalAttach(ctx, resize) + if err != nil { + return nil, err + } + defer cancel() + defer restoreTerminal(oldTermState) + + logrus.SetFormatter(&RawTtyFormatter{}) + term.SetRawTerminal(os.Stdin.Fd()) + } + + // TODO add detach keys support + reply, err := iopodman.Attach().Send(r.Conn, varlink.Upgrade, cid, detachKeys, start) + if err != nil { + restoreTerminal(oldTermState) + return nil, err + } + + // See if the server accepts the upgraded connection or returns an error + _, err = reply() + + if err != nil { + restoreTerminal(oldTermState) + return nil, err + } + + errChan := configureVarlinkAttachStdio(r.Conn.Reader, r.Conn.Writer, stdin, stdout, oldTermState, resize, nil) + return errChan, nil +} + // PauseContainers pauses container(s) based on CLI inputs. func (r *LocalRuntime) PauseContainers(ctx context.Context, cli *cliconfig.PauseValues) ([]string, map[string]error, error) { var ( @@ -1037,8 +995,11 @@ func (r *LocalRuntime) Commit(ctx context.Context, c *cliconfig.CommitValues, co // ExecContainer executes a command in the container func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecValues) (int, error) { + var ( + oldTermState *term.State + ec int = 125 + ) // default invalid command exit code - ec := 125 // Validate given environment variables env := map[string]string{} if err := parse.ReadKVStrings(env, []string{}, cli.Env); err != nil { @@ -1051,6 +1012,24 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal envs = append(envs, fmt.Sprintf("%s=%s", k, v)) } + resize := make(chan remotecommand.TerminalSize, 5) + 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 + // TODO FIXME tty + if haveTerminal && cli.Tty { + cancel, oldTermState, err := handleTerminalAttach(ctx, resize) + if err != nil { + return ec, err + } + defer cancel() + defer restoreTerminal(oldTermState) + + logrus.SetFormatter(&RawTtyFormatter{}) + term.SetRawTerminal(os.Stdin.Fd()) + } + opts := iopodman.ExecOpts{ Name: cli.InputArgs[0], Tty: cli.Tty, @@ -1061,16 +1040,69 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal Env: &envs, } - receive, err := iopodman.ExecContainer().Send(r.Conn, varlink.Upgrade, opts) + inputStream := os.Stdin + if !cli.Interactive { + inputStream = nil + } + + reply, err := iopodman.ExecContainer().Send(r.Conn, varlink.Upgrade, opts) if err != nil { return ec, errors.Wrapf(err, "Exec failed to contact service for %s", cli.InputArgs) } - _, err = receive() + _, err = reply() if err != nil { return ec, errors.Wrapf(err, "Exec operation failed for %s", cli.InputArgs) } - // TODO return exit code from exec call - return 0, nil + ecChan := make(chan int, 1) + errChan := configureVarlinkAttachStdio(r.Conn.Reader, r.Conn.Writer, inputStream, os.Stdout, oldTermState, resize, ecChan) + + select { + case err = <-errChan: + break + case ec = <-ecChan: + break + } + + return ec, err +} + +func configureVarlinkAttachStdio(reader *bufio.Reader, writer *bufio.Writer, stdin *os.File, stdout *os.File, oldTermState *term.State, resize chan remotecommand.TerminalSize, ecChan chan int) chan error { + var errChan chan error + // These are the special writers that encode input from the client. + varlinkStdinWriter := virtwriter.NewVirtWriteCloser(writer, virtwriter.ToStdin) + varlinkResizeWriter := virtwriter.NewVirtWriteCloser(writer, virtwriter.TerminalResize) + + go func() { + // Read from the wire and direct to stdout or stderr + err := virtwriter.Reader(reader, stdout, os.Stderr, nil, nil, ecChan) + defer restoreTerminal(oldTermState) + errChan <- err + }() + + go func() { + for termResize := range resize { + b, err := json.Marshal(termResize) + if err != nil { + defer restoreTerminal(oldTermState) + errChan <- err + } + _, err = varlinkResizeWriter.Write(b) + if err != nil { + defer restoreTerminal(oldTermState) + errChan <- err + } + } + }() + + // Takes stdinput and sends it over the wire after being encoded + go func() { + if _, err := io.Copy(varlinkStdinWriter, stdin); err != nil { + defer restoreTerminal(oldTermState) + errChan <- err + } + + }() + return errChan } -- cgit v1.2.3-54-g00ecf From 35ba77e0409036c455f85d9f8fcbe361f0693335 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Fri, 19 Jul 2019 16:19:17 -0400 Subject: Update e2e tests for remote exec including changing -l to the container id and separating a case of setting the env that remote can't handle Signed-off-by: Peter Hunt --- pkg/adapter/containers_remote.go | 9 +++---- pkg/varlinkapi/containers.go | 43 +++++++++++++++------------------ pkg/varlinkapi/virtwriter/virtwriter.go | 11 ++++----- test/e2e/exec_test.go | 28 +++++++++++++++------ 4 files changed, 48 insertions(+), 43 deletions(-) (limited to 'pkg/adapter/containers_remote.go') diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go index 665821bb6..7a9cf94b7 100644 --- a/pkg/adapter/containers_remote.go +++ b/pkg/adapter/containers_remote.go @@ -1054,15 +1054,14 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal if err != nil { return ec, errors.Wrapf(err, "Exec operation failed for %s", cli.InputArgs) } - ecChan := make(chan int, 1) errChan := configureVarlinkAttachStdio(r.Conn.Reader, r.Conn.Writer, inputStream, os.Stdout, oldTermState, resize, ecChan) select { - case err = <-errChan: - break - case ec = <-ecChan: - break + case ec = <-ecChan: + break + case err = <-errChan: + break } return ec, err diff --git a/pkg/varlinkapi/containers.go b/pkg/varlinkapi/containers.go index 4714f3fa0..a70a87675 100644 --- a/pkg/varlinkapi/containers.go +++ b/pkg/varlinkapi/containers.go @@ -800,51 +800,46 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO call.ReplyExecContainer() resizeChan := make(chan remotecommand.TerminalSize) - errChan := make(chan error) reader, writer, _, pipeWriter, streams := setupStreams(call) //reader, _, _, pipeWriter, streams := setupStreams(call) - ecChan := make(chan uint32, 1) + type ExitCodeError struct { + ExitCode uint32 + Error error + } + ecErrChan := make(chan ExitCodeError, 1) go func() { - fmt.Printf("ExecContainer Start Reader\n") if err := virtwriter.Reader(reader, nil, nil, pipeWriter, resizeChan, nil); err != nil { - //fmt.Printf("ExecContainer Reader err %s, %s\n", err.Error(), errors.Cause(err).Error()) - errChan <- errors.Wrapf(err, "error") + ecErrChan <- ExitCodeError{ + 125, //TODO FIXME magic number, define package? + err, + } } }() - fmt.Printf("ExecContainer Start ctr.Exec\n") - // TODO detach keys and resize - // TODO add handling for exit code - // TODO capture exit code and return to main thread + // TODO FIXME detach keys go func() { ec, err := ctr.Exec(opts.Tty, opts.Privileged, envs, opts.Cmd, user, workDir, streams, 0, resizeChan, "") if err != nil { logrus.Errorf("ExecContainer Exec err %s, %s\n", err.Error(), errors.Cause(err).Error()) - errChan <-err } - ecChan <-uint32(ec) - + ecErrChan <- ExitCodeError{ + uint32(ec), + err, + } }() - ec := uint32(125) - var execErr error - select { - case execErr = <-errChan: - fmt.Println(execErr.Error()) - case ec = <-ecChan: - fmt.Println("found", ec) - } + ecErr := <-ecErrChan - // TODO FIXME prevent all of vthese conversions - if err = virtwriter.HangUp(writer, int(ec)); err != nil { + // TODO FIXME prevent all of these conversions + if err = virtwriter.HangUp(writer, int(ecErr.ExitCode)); err != nil { logrus.Errorf("ExecContainer failed to HANG-UP on %s: %s", ctr.ID(), err.Error()) } - defer fmt.Println("Succeeded in exec'ing") + if err := call.Writer.Flush(); err != nil { logrus.Errorf("Exec Container err: %s", err.Error()) } - return execErr + return ecErr.Error } diff --git a/pkg/varlinkapi/virtwriter/virtwriter.go b/pkg/varlinkapi/virtwriter/virtwriter.go index 23f945704..8cdedeed8 100644 --- a/pkg/varlinkapi/virtwriter/virtwriter.go +++ b/pkg/varlinkapi/virtwriter/virtwriter.go @@ -113,7 +113,7 @@ func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remote if output != nil { _, err := io.CopyN(output, r, messageSize) if err != nil { - return errors.Wrapf(err, "issue stdout") + return errors.Wrapf(err, "issue stdout") } } case ToStderr: @@ -121,14 +121,14 @@ func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remote _, err := io.CopyN(errput, r, messageSize) if err != nil { return err - return errors.Wrapf(err, "issue stderr") + return errors.Wrapf(err, "issue stderr") } } case ToStdin: if input != nil { _, err := io.CopyN(input, r, messageSize) if err != nil { - return errors.Wrapf(err, "issue stdin") + return errors.Wrapf(err, "issue stdin") } } case TerminalResize: @@ -159,7 +159,7 @@ func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remote } if execEcChan != nil { ecInt := binary.BigEndian.Uint32(out) - execEcChan <-int(ecInt) + execEcChan <- int(ecInt) } return nil @@ -175,8 +175,7 @@ func HangUp(writer *bufio.Writer, ec int) (err error) { n := 0 msg := make([]byte, 4) - binary.LittleEndian.PutUint32(msg, uint32(ec)) - + binary.BigEndian.PutUint32(msg, uint32(ec)) writeQuit := NewVirtWriteCloser(writer, Quit) if n, err = writeQuit.Write(msg); err != nil { diff --git a/test/e2e/exec_test.go b/test/e2e/exec_test.go index 316575b0f..bd4fb7aad 100644 --- a/test/e2e/exec_test.go +++ b/test/e2e/exec_test.go @@ -65,6 +65,8 @@ var _ = Describe("Podman exec", func() { }) It("podman exec simple command using latest", func() { + // the remote client doesn't use latest + SkipIfRemote() setup := podmanTest.RunTopContainer("test1") setup.WaitWithDefaultTimeout() Expect(setup.ExitCode()).To(Equal(0)) @@ -75,27 +77,37 @@ var _ = Describe("Podman exec", func() { }) It("podman exec environment test", func() { + // passing environment variables is not supported in the remote client + SkipIfRemote() setup := podmanTest.RunTopContainer("test1") setup.WaitWithDefaultTimeout() Expect(setup.ExitCode()).To(Equal(0)) - session := podmanTest.Podman([]string{"exec", "-l", "--env", "FOO=BAR", "printenv", "FOO"}) + session := podmanTest.Podman([]string{"exec", "--env", "FOO=BAR", "test1", "printenv", "FOO"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) match, _ := session.GrepString("BAR") Expect(match).Should(BeTrue()) - session = podmanTest.Podman([]string{"exec", "-l", "--env", "PATH=/bin", "printenv", "PATH"}) + session = podmanTest.Podman([]string{"exec", "--env", "PATH=/bin", "test1", "printenv", "PATH"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) match, _ = session.GrepString("/bin") Expect(match).Should(BeTrue()) + }) + + It("podman exec os.Setenv env", func() { + // remote doesn't properly interpret os.Setenv + SkipIfRemote() + setup := podmanTest.RunTopContainer("test1") + setup.WaitWithDefaultTimeout() + Expect(setup.ExitCode()).To(Equal(0)) os.Setenv("FOO", "BAR") - session = podmanTest.Podman([]string{"exec", "-l", "--env", "FOO", "printenv", "FOO"}) + session := podmanTest.Podman([]string{"exec", "--env", "FOO", "test1", "printenv", "FOO"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) - match, _ = session.GrepString("BAR") + match, _ := session.GrepString("BAR") Expect(match).Should(BeTrue()) os.Unsetenv("FOO") @@ -141,13 +153,13 @@ var _ = Describe("Podman exec", func() { setup.WaitWithDefaultTimeout() Expect(setup.ExitCode()).To(Equal(0)) - session := podmanTest.Podman([]string{"exec", "-l", "--workdir", "/tmp", "pwd"}) + session := podmanTest.Podman([]string{"exec", "--workdir", "/tmp", "test1", "pwd"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) match, _ := session.GrepString("/tmp") Expect(match).Should(BeTrue()) - session = podmanTest.Podman([]string{"exec", "-l", "-w", "/tmp", "pwd"}) + session = podmanTest.Podman([]string{"exec", "-w", "/tmp", "test1", "pwd"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(0)) match, _ = session.GrepString("/tmp") @@ -159,11 +171,11 @@ var _ = Describe("Podman exec", func() { setup.WaitWithDefaultTimeout() Expect(setup.ExitCode()).To(Equal(0)) - session := podmanTest.Podman([]string{"exec", "-l", "--workdir", "/missing", "pwd"}) + session := podmanTest.Podman([]string{"exec", "--workdir", "/missing", "test1", "pwd"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(1)) - session = podmanTest.Podman([]string{"exec", "-l", "-w", "/missing", "pwd"}) + session = podmanTest.Podman([]string{"exec", "-w", "/missing", "test1", "pwd"}) session.WaitWithDefaultTimeout() Expect(session.ExitCode()).To(Equal(1)) }) -- cgit v1.2.3-54-g00ecf From 479eeac62cd74e32cbe74fc8afbfc82d4d8a8abd Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Mon, 22 Jul 2019 15:12:29 -0400 Subject: move editing of exitCode to runtime There's no way to get the error if we successfully get an exit code (as it's just printed to stderr instead). instead of relying on the error to be passed to podman, and edit based on the error code, process it on the varlink side instead Also move error codes to define package Signed-off-by: Peter Hunt --- cmd/podman/exec.go | 7 ------- libpod/container_api.go | 15 +++++---------- libpod/define/exec_codes.go | 13 +++++++++++++ pkg/adapter/containers.go | 9 ++++++++- pkg/adapter/containers_remote.go | 2 +- pkg/varlinkapi/containers.go | 12 ++++++++++-- 6 files changed, 37 insertions(+), 21 deletions(-) create mode 100644 libpod/define/exec_codes.go (limited to 'pkg/adapter/containers_remote.go') diff --git a/cmd/podman/exec.go b/cmd/podman/exec.go index 2e9b9e47e..649a7b0db 100644 --- a/cmd/podman/exec.go +++ b/cmd/podman/exec.go @@ -2,7 +2,6 @@ package main import ( "github.com/containers/libpod/cmd/podman/cliconfig" - "github.com/containers/libpod/libpod/define" "github.com/containers/libpod/pkg/adapter" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -70,11 +69,5 @@ func execCmd(c *cliconfig.ExecValues) error { defer runtime.DeferredShutdown(false) exitCode, err = runtime.ExecContainer(getContext(), c) - if errors.Cause(err) == define.ErrOCIRuntimePermissionDenied { - exitCode = 126 - } - if errors.Cause(err) == define.ErrOCIRuntimeNotFound { - exitCode = 127 - } return err } diff --git a/libpod/container_api.go b/libpod/container_api.go index 0cce6ca22..cd020e429 100644 --- a/libpod/container_api.go +++ b/libpod/container_api.go @@ -18,11 +18,6 @@ import ( "k8s.io/client-go/tools/remotecommand" ) -const ( - defaultExecExitCode = 125 - defaultExecExitCodeCannotInvoke = 126 -) - // Init creates a container in the OCI runtime func (c *Container) Init(ctx context.Context) (err error) { span, _ := opentracing.StartSpanFromContext(ctx, "containerInit") @@ -234,7 +229,7 @@ func (c *Container) Exec(tty, privileged bool, env, cmd []string, user, workDir defer c.lock.Unlock() if err := c.syncContainer(); err != nil { - return defaultExecExitCodeCannotInvoke, err + return define.ExecErrorCodeCannotInvoke, err } } @@ -242,7 +237,7 @@ func (c *Container) Exec(tty, privileged bool, env, cmd []string, user, workDir // TODO can probably relax this once we track exec sessions if conState != define.ContainerStateRunning { - return defaultExecExitCodeCannotInvoke, errors.Wrapf(define.ErrCtrStateInvalid, "cannot exec into container that is not running") + return define.ExecErrorCodeCannotInvoke, errors.Wrapf(define.ErrCtrStateInvalid, "cannot exec into container that is not running") } if privileged || c.config.Privileged { @@ -269,7 +264,7 @@ func (c *Container) Exec(tty, privileged bool, env, cmd []string, user, workDir logrus.Debugf("Creating new exec session in container %s with session id %s", c.ID(), sessionID) if err := c.createExecBundle(sessionID); err != nil { - return defaultExecExitCodeCannotInvoke, err + return define.ExecErrorCodeCannotInvoke, err } defer func() { @@ -281,7 +276,7 @@ func (c *Container) Exec(tty, privileged bool, env, cmd []string, user, workDir pid, attachChan, err := c.ociRuntime.execContainer(c, cmd, capList, env, tty, workDir, user, sessionID, streams, preserveFDs, resize, detachKeys) if err != nil { - ec := defaultExecExitCode + ec := define.ExecErrorCodeGeneric // Conmon will pass a non-zero exit code from the runtime as a pid here. // we differentiate a pid with an exit code by sending it as negative, so reverse // that change and return the exit code the runtime failed with. @@ -303,7 +298,7 @@ func (c *Container) Exec(tty, privileged bool, env, cmd []string, user, workDir if err := c.save(); err != nil { // Now we have a PID but we can't save it in the DB // TODO handle this better - return defaultExecExitCode, errors.Wrapf(err, "error saving exec sessions %s for container %s", sessionID, c.ID()) + return define.ExecErrorCodeGeneric, errors.Wrapf(err, "error saving exec sessions %s for container %s", sessionID, c.ID()) } c.newContainerEvent(events.Exec) logrus.Debugf("Successfully started exec session %s in container %s", sessionID, c.ID()) diff --git a/libpod/define/exec_codes.go b/libpod/define/exec_codes.go new file mode 100644 index 000000000..90a68cf93 --- /dev/null +++ b/libpod/define/exec_codes.go @@ -0,0 +1,13 @@ +package define + +const ( + // ExecErrorCodeGeneric is the default error code to return from an exec session if libpod failed + // prior to calling the runtime + ExecErrorCodeGeneric = 125 + // ExecErrorCodeCannotInvoke is the error code to return when the runtime fails to invoke a command + // an example of this can be found by trying to execute a directory: + // `podman exec -l /etc` + ExecErrorCodeCannotInvoke = 126 + // ExecErrorCodeNotFound is the error code to return when a command cannot be found + ExecErrorCodeNotFound = 127 +) diff --git a/pkg/adapter/containers.go b/pkg/adapter/containers.go index 47f1b091e..a8bff187c 100644 --- a/pkg/adapter/containers.go +++ b/pkg/adapter/containers.go @@ -1000,7 +1000,14 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal streams.AttachOutput = true streams.AttachError = true - return ExecAttachCtr(ctx, ctr.Container, cli.Tty, cli.Privileged, envs, cmd, cli.User, cli.Workdir, streams, cli.PreserveFDs, cli.DetachKeys) + ec, err = ExecAttachCtr(ctx, ctr.Container, cli.Tty, cli.Privileged, envs, cmd, cli.User, cli.Workdir, streams, cli.PreserveFDs, cli.DetachKeys) + if errors.Cause(err) == define.ErrOCIRuntimePermissionDenied { + ec = 126 + } + if errors.Cause(err) == define.ErrOCIRuntimeNotFound { + ec = 127 + } + return ec, err } // Prune removes stopped containers diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go index 7a9cf94b7..7fce30378 100644 --- a/pkg/adapter/containers_remote.go +++ b/pkg/adapter/containers_remote.go @@ -997,7 +997,7 @@ func (r *LocalRuntime) Commit(ctx context.Context, c *cliconfig.CommitValues, co func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecValues) (int, error) { var ( oldTermState *term.State - ec int = 125 + ec int = define.ExecErrorCodeGeneric ) // default invalid command exit code // Validate given environment variables diff --git a/pkg/varlinkapi/containers.go b/pkg/varlinkapi/containers.go index a70a87675..fa17bed84 100644 --- a/pkg/varlinkapi/containers.go +++ b/pkg/varlinkapi/containers.go @@ -812,7 +812,7 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO go func() { if err := virtwriter.Reader(reader, nil, nil, pipeWriter, resizeChan, nil); err != nil { ecErrChan <- ExitCodeError{ - 125, //TODO FIXME magic number, define package? + define.ExecErrorCodeGeneric, err, } } @@ -833,7 +833,15 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO ecErr := <-ecErrChan // TODO FIXME prevent all of these conversions - if err = virtwriter.HangUp(writer, int(ecErr.ExitCode)); err != nil { + exitCode := int(ecErr.ExitCode) + if errors.Cause(ecErr.Error) == define.ErrOCIRuntimePermissionDenied { + exitCode = define.ExecErrorCodeCannotInvoke + } + if errors.Cause(ecErr.Error) == define.ErrOCIRuntimeNotFound { + exitCode = define.ExecErrorCodeNotFound + } + + if err = virtwriter.HangUp(writer, exitCode); err != nil { logrus.Errorf("ExecContainer failed to HANG-UP on %s: %s", ctr.ID(), err.Error()) } -- cgit v1.2.3-54-g00ecf From 5bf99a82ff48cd5097182c55fe1347daf793324b Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Mon, 22 Jul 2019 15:56:58 -0400 Subject: add detach keys support for remote Signed-off-by: Peter Hunt --- API.md | 2 ++ cmd/podman/varlink/io.podman.varlink | 4 +++- pkg/adapter/containers_remote.go | 2 +- pkg/varlinkapi/containers.go | 15 ++++++++++----- test/e2e/exec_test.go | 4 +--- 5 files changed, 17 insertions(+), 10 deletions(-) (limited to 'pkg/adapter/containers_remote.go') diff --git a/API.md b/API.md index 31bffeb16..71d09fed8 100755 --- a/API.md +++ b/API.md @@ -1591,6 +1591,8 @@ user [?string](#?string) workdir [?string](#?string) env [?[]string](#?[]string) + +detachKeys [?string](#?string) ### type Image diff --git a/cmd/podman/varlink/io.podman.varlink b/cmd/podman/varlink/io.podman.varlink index cc66e7df0..f5f3250f7 100644 --- a/cmd/podman/varlink/io.podman.varlink +++ b/cmd/podman/varlink/io.podman.varlink @@ -514,7 +514,9 @@ type ExecOpts( # workdir to run command in container workdir: ?string, # slice of keyword=value environment variables - env: ?[]string + env: ?[]string, + # string of detach keys + detachKeys: ?string ) # GetVersion returns version and build information of the podman service diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go index 7fce30378..3b1b80315 100644 --- a/pkg/adapter/containers_remote.go +++ b/pkg/adapter/containers_remote.go @@ -1017,7 +1017,6 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal // Check if we are attached to a terminal. If we are, generate resize // events, and set the terminal to raw mode - // TODO FIXME tty if haveTerminal && cli.Tty { cancel, oldTermState, err := handleTerminalAttach(ctx, resize) if err != nil { @@ -1038,6 +1037,7 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal User: &cli.User, Workdir: &cli.Workdir, Env: &envs, + DetachKeys: &cli.DetachKeys, } inputStream := os.Stdin diff --git a/pkg/varlinkapi/containers.go b/pkg/varlinkapi/containers.go index fa17bed84..97eb36014 100644 --- a/pkg/varlinkapi/containers.go +++ b/pkg/varlinkapi/containers.go @@ -782,6 +782,9 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO fmt.Sprintf("exec requires a running container, %s is %s", ctr.ID(), state.String())) } + // ACK the client upgrade request + call.ReplyExecContainer() + envs := []string{} if opts.Env != nil { envs = *opts.Env @@ -796,8 +799,11 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO if opts.Workdir != nil { workDir = *opts.Workdir } - // ACK the client upgrade request - call.ReplyExecContainer() + + var detachKeys string + if opts.DetachKeys != nil { + detachKeys = *opts.DetachKeys + } resizeChan := make(chan remotecommand.TerminalSize) @@ -818,11 +824,10 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO } }() - // TODO FIXME detach keys go func() { - ec, err := ctr.Exec(opts.Tty, opts.Privileged, envs, opts.Cmd, user, workDir, streams, 0, resizeChan, "") + ec, err := ctr.Exec(opts.Tty, opts.Privileged, envs, opts.Cmd, user, workDir, streams, 0, resizeChan, detachKeys) if err != nil { - logrus.Errorf("ExecContainer Exec err %s, %s\n", err.Error(), errors.Cause(err).Error()) + logrus.Errorf(err.Error()) } ecErrChan <- ExitCodeError{ uint32(ec), diff --git a/test/e2e/exec_test.go b/test/e2e/exec_test.go index bd4fb7aad..6cf78a25c 100644 --- a/test/e2e/exec_test.go +++ b/test/e2e/exec_test.go @@ -77,8 +77,6 @@ var _ = Describe("Podman exec", func() { }) It("podman exec environment test", func() { - // passing environment variables is not supported in the remote client - SkipIfRemote() setup := podmanTest.RunTopContainer("test1") setup.WaitWithDefaultTimeout() Expect(setup.ExitCode()).To(Equal(0)) @@ -110,8 +108,8 @@ var _ = Describe("Podman exec", func() { match, _ := session.GrepString("BAR") Expect(match).Should(BeTrue()) os.Unsetenv("FOO") - }) + It("podman exec exit code", func() { setup := podmanTest.RunTopContainer("test1") setup.WaitWithDefaultTimeout() -- cgit v1.2.3-54-g00ecf From 74ab273e919715823fe9bb70c030a65b021897d1 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 23 Jul 2019 10:01:56 -0400 Subject: buffer errChan Signed-off-by: Peter Hunt --- pkg/adapter/containers_remote.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'pkg/adapter/containers_remote.go') diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go index 3b1b80315..0373913d0 100644 --- a/pkg/adapter/containers_remote.go +++ b/pkg/adapter/containers_remote.go @@ -1068,7 +1068,7 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal } func configureVarlinkAttachStdio(reader *bufio.Reader, writer *bufio.Writer, stdin *os.File, stdout *os.File, oldTermState *term.State, resize chan remotecommand.TerminalSize, ecChan chan int) chan error { - var errChan chan error + errChan := make(chan error, 1) // These are the special writers that encode input from the client. varlinkStdinWriter := virtwriter.NewVirtWriteCloser(writer, virtwriter.ToStdin) varlinkResizeWriter := virtwriter.NewVirtWriteCloser(writer, virtwriter.TerminalResize) -- cgit v1.2.3-54-g00ecf From 9e6928570459ba5451f2128a9391bbb48acefc1e Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 23 Jul 2019 11:16:34 -0400 Subject: only use stdin if specified Signed-off-by: Peter Hunt --- pkg/adapter/containers_remote.go | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) (limited to 'pkg/adapter/containers_remote.go') diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go index 0373913d0..751c184b3 100644 --- a/pkg/adapter/containers_remote.go +++ b/pkg/adapter/containers_remote.go @@ -1057,12 +1057,8 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal ecChan := make(chan int, 1) errChan := configureVarlinkAttachStdio(r.Conn.Reader, r.Conn.Writer, inputStream, os.Stdout, oldTermState, resize, ecChan) - select { - case ec = <-ecChan: - break - case err = <-errChan: - break - } + ec = <-ecChan + err = <-errChan return ec, err } @@ -1095,13 +1091,15 @@ func configureVarlinkAttachStdio(reader *bufio.Reader, writer *bufio.Writer, std } }() - // Takes stdinput and sends it over the wire after being encoded - go func() { - if _, err := io.Copy(varlinkStdinWriter, stdin); err != nil { - defer restoreTerminal(oldTermState) - errChan <- err - } + if stdin != nil { + // Takes stdinput and sends it over the wire after being encoded + go func() { + if _, err := io.Copy(varlinkStdinWriter, stdin); err != nil { + defer restoreTerminal(oldTermState) + errChan <- err + } - }() + }() + } return errChan } -- cgit v1.2.3-54-g00ecf From d59f083637f267b3653bc7cda46fd729dcf35c17 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 23 Jul 2019 11:47:36 -0400 Subject: always send generic error in case io fails Signed-off-by: Peter Hunt --- pkg/adapter/containers_remote.go | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'pkg/adapter/containers_remote.go') diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go index 751c184b3..b7390c113 100644 --- a/pkg/adapter/containers_remote.go +++ b/pkg/adapter/containers_remote.go @@ -1073,6 +1073,7 @@ func configureVarlinkAttachStdio(reader *bufio.Reader, writer *bufio.Writer, std // Read from the wire and direct to stdout or stderr err := virtwriter.Reader(reader, stdout, os.Stderr, nil, nil, ecChan) defer restoreTerminal(oldTermState) + sendGenericError(ecChan) errChan <- err }() @@ -1081,11 +1082,13 @@ func configureVarlinkAttachStdio(reader *bufio.Reader, writer *bufio.Writer, std b, err := json.Marshal(termResize) if err != nil { defer restoreTerminal(oldTermState) + sendGenericError(ecChan) errChan <- err } _, err = varlinkResizeWriter.Write(b) if err != nil { defer restoreTerminal(oldTermState) + sendGenericError(ecChan) errChan <- err } } @@ -1096,6 +1099,7 @@ func configureVarlinkAttachStdio(reader *bufio.Reader, writer *bufio.Writer, std go func() { if _, err := io.Copy(varlinkStdinWriter, stdin); err != nil { defer restoreTerminal(oldTermState) + sendGenericError(ecChan) errChan <- err } @@ -1103,3 +1107,9 @@ func configureVarlinkAttachStdio(reader *bufio.Reader, writer *bufio.Writer, std } return errChan } + +func sendGenericError(ecChan chan int) { + if ecChan != nil { + ecChan <- define.ExecErrorCodeGeneric + } +} -- cgit v1.2.3-54-g00ecf From 82dce36fb6ef3839c029b7450a75c85cf28cf377 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 23 Jul 2019 13:46:03 -0400 Subject: remove debug prints Signed-off-by: Peter Hunt --- pkg/adapter/containers_remote.go | 1 - pkg/varlinkapi/virtwriter/virtwriter.go | 11 +++++------ 2 files changed, 5 insertions(+), 7 deletions(-) (limited to 'pkg/adapter/containers_remote.go') diff --git a/pkg/adapter/containers_remote.go b/pkg/adapter/containers_remote.go index b7390c113..5a26f537f 100644 --- a/pkg/adapter/containers_remote.go +++ b/pkg/adapter/containers_remote.go @@ -735,7 +735,6 @@ func (r *LocalRuntime) attach(ctx context.Context, stdin, stdout *os.File, cid s term.SetRawTerminal(os.Stdin.Fd()) } - // TODO add detach keys support reply, err := iopodman.Attach().Send(r.Conn, varlink.Upgrade, cid, detachKeys, start) if err != nil { restoreTerminal(oldTermState) diff --git a/pkg/varlinkapi/virtwriter/virtwriter.go b/pkg/varlinkapi/virtwriter/virtwriter.go index bde3107df..27ecd1f52 100644 --- a/pkg/varlinkapi/virtwriter/virtwriter.go +++ b/pkg/varlinkapi/virtwriter/virtwriter.go @@ -113,7 +113,7 @@ func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remote if output != nil { _, err := io.CopyN(output, r, messageSize) if err != nil { - return errors.Wrapf(err, "issue stdout") + return err } } case ToStderr: @@ -121,14 +121,13 @@ func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remote _, err := io.CopyN(errput, r, messageSize) if err != nil { return err - return errors.Wrapf(err, "issue stderr") } } case ToStdin: if input != nil { _, err := io.CopyN(input, r, messageSize) if err != nil { - return errors.Wrapf(err, "issue stdin") + return err } } case TerminalResize: @@ -138,13 +137,13 @@ func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remote _, err = io.ReadFull(r, out) if err != nil { - return errors.Wrapf(err, "issue resizing") + return err } } // Resize events come over in bytes, need to be reserialized resizeEvent := remotecommand.TerminalSize{} if err := json.Unmarshal(out, &resizeEvent); err != nil { - return errors.Wrapf(err, "issue resizing") + return err } resize <- resizeEvent } @@ -154,7 +153,7 @@ func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remote _, err = io.ReadFull(r, out) if err != nil { - return errors.Wrapf(err, "issue quitting") + return err } } if execEcChan != nil { -- cgit v1.2.3-54-g00ecf