Fix issue with plugin scanner going to deep

cpuguy83 · cpuguy83 · commit b27f70d45a0f · 2018-02-02T16:49:14.000-05:00
The plugin spec says that plugins can live in one of:

- /var/run/docker/plugins/&lt;name&gt;.sock
- /var/run/docker/plugins/&lt;name&gt;/&lt;name&gt;.sock
- /etc/docker/plugins/&lt;name&gt;.[json,spec]
- /etc/docker/plugins/&lt;name&gt;/&lt;name&gt;.&lt;json,spec&gt;
- /usr/lib/docker/plugins/&lt;name&gt;.&lt;json,spec&gt;
- /usr/lib/docker/plugins/&lt;name&gt;/&lt;name&gt;.&lt;json,spec&gt;

However, the plugin scanner which is used by the volume list API was
doing `filepath.Walk`, which will walk the entire tree for each of the
supported paths.
This means that even v2 plugins in
`/var/run/docker/plugins/&lt;id&gt;/&lt;name&gt;.sock` were being detected as a v1
plugin.
When the v1 plugin loader tried to load such a plugin it would log an
error that it couldn't find it because it doesn't match one of the
supported patterns... e.g. when in a subdir, the subdir name must match
the plugin name for the socket.

There is no behavior change as the error is only on the `Scan()` call,
which is passing names to the plugin registry when someone calls the
volume list API.

Signed-off-by: Brian Goff &lt;cpuguy83@gmail.com&gt;
diff --git a/pkg/plugins/discovery.go b/pkg/plugins/discovery.go
@@ -2,14 +2,15 @@ package plugins
 
 import (
 	"encoding/json"
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/url"
 	"os"
 	"path/filepath"
 	"strings"
 	"sync"
+
+	"github.com/pkg/errors"
 )
 
 var (
@@ -28,30 +29,52 @@ func newLocalRegistry() localRegistry {
 // Scan scans all the plugin paths and returns all the names it found
 func Scan() ([]string, error) {
 	var names []string
-	if err := filepath.Walk(socketsPath, func(path string, fi os.FileInfo, err error) error {
-		if err != nil {
-			return nil
+	dirEntries, err := ioutil.ReadDir(socketsPath)
+	if err != nil && !os.IsNotExist(err) {
+		return nil, errors.Wrap(err, "error reading dir entries")
+	}
+
+	for _, fi := range dirEntries {
+		if fi.IsDir() {
+			fi, err = os.Stat(filepath.Join(socketsPath, fi.Name(), fi.Name()+".sock"))
+			if err != nil {
+				continue
+			}
 		}
 
 		if fi.Mode()&os.ModeSocket != 0 {
-			name := strings.TrimSuffix(fi.Name(), filepath.Ext(fi.Name()))
-			names = append(names, name)
+			names = append(names, strings.TrimSuffix(filepath.Base(fi.Name()), filepath.Ext(fi.Name())))
 		}
-		return nil
-	}); err != nil {
-		return nil, err
 	}
 
-	for _, path := range specsPaths {
-		if err := filepath.Walk(path, func(p string, fi os.FileInfo, err error) error {
-			if err != nil || fi.IsDir() {
-				return nil
+	for _, p := range specsPaths {
+		dirEntries, err := ioutil.ReadDir(p)
+		if err != nil && !os.IsNotExist(err) {
+			return nil, errors.Wrap(err, "error reading dir entries")
+		}
+
+		for _, fi := range dirEntries {
+			if fi.IsDir() {
+				infos, err := ioutil.ReadDir(filepath.Join(p, fi.Name()))
+				if err != nil {
+					continue
+				}
+
+				for _, info := range infos {
+					if strings.TrimSuffix(info.Name(), filepath.Ext(info.Name())) == fi.Name() {
+						fi = info
+						break
+					}
+				}
+			}
+
+			ext := filepath.Ext(fi.Name())
+			switch ext {
+			case ".spec", ".json":
+				plugin := strings.TrimSuffix(fi.Name(), ext)
+				names = append(names, plugin)
+			default:
 			}
-			name := strings.TrimSuffix(fi.Name(), filepath.Ext(fi.Name()))
-			names = append(names, name)
-			return nil
-		}); err != nil {
-			return nil, err
 		}
 	}
 	return names, nil
@@ -81,7 +104,7 @@ func (l *localRegistry) Plugin(name string) (*Plugin, error) {
 			return readPluginInfo(name, p)
 		}
 	}
-	return nil, ErrNotFound
+	return nil, errors.Wrapf(ErrNotFound, "could not find plugin %s in v1 plugin registry", name)
 }
 
 func readPluginInfo(name, path string) (*Plugin, error) {
diff --git a/pkg/plugins/discovery_unix_test.go b/pkg/plugins/discovery_unix_test.go
@@ -97,7 +97,63 @@ func TestScan(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
+	if len(pluginNamesNotEmpty) != 1 {
+		t.Fatalf("expected 1 plugin entry: %v", pluginNamesNotEmpty)
+	}
 	if p.Name() != pluginNamesNotEmpty[0] {
 		t.Fatalf("Unable to scan plugin with name %s", p.name)
 	}
 }
+
+func TestScanNotPlugins(t *testing.T) {
+	tmpdir, unregister := Setup(t)
+	defer unregister()
+
+	// not that `Setup()` above sets the sockets path and spec path dirs, which
+	// `Scan()` uses to find plugins to the returned `tmpdir`
+
+	notPlugin := filepath.Join(tmpdir, "not-a-plugin")
+	if err := os.MkdirAll(notPlugin, 0700); err != nil {
+		t.Fatal(err)
+	}
+
+	// this is named differently than the dir it's in, so the scanner should ignore it
+	l, err := net.Listen("unix", filepath.Join(notPlugin, "foo.sock"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer l.Close()
+
+	// same let's test a spec path
+	f, err := os.Create(filepath.Join(notPlugin, "foo.spec"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+
+	names, err := Scan()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(names) != 0 {
+		t.Fatalf("expected no plugins, got %v", names)
+	}
+
+	// Just as a sanity check, let's make an entry that the scanner should read
+	f, err = os.Create(filepath.Join(notPlugin, "not-a-plugin.spec"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+
+	names, err = Scan()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(names) != 1 {
+		t.Fatalf("expected 1 entry in result: %v", names)
+	}
+	if names[0] != "not-a-plugin" {
+		t.Fatalf("expected plugin named `not-a-plugin`, got: %s", names[0])
+	}
+}
diff --git a/pkg/plugins/plugin_test.go b/pkg/plugins/plugin_test.go
@@ -3,7 +3,6 @@ package plugins
 import (
 	"bytes"
 	"encoding/json"
-	"errors"
 	"io"
 	"io/ioutil"
 	"net/http"
@@ -15,6 +14,7 @@ import (
 
 	"github.com/docker/docker/pkg/plugins/transport"
 	"github.com/docker/go-connections/tlsconfig"
+	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -78,11 +78,11 @@ func TestGet(t *testing.T) {
 
 	// check negative case where plugin fruit doesn't implement banana
 	_, err = Get("fruit", "banana")
-	assert.Equal(t, err, ErrNotImplements)
+	assert.Equal(t, errors.Cause(err), ErrNotImplements)
 
 	// check negative case where plugin vegetable doesn't exist
 	_, err = Get("vegetable", "potato")
-	assert.Equal(t, err, ErrNotFound)
+	assert.Equal(t, errors.Cause(err), ErrNotFound)
 
 }
 
