From d52ca7162a492277aea8cff5aebcedf4f567f536 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Thu, 18 Jan 2024 09:24:19 +0800 Subject: [PATCH] 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: https://github.com/notaryproject/notation/issues/824 Resolve part of: https://github.com/notaryproject/notation/issues/867 Signed-off-by: Junjie Gao --------- Signed-off-by: Junjie Gao --- plugin/errors.go | 33 +++++++++++++++++++++++++++++- plugin/manager.go | 11 +++++++--- plugin/manager_test.go | 4 ++-- plugin/plugin.go | 46 +++++++++++++++++++++++++++--------------- plugin/plugin_test.go | 40 +++++++++++++++++------------------- 5 files changed, 90 insertions(+), 44 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index 2a87f140..ab7394d0 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -20,7 +20,7 @@ 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. @@ -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,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 index f43907cb..ffb80ca3 100644 --- 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 index f0937cd6..4dd9aa14 100644 --- 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 index c9a77a7b..be74b097 100644 --- 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 index 45a43fc4..a6f49a3b 100644 --- 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,23 +214,23 @@ 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.") } }) @@ -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.") } })