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/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.go b/pkg/agent/delivery/local_docker.go index 90caf203d..e236b78b8 100644 --- a/pkg/agent/delivery/local_docker.go +++ b/pkg/agent/delivery/local_docker.go @@ -1,10 +1,14 @@ package delivery import ( + "bytes" "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 +18,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 +82,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 +101,25 @@ func (d *LocalDockerDelivery) populateVolume( } defer func() { _ = binary.Close() }() + 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, data) +} + +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 +130,7 @@ func (d *LocalDockerDelivery) populateVolume( "--name", containerName, "-v", volumeName + ":" + volumeMountPath, "-i", - helperImage, + d.helperImageName(), "sh", "-c", script, } @@ -113,6 +144,44 @@ func (d *LocalDockerDelivery) populateVolume( return nil } +func (d *LocalDockerDelivery) populateVolumeDirectCopy( + ctx context.Context, + volumeName string, + data []byte, +) error { + mountpoint, err := d.volumeMountpoint(ctx, volumeName) + if err != nil { + return fmt.Errorf("inspect volume mountpoint: %w", err) + } + + destPath := filepath.Join(mountpoint, binaryName()) + + 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..6d9861707 100644 --- a/pkg/agent/delivery/local_docker_test.go +++ b/pkg/agent/delivery/local_docker_test.go @@ -1,9 +1,14 @@ package delivery import ( + "bytes" "context" + "io" + "os" + "path/filepath" "testing" + "github.com/devsy-org/devsy/pkg/provider" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -37,3 +42,85 @@ 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: defaultDockerCmd, + 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: defaultDockerCmd, + } + + d := NewAgentDelivery(opts) + local, ok := d.(*LocalDockerDelivery) + require.True(t, ok) + 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/devcontainer/delete.go b/pkg/devcontainer/delete.go index cb3d95122..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 } @@ -40,6 +42,13 @@ func (r *runner) Delete(ctx context.Context, options DeleteOptions) error { 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..01387bd7c --- /dev/null +++ b/pkg/devcontainer/delete_test.go @@ -0,0 +1,179 @@ +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" +) + +const ( + testContainerID = "container-abc" + testStatusRunning = "running" +) + +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: testContainerID, + State: config.ContainerDetailsState{Status: testStatusRunning}, + 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: testContainerID, + 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: testContainerID, + 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()) +} 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..845333bae 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. A direct-copy fallback is used if the helper container approach fails. + HelperImage string `json:"helperImage,omitempty"` } type ProviderKubernetesDriverConfig struct {