From 86bf3b2f25bac08cc757221d20075194ba8eef17 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Fri, 12 Jan 2024 10:27:12 +0800 Subject: [PATCH 01/18] fix: update error message Signed-off-by: Junjie Gao --- plugin/errors.go | 76 +++++++++++++++++++++++++++++++++++++++++++++++ plugin/manager.go | 2 +- plugin/plugin.go | 34 +++++++++++++-------- 3 files changed, 99 insertions(+), 13 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index 2a87f140..1d8bd841 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -47,3 +47,79 @@ func (e InstallEqualVersionError) Error() string { } return "installing plugin with version equal to the existing plugin version" } + +// PluginLibraryInternalError is used when there is an issue executing a plugin +type PluginLibraryInternalError struct { + Msg string + InnerError error +} + +func (e PluginLibraryInternalError) Error() string { + if e.Msg != "" { + return e.Msg + } + if e.InnerError != nil { + return e.InnerError.Error() + } + return "plugin library internal error" +} + +func (e PluginLibraryInternalError) Unwrap() error { + return e.InnerError +} + +// PluginMetadataValidationError is used when there is an issue with plugin metadata validation +type PluginMetadataValidationError struct { + Msg string + InnerError error +} + +func (e PluginMetadataValidationError) Error() string { + if e.Msg != "" { + return e.Msg + } + if e.InnerError != nil { + return e.InnerError.Error() + } + return "metadata validation error" +} + +func (e PluginMetadataValidationError) Unwrap() error { + return e.InnerError +} + +// PluginProtocolError is used when there is an issue with JSON serialization/deserialization +type PluginProtocolError struct { + Msg string + InnerError error +} + +func (e PluginProtocolError) Error() string { + if e.Msg != "" { + return e.Msg + } + if e.InnerError != nil { + return e.InnerError.Error() + } + return "plugin protocol error" +} + +func (e PluginProtocolError) Unwrap() error { + return e.InnerError +} + +// PluginListError is used when there is an issue with listing plugins +type PluginListError struct { + Err error +} + +func (e PluginListError) Error() string { + if e.Err != nil { + return e.Err.Error() + } + return "plugin list error" +} + +func (e PluginListError) Unwrap() error { + return errors.Unwrap(e.Err) +} diff --git a/plugin/manager.go b/plugin/manager.go index f43907cb..9524b923 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -64,7 +64,7 @@ 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 != nil { - return err + return &PluginListError{fmt.Errorf("failed to list plugin: %w", err)} } if dir == "." { // Ignore root dir. diff --git a/plugin/plugin.go b/plugin/plugin.go index c9a77a7b..b45e2394 100644 --- a/plugin/plugin.go +++ b/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("the plugin executable file is not found or inaccessible: %w", err) } if !fi.Mode().IsRegular() { // Ignore non-regular files. - return nil, ErrNotRegularFile + return nil, fmt.Errorf("the plugin executable file %q 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, &PluginMetadataValidationError{ + Msg: fmt.Sprintf("validate metadata 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,37 @@ 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 PluginLibraryInternalError{ + Msg: fmt.Sprintf("failed to execute the %s command for %s plugin", req.Command(), pluginName), + InnerError: 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)) + logger.Errorf("plugin %s execution status: %v", req.Command(), err) + logger.Errorf("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))} + return PluginProtocolError{ + Msg: fmt.Sprintf("failed to execute the %s command for %s plugin, and the error response isn't compliant with Notation plugin protocol: %s", req.Command(), pluginName, string(stderr)), + InnerError: jsonErr, + } } - return re + return fmt.Errorf("failed to execute the %s command for %s plugin: %w", req.Command(), pluginName, 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) + return PluginProtocolError{ + Msg: fmt.Sprintf("%s plugin response for %s command isn't compliant with Notation plugin protocol: %s", pluginName, req.Command(), string(stdout)), + InnerError: err, + } } return nil } From c572faa3fd1d041bf0e3c684fbaa12573b1e3fce Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Fri, 12 Jan 2024 11:07:15 +0800 Subject: [PATCH 02/18] fix: improve error message Signed-off-by: Junjie Gao --- plugin/manager.go | 6 +++--- plugin/plugin.go | 23 ++++++++++++++--------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/plugin/manager.go b/plugin/manager.go index 9524b923..f10b3815 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -146,11 +146,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 +163,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 { diff --git a/plugin/plugin.go b/plugin/plugin.go index b45e2394..2493fcf7 100644 --- a/plugin/plugin.go +++ b/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, fmt.Errorf("the plugin executable file is not found or inaccessible: %w", err) + return nil, fmt.Errorf("failed to create new CLI plugin: the plugin executable file is not found or inaccessible: %w", err) } if !fi.Mode().IsRegular() { // Ignore non-regular files. - return nil, fmt.Errorf("the plugin executable file %q is not a regular file", path) + return nil, fmt.Errorf("failed to create new CLI plugin: the plugin executable file %q is not a regular file", path) } // generate plugin @@ -186,15 +186,20 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re if err != nil { logger.Errorf("plugin %s execution status: %v", req.Command(), err) logger.Errorf("Plugin %s returned error: %s", req.Command(), string(stderr)) - var re proto.RequestError - jsonErr := json.Unmarshal(stderr, &re) - if jsonErr != nil { - return PluginProtocolError{ - Msg: fmt.Sprintf("failed to execute the %s command for %s plugin, and the error response isn't compliant with Notation plugin protocol: %s", req.Command(), pluginName, string(stderr)), - InnerError: jsonErr, + + if len(stderr) == 0 { + return 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 PluginProtocolError{ + Msg: fmt.Sprintf("failed to execute the %s command for %s plugin: the error response isn't compliant with Notation plugin protocol: %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 fmt.Errorf("failed to execute the %s command for %s plugin: %w", req.Command(), pluginName, re) } logger.Debugf("Plugin %s response: %s", req.Command(), string(stdout)) From bda230ac4c4fc7a74252adc4e791338ad439eb25 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Fri, 12 Jan 2024 13:10:16 +0800 Subject: [PATCH 03/18] fix: update error message Signed-off-by: Junjie Gao --- plugin/errors.go | 43 +++++++++++++------------------------------ plugin/manager.go | 8 +++++--- plugin/plugin.go | 11 +++++------ 3 files changed, 23 insertions(+), 39 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index 1d8bd841..a7f93e56 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -48,67 +48,50 @@ func (e InstallEqualVersionError) Error() string { return "installing plugin with version equal to the existing plugin version" } -// PluginLibraryInternalError is used when there is an issue executing a plugin -type PluginLibraryInternalError struct { +// 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 } -func (e PluginLibraryInternalError) Error() string { +func (e PluginUnknownError) Error() string { if e.Msg != "" { return e.Msg } if e.InnerError != nil { return e.InnerError.Error() } - return "plugin library internal error" + return "plugin unknown error" } -func (e PluginLibraryInternalError) Unwrap() error { +func (e PluginUnknownError) Unwrap() error { return e.InnerError } -// PluginMetadataValidationError is used when there is an issue with plugin metadata validation -type PluginMetadataValidationError struct { +// PluginValidityError is used when there is an issue with plugin validity and +// should be fixed by plugin developers. +type PluginValidityError struct { Msg string InnerError error } -func (e PluginMetadataValidationError) Error() string { +func (e PluginValidityError) Error() string { if e.Msg != "" { return e.Msg } if e.InnerError != nil { return e.InnerError.Error() } - return "metadata validation error" + return "plugin validity error" } -func (e PluginMetadataValidationError) Unwrap() error { - return e.InnerError -} - -// PluginProtocolError is used when there is an issue with JSON serialization/deserialization -type PluginProtocolError struct { - Msg string - InnerError error -} - -func (e PluginProtocolError) Error() string { - if e.Msg != "" { - return e.Msg - } - if e.InnerError != nil { - return e.InnerError.Error() - } - return "plugin protocol error" -} - -func (e PluginProtocolError) Unwrap() error { +func (e PluginValidityError) Unwrap() error { return e.InnerError } // PluginListError is used when there is an issue with listing plugins +// and should suggest user to check the plugin directory. type PluginListError struct { Err error } diff --git a/plugin/manager.go b/plugin/manager.go index f10b3815..cc4563ab 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -62,9 +62,9 @@ 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 &PluginListError{fmt.Errorf("failed to list plugin: %w", err)} + return err } if dir == "." { // Ignore root dir. @@ -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, &PluginListError{Err: fmt.Errorf("failed to list plugin: %w", err)} + } return plugins, nil } diff --git a/plugin/plugin.go b/plugin/plugin.go index 2493fcf7..55df8d98 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -105,7 +105,7 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque } // validate metadata if err = validate(&metadata); err != nil { - return nil, &PluginMetadataValidationError{ + return nil, &PluginValidityError{ Msg: fmt.Sprintf("validate metadata failed for %s plugin: %s", p.name, err), InnerError: err, } @@ -175,7 +175,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re data, err := json.Marshal(req) if err != nil { logger.Errorf("failed to marshal request: %+v", req) - return PluginLibraryInternalError{ + return PluginUnknownError{ Msg: fmt.Sprintf("failed to execute the %s command for %s plugin", req.Command(), pluginName), InnerError: err} } @@ -193,7 +193,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re var re proto.RequestError jsonErr := json.Unmarshal(stderr, &re) if jsonErr != nil { - return PluginProtocolError{ + return &PluginValidityError{ Msg: fmt.Sprintf("failed to execute the %s command for %s plugin: the error response isn't compliant with Notation plugin protocol: %s", req.Command(), pluginName, string(stderr)), InnerError: jsonErr, } @@ -204,9 +204,8 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re logger.Debugf("Plugin %s response: %s", req.Command(), string(stdout)) // deserialize response - err = json.Unmarshal(stdout, resp) - if err != nil { - return PluginProtocolError{ + if err = json.Unmarshal(stdout, resp); err != nil { + return &PluginValidityError{ Msg: fmt.Sprintf("%s plugin response for %s command isn't compliant with Notation plugin protocol: %s", pluginName, req.Command(), string(stdout)), InnerError: err, } From 513f5ad92c39c2f552f8d11ee84ebf9ecd479693 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Fri, 12 Jan 2024 13:58:25 +0800 Subject: [PATCH 04/18] test: update test Signed-off-by: Junjie Gao --- plugin/manager_test.go | 4 ++-- plugin/plugin.go | 4 ++-- plugin/plugin_test.go | 38 +++++++++++++++++--------------------- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/plugin/manager_test.go b/plugin/manager_test.go index f0937cd6..9da92cd6 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 := "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) diff --git a/plugin/plugin.go b/plugin/plugin.go index 55df8d98..201bf492 100644 --- a/plugin/plugin.go +++ b/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, fmt.Errorf("failed to create new CLI plugin: the plugin executable file is not found or inaccessible: %w", err) + return nil, fmt.Errorf("failed to instantiate the plugin: the executable file is not found or inaccessible: %w", err) } if !fi.Mode().IsRegular() { // Ignore non-regular files. - return nil, fmt.Errorf("failed to create new CLI plugin: the plugin executable file %q is not a regular file", path) + return nil, fmt.Errorf("failed to instantiate the plugin: the executable file %q is not a regular file", path) } // generate plugin diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 45a43fc4..3a16e370 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 test-plugin plugin: the error response isn't compliant with Notation plugin protocol: 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 := `failed to instantiate the plugin: 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.(*PluginValidityError); !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.(*PluginValidityError); !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.(*PluginValidityError); !ok { + t.Fatal("should be plugin validity error.") } }) From ef779214eb7cb7dc5eac5af766660c55add68421 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Fri, 12 Jan 2024 14:02:44 +0800 Subject: [PATCH 05/18] fix: update error type Signed-off-by: Junjie Gao --- plugin/errors.go | 10 +++++----- plugin/manager.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index a7f93e56..cebead7f 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -90,19 +90,19 @@ func (e PluginValidityError) Unwrap() error { return e.InnerError } -// PluginListError is used when there is an issue with listing plugins +// PluginDirectoryError is used when there is an issue with plugins directory // and should suggest user to check the plugin directory. -type PluginListError struct { +type PluginDirectoryError struct { Err error } -func (e PluginListError) Error() string { +func (e PluginDirectoryError) Error() string { if e.Err != nil { return e.Err.Error() } - return "plugin list error" + return "plugin directory error" } -func (e PluginListError) Unwrap() error { +func (e PluginDirectoryError) Unwrap() error { return errors.Unwrap(e.Err) } diff --git a/plugin/manager.go b/plugin/manager.go index cc4563ab..7d938085 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -80,7 +80,7 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { plugins = append(plugins, d.Name()) return fs.SkipDir }); err != nil { - return nil, &PluginListError{Err: fmt.Errorf("failed to list plugin: %w", err)} + return nil, &PluginDirectoryError{Err: fmt.Errorf("failed to list plugin: %w", err)} } return plugins, nil } From c212a7577a09433ae5c1250fd7a43d1a02941d34 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 15 Jan 2024 10:34:20 +0800 Subject: [PATCH 06/18] fix: resolve comments Signed-off-by: Junjie Gao --- plugin/errors.go | 33 ++++++++++++--------------------- plugin/manager.go | 2 +- plugin/plugin.go | 12 ++++++------ plugin/plugin_test.go | 8 ++++---- 4 files changed, 23 insertions(+), 32 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index cebead7f..e547fa92 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -59,50 +59,41 @@ func (e PluginUnknownError) Error() string { if e.Msg != "" { return e.Msg } - if e.InnerError != nil { - return e.InnerError.Error() - } - return "plugin unknown error" + return e.InnerError.Error() } func (e PluginUnknownError) Unwrap() error { return e.InnerError } -// PluginValidityError is used when there is an issue with plugin validity and +// PluginMalformedError is used when there is an issue with plugin and // should be fixed by plugin developers. -type PluginValidityError struct { +type PluginMalformedError struct { Msg string InnerError error } -func (e PluginValidityError) Error() string { +func (e PluginMalformedError) Error() string { if e.Msg != "" { return e.Msg } - if e.InnerError != nil { - return e.InnerError.Error() - } - return "plugin validity error" + return e.InnerError.Error() } -func (e PluginValidityError) Unwrap() error { +func (e PluginMalformedError) Unwrap() error { return e.InnerError } -// PluginDirectoryError is used when there is an issue with plugins directory -// and should suggest user to check the plugin directory. -type PluginDirectoryError struct { +// 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 } -func (e PluginDirectoryError) Error() string { - if e.Err != nil { - return e.Err.Error() - } - return "plugin directory error" +func (e PluginDirectryWalkError) Error() string { + return e.Err.Error() } -func (e PluginDirectoryError) Unwrap() error { +func (e PluginDirectryWalkError) Unwrap() error { return errors.Unwrap(e.Err) } diff --git a/plugin/manager.go b/plugin/manager.go index 7d938085..9e3e70d9 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -80,7 +80,7 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { plugins = append(plugins, d.Name()) return fs.SkipDir }); err != nil { - return nil, &PluginDirectoryError{Err: fmt.Errorf("failed to list plugin: %w", err)} + return nil, &PluginDirectryWalkError{Err: fmt.Errorf("failed to list plugin: %w", err)} } return plugins, nil } diff --git a/plugin/plugin.go b/plugin/plugin.go index 201bf492..2d3e2d97 100644 --- a/plugin/plugin.go +++ b/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, fmt.Errorf("failed to instantiate the plugin: the executable file is not found or inaccessible: %w", 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, fmt.Errorf("failed to instantiate the plugin: the executable file %q is not a regular file", path) + return nil, fmt.Errorf("plugin instantiation failed because the executable file %s is not a regular file", path) } // generate plugin @@ -105,8 +105,8 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque } // validate metadata if err = validate(&metadata); err != nil { - return nil, &PluginValidityError{ - Msg: fmt.Sprintf("validate metadata failed for %s plugin: %s", p.name, err), + return nil, &PluginMalformedError{ + Msg: fmt.Sprintf("metadata validation failed for %s plugin: %s", p.name, err), InnerError: err, } } @@ -193,7 +193,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re var re proto.RequestError jsonErr := json.Unmarshal(stderr, &re) if jsonErr != nil { - return &PluginValidityError{ + return &PluginMalformedError{ Msg: fmt.Sprintf("failed to execute the %s command for %s plugin: the error response isn't compliant with Notation plugin protocol: %s", req.Command(), pluginName, string(stderr)), InnerError: jsonErr, } @@ -205,7 +205,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re logger.Debugf("Plugin %s response: %s", req.Command(), string(stdout)) // deserialize response if err = json.Unmarshal(stdout, resp); err != nil { - return &PluginValidityError{ + return &PluginMalformedError{ Msg: fmt.Sprintf("%s plugin response for %s command isn't compliant with Notation plugin protocol: %s", pluginName, req.Command(), string(stdout)), InnerError: err, } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 3a16e370..98e2074e 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -194,7 +194,7 @@ func TestNewCLIPlugin_PathError(t *testing.T) { }) t.Run("plugin is not a regular file", func(t *testing.T) { - expectedErrMsg := `failed to instantiate the plugin: the executable file "./testdata/plugins/badplugin/notation-badplugin" is not a regular file` + 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 err.Error() != expectedErrMsg { t.Errorf("NewCLIPlugin() error = %v, want %v", err, expectedErrMsg) @@ -214,7 +214,7 @@ func TestNewCLIPlugin_ValidError(t *testing.T) { t.Run("command no response", func(t *testing.T) { executor = testCommander{} _, err := p.GetMetadata(ctx, &proto.GetMetadataRequest{}) - if _, ok := err.(*PluginValidityError); !ok { + if _, ok := err.(*PluginMalformedError); !ok { t.Fatal("should return plugin validity error") } }) @@ -222,7 +222,7 @@ func TestNewCLIPlugin_ValidError(t *testing.T) { t.Run("invalid json", func(t *testing.T) { executor = testCommander{stdout: []byte("content")} _, err := p.GetMetadata(ctx, &proto.GetMetadataRequest{}) - if _, ok := err.(*PluginValidityError); !ok { + if _, ok := err.(*PluginMalformedError); !ok { t.Fatal("should return plugin validity error") } }) @@ -238,7 +238,7 @@ 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 _, ok := err.(*PluginValidityError); !ok { + if _, ok := err.(*PluginMalformedError); !ok { t.Fatal("should be plugin validity error.") } }) From b93431b612393d998c69bedde07d7a1673653c92 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 15 Jan 2024 10:38:57 +0800 Subject: [PATCH 07/18] fix: resolve comment Signed-off-by: Junjie Gao --- plugin/plugin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/plugin.go b/plugin/plugin.go index 2d3e2d97..aec54815 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -177,7 +177,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re 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: err} + InnerError: fmt.Errorf("failed to marshal request: %w", err)} } logger.Debugf("Plugin %s request: %s", req.Command(), string(data)) From ceb6b3e18de1f4a382fc32cc0179c4590fa3c12a Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 15 Jan 2024 13:15:49 +0800 Subject: [PATCH 08/18] fix: update error message Signed-off-by: Junjie Gao --- plugin/plugin.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/plugin/plugin.go b/plugin/plugin.go index aec54815..e2f3e163 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -188,7 +188,10 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re logger.Errorf("Plugin %s returned error: %s", req.Command(), string(stderr)) if len(stderr) == 0 { - return fmt.Errorf("failed to execute the %s command for %s plugin: %w", req.Command(), pluginName, err) + return &PluginMalformedError{ + Msg: fmt.Sprintf("failed to execute the %s command for %s plugin: %s", req.Command(), pluginName, string(stderr)), + InnerError: err, + } } else { var re proto.RequestError jsonErr := json.Unmarshal(stderr, &re) From 92d0417575cee4d05e6e001b5d0f2215fa168882 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 15 Jan 2024 13:21:36 +0800 Subject: [PATCH 09/18] fix: update error message Signed-off-by: Junjie Gao --- plugin/plugin.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugin/plugin.go b/plugin/plugin.go index e2f3e163..92aa994a 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -189,7 +189,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re if len(stderr) == 0 { return &PluginMalformedError{ - Msg: fmt.Sprintf("failed to execute the %s command for %s plugin: %s", req.Command(), pluginName, string(stderr)), + Msg: fmt.Sprintf("failed to execute the %s command for %s plugin: %s", req.Command(), pluginName, err), InnerError: err, } } else { From 36cb0a66c0e5dd958ba1c135bf61a46746816fdd Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Mon, 15 Jan 2024 16:01:01 +0800 Subject: [PATCH 10/18] fix: add error message type PluginExectableFileError Signed-off-by: Junjie Gao --- plugin/errors.go | 25 +++++++++++++++++++++++++ plugin/plugin.go | 11 ++++++----- plugin/plugin_test.go | 2 +- 3 files changed, 32 insertions(+), 6 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index e547fa92..dca9d899 100644 --- a/plugin/errors.go +++ b/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,6 +42,7 @@ type InstallEqualVersionError struct { Msg string } +// Error returns the error message. func (e InstallEqualVersionError) Error() string { if e.Msg != "" { return e.Msg @@ -55,6 +57,7 @@ type PluginUnknownError struct { InnerError error } +// Error returns the error message. func (e PluginUnknownError) Error() string { if e.Msg != "" { return e.Msg @@ -62,6 +65,7 @@ func (e PluginUnknownError) Error() string { return e.InnerError.Error() } +// Unwrap returns the inner error. func (e PluginUnknownError) Unwrap() error { return e.InnerError } @@ -73,6 +77,7 @@ type PluginMalformedError struct { InnerError error } +// Error returns the error message. func (e PluginMalformedError) Error() string { if e.Msg != "" { return e.Msg @@ -80,6 +85,7 @@ func (e PluginMalformedError) Error() string { return e.InnerError.Error() } +// Unwrap returns the inner error. func (e PluginMalformedError) Unwrap() error { return e.InnerError } @@ -90,10 +96,29 @@ 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) +} diff --git a/plugin/plugin.go b/plugin/plugin.go index 92aa994a..173a5efa 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -188,16 +188,17 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re logger.Errorf("Plugin %s returned error: %s", req.Command(), string(stderr)) if len(stderr) == 0 { - return &PluginMalformedError{ - Msg: fmt.Sprintf("failed to execute the %s command for %s plugin: %s", req.Command(), pluginName, err), - InnerError: err, + // 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 protocol: %s", req.Command(), pluginName, string(stderr)), + 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, } } @@ -209,7 +210,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re // deserialize response 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 protocol: %s", pluginName, req.Command(), string(stdout)), + Msg: fmt.Sprintf("%s plugin response for %s command isn't compliant with Notation plugin requirement: %s", pluginName, req.Command(), string(stdout)), InnerError: err, } } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 98e2074e..17e9e15a 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -30,7 +30,7 @@ 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") - expectedErrMsg := "failed to execute the get-plugin-metadata command for test-plugin plugin: the error response isn't compliant with Notation plugin protocol: sad" + 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{}) From ddc1153e88ed263e040f6e631395c9e9ebf143d0 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 16 Jan 2024 10:02:35 +0800 Subject: [PATCH 11/18] fix: resolve comments Signed-off-by: Junjie Gao --- plugin/errors.go | 4 ++-- plugin/plugin.go | 4 ++-- plugin/plugin_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index dca9d899..c178c006 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -103,7 +103,7 @@ func (e PluginDirectryWalkError) Error() string { // Unwrap returns the inner error. func (e PluginDirectryWalkError) Unwrap() error { - return errors.Unwrap(e.Err) + return e.Err } // PluginExectableFileError is used when there is an issue with plugin @@ -120,5 +120,5 @@ func (e PluginExectableFileError) Error() string { // Unwrap returns the inner error. func (e PluginExectableFileError) Unwrap() error { - return errors.Unwrap(e.Err) + return e.Err } diff --git a/plugin/plugin.go b/plugin/plugin.go index 173a5efa..cda34655 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -85,7 +85,7 @@ func NewCLIPlugin(ctx context.Context, name, path string) (*CLIPlugin, error) { } if !fi.Mode().IsRegular() { // Ignore non-regular files. - return nil, fmt.Errorf("plugin instantiation failed because the executable file %s is not a regular file", path) + return nil, fmt.Errorf("plugin instantiation failed for the executable file %s: %w", path, ErrNotRegularFile) } // generate plugin @@ -175,7 +175,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re data, err := json.Marshal(req) if err != nil { logger.Errorf("failed to marshal request: %+v", req) - return PluginUnknownError{ + 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)} } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 17e9e15a..f5683afd 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -194,7 +194,7 @@ 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` + expectedErrMsg := `plugin instantiation failed for the executable file ./testdata/plugins/badplugin/notation-badplugin: not regular file` p, err := NewCLIPlugin(ctx, "badplugin", "./testdata/plugins/badplugin/notation-badplugin") if err.Error() != expectedErrMsg { t.Errorf("NewCLIPlugin() error = %v, want %v", err, expectedErrMsg) From b6c342cde4f05ad85a84ebfba9cd3ea94758d59e Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 16 Jan 2024 11:47:19 +0800 Subject: [PATCH 12/18] fix: resolve comments Signed-off-by: Junjie Gao --- plugin/errors.go | 30 +++--------------------------- plugin/manager.go | 2 +- plugin/manager_test.go | 4 ++-- plugin/plugin.go | 19 ++++++++----------- plugin/plugin_test.go | 6 +++--- 5 files changed, 17 insertions(+), 44 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index c178c006..08776ef0 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -92,33 +92,9 @@ func (e PluginMalformedError) Unwrap() error { // 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 e.Err -} +type PluginDirectryWalkError error -// PluginExectableFileError is used when there is an issue with plugin +// 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 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 e.Err -} +type PluginExecutableFileError error diff --git a/plugin/manager.go b/plugin/manager.go index 9e3e70d9..b3bc5caa 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -80,7 +80,7 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { plugins = append(plugins, d.Name()) return fs.SkipDir }); err != nil { - return nil, &PluginDirectryWalkError{Err: fmt.Errorf("failed to list plugin: %w", err)} + return nil, PluginDirectryWalkError(fmt.Errorf("failed to list plugin: %w", err)) } return plugins, nil } diff --git a/plugin/manager_test.go b/plugin/manager_test.go index 9da92cd6..dfe1df29 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 := "plugin executable name must be \"notation-foobar\" instead of \"notation-bar\"" + expectedErrorMsg := "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 := "plugin executable name must be \"notation-bar\" instead of \"notation-foo\"" + expectedErrorMsg := "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 cda34655..b8af0cf6 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -106,12 +106,12 @@ func (p *CLIPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataReque // validate metadata if err = validate(&metadata); err != nil { return nil, &PluginMalformedError{ - Msg: fmt.Sprintf("metadata validation failed for %s plugin: %s", p.name, err), + Msg: fmt.Sprintf("metadata validation failed for plugin %s: %s", p.name, err), InnerError: err, } } if metadata.Name != p.name { - return nil, fmt.Errorf("plugin 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 } @@ -174,9 +174,9 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re // serialize request data, err := json.Marshal(req) if err != nil { - logger.Errorf("failed to marshal request: %+v", req) + 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), + Msg: fmt.Sprintf("failed to execute the %s command for plugin %s", req.Command(), pluginName), InnerError: fmt.Errorf("failed to marshal request: %w", err)} } @@ -185,24 +185,21 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re stdout, stderr, err := executor.Output(ctx, pluginPath, req.Command(), data) if err != nil { 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), - } + 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 %s plugin: the error response isn't compliant with Notation plugin requirement: %s", req.Command(), pluginName, string(stderr)), + Msg: fmt.Sprintf("failed to execute the %s command for plugin %s: the plugin 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 fmt.Errorf("failed to execute the %s command for plugin %s: %w", req.Command(), pluginName, re) } } @@ -210,7 +207,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re // deserialize response 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)), + Msg: fmt.Sprintf("plugin %s response for %s command isn't compliant with Notation plugin requirement: %s", pluginName, req.Command(), string(stdout)), InnerError: err, } } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index f5683afd..b530b71c 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -30,7 +30,7 @@ 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") - expectedErrMsg := "failed to execute the get-plugin-metadata command for test-plugin plugin: the error response isn't compliant with Notation plugin requirement: sad" + expectedErrMsg := "failed to execute the get-plugin-metadata command for plugin test-plugin: the plugin 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{}) @@ -55,7 +55,7 @@ func TestGetMetadata(t *testing.T) { t.Run("plugin cause system error", func(t *testing.T) { exitErr := errors.New("system error") stderr := []byte("") - expectedErrMsg := "failed to execute the get-plugin-metadata command for test-plugin plugin: system error" + 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{}) @@ -230,7 +230,7 @@ func TestNewCLIPlugin_ValidError(t *testing.T) { 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.") } }) From eef1cd904f744a045061449d0cd780d3c79cadd5 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 16 Jan 2024 14:39:00 +0800 Subject: [PATCH 13/18] fix: update Signed-off-by: Junjie Gao --- plugin/errors.go | 2 +- plugin/plugin.go | 4 ++-- plugin/plugin_test.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index 08776ef0..f6ee53bb 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 regular file") // PluginDowngradeError is returned when installing a plugin with version // lower than the exisiting plugin version. diff --git a/plugin/plugin.go b/plugin/plugin.go index b8af0cf6..b2b027ec 100644 --- a/plugin/plugin.go +++ b/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, fmt.Errorf("plugin instantiation failed because the executable file is either not found or inaccessible: %w", err) + return nil, fmt.Errorf("plugin executable file is either not found or inaccessible: %w", err) } if !fi.Mode().IsRegular() { // Ignore non-regular files. - return nil, fmt.Errorf("plugin instantiation failed for the executable file %s: %w", path, ErrNotRegularFile) + return nil, ErrNotRegularFile } // generate plugin diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index b530b71c..083ef9be 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -194,7 +194,7 @@ func TestNewCLIPlugin_PathError(t *testing.T) { }) t.Run("plugin is not a regular file", func(t *testing.T) { - expectedErrMsg := `plugin instantiation failed for the executable file ./testdata/plugins/badplugin/notation-badplugin: not regular file` + expectedErrMsg := "plugin executable file is not regular file" p, err := NewCLIPlugin(ctx, "badplugin", "./testdata/plugins/badplugin/notation-badplugin") if err.Error() != expectedErrMsg { t.Errorf("NewCLIPlugin() error = %v, want %v", err, expectedErrMsg) From 8f325ac2a572fe151d5f2a2044ca7d871549233c Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 16 Jan 2024 14:56:29 +0800 Subject: [PATCH 14/18] fix: update plugin manager list Signed-off-by: Junjie Gao --- plugin/manager.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugin/manager.go b/plugin/manager.go index b3bc5caa..793bebfd 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -62,6 +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 + // check plugin directory exsitence + if _, err := fs.Stat(m.pluginFS, "."); errors.Is(err, os.ErrNotExist) { + return plugins, nil + } + if err := fs.WalkDir(m.pluginFS, ".", func(dir string, d fs.DirEntry, err error) error { if err != nil { return err From 57228410e32debbbf1bbff495a9f7266cb4aaad0 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 16 Jan 2024 16:16:16 +0800 Subject: [PATCH 15/18] fix: resolve comments Signed-off-by: Junjie Gao --- plugin/errors.go | 4 ++-- plugin/manager.go | 14 ++++++-------- plugin/manager_test.go | 4 ++-- 3 files changed, 10 insertions(+), 12 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index f6ee53bb..a76b3f25 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -90,9 +90,9 @@ func (e PluginMalformedError) Unwrap() error { return e.InnerError } -// PluginDirectryWalkError is used when there is an issue with plugins directory +// PluginDirectoryWalkError is used when there is an issue with plugins directory // and should suggest user to check the permission of plugin directory. -type PluginDirectryWalkError error +type PluginDirectoryWalkError error // PluginExecutableFileError is used when there is an issue with plugin // executable file and should suggest user to check the existence, permission diff --git a/plugin/manager.go b/plugin/manager.go index 793bebfd..ffb80ca3 100644 --- a/plugin/manager.go +++ b/plugin/manager.go @@ -62,13 +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 - // check plugin directory exsitence - if _, err := fs.Stat(m.pluginFS, "."); errors.Is(err, os.ErrNotExist) { - return plugins, nil - } - 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 == "." { @@ -85,7 +83,7 @@ func (m *CLIManager) List(ctx context.Context) ([]string, error) { plugins = append(plugins, d.Name()) return fs.SkipDir }); err != nil { - return nil, PluginDirectryWalkError(fmt.Errorf("failed to list plugin: %w", err)) + return nil, PluginDirectoryWalkError(fmt.Errorf("failed to list plugin: %w", err)) } return plugins, nil } @@ -157,7 +155,7 @@ func (m *CLIManager) Install(ctx context.Context, installOpts CLIInstallOptions) } newPluginMetadata, err := newPlugin.GetMetadata(ctx, &proto.GetMetadataRequest{}) if err != nil { - return nil, nil, err + return nil, nil, fmt.Errorf("failed to get metadata of new plugin: %w", err) } // check plugin existence and get existing plugin metadata var existingPluginMetadata *proto.GetMetadataResponse @@ -170,7 +168,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, err + return nil, nil, fmt.Errorf("failed to get metadata of existing plugin: %w", err) } // existing plugin is valid, and overwrite is not set, check version if !overwrite { diff --git a/plugin/manager_test.go b/plugin/manager_test.go index dfe1df29..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 := "plugin executable file 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 := "plugin executable file 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) From c6cb0f4c65284badaa1d18514f39fcd30b5c3916 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Tue, 16 Jan 2024 16:45:27 +0800 Subject: [PATCH 16/18] fix: resolve comments Signed-off-by: Junjie Gao --- plugin/errors.go | 2 +- plugin/plugin_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index a76b3f25..29629351 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("plugin executable file is 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. diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index 083ef9be..fbcdd76a 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -194,7 +194,7 @@ func TestNewCLIPlugin_PathError(t *testing.T) { }) t.Run("plugin is not a regular file", func(t *testing.T) { - expectedErrMsg := "plugin executable file is not regular file" + expectedErrMsg := "plugin executable file is not a regular file" p, err := NewCLIPlugin(ctx, "badplugin", "./testdata/plugins/badplugin/notation-badplugin") if err.Error() != expectedErrMsg { t.Errorf("NewCLIPlugin() error = %v, want %v", err, expectedErrMsg) From 53fb55633fd8712ca29f41cde34338f82b1f71fb Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 17 Jan 2024 10:54:17 +0800 Subject: [PATCH 17/18] fix: update error message Signed-off-by: Junjie Gao --- plugin/plugin.go | 3 ++- plugin/plugin_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/plugin/plugin.go b/plugin/plugin.go index b2b027ec..62067da9 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" @@ -195,7 +196,7 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re jsonErr := json.Unmarshal(stderr, &re) if jsonErr != nil { return &PluginMalformedError{ - Msg: fmt.Sprintf("failed to execute the %s command for plugin %s: the plugin isn't compliant with Notation plugin requirement: %s", req.Command(), pluginName, string(stderr)), + Msg: fmt.Sprintf("failed to execute the %s command for plugin %s: %s", req.Command(), pluginName, strings.TrimSuffix(string(stderr), "\n")), InnerError: jsonErr, } } diff --git a/plugin/plugin_test.go b/plugin/plugin_test.go index fbcdd76a..a6f49a3b 100644 --- a/plugin/plugin_test.go +++ b/plugin/plugin_test.go @@ -30,7 +30,7 @@ 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") - expectedErrMsg := "failed to execute the get-plugin-metadata command for plugin test-plugin: the plugin isn't compliant with Notation plugin requirement: sad" + 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{}) From c2e325beb92c9d7d747f12735445d8bce0b37620 Mon Sep 17 00:00:00 2001 From: Junjie Gao Date: Wed, 17 Jan 2024 11:20:59 +0800 Subject: [PATCH 18/18] fix: update error message Signed-off-by: Junjie Gao --- plugin/errors.go | 20 -------------------- plugin/plugin.go | 6 ++---- 2 files changed, 2 insertions(+), 24 deletions(-) diff --git a/plugin/errors.go b/plugin/errors.go index 29629351..ab7394d0 100644 --- a/plugin/errors.go +++ b/plugin/errors.go @@ -50,26 +50,6 @@ func (e InstallEqualVersionError) Error() string { 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 { diff --git a/plugin/plugin.go b/plugin/plugin.go index 62067da9..be74b097 100644 --- a/plugin/plugin.go +++ b/plugin/plugin.go @@ -175,10 +175,8 @@ func run(ctx context.Context, pluginName string, pluginPath string, req proto.Re // serialize request data, err := json.Marshal(req) if err != nil { - logger.Errorf("Failed to marshal request: %+v", req) - return &PluginUnknownError{ - Msg: fmt.Sprintf("failed to execute the %s command for plugin %s", req.Command(), pluginName), - InnerError: fmt.Errorf("failed to marshal request: %w", 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))