fix: improve plugin error message

plugin/errors.go

@@ -28,6 +28,7 @@ type PluginDowngradeError struct {
 	Msg string
 }
 
+// Error returns the error message.
 func (e PluginDowngradeError) Error() string {
 	if e.Msg != "" {
 		return e.Msg
@@ -41,9 +42,83 @@ 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"
 }
+
+// PluginUnknownError is used when there is an issue executing a plugin
+// and should be reported to Notation developers.
+type PluginUnknownError struct {
+	Msg        string
+	InnerError error
+}
+
+// Error returns the error message.
+func (e PluginUnknownError) Error() string {
+	if e.Msg != "" {
+		return e.Msg
+	}
+	return e.InnerError.Error()
+}
+
+// Unwrap returns the inner error.
+func (e PluginUnknownError) Unwrap() error {
+	return e.InnerError
+}
+
+// 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
+}
+
+// PluginDirectryWalkError is used when there is an issue with plugins directory
+// and should suggest user to check the permission of plugin directory.
+type PluginDirectryWalkError struct {
+	Err error
+}
+
+// Error returns the error message.
+func (e PluginDirectryWalkError) Error() string {
+	return e.Err.Error()
+}
+
+// Unwrap returns the inner error.
+func (e PluginDirectryWalkError) Unwrap() error {
+	return errors.Unwrap(e.Err)
+}
+
+// PluginExectableFileError 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 PluginExectableFileError struct {
+	Err error
+}
+
+// Error returns the error message.
+func (e PluginExectableFileError) Error() string {
+	return e.Err.Error()
+}
+
+// Unwrap returns the inner error.
+func (e PluginExectableFileError) Unwrap() error {
+	return errors.Unwrap(e.Err)
+}

plugin/manager.go

@@ -62,7 +62,7 @@ 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 {
 			return err
 		}
@@ -79,7 +79,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, &PluginDirectryWalkError{Err: fmt.Errorf("failed to list plugin: %w", err)}
+	}
 	return plugins, nil
 }
 
@@ -146,11 +148,11 @@ 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 {
-		return nil, nil, fmt.Errorf("failed to get metadata of new plugin: %w", err)
+		return nil, nil, err
 	}
 	// check plugin existence and get existing plugin metadata
 	var existingPluginMetadata *proto.GetMetadataResponse
@@ -163,7 +165,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions)
 	} else { // plugin already exists
 		existingPluginMetadata, err = existingPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{})
 		if err != nil && !overwrite { // fail only if overwrite is not set
-			return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err)
+			return nil, nil, err
 		}
 		// existing plugin is valid, and overwrite is not set, check version
 		if !overwrite {

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 := "plugin executable 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 := "plugin executable 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)

plugin/plugin.go

@@ -81,11 +81,11 @@ 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 instantiation failed because the executable file is either not found or inaccessible: %w", err)
 	}
 	if !fi.Mode().IsRegular() {
 		// Ignore non-regular files.
-		return nil, ErrNotRegularFile
+		return nil, fmt.Errorf("plugin instantiation failed because the executable file %s is not a regular file", path)
 	}
 
 	// generate plugin
@@ -105,10 +105,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 %s plugin: %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 name must be %q instead of %q", binName(metadata.Name), filepath.Base(p.path))
 	}
 	return &metadata, nil
 }
@@ -171,30 +174,45 @@ 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: %+v", req)
+		return PluginUnknownError{
+			Msg:        fmt.Sprintf("failed to execute the %s command for %s plugin", req.Command(), pluginName),
+			InnerError: fmt.Errorf("failed to marshal request: %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)
+		logger.Errorf("Plugin %s returned error: %s", req.Command(), string(stderr))
+
+		if len(stderr) == 0 {
+			// if stderr is empty, it is possible that the plugin is not
+			// running properly.
+			return &PluginExectableFileError{
+				Err: fmt.Errorf("failed to execute the %s command for %s plugin: %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 %s plugin: the error response isn't compliant with Notation plugin requirement: %s", req.Command(), pluginName, string(stderr)),
+					InnerError: jsonErr,
+				}
+			}
+			return fmt.Errorf("failed to execute the %s command for %s plugin: %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("%s plugin response for %s command isn't compliant with Notation plugin requirement: %s", pluginName, req.Command(), string(stdout)),
+			InnerError: err,
+		}
 	}
 	return nil
 }

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 test-plugin plugin: the error response isn't compliant with Notation plugin requirement: 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 test-plugin 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 instantiation failed because the executable file ./testdata/plugins/badplugin/notation-badplugin 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,16 +214,16 @@ 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")
 		}
 	})
 
@@ -242,8 +238,8 @@ func TestNewCLIPlugin_ValidError(t *testing.T) {
 	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.")
 		}
 	})