-
Notifications
You must be signed in to change notification settings - Fork 2
feat(delivery): add KubernetesDelivery strategy for K8s agent binary injection #267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
5fd2b88
feat(agent): implement AgentDelivery interface for container binary d…
skevetter c39204c
ci: add delivery label to PR integration test matrix
skevetter 7bdd166
refactor(e2e): remove redundant delivery E2E tests
skevetter eaed848
fix(delivery): clean up agent delivery volumes on workspace deletion
skevetter 5f311af
fix(delivery): use defer for volume cleanup to cover nil-container path
skevetter e5258fc
fix(test): extract repeated string literals as constants to satisfy g…
skevetter ad219fd
feat(delivery): make helper image configurable with direct-copy fallback
skevetter c78cd5b
fix(delivery): buffer binary before populate attempts, add fallback test
skevetter 9133d8d
fix(test): use defaultDockerCmd constant instead of string literal in…
skevetter 8c3c30e
feat(agent): implement AgentDelivery interface for container binary d…
skevetter e0ad863
feat(delivery): add KubernetesDelivery strategy for K8s agent binary …
skevetter 3d4a495
fix(delivery): move KubernetesDriver case before IsRemoteDocker check
skevetter 238a38f
fix(delivery): use gzip compression and atomic write for K8s binary i…
skevetter 8dac681
fix(delivery): stream raw binary instead of gzip for K8s delivery
skevetter ab8a127
fix(delivery): route K8s driver to LegacyShellDelivery for reliable i…
skevetter d2d5364
fix(delivery): update e2e test to use BinarySource instead of removed…
skevetter 8c47ee9
fix(delivery): add delivery label to CI test matrix so e2e tests run
skevetter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,120 @@ | ||
| package delivery | ||
|
|
||
| import ( | ||
| "context" | ||
| "io" | ||
| "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, | ||
| BinarySource: binarySourceFromPath(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", | ||
| BinarySource: binarySourceFromPath(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 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 { | ||
| 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" | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| 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( | ||
| `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, 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 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,113 @@ | ||
| 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 | ||
| assert.Contains(t, capturedCmd, "cat >") | ||
| assert.Contains(t, capturedCmd, destPath) | ||
| assert.Contains(t, capturedCmd, "chmod 755") | ||
|
|
||
| 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) | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling in binary discovery logic.
Two issues:
Line 113:
os.Getwd()error is silently ignored. If the working directory cannot be determined,binDirwill be an empty string, andfilepath.Joinwill produce a path relative to the current directory that likely doesn't exist.Line 119: The final fallback returns
/bin/sheven though we already verified it doesn't exist in the loop at lines 106-111. This will cause the test to fail with a confusing error whenos.Open()is later called on a non-existent path.Consider either panicking with a clear message if no suitable binary is found, or checking the error from
os.Getwd()and handling it appropriately.🛡️ Proposed fix
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() + binDir, err := os.Getwd() + if err != nil { + panic("failed to get working directory: " + err.Error()) + } devsy := filepath.Join(binDir, "bin", "devsy-linux-amd64") if _, err := os.Stat(devsy); err == nil { return devsy } - return "/bin/sh" + panic("no suitable test binary found: checked /bin/sh, /bin/busybox, and bin/devsy-linux-amd64") }Also applies to: 119-119
🤖 Prompt for AI Agents