adding path suffix to multipath chroot command #64
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaroncirillo-cision
requested review from
kumarp20,
bpjain2004,
francis-nijay,
harishp8889,
prablr79,
hoppea2,
tdawe,
coulof,
shaynafinocchiaro,
xuluna,
alikdell and
atye
as code owners
McThumpenstein
approved these changes
Oct 24, 2024
karthikk92
approved these changes
Dec 13, 2024
|
Please run sanity tests and attach the results |
donatwork
reviewed
Feb 10, 2025
| @@ -200,7 +201,7 @@ func (mp *Multipath) runCommand(ctx context.Context, command string, args []stri | |||
| } | |||
|
|
|||
| if mp.chroot != "" { | |||
| args = append([]string{mp.chroot, command}, args...) | |||
| args = append([]string{mp.chroot + os.Getenv("MP_CHROOT_SUFFIX"), command}, args...) | |||
There was a problem hiding this comment.
There is no validation of the contents of the variable. I'm a little concerned with the security implications of this. Should there be some validation?
There was a problem hiding this comment.
I suppose that's a good point. How would you propose that validation happens here?
There was a problem hiding this comment.
Perhaps there is another way to go about this all together that would be better? I wasn't super familiar with this project, and just needed to get things working for my environment, so I may have taken a non-optimal approach to this. I'm open to suggestions.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds the ability to add a path suffix to the chroot command issued when trying to invoke multipath commands through an environment variable called MP_CHROOT_SUFFIX. This was necessary on the environment I work in because our multipath daemon and associated binaries are located in /usr/local. When the chroot command would run it would try to chroot to /noderoot, but in my case it needed to chroot to /noderoot/usr/local. The additional env var allows this to happen without impacting current functionality. If there is another way you would like this implemented I'm open to suggestions.
The reason my multipath daemon is installed in /usr/local is because we are running a distribution called Talos (https://www.talos.dev/). This distribution does not come with multipath, and also has an immutable root filesystem, but they do have a system for adding extensions into /usr/local. So I have created a multipath extension for this distro that installs multipath into /usr/local.
GitHub Issues
N/A
Checklist:
How Has This Been Tested?
This has been tested by running local unit tests as well as running in my environment with the Dell Unity CSI Driver (https://github.com/dell/csi-unity). I validated that in our environment the multipath commands are now being executed correctly because of the updated chroot path.
If this PR is accepted there should also be an update made to the values file for the csi-unity helm chart that documents that this variable is available and the impact it can have.