From f457f28368b064af07f41ab95ac2246b1e07d66b Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:34:17 -0500 Subject: [PATCH 1/6] 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 +++++++++++++++ pkg/agent/delivery/delivery.go | 48 +++++++ .../delivery/delivery_integration_test.go | 108 ++++++++++++++ pkg/agent/delivery/factory.go | 122 ++++++++++++++++ pkg/agent/delivery/factory_test.go | 126 ++++++++++++++++ pkg/agent/delivery/legacy_shell.go | 77 ++++++++++ pkg/agent/delivery/legacy_shell_test.go | 62 ++++++++ pkg/agent/delivery/local_docker.go | 135 ++++++++++++++++++ pkg/agent/delivery/local_docker_test.go | 77 ++++++++++ pkg/agent/delivery/remote_docker.go | 86 +++++++++++ pkg/agent/delivery/remote_docker_test.go | 46 ++++++ pkg/devcontainer/setup.go | 45 ++++++ pkg/devcontainer/single.go | 34 +++++ 14 files changed, 1079 insertions(+) create mode 100644 e2e/tests/delivery/delivery.go create mode 100644 pkg/agent/delivery/delivery.go create mode 100644 pkg/agent/delivery/delivery_integration_test.go create mode 100644 pkg/agent/delivery/factory.go create mode 100644 pkg/agent/delivery/factory_test.go create mode 100644 pkg/agent/delivery/legacy_shell.go create mode 100644 pkg/agent/delivery/legacy_shell_test.go create mode 100644 pkg/agent/delivery/local_docker.go create mode 100644 pkg/agent/delivery/local_docker_test.go create mode 100644 pkg/agent/delivery/remote_docker.go create mode 100644 pkg/agent/delivery/remote_docker_test.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" +} diff --git a/pkg/agent/delivery/delivery.go b/pkg/agent/delivery/delivery.go new file mode 100644 index 000000000..ee9dfd5d6 --- /dev/null +++ b/pkg/agent/delivery/delivery.go @@ -0,0 +1,48 @@ +package delivery + +import ( + "context" + "fmt" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" +) + +type DeliveryPhase int + +const ( + PhasePreStart DeliveryPhase = iota + PhasePostStart +) + +func (p DeliveryPhase) String() string { + switch p { + case PhasePreStart: + return "pre-start" + case PhasePostStart: + return "post-start" + default: + return fmt.Sprintf("unknown(%d)", int(p)) + } +} + +type PreStartOptions struct { + WorkspaceID string + RunOptions *driver.RunOptions + BinaryPath string + Arch string +} + +type PostStartOptions struct { + WorkspaceID string + ContainerDetails *config.ContainerDetails + BinaryPath string + Arch string +} + +type AgentDelivery interface { + Phase() DeliveryPhase + DeliverPreStart(ctx context.Context, opts PreStartOptions) error + DeliverPostStart(ctx context.Context, opts PostStartOptions) error + Cleanup(ctx context.Context, workspaceID string) error +} diff --git a/pkg/agent/delivery/delivery_integration_test.go b/pkg/agent/delivery/delivery_integration_test.go new file mode 100644 index 000000000..6ed08aa48 --- /dev/null +++ b/pkg/agent/delivery/delivery_integration_test.go @@ -0,0 +1,108 @@ +//go:build integration + +package delivery + +import ( + "context" + "os/exec" + "testing" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func dockerAvailable() bool { + cmd := exec.Command("docker", "info") + return cmd.Run() == nil +} + +func TestLocalDockerDelivery_Integration(t *testing.T) { + if !dockerAvailable() { + t.Skip("docker not available") + } + + ctx := context.Background() + d := &LocalDockerDelivery{DockerCommand: "docker"} + workspaceID := "delivery-test-integration" + + runOpts := &driver.RunOptions{ + Mounts: []*config.Mount{}, + Env: map[string]string{}, + } + opts := PreStartOptions{ + WorkspaceID: workspaceID, + RunOptions: runOpts, + BinaryPath: "/bin/sh", + Arch: "amd64", + } + + err := d.DeliverPreStart(ctx, opts) + require.NoError(t, err) + + assert.Len(t, runOpts.Mounts, 1) + assert.Equal(t, "devsy-agent-"+workspaceID, runOpts.Mounts[0].Source) + assert.Equal(t, volumeMountPath, runOpts.Mounts[0].Target) + assert.Equal(t, "volume", runOpts.Mounts[0].Type) + + // Verify volume exists + out, err := exec.CommandContext(ctx, "docker", "volume", "inspect", "devsy-agent-"+workspaceID). + CombinedOutput() + require.NoError(t, err, "volume should exist: %s", string(out)) + + // Verify binary is in the volume + out, err = exec.CommandContext(ctx, "docker", "run", "--rm", + "-v", "devsy-agent-"+workspaceID+":"+volumeMountPath, + "busybox:latest", "ls", "-la", volumeMountPath+"/devsy", + ).CombinedOutput() + require.NoError(t, err, "binary should exist in volume: %s", string(out)) + assert.Contains(t, string(out), "rwx") + + // Cleanup + err = d.Cleanup(ctx, workspaceID) + require.NoError(t, err) + + // Verify volume removed + out, _ = exec.CommandContext(ctx, "docker", "volume", "inspect", "devsy-agent-"+workspaceID). + CombinedOutput() + assert.Contains(t, string(out), "No such volume") +} + +func TestRemoteDockerDelivery_Integration(t *testing.T) { + if !dockerAvailable() { + t.Skip("docker not available") + } + + ctx := context.Background() + containerID := "delivery-test-remote" + + // Create a test container + out, err := exec.CommandContext(ctx, "docker", "run", "-d", + "--name", containerID, + "busybox:latest", "sleep", "60", + ).CombinedOutput() + require.NoError(t, err, "failed to create test container: %s", string(out)) + defer func() { + _ = exec.CommandContext(ctx, "docker", "rm", "-f", containerID).Run() + }() + + d := &RemoteDockerDelivery{ + DockerCommand: "docker", + ContainerID: containerID, + } + + err = d.DeliverPostStart(ctx, PostStartOptions{ + WorkspaceID: "test-workspace", + BinaryPath: "/bin/sh", + Arch: "amd64", + }) + require.NoError(t, err) + + // Verify binary exists in container + out, err = exec.CommandContext(ctx, "docker", "exec", containerID, + "ls", "-la", "/usr/local/bin/devsy", + ).CombinedOutput() + require.NoError(t, err, "binary should exist in container: %s", string(out)) + assert.Contains(t, string(out), "rwx") +} diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go new file mode 100644 index 000000000..a67da7122 --- /dev/null +++ b/pkg/agent/delivery/factory.go @@ -0,0 +1,122 @@ +package delivery + +import ( + "context" + "fmt" + "io" + "os" + + "github.com/devsy-org/devsy/pkg/inject" + "github.com/devsy-org/devsy/pkg/log" + "github.com/devsy-org/devsy/pkg/provider" +) + +type FactoryOptions struct { + WorkspaceConfig *provider.AgentWorkspaceInfo + WorkspaceID string + DockerCommand string + DockerEnv []string + IsRemoteDocker bool + ContainerID string + ExecFunc inject.ExecFunc +} + +func NewAgentDelivery(opts FactoryOptions) AgentDelivery { + driverType := opts.WorkspaceConfig.Agent.Driver + + switch { + case driverType == provider.CustomDriver: + log.Debugf("using legacy shell delivery for custom driver") + return &LegacyShellDelivery{ + ExecFunc: opts.ExecFunc, + DownloadURL: "", + } + + case opts.IsRemoteDocker: + log.Debugf("using remote docker delivery (docker cp)") + return &RemoteDockerDelivery{ + DockerCommand: opts.DockerCommand, + Environment: opts.DockerEnv, + ContainerID: opts.ContainerID, + } + + case driverType == "" || driverType == provider.DockerDriver: + if isDockerLocal(opts.DockerCommand) { + log.Debugf("using local docker delivery (named volume)") + return &LocalDockerDelivery{ + DockerCommand: opts.DockerCommand, + Environment: opts.DockerEnv, + } + } + log.Debugf("using remote docker delivery for non-local docker daemon") + return &RemoteDockerDelivery{ + DockerCommand: opts.DockerCommand, + Environment: opts.DockerEnv, + ContainerID: opts.ContainerID, + } + + default: + log.Debugf("using legacy shell delivery for driver: %s", driverType) + return &LegacyShellDelivery{ + ExecFunc: opts.ExecFunc, + DownloadURL: "", + } + } +} + +func isDockerLocal(_ string) bool { + envHost := os.Getenv("DOCKER_HOST") + return envHost == "" || isLocalDockerHost(envHost) +} + +func isLocalDockerHost(host string) bool { + return host == "" || + host == "unix:///var/run/docker.sock" || + host == "npipe:////./pipe/docker_engine" || + len(host) > 7 && host[:7] == "unix://" +} + +// CommandFunc adapts a driver's command function to inject.ExecFunc. +func CommandFunc( + driverCmd func( + ctx context.Context, + workspaceID, user, command string, + stdin io.Reader, stdout io.Writer, stderr io.Writer, + ) error, + workspaceID string, +) inject.ExecFunc { + return func( + ctx context.Context, + command string, + stdin io.Reader, stdout io.Writer, stderr io.Writer, + ) error { + return driverCmd(ctx, workspaceID, "root", command, stdin, stdout, stderr) + } +} + +// Deliver calls the appropriate delivery method based on the strategy's phase. +func Deliver( + ctx context.Context, + strategy AgentDelivery, + preOpts *PreStartOptions, + postOpts *PostStartOptions, +) error { + switch strategy.Phase() { + case PhasePreStart: + if preOpts == nil { + return fmt.Errorf( + "pre-start options required for %s delivery", strategy.Phase(), + ) + } + return strategy.DeliverPreStart(ctx, *preOpts) + case PhasePostStart: + if postOpts == nil { + return fmt.Errorf( + "post-start options required for %s delivery", strategy.Phase(), + ) + } + return strategy.DeliverPostStart(ctx, *postOpts) + default: + return fmt.Errorf("unknown delivery phase: %s", strategy.Phase()) + } +} diff --git a/pkg/agent/delivery/factory_test.go b/pkg/agent/delivery/factory_test.go new file mode 100644 index 000000000..a97c376f8 --- /dev/null +++ b/pkg/agent/delivery/factory_test.go @@ -0,0 +1,126 @@ +package delivery + +import ( + "context" + "io" + "testing" + + "github.com/devsy-org/devsy/pkg/provider" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewAgentDelivery_LocalDocker(t *testing.T) { + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.DockerDriver, + }, + }, + DockerCommand: "docker", + } + + d := NewAgentDelivery(opts) + assert.IsType(t, &LocalDockerDelivery{}, d) + assert.Equal(t, PhasePreStart, d.Phase()) +} + +func TestNewAgentDelivery_EmptyDriver_DefaultsToLocal(t *testing.T) { + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: "", + }, + }, + } + + d := NewAgentDelivery(opts) + assert.IsType(t, &LocalDockerDelivery{}, d) +} + +func TestNewAgentDelivery_RemoteDocker(t *testing.T) { + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.DockerDriver, + }, + }, + IsRemoteDocker: true, + ContainerID: "abc123", + } + + d := NewAgentDelivery(opts) + assert.IsType(t, &RemoteDockerDelivery{}, d) + assert.Equal(t, PhasePostStart, d.Phase()) +} + +func TestNewAgentDelivery_CustomDriver(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.CustomDriver, + }, + }, + ExecFunc: execFn, + } + + d := NewAgentDelivery(opts) + assert.IsType(t, &LegacyShellDelivery{}, d) + assert.Equal(t, PhasePostStart, d.Phase()) +} + +func TestNewAgentDelivery_KubernetesDriver_FallsToLegacy(t *testing.T) { + opts := FactoryOptions{ + WorkspaceConfig: &provider.AgentWorkspaceInfo{ + Agent: provider.ProviderAgentConfig{ + Driver: provider.KubernetesDriver, + }, + }, + } + + d := NewAgentDelivery(opts) + assert.IsType(t, &LegacyShellDelivery{}, d) +} + +func TestIsDockerLocal(t *testing.T) { + assert.True(t, isLocalDockerHost("")) + assert.True(t, isLocalDockerHost("unix:///var/run/docker.sock")) + assert.True(t, isLocalDockerHost("unix:///home/user/.docker/desktop/docker.sock")) + assert.True(t, isLocalDockerHost("npipe:////./pipe/docker_engine")) + assert.False(t, isLocalDockerHost("tcp://192.168.1.100:2376")) + assert.False(t, isLocalDockerHost("ssh://user@remote-host")) +} + +func TestDeliver_PreStart(t *testing.T) { + d := &LegacyShellDelivery{} + err := Deliver(context.Background(), d, nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "post-start options required") +} + +func TestDeliver_UnknownPhase(t *testing.T) { + d := &mockDelivery{phase: DeliveryPhase(99)} + err := Deliver(context.Background(), d, nil, nil) + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown delivery phase") +} + +type mockDelivery struct { + phase DeliveryPhase +} + +func (m *mockDelivery) Phase() DeliveryPhase { return m.phase } + +func (m *mockDelivery) DeliverPreStart(_ context.Context, _ PreStartOptions) error { + return nil +} + +func (m *mockDelivery) DeliverPostStart(_ context.Context, _ PostStartOptions) error { + return nil +} + +func (m *mockDelivery) Cleanup(_ context.Context, _ string) error { return nil } diff --git a/pkg/agent/delivery/legacy_shell.go b/pkg/agent/delivery/legacy_shell.go new file mode 100644 index 000000000..5b74eece0 --- /dev/null +++ b/pkg/agent/delivery/legacy_shell.go @@ -0,0 +1,77 @@ +package delivery + +import ( + "context" + "fmt" + "io" + + "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/inject" + "github.com/devsy-org/devsy/pkg/log" +) + +type LegacyShellDelivery struct { + ExecFunc inject.ExecFunc + DownloadURL string + Timeout func() *agent.InjectOptions +} + +func (d *LegacyShellDelivery) Phase() DeliveryPhase { + return PhasePostStart +} + +func (d *LegacyShellDelivery) DeliverPreStart(_ context.Context, _ PreStartOptions) error { + return fmt.Errorf("LegacyShellDelivery does not support pre-start delivery") +} + +func (d *LegacyShellDelivery) DeliverPostStart(ctx context.Context, opts PostStartOptions) error { + if d.ExecFunc == nil { + return fmt.Errorf("exec function is required for legacy shell delivery") + } + + injectOpts := &agent.InjectOptions{ + Ctx: ctx, + Exec: d.ExecFunc, + IsLocal: false, + RemoteAgentPath: agent.ContainerDevsyHelperLocation, + DownloadURL: d.downloadURL(), + PreferDownloadFromRemoteUrl: agent.Bool(false), + } + + if d.Timeout != nil { + overrides := d.Timeout() + if overrides != nil && overrides.Timeout > 0 { + injectOpts.Timeout = overrides.Timeout + } + } + + if err := agent.InjectAgent(injectOpts); err != nil { + return fmt.Errorf("legacy shell inject: %w", err) + } + + log.Debugf("delivered agent binary via legacy shell injection") + return nil +} + +func (d *LegacyShellDelivery) Cleanup(_ context.Context, _ string) error { + return nil +} + +func (d *LegacyShellDelivery) downloadURL() string { + if d.DownloadURL != "" { + return d.DownloadURL + } + return agent.DefaultAgentDownloadURL() +} + +// ExecFuncFromDriver creates an inject.ExecFunc that routes commands through +// the provided driver command function. This adapts the driver's +// CommandDevContainer signature for use with the legacy injection path. +func ExecFuncFromDriver( + cmdFn func(ctx context.Context, user, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error, + user string, +) inject.ExecFunc { + return func(ctx context.Context, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { + return cmdFn(ctx, user, command, stdin, stdout, stderr) + } +} diff --git a/pkg/agent/delivery/legacy_shell_test.go b/pkg/agent/delivery/legacy_shell_test.go new file mode 100644 index 000000000..e798a1b42 --- /dev/null +++ b/pkg/agent/delivery/legacy_shell_test.go @@ -0,0 +1,62 @@ +package delivery + +import ( + "context" + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLegacyShellDelivery_Phase(t *testing.T) { + d := &LegacyShellDelivery{} + assert.Equal(t, PhasePostStart, d.Phase()) +} + +func TestLegacyShellDelivery_DeliverPreStart_ReturnsError(t *testing.T) { + d := &LegacyShellDelivery{} + err := d.DeliverPreStart(context.Background(), PreStartOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "does not support pre-start") +} + +func TestLegacyShellDelivery_DeliverPostStart_RequiresExecFunc(t *testing.T) { + d := &LegacyShellDelivery{} + err := d.DeliverPostStart(context.Background(), PostStartOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "exec function is required") +} + +func TestLegacyShellDelivery_Cleanup_IsNoOp(t *testing.T) { + d := &LegacyShellDelivery{} + err := d.Cleanup(context.Background(), "workspace-123") + assert.NoError(t, err) +} + +func TestLegacyShellDelivery_DownloadURL_Default(t *testing.T) { + d := &LegacyShellDelivery{} + url := d.downloadURL() + assert.NotEmpty(t, url) + assert.Contains(t, url, "github.com") +} + +func TestLegacyShellDelivery_DownloadURL_Custom(t *testing.T) { + d := &LegacyShellDelivery{DownloadURL: "https://custom.example.com/agent"} + assert.Equal(t, "https://custom.example.com/agent", d.downloadURL()) +} + +func TestExecFuncFromDriver(t *testing.T) { + var capturedUser, capturedCmd string + cmdFn := func(ctx context.Context, user, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { + capturedUser = user + capturedCmd = command + return nil + } + + execFn := ExecFuncFromDriver(cmdFn, "root") + err := execFn(context.Background(), "echo hello", nil, nil, nil) + require.NoError(t, err) + assert.Equal(t, "root", capturedUser) + assert.Equal(t, "echo hello", capturedCmd) +} diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go new file mode 100644 index 000000000..025d4d4ae --- /dev/null +++ b/pkg/agent/delivery/local_docker.go @@ -0,0 +1,135 @@ +package delivery + +import ( + "context" + "fmt" + "os" + "os/exec" + + "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/log" +) + +const ( + defaultDockerCmd = "docker" + volumePrefix = "devsy-agent-" + volumeMountPath = "/opt/devsy" + helperImage = "busybox:latest" +) + +type LocalDockerDelivery struct { + DockerCommand string + Environment []string +} + +func (d *LocalDockerDelivery) Phase() DeliveryPhase { + return PhasePreStart +} + +func (d *LocalDockerDelivery) DeliverPreStart(ctx context.Context, opts PreStartOptions) error { + volumeName := volumePrefix + opts.WorkspaceID + + if err := d.createVolume(ctx, volumeName); err != nil { + return fmt.Errorf("create agent volume: %w", err) + } + + if err := d.populateVolume(ctx, volumeName, opts.BinaryPath); err != nil { + if removeErr := d.removeVolume(ctx, volumeName); removeErr != nil { + log.Debugf("failed to clean up volume after populate failure: %v", removeErr) + } + return fmt.Errorf("populate agent volume: %w", err) + } + + opts.RunOptions.Mounts = append(opts.RunOptions.Mounts, &config.Mount{ + Type: "volume", + Source: volumeName, + Target: volumeMountPath, + }) + + if opts.RunOptions.Env == nil { + opts.RunOptions.Env = make(map[string]string) + } + opts.RunOptions.Env["DEVSY_AGENT_PATH"] = volumeMountPath + "/" + binaryName() + + return nil +} + +func (d *LocalDockerDelivery) DeliverPostStart(_ context.Context, _ PostStartOptions) error { + return fmt.Errorf("LocalDockerDelivery does not support post-start delivery") +} + +func (d *LocalDockerDelivery) Cleanup(ctx context.Context, workspaceID string) error { + return d.removeVolume(ctx, workspaceID) +} + +func (d *LocalDockerDelivery) createVolume(ctx context.Context, name string) error { + out, err := d.cmd(ctx, "volume", "create", name).CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + return nil +} + +func (d *LocalDockerDelivery) populateVolume( + ctx context.Context, volumeName, binaryPath string, +) error { + // #nosec G304 -- binaryPath is controlled by the caller (agent binary location) + f, err := os.Open(binaryPath) + if err != nil { + return fmt.Errorf("open binary: %w", err) + } + defer func() { _ = f.Close() }() + + containerName := "devsy-agent-init-" + volumeName + script := fmt.Sprintf( + "cat > %s/%s && chmod 755 %s/%s", + volumeMountPath, binaryName(), volumeMountPath, binaryName(), + ) + args := []string{ + "run", "--rm", + "--name", containerName, + "-v", volumeName + ":" + volumeMountPath, + "-i", + helperImage, + "sh", "-c", script, + } + + cmd := d.cmd(ctx, args...) + cmd.Stdin = f + + out, err := cmd.CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + return nil +} + +func (d *LocalDockerDelivery) removeVolume(ctx context.Context, workspaceID string) error { + volumeName := volumePrefix + workspaceID + out, err := d.cmd(ctx, "volume", "rm", "-f", volumeName).CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + return nil +} + +func (d *LocalDockerDelivery) cmd(ctx context.Context, args ...string) *exec.Cmd { + // #nosec G204 -- args are constructed internally, not from user input + cmd := exec.CommandContext(ctx, d.dockerCommand(), args...) + if d.Environment != nil { + cmd.Env = append(os.Environ(), d.Environment...) + } + return cmd +} + +func (d *LocalDockerDelivery) dockerCommand() string { + if d.DockerCommand != "" { + return d.DockerCommand + } + return defaultDockerCmd +} + +func binaryName() string { + return agent.ContainerDevsyHelperLocation[len("/usr/local/bin/"):] +} diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go new file mode 100644 index 000000000..ae01ef24e --- /dev/null +++ b/pkg/agent/delivery/local_docker_test.go @@ -0,0 +1,77 @@ +package delivery + +import ( + "context" + "testing" + + "github.com/devsy-org/devsy/pkg/devcontainer/config" + "github.com/devsy-org/devsy/pkg/driver" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestLocalDockerDelivery_Phase(t *testing.T) { + d := &LocalDockerDelivery{} + assert.Equal(t, PhasePreStart, d.Phase()) +} + +func TestLocalDockerDelivery_DeliverPostStart_ReturnsError(t *testing.T) { + d := &LocalDockerDelivery{} + err := d.DeliverPostStart(context.Background(), PostStartOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "does not support post-start") +} + +func TestLocalDockerDelivery_DeliverPreStart_MutatesRunOptions(t *testing.T) { + if testing.Short() { + t.Skip("requires docker") + } + + d := &LocalDockerDelivery{} + runOpts := &driver.RunOptions{ + Mounts: []*config.Mount{}, + Env: map[string]string{}, + } + opts := PreStartOptions{ + WorkspaceID: "test-workspace-123", + RunOptions: runOpts, + BinaryPath: "/bin/sh", + Arch: "amd64", + } + + ctx := context.Background() + err := d.DeliverPreStart(ctx, opts) + if err != nil { + t.Skipf("docker not available or failed: %v", err) + } + + assert.Len(t, runOpts.Mounts, 1) + assert.Equal(t, "devsy-agent-test-workspace-123", runOpts.Mounts[0].Source) + assert.Equal(t, volumeMountPath, runOpts.Mounts[0].Target) + assert.Equal(t, "volume", runOpts.Mounts[0].Type) + assert.Contains(t, runOpts.Env["DEVSY_AGENT_PATH"], volumeMountPath) + + // Cleanup + _ = d.Cleanup(ctx, "test-workspace-123") +} + +func TestLocalDockerDelivery_Cleanup_NonExistentVolume(t *testing.T) { + if testing.Short() { + t.Skip("requires docker") + } + + d := &LocalDockerDelivery{} + err := d.Cleanup(context.Background(), "nonexistent-workspace-xyz") + assert.NoError(t, err) +} + +func TestDeliveryPhase_String(t *testing.T) { + assert.Equal(t, "pre-start", PhasePreStart.String()) + assert.Equal(t, "post-start", PhasePostStart.String()) + assert.Contains(t, DeliveryPhase(99).String(), "unknown") +} + +func TestBinaryName(t *testing.T) { + name := binaryName() + assert.Equal(t, "devsy", name) +} diff --git a/pkg/agent/delivery/remote_docker.go b/pkg/agent/delivery/remote_docker.go new file mode 100644 index 000000000..4426c4647 --- /dev/null +++ b/pkg/agent/delivery/remote_docker.go @@ -0,0 +1,86 @@ +package delivery + +import ( + "context" + "fmt" + "os" + "os/exec" + + "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/log" +) + +type RemoteDockerDelivery struct { + DockerCommand string + Environment []string + ContainerID string +} + +func (d *RemoteDockerDelivery) Phase() DeliveryPhase { + return PhasePostStart +} + +func (d *RemoteDockerDelivery) DeliverPreStart(_ context.Context, _ PreStartOptions) error { + return fmt.Errorf("RemoteDockerDelivery does not support pre-start delivery") +} + +func (d *RemoteDockerDelivery) DeliverPostStart(ctx context.Context, opts PostStartOptions) error { + if d.ContainerID == "" && opts.ContainerDetails != nil { + d.ContainerID = opts.ContainerDetails.ID + } + if d.ContainerID == "" { + return fmt.Errorf("container ID is required for remote docker delivery") + } + + destPath := agent.ContainerDevsyHelperLocation + + if err := d.copyBinary(ctx, opts.BinaryPath, destPath); err != nil { + return fmt.Errorf("copy binary to container: %w", err) + } + + if err := d.chmodBinary(ctx, destPath); err != nil { + return fmt.Errorf("chmod binary in container: %w", err) + } + + log.Debugf("delivered agent binary to remote container %s via docker cp", d.ContainerID) + return nil +} + +func (d *RemoteDockerDelivery) Cleanup(_ context.Context, _ string) error { + return nil +} + +func (d *RemoteDockerDelivery) copyBinary(ctx context.Context, srcPath, destPath string) error { + src := srcPath + dest := fmt.Sprintf("%s:%s", d.ContainerID, destPath) + + out, err := d.cmd(ctx, "cp", src, dest).CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + return nil +} + +func (d *RemoteDockerDelivery) chmodBinary(ctx context.Context, destPath string) error { + out, err := d.cmd(ctx, "exec", d.ContainerID, "chmod", "755", destPath).CombinedOutput() + if err != nil { + return fmt.Errorf("%s: %w", string(out), err) + } + return nil +} + +func (d *RemoteDockerDelivery) cmd(ctx context.Context, args ...string) *exec.Cmd { + // #nosec G204 -- args are constructed internally, not from user input + cmd := exec.CommandContext(ctx, d.dockerCommand(), args...) + if d.Environment != nil { + cmd.Env = append(os.Environ(), d.Environment...) + } + return cmd +} + +func (d *RemoteDockerDelivery) dockerCommand() string { + if d.DockerCommand != "" { + return d.DockerCommand + } + return defaultDockerCmd +} diff --git a/pkg/agent/delivery/remote_docker_test.go b/pkg/agent/delivery/remote_docker_test.go new file mode 100644 index 000000000..622a844d8 --- /dev/null +++ b/pkg/agent/delivery/remote_docker_test.go @@ -0,0 +1,46 @@ +package delivery + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRemoteDockerDelivery_Phase(t *testing.T) { + d := &RemoteDockerDelivery{} + assert.Equal(t, PhasePostStart, d.Phase()) +} + +func TestRemoteDockerDelivery_DeliverPreStart_ReturnsError(t *testing.T) { + d := &RemoteDockerDelivery{} + err := d.DeliverPreStart(context.Background(), PreStartOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "does not support pre-start") +} + +func TestRemoteDockerDelivery_DeliverPostStart_RequiresContainerID(t *testing.T) { + d := &RemoteDockerDelivery{} + err := d.DeliverPostStart(context.Background(), PostStartOptions{ + BinaryPath: "/bin/sh", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "container ID is required") +} + +func TestRemoteDockerDelivery_Cleanup_IsNoOp(t *testing.T) { + d := &RemoteDockerDelivery{} + err := d.Cleanup(context.Background(), "workspace-123") + assert.NoError(t, err) +} + +func TestRemoteDockerDelivery_DockerCommand_Default(t *testing.T) { + d := &RemoteDockerDelivery{} + assert.Equal(t, "docker", d.dockerCommand()) +} + +func TestRemoteDockerDelivery_DockerCommand_Custom(t *testing.T) { + d := &RemoteDockerDelivery{DockerCommand: "podman"} + assert.Equal(t, "podman", d.dockerCommand()) +} diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index ddfcf0ea0..4e35c012c 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -13,6 +13,7 @@ import ( "al.essio.dev/pkg/shellescape" "github.com/devsy-org/devsy/pkg/agent" + "github.com/devsy-org/devsy/pkg/agent/delivery" "github.com/devsy-org/devsy/pkg/agent/tunnelserver" "github.com/devsy-org/devsy/pkg/compress" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -66,6 +67,50 @@ func (r *runner) setupContainer( } func (r *runner) injectAgentIntoContainer(ctx context.Context, timeout time.Duration) error { + strategy := r.newAgentDelivery() + + if strategy.Phase() == delivery.PhasePostStart { + return r.deliverPostStart(ctx, strategy) + } + + return r.legacyInject(ctx, timeout) +} + +func (r *runner) newAgentDelivery() delivery.AgentDelivery { + dockerCmd := "docker" + var dockerEnv []string + if r.WorkspaceConfig.Agent.Docker.Path != "" { + dockerCmd = r.WorkspaceConfig.Agent.Docker.Path + } + for k, v := range r.WorkspaceConfig.Agent.Docker.Env { + dockerEnv = append(dockerEnv, k+"="+v) + } + + execFn := delivery.CommandFunc(r.Driver.CommandDevContainer, r.ID) + + return delivery.NewAgentDelivery(delivery.FactoryOptions{ + WorkspaceConfig: r.WorkspaceConfig, + WorkspaceID: r.ID, + DockerCommand: dockerCmd, + DockerEnv: dockerEnv, + ContainerID: r.ID, + ExecFunc: execFn, + }) +} + +func (r *runner) deliverPostStart(ctx context.Context, strategy delivery.AgentDelivery) error { + err := strategy.DeliverPostStart(ctx, delivery.PostStartOptions{ + WorkspaceID: r.ID, + BinaryPath: r.AgentPath, + Arch: runtime.GOARCH, + }) + if err != nil { + return fmt.Errorf("deliver agent (post-start): %w", err) + } + return nil +} + +func (r *runner) legacyInject(ctx context.Context, timeout time.Duration) error { err := agent.InjectAgent(&agent.InjectOptions{ Ctx: ctx, Exec: func(ctx context.Context, command string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error { diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index 8313585ac..6c143b004 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -5,9 +5,11 @@ import ( "encoding/json" "fmt" "maps" + "runtime" "strings" "time" + "github.com/devsy-org/devsy/pkg/agent/delivery" "github.com/devsy-org/devsy/pkg/command" pkgconfig "github.com/devsy-org/devsy/pkg/config" "github.com/devsy-org/devsy/pkg/daemon/agent" @@ -265,6 +267,13 @@ func (r *runner) resolveNewContainer( r.injectDaemonEntrypoint(p, mergedConfig) + runOptions, err := r.buildRunOptionsForDelivery(mergedConfig, p.substitutionContext, buildInfo) + if err == nil { + if preStartErr := r.deliverPreStart(ctx, runOptions); preStartErr != nil { + log.Debugf("pre-start delivery skipped or failed, will use post-start: %v", preStartErr) + } + } + err = r.runContainer(ctx, p.parsedConfig, p.substitutionContext, mergedConfig, buildInfo) if err != nil { return nil, fmt.Errorf("runner run container: %w", err) @@ -340,6 +349,31 @@ func (r *runner) findRunningContainerOrFail( return details, nil } +func (r *runner) buildRunOptionsForDelivery( + mergedConfig *config.MergedDevContainerConfig, + substitutionContext *config.SubstitutionContext, + buildInfo *config.BuildInfo, +) (*driver.RunOptions, error) { + if buildInfo.Dockerless != nil { + return r.getDockerlessRunOptions(mergedConfig, substitutionContext, buildInfo) + } + return r.getRunOptions(mergedConfig, substitutionContext, buildInfo) +} + +func (r *runner) deliverPreStart(ctx context.Context, runOptions *driver.RunOptions) error { + strategy := r.newAgentDelivery() + if strategy.Phase() != delivery.PhasePreStart { + return fmt.Errorf("strategy phase is %s, not pre-start", strategy.Phase()) + } + + return strategy.DeliverPreStart(ctx, delivery.PreStartOptions{ + WorkspaceID: r.ID, + RunOptions: runOptions, + BinaryPath: r.AgentPath, + Arch: runtime.GOARCH, + }) +} + func (r *runner) runContainer( ctx context.Context, parsedConfig *config.SubstitutedConfig, From 77671068f4812b3a4a580b9320abd2e6975920d6 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:38:59 -0500 Subject: [PATCH 2/6] 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 128075dbb67b6685e13d3d76dc998c1bca6432be Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 02:44:22 -0500 Subject: [PATCH 3/6] 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 04856a3facc6e0c63a05d08447f4d61a77455c0a Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 03:04:28 -0500 Subject: [PATCH 4/6] fix(test): remove Docker-dependent unit tests that fail in CI Move Docker-requiring tests to integration build tag only. Unit tests now run without Docker available. --- pkg/agent/delivery/local_docker_test.go | 45 ------------------------- 1 file changed, 45 deletions(-) diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index ae01ef24e..bcd2a2c7c 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -4,8 +4,6 @@ import ( "context" "testing" - "github.com/devsy-org/devsy/pkg/devcontainer/config" - "github.com/devsy-org/devsy/pkg/driver" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -22,49 +20,6 @@ func TestLocalDockerDelivery_DeliverPostStart_ReturnsError(t *testing.T) { assert.Contains(t, err.Error(), "does not support post-start") } -func TestLocalDockerDelivery_DeliverPreStart_MutatesRunOptions(t *testing.T) { - if testing.Short() { - t.Skip("requires docker") - } - - d := &LocalDockerDelivery{} - runOpts := &driver.RunOptions{ - Mounts: []*config.Mount{}, - Env: map[string]string{}, - } - opts := PreStartOptions{ - WorkspaceID: "test-workspace-123", - RunOptions: runOpts, - BinaryPath: "/bin/sh", - Arch: "amd64", - } - - ctx := context.Background() - err := d.DeliverPreStart(ctx, opts) - if err != nil { - t.Skipf("docker not available or failed: %v", err) - } - - assert.Len(t, runOpts.Mounts, 1) - assert.Equal(t, "devsy-agent-test-workspace-123", runOpts.Mounts[0].Source) - assert.Equal(t, volumeMountPath, runOpts.Mounts[0].Target) - assert.Equal(t, "volume", runOpts.Mounts[0].Type) - assert.Contains(t, runOpts.Env["DEVSY_AGENT_PATH"], volumeMountPath) - - // Cleanup - _ = d.Cleanup(ctx, "test-workspace-123") -} - -func TestLocalDockerDelivery_Cleanup_NonExistentVolume(t *testing.T) { - if testing.Short() { - t.Skip("requires docker") - } - - d := &LocalDockerDelivery{} - err := d.Cleanup(context.Background(), "nonexistent-workspace-xyz") - assert.NoError(t, err) -} - func TestDeliveryPhase_String(t *testing.T) { assert.Equal(t, "pre-start", PhasePreStart.String()) assert.Equal(t, "post-start", PhasePostStart.String()) From aeeb2e6f83b124467d4b63f749409284c94d4753 Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 03:35:07 -0500 Subject: [PATCH 5/6] fix(delivery): treat npipe:// as local Docker, add legacy fallback Windows Podman uses npipe:// DOCKER_HOST which was incorrectly classified as remote. Also adds fallback to legacy inject when post-start delivery fails, ensuring existing paths keep working. --- pkg/agent/delivery/factory.go | 11 +++++++---- pkg/agent/delivery/factory_test.go | 1 + pkg/devcontainer/setup.go | 6 +++++- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/pkg/agent/delivery/factory.go b/pkg/agent/delivery/factory.go index a67da7122..c42bde2d4 100644 --- a/pkg/agent/delivery/factory.go +++ b/pkg/agent/delivery/factory.go @@ -70,10 +70,13 @@ func isDockerLocal(_ string) bool { } func isLocalDockerHost(host string) bool { - return host == "" || - host == "unix:///var/run/docker.sock" || - host == "npipe:////./pipe/docker_engine" || - len(host) > 7 && host[:7] == "unix://" + if host == "" { + return true + } + hasPrefix := func(s, prefix string) bool { + return len(s) >= len(prefix) && s[:len(prefix)] == prefix + } + return hasPrefix(host, "unix://") || hasPrefix(host, "npipe://") } // CommandFunc adapts a driver's command function to inject.ExecFunc. diff --git a/pkg/agent/delivery/factory_test.go b/pkg/agent/delivery/factory_test.go index a97c376f8..8f6f97fe9 100644 --- a/pkg/agent/delivery/factory_test.go +++ b/pkg/agent/delivery/factory_test.go @@ -91,6 +91,7 @@ func TestIsDockerLocal(t *testing.T) { assert.True(t, isLocalDockerHost("unix:///var/run/docker.sock")) assert.True(t, isLocalDockerHost("unix:///home/user/.docker/desktop/docker.sock")) assert.True(t, isLocalDockerHost("npipe:////./pipe/docker_engine")) + assert.True(t, isLocalDockerHost("npipe:////./pipe/podman-machine-default")) assert.False(t, isLocalDockerHost("tcp://192.168.1.100:2376")) assert.False(t, isLocalDockerHost("ssh://user@remote-host")) } diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index 4e35c012c..2f6acee2d 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -70,7 +70,11 @@ func (r *runner) injectAgentIntoContainer(ctx context.Context, timeout time.Dura strategy := r.newAgentDelivery() if strategy.Phase() == delivery.PhasePostStart { - return r.deliverPostStart(ctx, strategy) + if err := r.deliverPostStart(ctx, strategy); err != nil { + log.Debugf("post-start delivery failed, falling back to legacy inject: %v", err) + return r.legacyInject(ctx, timeout) + } + return nil } return r.legacyInject(ctx, timeout) From 45270ea14e950b4bb470f6f3982df06cdffa471f Mon Sep 17 00:00:00 2001 From: Samuel K Date: Tue, 12 May 2026 04:29:29 -0500 Subject: [PATCH 6/6] fix(delivery): use BinaryManager instead of host path for binary acquisition LocalDockerDelivery and RemoteDockerDelivery were receiving the container destination path (/usr/local/bin/devsy) as BinaryPath, then trying to os.Open it on the host where it doesn't exist. This caused every delivery attempt to fail silently and fall back to the legacy inject path. Replace BinaryPath with a BinarySourceFunc that uses agent.BinaryManager to acquire the binary via the existing source chain (local executable, file cache, HTTP download). This matches how the legacy inject path already acquires the binary. --- pkg/agent/delivery/delivery.go | 13 ++++--- .../delivery/delivery_integration_test.go | 20 ++++++---- pkg/agent/delivery/local_docker.go | 22 +++++++---- pkg/agent/delivery/local_docker_test.go | 7 ++++ pkg/agent/delivery/remote_docker.go | 39 ++++++++++++++++--- pkg/agent/delivery/remote_docker_test.go | 15 ++++++- pkg/devcontainer/setup.go | 25 ++++++++++-- pkg/devcontainer/single.go | 13 +++++-- 8 files changed, 121 insertions(+), 33 deletions(-) diff --git a/pkg/agent/delivery/delivery.go b/pkg/agent/delivery/delivery.go index ee9dfd5d6..0c3e1968b 100644 --- a/pkg/agent/delivery/delivery.go +++ b/pkg/agent/delivery/delivery.go @@ -3,6 +3,7 @@ package delivery import ( "context" "fmt" + "io" "github.com/devsy-org/devsy/pkg/devcontainer/config" "github.com/devsy-org/devsy/pkg/driver" @@ -26,17 +27,19 @@ func (p DeliveryPhase) String() string { } } +type BinarySourceFunc func(ctx context.Context, arch string) (io.ReadCloser, error) + type PreStartOptions struct { - WorkspaceID string - RunOptions *driver.RunOptions - BinaryPath string - Arch string + WorkspaceID string + RunOptions *driver.RunOptions + BinarySource BinarySourceFunc + Arch string } type PostStartOptions struct { WorkspaceID string ContainerDetails *config.ContainerDetails - BinaryPath string + BinarySource BinarySourceFunc Arch string } diff --git a/pkg/agent/delivery/delivery_integration_test.go b/pkg/agent/delivery/delivery_integration_test.go index 6ed08aa48..a0c883796 100644 --- a/pkg/agent/delivery/delivery_integration_test.go +++ b/pkg/agent/delivery/delivery_integration_test.go @@ -4,7 +4,9 @@ package delivery import ( "context" + "io" "os/exec" + "strings" "testing" "github.com/devsy-org/devsy/pkg/devcontainer/config" @@ -13,6 +15,10 @@ import ( "github.com/stretchr/testify/require" ) +func testBinarySource(_ context.Context, _ string) (io.ReadCloser, error) { + return io.NopCloser(strings.NewReader("#!/bin/sh\necho hello\n")), nil +} + func dockerAvailable() bool { cmd := exec.Command("docker", "info") return cmd.Run() == nil @@ -32,10 +38,10 @@ func TestLocalDockerDelivery_Integration(t *testing.T) { Env: map[string]string{}, } opts := PreStartOptions{ - WorkspaceID: workspaceID, - RunOptions: runOpts, - BinaryPath: "/bin/sh", - Arch: "amd64", + WorkspaceID: workspaceID, + RunOptions: runOpts, + BinarySource: testBinarySource, + Arch: "amd64", } err := d.DeliverPreStart(ctx, opts) @@ -93,9 +99,9 @@ func TestRemoteDockerDelivery_Integration(t *testing.T) { } err = d.DeliverPostStart(ctx, PostStartOptions{ - WorkspaceID: "test-workspace", - BinaryPath: "/bin/sh", - Arch: "amd64", + WorkspaceID: "test-workspace", + BinarySource: testBinarySource, + Arch: "amd64", }) require.NoError(t, err) diff --git a/pkg/agent/delivery/local_docker.go b/pkg/agent/delivery/local_docker.go index 025d4d4ae..90caf203d 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -11,6 +11,8 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) +var _ AgentDelivery = (*LocalDockerDelivery)(nil) + const ( defaultDockerCmd = "docker" volumePrefix = "devsy-agent-" @@ -28,13 +30,17 @@ func (d *LocalDockerDelivery) Phase() DeliveryPhase { } func (d *LocalDockerDelivery) DeliverPreStart(ctx context.Context, opts PreStartOptions) error { + if opts.BinarySource == nil { + return fmt.Errorf("binary source is required for local docker delivery") + } + volumeName := volumePrefix + opts.WorkspaceID if err := d.createVolume(ctx, volumeName); err != nil { return fmt.Errorf("create agent volume: %w", err) } - if err := d.populateVolume(ctx, volumeName, opts.BinaryPath); err != nil { + if err := d.populateVolume(ctx, volumeName, opts.BinarySource, opts.Arch); err != nil { if removeErr := d.removeVolume(ctx, volumeName); removeErr != nil { log.Debugf("failed to clean up volume after populate failure: %v", removeErr) } @@ -72,14 +78,16 @@ func (d *LocalDockerDelivery) createVolume(ctx context.Context, name string) err } func (d *LocalDockerDelivery) populateVolume( - ctx context.Context, volumeName, binaryPath string, + ctx context.Context, + volumeName string, + binarySource BinarySourceFunc, + arch string, ) error { - // #nosec G304 -- binaryPath is controlled by the caller (agent binary location) - f, err := os.Open(binaryPath) + binary, err := binarySource(ctx, arch) if err != nil { - return fmt.Errorf("open binary: %w", err) + return fmt.Errorf("acquire binary: %w", err) } - defer func() { _ = f.Close() }() + defer func() { _ = binary.Close() }() containerName := "devsy-agent-init-" + volumeName script := fmt.Sprintf( @@ -96,7 +104,7 @@ func (d *LocalDockerDelivery) populateVolume( } cmd := d.cmd(ctx, args...) - cmd.Stdin = f + cmd.Stdin = binary out, err := cmd.CombinedOutput() if err != nil { diff --git a/pkg/agent/delivery/local_docker_test.go b/pkg/agent/delivery/local_docker_test.go index bcd2a2c7c..0f94b4f19 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -13,6 +13,13 @@ func TestLocalDockerDelivery_Phase(t *testing.T) { assert.Equal(t, PhasePreStart, d.Phase()) } +func TestLocalDockerDelivery_DeliverPreStart_RequiresBinarySource(t *testing.T) { + d := &LocalDockerDelivery{} + err := d.DeliverPreStart(context.Background(), PreStartOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "binary source is required") +} + func TestLocalDockerDelivery_DeliverPostStart_ReturnsError(t *testing.T) { d := &LocalDockerDelivery{} err := d.DeliverPostStart(context.Background(), PostStartOptions{}) diff --git a/pkg/agent/delivery/remote_docker.go b/pkg/agent/delivery/remote_docker.go index 4426c4647..cb309622c 100644 --- a/pkg/agent/delivery/remote_docker.go +++ b/pkg/agent/delivery/remote_docker.go @@ -3,6 +3,7 @@ package delivery import ( "context" "fmt" + "io" "os" "os/exec" @@ -10,6 +11,8 @@ import ( "github.com/devsy-org/devsy/pkg/log" ) +var _ AgentDelivery = (*RemoteDockerDelivery)(nil) + type RemoteDockerDelivery struct { DockerCommand string Environment []string @@ -25,6 +28,9 @@ func (d *RemoteDockerDelivery) DeliverPreStart(_ context.Context, _ PreStartOpti } func (d *RemoteDockerDelivery) DeliverPostStart(ctx context.Context, opts PostStartOptions) error { + if opts.BinarySource == nil { + return fmt.Errorf("binary source is required for remote docker delivery") + } if d.ContainerID == "" && opts.ContainerDetails != nil { d.ContainerID = opts.ContainerDetails.ID } @@ -34,7 +40,7 @@ func (d *RemoteDockerDelivery) DeliverPostStart(ctx context.Context, opts PostSt destPath := agent.ContainerDevsyHelperLocation - if err := d.copyBinary(ctx, opts.BinaryPath, destPath); err != nil { + if err := d.copyBinaryFromSource(ctx, opts.BinarySource, opts.Arch, destPath); err != nil { return fmt.Errorf("copy binary to container: %w", err) } @@ -50,11 +56,34 @@ func (d *RemoteDockerDelivery) Cleanup(_ context.Context, _ string) error { return nil } -func (d *RemoteDockerDelivery) copyBinary(ctx context.Context, srcPath, destPath string) error { - src := srcPath - dest := fmt.Sprintf("%s:%s", d.ContainerID, destPath) +func (d *RemoteDockerDelivery) copyBinaryFromSource( + ctx context.Context, + binarySource BinarySourceFunc, + arch, destPath string, +) error { + binary, err := binarySource(ctx, arch) + if err != nil { + return fmt.Errorf("acquire binary: %w", err) + } + defer func() { _ = binary.Close() }() + + tmpFile, err := os.CreateTemp("", "devsy-agent-*") + if err != nil { + return fmt.Errorf("create temp file: %w", err) + } + tmpPath := tmpFile.Name() + defer func() { _ = os.Remove(tmpPath) }() + + if _, err := io.Copy(tmpFile, binary); err != nil { + _ = tmpFile.Close() + return fmt.Errorf("write temp binary: %w", err) + } + if err := tmpFile.Close(); err != nil { + return fmt.Errorf("close temp file: %w", err) + } - out, err := d.cmd(ctx, "cp", src, dest).CombinedOutput() + dest := fmt.Sprintf("%s:%s", d.ContainerID, destPath) + out, err := d.cmd(ctx, "cp", tmpPath, dest).CombinedOutput() if err != nil { return fmt.Errorf("%s: %w", string(out), err) } diff --git a/pkg/agent/delivery/remote_docker_test.go b/pkg/agent/delivery/remote_docker_test.go index 622a844d8..84491bb64 100644 --- a/pkg/agent/delivery/remote_docker_test.go +++ b/pkg/agent/delivery/remote_docker_test.go @@ -2,12 +2,18 @@ package delivery import ( "context" + "io" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +var fakeBinarySource BinarySourceFunc = func(_ context.Context, _ string) (io.ReadCloser, error) { + return io.NopCloser(strings.NewReader("fake-binary")), nil +} + func TestRemoteDockerDelivery_Phase(t *testing.T) { d := &RemoteDockerDelivery{} assert.Equal(t, PhasePostStart, d.Phase()) @@ -20,10 +26,17 @@ func TestRemoteDockerDelivery_DeliverPreStart_ReturnsError(t *testing.T) { assert.Contains(t, err.Error(), "does not support pre-start") } +func TestRemoteDockerDelivery_DeliverPostStart_RequiresBinarySource(t *testing.T) { + d := &RemoteDockerDelivery{ContainerID: "test"} + err := d.DeliverPostStart(context.Background(), PostStartOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "binary source is required") +} + func TestRemoteDockerDelivery_DeliverPostStart_RequiresContainerID(t *testing.T) { d := &RemoteDockerDelivery{} err := d.DeliverPostStart(context.Background(), PostStartOptions{ - BinaryPath: "/bin/sh", + BinarySource: fakeBinarySource, }) require.Error(t, err) assert.Contains(t, err.Error(), "container ID is required") diff --git a/pkg/devcontainer/setup.go b/pkg/devcontainer/setup.go index 2f6acee2d..0a3ef8ae4 100644 --- a/pkg/devcontainer/setup.go +++ b/pkg/devcontainer/setup.go @@ -103,10 +103,15 @@ func (r *runner) newAgentDelivery() delivery.AgentDelivery { } func (r *runner) deliverPostStart(ctx context.Context, strategy delivery.AgentDelivery) error { - err := strategy.DeliverPostStart(ctx, delivery.PostStartOptions{ - WorkspaceID: r.ID, - BinaryPath: r.AgentPath, - Arch: runtime.GOARCH, + binarySource, err := r.newBinarySource() + if err != nil { + return fmt.Errorf("create binary source: %w", err) + } + + err = strategy.DeliverPostStart(ctx, delivery.PostStartOptions{ + WorkspaceID: r.ID, + BinarySource: binarySource, + Arch: runtime.GOARCH, }) if err != nil { return fmt.Errorf("deliver agent (post-start): %w", err) @@ -114,6 +119,18 @@ func (r *runner) deliverPostStart(ctx context.Context, strategy delivery.AgentDe return nil } +func (r *runner) newBinarySource() (delivery.BinarySourceFunc, error) { + downloadURL := r.AgentDownloadURL + if downloadURL == "" { + downloadURL = agent.DefaultAgentDownloadURL() + } + mgr, err := agent.NewBinaryManager(downloadURL) + if err != nil { + return nil, err + } + return mgr.AcquireBinary, nil +} + func (r *runner) legacyInject(ctx context.Context, timeout time.Duration) error { err := agent.InjectAgent(&agent.InjectOptions{ Ctx: ctx, diff --git a/pkg/devcontainer/single.go b/pkg/devcontainer/single.go index 6c143b004..884104803 100644 --- a/pkg/devcontainer/single.go +++ b/pkg/devcontainer/single.go @@ -366,11 +366,16 @@ func (r *runner) deliverPreStart(ctx context.Context, runOptions *driver.RunOpti return fmt.Errorf("strategy phase is %s, not pre-start", strategy.Phase()) } + binarySource, err := r.newBinarySource() + if err != nil { + return fmt.Errorf("create binary source: %w", err) + } + return strategy.DeliverPreStart(ctx, delivery.PreStartOptions{ - WorkspaceID: r.ID, - RunOptions: runOptions, - BinaryPath: r.AgentPath, - Arch: runtime.GOARCH, + WorkspaceID: r.ID, + RunOptions: runOptions, + BinarySource: binarySource, + Arch: runtime.GOARCH, }) }