Repository: puppetlabs/wash
Pull request #151 (2019-03-20 16:44:24)

Make external plugin implementation private
The external plugin implementation is an implementation detail in wash,
it should not be exported from the package. Make most of the
implementation private. Leaves RegisterExternalPlugin (and
ExternalPluginSpec) public to allow the CLI to augment the registry.

Signed-off-by: Michael Smith <michael.smith@puppet.com>


Patch:
diff --git a/cmd/server.go b/cmd/server.go
index 151dcb9..204dbe3 100644
--- a/cmd/server.go
+++ b/cmd/server.go
@@ -171,11 +171,9 @@ func loadLogger(levelStr string, logfile string) (*os.File, error) {
 
 func loadPlugin(registry *plugin.Registry, name string, root plugin.Root) {
 	log.Infof("Loading %v", name)
-	if err := root.Init(); err != nil {
+	if err := registry.RegisterPlugin(root); err != nil {
 		// %+v is a convention used by some errors to print additional context such as a stack trace
 		log.Warnf("%v failed to load: %+v", name, err)
-	} else {
-		registry.RegisterPlugin(name, root)
 	}
 }
 
@@ -213,8 +211,11 @@ func loadExternalPlugins(registry *plugin.Registry, externalPluginsPath string)
 	}
 
 	for _, p := range externalPlugins {
-		root := plugin.NewExternalPluginRoot(p)
-		loadPlugin(registry, p.Name, root)
+		log.Infof("Loading %v", p.Script)
+		if err := registry.RegisterExternalPlugin(p); err != nil {
+			// %+v is a convention used by some errors to print additional context such as a stack trace
+			log.Warnf("%v failed to load: %+v", p.Script, err)
+		}
 	}
 
 	log.Infof("Finished loading external plugins")
diff --git a/docs/external_plugins/README.md b/docs/external_plugins/README.md
index 2b8223b..45f45b0 100644
--- a/docs/external_plugins/README.md
+++ b/docs/external_plugins/README.md
@@ -8,14 +8,11 @@ External plugins let Wash talk to other things outside of the built-in plugins.
 
 2. Write the [plugin script](plugin_script.md). This is the script that Wash will shell out to whenever it needs to invoke an action on a specific entry within your plugin.
 
-3. Add the plugin to the (configurable) `plugins.yaml` file by specifying its name and path to the plugin script. An example `plugins.yaml` file is shown below:
+3. Add the plugin to the (configurable) `plugins.yaml` file by specifying a path to the plugin script. The name will be determined by invoking the script with the `init` action. An example `plugins.yaml` file is shown below:
 
     ```
-    - name: 'external-aws'
-      script: '/Users/enis.inan/wash/external-plugins/external-aws.rb'
-  
-    - name: 'network'
-      script: '/Users/enis.inan/wash/external-plugins/network.sh'
+    - script: '/Users/enis.inan/wash/external-plugins/external-aws.rb'
+    - script: '/Users/enis.inan/wash/external-plugins/network.sh'
     ```
 4. Start the Wash server to see your plugin in action.
 
diff --git a/plugin/action.go b/plugin/action.go
index bc005bf..c6b6d6e 100644
--- a/plugin/action.go
+++ b/plugin/action.go
@@ -44,9 +44,9 @@ var ExecAction = newAction("exec", "Execable")
 // entry's supported actions.
 func SupportedActionsOf(entry Entry) []string {
 	switch t := entry.(type) {
-	case *ExternalPluginRoot:
+	case *externalPluginRoot:
 		return t.supportedActions
-	case *ExternalPluginEntry:
+	case *externalPluginEntry:
 		return t.supportedActions
 	default:
 		actions := make([]string, 0)
diff --git a/plugin/externalPluginEntry.go b/plugin/externalPluginEntry.go
index 7c8d512..691f6dd 100644
--- a/plugin/externalPluginEntry.go
+++ b/plugin/externalPluginEntry.go
@@ -68,7 +68,7 @@ type decodedExternalPluginEntry struct {
 	State            string            `json:"state"`
 }
 
-func (e decodedExternalPluginEntry) toExternalPluginEntry() (*ExternalPluginEntry, error) {
+func (e decodedExternalPluginEntry) toExternalPluginEntry() (*externalPluginEntry, error) {
 	if e.Name == "" {
 		return nil, fmt.Errorf("the entry name must be provided")
 	}
@@ -81,7 +81,7 @@ func (e decodedExternalPluginEntry) toExternalPluginEntry() (*ExternalPluginEntr
 		return nil, err
 	}
 
-	entry := &ExternalPluginEntry{
+	entry := &externalPluginEntry{
 		EntryBase:        NewEntry(e.Name),
 		supportedActions: e.SupportedActions,
 		state:            e.State,
@@ -92,18 +92,18 @@ func (e decodedExternalPluginEntry) toExternalPluginEntry() (*ExternalPluginEntr
 	return entry, nil
 }
 
-// ExternalPluginEntry represents an entry from an external plugin. It consists
+// externalPluginEntry represents an entry from an external plugin. It consists
 // of its name, its object (as serialized JSON), and the path to its
 // main plugin script.
-type ExternalPluginEntry struct {
+type externalPluginEntry struct {
 	EntryBase
-	script           ExternalPluginScript
+	script           externalPluginScript
 	supportedActions []string
 	state            string
 	attr             Attributes
 }
 
-func (e *ExternalPluginEntry) setCacheTTLs(ttls decodedCacheTTLs) {
+func (e *externalPluginEntry) setCacheTTLs(ttls decodedCacheTTLs) {
 	if ttls.List != 0 {
 		e.SetTTLOf(List, ttls.List*time.Second)
 	}
@@ -116,7 +116,7 @@ func (e *ExternalPluginEntry) setCacheTTLs(ttls decodedCacheTTLs) {
 }
 
 // List lists the entry's children, if it has any.
-func (e *ExternalPluginEntry) List(ctx context.Context) ([]Entry, error) {
+func (e *externalPluginEntry) List(ctx context.Context) ([]Entry, error) {
 	stdout, err := e.script.InvokeAndWait(ctx, "list", e.ID(), e.state)
 	if err != nil {
 		return nil, err
@@ -149,7 +149,7 @@ func (e *ExternalPluginEntry) List(ctx context.Context) ([]Entry, error) {
 }
 
 // Open returns the entry's content
-func (e *ExternalPluginEntry) Open(ctx context.Context) (SizedReader, error) {
+func (e *externalPluginEntry) Open(ctx context.Context) (SizedReader, error) {
 	stdout, err := e.script.InvokeAndWait(ctx, "read", e.ID(), e.state)
 	if err != nil {
 		return nil, err
@@ -159,7 +159,7 @@ func (e *ExternalPluginEntry) Open(ctx context.Context) (SizedReader, error) {
 }
 
 // Metadata displays the resource's metadata
-func (e *ExternalPluginEntry) Metadata(ctx context.Context) (MetadataMap, error) {
+func (e *externalPluginEntry) Metadata(ctx context.Context) (MetadataMap, error) {
 	stdout, err := e.script.InvokeAndWait(ctx, "metadata", e.ID(), e.state)
 	if err != nil {
 		return nil, err
@@ -181,7 +181,7 @@ func (e *ExternalPluginEntry) Metadata(ctx context.Context) (MetadataMap, error)
 }
 
 // Attr returns the entry's filesystem attributes
-func (e *ExternalPluginEntry) Attr() Attributes {
+func (e *externalPluginEntry) Attr() Attributes {
 	return e.attr
 }
 
@@ -219,7 +219,7 @@ func (s *stdoutStreamer) Close() error {
 }
 
 // Stream streams the entry's content
-func (e *ExternalPluginEntry) Stream(ctx context.Context) (io.Reader, error) {
+func (e *externalPluginEntry) Stream(ctx context.Context) (io.Reader, error) {
 	cmd, stdoutR, stderrR, err := CreateCommand(
 		e.script.Path(),
 		"stream",
@@ -339,7 +339,7 @@ func (e *ExternalPluginEntry) Stream(ctx context.Context) (io.Reader, error) {
 }
 
 // Exec executes a command on the given entry
-func (e *ExternalPluginEntry) Exec(ctx context.Context, cmd string, args []string, opts ExecOptions) (ExecResult, error) {
+func (e *externalPluginEntry) Exec(ctx context.Context, cmd string, args []string, opts ExecOptions) (ExecResult, error) {
 	execResult := ExecResult{}
 
 	// TODO: Figure out how to pass-in opts when we have entries
diff --git a/plugin/externalPluginRoot.go b/plugin/externalPluginRoot.go
index 2de6ebe..5fb2a18 100644
--- a/plugin/externalPluginRoot.go
+++ b/plugin/externalPluginRoot.go
@@ -9,27 +9,21 @@ import (
 	"github.com/puppetlabs/wash/journal"
 )
 
-// ExternalPluginSpec represents an external plugin's YAML specification.
-type ExternalPluginSpec struct {
-	Name   string
-	Script string
+// externalPluginRoot represents an external plugin's root.
+type externalPluginRoot struct {
+	*externalPluginEntry
 }
 
-// ExternalPluginRoot represents an external plugin's root.
-type ExternalPluginRoot struct {
-	*ExternalPluginEntry
-}
-
-// NewExternalPluginRoot returns a new external plugin root given
+// newExternalPluginRoot returns a new external plugin root given
 // the plugin script
-func NewExternalPluginRoot(plugin ExternalPluginSpec) *ExternalPluginRoot {
-	return &ExternalPluginRoot{&ExternalPluginEntry{
-		script: NewExternalPluginScript(plugin.Script),
+func newExternalPluginRoot(script string) *externalPluginRoot {
+	return &externalPluginRoot{&externalPluginEntry{
+		script: externalPluginScriptImpl{path: script},
 	}}
 }
 
 // Init initializes the external plugin root
-func (r *ExternalPluginRoot) Init() error {
+func (r *externalPluginRoot) Init() error {
 	ctx := context.Background()
 	script := r.script
 
@@ -55,17 +49,17 @@ func (r *ExternalPluginRoot) Init() error {
 		return err
 	}
 
-	r.ExternalPluginEntry = entry
-	r.ExternalPluginEntry.script = script
+	r.externalPluginEntry = entry
+	r.externalPluginEntry.script = script
 
 	return nil
 }
 
 // List lists the root's entries.
 //
-// We need this b/c *ExternalPluginEntry#List has a different receiver
-// (*ExternalPluginEntry) than *ExternalPluginRoot (i.e. b/c Go's
+// We need this b/c *externalPluginEntry#List has a different receiver
+// (*externalPluginEntry) than *externalPluginRoot (i.e. b/c Go's
 // typechecker complains about it)
-func (r *ExternalPluginRoot) List(ctx context.Context) ([]Entry, error) {
-	return r.ExternalPluginEntry.List(ctx)
+func (r *externalPluginRoot) List(ctx context.Context) ([]Entry, error) {
+	return r.externalPluginEntry.List(ctx)
 }
diff --git a/plugin/externalPluginScript.go b/plugin/externalPluginScript.go
index 40acb7e..26c1fcb 100644
--- a/plugin/externalPluginScript.go
+++ b/plugin/externalPluginScript.go
@@ -10,23 +10,17 @@ import (
 	"github.com/puppetlabs/wash/journal"
 )
 
-// ExternalPluginScript represents an external plugin's script
-type ExternalPluginScript interface {
+// externalPluginScript represents an external plugin's script
+type externalPluginScript interface {
 	Path() string
 	InvokeAndWait(ctx context.Context, args ...string) ([]byte, error)
 }
 
-type externalPluginScript struct {
+type externalPluginScriptImpl struct {
 	path string
 }
 
-// NewExternalPluginScript returns a new external plugin script
-// object
-func NewExternalPluginScript(path string) ExternalPluginScript {
-	return externalPluginScript{path: path}
-}
-
-func (s externalPluginScript) Path() string {
+func (s externalPluginScriptImpl) Path() string {
 	return s.path
 }
 
@@ -38,7 +32,7 @@ func (s externalPluginScript) Path() string {
 // https://golang.org/pkg/os/exec/#Cmd.Wait. Could this be specified by
 // plugin authors in the top-level YAML file? Should it be a per-entry
 // thing?
-func (s externalPluginScript) InvokeAndWait(ctx context.Context, args ...string) ([]byte, error) {
+func (s externalPluginScriptImpl) InvokeAndWait(ctx context.Context, args ...string) ([]byte, error) {
 	journal.Record(ctx, "Running command: %v %v", s.Path(), strings.Join(args, " "))
 
 	cmd := exec.Command(s.Path(), args...)
diff --git a/plugin/helpers.go b/plugin/helpers.go
index cf06491..aff2308 100644
--- a/plugin/helpers.go
+++ b/plugin/helpers.go
@@ -132,7 +132,7 @@ func FillAttr(ctx context.Context, entry Entry, attr *Attributes) error {
 		attrRet := item.Attr()
 
 		// We need the zero-value check for external plugin entries,
-		// b/c the ExternalPluginEntry class has Attr() implemented,
+		// b/c the externalPluginEntry class has Attr() implemented,
 		// but not all external plugin entries have attributes.
 		if attrRet != (Attributes{}) {
 			(*attr) = attrRet
diff --git a/plugin/registry.go b/plugin/registry.go
index 3af04bf..bfeffe6 100644
--- a/plugin/registry.go
+++ b/plugin/registry.go
@@ -2,6 +2,8 @@ package plugin
 
 import (
 	"context"
+	"fmt"
+	"os"
 )
 
 // Registry represents the plugin registry. It is also Wash's root.
@@ -29,10 +31,37 @@ func (r *Registry) Plugins() map[string]Root {
 	return r.plugins
 }
 
-// RegisterPlugin registers the given plugin
-func (r *Registry) RegisterPlugin(name string, root Root) {
-	r.plugins[name] = root
+// RegisterPlugin initializes the given plugin and adds it to the registry if
+// initialization was successful.
+func (r *Registry) RegisterPlugin(root Root) error {
+	if err := root.Init(); err != nil {
+		return err
+	}
+
+	r.plugins[root.Name()] = root
 	r.pluginRoots = append(r.pluginRoots, root)
+	return nil
+}
+
+// ExternalPluginSpec represents an external plugin's specification.
+type ExternalPluginSpec struct {
+	Script string
+}
+
+// RegisterExternalPlugin initializes an external plugin from its spec and
+// passes it to RegisterPlugin.
+func (r *Registry) RegisterExternalPlugin(spec ExternalPluginSpec) error {
+	fi, err := os.Stat(spec.Script)
+	if err != nil {
+		return err
+	} else if !fi.Mode().IsRegular() {
+		return fmt.Errorf("script %v is not a file", spec.Script)
+	} else if fi.Mode().Perm()&0100 == 0 {
+		return fmt.Errorf("script %v is not executable", spec.Script)
+	}
+
+	root := newExternalPluginRoot(spec.Script)
+	return r.RegisterPlugin(root)
 }
 
 // List all of Wash's loaded plugins


Test patch:
diff --git a/plugin/entryBase_test.go b/plugin/entryBase_test.go
index d084f4e..c25cc49 100644
--- a/plugin/entryBase_test.go
+++ b/plugin/entryBase_test.go
@@ -51,7 +51,6 @@ func (suite *EntryBaseTestSuite) TestNewEntry() {
 	e.TurnOffCaching()
 	assertOpTTL(Open, "Open", -1)
 	assertOpTTL(Metadata, "Metadata", -1)
-
 }
 
 func TestEntryBase(t *testing.T) {
diff --git a/plugin/externalPluginEntry_test.go b/plugin/externalPluginEntry_test.go
index ad67935..3edb668 100644
--- a/plugin/externalPluginEntry_test.go
+++ b/plugin/externalPluginEntry_test.go
@@ -139,7 +139,7 @@ func (suite *ExternalPluginEntryTestSuite) TestSetCacheTTLs() {
 		Metadata: 20,
 	}
 
-	entry := ExternalPluginEntry{
+	entry := externalPluginEntry{
 		EntryBase: NewEntry("foo"),
 	}
 	entry.setCacheTTLs(decodedTTLs)
@@ -157,7 +157,7 @@ func (suite *ExternalPluginEntryTestSuite) TestSetCacheTTLs() {
 
 func (suite *ExternalPluginEntryTestSuite) TestList() {
 	mockScript := &mockExternalPluginScript{path: "plugin_script"}
-	entry := ExternalPluginEntry{
+	entry := externalPluginEntry{
 		EntryBase: NewEntry("foo"),
 		script:    mockScript,
 	}
@@ -188,7 +188,7 @@ func (suite *ExternalPluginEntryTestSuite) TestList() {
 	entries, err := entry.List(ctx)
 	if suite.NoError(err) {
 		expectedEntries := []Entry{
-			&ExternalPluginEntry{
+			&externalPluginEntry{
 				EntryBase:        NewEntry("foo"),
 				supportedActions: []string{"list"},
 				script:           entry.script,
@@ -201,7 +201,7 @@ func (suite *ExternalPluginEntryTestSuite) TestList() {
 
 func (suite *ExternalPluginEntryTestSuite) TestOpen() {
 	mockScript := &mockExternalPluginScript{path: "plugin_script"}
-	entry := ExternalPluginEntry{
+	entry := externalPluginEntry{
 		EntryBase: NewEntry("foo"),
 		script:    mockScript,
 	}
@@ -230,7 +230,7 @@ func (suite *ExternalPluginEntryTestSuite) TestOpen() {
 
 func (suite *ExternalPluginEntryTestSuite) TestMetadata() {
 	mockScript := &mockExternalPluginScript{path: "plugin_script"}
-	entry := ExternalPluginEntry{
+	entry := externalPluginEntry{
 		EntryBase: NewEntry("foo"),
 		script:    mockScript,
 	}
@@ -264,7 +264,7 @@ func (suite *ExternalPluginEntryTestSuite) TestMetadata() {
 }
 
 func (suite *ExternalPluginEntryTestSuite) TestAttr() {
-	entry := ExternalPluginEntry{attr: Attributes{Size: 10}}
+	entry := externalPluginEntry{attr: Attributes{Size: 10}}
 	suite.Equal(entry.attr, entry.Attr())
 }
 
diff --git a/plugin/externalPluginRoot_test.go b/plugin/externalPluginRoot_test.go
index 50bf772..68f8bf1 100644
--- a/plugin/externalPluginRoot_test.go
+++ b/plugin/externalPluginRoot_test.go
@@ -14,7 +14,7 @@ type ExternalPluginRootTestSuite struct {
 
 func (suite *ExternalPluginRootTestSuite) TestInit() {
 	mockScript := &mockExternalPluginScript{path: "plugin_script"}
-	root := &ExternalPluginRoot{&ExternalPluginEntry{
+	root := &externalPluginRoot{&externalPluginEntry{
 		EntryBase: NewEntry("foo"),
 		script:    mockScript,
 	}}
@@ -40,8 +40,8 @@ func (suite *ExternalPluginRootTestSuite) TestInit() {
 	mockInvokeAndWait([]byte(stdout), nil)
 	err = root.Init()
 	if suite.NoError(err) {
-		expectedRoot := &ExternalPluginRoot{
-			ExternalPluginEntry: &ExternalPluginEntry{
+		expectedRoot := &externalPluginRoot{
+			externalPluginEntry: &externalPluginEntry{
 				EntryBase:        NewEntry("foo"),
 				supportedActions: []string{"list"},
 				script:           root.script,
diff --git a/plugin/registry_test.go b/plugin/registry_test.go
new file mode 100644
index 0000000..bbae267
--- /dev/null
+++ b/plugin/registry_test.go
@@ -0,0 +1,90 @@
+package plugin
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/suite"
+)
+
+type RegistryTestSuite struct {
+	suite.Suite
+}
+
+func (suite *RegistryTestSuite) TestEmptyRegistry() {
+	reg := NewRegistry()
+	suite.Empty(reg.Plugins())
+}
+
+type mockRoot struct {
+	EntryBase
+	mock.Mock
+}
+
+func (m *mockRoot) Init() error {
+	args := m.Called()
+	return args.Error(0)
+}
+
+func (m *mockRoot) List(ctx context.Context) ([]Entry, error) {
+	args := m.Called(ctx)
+	return args.Get(0).([]Entry), args.Error(1)
+}
+
+func (suite *RegistryTestSuite) TestRegisterPlugin() {
+	reg := NewRegistry()
+	m := &mockRoot{EntryBase: NewEntry("mine")}
+	m.On("Init").Return(nil)
+
+	suite.NoError(reg.RegisterPlugin(m))
+	m.AssertExpectations(suite.T())
+	suite.Contains(reg.Plugins(), "mine")
+}
+
+func (suite *RegistryTestSuite) TestRegisterPluginError() {
+	reg := NewRegistry()
+	m := &mockRoot{EntryBase: NewEntry("mine")}
+	m.On("Init").Return(errors.New("failed"))
+
+	suite.EqualError(reg.RegisterPlugin(m), "failed")
+	m.AssertExpectations(suite.T())
+	suite.NotContains(reg.Plugins(), "mine")
+}
+
+func (suite *RegistryTestSuite) TestRegisterExternalPlugin() {
+	reg := NewRegistry()
+	spec := ExternalPluginSpec{Script: "testdata/external.sh"}
+
+	suite.NoError(reg.RegisterExternalPlugin(spec))
+	suite.Contains(reg.Plugins(), "test")
+}
+
+func (suite *RegistryTestSuite) TestRegisterExternalPluginNoExec() {
+	reg := NewRegistry()
+	spec := ExternalPluginSpec{Script: "testdata/noexec"}
+
+	suite.EqualError(reg.RegisterExternalPlugin(spec), "script testdata/noexec is not executable")
+	suite.NotContains(reg.Plugins(), "test")
+}
+
+func (suite *RegistryTestSuite) TestRegisterExternalPluginNoExist() {
+	reg := NewRegistry()
+	spec := ExternalPluginSpec{Script: "testdata/noexist"}
+
+	suite.EqualError(reg.RegisterExternalPlugin(spec), "stat testdata/noexist: no such file or directory")
+	suite.NotContains(reg.Plugins(), "test")
+}
+
+func (suite *RegistryTestSuite) TestRegisterExternalPluginNotFile() {
+	reg := NewRegistry()
+	spec := ExternalPluginSpec{Script: "testdata/notfile"}
+
+	suite.EqualError(reg.RegisterExternalPlugin(spec), "script testdata/notfile is not a file")
+	suite.NotContains(reg.Plugins(), "test")
+}
+
+func TestRegistry(t *testing.T) {
+	suite.Run(t, new(RegistryTestSuite))
+}
diff --git a/plugin/testdata/external.sh b/plugin/testdata/external.sh
new file mode 100755
index 0000000..8bb27c9
--- /dev/null
+++ b/plugin/testdata/external.sh
@@ -0,0 +1,7 @@
+#!/bin/sh
+echo '{
+  "name": "test",
+  "supported_actions": [
+    "list"
+  ]
+}'
\ No newline at end of file