Use user data path for plugin discovery in rootless mode#44733
Use user data path for plugin discovery in rootless mode#44733neersighted merged 2 commits intomasterfrom unknown repository
Conversation
vvoland
left a comment
vvoland
left a comment
There was a problem hiding this comment.
Thank you for your contribution! Looks good to me, but I left one comment regarding mutating the global variable.
| // ConfigureSpecsPaths configures plugins.specsPaths depending on if the daemon | ||
| // is running in rootless mode or not. On Windows, it is a no-op. | ||
| func ConfigureSpecsPaths() { | ||
| if rootless.RunningWithRootlessKit() { | ||
| dataHome, err := homedir.GetDataHome() | ||
| if err == nil { | ||
| specsPaths = []string{filepath.Join(dataHome, "docker/plugins")} |
There was a problem hiding this comment.
Personally I would prefer to avoid functions that mutate global variables and depend on being called in a specific time and place.
I think it would be better to change specsPaths into a function returning paths, which would be defined differently in each discovery_<platform>.go. This way we don't have any global mutable variable and don't need to worry about calling the ConfigureSpecsPaths.
What do you think?
There was a problem hiding this comment.
Ah, yes, was thinking along the same lines as well; was also wondering; would it do any harm to unconditionally append homedir.GetDataHome() to the list of paths to look in? (if the XDG_XXXvar is set) Perhaps needs async.Onceto resolve the path once (if there's nothing like that yet), but then aspecPaths()` function could return the lists of paths to look for.
There was a problem hiding this comment.
^^ I didn't dig into all code-paths though, so be sure to look if that idea works 😅
There was a problem hiding this comment.
I introduced func SpecsPaths() []string while keeping specsPaths around to avoid a large diff in the Unit-Tests.
would it do any harm to unconditionally append
homedir.GetDataHome()to the list of paths to look in?
Right now the user can use his own plugins without being affected by plugins installed in the host system.
If we add $XDG_DATA_HOME/.local/share/docker/plugins to SpecsPaths unconditionally, the user no longer has this option
Code in |
SGTM |
ghost
requested a review
from 
neersighted
left a comment
neersighted
left a comment
There was a problem hiding this comment.
Slightly tangential to this PR, but I wonder if the homedir package should fall back to the starting directory from the passwd if HOME is not found in the environment. Comparison to other tools (e.g. Git, GnuPG, other well-known Unix software) would be interesting.
Doing so too is a good idea 👍 but I would not change this behavior in this PR Checks fail because checks fail in the |
|
The failed check on master is a Jenkins alt-arch issues; the failure here is because of a stale branch. Please rebase onto master. I'm ready to give this a LGTM; I have two slight objections but I believe they can be follow-ups if you are willing:
|
Signed-off-by: Jan Garcia <[email protected]>
Signed-off-by: Jan Garcia <[email protected]>
|
Thank you, I rebased the branch as requested. I'll create two follow-up PRs for
|
|
@AkihiroSuda I see you added the cherry-pick label; I'm assuming you think we should include this in the 23.0 branch, but not the 20.10 branch? |
Yes |
4 participants
Signed-off-by: Jan Garcia [email protected]
fixes #43111
- What I did
We now dynamically determine the plugins path when running in rootless mode.
- How I did it
Introduce a new function,
plugins.SpecsPathswhich returns$XDG_DATA_HOME/.local/share/docker/pluginson Unix in rootless mode and
specsPathsotherwise.- How to verify it
You can test this by running
dockerdin rootless mode while/etc/dockeris not accessible by the user.Without this patch, creating volumes will fail. With this patch creating volumes works.
- Description for the changelog
Use user data path for plugin discovery in rootless mode