diff --git a/pkg/api/metrics.go b/pkg/api/metrics.go index 2404dbb60e2..b5295f98d11 100644 --- a/pkg/api/metrics.go +++ b/pkg/api/metrics.go @@ -10,11 +10,8 @@ import ( "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/plugins/backendplugin" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/query" - "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" ) @@ -31,16 +28,7 @@ func (hs *HTTPServer) handleQueryMetricsError(err error) *response.NormalRespons return response.Error(http.StatusInternalServerError, fmt.Sprint("Secrets Plugin error: ", err.Error()), err) } - var badQuery query.ErrBadQuery - if errors.As(err, &badQuery) { - return response.Error(http.StatusBadRequest, util.Capitalize(badQuery.Message), err) - } - - if errors.Is(err, backendplugin.ErrPluginNotRegistered) { - return response.Error(http.StatusNotFound, "Plugin not found", err) - } - - return response.Error(http.StatusInternalServerError, "Query data error", err) + return response.ErrOrFallback(http.StatusInternalServerError, "Query data error", err) } // QueryMetricsV2 returns query metrics. diff --git a/pkg/api/metrics_test.go b/pkg/api/metrics_test.go index 2299e94ff1f..afb7fb303b6 100644 --- a/pkg/api/metrics_test.go +++ b/pkg/api/metrics_test.go @@ -4,42 +4,32 @@ import ( "bytes" "context" "encoding/json" + "errors" "fmt" + "io" "net/http" "strings" "testing" "github.com/grafana/grafana-plugin-sdk-go/backend" "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/backendplugin" + pluginClient "github.com/grafana/grafana/pkg/plugins/manager/client" + "github.com/grafana/grafana/pkg/plugins/manager/registry" "github.com/grafana/grafana/pkg/services/datasources" + fakeDatasources "github.com/grafana/grafana/pkg/services/datasources/fakes" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/query" "github.com/grafana/grafana/pkg/services/quota/quotatest" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/util/errutil" "github.com/grafana/grafana/pkg/web/webtest" - - "golang.org/x/oauth2" - - fakeDatasources "github.com/grafana/grafana/pkg/services/datasources/fakes" - "github.com/grafana/grafana/pkg/services/query" ) -var queryDatasourceInput = `{ -"from": "", - "to": "", - "queries": [ - { - "datasource": { - "type": "datasource", - "uid": "grafana" - }, - "queryType": "randomWalk", - "refId": "A" - } - ] - }` - type fakePluginRequestValidator struct { err error } @@ -98,7 +88,7 @@ func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) { }) t.Run("Status code is 400 when data source response has an error and feature toggle is disabled", func(t *testing.T) { - req := serverFeatureDisabled.NewPostRequest("/api/ds/query", strings.NewReader(queryDatasourceInput)) + req := serverFeatureDisabled.NewPostRequest("/api/ds/query", strings.NewReader(reqValid)) webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, OrgRole: org.RoleViewer}) resp, err := serverFeatureDisabled.SendJSON(req) require.NoError(t, err) @@ -107,7 +97,7 @@ func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) { }) t.Run("Status code is 207 when data source response has an error and feature toggle is enabled", func(t *testing.T) { - req := serverFeatureEnabled.NewPostRequest("/api/ds/query", strings.NewReader(queryDatasourceInput)) + req := serverFeatureEnabled.NewPostRequest("/api/ds/query", strings.NewReader(reqValid)) webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, OrgRole: org.RoleViewer}) resp, err := serverFeatureEnabled.SendJSON(req) require.NoError(t, err) @@ -141,7 +131,7 @@ func TestAPIEndpoint_Metrics_PluginDecryptionFailure(t *testing.T) { }) t.Run("Status code is 500 and a secrets plugin error is returned if there is a problem getting secrets from the remote plugin", func(t *testing.T) { - req := httpServer.NewPostRequest("/api/ds/query", strings.NewReader(queryDatasourceInput)) + req := httpServer.NewPostRequest("/api/ds/query", strings.NewReader(reqValid)) webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, OrgRole: org.RoleViewer}) resp, err := httpServer.SendJSON(req) require.NoError(t, err) @@ -157,3 +147,168 @@ func TestAPIEndpoint_Metrics_PluginDecryptionFailure(t *testing.T) { require.Contains(t, resObj.Message, "Secrets Plugin error:") }) } + +var reqValid = `{ + "from": "", + "to": "", + "queries": [ + { + "datasource": { + "type": "datasource", + "uid": "grafana" + }, + "queryType": "randomWalk", + "refId": "A" + } + ] +}` + +var reqNoQueries = `{ + "from": "", + "to": "", + "queries": [] +}` + +var reqQueryWithInvalidDatasourceID = `{ + "from": "", + "to": "", + "queries": [ + { + "queryType": "randomWalk", + "refId": "A" + } + ] +}` + +var reqDatasourceByUidNotFound = `{ + "from": "", + "to": "", + "queries": [ + { + "datasource": { + "type": "datasource", + "uid": "not-found" + }, + "queryType": "randomWalk", + "refId": "A" + } + ] +}` + +var reqDatasourceByIdNotFound = `{ + "from": "", + "to": "", + "queries": [ + { + "datasourceId": 1, + "queryType": "randomWalk", + "refId": "A" + } + ] +}` + +func TestDataSourceQueryError(t *testing.T) { + tcs := []struct { + request string + clientErr error + expectedStatus int + expectedBody string + }{ + { + request: reqValid, + clientErr: backendplugin.ErrPluginUnavailable, + expectedStatus: http.StatusInternalServerError, + expectedBody: `{"message":"Internal server error","messageId":"plugin.unavailable","statusCode":500,"traceID":""}`, + }, + { + request: reqValid, + clientErr: backendplugin.ErrMethodNotImplemented, + expectedStatus: http.StatusNotImplemented, + expectedBody: `{"message":"Not implemented","messageId":"plugin.notImplemented","statusCode":501,"traceID":""}`, + }, + { + request: reqValid, + clientErr: errors.New("surprise surprise"), + expectedStatus: errutil.StatusInternal.HTTPStatus(), + expectedBody: `{"message":"An error occurred within the plugin","messageId":"plugin.downstreamError","statusCode":500,"traceID":""}`, + }, + { + request: reqNoQueries, + expectedStatus: http.StatusBadRequest, + expectedBody: `{"message":"No queries found","messageId":"query.noQueries","statusCode":400,"traceID":""}`, + }, + { + request: reqQueryWithInvalidDatasourceID, + expectedStatus: http.StatusBadRequest, + expectedBody: `{"message":"Query does not contain a valid data source identifier","messageId":"query.invalidDatasourceId","statusCode":400,"traceID":""}`, + }, + { + request: reqDatasourceByUidNotFound, + expectedStatus: http.StatusNotFound, + expectedBody: `{"error":"data source not found","message":"Data source not found","traceID":""}`, + }, + { + request: reqDatasourceByIdNotFound, + expectedStatus: http.StatusNotFound, + expectedBody: `{"error":"data source not found","message":"Data source not found","traceID":""}`, + }, + } + + for _, tc := range tcs { + t.Run(fmt.Sprintf("Plugin client error %q should propagate to API", tc.clientErr), func(t *testing.T) { + p := &plugins.Plugin{ + JSONData: plugins.JSONData{ + ID: "grafana", + }, + } + p.RegisterClient(&fakePluginBackend{ + qdr: func(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { + return nil, tc.clientErr + }, + }) + srv := SetupAPITestServer(t, func(hs *HTTPServer) { + r := registry.NewInMemory() + err := r.Add(context.Background(), p) + require.NoError(t, err) + hs.queryDataService = query.ProvideService( + nil, + &fakeDatasources.FakeCacheService{}, + nil, + &fakePluginRequestValidator{}, + &fakeDatasources.FakeDataSourceService{}, + pluginClient.ProvideService(r), + &fakeOAuthTokenService{}, + ) + hs.QuotaService = quotatest.NewQuotaServiceFake() + }) + req := srv.NewPostRequest("/api/ds/query", strings.NewReader(tc.request)) + webtest.RequestWithSignedInUser(req, &user.SignedInUser{UserID: 1, OrgID: 1, OrgRole: org.RoleViewer}) + resp, err := srv.SendJSON(req) + require.NoError(t, err) + + require.Equal(t, tc.expectedStatus, resp.StatusCode) + require.Equal(t, tc.expectedStatus, resp.StatusCode) + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, tc.expectedBody, string(body)) + require.NoError(t, resp.Body.Close()) + }) + } +} + +type fakePluginBackend struct { + qdr backend.QueryDataHandlerFunc + + backendplugin.Plugin +} + +func (f *fakePluginBackend) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { + if f.qdr != nil { + return f.qdr(ctx, req) + } + return backend.NewQueryDataResponse(), nil +} + +func (f *fakePluginBackend) IsDecommissioned() bool { + return false +} diff --git a/pkg/api/plugins_test.go b/pkg/api/plugins_test.go index f858aa5d47e..217eaf165ab 100644 --- a/pkg/api/plugins_test.go +++ b/pkg/api/plugins_test.go @@ -16,6 +16,7 @@ import ( "github.com/stretchr/testify/require" "github.com/grafana/grafana-plugin-sdk-go/backend" + "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/log/logtest" @@ -319,11 +320,10 @@ func Test_GetPluginAssets(t *testing.T) { } func TestMakePluginResourceRequest(t *testing.T) { - pluginClient := &fakePluginClient{} hs := HTTPServer{ Cfg: setting.NewCfg(), log: log.New(), - pluginClient: pluginClient, + pluginClient: &fakePluginClient{}, } req := httptest.NewRequest(http.MethodGet, "/", nil) resp := httptest.NewRecorder() diff --git a/pkg/plugins/errors.go b/pkg/plugins/errors.go new file mode 100644 index 00000000000..d7f5b4387b3 --- /dev/null +++ b/pkg/plugins/errors.go @@ -0,0 +1,16 @@ +package plugins + +import "github.com/grafana/grafana/pkg/util/errutil" + +var ( + // ErrPluginNotRegistered error returned when a plugin is not registered. + ErrPluginNotRegistered = errutil.NewBase(errutil.StatusNotFound, "plugin.notRegistered") + // ErrHealthCheckFailed error returned when a plugin health check failed. + ErrHealthCheckFailed = errutil.NewBase(errutil.StatusInternal, "plugin.failedHealthCheck") + // ErrPluginUnavailable error returned when a plugin is unavailable. + ErrPluginUnavailable = errutil.NewBase(errutil.StatusInternal, "plugin.unavailable") + // ErrMethodNotImplemented error returned when a plugin method is not implemented. + ErrMethodNotImplemented = errutil.NewBase(errutil.StatusNotImplemented, "plugin.notImplemented") + // ErrPluginDownstreamError error returned when a plugin method is not implemented. + ErrPluginDownstreamError = errutil.NewBase(errutil.StatusInternal, "plugin.downstreamError", errutil.WithPublicMessage("An error occurred within the plugin")) +) diff --git a/pkg/plugins/manager/client/client.go b/pkg/plugins/manager/client/client.go index 4c9aaf4fb0e..686af19359e 100644 --- a/pkg/plugins/manager/client/client.go +++ b/pkg/plugins/manager/client/client.go @@ -28,7 +28,7 @@ func ProvideService(pluginRegistry registry.Service) *Service { func (s *Service) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { plugin, exists := s.plugin(ctx, req.PluginContext.PluginID) if !exists { - return nil, backendplugin.ErrPluginNotRegistered + return nil, plugins.ErrPluginNotRegistered.Errorf("%w", backendplugin.ErrPluginNotRegistered) } var resp *backend.QueryDataResponse @@ -39,14 +39,14 @@ func (s *Service) QueryData(ctx context.Context, req *backend.QueryDataRequest) if err != nil { if errors.Is(err, backendplugin.ErrMethodNotImplemented) { - return nil, err + return nil, plugins.ErrMethodNotImplemented.Errorf("%w", backendplugin.ErrMethodNotImplemented) } if errors.Is(err, backendplugin.ErrPluginUnavailable) { - return nil, err + return nil, plugins.ErrPluginUnavailable.Errorf("%w", backendplugin.ErrPluginUnavailable) } - return nil, fmt.Errorf("%v: %w", "failed to query data", err) + return nil, plugins.ErrPluginDownstreamError.Errorf("%v: %w", "failed to query data", err) } for refID, res := range resp.Responses { diff --git a/pkg/plugins/manager/client/client_test.go b/pkg/plugins/manager/client/client_test.go new file mode 100644 index 00000000000..5351e0a478b --- /dev/null +++ b/pkg/plugins/manager/client/client_test.go @@ -0,0 +1,88 @@ +package client + +import ( + "context" + "errors" + "fmt" + "testing" + + "github.com/grafana/grafana-plugin-sdk-go/backend" + "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/plugins/backendplugin" + "github.com/grafana/grafana/pkg/plugins/manager/fakes" + "github.com/stretchr/testify/require" +) + +func TestQueryData(t *testing.T) { + t.Run("Empty registry should return not registered error", func(t *testing.T) { + registry := fakes.NewFakePluginRegistry() + client := ProvideService(registry) + _, err := client.QueryData(context.Background(), &backend.QueryDataRequest{}) + require.Error(t, err) + require.ErrorIs(t, err, plugins.ErrPluginNotRegistered) + }) + + t.Run("Non-empty registry", func(t *testing.T) { + tcs := []struct { + err error + expectedError error + }{ + { + err: backendplugin.ErrPluginUnavailable, + expectedError: plugins.ErrPluginUnavailable, + }, + { + err: backendplugin.ErrMethodNotImplemented, + expectedError: plugins.ErrMethodNotImplemented, + }, + { + err: errors.New("surprise surprise"), + expectedError: plugins.ErrPluginDownstreamError, + }, + } + + for _, tc := range tcs { + t.Run(fmt.Sprintf("Plugin client error %q should return expected error", tc.err), func(t *testing.T) { + registry := fakes.NewFakePluginRegistry() + p := &plugins.Plugin{ + JSONData: plugins.JSONData{ + ID: "grafana", + }, + } + p.RegisterClient(&fakePluginBackend{ + qdr: func(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { + return nil, tc.err + }, + }) + err := registry.Add(context.Background(), p) + require.NoError(t, err) + + client := ProvideService(registry) + _, err = client.QueryData(context.Background(), &backend.QueryDataRequest{ + PluginContext: backend.PluginContext{ + PluginID: "grafana", + }, + }) + require.Error(t, err) + require.ErrorIs(t, err, tc.expectedError) + }) + } + }) +} + +type fakePluginBackend struct { + qdr backend.QueryDataHandlerFunc + + backendplugin.Plugin +} + +func (f *fakePluginBackend) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { + if f.qdr != nil { + return f.qdr(ctx, req) + } + return backend.NewQueryDataResponse(), nil +} + +func (f *fakePluginBackend) IsDecommissioned() bool { + return false +} diff --git a/pkg/services/live/live.go b/pkg/services/live/live.go index c5f08f3e151..06b64a81850 100644 --- a/pkg/services/live/live.go +++ b/pkg/services/live/live.go @@ -46,6 +46,7 @@ import ( "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" + "github.com/grafana/grafana/pkg/util/errutil" "github.com/grafana/grafana/pkg/web" "github.com/centrifugal/centrifuge" @@ -598,8 +599,8 @@ func (g *GrafanaLive) handleOnRPC(client *centrifuge.Client, e centrifuge.RPCEve if errors.Is(err, datasources.ErrDataSourceAccessDenied) { return centrifuge.RPCReply{}, ¢rifuge.Error{Code: uint32(http.StatusForbidden), Message: http.StatusText(http.StatusForbidden)} } - var badQuery *query.ErrBadQuery - if errors.As(err, &badQuery) { + var gfErr *errutil.Error + if errors.As(err, &gfErr) && gfErr.Reason.Status() == errutil.StatusBadRequest { return centrifuge.RPCReply{}, ¢rifuge.Error{Code: uint32(http.StatusBadRequest), Message: http.StatusText(http.StatusBadRequest)} } return centrifuge.RPCReply{}, centrifuge.ErrorInternal diff --git a/pkg/services/query/errors.go b/pkg/services/query/errors.go index f94278669ff..25846d9fb98 100644 --- a/pkg/services/query/errors.go +++ b/pkg/services/query/errors.go @@ -1,17 +1,12 @@ package query -import "fmt" - -// ErrBadQuery returned whenever request is malformed and must contain a message -// suitable to return in API response. -type ErrBadQuery struct { - Message string -} - -func NewErrBadQuery(msg string) *ErrBadQuery { - return &ErrBadQuery{Message: msg} -} - -func (e ErrBadQuery) Error() string { - return fmt.Sprintf("bad query: %s", e.Message) -} +import ( + "github.com/grafana/grafana/pkg/util/errutil" +) + +var ( + ErrNoQueriesFound = errutil.NewBase(errutil.StatusBadRequest, "query.noQueries", errutil.WithPublicMessage("No queries found")).Errorf("no queries found") + ErrInvalidDatasourceID = errutil.NewBase(errutil.StatusBadRequest, "query.invalidDatasourceId", errutil.WithPublicMessage("Query does not contain a valid data source identifier")).Errorf("invalid data source identifier") + ErrMultipleDatasources = errutil.NewBase(errutil.StatusBadRequest, "query.differentDatasources", errutil.WithPublicMessage("All queries must use the same datasource")).Errorf("all queries must use the same datasource") + ErrMissingDataSourceInfo = errutil.NewBase(errutil.StatusBadRequest, "query.missingDataSourceInfo").MustTemplate("query missing datasource info: {{ .Public.RefId }}", errutil.WithPublic("Query {{ .Public.RefId }} is missing datasource information")) +) diff --git a/pkg/services/query/query.go b/pkg/services/query/query.go index aaf9dc7867b..2cffca07861 100644 --- a/pkg/services/query/query.go +++ b/pkg/services/query/query.go @@ -21,6 +21,7 @@ import ( "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tsdb/grafanads" "github.com/grafana/grafana/pkg/tsdb/legacydata" + "github.com/grafana/grafana/pkg/util/errutil" "github.com/grafana/grafana/pkg/util/proxyutil" "github.com/grafana/grafana-plugin-sdk-go/backend" @@ -117,7 +118,11 @@ func (s *Service) handleExpressions(ctx context.Context, user *user.SignedInUser for _, pq := range parsedReq.parsedQueries { if pq.datasource == nil { - return nil, NewErrBadQuery(fmt.Sprintf("query mising datasource info: %s", pq.query.RefID)) + return nil, ErrMissingDataSourceInfo.Build(errutil.TemplateData{ + Public: map[string]interface{}{ + "RefId": pq.query.RefID, + }, + }) } exprReq.Queries = append(exprReq.Queries, expr.Query{ @@ -211,7 +216,7 @@ type parsedRequest struct { func (s *Service) parseMetricRequest(ctx context.Context, user *user.SignedInUser, skipCache bool, reqDTO dtos.MetricRequest) (*parsedRequest, error) { if len(reqDTO.Queries) == 0 { - return nil, NewErrBadQuery("no queries found") + return nil, ErrNoQueriesFound } timeRange := legacydata.NewDataTimeRange(reqDTO.From, reqDTO.To) @@ -228,7 +233,7 @@ func (s *Service) parseMetricRequest(ctx context.Context, user *user.SignedInUse return nil, err } if ds == nil { - return nil, NewErrBadQuery("invalid data source ID") + return nil, ErrInvalidDatasourceID } datasourcesByUid[ds.Uid] = ds @@ -262,7 +267,7 @@ func (s *Service) parseMetricRequest(ctx context.Context, user *user.SignedInUse if !req.hasExpression { if len(datasourcesByUid) > 1 { // We do not (yet) support mixed query type - return nil, NewErrBadQuery("all queries must use the same datasource") + return nil, ErrMultipleDatasources } } @@ -314,7 +319,7 @@ func (s *Service) getDataSourceFromQuery(ctx context.Context, user *user.SignedI return ds, nil } - return nil, NewErrBadQuery("missing data source ID/UID") + return nil, ErrInvalidDatasourceID } func (s *Service) decryptSecureJsonDataFn(ctx context.Context) func(ds *datasources.DataSource) (map[string]string, error) { diff --git a/pkg/util/errutil/errors.go b/pkg/util/errutil/errors.go index 169fcb018dc..6f20a90034c 100644 --- a/pkg/util/errutil/errors.go +++ b/pkg/util/errutil/errors.go @@ -166,12 +166,16 @@ func (e Error) Is(other error) bool { o, isGrafanaError := other.(Error) //nolint:errorlint base, isBase := other.(Base) + //nolint:errorlint + templateErr, isTemplateErr := other.(Template) switch { case isGrafanaError: return o.Reason == e.Reason && o.MessageID == e.MessageID && o.Error() == e.Error() case isBase: return base.Is(e) + case isTemplateErr: + return templateErr.Base.Is(e) default: return false } diff --git a/pkg/util/errutil/template.go b/pkg/util/errutil/template.go index 06954ba5b33..77d297bce52 100644 --- a/pkg/util/errutil/template.go +++ b/pkg/util/errutil/template.go @@ -115,3 +115,7 @@ func (t Template) Build(data TemplateData) error { return e } + +func (t Template) Error() string { + return t.Base.Error() +} diff --git a/pkg/util/errutil/template_test.go b/pkg/util/errutil/template_test.go index 1e0fa48b147..7a732db1a2d 100644 --- a/pkg/util/errutil/template_test.go +++ b/pkg/util/errutil/template_test.go @@ -3,10 +3,30 @@ package errutil_test import ( "errors" "fmt" + "testing" "github.com/grafana/grafana/pkg/util/errutil" + "github.com/stretchr/testify/require" ) +func TestTemplate(t *testing.T) { + tmpl := errutil.NewBase(errutil.StatusInternal, "template.sample-error").MustTemplate("[{{ .Public.user }}] got error: {{ .Error }}") + err := tmpl.Build(errutil.TemplateData{ + Public: map[string]interface{}{ + "user": "grot the bot", + }, + Error: errors.New("oh noes"), + }) + + t.Run("Built error should return true when compared with templated error ", func(t *testing.T) { + require.True(t, errors.Is(err, tmpl)) + }) + + t.Run("Built error should return true when compared with templated error base ", func(t *testing.T) { + require.True(t, errors.Is(err, tmpl.Base)) + }) +} + func ExampleTemplate() { // Initialization, this is typically done on a package or global // level.