From 6de3d71ab6c829b4eb504305abacd6242fca5b6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 28 Nov 2024 12:45:19 +0100 Subject: [PATCH 1/2] cli-plugins: Fix searching inaccessible directories MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix a case where one inaccessible plugin search path stops the whole search and prevents latter paths from being scanned. Remove a preliminary `Stat` call that verifies whether path is an actual directory and is accessible. It's unneeded and doesn't actually check whether the directory can be listed or not. `os.ReadDir` will fail in such case anyway, so just attempt to do that and ignore any encountered error, instead of erroring out the whole plugin candidate listing. Signed-off-by: Paweł Gronowski --- cli-plugins/manager/manager.go | 10 +++------- cli-plugins/manager/manager_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 27 insertions(+), 7 deletions(-) diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index f9229c52576c..ef4b489606d3 100644 --- a/cli-plugins/manager/manager.go +++ b/cli-plugins/manager/manager.go @@ -77,8 +77,10 @@ func getPluginDirs(cfg *configfile.ConfigFile) ([]string, error) { func addPluginCandidatesFromDir(res map[string][]string, d string) error { dentries, err := os.ReadDir(d) + // Silently ignore any directories which we cannot list (e.g. due to + // permissions or anything else) or which is not a directory if err != nil { - return err + return nil } for _, dentry := range dentries { switch dentry.Type() & os.ModeType { @@ -106,12 +108,6 @@ func addPluginCandidatesFromDir(res map[string][]string, d string) error { func listPluginCandidates(dirs []string) (map[string][]string, error) { result := make(map[string][]string) for _, d := range dirs { - // Silently ignore any directories which we cannot - // Stat (e.g. due to permissions or anything else) or - // which is not a directory. - if fi, err := os.Stat(d); err != nil || !fi.IsDir() { - continue - } if err := addPluginCandidatesFromDir(result, d); err != nil { // Silently ignore paths which don't exist. if os.IsNotExist(err) { diff --git a/cli-plugins/manager/manager_test.go b/cli-plugins/manager/manager_test.go index 1583420a4824..42945cc4e320 100644 --- a/cli-plugins/manager/manager_test.go +++ b/cli-plugins/manager/manager_test.go @@ -82,6 +82,30 @@ func TestListPluginCandidates(t *testing.T) { assert.DeepEqual(t, candidates, exp) } +// Regression test for https://github.com/docker/cli/issues/5643. +// Check that inaccessible directories that come before accessible ones are ignored +// and do not prevent the latter from being processed. +func TestListPluginCandidatesInaccesibleDir(t *testing.T) { + dir := fs.NewDir(t, t.Name(), + fs.WithDir("no-perm", fs.WithMode(0)), + fs.WithDir("plugins", + fs.WithFile("docker-buildx", ""), + ), + ) + defer dir.Remove() + + candidates, err := listPluginCandidates([]string{ + dir.Join("no-perm"), + dir.Join("plugins"), + }) + assert.NilError(t, err) + assert.DeepEqual(t, candidates, map[string][]string{ + "buildx": { + dir.Join("plugins", "docker-buildx"), + }, + }) +} + func TestGetPlugin(t *testing.T) { dir := fs.NewDir(t, t.Name(), fs.WithFile("docker-bbb", ` From fcd94feefb01a68e1d3600702247da6e6a919552 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Gronowski?= Date: Thu, 28 Nov 2024 14:24:40 +0100 Subject: [PATCH 2/2] cli-plugins: Simplify addPluginCandidatesFromDir MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The returned error is always nil now, so just remove it. Signed-off-by: Paweł Gronowski --- cli-plugins/manager/manager.go | 28 +++++++--------------------- cli-plugins/manager/manager_test.go | 6 ++---- 2 files changed, 9 insertions(+), 25 deletions(-) diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index ef4b489606d3..223f3ae0a7fd 100644 --- a/cli-plugins/manager/manager.go +++ b/cli-plugins/manager/manager.go @@ -75,12 +75,12 @@ func getPluginDirs(cfg *configfile.ConfigFile) ([]string, error) { return pluginDirs, nil } -func addPluginCandidatesFromDir(res map[string][]string, d string) error { +func addPluginCandidatesFromDir(res map[string][]string, d string) { dentries, err := os.ReadDir(d) // Silently ignore any directories which we cannot list (e.g. due to // permissions or anything else) or which is not a directory if err != nil { - return nil + return } for _, dentry := range dentries { switch dentry.Type() & os.ModeType { @@ -101,22 +101,15 @@ func addPluginCandidatesFromDir(res map[string][]string, d string) error { } res[name] = append(res[name], filepath.Join(d, dentry.Name())) } - return nil } // listPluginCandidates returns a map from plugin name to the list of (unvalidated) Candidates. The list is in descending order of priority. -func listPluginCandidates(dirs []string) (map[string][]string, error) { +func listPluginCandidates(dirs []string) map[string][]string { result := make(map[string][]string) for _, d := range dirs { - if err := addPluginCandidatesFromDir(result, d); err != nil { - // Silently ignore paths which don't exist. - if os.IsNotExist(err) { - continue - } - return nil, err // Or return partial result? - } + addPluginCandidatesFromDir(result, d) } - return result, nil + return result } // GetPlugin returns a plugin on the system by its name @@ -126,11 +119,7 @@ func GetPlugin(name string, dockerCli command.Cli, rootcmd *cobra.Command) (*Plu return nil, err } - candidates, err := listPluginCandidates(pluginDirs) - if err != nil { - return nil, err - } - + candidates := listPluginCandidates(pluginDirs) if paths, ok := candidates[name]; ok { if len(paths) == 0 { return nil, errPluginNotFound(name) @@ -156,10 +145,7 @@ func ListPlugins(dockerCli command.Cli, rootcmd *cobra.Command) ([]Plugin, error return nil, err } - candidates, err := listPluginCandidates(pluginDirs) - if err != nil { - return nil, err - } + candidates := listPluginCandidates(pluginDirs) var plugins []Plugin var mu sync.Mutex diff --git a/cli-plugins/manager/manager_test.go b/cli-plugins/manager/manager_test.go index 42945cc4e320..92cbd002d4a4 100644 --- a/cli-plugins/manager/manager_test.go +++ b/cli-plugins/manager/manager_test.go @@ -51,8 +51,7 @@ func TestListPluginCandidates(t *testing.T) { dirs = append(dirs, dir.Join(d)) } - candidates, err := listPluginCandidates(dirs) - assert.NilError(t, err) + candidates := listPluginCandidates(dirs) exp := map[string][]string{ "plugin1": { dir.Join("plugins1", "docker-plugin1"), @@ -94,11 +93,10 @@ func TestListPluginCandidatesInaccesibleDir(t *testing.T) { ) defer dir.Remove() - candidates, err := listPluginCandidates([]string{ + candidates := listPluginCandidates([]string{ dir.Join("no-perm"), dir.Join("plugins"), }) - assert.NilError(t, err) assert.DeepEqual(t, candidates, map[string][]string{ "buildx": { dir.Join("plugins", "docker-buildx"),