Skip to content

Commit

Permalink
Merge pull request #4337 from thaJeztah/dont_mutate_configfile
Browse files Browse the repository at this point in the history 
cli/command/container: don't mutate ConfigFile.DetachKeys
  • Loading branch information
@thaJeztah
thaJeztah authored Jun 12, 2023
2 parents 8aee745 + 2331e4d commit 7d723e2
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 89 deletions.
15 changes: 8 additions & 7 deletions cli/command/container/attach.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,12 @@ func NewAttachCommand(dockerCli command.Cli) *cobra.Command {

func runAttach(dockerCli command.Cli, opts *attachOptions) error {
ctx := context.Background()
client := dockerCli.Client()
apiClient := dockerCli.Client()

// request channel to wait for client
resultC, errC := client.ContainerWait(ctx, opts.container, "")
resultC, errC := apiClient.ContainerWait(ctx, opts.container, "")

c, err := inspectContainerAndCheckState(ctx, client, opts.container)
c, err := inspectContainerAndCheckState(ctx, apiClient, opts.container)
if err != nil {
return err
}
Expand All @@ -86,16 +86,17 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error {
return err
}

detachKeys := dockerCli.ConfigFile().DetachKeys
if opts.detachKeys != "" {
dockerCli.ConfigFile().DetachKeys = opts.detachKeys
detachKeys = opts.detachKeys
}

options := types.ContainerAttachOptions{
Stream: true,
Stdin: !opts.noStdin && c.Config.OpenStdin,
Stdout: true,
Stderr: true,
DetachKeys: dockerCli.ConfigFile().DetachKeys,
DetachKeys: detachKeys,
}

var in io.ReadCloser
Expand All @@ -109,7 +110,7 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error {
defer signal.StopCatch(sigc)
}

resp, errAttach := client.ContainerAttach(ctx, opts.container, options)
resp, errAttach := apiClient.ContainerAttach(ctx, opts.container, options)
if errAttach != nil {
return errAttach
}
Expand All @@ -123,7 +124,7 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error {
// the container and not exit.
//
// Recheck the container's state to avoid attach block.
_, err = inspectContainerAndCheckState(ctx, client, opts.container)
_, err = inspectContainerAndCheckState(ctx, apiClient, opts.container)
if err != nil {
return err
}
Expand Down
26 changes: 13 additions & 13 deletions cli/command/container/create.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,11 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, options *createOptio
reportError(dockerCli.Err(), "create", err.Error(), true)
return cli.StatusError{StatusCode: 125}
}
response, err := createContainer(context.Background(), dockerCli, containerCfg, options)
id, err := createContainer(context.Background(), dockerCli, containerCfg, options)
if err != nil {
return err
}
fmt.Fprintln(dockerCli.Out(), response.ID)
_, _ = fmt.Fprintln(dockerCli.Out(), id)
return nil
}

Expand Down Expand Up @@ -185,7 +185,7 @@ func newCIDFile(path string) (*cidFile, error) {
}

//nolint:gocyclo
func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *containerConfig, opts *createOptions) (*container.CreateResponse, error) {
func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *containerConfig, opts *createOptions) (containerID string, err error) {
config := containerCfg.Config
hostConfig := containerCfg.HostConfig
networkingConfig := containerCfg.NetworkingConfig
Expand All @@ -200,13 +200,13 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c

containerIDFile, err := newCIDFile(hostConfig.ContainerIDFile)
if err != nil {
return nil, err
return "", err
}
defer containerIDFile.Close()

ref, err := reference.ParseAnyReference(config.Image)
if err != nil {
return nil, err
return "", err
}
if named, ok := ref.(reference.Named); ok {
namedRef = reference.TagNameOnly(named)
Expand All @@ -215,7 +215,7 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c
var err error
trustedRef, err = image.TrustedReference(ctx, dockerCli, taggedRef)
if err != nil {
return nil, err
return "", err
}
config.Image = reference.FamiliarString(trustedRef)
}
Expand All @@ -239,14 +239,14 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c
if opts.platform != "" && versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.41") {
p, err := platforms.Parse(opts.platform)
if err != nil {
return nil, errors.Wrap(err, "error parsing specified platform")
return "", errors.Wrap(err, "error parsing specified platform")
}
platform = &p
}

if opts.pull == PullImageAlways {
if err := pullAndTagImage(); err != nil {
return nil, err
return "", err
}
}

Expand All @@ -262,24 +262,24 @@ func createContainer(ctx context.Context, dockerCli command.Cli, containerCfg *c
}

if err := pullAndTagImage(); err != nil {
return nil, err
return "", err
}

var retryErr error
response, retryErr = dockerCli.Client().ContainerCreate(ctx, config, hostConfig, networkingConfig, platform, opts.name)
if retryErr != nil {
return nil, retryErr
return "", retryErr
}
} else {
return nil, err
return "", err
}
}

