fix: improve plugin error message (#371)

- added `PluginUnknownError`, `PluginValidityError`, `PluginDirectoryError`. For each error types, Notation CLI should provide a recommanded suggestion to solve them. - improved error message Resolve part of: notaryproject/notation#824 Resolve part of: notaryproject/notation#867 Signed-off-by: Junjie Gao <[email protected]> --------- Signed-off-by: Junjie Gao <[email protected]>
notaryproject · Jan 18, 2024 · d52ca71 · d52ca71
1 parent 66da1e3
commit d52ca71
Show file tree

Hide file tree

Showing 5 changed files with 90 additions and 44 deletions.
diff --git a/plugin/errors.go b/plugin/errors.go
@@ -20,14 +20,15 @@ import "errors"
 var ErrNotCompliant = errors.New("plugin not compliant")
 
 // ErrNotRegularFile is returned when the plugin file is not an regular file.
-var ErrNotRegularFile = errors.New("not regular file")
+var ErrNotRegularFile = errors.New("plugin executable file is not a regular file")
 
 // PluginDowngradeError is returned when installing a plugin with version
 // lower than the exisiting plugin version.
 type PluginDowngradeError struct {
 	Msg string
 }
 
+// Error returns the error message.
 func (e PluginDowngradeError) Error() string {
 	if e.Msg != "" {
 		return e.Msg
@@ -41,9 +42,39 @@ type InstallEqualVersionError struct {
 	Msg string
 }
 
+// Error returns the error message.
 func (e InstallEqualVersionError) Error() string {
 	if e.Msg != "" {
 		return e.Msg
 	}
 	return "installing plugin with version equal to the existing plugin version"
 }
+
+// PluginMalformedError is used when there is an issue with plugin and
+// should be fixed by plugin developers.
+type PluginMalformedError struct {
+	Msg        string
+	InnerError error
+}
+
+// Error returns the error message.
+func (e PluginMalformedError) Error() string {
+	if e.Msg != "" {
+		return e.Msg
+	}
+	return e.InnerError.Error()
+}
+
+// Unwrap returns the inner error.
+func (e PluginMalformedError) Unwrap() error {
+	return e.InnerError
+}
+
+// PluginDirectoryWalkError is used when there is an issue with plugins directory
+// and should suggest user to check the permission of plugin directory.
+type PluginDirectoryWalkError error
+
+// PluginExecutableFileError is used when there is an issue with plugin
+// executable file and should suggest user to check the existence, permission
+// and platform/arch compatibility of plugin.
+type PluginExecutableFileError error
diff --git a/plugin/manager.go b/plugin/manager.go
@@ -62,8 +62,11 @@ func (m *CLIManager) Get(ctx context.Context, name string) (Plugin, error) {
 // List produces a list of the plugin names on the system.
 func (m *CLIManager) List(ctx context.Context) ([]string, error) {
 	var plugins []string
-	fs.WalkDir(m.pluginFS, ".", func(dir string, d fs.DirEntry, err error) error {
+	if err := fs.WalkDir(m.pluginFS, ".", func(dir string, d fs.DirEntry, err error) error {
 		if err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				return nil
+			}
 			return err
 		}
 		if dir == "." {
@@ -79,7 +82,9 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) {
 		// add plugin name
 		plugins = append(plugins, d.Name())
 		return fs.SkipDir
-	})
+	}); err != nil {
+		return nil, PluginDirectoryWalkError(fmt.Errorf("failed to list plugin: %w", err))
+	}
 	return plugins, nil
 }
 
@@ -146,7 +151,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions)
 	// validate and get new plugin metadata
 	newPlugin, err := NewCLIPlugin(ctx, pluginName, pluginExecutableFile)
 	if err != nil {
-		return nil, nil, fmt.Errorf("failed to create new CLI plugin: %w", err)
+		return nil, nil, err
 	}
 	newPluginMetadata, err := newPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{})
 	if err != nil {

diff --git a/plugin/manager_test.go b/plugin/manager_test.go
@@ -388,7 +388,7 @@ func TestManager_Install(t *testing.T) {
 		installOpts := CLIInstallOptions{
 			PluginPath: newPluginFilePath,
 		}
-		expectedErrorMsg := "failed to get metadata of new plugin: executable name must be \"notation-foobar\" instead of \"notation-bar\""
+		expectedErrorMsg := "failed to get metadata of new plugin: plugin executable file name must be \"notation-foobar\" instead of \"notation-bar\""
 		_, _, err := mgr.Install(context.Background(), installOpts)
 		if err == nil || err.Error() != expectedErrorMsg {
 			t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)
@@ -405,7 +405,7 @@ func TestManager_Install(t *testing.T) {
 		installOpts := CLIInstallOptions{
 			PluginPath: newPluginFilePath,
 		}
-		expectedErrorMsg := "failed to get metadata of existing plugin: executable name must be \"notation-bar\" instead of \"notation-foo\""
+		expectedErrorMsg := "failed to get metadata of existing plugin: plugin executable file name must be \"notation-bar\" instead of \"notation-foo\""
 		_, _, err := mgr.Install(context.Background(), installOpts)
 		if err == nil || err.Error() != expectedErrorMsg {
 			t.Fatalf("expecting error %s, but got %v", expectedErrorMsg, err)

diff --git a/plugin/plugin.go b/plugin/plugin.go
@@ -25,6 +25,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 
 	"github.com/notaryproject/notation-go/internal/slices"
 	"github.com/notaryproject/notation-go/log"
@@ -81,7 +82,7 @@ func NewCLIPlugin(ctx context.Context, name, path string) (*CLIPlugin, error) {
 	if err != nil {
 		// Ignore any file which we cannot Stat
 		// (e.g. due to permissions or anything else).
-		return nil, err
+		return nil, fmt.Errorf("plugin executable file is either not found or inaccessible: %w", err)
 	}
 	if !fi.Mode().IsRegular() {
 		// Ignore non-regular files.
@@ -105,10 +106,13 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque
 	}
 	// validate metadata
 	if err = validate(&metadata); err != nil {
-		return nil, fmt.Errorf("invalid metadata: %w", err)
+		return nil, &PluginMalformedError{
+			Msg:        fmt.Sprintf("metadata validation failed for plugin %s: %s", p.name, err),
+			InnerError: err,
+		}
 	}
 	if metadata.Name != p.name {
-		return nil, fmt.Errorf("executable name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path))
+		return nil, fmt.Errorf("plugin executable file name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path))
 	}
 	return &metadata, nil
 }
@@ -171,30 +175,40 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re
 	// serialize request
 	data, err := json.Marshal(req)
 	if err != nil {
-		return fmt.Errorf("%s: failed to marshal request object: %w", pluginName, err)
+		logger.Errorf("Failed to marshal request object: %+v", req)
+		return fmt.Errorf("failed to marshal request object: %w", err)
 	}
 
 	logger.Debugf("Plugin %s request: %s", req.Command(), string(data))
 	// execute request
 	stdout, stderr, err := executor.Output(ctx, pluginPath, req.Command(), data)
 	if err != nil {
-		logger.Debugf("plugin %s execution status: %v", req.Command(), err)
-		logger.Debugf("Plugin %s returned error: %s", req.Command(), string(stderr))
-		var re proto.RequestError
-		jsonErr := json.Unmarshal(stderr, &re)
-		if jsonErr != nil {
-			return proto.RequestError{
-				Code: proto.ErrorCodeGeneric,
-				Err:  fmt.Errorf("response is not in JSON format. error: %v, stderr: %s", err, string(stderr))}
+		logger.Errorf("plugin %s execution status: %v", req.Command(), err)
+
+		if len(stderr) == 0 {
+			// if stderr is empty, it is possible that the plugin is not
+			// running properly.
+			return PluginExecutableFileError(fmt.Errorf("failed to execute the %s command for plugin %s: %w", req.Command(), pluginName, err))
+		} else {
+			var re proto.RequestError
+			jsonErr := json.Unmarshal(stderr, &re)
+			if jsonErr != nil {
+				return &PluginMalformedError{
+					Msg:        fmt.Sprintf("failed to execute the %s command for plugin %s: %s", req.Command(), pluginName, strings.TrimSuffix(string(stderr), "\n")),
+					InnerError: jsonErr,
+				}
+			}
+			return fmt.Errorf("failed to execute the %s command for plugin %s: %w", req.Command(), pluginName, re)
 		}
-		return re
 	}
 
 	logger.Debugf("Plugin %s response: %s", req.Command(), string(stdout))
 	// deserialize response
-	err = json.Unmarshal(stdout, resp)
-	if err != nil {
-		return fmt.Errorf("failed to decode json response: %w", ErrNotCompliant)
+	if err = json.Unmarshal(stdout, resp); err != nil {
+		return &PluginMalformedError{
+			Msg:        fmt.Sprintf("plugin %s response for %s command isn't compliant with Notation plugin requirement: %s", pluginName, req.Command(), string(stdout)),
+			InnerError: err,
+		}
 	}
 	return nil
 }

diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go
@@ -17,7 +17,6 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"fmt"
 	"os"
 	"reflect"
 	"strconv"
@@ -31,14 +30,12 @@ func TestGetMetadata(t *testing.T) {
 	t.Run("plugin error is in invalid json format", func(t *testing.T) {
 		exitErr := errors.New("unknown error")
 		stderr := []byte("sad")
-		wantErr := proto.RequestError{
-			Code: proto.ErrorCodeGeneric,
-			Err:  fmt.Errorf("response is not in JSON format. error: %v, stderr: %s", exitErr, string(stderr))}
-		plugin := CLIPlugin{}
+		expectedErrMsg := "failed to execute the get-plugin-metadata command for plugin test-plugin: sad"
+		plugin := CLIPlugin{name: "test-plugin"}
 		executor = testCommander{stdout: nil, stderr: stderr, err: exitErr}
 		_, err := plugin.GetMetadata(context.Background(), &proto.GetMetadataRequest{})
-		if !errors.Is(err, wantErr) {
-			t.Fatalf("should error. got err = %v, want %v", err, wantErr)
+		if err.Error() != expectedErrMsg {
+			t.Fatalf("should error. got err = %v, want %v", err, expectedErrMsg)
 		}
 	})
 
@@ -58,14 +55,12 @@ func TestGetMetadata(t *testing.T) {
 	t.Run("plugin cause system error", func(t *testing.T) {
 		exitErr := errors.New("system error")
 		stderr := []byte("")
-		wantErr := proto.RequestError{
-			Code: proto.ErrorCodeGeneric,
-			Err:  fmt.Errorf("response is not in JSON format. error: %v, stderr: %s", exitErr, string(stderr))}
-		plugin := CLIPlugin{}
+		expectedErrMsg := "failed to execute the get-plugin-metadata command for plugin test-plugin: system error"
+		plugin := CLIPlugin{name: "test-plugin"}
 		executor = testCommander{stdout: nil, stderr: stderr, err: exitErr}
 		_, err := plugin.GetMetadata(context.Background(), &proto.GetMetadataRequest{})
-		if !errors.Is(err, wantErr) {
-			t.Fatalf("should error. got err = %v, want %v", err, wantErr)
+		if err.Error() != expectedErrMsg {
+			t.Fatalf("should error. got err = %v, want %v", err, expectedErrMsg)
 		}
 	})
 
@@ -199,9 +194,10 @@ func TestNewCLIPlugin_PathError(t *testing.T) {
 	})
 
 	t.Run("plugin is not a regular file", func(t *testing.T) {
+		expectedErrMsg := "plugin executable file is not a regular file"
 		p, err := NewCLIPlugin(ctx, "badplugin", "./testdata/plugins/badplugin/notation-badplugin")
-		if !errors.Is(err, ErrNotRegularFile) {
-			t.Errorf("NewCLIPlugin() error = %v, want %v", err, ErrNotRegularFile)
+		if err.Error() != expectedErrMsg {
+			t.Errorf("NewCLIPlugin() error = %v, want %v", err, expectedErrMsg)
 		}
 		if p != nil {
 			t.Errorf("NewCLIPlugin() plugin = %v, want nil", p)
@@ -218,32 +214,32 @@ func TestNewCLIPlugin_ValidError(t *testing.T) {
 	t.Run("command no response", func(t *testing.T) {
 		executor = testCommander{}
 		_, err := p.GetMetadata(ctx, &proto.GetMetadataRequest{})
-		if !strings.Contains(err.Error(), ErrNotCompliant.Error()) {
-			t.Fatal("should fail the operation.")
+		if _, ok := err.(*PluginMalformedError); !ok {
+			t.Fatal("should return plugin validity error")
 		}
 	})
 
 	t.Run("invalid json", func(t *testing.T) {
 		executor = testCommander{stdout: []byte("content")}
 		_, err := p.GetMetadata(ctx, &proto.GetMetadataRequest{})
-		if !strings.Contains(err.Error(), ErrNotCompliant.Error()) {
-			t.Fatal("should fail the operation.")
+		if _, ok := err.(*PluginMalformedError); !ok {
+			t.Fatal("should return plugin validity error")
 		}
 	})
 
 	t.Run("invalid metadata name", func(t *testing.T) {
 		executor = testCommander{stdout: metadataJSON(invalidMetadataName)}
 		_, err := p.GetMetadata(ctx, &proto.GetMetadataRequest{})
-		if !strings.Contains(err.Error(), "executable name must be") {
+		if !strings.Contains(err.Error(), "executable file name must be") {
 			t.Fatal("should fail the operation.")
 		}
 	})
 
 	t.Run("invalid metadata content", func(t *testing.T) {
 		executor = testCommander{stdout: metadataJSON(proto.GetMetadataResponse{Name: "foo"})}
 		_, err := p.GetMetadata(ctx, &proto.GetMetadataRequest{})
-		if !strings.Contains(err.Error(), "invalid metadata") {
-			t.Fatal("should fail the operation.")
+		if _, ok := err.(*PluginMalformedError); !ok {
+			t.Fatal("should be plugin validity error.")
 		}
 	})