-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
exec: allow running commands from host volume #14851
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:improvement | ||
exec: Allow running commands from mounted host volumes | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,32 +120,15 @@ func (l *LibcontainerExecutor) Launch(command *ExecCommand) (*ProcessState, erro | |
l.container = container | ||
|
||
// Look up the binary path and make it executable | ||
absPath, err := lookupTaskBin(command) | ||
|
||
taskPath, hostPath, err := lookupTaskBin(command) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
if err := makeExecutable(absPath); err != nil { | ||
if err := makeExecutable(hostPath); err != nil { | ||
return nil, err | ||
} | ||
|
||
path := absPath | ||
|
||
// Ensure that the path is contained in the chroot, and find it relative to the container | ||
rel, err := filepath.Rel(command.TaskDir, path) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to determine relative path base=%q target=%q: %v", command.TaskDir, path, err) | ||
} | ||
|
||
// Turn relative-to-chroot path into absolute path to avoid | ||
// libcontainer trying to resolve the binary using $PATH. | ||
// Do *not* use filepath.Join as it will translate ".."s returned by | ||
// filepath.Rel. Prepending "/" will cause the path to be rooted in the | ||
// chroot which is the desired behavior. | ||
path = "/" + rel | ||
|
||
combined := append([]string{path}, command.Args...) | ||
combined := append([]string{taskPath}, command.Args...) | ||
stdout, err := command.Stdout() | ||
if err != nil { | ||
return nil, err | ||
|
@@ -805,52 +788,125 @@ func cmdMounts(mounts []*drivers.MountConfig) []*lconfigs.Mount { | |
return r | ||
} | ||
|
||
// lookupTaskBin finds the file `bin` in taskDir/local, taskDir in that order, then performs | ||
// a PATH search inside taskDir. It returns an absolute path. See also executor.lookupBin | ||
func lookupTaskBin(command *ExecCommand) (string, error) { | ||
// lookupTaskBin finds the file `bin` in taskDir/local, taskDir in that order, | ||
// then performs a PATH search inside taskDir. It returns an absolute path | ||
tgross marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// inside the container that will get passed as arg[0] to the launched process, | ||
// and the absolute path to that binary as seen by the host (these will be | ||
// identical for binaries that don't come from mounts). | ||
// | ||
// See also executor.lookupBin for a version used by non-isolated drivers. | ||
func lookupTaskBin(command *ExecCommand) (string, string, error) { | ||
taskDir := command.TaskDir | ||
bin := command.Cmd | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does it make sense to search There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does sound confusing, and I spent much of yesterday puzzled over that! But it's the existing behavior (ref |
||
|
||
// Check in the local directory | ||
localDir := filepath.Join(taskDir, allocdir.TaskLocal) | ||
local := filepath.Join(localDir, bin) | ||
if _, err := os.Stat(local); err == nil { | ||
return local, nil | ||
taskPath, hostPath, err := getPathInTaskDir(command.TaskDir, localDir, bin) | ||
if err == nil { | ||
return taskPath, hostPath, nil | ||
} | ||
|
||
// Check at the root of the task's directory | ||
root := filepath.Join(taskDir, bin) | ||
if _, err := os.Stat(root); err == nil { | ||
return root, nil | ||
taskPath, hostPath, err = getPathInTaskDir(command.TaskDir, command.TaskDir, bin) | ||
if err == nil { | ||
return taskPath, hostPath, nil | ||
} | ||
|
||
// Check in our mounts | ||
for _, mount := range command.Mounts { | ||
taskPath, hostPath, err = getPathInMount(mount.HostPath, mount.TaskPath, bin) | ||
if err == nil { | ||
return taskPath, hostPath, nil | ||
} | ||
} | ||
|
||
// If there's a / in the binary's path, we can't fallback to a PATH search | ||
if strings.Contains(bin, "/") { | ||
return "", fmt.Errorf("file %s not found under path %s", bin, taskDir) | ||
return "", "", fmt.Errorf("file %s not found under path %s", bin, taskDir) | ||
} | ||
|
||
path := "/usr/local/bin:/usr/bin:/bin" | ||
// look for a file using a PATH-style lookup inside the directory | ||
// root. Similar to the stdlib's exec.LookPath except: | ||
// - uses a restricted lookup PATH rather than the agent process's PATH env var. | ||
// - does not require that the file is already executable (this will be ensured | ||
// by the caller) | ||
// - does not prevent using relative path as added to exec.LookPath in go1.19 | ||
// (this gets fixed-up in the caller) | ||
|
||
return lookPathIn(path, taskDir, bin) | ||
} | ||
// This is a fake PATH so that we're not using the agent's PATH | ||
restrictedPaths := []string{"/usr/local/bin", "/usr/bin", "/bin"} | ||
|
||
// lookPathIn looks for a file with PATH inside the directory root. Like exec.LookPath | ||
func lookPathIn(path string, root string, bin string) (string, error) { | ||
// exec.LookPath(file string) | ||
for _, dir := range filepath.SplitList(path) { | ||
if dir == "" { | ||
// match unix shell behavior, empty path element == . | ||
dir = "." | ||
} | ||
path := filepath.Join(root, dir, bin) | ||
f, err := os.Stat(path) | ||
if err != nil { | ||
continue | ||
} | ||
if m := f.Mode(); !m.IsDir() { | ||
Comment on lines
-836
to
-849
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note for reviewers: this was copied almost verbatim out of the stdlib but passed a mock PATH. But since we control the mock path there's no need to do any PATH parsing and we can just iterate over a known set of directories instead. That lets us use |
||
return path, nil | ||
for _, dir := range restrictedPaths { | ||
pathDir := filepath.Join(command.TaskDir, dir) | ||
taskPath, hostPath, err = getPathInTaskDir(command.TaskDir, pathDir, bin) | ||
if err == nil { | ||
return taskPath, hostPath, nil | ||
} | ||
} | ||
return "", fmt.Errorf("file %s not found under path %s", bin, root) | ||
|
||
return "", "", fmt.Errorf("file %s not found under path", bin) | ||
} | ||
|
||
// getPathInTaskDir searches for the binary in the task directory and nested | ||
// search directory. It returns the absolute path rooted inside the container | ||
// and the absolute path on the host. | ||
func getPathInTaskDir(taskDir, searchDir, bin string) (string, string, error) { | ||
|
||
hostPath := filepath.Join(searchDir, bin) | ||
|
||
f, err := os.Stat(hostPath) | ||
if err != nil { | ||
return "", "", err | ||
} | ||
if m := f.Mode(); m.IsDir() { | ||
return "", "", fmt.Errorf("path was directory, not file") | ||
} | ||
|
||
// Find the path relative to the task directory | ||
rel, err := filepath.Rel(taskDir, hostPath) | ||
if rel == "" || err != nil { | ||
return "", "", fmt.Errorf( | ||
"failed to determine relative path base=%q target=%q: %v", | ||
taskDir, hostPath, err) | ||
} | ||
|
||
// Turn relative-to-taskdir path into re-rooted absolute path to avoid | ||
// libcontainer trying to resolve the binary using $PATH. | ||
// Do *not* use filepath.Join as it will translate ".."s returned by | ||
// filepath.Rel. Prepending "/" will cause the path to be rooted in the | ||
// chroot which is the desired behavior. | ||
return filepath.Clean("/" + rel), hostPath, nil | ||
} | ||
|
||
// getPathInMount for the binary in the mount's host path, constructing the path | ||
// considering that the bin path is rooted in the mount's task path and not its | ||
// host path. It returns the absolute path rooted inside the container and the | ||
// absolute path on the host. | ||
func getPathInMount(mountHostPath, mountTaskPath, bin string) (string, string, error) { | ||
|
||
// Find the path relative to the mount point in the task so that we can | ||
// trim off any shared prefix when we search on the host path | ||
mountRel, err := filepath.Rel(mountTaskPath, bin) | ||
if mountRel == "" || err != nil { | ||
return "", "", fmt.Errorf("path was not relative to the mount task path") | ||
} | ||
|
||
hostPath := filepath.Join(mountHostPath, mountRel) | ||
|
||
f, err := os.Stat(hostPath) | ||
if err != nil { | ||
return "", "", err | ||
} | ||
if m := f.Mode(); m.IsDir() { | ||
return "", "", fmt.Errorf("path was directory, not file") | ||
} | ||
tgross marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Turn relative-to-taskdir path into re-rooted absolute path to avoid | ||
// libcontainer trying to resolve the binary using $PATH. | ||
// Do *not* use filepath.Join as it will translate ".."s returned by | ||
// filepath.Rel. Prepending "/" will cause the path to be rooted in the | ||
// chroot which is the desired behavior. | ||
return filepath.Clean("/" + bin), hostPath, nil | ||
} | ||
|
||
func newSetCPUSetCgroupHook(cgroupPath string) lconfigs.Hook { | ||
|
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.
Note for reviewers: this has been folded in to
lookupTaskBin
's inner functions. The alternative was to havelookupTaskBin
pass back flags to the caller about where it got the paths, and that was all pretty gross.