Update Validator for /usr/bin symlink#2464
Conversation
JunAr7112
requested review from
cdesiniotis,
karthikvetrivel,
rahulait,
rajathagasthya,
shivamerla and
tariq1890
as code owners
JunAr7112
force-pushed
the
fix_symlink
branch
2 times, most recently
from
a345546 to
037a688
Compare
| } | ||
| fileInfo, err := os.Lstat("/host/usr/bin/nvidia-smi") | ||
|
|
||
| nvidiaSMIPath := "/host/usr/bin/nvidia-smi" |
There was a problem hiding this comment.
Can we move this newly added logic to a new function which correctly resolves the nvidia-smi path? Then, we can easily write few unit-tests for it as well.
There was a problem hiding this comment.
+1. Let's please write unit tests in this PR.
|
Overall, LGTM. Just a minor suggestion to have a separate function for it and also add unit-tests for it. |
cdesiniotis
left a comment
cdesiniotis
left a comment
There was a problem hiding this comment.
I would recommend we use existing packages for symlink resolution within a root. For example, we could use moby's FollowSymlinkInScope or https://pkg.go.dev/github.com/cyphar/filepath-securejoin. The former will probably suffice since the host path to nvidia-smi is assumed to be trustworthy in this context.
| } | ||
| fileInfo, err := os.Lstat("/host/usr/bin/nvidia-smi") | ||
|
|
||
| nvidiaSMIPath := "/host/usr/bin/nvidia-smi" |
There was a problem hiding this comment.
+1. Let's please write unit tests in this PR.
| if filepath.IsAbs(target) { | ||
| nvidiaSMIPath = filepath.Join("/host", strings.TrimPrefix(target, "/"), "nvidia-smi") | ||
| } else { | ||
| nvidiaSMIPath = filepath.Join("/host/usr", target, "nvidia-smi") |
There was a problem hiding this comment.
I am not following this logic. For example, what happens if /host/usr/bin/nvidia-smi is a symlink to ../../nvidia-smi?
There was a problem hiding this comment.
I can try using symlinkinScope here. wrt the current format, if /usr/bin points to an absolute host path, it prefixes /host; if it is relative, it resolves it under /host/usr
Signed-off-by: Arjun <agadiyar@nvidia.com>
3 participants
Description
This MR is in response to this issue: #1357. In certain scenarios, we might have a symlink with /usr/bin which causes os.Lstat() to be unable to properly parse the nvidia-smi location. Inside the validator container, the host root filesystem is mounted at /host. So the validator tries to resolve: /host/usr/bin/nvidia-smi
But because /host/usr/bin points to an absolute symlink target, /run/.../bin, the path resolution happens from the validator container’s root, not from /host. In other words, it looks for: /run/.../bin/nvidia-smi inside the container namespace, instead of /host/run/.../bin/nvidia-smi. So the validator falsely concludes that the host-installed driver is not present.
This change ensures that the /usr/bin symlink will be parsed correctly.