Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dial-stdio: handle connections which lack CloseRead method. #1718

Merged
merged 3 commits into from
Mar 13, 2019
Merged

dial-stdio: handle connections which lack CloseRead method. #1718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8919bbf
dial-stdio: handle connections which lack CloseRead method.
Mar 6, 2019
186e745
dial-stdio: Close the connection
Mar 12, 2019
0449ad8
Revert "Disable `docker system dial-stdio` on Windows"
Mar 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions cli-plugins/plugin/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"encoding/json"
"fmt"
"os"
"runtime"
"sync"

"github.com/docker/cli/cli"
Expand Down Expand Up @@ -75,11 +74,7 @@ func PersistentPreRunE(cmd *cobra.Command, args []string) error {
}
// flags must be the original top-level command flags, not cmd.Flags()
options.opts.Common.SetDefaultOptions(options.flags)
var initopts []command.InitializeOpt
if runtime.GOOS != "windows" {
initopts = append(initopts, withPluginClientConn(options.name))
}
err = options.dockerCli.Initialize(options.opts, initopts...)
err = options.dockerCli.Initialize(options.opts, withPluginClientConn(options.name))
})
return err
}
Expand Down
8 changes: 1 addition & 7 deletions cli/command/system/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package system

import (
"runtime"

"github.com/docker/cli/cli"
"github.com/docker/cli/cli/command"
"github.com/spf13/cobra"
Expand All @@ -21,12 +19,8 @@ func NewSystemCommand(dockerCli command.Cli) *cobra.Command {
NewInfoCommand(dockerCli),
newDiskUsageCommand(dockerCli),
newPruneCommand(dockerCli),
newDialStdioCommand(dockerCli),
)
if runtime.GOOS != "windows" {
cmd.AddCommand(
newDialStdioCommand(dockerCli),
)
}

return cmd
}
25 changes: 23 additions & 2 deletions cli/command/system/dial_stdio.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,18 @@ func runDialStdio(dockerCli command.Cli) error {
if err != nil {
return errors.Wrap(err, "failed to open the raw stream connection")
}
connHalfCloser, ok := conn.(halfCloser)
if !ok {
defer conn.Close()

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defer conn.Close here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems plausible and WFM in my simple cases, so I've pushed. I think this was just a preexisting leak, but it also help allay concerns about the nopCloseReader here too.

Would be good to get this tested with your ssh:// use case though.

var connHalfCloser halfCloser
switch t := conn.(type) {
case halfCloser:
connHalfCloser = t
case halfReadWriteCloser:
connHalfCloser = &nopCloseReader{t}
default:
return errors.New("the raw stream connection does not implement halfCloser")
}

stdin2conn := make(chan error)
conn2stdout := make(chan error)
go func() {
Expand Down Expand Up @@ -90,6 +98,19 @@ type halfCloser interface {
halfWriteCloser
}

type halfReadWriteCloser interface {
io.Reader
halfWriteCloser
}

type nopCloseReader struct {
halfReadWriteCloser
}

func (x *nopCloseReader) CloseRead() error {
return nil
}

type halfReadCloserWrapper struct {
io.ReadCloser
}
Expand Down