feat(delivery): add KubernetesDelivery strategy for K8s agent binary injection#267
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends the agent delivery system with Kubernetes support, configurable helper images for Docker operations, and cleanup during container deletion. It introduces ChangesAgent Delivery System
Sequence Diagram(s)sequenceDiagram
participant KubernetesDelivery
participant BinarySource
participant ExecFunc
participant Container
KubernetesDelivery->>KubernetesDelivery: validate BinarySource and ExecFunc
KubernetesDelivery->>BinarySource: acquire binary data
KubernetesDelivery->>KubernetesDelivery: construct shell script with cat, chmod, mv
KubernetesDelivery->>ExecFunc: execute script with binary stdin
ExecFunc->>Container: run shell command inside container
Container->>Container: write binary to agent.ContainerDevsyHelperLocation
flowchart TD
PopulateVolume["populateVolume reads binary into memory"]
PopulateVolume --> HelperAttempt["Try helper-container with helperImageName"]
HelperAttempt --> HelperSuccess{Helper succeeds?}
HelperSuccess -->|Yes| HelperDone["Volume populated"]
HelperSuccess -->|No| DirectCopy["Fall back to direct copy"]
DirectCopy --> GetMountpoint["volumeMountpoint from docker volume inspect"]
GetMountpoint --> WriteBinary["Write binary with restrictive perms"]
WriteBinary --> Chmod["chmod 755 for execution"]
Chmod --> DirectDone["Volume populated"]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…elivery 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.
The existing up-* tests already exercise the delivery path through setupContainer → injectAgentIntoContainer. No need for separate tests.
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().
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.
…oconst lint Resolves golangci-lint goconst violations in delete_test.go.
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.
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.
…elivery 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.
…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.
…njection 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.
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.
…njection 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.
60a9c77 to
ab8a127
Compare
✅ Deploy Preview for devsydev canceled.
|
|
If you're new to commit signing, there are different ways to set it up: Sign commits with
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
pkg/agent/delivery/kubernetes.go (2)
41-45: Verify write permissions in target directory.The script uses
mktempto create a temporary file in the same directory as the destination (/usr/local/bin/). This requires write access to that directory. If the container runs with restricted permissions or the directory is read-only, this operation will fail. Consider documenting this requirement or adding error handling guidance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/agent/delivery/kubernetes.go` around lines 41 - 45, The inline shell script constructed in kubernetes.go (variable "script" using destPath) assumes the process can create a temp file in the destination directory (e.g., /usr/local/bin), which will fail if the directory is not writable; update the logic to first verify write permission for the directory containing destPath (or attempt a safe temp-file create and detect failure) and handle the error path by either: 1) falling back to creating the temp file in a writable location (e.g., /tmp) and then attempting a move with clear error reporting, or 2) returning a clear, documented error that explains the need for write permission to destPath; add the check and improved error message around the code that builds/executes "script" and document the permission requirement for deployments that mount read-only container filesystems.
41-45: ⚡ Quick winQuote shell variable to prevent potential injection.
The destination path in the
mvcommand (line 44) should be quoted to prevent issues ifagent.ContainerDevsyHelperLocationever contains shell metacharacters or spaces. While it's likely a constant path, defensive quoting is a best practice.🛡️ Proposed fix to add quoting
script := fmt.Sprintf( - `set -e; t=$(mktemp %s.XXXXXX); cat > "$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, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/agent/delivery/kubernetes.go` around lines 41 - 45, The shell script built in kubernetes.go uses destPath unquoted, which can allow word-splitting or metacharacter injection; update the fmt.Sprintf call that builds the script (the variable `script` where `destPath` is injected) to quote the destination path in the template (wrap the %s occurrences with quotes for the mktemp pattern and the mv target, e.g. use "...t=$(mktemp \"%s\".XXXXXX)... mv \"$t\" \"%s\"..."), so both uses of `destPath` are quoted.pkg/devcontainer/delete.go (1)
17-17: ⚡ Quick winConsider deferring cleanup only when a container exists.
The cleanup is deferred immediately after
FindDevContainer, but before checking whethercontainerDetailsis nil. This means cleanup will attempt to run even when no container exists, which may be unnecessary overhead.♻️ Proposed refactor to defer cleanup only when needed
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) } - defer r.cleanupDeliveryVolume(ctx) if containerDetails == nil { return nil } + defer r.cleanupDeliveryVolume(ctx) log.Infof("deleting devcontainer: devcontainerID=%s", containerDetails.ID)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/delete.go` at line 17, The defer call to cleanupDeliveryVolume(ctx) is currently placed immediately after FindDevContainer and runs even when containerDetails is nil; change the logic so you only defer r.cleanupDeliveryVolume(ctx) after confirming a container exists (e.g., after the nil check on containerDetails in the function that calls FindDevContainer, such as DeleteDevContainer or the surrounding method) so cleanup is scheduled only when needed; locate the call to FindDevContainer and move the defer r.cleanupDeliveryVolume(ctx) to immediately after the branch that verifies containerDetails != nil (or wrap it in an if containerDetails != nil { defer r.cleanupDeliveryVolume(ctx) }).pkg/devcontainer/delete_test.go (2)
112-132: ⚡ Quick winTest name mentions cleanup but doesn't verify it.
The test function is named
TestDelete_RunningContainer_StopsDeletesAndCleansUp, but it only verifies thatStopDevContainerandDeleteDevContainerare called. It does not verify that cleanup occurs. Consider either removing "AndCleansUp" from the test name or adding assertions to verify cleanup behavior.🔧 Proposed fix to align test name with behavior
-func TestDelete_RunningContainer_StopsDeletesAndCleansUp(t *testing.T) { +func TestDelete_RunningContainer_StopsAndDeletes(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/delete_test.go` around lines 112 - 132, The test TestDelete_RunningContainer_StopsDeletesAndCleansUp claims to verify cleanup but only checks StopDevContainer and DeleteDevContainer; either rename the test to remove "AndCleansUp" or add assertions that verify cleanup occurred (e.g., on the mockDriver check a cleanupCalled boolean or similar flag, or assert the cleanup side-effect such as removal of temp resources created by Delete via the runner.Delete call). Update the test to assert mockDriver.cleanupCalled (or the appropriate cleanup indicator) after calling r.Delete, or change the test name to TestDelete_RunningContainer_StopsAndDeletes to match current behavior.
174-179: ⚖️ Poor tradeoffLimited test coverage for cleanup behavior.
The test
TestCleanupDeliveryVolume_DoesNotPaniconly verifies that the function doesn't panic, but doesn't verify that cleanup was actually attempted or that errors are handled correctly. While this is acceptable as a regression test, consider adding more comprehensive assertions if you want to verify the cleanup integration works as expected.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/devcontainer/delete_test.go` around lines 174 - 179, The test TestCleanupDeliveryVolume_DoesNotPanic should be expanded to assert that cleanup was actually attempted and that errors are handled: modify the mockDriver used by newTestRunner to record whether its cleanup method (e.g., the method on mockDriver that r.cleanupDeliveryVolume calls) was invoked and optionally make it return an error to validate error handling, then in the test call r.cleanupDeliveryVolume(context.Background()) and add assertions using t.Fatalf/t.Errorf that the mockDriver recorded the call and that any error path was exercised (for example, add a subtest where mockDriver returns an error and assert the runner handles it without panicking). Ensure you reference mockDriver, newTestRunner, and r.cleanupDeliveryVolume when locating and updating the test and mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@e2e/tests/delivery/delivery.go`:
- Line 113: The code silently ignores os.Getwd() and unconditionally returns
"/bin/sh" even when it doesn't exist; fix by checking and handling the error
returned by os.Getwd() (the binDir variable) and aborting/returning an error if
Getwd fails, then change the binary discovery logic (the loop building paths
with filepath.Join and the final fallback) to return a clear error (or panic)
when no candidate exists instead of returning "/bin/sh"; include the list of
attempted paths in the error message so callers (and the later os.Open call) see
exactly which files were tried.
In `@pkg/devcontainer/delete_test.go`:
- Line 107: The test in delete_test.go calls the helper searchString but that
helper is defined only in run_test.go (tests don't share helpers across files),
so add a local copy of the searchString helper into delete_test.go (or move its
implementation from run_test.go into delete_test.go) and update any imports if
needed; ensure the function signature and behavior match the original
searchString used in run_test.go so delete_test.go compiles and the check if
!searchString(err.Error(), "find dev container") continues to work.
---
Nitpick comments:
In `@pkg/agent/delivery/kubernetes.go`:
- Around line 41-45: The inline shell script constructed in kubernetes.go
(variable "script" using destPath) assumes the process can create a temp file in
the destination directory (e.g., /usr/local/bin), which will fail if the
directory is not writable; update the logic to first verify write permission for
the directory containing destPath (or attempt a safe temp-file create and detect
failure) and handle the error path by either: 1) falling back to creating the
temp file in a writable location (e.g., /tmp) and then attempting a move with
clear error reporting, or 2) returning a clear, documented error that explains
the need for write permission to destPath; add the check and improved error
message around the code that builds/executes "script" and document the
permission requirement for deployments that mount read-only container
filesystems.
- Around line 41-45: The shell script built in kubernetes.go uses destPath
unquoted, which can allow word-splitting or metacharacter injection; update the
fmt.Sprintf call that builds the script (the variable `script` where `destPath`
is injected) to quote the destination path in the template (wrap the %s
occurrences with quotes for the mktemp pattern and the mv target, e.g. use
"...t=$(mktemp \"%s\".XXXXXX)... mv \"$t\" \"%s\"..."), so both uses of
`destPath` are quoted.
In `@pkg/devcontainer/delete_test.go`:
- Around line 112-132: The test
TestDelete_RunningContainer_StopsDeletesAndCleansUp claims to verify cleanup but
only checks StopDevContainer and DeleteDevContainer; either rename the test to
remove "AndCleansUp" or add assertions that verify cleanup occurred (e.g., on
the mockDriver check a cleanupCalled boolean or similar flag, or assert the
cleanup side-effect such as removal of temp resources created by Delete via the
runner.Delete call). Update the test to assert mockDriver.cleanupCalled (or the
appropriate cleanup indicator) after calling r.Delete, or change the test name
to TestDelete_RunningContainer_StopsAndDeletes to match current behavior.
- Around line 174-179: The test TestCleanupDeliveryVolume_DoesNotPanic should be
expanded to assert that cleanup was actually attempted and that errors are
handled: modify the mockDriver used by newTestRunner to record whether its
cleanup method (e.g., the method on mockDriver that r.cleanupDeliveryVolume
calls) was invoked and optionally make it return an error to validate error
handling, then in the test call r.cleanupDeliveryVolume(context.Background())
and add assertions using t.Fatalf/t.Errorf that the mockDriver recorded the call
and that any error path was exercised (for example, add a subtest where
mockDriver returns an error and assert the runner handles it without panicking).
Ensure you reference mockDriver, newTestRunner, and r.cleanupDeliveryVolume when
locating and updating the test and mock.
In `@pkg/devcontainer/delete.go`:
- Line 17: The defer call to cleanupDeliveryVolume(ctx) is currently placed
immediately after FindDevContainer and runs even when containerDetails is nil;
change the logic so you only defer r.cleanupDeliveryVolume(ctx) after confirming
a container exists (e.g., after the nil check on containerDetails in the
function that calls FindDevContainer, such as DeleteDevContainer or the
surrounding method) so cleanup is scheduled only when needed; locate the call to
FindDevContainer and move the defer r.cleanupDeliveryVolume(ctx) to immediately
after the branch that verifies containerDetails != nil (or wrap it in an if
containerDetails != nil { defer r.cleanupDeliveryVolume(ctx) }).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d822249d-32fb-4159-8cf8-63d64c87f9b9
📒 Files selected for processing (12)
e2e/e2e_suite_test.goe2e/tests/delivery/delivery.gopkg/agent/delivery/factory.gopkg/agent/delivery/factory_test.gopkg/agent/delivery/kubernetes.gopkg/agent/delivery/kubernetes_test.gopkg/agent/delivery/local_docker.gopkg/agent/delivery/local_docker_test.gopkg/devcontainer/delete.gopkg/devcontainer/delete_test.gopkg/devcontainer/setup.gopkg/provider/provider.go
| } | ||
| } | ||
|
|
||
| binDir, _ := os.Getwd() |
There was a problem hiding this comment.
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
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/tests/delivery/delivery.go` at line 113, The code silently ignores
os.Getwd() and unconditionally returns "/bin/sh" even when it doesn't exist; fix
by checking and handling the error returned by os.Getwd() (the binDir variable)
and aborting/returning an error if Getwd fails, then change the binary discovery
logic (the loop building paths with filepath.Join and the final fallback) to
return a clear error (or panic) when no candidate exists instead of returning
"/bin/sh"; include the list of attempted paths in the error message so callers
(and the later os.Open call) see exactly which files were tried.
| if err == nil { | ||
| t.Fatal("expected error, got nil") | ||
| } | ||
| if !searchString(err.Error(), "find dev container") { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for searchString function definition or import
# Check if searchString is defined elsewhere in the package
rg -nP --type=go '\bfunc searchString\b' pkg/devcontainer/
# Check if there's a strings utility that should be imported
rg -nP --type=go 'func.*searchString|SearchString'Repository: devsy-org/devsy
Length of output: 309
🏁 Script executed:
cat -n pkg/devcontainer/delete_test.go | sed -n '100,120p'Repository: devsy-org/devsy
Length of output: 791
🏁 Script executed:
# Check if searchString is defined in delete_test.go
rg -n 'func searchString' pkg/devcontainer/delete_test.goRepository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Check the full imports in delete_test.go
head -30 pkg/devcontainer/delete_test.goRepository: devsy-org/devsy
Length of output: 581
🏁 Script executed:
# Get total lines in delete_test.go
wc -l pkg/devcontainer/delete_test.goRepository: devsy-org/devsy
Length of output: 95
🏁 Script executed:
# Search for all occurrences of searchString in delete_test.go
rg -n 'searchString' pkg/devcontainer/delete_test.goRepository: devsy-org/devsy
Length of output: 118
🏁 Script executed:
# Verify searchString is NOT defined anywhere in delete_test.go
rg -n 'searchString' pkg/devcontainer/delete_test.go | grep -v 'searchString('Repository: devsy-org/devsy
Length of output: 41
🏁 Script executed:
# Check if there's any struct embedding or test helper that might provide searchString
cat -n pkg/devcontainer/delete_test.go | head -80Repository: devsy-org/devsy
Length of output: 2336
🏁 Script executed:
# Verify run_test.go actually has the searchString definition
cat -n pkg/devcontainer/run_test.go | sed -n '120,135p'Repository: devsy-org/devsy
Length of output: 555
Move searchString helper function to delete_test.go or define it there.
The function searchString is called on line 107 but is only defined in run_test.go. Test files in Go cannot share helper functions, so this will fail to compile.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/devcontainer/delete_test.go` at line 107, The test in delete_test.go
calls the helper searchString but that helper is defined only in run_test.go
(tests don't share helpers across files), so add a local copy of the
searchString helper into delete_test.go (or move its implementation from
run_test.go into delete_test.go) and update any imports if needed; ensure the
function signature and behavior match the original searchString used in
run_test.go so delete_test.go compiles and the check if
!searchString(err.Error(), "find dev container") continues to work.
… 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.
a5c3c0a to
d2d5364
Compare
Summary
KubernetesDeliverystruct implementing theAgentDeliveryinterface as aPhasePostStartstrategy, streaming the agent binary into running K8s containers viacat > /usr/local/bin/devsy && chmod 755with stdin piped fromBinarySourceFuncNewAgentDeliveryforKubernetesDriver, placed before theIsRemoteDockercheck to match theCustomDriverprecedence patternSummary by CodeRabbit
Release Notes
New Features
Tests