Alerting: Improve validation of query and expressions on rule submit (#53258)

* Improve error messages of server-side expression 
* move validation of alert queries and a condition to eval package
This commit is contained in:
Yuriy Tseretyan
2022-09-21 15:14:11 -04:00
committed by GitHub
parent 879241a48f
commit 2d38664fe6
19 changed files with 311 additions and 146 deletions
+12 -10
View File
@@ -92,6 +92,8 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) {
ac: api.AccessControl,
}
evaluator := eval.NewEvaluator(api.Cfg, log.New("ngalert.eval"), api.DatasourceCache, api.ExpressionService)
// Register endpoints for proxying to Alertmanager-compatible backends.
api.RegisterAlertmanagerApiEndpoints(NewForkingAM(
api.DatasourceCache,
@@ -109,15 +111,15 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) {
api.DatasourceCache,
NewLotexRuler(proxy, logger),
&RulerSrv{
DatasourceCache: api.DatasourceCache,
QuotaService: api.QuotaService,
scheduleService: api.Schedule,
store: api.RuleStore,
provenanceStore: api.ProvenanceStore,
xactManager: api.TransactionManager,
log: logger,
cfg: &api.Cfg.UnifiedAlerting,
ac: api.AccessControl,
conditionValidator: evaluator,
QuotaService: api.QuotaService,
scheduleService: api.Schedule,
store: api.RuleStore,
provenanceStore: api.ProvenanceStore,
xactManager: api.TransactionManager,
log: logger,
cfg: &api.Cfg.UnifiedAlerting,
ac: api.AccessControl,
},
), m)
api.RegisterTestingApiEndpoints(NewTestingApi(
@@ -126,7 +128,7 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) {
DatasourceCache: api.DatasourceCache,
log: logger,
accessControl: api.AccessControl,
evaluator: eval.NewEvaluator(api.Cfg, log.New("ngalert.eval"), api.DatasourceCache, api.ExpressionService),
evaluator: evaluator,
}), m)
api.RegisterConfigurationApiEndpoints(NewConfiguration(
&ConfigSrv{
+18 -11
View File
@@ -10,10 +10,10 @@ import (
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/dashboards"
"github.com/grafana/grafana/pkg/services/datasources"
"github.com/grafana/grafana/pkg/services/ngalert/provisioning"
"github.com/grafana/grafana/pkg/services/ngalert/store"
"github.com/grafana/grafana/pkg/services/quota"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
"github.com/prometheus/common/model"
@@ -28,16 +28,21 @@ import (
"github.com/grafana/grafana/pkg/util"
)
type ConditionValidator interface {
// Validate validates that the condition is correct. Returns nil if the condition is correct. Otherwise, error that describes the failure
Validate(ctx context.Context, user *user.SignedInUser, condition ngmodels.Condition) error
}
type RulerSrv struct {
xactManager provisioning.TransactionManager
provenanceStore provisioning.ProvisioningStore
store store.RuleStore
DatasourceCache datasources.CacheService
QuotaService quota.Service
scheduleService schedule.ScheduleService
log log.Logger
cfg *setting.UnifiedAlertingSettings
ac accesscontrol.AccessControl
xactManager provisioning.TransactionManager
provenanceStore provisioning.ProvisioningStore
store store.RuleStore
QuotaService quota.Service
scheduleService schedule.ScheduleService
log log.Logger
cfg *setting.UnifiedAlertingSettings
ac accesscontrol.AccessControl
conditionValidator ConditionValidator
}
var (
@@ -302,7 +307,9 @@ func (srv RulerSrv) RoutePostNameRulesConfig(c *models.ReqContext, ruleGroupConf
return toNamespaceErrorResponse(err)
}
rules, err := validateRuleGroup(&ruleGroupConfig, c.SignedInUser.OrgID, namespace, conditionValidator(c, srv.DatasourceCache), srv.cfg)
rules, err := validateRuleGroup(&ruleGroupConfig, c.SignedInUser.OrgID, namespace, func(condition ngmodels.Condition) error {
return srv.conditionValidator.Validate(c.Req.Context(), c.SignedInUser, condition)
}, srv.cfg)
if err != nil {
return ErrResp(http.StatusBadRequest, err, "")
}
@@ -646,7 +646,6 @@ func createService(ac *acMock.Mock, store *store.FakeRuleStore, scheduler schedu
return &RulerSrv{
xactManager: store,
store: store,
DatasourceCache: nil,
QuotaService: nil,
provenanceStore: provisioning.NewFakeProvisioningStore(),
scheduleService: scheduler,
@@ -88,7 +88,6 @@ func validateRuleNode(
if len(ruleNode.GrafanaManagedAlert.Data) != 0 {
cond := ngmodels.Condition{
Condition: ruleNode.GrafanaManagedAlert.Condition,
OrgID: orgId,
Data: ruleNode.GrafanaManagedAlert.Data,
}
if err := conditionValidator(cond); err != nil {
+1 -6
View File
@@ -40,11 +40,10 @@ func (srv TestingApiSrv) RouteTestGrafanaRuleConfig(c *models.ReqContext, body a
evalCond := ngmodels.Condition{
Condition: body.GrafanaManagedCondition.Condition,
OrgID: c.SignedInUser.OrgID,
Data: body.GrafanaManagedCondition.Data,
}
if err := validateCondition(c.Req.Context(), evalCond, c.SignedInUser, c.SkipCache, srv.DatasourceCache); err != nil {
if err := srv.evaluator.Validate(c.Req.Context(), c.SignedInUser, evalCond); err != nil {
return ErrResp(http.StatusBadRequest, err, "invalid condition")
}
@@ -111,10 +110,6 @@ func (srv TestingApiSrv) RouteEvalQueries(c *models.ReqContext, cmd apimodels.Ev
return ErrResp(http.StatusUnauthorized, fmt.Errorf("%w to query one or many data sources used by the rule", ErrAuthorization), "")
}
if _, err := validateQueriesAndExpressions(c.Req.Context(), cmd.Data, c.SignedInUser, c.SkipCache, srv.DatasourceCache); err != nil {
return ErrResp(http.StatusBadRequest, err, "invalid queries or expressions")
}
evalResults, err := srv.evaluator.QueriesAndExpressionsEval(c.Req.Context(), c.SignedInUser, cmd.Data, now)
if err != nil {
return ErrResp(http.StatusBadRequest, err, "Failed to evaluate queries and expressions")
@@ -70,6 +70,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) {
evaluator := &eval.FakeEvaluator{}
var result []eval.Result
evaluator.EXPECT().Validate(mock.Anything, mock.Anything, mock.Anything).Return(nil)
evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(result)
srv := createTestingApiSrv(ds, ac, evaluator)
@@ -86,6 +87,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) {
require.Equal(t, http.StatusOK, response.Status())
evaluator.AssertCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
evaluator.AssertCalled(t, "Validate", mock.Anything, mock.Anything, mock.Anything)
})
})
@@ -110,6 +112,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) {
evaluator := &eval.FakeEvaluator{}
var result []eval.Result
evaluator.EXPECT().Validate(mock.Anything, mock.Anything, mock.Anything).Return(nil)
evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(result)
srv := createTestingApiSrv(ds, ac, evaluator)
@@ -125,6 +128,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) {
require.Equal(t, http.StatusUnauthorized, response.Status())
evaluator.AssertNotCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
evaluator.AssertNotCalled(t, "Validate", mock.Anything, mock.Anything, mock.Anything)
rc.IsSignedIn = true
@@ -140,6 +144,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) {
require.Equal(t, http.StatusOK, response.Status())
evaluator.AssertCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything, mock.Anything)
evaluator.AssertCalled(t, "Validate", mock.Anything, mock.Anything, mock.Anything)
})
})
}
-59
View File
@@ -2,7 +2,6 @@ package api
import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
@@ -23,7 +22,6 @@ import (
apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/org"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/web"
)
@@ -207,63 +205,6 @@ func messageExtractor(resp *response.NormalResponse) (interface{}, error) {
return map[string]string{"message": string(resp.Body())}, nil
}
func validateCondition(ctx context.Context, c ngmodels.Condition, user *user.SignedInUser, skipCache bool, datasourceCache datasources.CacheService) error {
if len(c.Data) == 0 {
return nil
}
refIDs, err := validateQueriesAndExpressions(ctx, c.Data, user, skipCache, datasourceCache)
if err != nil {
return err
}
t := make([]string, 0, len(refIDs))
for refID := range refIDs {
t = append(t, refID)
}
if _, ok := refIDs[c.Condition]; !ok {
return fmt.Errorf("condition %s not found in any query or expression: it should be one of: [%s]", c.Condition, strings.Join(t, ","))
}
return nil
}
// conditionValidator returns a curried validateCondition that accepts only condition
func conditionValidator(c *models.ReqContext, cache datasources.CacheService) func(ngmodels.Condition) error {
return func(condition ngmodels.Condition) error {
return validateCondition(c.Req.Context(), condition, c.SignedInUser, c.SkipCache, cache)
}
}
func validateQueriesAndExpressions(ctx context.Context, data []ngmodels.AlertQuery, user *user.SignedInUser, skipCache bool, datasourceCache datasources.CacheService) (map[string]struct{}, error) {
refIDs := make(map[string]struct{})
if len(data) == 0 {
return nil, nil
}
for _, query := range data {
datasourceUID, err := query.GetDatasource()
if err != nil {
return nil, err
}
isExpression, err := query.IsExpression()
if err != nil {
return nil, err
}
if isExpression {
refIDs[query.RefID] = struct{}{}
continue
}
_, err = datasourceCache.GetDatasourceByUID(ctx, datasourceUID, user, skipCache)
if err != nil {
return nil, fmt.Errorf("invalid query %s: %w: %s", query.RefID, err, datasourceUID)
}
refIDs[query.RefID] = struct{}{}
}
return refIDs, nil
}
// ErrorResp creates a response with a visible error
func ErrResp(status int, err error, msg string, args ...interface{}) *response.NormalResponse {
if msg != "" {
+40 -4
View File
@@ -4,6 +4,7 @@ package eval
import (
"context"
"errors"
"fmt"
"runtime/debug"
"sort"
@@ -29,6 +30,8 @@ type Evaluator interface {
ConditionEval(ctx context.Context, user *user.SignedInUser, condition models.Condition, now time.Time) Results
// QueriesAndExpressionsEval executes queries and expressions and returns the result.
QueriesAndExpressionsEval(ctx context.Context, user *user.SignedInUser, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error)
// Validate validates that the condition is correct. Returns nil if the condition is correct. Otherwise, error that describes the failure
Validate(ctx context.Context, user *user.SignedInUser, condition models.Condition) error
}
type evaluatorImpl struct {
@@ -173,16 +176,16 @@ func getExprRequest(ctx AlertExecCtx, data []models.AlertQuery, now time.Time, d
for _, q := range data {
model, err := q.GetModel()
if err != nil {
return nil, fmt.Errorf("failed to get query model: %w", err)
return nil, fmt.Errorf("failed to get query model from '%s': %w", q.RefID, err)
}
interval, err := q.GetIntervalDuration()
if err != nil {
return nil, fmt.Errorf("failed to retrieve intervalMs from the model: %w", err)
return nil, fmt.Errorf("failed to retrieve intervalMs from '%s': %w", q.RefID, err)
}
maxDatapoints, err := q.GetMaxDatapoints()
if err != nil {
return nil, fmt.Errorf("failed to retrieve maxDatapoints from the model: %w", err)
return nil, fmt.Errorf("failed to retrieve maxDatapoints from '%s': %w", q.RefID, err)
}
ds, ok := datasources[q.DatasourceUID]
@@ -192,7 +195,7 @@ func getExprRequest(ctx AlertExecCtx, data []models.AlertQuery, now time.Time, d
} else {
ds, err = dsCacheService.GetDatasourceByUID(ctx.Ctx, q.DatasourceUID, ctx.User, true)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to build query '%s': %w", q.RefID, err)
}
}
datasources[q.DatasourceUID] = ds
@@ -573,3 +576,36 @@ func (e *evaluatorImpl) QueriesAndExpressionsEval(ctx context.Context, user *use
return execResult, nil
}
func (e *evaluatorImpl) Validate(ctx context.Context, user *user.SignedInUser, condition models.Condition) error {
evalctx := AlertExecCtx{
User: user,
ExpressionsEnabled: e.cfg.ExpressionsEnabled,
Log: e.log,
Ctx: ctx,
}
if len(condition.Data) == 0 {
return errors.New("expression list is empty. must be at least 1 expression")
}
if len(condition.Condition) == 0 {
return errors.New("condition must not be empty")
}
req, err := getExprRequest(evalctx, condition.Data, time.Now(), e.dataSourceCache)
if err != nil {
return err
}
pipeline, err := e.expressionService.BuildPipeline(req)
if err != nil {
return err
}
conditions := make([]string, 0, len(pipeline))
for _, node := range pipeline {
if node.RefID() == condition.Condition {
return nil
}
conditions = append(conditions, node.RefID())
}
return fmt.Errorf("condition %s does not exist, must be one of %v", condition.Condition, conditions)
}
+115
View File
@@ -1,13 +1,23 @@
package eval
import (
"context"
"fmt"
"math/rand"
"testing"
"time"
"github.com/grafana/grafana-plugin-sdk-go/data"
"github.com/stretchr/testify/require"
ptr "github.com/xorcare/pointer"
"github.com/grafana/grafana/pkg/expr"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/services/datasources"
fakes "github.com/grafana/grafana/pkg/services/datasources/fakes"
"github.com/grafana/grafana/pkg/services/ngalert/models"
"github.com/grafana/grafana/pkg/services/user"
"github.com/grafana/grafana/pkg/setting"
)
func TestEvaluateExecutionResult(t *testing.T) {
@@ -340,3 +350,108 @@ func TestEvaluateExecutionResultsNoData(t *testing.T) {
require.ElementsMatch(t, []string{"A,B", "C"}, refIDs)
})
}
func TestValidate(t *testing.T) {
testCases := []struct {
name string
condition func(service *fakes.FakeCacheService) models.Condition
error bool
}{
{
name: "fail if no expressions",
error: true,
condition: func(service *fakes.FakeCacheService) models.Condition {
return models.Condition{
Condition: "A",
Data: []models.AlertQuery{},
}
},
},
{
name: "fail if condition RefID does not exist",
error: true,
condition: func(service *fakes.FakeCacheService) models.Condition {
ds := models.GenerateAlertQuery()
service.DataSources = append(service.DataSources, &datasources.DataSource{
Uid: ds.DatasourceUID,
})
return models.Condition{
Condition: "C",
Data: []models.AlertQuery{
ds,
models.CreateClassicConditionExpression("B", ds.RefID, "last", "gt", rand.Int()),
},
}
},
},
{
name: "fail if condition RefID is empty",
error: true,
condition: func(service *fakes.FakeCacheService) models.Condition {
ds := models.GenerateAlertQuery()
service.DataSources = append(service.DataSources, &datasources.DataSource{
Uid: ds.DatasourceUID,
})
return models.Condition{
Condition: "",
Data: []models.AlertQuery{
ds,
models.CreateClassicConditionExpression("B", ds.RefID, "last", "gt", rand.Int()),
},
}
},
},
{
name: "fail if datasource with UID does not exists",
error: true,
condition: func(service *fakes.FakeCacheService) models.Condition {
ds := models.GenerateAlertQuery()
// do not update the cache service
return models.Condition{
Condition: ds.RefID,
Data: []models.AlertQuery{
ds,
},
}
},
},
{
name: "pass if datasource exists and condition is correct",
error: false,
condition: func(service *fakes.FakeCacheService) models.Condition {
ds := models.GenerateAlertQuery()
service.DataSources = append(service.DataSources, &datasources.DataSource{
Uid: ds.DatasourceUID,
})
return models.Condition{
Condition: "B",
Data: []models.AlertQuery{
ds,
models.CreateClassicConditionExpression("B", ds.RefID, "last", "gt", rand.Int()),
},
}
},
},
}
for _, testCase := range testCases {
u := &user.SignedInUser{}
t.Run(testCase.name, func(t *testing.T) {
cacheService := &fakes.FakeCacheService{}
condition := testCase.condition(cacheService)
evaluator := NewEvaluator(&setting.Cfg{ExpressionsEnabled: true}, log.New("test"), cacheService, expr.ProvideService(&setting.Cfg{ExpressionsEnabled: true}, nil, nil))
err := evaluator.Validate(context.Background(), u, condition)
if testCase.error {
require.Error(t, err)
} else {
require.NoError(t, err)
}
})
}
}
+42 -18
View File
@@ -1,4 +1,4 @@
// Code generated by mockery v2.14.0. DO NOT EDIT.
// Code generated by mockery v2.10.0. DO NOT EDIT.
package eval
@@ -51,10 +51,10 @@ type FakeEvaluator_ConditionEval_Call struct {
}
// ConditionEval is a helper method to define mock.On call
// - ctx context.Context
// - _a1 *user.SignedInUser
// - condition models.Condition
// - now time.Time
// - ctx context.Context
// - _a1 *user.SignedInUser
// - condition models.Condition
// - now time.Time
func (_e *FakeEvaluator_Expecter) ConditionEval(ctx interface{}, _a1 interface{}, condition interface{}, now interface{}) *FakeEvaluator_ConditionEval_Call {
return &FakeEvaluator_ConditionEval_Call{Call: _e.mock.On("ConditionEval", ctx, _a1, condition, now)}
}
@@ -100,10 +100,10 @@ type FakeEvaluator_QueriesAndExpressionsEval_Call struct {
}
// QueriesAndExpressionsEval is a helper method to define mock.On call
// - ctx context.Context
// - _a1 *user.SignedInUser
// - data []models.AlertQuery
// - now time.Time
// - ctx context.Context
// - _a1 *user.SignedInUser
// - data []models.AlertQuery
// - now time.Time
func (_e *FakeEvaluator_Expecter) QueriesAndExpressionsEval(ctx interface{}, _a1 interface{}, data interface{}, now interface{}) *FakeEvaluator_QueriesAndExpressionsEval_Call {
return &FakeEvaluator_QueriesAndExpressionsEval_Call{Call: _e.mock.On("QueriesAndExpressionsEval", ctx, _a1, data, now)}
}
@@ -120,17 +120,41 @@ func (_c *FakeEvaluator_QueriesAndExpressionsEval_Call) Return(_a0 *backend.Quer
return _c
}
type mockConstructorTestingTNewFakeEvaluator interface {
mock.TestingT
Cleanup(func())
// Validate provides a mock function with given fields: ctx, _a1, condition
func (_m *FakeEvaluator) Validate(ctx context.Context, _a1 *user.SignedInUser, condition models.Condition) error {
ret := _m.Called(ctx, _a1, condition)
var r0 error
if rf, ok := ret.Get(0).(func(context.Context, *user.SignedInUser, models.Condition) error); ok {
r0 = rf(ctx, _a1, condition)
} else {
r0 = ret.Error(0)
}
return r0
}
// NewFakeEvaluator creates a new instance of FakeEvaluator. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations.
func NewFakeEvaluator(t mockConstructorTestingTNewFakeEvaluator) *FakeEvaluator {
mock := &FakeEvaluator{}
mock.Mock.Test(t)
// FakeEvaluator_Validate_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'Validate'
type FakeEvaluator_Validate_Call struct {
*mock.Call
}
t.Cleanup(func() { mock.AssertExpectations(t) })
// Validate is a helper method to define mock.On call
// - ctx context.Context
// - _a1 *user.SignedInUser
// - condition models.Condition
func (_e *FakeEvaluator_Expecter) Validate(ctx interface{}, _a1 interface{}, condition interface{}) *FakeEvaluator_Validate_Call {
return &FakeEvaluator_Validate_Call{Call: _e.mock.On("Validate", ctx, _a1, condition)}
}
return mock
func (_c *FakeEvaluator_Validate_Call) Run(run func(ctx context.Context, _a1 *user.SignedInUser, condition models.Condition)) *FakeEvaluator_Validate_Call {
_c.Call.Run(func(args mock.Arguments) {
run(args[0].(context.Context), args[1].(*user.SignedInUser), args[2].(models.Condition))
})
return _c
}
func (_c *FakeEvaluator_Validate_Call) Return(_a0 error) *FakeEvaluator_Validate_Call {
_c.Call.Return(_a0)
return _c
}
@@ -180,7 +180,6 @@ func (alertRule *AlertRule) GetLabels(opts ...LabelOption) map[string]string {
func (alertRule *AlertRule) GetEvalCondition() Condition {
return Condition{
Condition: alertRule.Condition,
OrgID: alertRule.OrgID,
Data: alertRule.Data,
}
}
@@ -392,7 +391,6 @@ type Condition struct {
// Condition is the RefID of the query or expression from
// the Data property to get the results for.
Condition string `json:"condition"`
OrgID int64 `json:"-"`
// Data is an array of data source queries and/or server side expressions.
Data []AlertQuery `json:"data"`
+43
View File
@@ -8,6 +8,7 @@ import (
"github.com/grafana/grafana-plugin-sdk-go/data"
"github.com/grafana/grafana/pkg/expr"
models2 "github.com/grafana/grafana/pkg/models"
"github.com/grafana/grafana/pkg/util"
)
@@ -276,3 +277,45 @@ func CopyRule(r *AlertRule) *AlertRule {
return &result
}
func CreateClassicConditionExpression(refID string, inputRefID string, reducer string, operation string, threshold int) AlertQuery {
return AlertQuery{
RefID: refID,
QueryType: expr.DatasourceType,
DatasourceUID: expr.OldDatasourceUID,
// the format corresponds to model `ClassicConditionJSON` in /pkg/expr/classic/classic.go
Model: json.RawMessage(fmt.Sprintf(`
{
"refId": "%[1]s",
"hide": false,
"type": "classic_conditions",
"datasource": {
"uid": "%[6]s",
"type": "%[7]s"
},
"conditions": [
{
"type": "query",
"evaluator": {
"params": [
%[4]d
],
"type": "%[3]s"
},
"operator": {
"type": "and"
},
"query": {
"params": [
"%[2]s"
]
},
"reducer": {
"params": [],
"type": "%[5]s"
}
}
]
}`, refID, inputRefID, operation, threshold, reducer, expr.OldDatasourceUID, expr.DatasourceType)),
}
}