for _, w := range response.Warnings {
fmt.Fprintf(dockerCli.Err(), "WARNING: %s\n", w)
_, _ = fmt.Fprintf(dockerCli.Err(), "WARNING: %s\n", w)
}
err = containerIDFile.Write(response.ID)
return &response, err
return response.ID, err
}

func warnOnOomKillDisable(hostConfig container.HostConfig, stderr io.Writer) {
Expand Down
96 changes: 50 additions & 46 deletions cli/command/container/create_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,10 @@ func TestCIDFileCloseWithWrite(t *testing.T) {
}

func TestCreateContainerImagePullPolicy(t *testing.T) {
imageName := "does-not-exist-locally"
containerID := "abcdef"
const (
imageName = "does-not-exist-locally"
containerID = "abcdef"
)
config := &containerConfig{
Config: &container.Config{
Image: imageName,
Expand All @@ -91,69 +93,71 @@ func TestCreateContainerImagePullPolicy(t *testing.T) {
cases := []struct {
PullPolicy string
ExpectedPulls int
ExpectedBody container.CreateResponse
ExpectedID string
ExpectedErrMsg string
ResponseCounter int
}{
{
PullPolicy: PullImageMissing,
ExpectedPulls: 1,
ExpectedBody: container.CreateResponse{ID: containerID},
ExpectedID: containerID,
}, {
PullPolicy: PullImageAlways,
ExpectedPulls: 1,
ExpectedBody: container.CreateResponse{ID: containerID},
ExpectedID: containerID,
ResponseCounter: 1, // This lets us return a container on the first pull
}, {
PullPolicy: PullImageNever,
ExpectedPulls: 0,
ExpectedErrMsg: "error fake not found",
},
}
for _, c := range cases {
c := c
pullCounter := 0
for _, tc := range cases {
tc := tc
t.Run(tc.PullPolicy, func(t *testing.T) {
pullCounter := 0

client := &fakeClient{
createContainerFunc: func(
config *container.Config,
hostConfig *container.HostConfig,
networkingConfig *network.NetworkingConfig,
platform *specs.Platform,
containerName string,
) (container.CreateResponse, error) {
defer func() { c.ResponseCounter++ }()
switch c.ResponseCounter {
case 0:
return container.CreateResponse{}, fakeNotFound{}
default:
return container.CreateResponse{ID: containerID}, nil
}
},
imageCreateFunc: func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) {
defer func() { pullCounter++ }()
return io.NopCloser(strings.NewReader("")), nil
},
infoFunc: func() (types.Info, error) {
return types.Info{IndexServerAddress: "https://indexserver.example.com"}, nil
},
}
cli := test.NewFakeCli(client)
body, err := createContainer(context.Background(), cli, config, &createOptions{
name: "name",
platform: runtime.GOOS,
untrusted: true,
pull: c.PullPolicy,
})
client := &fakeClient{
createContainerFunc: func(
config *container.Config,
hostConfig *container.HostConfig,
networkingConfig *network.NetworkingConfig,
platform *specs.Platform,
containerName string,
) (container.CreateResponse, error) {
defer func() { tc.ResponseCounter++ }()
switch tc.ResponseCounter {
case 0:
return container.CreateResponse{}, fakeNotFound{}
default:
return container.CreateResponse{ID: containerID}, nil
}
},
imageCreateFunc: func(parentReference string, options types.ImageCreateOptions) (io.ReadCloser, error) {
defer func() { pullCounter++ }()
return io.NopCloser(strings.NewReader("")), nil
},
infoFunc: func() (types.Info, error) {
return types.Info{IndexServerAddress: "https://indexserver.example.com"}, nil
},
}
fakeCLI := test.NewFakeCli(client)
id, err := createContainer(context.Background(), fakeCLI, config, &createOptions{
name: "name",
platform: runtime.GOOS,
untrusted: true,
pull: tc.PullPolicy,
})

if c.ExpectedErrMsg != "" {
assert.ErrorContains(t, err, c.ExpectedErrMsg)
} else {
assert.NilError(t, err)
assert.Check(t, is.DeepEqual(c.ExpectedBody, *body))
}
if tc.ExpectedErrMsg != "" {
assert.Check(t, is.ErrorContains(err, tc.ExpectedErrMsg))
} else {
assert.Check(t, err)
assert.Check(t, is.Equal(tc.ExpectedID, id))
}

assert.Check(t, is.Equal(c.ExpectedPulls, pullCounter))
assert.Check(t, is.Equal(tc.ExpectedPulls, pullCounter))
})
}
}

Expand Down
41 changes: 20 additions & 21 deletions cli/command/container/run.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,14 +144,14 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio
ctx, cancelFun := context.WithCancel(context.Background())
defer cancelFun()

createResponse, err := createContainer(ctx, dockerCli, containerCfg, &opts.createOptions)
containerID, err := createContainer(ctx, dockerCli, containerCfg, &opts.createOptions)
if err != nil {
reportError(stderr, "run", err.Error(), true)
return runStartContainerErr(err)
}
if opts.sigProxy {
sigc := notifyAllSignals()
go ForwardAllSignals(ctx, dockerCli, createResponse.ID, sigc)
go ForwardAllSignals(ctx, dockerCli, containerID, sigc)
defer signal.StopCatch(sigc)
}

Expand All @@ -164,26 +164,33 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio
waitDisplayID = make(chan struct{})
go func() {
defer close(waitDisplayID)
fmt.Fprintln(stdout, createResponse.ID)
_, _ = fmt.Fprintln(stdout, containerID)
}()
}
attach := config.AttachStdin || config.AttachStdout || config.AttachStderr
if attach {
detachKeys := dockerCli.ConfigFile().DetachKeys
if opts.detachKeys != "" {
dockerCli.ConfigFile().DetachKeys = opts.detachKeys
detachKeys = opts.detachKeys
}

closeFn, err := attachContainer(ctx, dockerCli, &errCh, config, createResponse.ID)
closeFn, err := attachContainer(ctx, dockerCli, containerID, &errCh, config, types.ContainerAttachOptions{
Stream: true,
Stdin: config.AttachStdin,
Stdout: config.AttachStdout,
Stderr: config.AttachStderr,
DetachKeys: detachKeys,
})
if err != nil {
return err
}
defer closeFn()
}

statusChan := waitExitOrRemoved(ctx, dockerCli, createResponse.ID, copts.autoRemove)
statusChan := waitExitOrRemoved(ctx, dockerCli, containerID, copts.autoRemove)

// start the container
if err := client.ContainerStart(ctx, createResponse.ID, types.ContainerStartOptions{}); err != nil {
if err := client.ContainerStart(ctx, containerID, types.ContainerStartOptions{}); err != nil {
// If we have hijackedIOStreamer, we should notify
// hijackedIOStreamer we are going to exit and wait
// to avoid the terminal are not restored.
Expand All @@ -201,8 +208,8 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio
}

if (config.AttachStdin || config.AttachStdout || config.AttachStderr) && config.Tty && dockerCli.Out().IsTerminal() {
if err := MonitorTtySize(ctx, dockerCli, createResponse.ID, false); err != nil {
fmt.Fprintln(stderr, "Error monitoring TTY size:", err)
if err := MonitorTtySize(ctx, dockerCli, containerID, false); err != nil {
_, _ = fmt.Fprintln(stderr, "Error monitoring TTY size:", err)
}
}

Expand Down Expand Up @@ -232,15 +239,7 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio
return nil
}

func attachContainer(ctx context.Context, dockerCli command.Cli, errCh *chan error, config *container.Config, containerID string) (func(), error) {
options := types.ContainerAttachOptions{
Stream: true,
Stdin: config.AttachStdin,
Stdout: config.AttachStdout,
Stderr: config.AttachStderr,
DetachKeys: dockerCli.ConfigFile().DetachKeys,
}

func attachContainer(ctx context.Context, dockerCli command.Cli, containerID string, errCh *chan error, config *container.Config, options types.ContainerAttachOptions) (func(), error) {
resp, errAttach := dockerCli.Client().ContainerAttach(ctx, containerID, options)
if errAttach != nil {
return nil, errAttach
Expand All @@ -250,13 +249,13 @@ func attachContainer(ctx context.Context, dockerCli command.Cli, errCh *chan err
out, cerr io.Writer
in io.ReadCloser
)
if config.AttachStdin {
if options.Stdin {
in = dockerCli.In()
}
if config.AttachStdout {
if options.Stdout {
out = dockerCli.Out()
}
if config.AttachStderr {
if options.Stderr {
if config.Tty {
cerr = dockerCli.Out()
} else {
Expand Down
5 changes: 3 additions & 2 deletions cli/command/container/start.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,16 +94,17 @@ func RunStart(dockerCli command.Cli, opts *StartOptions) error {
defer signal.StopCatch(sigc)
}

detachKeys := dockerCli.ConfigFile().DetachKeys
if opts.DetachKeys != "" {
dockerCli.ConfigFile().DetachKeys = opts.DetachKeys
detachKeys = opts.DetachKeys
}

options := types.ContainerAttachOptions{
Stream: true,
Stdin: opts.OpenStdin && c.Config.OpenStdin,
Stdout: true,
Stderr: true,
DetachKeys: dockerCli.ConfigFile().DetachKeys,
DetachKeys: detachKeys,
}

var in io.ReadCloser
Expand Down

0 comments on commit 7d723e2

Please sign in to comment.