From 0551714d382eef834c67f1d2a969a2b70d2a6ad4 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 1/3] 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 3ae056af..7b0f9a27 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 00000000..04cfcb60 --- /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) + } + }) + } +} From ab06d09a7313a95869ca6ad249dc6caefafa564d Mon Sep 17 00:00:00 2001 From: Karthik Chowdary <21139050+Karthik-Chowdary@users.noreply.github.com> Date: Fri, 4 Sep 2026 06:51:51 +0000 Subject: [PATCH 2/3] test(docker): follow table-driven conventions Signed-off-by: Karthik Chowdary <21139050+Karthik-Chowdary@users.noreply.github.com> --- internal/docker/docker_test.go | 106 ++++++++++++++++++++------------- 1 file changed, 63 insertions(+), 43 deletions(-) diff --git a/internal/docker/docker_test.go b/internal/docker/docker_test.go index 04cfcb60..8a0f7592 100644 --- a/internal/docker/docker_test.go +++ b/internal/docker/docker_test.go @@ -18,87 +18,107 @@ package docker import ( "context" - "errors" - "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/moby/moby/client" + + "github.com/crossplane/crossplane-runtime/v2/pkg/errors" ) func TestStartAndAttach(t *testing.T) { + t.Parallel() + errStart := errors.New("start failed") errAttach := errors.New("attach failed") + type args struct { + stdin bool + startErr error + attachErr error + } + type want struct { + calls []string + err error + options client.ContainerAttachOptions + } + cases := map[string]struct { - stdin bool - startErr error - attachErr error - wantCalls []string - wantErr string - wantOptions client.ContainerAttachOptions + reason string + args args + want want }{ "Success": { - stdin: true, - wantCalls: []string{"start", "attach"}, - wantOptions: client.ContainerAttachOptions{ - Stream: true, - Stdout: true, - Stderr: true, - Stdin: true, - Logs: true, + reason: "A container must be started before attaching, and attach must replay logs while streaming all configured channels.", + args: args{stdin: true}, + want: want{ + calls: []string{"start", "attach"}, + options: 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", + reason: "Attaching cannot succeed when starting the container fails, so the start error must be preserved and attach must not be attempted.", + args: args{startErr: errStart}, + want: want{ + calls: []string{"start"}, + err: errStart, + }, }, "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, + reason: "An attach failure after a successful start must be preserved for callers.", + args: args{attachErr: errAttach}, + want: want{ + calls: []string{"start", "attach"}, + err: errAttach, + options: client.ContainerAttachOptions{ + Stream: true, + Stdout: true, + Stderr: true, + Logs: true, + }, }, }, } for name, tc := range cases { t.Run(name, func(t *testing.T) { + t.Parallel() + calls := []string{} var gotOptions client.ContainerAttachOptions - _, err := startAndAttach(context.Background(), "container-id", tc.stdin, runContainerCalls{ + _, err := startAndAttach(t.Context(), "container-id", tc.args.stdin, runContainerCalls{ start: func(_ context.Context, id string, _ client.ContainerStartOptions) error { - if id != "container-id" { - t.Errorf("start id = %q, want container-id", id) + if diff := cmp.Diff("container-id", id); diff != "" { + t.Errorf("%s\nstart container ID: -want, +got:\n%s", tc.reason, diff) } calls = append(calls, "start") - return tc.startErr + return tc.args.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) + if diff := cmp.Diff("container-id", id); diff != "" { + t.Errorf("%s\nattach container ID: -want, +got:\n%s", tc.reason, diff) } calls = append(calls, "attach") gotOptions = opts - return client.ContainerAttachResult{}, tc.attachErr + return client.ContainerAttachResult{}, tc.args.attachErr }, }) - if strings.Join(calls, ",") != strings.Join(tc.wantCalls, ",") { - t.Errorf("calls = %v, want %v", calls, tc.wantCalls) + if diff := cmp.Diff(tc.want.calls, calls); diff != "" { + t.Errorf("%s\nstartAndAttach(...) calls: -want, +got:\n%s", tc.reason, diff) } - if gotOptions != tc.wantOptions { - t.Errorf("attach options = %+v, want %+v", gotOptions, tc.wantOptions) + if diff := cmp.Diff(tc.want.options, gotOptions); diff != "" { + t.Errorf("%s\nstartAndAttach(...) attach options: -want, +got:\n%s", tc.reason, diff) } - 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) + if diff := cmp.Diff(tc.want.err, err, cmpopts.EquateErrors()); diff != "" { + t.Errorf("%s\nstartAndAttach(...): -want error, +got error:\n%s", tc.reason, diff) } }) } From 68a0fe7baec33ccd3528b5228bccbfe186676986 Mon Sep 17 00:00:00 2001 From: Karthik Chowdary <21139050+Karthik-Chowdary@users.noreply.github.com> Date: Sat, 12 Sep 2026 06:50:32 +0000 Subject: [PATCH 3/3] refactor(docker): use narrow container client interface Signed-off-by: Karthik Chowdary <21139050+Karthik-Chowdary@users.noreply.github.com> --- internal/docker/docker.go | 20 +++++++------------- internal/docker/docker_test.go | 21 +++++++++++++++++---- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/internal/docker/docker.go b/internal/docker/docker.go index 7b0f9a27..79034f32 100644 --- a/internal/docker/docker.go +++ b/internal/docker/docker.go @@ -459,13 +459,7 @@ func RunContainer(ctx context.Context, img string, opts ...RunContainerOption) ( // 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 := 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, - }) + attach, err := startAndAttach(ctx, resp.ID, cfg.stdin != nil, cli) if err != nil { return nil, nil, err } @@ -506,20 +500,20 @@ 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) +type containerStarter interface { + ContainerStart(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) + ContainerAttach(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 { +func startAndAttach(ctx context.Context, id string, stdin bool, cli containerStarter) (client.ContainerAttachResult, error) { + if _, err := cli.ContainerStart(ctx, id, client.ContainerStartOptions{}); err != nil { return client.ContainerAttachResult{}, errors.Wrap(err, "failed to start container") } - rsp, err := calls.attach(ctx, id, client.ContainerAttachOptions{ + rsp, err := cli.ContainerAttach(ctx, id, client.ContainerAttachOptions{ Stream: true, Stdout: true, Stderr: true, diff --git a/internal/docker/docker_test.go b/internal/docker/docker_test.go index 8a0f7592..e194799f 100644 --- a/internal/docker/docker_test.go +++ b/internal/docker/docker_test.go @@ -27,6 +27,19 @@ import ( "github.com/crossplane/crossplane-runtime/v2/pkg/errors" ) +type mockContainerStarter struct { + containerStart func(context.Context, string, client.ContainerStartOptions) (client.ContainerStartResult, error) + containerAttach func(context.Context, string, client.ContainerAttachOptions) (client.ContainerAttachResult, error) +} + +func (m *mockContainerStarter) ContainerStart(ctx context.Context, id string, opts client.ContainerStartOptions) (client.ContainerStartResult, error) { + return m.containerStart(ctx, id, opts) +} + +func (m *mockContainerStarter) ContainerAttach(ctx context.Context, id string, opts client.ContainerAttachOptions) (client.ContainerAttachResult, error) { + return m.containerAttach(ctx, id, opts) +} + func TestStartAndAttach(t *testing.T) { t.Parallel() @@ -93,15 +106,15 @@ func TestStartAndAttach(t *testing.T) { calls := []string{} var gotOptions client.ContainerAttachOptions - _, err := startAndAttach(t.Context(), "container-id", tc.args.stdin, runContainerCalls{ - start: func(_ context.Context, id string, _ client.ContainerStartOptions) error { + _, err := startAndAttach(t.Context(), "container-id", tc.args.stdin, &mockContainerStarter{ + containerStart: func(_ context.Context, id string, _ client.ContainerStartOptions) (client.ContainerStartResult, error) { if diff := cmp.Diff("container-id", id); diff != "" { t.Errorf("%s\nstart container ID: -want, +got:\n%s", tc.reason, diff) } calls = append(calls, "start") - return tc.args.startErr + return client.ContainerStartResult{}, tc.args.startErr }, - attach: func(_ context.Context, id string, opts client.ContainerAttachOptions) (client.ContainerAttachResult, error) { + containerAttach: func(_ context.Context, id string, opts client.ContainerAttachOptions) (client.ContainerAttachResult, error) { if diff := cmp.Diff("container-id", id); diff != "" { t.Errorf("%s\nattach container ID: -want, +got:\n%s", tc.reason, diff) }