Plugins: Improve grafana-cli UX + API response messaging for plugin install incompatibility scenario (#36556) (#36692)

* improve UX for plugin install incompatability

* refactor test

(cherry picked from commit e06335ffe9)

Co-authored-by: Will Browne <wbrowne@users.noreply.github.com>

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) {
+		var clientError installer.Response4xxError
-			return response.Error(http.StatusNotFound, "Plugin not found", err)
+		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)

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 {
+type Response4xxError struct {
-	Message string
+	Message    string
-	Status  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) {
+		return Plugin{}, err
-			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)
 	}
 
 	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

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"))
+		status, body := makePostRequest(t, grafanaAPIURL(usernameNonAdmin, grafanaListedAddr, "plugins/grafana-plugin/install"))
-		assert.Equal(t, 403, statusCode)
+		assert.Equal(t, 403, status)
-		assert.JSONEq(t, "{\"message\": \"Permission denied\"}", body)
+		assert.Equal(t, "Permission denied", body["message"])
 
-		statusCode, body = makePostRequest(t, grafanaAPIURL(usernameNonAdmin, grafanaListedAddr, "plugins/grafana-plugin/uninstall"))
+		status, body = makePostRequest(t, grafanaAPIURL(usernameNonAdmin, grafanaListedAddr, "plugins/grafana-plugin/uninstall"))
-		assert.Equal(t, 403, statusCode)
+		assert.Equal(t, 403, status)
-		assert.JSONEq(t, "{\"message\": \"Permission denied\"}", body)
+		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 {