Validate volume mount source paths before detached process#4359
Validate volume mount source paths before detached process#4359mvanhorn wants to merge 2 commits intostacklok:mainfrom
Conversation
When running with --volume, validate that the source path exists on the host filesystem before spawning the detached process. Previously, path errors were silently buried in proxy log files. The validation: - Uses mount.IsResourceURI() to skip resource URIs - Distinguishes "not exists" from "permission denied" via os.IsNotExist - Only runs in CLI context (skipped for Kubernetes operator) - Resolves relative paths to absolute before checking Updates tests to use real temp directories and adds a test case for the non-existent path error. Fixes stacklok#2485
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4359 +/- ##
==========================================
- Coverage 68.35% 68.14% -0.21%
==========================================
Files 479 479
Lines 48736 48769 +33
==========================================
- Hits 33315 33236 -79
- Misses 12441 12466 +25
- Partials 2980 3067 +87 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Review from Claude Code — 2 inline comments on the new validation logic and test coverage.
pkg/runner/config_builder.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("failed to resolve relative path %s: %w", source, err) | ||
| } | ||
| absSource = filepath.Join(cwd, source) |
There was a problem hiding this comment.
Suggestion: Consider using filepath.Abs(source) instead of the manual os.Getwd() + filepath.Join() approach. filepath.Abs does the same thing internally but also calls filepath.Clean, normalizing .. segments in the resolved path. This would also be consistent with the existing convertRelativePathToAbsolute in pkg/container/docker/client.go:807 which already uses filepath.Abs.
| absSource = filepath.Join(cwd, source) | |
| absSource, err := filepath.Abs(source) | |
| if err != nil { | |
| return fmt.Errorf("failed to resolve volume source path %s: %w", source, err) | |
| } |
| builderOptions: []RunConfigBuilderOption{ | ||
| WithVolumes([]string{"/nonexistent/path:/container"}), | ||
| WithPermissionProfile(permissions.BuiltinNoneProfile()), | ||
| }, |
There was a problem hiding this comment.
Suggestion: Consider adding test cases for the two skip conditions in the new validation:
- Operator context:
NewOperatorRunConfigBuilderwithWithVolumes([]string{"/nonexistent/path:/container"})should succeed — operator paths reference K8s node paths, not the operator pod's filesystem. - Resource URI: A CLI-context build with
WithVolumes([]string{"volume://my-volume:/container/data"})should succeed — resource URIs have no host path to stat.
Both guards are new code and currently have no test coverage.
Use filepath.Abs instead of manual Getwd+Join for resolving relative paths. Add test verifying operator context skips host path validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed both suggestions in 53298a0:
|
2 participants
Summary
Volume mount errors were silently buried in proxy log files when the source path didn't exist. Users saw a success message but the server immediately entered error state. This adds pre-flight validation in
processVolumeMounts()so path errors surface immediately in the terminal.This picks up where #3932 left off, addressing the two must-fix items from @JAORMX's review:
mount.IsResourceURI()instead ofstrings.HasPrefixfor resource URI detectionos.IsNotExist(err)to distinguish "not exists" from "permission denied"Fixes #2485
Type of change
Test plan
go build ./...- passesgo test ./pkg/runner/... -count=1- all tests passmount.IsResourceURI())BuildContextCLIcheck)Changes
pkg/runner/config_builder.gomount.Parse(), before duplicate checkpkg/runner/config_builder_test.gopkg/runner/config_test.goGenerated with Claude Code