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 ++++++++++++++++++-------------- pkg/varlinkapi/attach.go | 4 +- pkg/varlinkapi/containers.go | 52 ++++---- pkg/varlinkapi/virtwriter/virtwriter.go | 70 +++++++---- 4 files changed, 202 insertions(+), 140 deletions(-) (limited to 'pkg') 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 } diff --git a/pkg/varlinkapi/attach.go b/pkg/varlinkapi/attach.go index 97ba525a5..1f8d48eb9 100644 --- a/pkg/varlinkapi/attach.go +++ b/pkg/varlinkapi/attach.go @@ -68,7 +68,7 @@ func (i *LibpodAPI) Attach(call iopodman.VarlinkCall, name string, detachKeys st reader, writer, _, pw, streams := setupStreams(call) go func() { - if err := virtwriter.Reader(reader, nil, nil, pw, resize); err != nil { + if err := virtwriter.Reader(reader, nil, nil, pw, resize, nil); err != nil { errChan <- err } }() @@ -83,7 +83,7 @@ func (i *LibpodAPI) Attach(call iopodman.VarlinkCall, name string, detachKeys st logrus.Error(finalErr) } - if err = virtwriter.HangUp(writer); err != nil { + if err = virtwriter.HangUp(writer, 0); err != nil { logrus.Errorf("Failed to HANG-UP attach to %s: %s", ctr.ID(), err.Error()) } return call.Writer.Flush() diff --git a/pkg/varlinkapi/containers.go b/pkg/varlinkapi/containers.go index 19a8bfd2e..4714f3fa0 100644 --- a/pkg/varlinkapi/containers.go +++ b/pkg/varlinkapi/containers.go @@ -796,45 +796,55 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO if opts.Workdir != nil { workDir = *opts.Workdir } + // ACK the client upgrade request + 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) go func() { fmt.Printf("ExecContainer Start Reader\n") - if err := virtwriter.Reader(reader, nil, nil, pipeWriter, resizeChan); err != nil { - fmt.Printf("ExecContainer Reader err %s, %s\n", err.Error(), errors.Cause(err).Error()) - errChan <- err + 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") } }() - // Debugging... - time.Sleep(5 * time.Second) - + 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 go func() { - 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 - _, err := ctr.Exec(opts.Tty, opts.Privileged, envs, opts.Cmd, user, workDir, streams, 0, nil, "") + ec, err := ctr.Exec(opts.Tty, opts.Privileged, envs, opts.Cmd, user, workDir, streams, 0, resizeChan, "") if err != nil { - fmt.Printf("ExecContainer Exec err %s, %s\n", err.Error(), errors.Cause(err).Error()) - errChan <- errors.Wrapf(err, "ExecContainer failed for container %s", ctr.ID()) + logrus.Errorf("ExecContainer Exec err %s, %s\n", err.Error(), errors.Cause(err).Error()) + errChan <-err } - }() + ecChan <-uint32(ec) - execErr := <-errChan + }() - if execErr != nil && errors.Cause(execErr) != io.EOF { - fmt.Printf("ExecContainer err: %s\n", execErr.Error()) - return call.ReplyErrorOccurred(execErr.Error()) + ec := uint32(125) + var execErr error + select { + case execErr = <-errChan: + fmt.Println(execErr.Error()) + case ec = <-ecChan: + fmt.Println("found", ec) } - if err = virtwriter.HangUp(writer); err != nil { - fmt.Printf("ExecContainer hangup err: %s\n", err.Error()) + // TODO FIXME prevent all of vthese conversions + if err = virtwriter.HangUp(writer, int(ec)); err != nil { logrus.Errorf("ExecContainer failed to HANG-UP on %s: %s", ctr.ID(), err.Error()) } - return call.Writer.Flush() + defer fmt.Println("Succeeded in exec'ing") + if err := call.Writer.Flush(); err != nil { + logrus.Errorf("Exec Container err: %s", err.Error()) + } + + return execErr } diff --git a/pkg/varlinkapi/virtwriter/virtwriter.go b/pkg/varlinkapi/virtwriter/virtwriter.go index 0da2a91fc..23f945704 100644 --- a/pkg/varlinkapi/virtwriter/virtwriter.go +++ b/pkg/varlinkapi/virtwriter/virtwriter.go @@ -89,10 +89,14 @@ func (v VirtWriteCloser) Write(input []byte) (int, error) { } // Reader decodes the content that comes over the wire and directs it to the proper destination. -func Reader(r *bufio.Reader, output io.Writer, errput io.Writer, input io.Writer, resize chan remotecommand.TerminalSize) error { +func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remotecommand.TerminalSize, execEcChan chan int) error { var messageSize int64 headerBytes := make([]byte, 8) + if r == nil { + return errors.Errorf("Reader must not be nil") + } + for { n, err := io.ReadFull(r, headerBytes) if err != nil { @@ -106,44 +110,57 @@ func Reader(r *bufio.Reader, output io.Writer, errput io.Writer, input io.Writer switch IntToSocketDest(int(headerBytes[0])) { case ToStdout: - _, err := io.CopyN(output, r, messageSize) - if err != nil { - return err + if output != nil { + _, err := io.CopyN(output, r, messageSize) + if err != nil { + return errors.Wrapf(err, "issue stdout") + } } case ToStderr: - _, err := io.CopyN(errput, r, messageSize) - if err != nil { - return err + if errput != nil { + _, err := io.CopyN(errput, r, messageSize) + if err != nil { + return err + return errors.Wrapf(err, "issue stderr") + } } case ToStdin: - _, err := io.CopyN(input, r, messageSize) - if err != nil { - return err - } - case TerminalResize: - out := make([]byte, messageSize) - if messageSize > 0 { - _, err = io.ReadFull(r, out) - + if input != nil { + _, err := io.CopyN(input, r, messageSize) if err != nil { - return err + return errors.Wrapf(err, "issue stdin") } } - // Resize events come over in bytes, need to be reserialized - resizeEvent := remotecommand.TerminalSize{} - if err := json.Unmarshal(out, &resizeEvent); err != nil { - return err + case TerminalResize: + if resize != nil { + out := make([]byte, messageSize) + if messageSize > 0 { + _, err = io.ReadFull(r, out) + + if err != nil { + return errors.Wrapf(err, "issue resizing") + } + } + // 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") + } + resize <- resizeEvent } - resize <- resizeEvent case Quit: out := make([]byte, messageSize) if messageSize > 0 { _, err = io.ReadFull(r, out) if err != nil { - return err + return errors.Wrapf(err, "issue quitting") } } + if execEcChan != nil { + ecInt := binary.BigEndian.Uint32(out) + execEcChan <-int(ecInt) + } return nil default: @@ -154,9 +171,12 @@ func Reader(r *bufio.Reader, output io.Writer, errput io.Writer, input io.Writer } // HangUp sends message to peer to close connection -func HangUp(writer *bufio.Writer) (err error) { +func HangUp(writer *bufio.Writer, ec int) (err error) { n := 0 - msg := []byte("HANG-UP") + msg := make([]byte, 4) + + binary.LittleEndian.PutUint32(msg, uint32(ec)) + writeQuit := NewVirtWriteCloser(writer, Quit) if n, err = writeQuit.Write(msg); err != nil { -- 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') 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') 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') 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 638b73a046e21a2a033e9831cf622e96aa1c8fc9 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Mon, 22 Jul 2019 16:10:02 -0400 Subject: remove unnecessary conversions Signed-off-by: Peter Hunt --- pkg/varlinkapi/containers.go | 5 ++--- pkg/varlinkapi/virtwriter/virtwriter.go | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) (limited to 'pkg') diff --git a/pkg/varlinkapi/containers.go b/pkg/varlinkapi/containers.go index 97eb36014..6e2b26c0e 100644 --- a/pkg/varlinkapi/containers.go +++ b/pkg/varlinkapi/containers.go @@ -808,7 +808,7 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO resizeChan := make(chan remotecommand.TerminalSize) reader, writer, _, pipeWriter, streams := setupStreams(call) - //reader, _, _, pipeWriter, streams := setupStreams(call) + type ExitCodeError struct { ExitCode uint32 Error error @@ -837,8 +837,7 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO ecErr := <-ecErrChan - // TODO FIXME prevent all of these conversions - exitCode := int(ecErr.ExitCode) + exitCode := ecErr.ExitCode if errors.Cause(ecErr.Error) == define.ErrOCIRuntimePermissionDenied { exitCode = define.ExecErrorCodeCannotInvoke } diff --git a/pkg/varlinkapi/virtwriter/virtwriter.go b/pkg/varlinkapi/virtwriter/virtwriter.go index 8cdedeed8..bde3107df 100644 --- a/pkg/varlinkapi/virtwriter/virtwriter.go +++ b/pkg/varlinkapi/virtwriter/virtwriter.go @@ -171,11 +171,11 @@ func Reader(r *bufio.Reader, output, errput, input io.Writer, resize chan remote } // HangUp sends message to peer to close connection -func HangUp(writer *bufio.Writer, ec int) (err error) { +func HangUp(writer *bufio.Writer, ec uint32) (err error) { n := 0 msg := make([]byte, 4) - binary.BigEndian.PutUint32(msg, uint32(ec)) + binary.BigEndian.PutUint32(msg, ec) writeQuit := NewVirtWriteCloser(writer, Quit) if n, err = writeQuit.Write(msg); err != nil { -- cgit v1.2.3-54-g00ecf From a4041dafae7cabdb1a9d57e1606fe9c03ac267a8 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Mon, 22 Jul 2019 16:59:49 -0400 Subject: move handleTerminalAttach to generic build Signed-off-by: Peter Hunt --- pkg/adapter/terminal.go | 23 +++++++++++++++++++++++ pkg/adapter/terminal_linux.go | 23 ----------------------- 2 files changed, 23 insertions(+), 23 deletions(-) (limited to 'pkg') diff --git a/pkg/adapter/terminal.go b/pkg/adapter/terminal.go index 373c78322..51b747d23 100644 --- a/pkg/adapter/terminal.go +++ b/pkg/adapter/terminal.go @@ -7,6 +7,7 @@ import ( "github.com/docker/docker/pkg/signal" "github.com/docker/docker/pkg/term" + "github.com/pkg/errors" "github.com/sirupsen/logrus" "k8s.io/client-go/tools/remotecommand" ) @@ -76,3 +77,25 @@ func (f *RawTtyFormatter) Format(entry *logrus.Entry) ([]byte, error) { return bytes, err } + +func handleTerminalAttach(ctx context.Context, resize chan remotecommand.TerminalSize) (context.CancelFunc, *term.State, error) { + logrus.Debugf("Handling terminal attach") + + subCtx, cancel := context.WithCancel(ctx) + + resizeTty(subCtx, resize) + + oldTermState, err := term.SaveState(os.Stdin.Fd()) + if err != nil { + // allow caller to not have to do any cleaning up if we error here + cancel() + return nil, nil, errors.Wrapf(err, "unable to save terminal state") + } + + logrus.SetFormatter(&RawTtyFormatter{}) + if _, err := term.SetRawTerminal(os.Stdin.Fd()); err != nil { + return cancel, nil, err + } + + return cancel, oldTermState, nil +} diff --git a/pkg/adapter/terminal_linux.go b/pkg/adapter/terminal_linux.go index de2600b75..26cfd7b5e 100644 --- a/pkg/adapter/terminal_linux.go +++ b/pkg/adapter/terminal_linux.go @@ -6,7 +6,6 @@ import ( "os" "github.com/containers/libpod/libpod" - "github.com/docker/docker/pkg/term" "github.com/pkg/errors" "github.com/sirupsen/logrus" "golang.org/x/crypto/ssh/terminal" @@ -108,25 +107,3 @@ func StartAttachCtr(ctx context.Context, ctr *libpod.Container, stdout, stderr, return nil } - -func handleTerminalAttach(ctx context.Context, resize chan remotecommand.TerminalSize) (context.CancelFunc, *term.State, error) { - logrus.Debugf("Handling terminal attach") - - subCtx, cancel := context.WithCancel(ctx) - - resizeTty(subCtx, resize) - - oldTermState, err := term.SaveState(os.Stdin.Fd()) - if err != nil { - // allow caller to not have to do any cleaning up if we error here - cancel() - return nil, nil, errors.Wrapf(err, "unable to save terminal state") - } - - logrus.SetFormatter(&RawTtyFormatter{}) - if _, err := term.SetRawTerminal(os.Stdin.Fd()); err != nil { - return cancel, nil, err - } - - return cancel, oldTermState, nil -} -- 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') 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') 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') 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') 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 From 01a8483a59eed8bc706b5219b903704544b66c10 Mon Sep 17 00:00:00 2001 From: Peter Hunt Date: Tue, 23 Jul 2019 15:30:05 -0400 Subject: refactor to reduce duplicated error parsing Signed-off-by: Peter Hunt --- libpod/define/exec_codes.go | 17 +++++++++++++++++ pkg/adapter/containers.go | 8 +------- pkg/varlinkapi/containers.go | 10 ++-------- 3 files changed, 20 insertions(+), 15 deletions(-) (limited to 'pkg') diff --git a/libpod/define/exec_codes.go b/libpod/define/exec_codes.go index 90a68cf93..7184f1e59 100644 --- a/libpod/define/exec_codes.go +++ b/libpod/define/exec_codes.go @@ -1,5 +1,9 @@ package define +import ( + "github.com/pkg/errors" +) + const ( // ExecErrorCodeGeneric is the default error code to return from an exec session if libpod failed // prior to calling the runtime @@ -11,3 +15,16 @@ const ( // ExecErrorCodeNotFound is the error code to return when a command cannot be found ExecErrorCodeNotFound = 127 ) + +// TranslateExecErrorToExitCode takes an error and checks whether it +// has a predefined exit code associated. If so, it returns that, otherwise it returns +// the exit code originally stated in libpod.Exec() +func TranslateExecErrorToExitCode(originalEC int, err error) int { + if errors.Cause(err) == ErrOCIRuntimePermissionDenied { + return ExecErrorCodeCannotInvoke + } + if errors.Cause(err) == ErrOCIRuntimeNotFound { + return ExecErrorCodeNotFound + } + return originalEC +} diff --git a/pkg/adapter/containers.go b/pkg/adapter/containers.go index a8bff187c..faaef3e60 100644 --- a/pkg/adapter/containers.go +++ b/pkg/adapter/containers.go @@ -1001,13 +1001,7 @@ func (r *LocalRuntime) ExecContainer(ctx context.Context, cli *cliconfig.ExecVal streams.AttachError = true 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 + return define.TranslateExecErrorToExitCode(ec, err), err } // Prune removes stopped containers diff --git a/pkg/varlinkapi/containers.go b/pkg/varlinkapi/containers.go index 6e2b26c0e..cd5f305c9 100644 --- a/pkg/varlinkapi/containers.go +++ b/pkg/varlinkapi/containers.go @@ -837,15 +837,9 @@ func (i *LibpodAPI) ExecContainer(call iopodman.VarlinkCall, opts iopodman.ExecO ecErr := <-ecErrChan - exitCode := ecErr.ExitCode - if errors.Cause(ecErr.Error) == define.ErrOCIRuntimePermissionDenied { - exitCode = define.ExecErrorCodeCannotInvoke - } - if errors.Cause(ecErr.Error) == define.ErrOCIRuntimeNotFound { - exitCode = define.ExecErrorCodeNotFound - } + exitCode := define.TranslateExecErrorToExitCode(int(ecErr.ExitCode), ecErr.Error) - if err = virtwriter.HangUp(writer, exitCode); err != nil { + if err = virtwriter.HangUp(writer, uint32(exitCode)); err != nil { logrus.Errorf("ExecContainer failed to HANG-UP on %s: %s", ctr.ID(), err.Error()) } -- cgit v1.2.3-54-g00ecf