From e06335ffe99cf64ee7ff13ff4d1e3bf9844404d4 Mon Sep 17 00:00:00 2001 From: Will Browne Date: Tue, 13 Jul 2021 00:58:46 -0700 Subject: [PATCH] Plugins: Improve grafana-cli UX + API response messaging for plugin install incompatibility scenario (#36556) * improve UX for plugin install incompatability * refactor test --- pkg/api/plugins.go | 5 ++- pkg/plugins/manager/installer/installer.go | 43 ++++++++++------------ pkg/tests/api/plugins/api_install_test.go | 26 ++++++++----- 3 files changed, 38 insertions(+), 36 deletions(-) diff --git a/pkg/api/plugins.go b/pkg/api/plugins.go index f63ff573ac2..7682eaae8eb 100644 --- a/pkg/api/plugins.go +++ b/pkg/api/plugins.go @@ -387,8 +387,9 @@ func (hs *HTTPServer) InstallPlugin(c *models.ReqContext, dto dtos.InstallPlugin if errors.As(err, &versionNotFoundErr) { return response.Error(http.StatusNotFound, "Plugin version not found", err) } - if errors.Is(err, installer.ErrPluginNotFound) { - return response.Error(http.StatusNotFound, "Plugin not found", err) + var clientError installer.Response4xxError + if errors.As(err, &clientError) { + return response.Error(clientError.StatusCode, clientError.Message, err) } if errors.Is(err, plugins.ErrInstallCorePlugin) { return response.Error(http.StatusForbidden, "Cannot install or change a Core plugin", err) diff --git a/pkg/plugins/manager/installer/installer.go b/pkg/plugins/manager/installer/installer.go index 6e2ef50543a..80afb531873 100644 --- a/pkg/plugins/manager/installer/installer.go +++ b/pkg/plugins/manager/installer/installer.go @@ -41,20 +41,23 @@ const ( ) var ( - ErrPluginNotFound = errors.New("plugin not found") - reGitBuild = regexp.MustCompile("^[a-zA-Z0-9_.-]*/") + reGitBuild = regexp.MustCompile("^[a-zA-Z0-9_.-]*/") ) -type BadRequestError struct { - Message string - Status string +type Response4xxError struct { + Message string + StatusCode int + SystemInfo string } -func (e *BadRequestError) Error() string { +func (e Response4xxError) Error() string { if len(e.Message) > 0 { - return fmt.Sprintf("%s: %s", e.Status, e.Message) + if len(e.SystemInfo) > 0 { + return fmt.Sprintf("%s (%s)", e.Message, e.SystemInfo) + } + return fmt.Sprintf("%d: %s", e.StatusCode, e.Message) } - return e.Status + return fmt.Sprintf("%d", e.StatusCode) } type ErrVersionUnsupported struct { @@ -248,7 +251,7 @@ func (i *Installer) DownloadFile(pluginID string, tmpFile *os.File, url string, // slow network. As this is CLI operation hanging is not a big of an issue as user can just abort. bodyReader, err := i.sendRequestWithoutTimeout(url) if err != nil { - return errutil.Wrap("Failed to send request", err) + return err } defer func() { if err := bodyReader.Close(); err != nil { @@ -274,11 +277,7 @@ func (i *Installer) getPluginMetadataFromPluginRepo(pluginID, pluginRepoURL stri i.log.Debugf("Fetching metadata for plugin \"%s\" from repo %s", pluginID, pluginRepoURL) body, err := i.sendRequestGetBytes(pluginRepoURL, "repo", pluginID) if err != nil { - if errors.Is(err, ErrPluginNotFound) { - i.log.Errorf("failed to find plugin '%s' in plugin repository. Please check if plugin ID is correct", pluginID) - return Plugin{}, err - } - return Plugin{}, errutil.Wrap("Failed to send request", err) + return Plugin{}, err } var data Plugin @@ -354,14 +353,6 @@ func (i *Installer) createRequest(URL string, subPaths ...string) (*http.Request } func (i *Installer) handleResponse(res *http.Response) (io.ReadCloser, error) { - if res.StatusCode == 404 { - return nil, ErrPluginNotFound - } - - if res.StatusCode/100 != 2 && res.StatusCode/100 != 4 { - return nil, fmt.Errorf("API returned invalid status: %s", res.Status) - } - if res.StatusCode/100 == 4 { body, err := ioutil.ReadAll(res.Body) defer func() { @@ -370,7 +361,7 @@ func (i *Installer) handleResponse(res *http.Response) (io.ReadCloser, error) { } }() if err != nil || len(body) == 0 { - return nil, &BadRequestError{Status: res.Status} + return nil, Response4xxError{StatusCode: res.StatusCode} } var message string var jsonBody map[string]string @@ -380,7 +371,11 @@ func (i *Installer) handleResponse(res *http.Response) (io.ReadCloser, error) { } else { message = jsonBody["message"] } - return nil, &BadRequestError{Status: res.Status, Message: message} + return nil, Response4xxError{StatusCode: res.StatusCode, Message: message, SystemInfo: i.fullSystemInfoString()} + } + + if res.StatusCode/100 != 2 { + return nil, fmt.Errorf("API returned invalid status: %s", res.Status) } return res.Body, nil diff --git a/pkg/tests/api/plugins/api_install_test.go b/pkg/tests/api/plugins/api_install_test.go index 532f4028a7e..6b155e09df8 100644 --- a/pkg/tests/api/plugins/api_install_test.go +++ b/pkg/tests/api/plugins/api_install_test.go @@ -3,6 +3,7 @@ package plugins import ( "bytes" "context" + "encoding/json" "fmt" "io/ioutil" "net/http" @@ -36,23 +37,24 @@ func TestPluginInstallAccess(t *testing.T) { createUser(t, store, usernameAdmin, defaultPassword, true) t.Run("Request is forbidden if not from an admin", func(t *testing.T) { - statusCode, body := makePostRequest(t, grafanaAPIURL(usernameNonAdmin, grafanaListedAddr, "plugins/grafana-plugin/install")) - assert.Equal(t, 403, statusCode) - assert.JSONEq(t, "{\"message\": \"Permission denied\"}", body) + status, body := makePostRequest(t, grafanaAPIURL(usernameNonAdmin, grafanaListedAddr, "plugins/grafana-plugin/install")) + assert.Equal(t, 403, status) + assert.Equal(t, "Permission denied", body["message"]) - statusCode, body = makePostRequest(t, grafanaAPIURL(usernameNonAdmin, grafanaListedAddr, "plugins/grafana-plugin/uninstall")) - assert.Equal(t, 403, statusCode) - assert.JSONEq(t, "{\"message\": \"Permission denied\"}", body) + status, body = makePostRequest(t, grafanaAPIURL(usernameNonAdmin, grafanaListedAddr, "plugins/grafana-plugin/uninstall")) + assert.Equal(t, 403, status) + assert.Equal(t, "Permission denied", body["message"]) }) t.Run("Request is not forbidden if from an admin", func(t *testing.T) { statusCode, body := makePostRequest(t, grafanaAPIURL(usernameAdmin, grafanaListedAddr, "plugins/test/install")) + assert.Equal(t, 404, statusCode) - assert.JSONEq(t, "{\"error\":\"plugin not found\", \"message\":\"Plugin not found\"}", body) + assert.Equal(t, "Plugin not found", body["message"]) statusCode, body = makePostRequest(t, grafanaAPIURL(usernameAdmin, grafanaListedAddr, "plugins/test/uninstall")) assert.Equal(t, 404, statusCode) - assert.JSONEq(t, "{\"error\":\"plugin is not installed\", \"message\":\"Plugin not installed\"}", body) + assert.Equal(t, "Plugin not installed", body["message"]) }) } @@ -68,7 +70,7 @@ func createUser(t *testing.T, store *sqlstore.SQLStore, username, password strin require.NoError(t, err) } -func makePostRequest(t *testing.T, URL string) (int, string) { +func makePostRequest(t *testing.T, URL string) (int, map[string]interface{}) { t.Helper() // nolint:gosec @@ -81,7 +83,11 @@ func makePostRequest(t *testing.T, URL string) (int, string) { b, err := ioutil.ReadAll(resp.Body) require.NoError(t, err) - return resp.StatusCode, string(b) + var body = make(map[string]interface{}) + err = json.Unmarshal(b, &body) + require.NoError(t, err) + + return resp.StatusCode, body } func grafanaAPIURL(username string, grafanaListedAddr string, path string) string {