From 2691872c7a9c863767c2742280236e56889ea8f1 Mon Sep 17 00:00:00 2001 From: Jeff Levin Date: Mon, 16 May 2022 14:17:05 -0700 Subject: [PATCH] remove validatedQueries feature toggle (#48381) * remove validatedQueries feature toggle --- .../src/types/featureToggles.gen.ts | 1 - pkg/api/api.go | 3 - pkg/api/common_test.go | 10 - pkg/api/metrics.go | 106 +--- pkg/api/metrics_test.go | 453 +----------------- pkg/services/featuremgmt/registry.go | 6 - pkg/services/featuremgmt/toggles_gen.go | 4 - 7 files changed, 5 insertions(+), 578 deletions(-) diff --git a/packages/grafana-data/src/types/featureToggles.gen.ts b/packages/grafana-data/src/types/featureToggles.gen.ts index a27f9c18f6d..51ff4d07698 100644 --- a/packages/grafana-data/src/types/featureToggles.gen.ts +++ b/packages/grafana-data/src/types/featureToggles.gen.ts @@ -38,7 +38,6 @@ export interface FeatureToggles { newNavigation?: boolean; showFeatureFlagsInUI?: boolean; disable_http_request_histogram?: boolean; - validatedQueries?: boolean; publicDashboards?: boolean; lokiLive?: boolean; swaggerUi?: boolean; diff --git a/pkg/api/api.go b/pkg/api/api.go index 6672bfa0e6d..359ac17c107 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -434,9 +434,6 @@ func (hs *HTTPServer) registerRoutes() { // DataSource w/ expressions apiRoute.Post("/ds/query", authorize(reqSignedIn, ac.EvalPermission(datasources.ActionQuery)), routing.Wrap(hs.QueryMetricsV2)) - // Validated query - apiRoute.Post("/dashboards/org/:orgId/uid/:dashboardUid/panels/:panelId/query", authorize(reqSignedIn, ac.EvalPermission(datasources.ActionQuery)), routing.Wrap(hs.QueryMetricsFromDashboard)) - apiRoute.Group("/alerts", func(alertsRoute routing.RouteRegister) { alertsRoute.Post("/test", routing.Wrap(hs.AlertTest)) alertsRoute.Post("/:alertId/pause", reqEditorRole, routing.Wrap(hs.PauseAlert)) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index 7d49e432812..990612ec2b4 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -38,7 +38,6 @@ import ( "github.com/grafana/grafana/pkg/services/searchusers" "github.com/grafana/grafana/pkg/services/searchusers/filters" "github.com/grafana/grafana/pkg/services/sqlstore" - "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" "github.com/grafana/grafana/pkg/web/webtest" @@ -330,15 +329,6 @@ func setupHTTPServer(t *testing.T, useFakeAccessControl bool, enableAccessContro return setupHTTPServerWithCfg(t, useFakeAccessControl, enableAccessControl, setting.NewCfg()) } -func setupHTTPServerWithMockDb(t *testing.T, useFakeAccessControl bool, enableAccessControl bool) accessControlScenarioContext { - // Use a new conf - cfg := setting.NewCfg() - db := sqlstore.InitTestDB(t) - db.Cfg = setting.NewCfg() - - return setupHTTPServerWithCfgDb(t, useFakeAccessControl, enableAccessControl, cfg, db, mockstore.NewSQLStoreMock()) -} - func setupHTTPServerWithCfg(t *testing.T, useFakeAccessControl, enableAccessControl bool, cfg *setting.Cfg) accessControlScenarioContext { db := sqlstore.InitTestDB(t, sqlstore.InitTestDBOpt{}) return setupHTTPServerWithCfgDb(t, useFakeAccessControl, enableAccessControl, cfg, db, db) diff --git a/pkg/api/metrics.go b/pkg/api/metrics.go index f9baf5a1373..aa6a6a8a170 100644 --- a/pkg/api/metrics.go +++ b/pkg/api/metrics.go @@ -1,10 +1,8 @@ package api import ( - "context" "errors" "net/http" - "strconv" "github.com/grafana/grafana-plugin-sdk-go/backend" "github.com/grafana/grafana/pkg/api/dtos" @@ -13,27 +11,11 @@ import ( "github.com/grafana/grafana/pkg/plugins/backendplugin" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/query" - "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/tsdb/legacydata" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web" ) -// QueryMetricsV2 returns query metrics. -// POST /api/ds/query DataSource query w/ expressions -func (hs *HTTPServer) QueryMetricsV2(c *models.ReqContext) response.Response { - reqDTO := dtos.MetricRequest{} - if err := web.Bind(c.Req, &reqDTO); err != nil { - return response.Error(http.StatusBadRequest, "bad request data", err) - } - - resp, err := hs.queryDataService.QueryData(c.Req.Context(), c.SignedInUser, c.SkipCache, reqDTO, true) - if err != nil { - return hs.handleQueryMetricsError(err) - } - return hs.toJsonStreamingResponse(resp) -} - func (hs *HTTPServer) handleQueryMetricsError(err error) *response.NormalResponse { if errors.Is(err, models.ErrDataSourceAccessDenied) { return response.Error(http.StatusForbidden, "Access denied to data source", err) @@ -53,96 +35,14 @@ func (hs *HTTPServer) handleQueryMetricsError(err error) *response.NormalRespons return response.Error(http.StatusInternalServerError, "Query data error", err) } -func parseDashboardQueryParams(params map[string]string) (models.GetDashboardQuery, int64, error) { - query := models.GetDashboardQuery{} - - if params[":orgId"] == "" || params[":dashboardUid"] == "" || params[":panelId"] == "" { - return query, 0, models.ErrDashboardOrPanelIdentifierNotSet - } - - orgId, err := strconv.ParseInt(params[":orgId"], 10, 64) - if err != nil { - return query, 0, models.ErrDashboardPanelIdentifierInvalid - } - - panelId, err := strconv.ParseInt(params[":panelId"], 10, 64) - if err != nil { - return query, 0, models.ErrDashboardPanelIdentifierInvalid - } - - query.Uid = params[":dashboardUid"] - query.OrgId = orgId - - return query, panelId, nil -} - -func checkDashboardAndPanel(ctx context.Context, ss sqlstore.Store, query models.GetDashboardQuery, panelId int64) error { - // Query the dashboard - if err := ss.GetDashboard(ctx, &query); err != nil { - return err - } - - if query.Result == nil { - return models.ErrDashboardCorrupt - } - - // dashboard saved but no panels - dashboard := query.Result - if dashboard.Data == nil { - return models.ErrDashboardCorrupt - } - - // FIXME: parse the dashboard JSON in a more performant/structured way. - panels := dashboard.Data.Get("panels") - - for i := 0; ; i++ { - panel, ok := panels.CheckGetIndex(i) - if !ok { - break - } - - if panel.Get("id").MustInt64(-1) == panelId { - return nil - } - } - - // no panel with that ID - return models.ErrDashboardPanelNotFound -} - -// QueryMetricsFromDashboard returns query metrics. -// POST /dashboards/org/:orgId/uid/:dashboardUid/panels/:panelId/query DataSource query w/ expressions -func (hs *HTTPServer) QueryMetricsFromDashboard(c *models.ReqContext) response.Response { - // check feature flag - if !hs.Features.IsEnabled(featuremgmt.FlagValidatedQueries) { - return response.Respond(http.StatusNotFound, "404 page not found\n") - } - - // build query +// QueryMetricsV2 returns query metrics. +// POST /api/ds/query DataSource query w/ expressions +func (hs *HTTPServer) QueryMetricsV2(c *models.ReqContext) response.Response { reqDTO := dtos.MetricRequest{} if err := web.Bind(c.Req, &reqDTO); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } - params := web.Params(c.Req) - getDashboardQuery, panelId, err := parseDashboardQueryParams(params) - // check dashboard: inside the statement is the happy path. we should maybe - // refactor this as it's not super obvious - if err == nil { - err = checkDashboardAndPanel(c.Req.Context(), hs.SQLStore, getDashboardQuery, panelId) - } - - // 404 if dashboard or panel not found - if err != nil { - c.Logger.Warn("Failed to find dashboard or panel for validated query", "err", err) - var dashboardErr models.DashboardErr - if ok := errors.As(err, &dashboardErr); ok { - return response.Error(dashboardErr.StatusCode, dashboardErr.Error(), err) - } - return response.Error(http.StatusNotFound, "Dashboard or panel not found", err) - } - - // return panel data resp, err := hs.queryDataService.QueryData(c.Req.Context(), c.SignedInUser, c.SkipCache, reqDTO, true) if err != nil { return hs.handleQueryMetricsError(err) diff --git a/pkg/api/metrics_test.go b/pkg/api/metrics_test.go index de14e9d9da9..ce37e0f328c 100644 --- a/pkg/api/metrics_test.go +++ b/pkg/api/metrics_test.go @@ -2,37 +2,25 @@ package api import ( "context" - "encoding/json" "fmt" "net/http" - "strconv" "strings" "testing" - acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/featuremgmt" - "github.com/grafana/grafana/pkg/services/sqlstore/mockstore" "github.com/grafana/grafana/pkg/web/webtest" "golang.org/x/oauth2" "github.com/grafana/grafana-plugin-sdk-go/backend" - "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/plugins" fakeDatasources "github.com/grafana/grafana/pkg/services/datasources/fakes" - datasources "github.com/grafana/grafana/pkg/services/datasources/service" "github.com/grafana/grafana/pkg/services/query" - "github.com/grafana/grafana/pkg/services/secrets/fakes" - "github.com/grafana/grafana/pkg/services/secrets/kvstore" - secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -var ( - queryDatasourceInput = `{ - "from": "", +var queryDatasourceInput = `{ +"from": "", "to": "", "queries": [ { @@ -46,123 +34,6 @@ var ( ] }` - getDashboardByIdOutput = `{ - "annotations": { - "list": [ - { - "builtIn": 1, - "datasource": "-- Grafana --", - "enable": true, - "hide": true, - "iconColor": "rgba(0, 211, 255, 1)", - "name": "Annotations & Alerts", - "target": { - "limit": 100, - "matchAny": false, - "tags": [], - "type": "dashboard" - }, - "type": "dashboard" - } - ] - }, - "editable": true, - "fiscalYearStartMonth": 0, - "graphTooltip": 0, - "links": [], - "liveNow": false, - "panels": [ - { - "fieldConfig": { - "defaults": { - "color": { - "mode": "palette-classic" - }, - "custom": { - "axisLabel": "", - "axisPlacement": "auto", - "barAlignment": 0, - "drawStyle": "line", - "fillOpacity": 0, - "gradientMode": "none", - "hideFrom": { - "legend": false, - "tooltip": false, - "viz": false - }, - "lineInterpolation": "linear", - "lineWidth": 1, - "pointSize": 5, - "scaleDistribution": { - "type": "linear" - }, - "showPoints": "auto", - "spanNulls": false, - "stacking": { - "group": "A", - "mode": "none" - }, - "thresholdsStyle": { - "mode": "off" - } - }, - "mappings": [], - "thresholds": { - "mode": "absolute", - "steps": [ - { - "color": "green", - "value": null - }, - { - "color": "red", - "value": 80 - } - ] - } - }, - "overrides": [] - }, - "gridPos": { - "h": 9, - "w": 12, - "x": 0, - "y": 0 - }, - "id": 2, - "options": { - "legend": { - "calcs": [], - "displayMode": "list", - "placement": "bottom" - }, - "tooltip": { - "mode": "single", - "sort": "none" - } - }, - "title": "Panel Title", - "type": "timeseries" - } - ], - "schemaVersion": 35, - "style": "dark", - "tags": [], - "templating": { - "list": [] - }, - "time": { - "from": "now-6h", - "to": "now" - }, - "timepicker": {}, - "timezone": "", - "title": "New dashboard", - "version": 0, - "weekStart": "" - }` -) - type fakePluginRequestValidator struct { err error } @@ -184,326 +55,6 @@ func (ts *fakeOAuthTokenService) IsOAuthPassThruEnabled(*models.DataSource) bool return ts.passThruEnabled } -func (c *dashboardFakePluginClient) QueryData(ctx context.Context, req *backend.QueryDataRequest) (*backend.QueryDataResponse, error) { - c.req = req - resp := backend.Responses{} - return &backend.QueryDataResponse{Responses: resp}, nil -} - -type dashboardFakePluginClient struct { - plugins.Client - - req *backend.QueryDataRequest -} - -// `/dashboards/org/:orgId/uid/:dashboardUid/panels/:panelId/query` endpoints test -func TestAPIEndpoint_Metrics_QueryMetricsFromDashboard(t *testing.T) { - sc := setupHTTPServerWithMockDb(t, false, false) - - secretsStore := kvstore.SetupTestService(t) - secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore()) - ds := datasources.ProvideService(nil, secretsService, secretsStore, nil, featuremgmt.WithFeatures(), acmock.New(), acmock.NewMockedPermissionsService()) - - setInitCtxSignedInViewer(sc.initCtx) - sc.hs.queryDataService = query.ProvideService( - nil, - nil, - nil, - &fakePluginRequestValidator{}, - ds, - &dashboardFakePluginClient{}, - &fakeOAuthTokenService{}, - ) - - sc.hs.Features = featuremgmt.WithFeatures(featuremgmt.FlagValidatedQueries, true) - - dashboardJson, err := simplejson.NewFromReader(strings.NewReader(getDashboardByIdOutput)) - if err != nil { - t.Fatalf("Failed to unmarshal dashboard json: %v", err) - } - - mockDb := sc.hs.SQLStore.(*mockstore.SQLStoreMock) - - t.Run("Can query a valid dashboard", func(t *testing.T) { - mockDb.ExpectedDashboard = &models.Dashboard{ - Uid: "1", - OrgId: testOrgID, - Data: dashboardJson, - } - mockDb.ExpectedError = nil - - response := callAPI( - sc.server, - http.MethodPost, - fmt.Sprintf("/api/dashboards/org/%d/uid/%s/panels/%s/query", testOrgID, "1", "2"), - strings.NewReader(queryDatasourceInput), - t, - ) - assert.Equal(t, http.StatusOK, response.Code) - }) - - t.Run("Cannot query without a valid orgid or dashboard or panel ID", func(t *testing.T) { - mockDb.ExpectedDashboard = nil - mockDb.ExpectedError = models.ErrDashboardOrPanelIdentifierNotSet - - response := callAPI( - sc.server, - http.MethodPost, - "/api/dashboards/org//uid//panels//query", - strings.NewReader(queryDatasourceInput), - t, - ) - - assert.Equal(t, http.StatusBadRequest, response.Code) - - var res map[string]interface{} - err := json.Unmarshal(response.Body.Bytes(), &res) - assert.NoError(t, err) - assert.Equal(t, models.ErrDashboardOrPanelIdentifierNotSet.Error(), res["error"]) - assert.Equal(t, models.ErrDashboardOrPanelIdentifierNotSet.Error(), res["message"]) - }) - - t.Run("Cannot query without a valid orgid", func(t *testing.T) { - response := callAPI( - sc.server, - http.MethodPost, - fmt.Sprintf("/api/dashboards/org//uid/%s/panels/%s/query", "1", "2"), - strings.NewReader(queryDatasourceInput), - t, - ) - assert.Equal(t, http.StatusBadRequest, response.Code) - var res map[string]interface{} - assert.NoError(t, json.Unmarshal(response.Body.Bytes(), &res)) - assert.Equal(t, models.ErrDashboardOrPanelIdentifierNotSet.Error(), res["error"]) - assert.Equal(t, models.ErrDashboardOrPanelIdentifierNotSet.Error(), res["message"]) - }) - - t.Run("Cannot query without a valid dashboard or panel ID", func(t *testing.T) { - response := callAPI( - sc.server, - http.MethodPost, - fmt.Sprintf("/api/dashboards/org//uid/%s/panels/%s/query", "1", "2"), - strings.NewReader(queryDatasourceInput), - t, - ) - assert.Equal(t, http.StatusBadRequest, response.Code) - - var res map[string]interface{} - assert.NoError(t, json.Unmarshal(response.Body.Bytes(), &res)) - assert.Equal(t, models.ErrDashboardOrPanelIdentifierNotSet.Error(), res["error"]) - assert.Equal(t, models.ErrDashboardOrPanelIdentifierNotSet.Error(), res["message"]) - }) - - t.Run("Cannot query when ValidatedQueries is disabled", func(t *testing.T) { - sc.hs.Features = featuremgmt.WithFeatures(featuremgmt.FlagValidatedQueries, false) - - response := callAPI( - sc.server, - http.MethodPost, - "/api/dashboards/uid/1/panels/1/query", - strings.NewReader(queryDatasourceInput), - t, - ) - - assert.Equal(t, http.StatusNotFound, response.Code) - assert.Equal( - t, - "404 page not found\n", - response.Body.String(), - ) - }) -} - -func TestAPIEndpoint_Metrics_checkDashboardAndPanel(t *testing.T) { - dashboardJson, err := simplejson.NewFromReader(strings.NewReader(getDashboardByIdOutput)) - if err != nil { - t.Fatalf("Failed to unmarshal dashboard json: %v", err) - } - - tests := []struct { - name string - orgId int64 - dashboardUid string - panelId int64 - dashboardQueryResult *models.Dashboard - expectedError error - }{ - { - name: "Work when correct dashboardId and panelId given", - orgId: testOrgID, - dashboardUid: "1", - panelId: 2, - dashboardQueryResult: &models.Dashboard{ - Uid: "1", - OrgId: testOrgID, - Data: dashboardJson, - }, - expectedError: nil, - }, - { - name: "404 on invalid orgId", - orgId: 7, - dashboardUid: "1", - panelId: 2, - dashboardQueryResult: nil, - expectedError: models.ErrDashboardNotFound, - }, - { - name: "404 on invalid dashboardId", - orgId: testOrgID, - dashboardUid: "", - panelId: 2, - dashboardQueryResult: nil, - expectedError: models.ErrDashboardNotFound, - }, - { - name: "404 on invalid panelId", - orgId: testOrgID, - dashboardUid: "1", - panelId: 0, - dashboardQueryResult: nil, - expectedError: models.ErrDashboardNotFound, - }, - { - name: "Fails when the dashboard does not exist", - orgId: testOrgID, - dashboardUid: "1", - panelId: 2, - dashboardQueryResult: nil, - expectedError: models.ErrDashboardNotFound, - }, - { - name: "Fails when the panel does not exist", - orgId: testOrgID, - dashboardUid: "1", - panelId: 3, - dashboardQueryResult: &models.Dashboard{ - Id: 1, - OrgId: testOrgID, - Data: dashboardJson, - }, - expectedError: models.ErrDashboardPanelNotFound, - }, - { - name: "Fails when the dashboard contents are nil", - orgId: testOrgID, - dashboardUid: "1", - panelId: 3, - dashboardQueryResult: &models.Dashboard{ - Uid: "1", - OrgId: testOrgID, - Data: nil, - }, - expectedError: models.ErrDashboardCorrupt, - }, - } - - ss := mockstore.NewSQLStoreMock() - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - ss.ExpectedDashboard = test.dashboardQueryResult - ss.ExpectedError = test.expectedError - - query := models.GetDashboardQuery{ - OrgId: test.orgId, - Uid: test.dashboardUid, - } - - assert.Equal(t, test.expectedError, checkDashboardAndPanel(context.Background(), ss, query, test.panelId)) - }) - } -} - -func TestAPIEndpoint_Metrics_ParseDashboardQueryParams(t *testing.T) { - tests := []struct { - name string - params map[string]string - expectedDashboardQuery models.GetDashboardQuery - expectedPanelId int64 - expectedError error - }{ - { - name: "Work when correct orgId, dashboardId and panelId given", - params: map[string]string{ - ":orgId": strconv.FormatInt(testOrgID, 10), - ":dashboardUid": "1", - ":panelId": "2", - }, - expectedDashboardQuery: models.GetDashboardQuery{ - Uid: "1", - OrgId: 1, - }, - expectedPanelId: 2, - expectedError: nil, - }, - { - name: "Get error when dashboardUid not given", - params: map[string]string{ - ":orgId": strconv.FormatInt(testOrgID, 10), - ":dashboardUid": "", - ":panelId": "1", - }, - expectedDashboardQuery: models.GetDashboardQuery{}, - expectedPanelId: 0, - expectedError: models.ErrDashboardOrPanelIdentifierNotSet, - }, - { - name: "Get error when panelId not given", - params: map[string]string{ - ":orgId": strconv.FormatInt(testOrgID, 10), - ":dashboardUid": "1", - ":panelId": "", - }, - expectedDashboardQuery: models.GetDashboardQuery{}, - expectedPanelId: 0, - expectedError: models.ErrDashboardOrPanelIdentifierNotSet, - }, - { - name: "Get error when orgId not given", - params: map[string]string{ - ":orgId": "", - ":dashboardUid": "1", - ":panelId": "2", - }, - expectedDashboardQuery: models.GetDashboardQuery{}, - expectedPanelId: 0, - expectedError: models.ErrDashboardOrPanelIdentifierNotSet, - }, - { - name: "Get error when panelId not is invalid", - params: map[string]string{ - ":orgId": strconv.FormatInt(testOrgID, 10), - ":dashboardUid": "1", - ":panelId": "aaa", - }, - expectedDashboardQuery: models.GetDashboardQuery{}, - expectedPanelId: 0, - expectedError: models.ErrDashboardPanelIdentifierInvalid, - }, - { - name: "Get error when orgId not is invalid", - params: map[string]string{ - ":orgId": "aaa", - ":dashboardUid": "1", - ":panelId": "2", - }, - expectedDashboardQuery: models.GetDashboardQuery{}, - expectedPanelId: 0, - expectedError: models.ErrDashboardPanelIdentifierInvalid, - }, - } - - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - // other validations? - dashboardQuery, panelId, err := parseDashboardQueryParams(test.params) - assert.Equal(t, test.expectedDashboardQuery, dashboardQuery) - assert.Equal(t, test.expectedPanelId, panelId) - assert.Equal(t, test.expectedError, err) - }) - } -} - // `/ds/query` endpoint test func TestAPIEndpoint_Metrics_QueryMetricsV2(t *testing.T) { qds := query.ProvideService( diff --git a/pkg/services/featuremgmt/registry.go b/pkg/services/featuremgmt/registry.go index 7a988555ced..216251e1f0c 100644 --- a/pkg/services/featuremgmt/registry.go +++ b/pkg/services/featuremgmt/registry.go @@ -127,12 +127,6 @@ var ( Description: "Do not create histograms for http requests", State: FeatureStateAlpha, }, - { - Name: "validatedQueries", - Description: "only execute the query saved in a panel", - State: FeatureStateAlpha, - RequiresDevMode: true, - }, { Name: "publicDashboards", Description: "enables public access to dashboards", diff --git a/pkg/services/featuremgmt/toggles_gen.go b/pkg/services/featuremgmt/toggles_gen.go index e1a3d76171e..4aad3fd8cb1 100644 --- a/pkg/services/featuremgmt/toggles_gen.go +++ b/pkg/services/featuremgmt/toggles_gen.go @@ -95,10 +95,6 @@ const ( // Do not create histograms for http requests FlagDisableHttpRequestHistogram = "disable_http_request_histogram" - // FlagValidatedQueries - // only execute the query saved in a panel - FlagValidatedQueries = "validatedQueries" - // FlagPublicDashboards // enables public access to dashboards FlagPublicDashboards = "publicDashboards"