v2, util: Take the full binary path when starting the shimv2 process#5007
Conversation
The current code simply ignores the full binary path when starting the shimv2 process, and instead fallbacks to a binary in the path, and this is problematic (and confusing) for those using CRI-O, which has this bits vendored. The reason it's problematic with CRI-O is because the user can simply set the full binary path and, instead of having that executed, CRI-O will simply fail to create the container unless that binary is part of the path, which may not be case in a few different scenarios (testing being the most common one). Fixes: containerd#5006 Signed-off-by: Fabiano Fidêncio <[email protected]>
|
Hi @fidencio. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Build succeeded.
|
| if cmdPath == "" { | ||
| return nil, errors.Wrapf(os.ErrNotExist, "runtime %q binary not installed %q", runtime, name) | ||
| binaryPath := BinaryPath(runtime) | ||
| if _, serr := os.Stat(binaryPath); serr == nil { |
There was a problem hiding this comment.
os.Stat works for relative paths as well no? how do we test binaryPath is indeed a "full binary path"
There was a problem hiding this comment.
@dims, first of all, thanks for the very quick review.
Please, take a look at the newly introduced BinaryPath().
There I'm getting the joining the $prefix and the binary name, and returning its absolute path. This is enough to take the full path into consideration, right?
Or your consideration is that filepath.Abs() returns also a relative path and using the relative path could be problematic?
There was a problem hiding this comment.
Ah i missed that. thanks! (the logic in the BinaryPath)
|
@mxpv, may I bother you to take a look at this one? |
|
@dims, may I bother you for an "/okay-to-test"? :-) |
|
/ok-to-test |
|
@dims, @mxpv, is there something blocking / missing this PR to be merged? Please, do not take this as a push, I'm just trying to get information on all the needed pieces to have |
|
LGTM |
4 participants
The current code simply ignores the full binary path when starting the
shimv2 process, and instead fallbacks to a binary in the path, and this
is problematic (and confusing) for those using CRI-O, which has this
bits vendored.
The reason it's problematic with CRI-O is because the user can simply
set the full binary path and, instead of having that executed, CRI-O
will simply fail to create the container unless that binary is part of
the path, which may not be case in a few different scenarios (testing
being the most common one).
Fixes: #5006
Signed-off-by: Fabiano Fidêncio [email protected]