From 751780323da78bc7e41e48eb916e937811dc7667 Mon Sep 17 00:00:00 2001 From: Karthik Chowdary <21139050+Karthik-Chowdary@users.noreply.github.com> Date: Thu, 3 Sep 2026 09:41:10 +0000 Subject: [PATCH] fix(docker): start containers before attaching Podman rejects stream attachment while a container is still in the created state. Start first, then attach with logs enabled so output produced between the two operations is replayed. Add unit coverage for call ordering, options, and start and attach errors. Fixes: #299 Signed-off-by: Karthik Chowdary <21139050+Karthik-Chowdary@users.noreply.github.com> --- internal/docker/docker.go | 46 +++++++++++---- internal/docker/docker_test.go | 105 +++++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+), 12 deletions(-) create mode 100644 internal/docker/docker_test.go diff --git a/internal/docker/docker.go b/internal/docker/docker.go index 3ae056a..7b0f9a2 100644 --- a/internal/docker/docker.go +++ b/internal/docker/docker.go @@ -454,24 +454,23 @@ func RunContainer(ctx context.Context, img string, opts ...RunContainerOption) ( _, _ = cli.ContainerRemove(context.Background(), resp.ID, client.ContainerRemoveOptions{Force: true}) }() - // Attach before starting so we don't miss any output. Docker - // multiplexes stdout/stderr with 8-byte frame headers when the + // Start before attaching. Podman's Docker-compatible API rejects attach for + // a created container, while Docker supports both orderings. Request logs + // when attaching so output written between start and attach is not lost. + // Docker multiplexes stdout/stderr with 8-byte frame headers when the // container is not using a TTY. - attach, err := cli.ContainerAttach(ctx, resp.ID, client.ContainerAttachOptions{ - Stream: true, - Stdout: true, - Stderr: true, - Stdin: cfg.stdin != nil, + attach, err := startAndAttach(ctx, resp.ID, cfg.stdin != nil, runContainerCalls{ + start: func(ctx context.Context, id string, opts client.ContainerStartOptions) error { + _, err := cli.ContainerStart(ctx, id, opts) + return err + }, + attach: cli.ContainerAttach, }) if err != nil { - return nil, nil, errors.Wrap(err, "failed to attach to container") + return nil, nil, err } defer attach.Close() - if _, err := cli.ContainerStart(ctx, resp.ID, client.ContainerStartOptions{}); err != nil { - return nil, nil, errors.Wrap(err, "failed to start container") - } - // Write stdin data if provided, then close the write side so the // container sees EOF. if cfg.stdin != nil { @@ -507,6 +506,29 @@ func RunContainer(ctx context.Context, img string, opts ...RunContainerOption) ( return stdout.Bytes(), stderr.Bytes(), nil } +type runContainerCalls struct { + start func(context.Context, string, client.ContainerStartOptions) error + attach func(context.Context, string, client.ContainerAttachOptions) (client.ContainerAttachResult, error) +} + +// startAndAttach starts a container before attaching to its streams. Podman's +// Docker-compatible API does not support attaching to a created container. The +// Logs option ensures output produced between these two calls is replayed. +func startAndAttach(ctx context.Context, id string, stdin bool, calls runContainerCalls) (client.ContainerAttachResult, error) { + if err := calls.start(ctx, id, client.ContainerStartOptions{}); err != nil { + return client.ContainerAttachResult{}, errors.Wrap(err, "failed to start container") + } + + rsp, err := calls.attach(ctx, id, client.ContainerAttachOptions{ + Stream: true, + Stdout: true, + Stderr: true, + Stdin: stdin, + Logs: true, + }) + return rsp, errors.Wrap(err, "failed to attach to container") +} + // CopyFromContainer copies files from a container to an afero filesystem. func CopyFromContainer(ctx context.Context, cid, basePath string, fs afero.Fs) error { cli, err := NewClient() diff --git a/internal/docker/docker_test.go b/internal/docker/docker_test.go new file mode 100644 index 0000000..04cfcb6 --- /dev/null +++ b/internal/docker/docker_test.go @@ -0,0 +1,105 @@ +/* +Copyright 2026 The Crossplane Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package docker + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/moby/moby/client" +) + +func TestStartAndAttach(t *testing.T) { + errStart := errors.New("start failed") + errAttach := errors.New("attach failed") + + cases := map[string]struct { + stdin bool + startErr error + attachErr error + wantCalls []string + wantErr string + wantOptions client.ContainerAttachOptions + }{ + "Success": { + stdin: true, + wantCalls: []string{"start", "attach"}, + wantOptions: client.ContainerAttachOptions{ + Stream: true, + Stdout: true, + Stderr: true, + Stdin: true, + Logs: true, + }, + }, + "StartFailureDoesNotAttach": { + startErr: errStart, + wantCalls: []string{"start"}, + wantErr: "failed to start container: start failed", + }, + "AttachFailure": { + attachErr: errAttach, + wantCalls: []string{"start", "attach"}, + wantErr: "failed to attach to container: attach failed", + wantOptions: client.ContainerAttachOptions{ + Stream: true, + Stdout: true, + Stderr: true, + Logs: true, + }, + }, + } + + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + calls := []string{} + var gotOptions client.ContainerAttachOptions + _, err := startAndAttach(context.Background(), "container-id", tc.stdin, runContainerCalls{ + start: func(_ context.Context, id string, _ client.ContainerStartOptions) error { + if id != "container-id" { + t.Errorf("start id = %q, want container-id", id) + } + calls = append(calls, "start") + return tc.startErr + }, + attach: func(_ context.Context, id string, opts client.ContainerAttachOptions) (client.ContainerAttachResult, error) { + if id != "container-id" { + t.Errorf("attach id = %q, want container-id", id) + } + calls = append(calls, "attach") + gotOptions = opts + return client.ContainerAttachResult{}, tc.attachErr + }, + }) + + if strings.Join(calls, ",") != strings.Join(tc.wantCalls, ",") { + t.Errorf("calls = %v, want %v", calls, tc.wantCalls) + } + if gotOptions != tc.wantOptions { + t.Errorf("attach options = %+v, want %+v", gotOptions, tc.wantOptions) + } + switch { + case tc.wantErr == "" && err != nil: + t.Fatalf("unexpected error: %v", err) + case tc.wantErr != "" && (err == nil || err.Error() != tc.wantErr): + t.Fatalf("error = %v, want %q", err, tc.wantErr) + } + }) + } +}