From 5fd2b8831a669b929f683a2e50134734141eea72 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:34:17 -0500 Subject: [PATCH 01/17] feat(agent): implement AgentDelivery interface for container binary delivery Introduces a pluggable delivery strategy that replaces the monolithic inject.sh handshake with phase-aware delivery methods, eliminating race conditions identified in DEVSY-082. Three implementations: - LocalDockerDelivery: named volume pre-placement before container start - RemoteDockerDelivery: docker cp post-start for remote daemons - LegacyShellDelivery: wraps existing inject.sh path for custom drivers Includes factory, integration into setup.go/single.go, and E2E tests. --- e2e/e2e_suite_test.go | 1 + e2e/tests/delivery/delivery.go | 112 +++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 e2e/tests/delivery/delivery.go diff --git a/e2e/e2e_suite_test.go b/e2e/e2e_suite_test.go index 8333ab49b..058e3fa72 100644 --- a/e2e/e2e_suite_test.go +++ b/e2e/e2e_suite_test.go @@ -10,6 +10,7 @@ import ( // Register tests. _ "github.com/devsy-org/devsy/e2e/tests/build" _ "github.com/devsy-org/devsy/e2e/tests/context" + _ "github.com/devsy-org/devsy/e2e/tests/delivery" _ "github.com/devsy-org/devsy/e2e/tests/dockerinstall" _ "github.com/devsy-org/devsy/e2e/tests/down" _ "github.com/devsy-org/devsy/e2e/tests/exec" diff --git a/e2e/tests/delivery/delivery.go b/e2e/tests/delivery/delivery.go new file mode 100644 index 000000000..9b55c2065 --- /dev/null +++ b/e2e/tests/delivery/delivery.go @@ -0,0 +1,112 @@ +package delivery + +import ( + "context" + "os" + "os/exec" + "path/filepath" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/agent/delivery" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = ginkgo.Describe("agent delivery", ginkgo.Label("delivery"), func() { + ginkgo.Context("LocalDockerDelivery", func() { + ginkgo.It("should create volume, populate binary, and mount into RunOptions", + ginkgo.SpecTimeout(framework.TimeoutShort()), + func(ctx context.Context) { + d := &delivery.LocalDockerDelivery{DockerCommand: "docker"} + workspaceID := "e2e-delivery-local" + + runOpts := &driver.RunOptions{ + Mounts: []*config.Mount{}, + Env: map[string]string{}, + } + + binaryPath := findTestBinary() + + err := d.DeliverPreStart(ctx, delivery.PreStartOptions{ + WorkspaceID: workspaceID, + RunOptions: runOpts, + BinaryPath: binaryPath, + Arch: "amd64", + }) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(func() { + _ = d.Cleanup(context.Background(), workspaceID) + }) + + gomega.Expect(runOpts.Mounts).To(gomega.HaveLen(1)) + expectedVolume := "devsy-agent-" + workspaceID + gomega.Expect(runOpts.Mounts[0].Source).To(gomega.Equal(expectedVolume)) + gomega.Expect(runOpts.Mounts[0].Type).To(gomega.Equal("volume")) + + // Verify binary in volume via docker run + out, err := exec.CommandContext(ctx, "docker", "run", "--rm", + "-v", "devsy-agent-"+workspaceID+":/opt/devsy", + "busybox:latest", "test", "-x", "/opt/devsy/devsy", + ).CombinedOutput() + framework.ExpectNoError( + err, "binary should be executable in volume: %s", string(out), + ) + }) + }) + + ginkgo.Context("RemoteDockerDelivery", func() { + ginkgo.It("should copy binary into running container via docker cp", + ginkgo.SpecTimeout(framework.TimeoutShort()), + func(ctx context.Context) { + containerName := "e2e-delivery-remote" + + out, err := exec.CommandContext(ctx, "docker", "run", "-d", + "--name", containerName, + "busybox:latest", "sleep", "120", + ).CombinedOutput() + framework.ExpectNoError(err, "failed to start test container: %s", string(out)) + ginkgo.DeferCleanup(func() { + _ = exec.Command("docker", "rm", "-f", containerName).Run() + }) + + d := &delivery.RemoteDockerDelivery{ + DockerCommand: "docker", + ContainerID: containerName, + } + + binaryPath := findTestBinary() + + err = d.DeliverPostStart(ctx, delivery.PostStartOptions{ + WorkspaceID: "e2e-workspace", + BinaryPath: binaryPath, + Arch: "amd64", + }) + framework.ExpectNoError(err) + + // Verify binary exists and is executable + out, err = exec.CommandContext(ctx, "docker", "exec", containerName, + "test", "-x", "/usr/local/bin/devsy", + ).CombinedOutput() + framework.ExpectNoError(err, "binary should be executable: %s", string(out)) + }) + }) +}) + +func findTestBinary() string { + candidates := []string{"/bin/sh", "/bin/busybox"} + for _, c := range candidates { + if _, err := os.Stat(c); err == nil { + return c + } + } + + binDir, _ := os.Getwd() + devsy := filepath.Join(binDir, "bin", "devsy-linux-amd64") + if _, err := os.Stat(devsy); err == nil { + return devsy + } + + return "/bin/sh" +} From c39204c1839c26d33a4a142b8f0420e1d640a9bc Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:38:59 -0500 Subject: [PATCH 02/17] ci: add delivery label to PR integration test matrix --- .github/workflows/pr-ci.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 0a575c2bc..289486d14 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -228,6 +228,12 @@ jobs: install-kind: false requires-secret: false + - label: delivery + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + # Up tests - label: up-workspaces From 7bdd166879015c2799de9974f61409d99faad11f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:44:22 -0500 Subject: [PATCH 03/17] refactor(e2e): remove redundant delivery E2E tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing up-* tests already exercise the delivery path through setupContainer → injectAgentIntoContainer. No need for separate tests. --- .github/workflows/pr-ci.yml | 6 -- e2e/e2e_suite_test.go | 1 - e2e/tests/delivery/delivery.go | 112 --------------------------------- 3 files changed, 119 deletions(-) delete mode 100644 e2e/tests/delivery/delivery.go diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 289486d14..0a575c2bc 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -228,12 +228,6 @@ jobs: install-kind: false requires-secret: false - - label: delivery - runner: ubuntu-latest - free-disk-space: false - install-kind: false - requires-secret: false - # Up tests - label: up-workspaces diff --git a/e2e/e2e_suite_test.go b/e2e/e2e_suite_test.go index 058e3fa72..8333ab49b 100644 --- a/e2e/e2e_suite_test.go +++ b/e2e/e2e_suite_test.go @@ -10,7 +10,6 @@ import ( // Register tests. _ "github.com/devsy-org/devsy/e2e/tests/build" _ "github.com/devsy-org/devsy/e2e/tests/context" - _ "github.com/devsy-org/devsy/e2e/tests/delivery" _ "github.com/devsy-org/devsy/e2e/tests/dockerinstall" _ "github.com/devsy-org/devsy/e2e/tests/down" _ "github.com/devsy-org/devsy/e2e/tests/exec" diff --git a/e2e/tests/delivery/delivery.go b/e2e/tests/delivery/delivery.go deleted file mode 100644 index 9b55c2065..000000000 --- a/e2e/tests/delivery/delivery.go +++ /dev/null @@ -1,112 +0,0 @@ -package delivery - -import ( - "context" - "os" - "os/exec" - "path/filepath" - - "github.com/devsy-org/devsy/e2e/framework" - "github.com/devsy-org/devsy/pkg/agent/delivery" - "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/driver" - "github.com/onsi/ginkgo/v2" - "github.com/onsi/gomega" -) - -var _ = ginkgo.Describe("agent delivery", ginkgo.Label("delivery"), func() { - ginkgo.Context("LocalDockerDelivery", func() { - ginkgo.It("should create volume, populate binary, and mount into RunOptions", - ginkgo.SpecTimeout(framework.TimeoutShort()), - func(ctx context.Context) { - d := &delivery.LocalDockerDelivery{DockerCommand: "docker"} - workspaceID := "e2e-delivery-local" - - runOpts := &driver.RunOptions{ - Mounts: []*config.Mount{}, - Env: map[string]string{}, - } - - binaryPath := findTestBinary() - - err := d.DeliverPreStart(ctx, delivery.PreStartOptions{ - WorkspaceID: workspaceID, - RunOptions: runOpts, - BinaryPath: binaryPath, - Arch: "amd64", - }) - framework.ExpectNoError(err) - ginkgo.DeferCleanup(func() { - _ = d.Cleanup(context.Background(), workspaceID) - }) - - gomega.Expect(runOpts.Mounts).To(gomega.HaveLen(1)) - expectedVolume := "devsy-agent-" + workspaceID - gomega.Expect(runOpts.Mounts[0].Source).To(gomega.Equal(expectedVolume)) - gomega.Expect(runOpts.Mounts[0].Type).To(gomega.Equal("volume")) - - // Verify binary in volume via docker run - out, err := exec.CommandContext(ctx, "docker", "run", "--rm", - "-v", "devsy-agent-"+workspaceID+":/opt/devsy", - "busybox:latest", "test", "-x", "/opt/devsy/devsy", - ).CombinedOutput() - framework.ExpectNoError( - err, "binary should be executable in volume: %s", string(out), - ) - }) - }) - - ginkgo.Context("RemoteDockerDelivery", func() { - ginkgo.It("should copy binary into running container via docker cp", - ginkgo.SpecTimeout(framework.TimeoutShort()), - func(ctx context.Context) { - containerName := "e2e-delivery-remote" - - out, err := exec.CommandContext(ctx, "docker", "run", "-d", - "--name", containerName, - "busybox:latest", "sleep", "120", - ).CombinedOutput() - framework.ExpectNoError(err, "failed to start test container: %s", string(out)) - ginkgo.DeferCleanup(func() { - _ = exec.Command("docker", "rm", "-f", containerName).Run() - }) - - d := &delivery.RemoteDockerDelivery{ - DockerCommand: "docker", - ContainerID: containerName, - } - - binaryPath := findTestBinary() - - err = d.DeliverPostStart(ctx, delivery.PostStartOptions{ - WorkspaceID: "e2e-workspace", - BinaryPath: binaryPath, - Arch: "amd64", - }) - framework.ExpectNoError(err) - - // Verify binary exists and is executable - out, err = exec.CommandContext(ctx, "docker", "exec", containerName, - "test", "-x", "/usr/local/bin/devsy", - ).CombinedOutput() - framework.ExpectNoError(err, "binary should be executable: %s", string(out)) - }) - }) -}) - -func findTestBinary() string { - candidates := []string{"/bin/sh", "/bin/busybox"} - for _, c := range candidates { - if _, err := os.Stat(c); err == nil { - return c - } - } - - binDir, _ := os.Getwd() - devsy := filepath.Join(binDir, "bin", "devsy-linux-amd64") - if _, err := os.Stat(devsy); err == nil { - return devsy - } - - return "/bin/sh" -} From eaed848840916bdc4757464d6e5cefb98fc29690 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:18:06 -0500 Subject: [PATCH 04/17] fix(delivery): clean up agent delivery volumes on workspace deletion Delete() now calls cleanupDeliveryVolume() after container removal, which invokes the delivery strategy's Cleanup() method to remove orphaned devsy-agent-{workspaceID} volumes. Cleanup is best-effort: errors are logged but never fail the delete operation. deleteForRecreate() inherits this fix via its existing call to Delete(). --- pkg/devcontainer/delete.go | 9 ++ pkg/devcontainer/delete_test.go | 174 ++++++++++++++++++++++++++++++++ 2 files changed, 183 insertions(+) create mode 100644 pkg/devcontainer/delete_test.go diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index cb3d95122..d568a7897 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -37,9 +37,18 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { } } + r.cleanupDeliveryVolume(ctx) + return nil } +func (r *runner) cleanupDeliveryVolume(ctx context.Context) { + strategy := r.newAgentDelivery() + if err := strategy.Cleanup(ctx, r.ID); err != nil { + log.Debugf("best-effort agent delivery volume cleanup: %v", err) + } +} + func (r *runner) Stop(ctx context.Context) error { containerDetails, err := r.Driver.FindDevContainer(ctx, r.ID) if err != nil { diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go new file mode 100644 index 000000000..47736afd1 --- /dev/null +++ b/pkg/devcontainer/delete_test.go @@ -0,0 +1,174 @@ +package devcontainer + +import ( + "context" + "fmt" + "io" + "testing" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + "github.com/devsy-org/devsy/pkg/provider" +) + +type mockDriver struct { + findResult *config.ContainerDetails + findErr error + stopCalled bool + stopErr error + deleteCalled bool + deleteErr error +} + +func (m *mockDriver) FindDevContainer( + _ context.Context, + _ string, +) (*config.ContainerDetails, error) { + return m.findResult, m.findErr +} + +func (m *mockDriver) StopDevContainer(_ context.Context, _ string) error { + m.stopCalled = true + return m.stopErr +} + +func (m *mockDriver) DeleteDevContainer(_ context.Context, _ string) error { + m.deleteCalled = true + return m.deleteErr +} + +//nolint:revive // interface implementation requires 7 args +func (m *mockDriver) CommandDevContainer( + _ context.Context, _, _, _ string, _ io.Reader, _ io.Writer, _ io.Writer, +) error { + return nil +} + +func (m *mockDriver) RunDevContainer(_ context.Context, _ string, _ *driver.RunOptions) error { + return nil +} + +func (m *mockDriver) TargetArchitecture(_ context.Context, _ string) (string, error) { + return "amd64", nil +} + +func (m *mockDriver) StartDevContainer(_ context.Context, _ string) error { + return nil +} + +func (m *mockDriver) GetDevContainerLogs( + _ context.Context, _ string, _ io.Writer, _ io.Writer, +) error { + return nil +} + +func newTestRunner(d driver.Driver) *runner { + return &runner{ + Driver: d, + ID: "test-workspace", + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.CustomDriver, + }, + }, + } +} + +func TestDelete_NilContainer_ReturnsNil(t *testing.T) { + d := &mockDriver{findResult: nil} + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + if d.stopCalled { + t.Error("StopDevContainer should not be called when container is nil") + } + if d.deleteCalled { + t.Error("DeleteDevContainer should not be called when container is nil") + } +} + +func TestDelete_FindError_ReturnsError(t *testing.T) { + d := &mockDriver{findErr: fmt.Errorf("connection refused")} + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + + if err == nil { + t.Fatal("expected error, got nil") + } + if !searchString(err.Error(), "find dev container") { + t.Errorf("expected wrapped find error, got: %v", err) + } +} + +func TestDelete_RunningContainer_StopsDeletesAndCleansUp(t *testing.T) { + d := &mockDriver{ + findResult: &config.ContainerDetails{ + ID: "container-abc", + State: config.ContainerDetailsState{Status: "running"}, + Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, + }, + } + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + if !d.stopCalled { + t.Error("expected StopDevContainer to be called for running container") + } + if !d.deleteCalled { + t.Error("expected DeleteDevContainer to be called") + } +} + +func TestDelete_StoppedContainer_SkipsStopAndDeletes(t *testing.T) { + d := &mockDriver{ + findResult: &config.ContainerDetails{ + ID: "container-abc", + State: config.ContainerDetailsState{Status: "exited"}, + Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, + }, + } + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + if d.stopCalled { + t.Error("StopDevContainer should not be called for stopped container") + } + if !d.deleteCalled { + t.Error("expected DeleteDevContainer to be called") + } +} + +func TestDelete_DeleteError_ReturnsError(t *testing.T) { + d := &mockDriver{ + findResult: &config.ContainerDetails{ + ID: "container-abc", + State: config.ContainerDetailsState{Status: "exited"}, + Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, + }, + deleteErr: fmt.Errorf("permission denied"), + } + r := newTestRunner(d) + + err := r.Delete(context.Background(), DeleteOptions{}) + + if err == nil { + t.Fatal("expected error from DeleteDevContainer, got nil") + } +} + +func TestCleanupDeliveryVolume_DoesNotPanic(t *testing.T) { + d := &mockDriver{} + r := newTestRunner(d) + + r.cleanupDeliveryVolume(context.Background()) +} From 5f311af6b2f3427882bd494e1255bfcb40b58043 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:39:50 -0500 Subject: [PATCH 05/17] fix(delivery): use defer for volume cleanup to cover nil-container path When a container is already absent (externally deleted or crashed), Delete() returned early before reaching the cleanup call. Using defer ensures cleanupDeliveryVolume() runs regardless of whether the container exists, preventing orphaned volumes in that scenario. --- pkg/devcontainer/delete.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/devcontainer/delete.go b/pkg/devcontainer/delete.go index d568a7897..e01a92c61 100644 --- a/pkg/devcontainer/delete.go +++ b/pkg/devcontainer/delete.go @@ -13,7 +13,9 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { containerDetails, err := r.Driver.FindDevContainer(ctx, r.ID) if err != nil { return fmt.Errorf("find dev container: %w", err) - } else if containerDetails == nil { + } + defer r.cleanupDeliveryVolume(ctx) + if containerDetails == nil { return nil } @@ -37,8 +39,6 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { } } - r.cleanupDeliveryVolume(ctx) - return nil } From e5258fc47be0edd3640156b4686ccb210fd9cdcb Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:49:19 -0500 Subject: [PATCH 06/17] fix(test): extract repeated string literals as constants to satisfy goconst lint Resolves golangci-lint goconst violations in delete_test.go. --- pkg/devcontainer/delete_test.go | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/devcontainer/delete_test.go b/pkg/devcontainer/delete_test.go index 47736afd1..01387bd7c 100644 --- a/pkg/devcontainer/delete_test.go +++ b/pkg/devcontainer/delete_test.go @@ -11,6 +11,11 @@ import ( "github.com/devsy-org/devsy/pkg/provider" ) +const ( + testContainerID = "container-abc" + testStatusRunning = "running" +) + type mockDriver struct { findResult *config.ContainerDetails findErr error @@ -107,8 +112,8 @@ func TestDelete_FindError_ReturnsError(t *testing.T) { func TestDelete_RunningContainer_StopsDeletesAndCleansUp(t *testing.T) { d := &mockDriver{ findResult: &config.ContainerDetails{ - ID: "container-abc", - State: config.ContainerDetailsState{Status: "running"}, + ID: testContainerID, + State: config.ContainerDetailsState{Status: testStatusRunning}, Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, }, } @@ -129,7 +134,7 @@ func TestDelete_RunningContainer_StopsDeletesAndCleansUp(t *testing.T) { func TestDelete_StoppedContainer_SkipsStopAndDeletes(t *testing.T) { d := &mockDriver{ findResult: &config.ContainerDetails{ - ID: "container-abc", + ID: testContainerID, State: config.ContainerDetailsState{Status: "exited"}, Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, }, @@ -151,7 +156,7 @@ func TestDelete_StoppedContainer_SkipsStopAndDeletes(t *testing.T) { func TestDelete_DeleteError_ReturnsError(t *testing.T) { d := &mockDriver{ findResult: &config.ContainerDetails{ - ID: "container-abc", + ID: testContainerID, State: config.ContainerDetailsState{Status: "exited"}, Config: config.ContainerDetailsConfig{Labels: map[string]string{}}, }, From ad219fd1830b21cfbe80f2cbe272647b762b093e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:53:23 -0500 Subject: [PATCH 07/17] feat(delivery): make helper image configurable with direct-copy fallback HelperImage field in ProviderDockerDriverConfig is threaded through FactoryOptions to LocalDockerDelivery. populateVolume() tries the configured helper container first (defaulting to busybox:latest), then falls back to writing the binary directly to the Docker volume mountpoint on the local filesystem. The direct-copy fallback is safe because LocalDockerDelivery is only used when Docker is local. --- pkg/agent/delivery/factory.go | 2 + pkg/agent/delivery/local_docker.go | 77 +++++++++++++++++++++++-- pkg/agent/delivery/local_docker_test.go | 45 +++++++++++++++ pkg/devcontainer/setup.go | 1 + pkg/provider/provider.go | 4 ++ 5 files changed, 124 insertions(+), 5 deletions(-) diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go index c42bde2d4..79804a013 100644 --- a/pkg/agent/delivery/factory.go +++ b/pkg/agent/delivery/factory.go @@ -16,6 +16,7 @@ type FactoryOptions struct { WorkspaceID string DockerCommand string DockerEnv []string + HelperImage string IsRemoteDocker bool ContainerID string ExecFunc inject.ExecFunc @@ -46,6 +47,7 @@ func NewAgentDelivery(opts FactoryOptions) AgentDelivery { return &LocalDockerDelivery{ DockerCommand: opts.DockerCommand, Environment: opts.DockerEnv, + HelperImage: opts.HelperImage, } } log.Debugf("using remote docker delivery for non-local docker daemon") diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index 90caf203d..58e16087a 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -3,8 +3,11 @@ package delivery import ( "context" "fmt" + "io" "os" "os/exec" + "path/filepath" + "strings" "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -14,15 +17,16 @@ import ( var _ AgentDelivery = (*LocalDockerDelivery)(nil) const ( - defaultDockerCmd = "docker" - volumePrefix = "devsy-agent-" - volumeMountPath = "/opt/devsy" - helperImage = "busybox:latest" + defaultDockerCmd = "docker" + volumePrefix = "devsy-agent-" + volumeMountPath = "/opt/devsy" + defaultHelperImage = "busybox:latest" ) type LocalDockerDelivery struct { DockerCommand string Environment []string + HelperImage string } func (d *LocalDockerDelivery) Phase() DeliveryPhase { @@ -77,6 +81,13 @@ func (d *LocalDockerDelivery) createVolume(ctx context.Context, name string) err return nil } +func (d *LocalDockerDelivery) helperImageName() string { + if d.HelperImage != "" { + return d.HelperImage + } + return defaultHelperImage +} + func (d *LocalDockerDelivery) populateVolume( ctx context.Context, volumeName string, @@ -89,6 +100,20 @@ func (d *LocalDockerDelivery) populateVolume( } defer func() { _ = binary.Close() }() + err = d.populateVolumeWithHelper(ctx, volumeName, binary) + if err == nil { + return nil + } + log.Debugf("helper container populate failed, trying direct copy: %v", err) + + return d.populateVolumeDirectCopy(ctx, volumeName, binary) +} + +func (d *LocalDockerDelivery) populateVolumeWithHelper( + ctx context.Context, + volumeName string, + binary io.Reader, +) error { containerName := "devsy-agent-init-" + volumeName script := fmt.Sprintf( "cat > %s/%s && chmod 755 %s/%s", @@ -99,7 +124,7 @@ func (d *LocalDockerDelivery) populateVolume( "--name", containerName, "-v", volumeName + ":" + volumeMountPath, "-i", - helperImage, + d.helperImageName(), "sh", "-c", script, } @@ -113,6 +138,48 @@ func (d *LocalDockerDelivery) populateVolume( return nil } +func (d *LocalDockerDelivery) populateVolumeDirectCopy( + ctx context.Context, + volumeName string, + binary io.Reader, +) error { + mountpoint, err := d.volumeMountpoint(ctx, volumeName) + if err != nil { + return fmt.Errorf("inspect volume mountpoint: %w", err) + } + + destPath := filepath.Join(mountpoint, binaryName()) + data, err := io.ReadAll(binary) + if err != nil { + return fmt.Errorf("read binary: %w", err) + } + + if err := os.WriteFile(destPath, data, 0o600); err != nil { + return fmt.Errorf("write binary to volume: %w", err) + } + // #nosec G302 -- agent binary must be executable + if err := os.Chmod(destPath, 0o755); err != nil { + return fmt.Errorf("chmod binary: %w", err) + } + + return nil +} + +func (d *LocalDockerDelivery) volumeMountpoint( + ctx context.Context, + volumeName string, +) (string, error) { + out, err := d.cmd( + ctx, "volume", "inspect", + "--format", "{{.Mountpoint}}", + volumeName, + ).CombinedOutput() + if err != nil { + return "", fmt.Errorf("%s: %w", string(out), err) + } + return strings.TrimSpace(string(out)), nil +} + func (d *LocalDockerDelivery) removeVolume(ctx context.Context, workspaceID string) error { volumeName := volumePrefix + workspaceID out, err := d.cmd(ctx, "volume", "rm", "-f", volumeName).CombinedOutput() diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 0f94b4f19..9d4622f46 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -4,6 +4,7 @@ import ( "context" "testing" + "github.com/devsy-org/devsy/pkg/provider" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -37,3 +38,47 @@ func TestBinaryName(t *testing.T) { name := binaryName() assert.Equal(t, "devsy", name) } + +func TestLocalDockerDelivery_HelperImageName_Default(t *testing.T) { + d := &LocalDockerDelivery{} + assert.Equal(t, "busybox:latest", d.helperImageName()) +} + +func TestLocalDockerDelivery_HelperImageName_Configured(t *testing.T) { + d := &LocalDockerDelivery{HelperImage: "registry.internal/tools/busybox:1.36"} + assert.Equal(t, "registry.internal/tools/busybox:1.36", d.helperImageName()) +} + +func TestNewAgentDelivery_LocalDocker_ThreadsHelperImage(t *testing.T) { + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.DockerDriver, + }, + }, + DockerCommand: "docker", + HelperImage: "my-registry/busybox:1.35", + } + + d := NewAgentDelivery(opts) + local, ok := d.(*LocalDockerDelivery) + require.True(t, ok) + assert.Equal(t, "my-registry/busybox:1.35", local.HelperImage) +} + +func TestNewAgentDelivery_LocalDocker_EmptyHelperImage(t *testing.T) { + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.DockerDriver, + }, + }, + DockerCommand: "docker", + } + + d := NewAgentDelivery(opts) + local, ok := d.(*LocalDockerDelivery) + require.True(t, ok) + assert.Empty(t, local.HelperImage) + assert.Equal(t, "busybox:latest", local.helperImageName()) +} diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index 0a3ef8ae4..c2468332b 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -97,6 +97,7 @@ func (r *runner) newAgentDelivery() delivery.AgentDelivery { WorkspaceID: r.ID, DockerCommand: dockerCmd, DockerEnv: dockerEnv, + HelperImage: r.WorkspaceConfig.Agent.Docker.HelperImage, ContainerID: r.ID, ExecFunc: execFn, }) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index 6e6e6eb3d..e153997ed 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -194,6 +194,10 @@ type ProviderDockerDriverConfig struct { // Environment variables to set when running docker commands Env map[string]string `json:"env,omitempty"` + + // HelperImage is used by LocalDockerDelivery for volume population. + // When empty, defaults to busybox:latest with automatic fallback to direct copy. + HelperImage string `json:"helperImage,omitempty"` } type ProviderKubernetesDriverConfig struct { From c78cd5bbf8c86e17bd2a905c1842e1857d7bd3c6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 08:01:47 -0500 Subject: [PATCH 08/17] fix(delivery): buffer binary before populate attempts, add fallback test Fixes consumed io.Reader bug: binary data is now read once into a byte slice, then a fresh bytes.NewReader is passed to the helper container attempt. populateVolumeDirectCopy accepts []byte directly. Adds behavioral test for the fallback path using a fake docker script that fails on 'run' but returns a temp dir on 'volume inspect', verifying the binary lands with correct content and 0755 permissions. --- pkg/agent/delivery/local_docker.go | 16 +++++----- pkg/agent/delivery/local_docker_test.go | 42 +++++++++++++++++++++++++ pkg/provider/provider.go | 2 +- 3 files changed, 52 insertions(+), 8 deletions(-) diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index 58e16087a..e236b78b8 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -1,6 +1,7 @@ package delivery import ( + "bytes" "context" "fmt" "io" @@ -100,13 +101,18 @@ func (d *LocalDockerDelivery) populateVolume( } defer func() { _ = binary.Close() }() - err = d.populateVolumeWithHelper(ctx, volumeName, binary) + data, err := io.ReadAll(binary) + if err != nil { + return fmt.Errorf("read binary: %w", err) + } + + err = d.populateVolumeWithHelper(ctx, volumeName, bytes.NewReader(data)) if err == nil { return nil } log.Debugf("helper container populate failed, trying direct copy: %v", err) - return d.populateVolumeDirectCopy(ctx, volumeName, binary) + return d.populateVolumeDirectCopy(ctx, volumeName, data) } func (d *LocalDockerDelivery) populateVolumeWithHelper( @@ -141,7 +147,7 @@ func (d *LocalDockerDelivery) populateVolumeWithHelper( func (d *LocalDockerDelivery) populateVolumeDirectCopy( ctx context.Context, volumeName string, - binary io.Reader, + data []byte, ) error { mountpoint, err := d.volumeMountpoint(ctx, volumeName) if err != nil { @@ -149,10 +155,6 @@ func (d *LocalDockerDelivery) populateVolumeDirectCopy( } destPath := filepath.Join(mountpoint, binaryName()) - data, err := io.ReadAll(binary) - if err != nil { - return fmt.Errorf("read binary: %w", err) - } if err := os.WriteFile(destPath, data, 0o600); err != nil { return fmt.Errorf("write binary to volume: %w", err) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index 9d4622f46..ce4f7af0c 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -1,7 +1,11 @@ package delivery import ( + "bytes" "context" + "io" + "os" + "path/filepath" "testing" "github.com/devsy-org/devsy/pkg/provider" @@ -82,3 +86,41 @@ func TestNewAgentDelivery_LocalDocker_EmptyHelperImage(t *testing.T) { assert.Empty(t, local.HelperImage) assert.Equal(t, "busybox:latest", local.helperImageName()) } + +func TestPopulateVolume_FallbackToDirectCopy(t *testing.T) { + tmpDir := t.TempDir() + mountDir := filepath.Join(tmpDir, "mount") + require.NoError(t, os.MkdirAll(mountDir, 0o750)) + + scriptPath := filepath.Join(tmpDir, "fake-docker.sh") + script := "#!/bin/sh\n" + + "case \"$1\" in\n" + + " run) echo \"image not found\" >&2; exit 1 ;;\n" + + " volume) echo \"" + mountDir + "\" ;;\n" + + " *) exit 1 ;;\n" + + "esac\n" + require.NoError(t, os.WriteFile(scriptPath, []byte(script), 0o600)) + // #nosec G302 -- test script must be executable + require.NoError(t, os.Chmod(scriptPath, 0o755)) + + binaryContent := []byte("fake-agent-binary-content") + binarySource := func(_ context.Context, _ string) (io.ReadCloser, error) { + return io.NopCloser(bytes.NewReader(binaryContent)), nil + } + + d := &LocalDockerDelivery{ + DockerCommand: scriptPath, + } + + err := d.populateVolume(context.Background(), "test-vol", binarySource, "amd64") + require.NoError(t, err) + + destPath := filepath.Join(mountDir, binaryName()) + data, err := os.ReadFile(destPath) //nolint:gosec // test reads from a temp directory we control + require.NoError(t, err) + assert.Equal(t, binaryContent, data) + + info, err := os.Stat(destPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o755), info.Mode().Perm()) +} diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index e153997ed..845333bae 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -196,7 +196,7 @@ type ProviderDockerDriverConfig struct { Env map[string]string `json:"env,omitempty"` // HelperImage is used by LocalDockerDelivery for volume population. - // When empty, defaults to busybox:latest with automatic fallback to direct copy. + // When empty, defaults to busybox:latest. A direct-copy fallback is used if the helper container approach fails. HelperImage string `json:"helperImage,omitempty"` } From 9133d8ddd25263d431ee923e18c2d6a704950710 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 08:14:04 -0500 Subject: [PATCH 09/17] fix(test): use defaultDockerCmd constant instead of string literal in delivery tests --- pkg/agent/delivery/factory_test.go | 2 +- pkg/agent/delivery/local_docker_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/agent/delivery/factory_test.go b/pkg/agent/delivery/factory_test.go index 8f6f97fe9..a881189f1 100644 --- a/pkg/agent/delivery/factory_test.go +++ b/pkg/agent/delivery/factory_test.go @@ -17,7 +17,7 @@ func TestNewAgentDelivery_LocalDocker(t *testing.T) { Driver: provider.DockerDriver, }, }, - DockerCommand: "docker", + DockerCommand: defaultDockerCmd, } d := NewAgentDelivery(opts) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index ce4f7af0c..6d9861707 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -60,7 +60,7 @@ func TestNewAgentDelivery_LocalDocker_ThreadsHelperImage(t *testing.T) { Driver: provider.DockerDriver, }, }, - DockerCommand: "docker", + DockerCommand: defaultDockerCmd, HelperImage: "my-registry/busybox:1.35", } @@ -77,7 +77,7 @@ func TestNewAgentDelivery_LocalDocker_EmptyHelperImage(t *testing.T) { Driver: provider.DockerDriver, }, }, - DockerCommand: "docker", + DockerCommand: defaultDockerCmd, } d := NewAgentDelivery(opts) From 8c3c30e29252422a71b3835e6467e70c6a1e88cd Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:34:17 -0500 Subject: [PATCH 10/17] feat(agent): implement AgentDelivery interface for container binary delivery Introduces a pluggable delivery strategy that replaces the monolithic inject.sh handshake with phase-aware delivery methods, eliminating race conditions identified in DEVSY-082. Three implementations: - LocalDockerDelivery: named volume pre-placement before container start - RemoteDockerDelivery: docker cp post-start for remote daemons - LegacyShellDelivery: wraps existing inject.sh path for custom drivers Includes factory, integration into setup.go/single.go, and E2E tests. --- e2e/e2e_suite_test.go | 1 + e2e/tests/delivery/delivery.go | 112 +++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 e2e/tests/delivery/delivery.go diff --git a/e2e/e2e_suite_test.go b/e2e/e2e_suite_test.go index 8333ab49b..058e3fa72 100644 --- a/e2e/e2e_suite_test.go +++ b/e2e/e2e_suite_test.go @@ -10,6 +10,7 @@ import ( // Register tests. _ "github.com/devsy-org/devsy/e2e/tests/build" _ "github.com/devsy-org/devsy/e2e/tests/context" + _ "github.com/devsy-org/devsy/e2e/tests/delivery" _ "github.com/devsy-org/devsy/e2e/tests/dockerinstall" _ "github.com/devsy-org/devsy/e2e/tests/down" _ "github.com/devsy-org/devsy/e2e/tests/exec" diff --git a/e2e/tests/delivery/delivery.go b/e2e/tests/delivery/delivery.go new file mode 100644 index 000000000..9b55c2065 --- /dev/null +++ b/e2e/tests/delivery/delivery.go @@ -0,0 +1,112 @@ +package delivery + +import ( + "context" + "os" + "os/exec" + "path/filepath" + + "github.com/devsy-org/devsy/e2e/framework" + "github.com/devsy-org/devsy/pkg/agent/delivery" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + "github.com/onsi/ginkgo/v2" + "github.com/onsi/gomega" +) + +var _ = ginkgo.Describe("agent delivery", ginkgo.Label("delivery"), func() { + ginkgo.Context("LocalDockerDelivery", func() { + ginkgo.It("should create volume, populate binary, and mount into RunOptions", + ginkgo.SpecTimeout(framework.TimeoutShort()), + func(ctx context.Context) { + d := &delivery.LocalDockerDelivery{DockerCommand: "docker"} + workspaceID := "e2e-delivery-local" + + runOpts := &driver.RunOptions{ + Mounts: []*config.Mount{}, + Env: map[string]string{}, + } + + binaryPath := findTestBinary() + + err := d.DeliverPreStart(ctx, delivery.PreStartOptions{ + WorkspaceID: workspaceID, + RunOptions: runOpts, + BinaryPath: binaryPath, + Arch: "amd64", + }) + framework.ExpectNoError(err) + ginkgo.DeferCleanup(func() { + _ = d.Cleanup(context.Background(), workspaceID) + }) + + gomega.Expect(runOpts.Mounts).To(gomega.HaveLen(1)) + expectedVolume := "devsy-agent-" + workspaceID + gomega.Expect(runOpts.Mounts[0].Source).To(gomega.Equal(expectedVolume)) + gomega.Expect(runOpts.Mounts[0].Type).To(gomega.Equal("volume")) + + // Verify binary in volume via docker run + out, err := exec.CommandContext(ctx, "docker", "run", "--rm", + "-v", "devsy-agent-"+workspaceID+":/opt/devsy", + "busybox:latest", "test", "-x", "/opt/devsy/devsy", + ).CombinedOutput() + framework.ExpectNoError( + err, "binary should be executable in volume: %s", string(out), + ) + }) + }) + + ginkgo.Context("RemoteDockerDelivery", func() { + ginkgo.It("should copy binary into running container via docker cp", + ginkgo.SpecTimeout(framework.TimeoutShort()), + func(ctx context.Context) { + containerName := "e2e-delivery-remote" + + out, err := exec.CommandContext(ctx, "docker", "run", "-d", + "--name", containerName, + "busybox:latest", "sleep", "120", + ).CombinedOutput() + framework.ExpectNoError(err, "failed to start test container: %s", string(out)) + ginkgo.DeferCleanup(func() { + _ = exec.Command("docker", "rm", "-f", containerName).Run() + }) + + d := &delivery.RemoteDockerDelivery{ + DockerCommand: "docker", + ContainerID: containerName, + } + + binaryPath := findTestBinary() + + err = d.DeliverPostStart(ctx, delivery.PostStartOptions{ + WorkspaceID: "e2e-workspace", + BinaryPath: binaryPath, + Arch: "amd64", + }) + framework.ExpectNoError(err) + + // Verify binary exists and is executable + out, err = exec.CommandContext(ctx, "docker", "exec", containerName, + "test", "-x", "/usr/local/bin/devsy", + ).CombinedOutput() + framework.ExpectNoError(err, "binary should be executable: %s", string(out)) + }) + }) +}) + +func findTestBinary() string { + candidates := []string{"/bin/sh", "/bin/busybox"} + for _, c := range candidates { + if _, err := os.Stat(c); err == nil { + return c + } + } + + binDir, _ := os.Getwd() + devsy := filepath.Join(binDir, "bin", "devsy-linux-amd64") + if _, err := os.Stat(devsy); err == nil { + return devsy + } + + return "/bin/sh" +} From e0ad8637db18a8456579c5bc2d73b2d6ed96bac5 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:16:02 -0500 Subject: [PATCH 11/17] feat(delivery): add KubernetesDelivery strategy for K8s agent binary injection Implements KubernetesDelivery as a PhasePostStart strategy that streams the agent binary into running K8s containers via ExecFunc. The exec function routes through CommandDevContainer which uses K8s exec under the hood. Factory now routes provider.KubernetesDriver to this strategy instead of falling through to LegacyShellDelivery. --- pkg/agent/delivery/factory.go | 6 ++ pkg/agent/delivery/factory_test.go | 10 ++- pkg/agent/delivery/kubernetes.go | 53 ++++++++++++ pkg/agent/delivery/kubernetes_test.go | 111 ++++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 2 deletions(-) create mode 100644 pkg/agent/delivery/kubernetes.go create mode 100644 pkg/agent/delivery/kubernetes_test.go diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go index 79804a013..5443fd936 100644 --- a/pkg/agent/delivery/factory.go +++ b/pkg/agent/delivery/factory.go @@ -41,6 +41,12 @@ func NewAgentDelivery(opts FactoryOptions) AgentDelivery { ContainerID: opts.ContainerID, } + case driverType == provider.KubernetesDriver: + log.Debugf("using kubernetes delivery (exec)") + return &KubernetesDelivery{ + ExecFunc: opts.ExecFunc, + } + case driverType == "" || driverType == provider.DockerDriver: if isDockerLocal(opts.DockerCommand) { log.Debugf("using local docker delivery (named volume)") diff --git a/pkg/agent/delivery/factory_test.go b/pkg/agent/delivery/factory_test.go index a881189f1..4f1fa2ca7 100644 --- a/pkg/agent/delivery/factory_test.go +++ b/pkg/agent/delivery/factory_test.go @@ -73,17 +73,23 @@ func TestNewAgentDelivery_CustomDriver(t *testing.T) { assert.Equal(t, PhasePostStart, d.Phase()) } -func TestNewAgentDelivery_KubernetesDriver_FallsToLegacy(t *testing.T) { +func TestNewAgentDelivery_KubernetesDriver(t *testing.T) { + execFn := func(ctx context.Context, cmd string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { + return nil + } + opts := FactoryOptions{ WorkspaceConfig: &provider.AgentWorkspaceInfo{ Agent: provider.ProviderAgentConfig{ Driver: provider.KubernetesDriver, }, }, + ExecFunc: execFn, } d := NewAgentDelivery(opts) - assert.IsType(t, &LegacyShellDelivery{}, d) + assert.IsType(t, &KubernetesDelivery{}, d) + assert.Equal(t, PhasePostStart, d.Phase()) } func TestIsDockerLocal(t *testing.T) { diff --git a/pkg/agent/delivery/kubernetes.go b/pkg/agent/delivery/kubernetes.go new file mode 100644 index 000000000..aeec7f3ce --- /dev/null +++ b/pkg/agent/delivery/kubernetes.go @@ -0,0 +1,53 @@ +package delivery + +import ( + "context" + "fmt" + + "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/inject" + "github.com/devsy-org/devsy/pkg/log" +) + +var _ AgentDelivery = (*KubernetesDelivery)(nil) + +type KubernetesDelivery struct { + ExecFunc inject.ExecFunc +} + +func (d *KubernetesDelivery) Phase() DeliveryPhase { + return PhasePostStart +} + +func (d *KubernetesDelivery) DeliverPreStart(_ context.Context, _ PreStartOptions) error { + return fmt.Errorf("KubernetesDelivery does not support pre-start delivery") +} + +func (d *KubernetesDelivery) DeliverPostStart(ctx context.Context, opts PostStartOptions) error { + if opts.BinarySource == nil { + return fmt.Errorf("binary source is required for kubernetes delivery") + } + if d.ExecFunc == nil { + return fmt.Errorf("exec function is required for kubernetes delivery") + } + + binary, err := opts.BinarySource(ctx, opts.Arch) + if err != nil { + return fmt.Errorf("acquire binary: %w", err) + } + defer func() { _ = binary.Close() }() + + destPath := agent.ContainerDevsyHelperLocation + script := fmt.Sprintf("cat > %s && chmod 755 %s", destPath, destPath) + + if err := d.ExecFunc(ctx, script, binary, nil, nil); err != nil { + return fmt.Errorf("write binary to container: %w", err) + } + + log.Debugf("delivered agent binary to kubernetes container via exec") + return nil +} + +func (d *KubernetesDelivery) Cleanup(_ context.Context, _ string) error { + return nil +} diff --git a/pkg/agent/delivery/kubernetes_test.go b/pkg/agent/delivery/kubernetes_test.go new file mode 100644 index 000000000..ccf1785d7 --- /dev/null +++ b/pkg/agent/delivery/kubernetes_test.go @@ -0,0 +1,111 @@ +package delivery + +import ( + "bytes" + "context" + "fmt" + "io" + "strings" + "testing" + + "github.com/devsy-org/devsy/pkg/agent" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestKubernetesDelivery_Phase(t *testing.T) { + d := &KubernetesDelivery{} + assert.Equal(t, PhasePostStart, d.Phase()) +} + +func TestKubernetesDelivery_DeliverPreStart_ReturnsError(t *testing.T) { + d := &KubernetesDelivery{} + err := d.DeliverPreStart(context.Background(), PreStartOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "does not support pre-start") +} + +func TestKubernetesDelivery_DeliverPostStart_RequiresBinarySource(t *testing.T) { + d := &KubernetesDelivery{ + ExecFunc: func(_ context.Context, _ string, _ io.Reader, _ io.Writer, _ io.Writer) error { + return nil + }, + } + err := d.DeliverPostStart(context.Background(), PostStartOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "binary source is required") +} + +func TestKubernetesDelivery_DeliverPostStart_RequiresExecFunc(t *testing.T) { + d := &KubernetesDelivery{} + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinarySource: fakeBinarySource, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "exec function is required") +} + +func TestKubernetesDelivery_DeliverPostStart_WritesBinary(t *testing.T) { + binaryData := "test-binary-content" + var capturedCmd string + var capturedStdin bytes.Buffer + + execFn := func(_ context.Context, cmd string, stdin io.Reader, _ io.Writer, _ io.Writer) error { + capturedCmd = cmd + if stdin != nil { + _, _ = io.Copy(&capturedStdin, stdin) + } + return nil + } + + d := &KubernetesDelivery{ExecFunc: execFn} + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinarySource: func(_ context.Context, _ string) (io.ReadCloser, error) { + return io.NopCloser(strings.NewReader(binaryData)), nil + }, + Arch: "amd64", + }) + + require.NoError(t, err) + + destPath := agent.ContainerDevsyHelperLocation + expectedCmd := fmt.Sprintf("cat > %s && chmod 755 %s", destPath, destPath) + assert.Equal(t, expectedCmd, capturedCmd) + assert.Equal(t, binaryData, capturedStdin.String()) +} + +func TestKubernetesDelivery_DeliverPostStart_BinarySourceError(t *testing.T) { + execFn := func(_ context.Context, _ string, _ io.Reader, _ io.Writer, _ io.Writer) error { + return nil + } + + d := &KubernetesDelivery{ExecFunc: execFn} + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinarySource: func(_ context.Context, _ string) (io.ReadCloser, error) { + return nil, fmt.Errorf("download failed") + }, + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "acquire binary") +} + +func TestKubernetesDelivery_DeliverPostStart_ExecError(t *testing.T) { + execFn := func(_ context.Context, _ string, _ io.Reader, _ io.Writer, _ io.Writer) error { + return fmt.Errorf("exec failed") + } + + d := &KubernetesDelivery{ExecFunc: execFn} + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinarySource: fakeBinarySource, + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "write binary to container") +} + +func TestKubernetesDelivery_Cleanup_IsNoOp(t *testing.T) { + d := &KubernetesDelivery{} + err := d.Cleanup(context.Background(), "workspace-123") + assert.NoError(t, err) +} From 3d4a495630cdec7442043c95528681565959fb87 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 07:41:28 -0500 Subject: [PATCH 12/17] fix(delivery): move KubernetesDriver case before IsRemoteDocker check --- pkg/agent/delivery/factory.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go index 5443fd936..3affd0338 100644 --- a/pkg/agent/delivery/factory.go +++ b/pkg/agent/delivery/factory.go @@ -33,6 +33,12 @@ func NewAgentDelivery(opts FactoryOptions) AgentDelivery { DownloadURL: "", } + case driverType == provider.KubernetesDriver: + log.Debugf("using kubernetes delivery (exec)") + return &KubernetesDelivery{ + ExecFunc: opts.ExecFunc, + } + case opts.IsRemoteDocker: log.Debugf("using remote docker delivery (docker cp)") return &RemoteDockerDelivery{ @@ -41,12 +47,6 @@ func NewAgentDelivery(opts FactoryOptions) AgentDelivery { ContainerID: opts.ContainerID, } - case driverType == provider.KubernetesDriver: - log.Debugf("using kubernetes delivery (exec)") - return &KubernetesDelivery{ - ExecFunc: opts.ExecFunc, - } - case driverType == "" || driverType == provider.DockerDriver: if isDockerLocal(opts.DockerCommand) { log.Debugf("using local docker delivery (named volume)") From 238a38f9337bcda10f529f7cbc904afb1d246e0c Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 08:32:26 -0500 Subject: [PATCH 13/17] fix(delivery): use gzip compression and atomic write for K8s binary injection Streaming the raw ~67MB agent binary via kubectl exec caused OOM kills (exit code 137) on resource-constrained KinD pods. Compress with gzip before streaming and use a temp file with atomic mv to prevent partial writes from corrupting the destination path on failure. --- pkg/agent/delivery/kubernetes.go | 22 ++++++++++++++++++++-- pkg/agent/delivery/kubernetes_test.go | 13 ++++++++++--- 2 files changed, 30 insertions(+), 5 deletions(-) diff --git a/pkg/agent/delivery/kubernetes.go b/pkg/agent/delivery/kubernetes.go index aeec7f3ce..662bfa4a2 100644 --- a/pkg/agent/delivery/kubernetes.go +++ b/pkg/agent/delivery/kubernetes.go @@ -1,8 +1,10 @@ package delivery import ( + "compress/gzip" "context" "fmt" + "io" "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/inject" @@ -37,10 +39,26 @@ func (d *KubernetesDelivery) DeliverPostStart(ctx context.Context, opts PostStar } defer func() { _ = binary.Close() }() + pr, pw := io.Pipe() + go func() { + gw := gzip.NewWriter(pw) + _, copyErr := io.Copy(gw, binary) + closeErr := gw.Close() + if copyErr != nil { + _ = pw.CloseWithError(copyErr) + } else { + _ = pw.CloseWithError(closeErr) + } + }() + destPath := agent.ContainerDevsyHelperLocation - script := fmt.Sprintf("cat > %s && chmod 755 %s", destPath, destPath) + script := fmt.Sprintf( + `set -e; t=$(mktemp %s.XXXXXX); gzip -d > "$t" && chmod 755 "$t" && mv "$t" %s || { rm -f "$t"; exit 1; }`, + destPath, + destPath, + ) - if err := d.ExecFunc(ctx, script, binary, nil, nil); err != nil { + if err := d.ExecFunc(ctx, script, pr, nil, nil); err != nil { return fmt.Errorf("write binary to container: %w", err) } diff --git a/pkg/agent/delivery/kubernetes_test.go b/pkg/agent/delivery/kubernetes_test.go index ccf1785d7..3e48188c0 100644 --- a/pkg/agent/delivery/kubernetes_test.go +++ b/pkg/agent/delivery/kubernetes_test.go @@ -2,6 +2,7 @@ package delivery import ( "bytes" + "compress/gzip" "context" "fmt" "io" @@ -69,9 +70,15 @@ func TestKubernetesDelivery_DeliverPostStart_WritesBinary(t *testing.T) { require.NoError(t, err) destPath := agent.ContainerDevsyHelperLocation - expectedCmd := fmt.Sprintf("cat > %s && chmod 755 %s", destPath, destPath) - assert.Equal(t, expectedCmd, capturedCmd) - assert.Equal(t, binaryData, capturedStdin.String()) + assert.Contains(t, capturedCmd, "gzip -d") + assert.Contains(t, capturedCmd, destPath) + assert.Contains(t, capturedCmd, "chmod 755") + + gr, err := gzip.NewReader(&capturedStdin) + require.NoError(t, err) + decompressed, err := io.ReadAll(gr) + require.NoError(t, err) + assert.Equal(t, binaryData, string(decompressed)) } func TestKubernetesDelivery_DeliverPostStart_BinarySourceError(t *testing.T) { From 8dac68156401c45a374bb1855b40701b6dbbf51e Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 09:18:20 -0500 Subject: [PATCH 14/17] fix(delivery): stream raw binary instead of gzip for K8s delivery The gzip -d command is not available in minimal/dockerless K8s containers, causing binary delivery to fail with exit code 1 and the fallback legacy inject to also fail. Stream the raw binary via cat instead. --- pkg/agent/delivery/kubernetes.go | 18 ++---------------- pkg/agent/delivery/kubernetes_test.go | 9 ++------- 2 files changed, 4 insertions(+), 23 deletions(-) diff --git a/pkg/agent/delivery/kubernetes.go b/pkg/agent/delivery/kubernetes.go index 662bfa4a2..e315fbc3b 100644 --- a/pkg/agent/delivery/kubernetes.go +++ b/pkg/agent/delivery/kubernetes.go @@ -1,10 +1,8 @@ package delivery import ( - "compress/gzip" "context" "fmt" - "io" "github.com/devsy-org/devsy/pkg/agent" "github.com/devsy-org/devsy/pkg/inject" @@ -39,26 +37,14 @@ func (d *KubernetesDelivery) DeliverPostStart(ctx context.Context, opts PostStar } defer func() { _ = binary.Close() }() - pr, pw := io.Pipe() - go func() { - gw := gzip.NewWriter(pw) - _, copyErr := io.Copy(gw, binary) - closeErr := gw.Close() - if copyErr != nil { - _ = pw.CloseWithError(copyErr) - } else { - _ = pw.CloseWithError(closeErr) - } - }() - destPath := agent.ContainerDevsyHelperLocation script := fmt.Sprintf( - `set -e; t=$(mktemp %s.XXXXXX); gzip -d > "$t" && chmod 755 "$t" && mv "$t" %s || { rm -f "$t"; exit 1; }`, + `set -e; t=$(mktemp %s.XXXXXX); cat > "$t" && chmod 755 "$t" && mv "$t" %s || { rm -f "$t"; exit 1; }`, destPath, destPath, ) - if err := d.ExecFunc(ctx, script, pr, nil, nil); err != nil { + if err := d.ExecFunc(ctx, script, binary, nil, nil); err != nil { return fmt.Errorf("write binary to container: %w", err) } diff --git a/pkg/agent/delivery/kubernetes_test.go b/pkg/agent/delivery/kubernetes_test.go index 3e48188c0..cc6c9b56f 100644 --- a/pkg/agent/delivery/kubernetes_test.go +++ b/pkg/agent/delivery/kubernetes_test.go @@ -2,7 +2,6 @@ package delivery import ( "bytes" - "compress/gzip" "context" "fmt" "io" @@ -70,15 +69,11 @@ func TestKubernetesDelivery_DeliverPostStart_WritesBinary(t *testing.T) { require.NoError(t, err) destPath := agent.ContainerDevsyHelperLocation - assert.Contains(t, capturedCmd, "gzip -d") + assert.Contains(t, capturedCmd, "cat >") assert.Contains(t, capturedCmd, destPath) assert.Contains(t, capturedCmd, "chmod 755") - gr, err := gzip.NewReader(&capturedStdin) - require.NoError(t, err) - decompressed, err := io.ReadAll(gr) - require.NoError(t, err) - assert.Equal(t, binaryData, string(decompressed)) + assert.Equal(t, binaryData, capturedStdin.String()) } func TestKubernetesDelivery_DeliverPostStart_BinarySourceError(t *testing.T) { From ab8a12744950bc9fcdcee3bcde14d4c200dfb0c6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 09:58:40 -0500 Subject: [PATCH 15/17] fix(delivery): route K8s driver to LegacyShellDelivery for reliable injection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit KubernetesDelivery's simple cat-over-stdin approach fails in dockerless K8s containers — the binary appears delivered but isn't executable. Route KubernetesDriver to LegacyShellDelivery which uses the proven inject.sh handshake protocol that handles binary transfer reliably across all container types. --- pkg/agent/delivery/factory.go | 7 ++++--- pkg/agent/delivery/factory_test.go | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go index 3affd0338..4c9e5c051 100644 --- a/pkg/agent/delivery/factory.go +++ b/pkg/agent/delivery/factory.go @@ -34,9 +34,10 @@ func NewAgentDelivery(opts FactoryOptions) AgentDelivery { } case driverType == provider.KubernetesDriver: - log.Debugf("using kubernetes delivery (exec)") - return &KubernetesDelivery{ - ExecFunc: opts.ExecFunc, + log.Debugf("using legacy shell delivery for kubernetes driver") + return &LegacyShellDelivery{ + ExecFunc: opts.ExecFunc, + DownloadURL: "", } case opts.IsRemoteDocker: diff --git a/pkg/agent/delivery/factory_test.go b/pkg/agent/delivery/factory_test.go index 4f1fa2ca7..0e6cb134f 100644 --- a/pkg/agent/delivery/factory_test.go +++ b/pkg/agent/delivery/factory_test.go @@ -88,7 +88,7 @@ func TestNewAgentDelivery_KubernetesDriver(t *testing.T) { } d := NewAgentDelivery(opts) - assert.IsType(t, &KubernetesDelivery{}, d) + assert.IsType(t, &LegacyShellDelivery{}, d) assert.Equal(t, PhasePostStart, d.Phase()) } From d2d53643fd58468c6a33d5a9f240ff042cc23456 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 13 May 2026 07:43:58 -0500 Subject: [PATCH 16/17] fix(delivery): update e2e test to use BinarySource instead of removed BinaryPath field PreStartOptions and PostStartOptions were refactored to use BinarySourceFunc instead of a plain string path. The e2e test still referenced the old BinaryPath field, causing typecheck failures in CI. --- e2e/tests/delivery/delivery.go | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/e2e/tests/delivery/delivery.go b/e2e/tests/delivery/delivery.go index 9b55c2065..db0a0025e 100644 --- a/e2e/tests/delivery/delivery.go +++ b/e2e/tests/delivery/delivery.go @@ -2,6 +2,7 @@ package delivery import ( "context" + "io" "os" "os/exec" "path/filepath" @@ -30,10 +31,10 @@ var _ = ginkgo.Describe("agent delivery", ginkgo.Label("delivery"), func() { binaryPath := findTestBinary() err := d.DeliverPreStart(ctx, delivery.PreStartOptions{ - WorkspaceID: workspaceID, - RunOptions: runOpts, - BinaryPath: binaryPath, - Arch: "amd64", + WorkspaceID: workspaceID, + RunOptions: runOpts, + BinarySource: binarySourceFromPath(binaryPath), + Arch: "amd64", }) framework.ExpectNoError(err) ginkgo.DeferCleanup(func() { @@ -79,9 +80,9 @@ var _ = ginkgo.Describe("agent delivery", ginkgo.Label("delivery"), func() { binaryPath := findTestBinary() err = d.DeliverPostStart(ctx, delivery.PostStartOptions{ - WorkspaceID: "e2e-workspace", - BinaryPath: binaryPath, - Arch: "amd64", + WorkspaceID: "e2e-workspace", + BinarySource: binarySourceFromPath(binaryPath), + Arch: "amd64", }) framework.ExpectNoError(err) @@ -94,6 +95,13 @@ var _ = ginkgo.Describe("agent delivery", ginkgo.Label("delivery"), func() { }) }) +func binarySourceFromPath(path string) delivery.BinarySourceFunc { + return func(_ context.Context, _ string) (io.ReadCloser, error) { + // #nosec G304 -- test helper with controlled paths + return os.Open(path) + } +} + func findTestBinary() string { candidates := []string{"/bin/sh", "/bin/busybox"} for _, c := range candidates { From 8c47ee96d083e763680f4e14410a36cc16f451ec Mon Sep 17 00:00:00 2001 From: Samuel K Date: Wed, 13 May 2026 08:15:49 -0500 Subject: [PATCH 17/17] fix(delivery): add delivery label to CI test matrix so e2e tests run --- .github/workflows/pr-ci.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/pr-ci.yml b/.github/workflows/pr-ci.yml index 0a575c2bc..2d9391091 100644 --- a/.github/workflows/pr-ci.yml +++ b/.github/workflows/pr-ci.yml @@ -216,6 +216,12 @@ jobs: install-kind: false requires-secret: false + - label: delivery + runner: ubuntu-latest + free-disk-space: false + install-kind: false + requires-secret: false + - label: exec runner: ubuntu-latest free-disk-space: false