From 2d38664fe68db74108625029ab084dfe412c7996 Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Wed, 21 Sep 2022 15:14:11 -0400 Subject: [PATCH] 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 --- pkg/expr/classic/classic.go | 6 +- pkg/expr/commands.go | 39 +++--- pkg/expr/graph.go | 2 +- pkg/expr/graph_test.go | 2 +- pkg/expr/mathexp/reduce.go | 2 +- pkg/expr/nodes.go | 6 +- pkg/services/ngalert/api/api.go | 22 ++-- pkg/services/ngalert/api/api_ruler.go | 29 +++-- pkg/services/ngalert/api/api_ruler_test.go | 1 - .../ngalert/api/api_ruler_validation.go | 1 - pkg/services/ngalert/api/api_testing.go | 7 +- pkg/services/ngalert/api/api_testing_test.go | 5 + pkg/services/ngalert/api/util.go | 59 --------- pkg/services/ngalert/eval/eval.go | 44 ++++++- pkg/services/ngalert/eval/eval_test.go | 115 ++++++++++++++++++ pkg/services/ngalert/eval/evaluator_mock.go | 60 ++++++--- pkg/services/ngalert/models/alert_rule.go | 2 - pkg/services/ngalert/models/testing.go | 43 +++++++ .../api/alerting/api_alertmanager_test.go | 12 +- 19 files changed, 311 insertions(+), 146 deletions(-) diff --git a/pkg/expr/classic/classic.go b/pkg/expr/classic/classic.go index 6520d5be0d3..2fd26fb0b5e 100644 --- a/pkg/expr/classic/classic.go +++ b/pkg/expr/classic/classic.go @@ -203,19 +203,19 @@ func UnmarshalConditionsCmd(rawQuery map[string]interface{}, refID string) (*Con cond := condition{} if i > 0 && cj.Operator.Type != "and" && cj.Operator.Type != "or" { - return nil, fmt.Errorf("classic condition %v operator must be `and` or `or`", i+1) + return nil, fmt.Errorf("condition %v operator must be `and` or `or`", i+1) } cond.Operator = cj.Operator.Type if len(cj.Query.Params) == 0 || cj.Query.Params[0] == "" { - return nil, fmt.Errorf("classic condition %v is missing the query refID argument", i+1) + return nil, fmt.Errorf("condition %v is missing the query refID argument", i+1) } cond.QueryRefID = cj.Query.Params[0] cond.Reducer = classicReducer(cj.Reducer.Type) if !cond.Reducer.ValidReduceFunc() { - return nil, fmt.Errorf("reducer '%v' in condition %v is not a valid reducer", cond.Reducer, i+1) + return nil, fmt.Errorf("invalid reducer '%v' in condition %v", cond.Reducer, i+1) } cond.Evaluator, err = newAlertEvaluator(cj.Evaluator) diff --git a/pkg/expr/commands.go b/pkg/expr/commands.go index 16ea10a4366..33052cec2db 100644 --- a/pkg/expr/commands.go +++ b/pkg/expr/commands.go @@ -2,6 +2,7 @@ package expr import ( "context" + "errors" "fmt" "strings" "time" @@ -43,16 +44,16 @@ func NewMathCommand(refID, expr string) (*MathCommand, error) { func UnmarshalMathCommand(rn *rawNode) (*MathCommand, error) { rawExpr, ok := rn.Query["expression"] if !ok { - return nil, fmt.Errorf("math command for refId %v is missing an expression", rn.RefID) + return nil, errors.New("command is missing an expression") } exprString, ok := rawExpr.(string) if !ok { - return nil, fmt.Errorf("expected math command for refId %v expression to be a string, got %T", rn.RefID, rawExpr) + return nil, fmt.Errorf("math expression is expected to be a string, got %T", rawExpr) } gm, err := NewMathCommand(rn.RefID, exprString) if err != nil { - return nil, fmt.Errorf("invalid math command type in '%v': %v", rn.RefID, err) + return nil, fmt.Errorf("invalid math command type: %w", err) } return gm, nil } @@ -96,21 +97,21 @@ func NewReduceCommand(refID, reducer, varToReduce string, mapper mathexp.ReduceM func UnmarshalReduceCommand(rn *rawNode) (*ReduceCommand, error) { rawVar, ok := rn.Query["expression"] if !ok { - return nil, fmt.Errorf("no variable specified to reduce for refId %v", rn.RefID) + return nil, errors.New("no expression ID is specified to reduce. Must be a reference to an existing query or expression") } varToReduce, ok := rawVar.(string) if !ok { - return nil, fmt.Errorf("expected reduce variable to be a string, got %T for refId %v", rawVar, rn.RefID) + return nil, fmt.Errorf("expression ID is expected to be a string, got %T", rawVar) } varToReduce = strings.TrimPrefix(varToReduce, "$") rawReducer, ok := rn.Query["reducer"] if !ok { - return nil, fmt.Errorf("no reducer specified for refId %v", rn.RefID) + return nil, errors.New("no reducer specified") } redFunc, ok := rawReducer.(string) if !ok { - return nil, fmt.Errorf("expected reducer to be a string, got %T for refId %v", rawReducer, rn.RefID) + return nil, fmt.Errorf("expected reducer to be a string, got %T", rawReducer) } var mapper mathexp.ReduceMapper = nil @@ -126,20 +127,20 @@ func UnmarshalReduceCommand(rn *rawNode) (*ReduceCommand, error) { case "replaceNN": valueStr, ok := s["replaceWithValue"] if !ok { - return nil, fmt.Errorf("expected settings.replaceWithValue to be specified when mode is 'replaceNN' for refId %v", rn.RefID) + return nil, errors.New("setting replaceWithValue must be specified when mode is 'replaceNN'") } switch value := valueStr.(type) { case float64: mapper = mathexp.ReplaceNonNumberWithValue{Value: value} default: - return nil, fmt.Errorf("expected settings.replaceWithValue to be a number, got %T for refId %v", value, rn.RefID) + return nil, fmt.Errorf("setting replaceWithValue must be a number, got %T", value) } default: - return nil, fmt.Errorf("reducer mode %s is not supported for refId %v. Supported only: [dropNN,replaceNN]", mode, rn.RefID) + return nil, fmt.Errorf("reducer mode '%s' is not supported. Supported only: [dropNN,replaceNN]", mode) } } default: - return nil, fmt.Errorf("expected settings to be an object, got %T for refId %v", s, rn.RefID) + return nil, fmt.Errorf("field settings must be an object, got %T for refId %v", s, rn.RefID) } } return NewReduceCommand(rn.RefID, redFunc, varToReduce, mapper) @@ -211,40 +212,40 @@ func NewResampleCommand(refID, rawWindow, varToResample string, downsampler stri func UnmarshalResampleCommand(rn *rawNode) (*ResampleCommand, error) { rawVar, ok := rn.Query["expression"] if !ok { - return nil, fmt.Errorf("no variable to resample for refId %v", rn.RefID) + return nil, errors.New("no expression ID to resample. must be a reference to an existing query or expression") } varToReduce, ok := rawVar.(string) if !ok { - return nil, fmt.Errorf("expected resample input variable to be type string, but got type %T for refId %v", rawVar, rn.RefID) + return nil, fmt.Errorf("expected resample input variable to be type string, but got type %T", rawVar) } varToReduce = strings.TrimPrefix(varToReduce, "$") varToResample := varToReduce rawWindow, ok := rn.Query["window"] if !ok { - return nil, fmt.Errorf("no time duration specified for the window in resample command for refId %v", rn.RefID) + return nil, errors.New("no time duration specified for the window in resample command") } window, ok := rawWindow.(string) if !ok { - return nil, fmt.Errorf("expected resample window to be a string, got %T for refId %v", rawWindow, rn.RefID) + return nil, fmt.Errorf("resample window is expected to be a string, got %T", rawWindow) } rawDownsampler, ok := rn.Query["downsampler"] if !ok { - return nil, fmt.Errorf("no downsampler function specified in resample command for refId %v", rn.RefID) + return nil, errors.New("no downsampler function specified in resample command") } downsampler, ok := rawDownsampler.(string) if !ok { - return nil, fmt.Errorf("expected resample downsampler to be a string, got type %T for refId %v", downsampler, rn.RefID) + return nil, fmt.Errorf("expected resample downsampler to be a string, got type %T", downsampler) } rawUpsampler, ok := rn.Query["upsampler"] if !ok { - return nil, fmt.Errorf("no downsampler specified in resample command for refId %v", rn.RefID) + return nil, errors.New("no upsampler specified in resample command") } upsampler, ok := rawUpsampler.(string) if !ok { - return nil, fmt.Errorf("expected resample downsampler to be a string, got type %T for refId %v", upsampler, rn.RefID) + return nil, fmt.Errorf("expected resample downsampler to be a string, got type %T", upsampler) } return NewResampleCommand(rn.RefID, window, varToResample, downsampler, upsampler, rn.TimeRange) diff --git a/pkg/expr/graph.go b/pkg/expr/graph.go index 9a9055d8f62..9f5a59d7415 100644 --- a/pkg/expr/graph.go +++ b/pkg/expr/graph.go @@ -189,7 +189,7 @@ func buildGraphEdges(dp *simple.DirectedGraph, registry map[string]Node) error { } if neededNode.ID() == cmdNode.ID() { - return fmt.Errorf("can not add self referencing node for var '%v' ", neededVar) + return fmt.Errorf("expression '%v' cannot reference itself. Must be query or another expression", neededVar) } if cmdNode.CMDType == TypeClassicConditions { diff --git a/pkg/expr/graph_test.go b/pkg/expr/graph_test.go index e69a6c3f5f0..325118bafaf 100644 --- a/pkg/expr/graph_test.go +++ b/pkg/expr/graph_test.go @@ -77,7 +77,7 @@ func TestServicebuildPipeLine(t *testing.T) { }, }, }, - expectErrContains: "self referencing node", + expectErrContains: "expression 'A' cannot reference itself. Must be query or another expression", }, { name: "missing dependency will error", diff --git a/pkg/expr/mathexp/reduce.go b/pkg/expr/mathexp/reduce.go index 1af9ffac71c..b4b6611eb85 100644 --- a/pkg/expr/mathexp/reduce.go +++ b/pkg/expr/mathexp/reduce.go @@ -123,7 +123,7 @@ func (s Series) Reduce(refID, rFunc string, mapper ReduceMapper) (Number, error) floatField := Float64Field(*fVec) reduceFunc, err := GetReduceFunc(rFunc) if err != nil { - return number, err + return number, fmt.Errorf("invalid expression '%s': %w", refID, err) } f = reduceFunc(&floatField) if f != nil && mapper != nil { diff --git a/pkg/expr/nodes.go b/pkg/expr/nodes.go index 4c49bf133ef..333cd1bb9c6 100644 --- a/pkg/expr/nodes.go +++ b/pkg/expr/nodes.go @@ -99,7 +99,7 @@ func (gn *CMDNode) Execute(ctx context.Context, vars mathexp.Vars, s *Service) ( func buildCMDNode(dp *simple.DirectedGraph, rn *rawNode) (*CMDNode, error) { commandType, err := rn.GetCommandType() if err != nil { - return nil, fmt.Errorf("invalid expression command type in '%v'", rn.RefID) + return nil, fmt.Errorf("invalid command type in expression '%v': %w", rn.RefID, err) } node := &CMDNode{ @@ -120,10 +120,10 @@ func buildCMDNode(dp *simple.DirectedGraph, rn *rawNode) (*CMDNode, error) { case TypeClassicConditions: node.Command, err = classic.UnmarshalConditionsCmd(rn.Query, rn.RefID) default: - return nil, fmt.Errorf("expression command type '%v' in '%v' not implemented", commandType, rn.RefID) + return nil, fmt.Errorf("expression command type '%v' in expression '%v' not implemented", commandType, rn.RefID) } if err != nil { - return nil, err + return nil, fmt.Errorf("failed to parse expression '%v': %w", rn.RefID, err) } return node, nil diff --git a/pkg/services/ngalert/api/api.go b/pkg/services/ngalert/api/api.go index 21466fcb2a2..2525ffa410d 100644 --- a/pkg/services/ngalert/api/api.go +++ b/pkg/services/ngalert/api/api.go @@ -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{ diff --git a/pkg/services/ngalert/api/api_ruler.go b/pkg/services/ngalert/api/api_ruler.go index 6ded48c03c1..5562f13eff0 100644 --- a/pkg/services/ngalert/api/api_ruler.go +++ b/pkg/services/ngalert/api/api_ruler.go @@ -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, "") } diff --git a/pkg/services/ngalert/api/api_ruler_test.go b/pkg/services/ngalert/api/api_ruler_test.go index 082ad2f57d8..b8dbb4e7939 100644 --- a/pkg/services/ngalert/api/api_ruler_test.go +++ b/pkg/services/ngalert/api/api_ruler_test.go @@ -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, diff --git a/pkg/services/ngalert/api/api_ruler_validation.go b/pkg/services/ngalert/api/api_ruler_validation.go index ea1eb185acb..5b4e36c272d 100644 --- a/pkg/services/ngalert/api/api_ruler_validation.go +++ b/pkg/services/ngalert/api/api_ruler_validation.go @@ -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 { diff --git a/pkg/services/ngalert/api/api_testing.go b/pkg/services/ngalert/api/api_testing.go index 85a136f76d2..c644dc1988a 100644 --- a/pkg/services/ngalert/api/api_testing.go +++ b/pkg/services/ngalert/api/api_testing.go @@ -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") diff --git a/pkg/services/ngalert/api/api_testing_test.go b/pkg/services/ngalert/api/api_testing_test.go index 7df02986af8..5bd74ab7013 100644 --- a/pkg/services/ngalert/api/api_testing_test.go +++ b/pkg/services/ngalert/api/api_testing_test.go @@ -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) }) }) } diff --git a/pkg/services/ngalert/api/util.go b/pkg/services/ngalert/api/util.go index 543c08c7811..9519eaae9ae 100644 --- a/pkg/services/ngalert/api/util.go +++ b/pkg/services/ngalert/api/util.go @@ -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 != "" { diff --git a/pkg/services/ngalert/eval/eval.go b/pkg/services/ngalert/eval/eval.go index 4a847c3c791..b29ba2f9290 100644 --- a/pkg/services/ngalert/eval/eval.go +++ b/pkg/services/ngalert/eval/eval.go @@ -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) +} diff --git a/pkg/services/ngalert/eval/eval_test.go b/pkg/services/ngalert/eval/eval_test.go index 0f6a1f6d3c2..7a1ff6215cc 100644 --- a/pkg/services/ngalert/eval/eval_test.go +++ b/pkg/services/ngalert/eval/eval_test.go @@ -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) + } + }) + } +} diff --git a/pkg/services/ngalert/eval/evaluator_mock.go b/pkg/services/ngalert/eval/evaluator_mock.go index bb3f7d3d1f0..d2ad62726f5 100644 --- a/pkg/services/ngalert/eval/evaluator_mock.go +++ b/pkg/services/ngalert/eval/evaluator_mock.go @@ -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 } diff --git a/pkg/services/ngalert/models/alert_rule.go b/pkg/services/ngalert/models/alert_rule.go index 1f545cdb9ad..163503cc048 100644 --- a/pkg/services/ngalert/models/alert_rule.go +++ b/pkg/services/ngalert/models/alert_rule.go @@ -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"` diff --git a/pkg/services/ngalert/models/testing.go b/pkg/services/ngalert/models/testing.go index 33988cd9a76..a8c3c57b76f 100644 --- a/pkg/services/ngalert/models/testing.go +++ b/pkg/services/ngalert/models/testing.go @@ -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)), + } +} diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index 4068a53b89d..5e3afbef2a9 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -1026,7 +1026,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedMessage: "invalid rule specification at index [0]: failed to validate condition of alert rule AlwaysFiring: invalid query A: data source not found: unknown", + expectedMessage: "invalid rule specification at index [0]: failed to validate condition of alert rule AlwaysFiring: failed to build query 'A': data source not found", }, { desc: "alert rule with invalid condition", @@ -1056,7 +1056,7 @@ func TestAlertRuleCRUD(t *testing.T) { }, }, }, - expectedMessage: "invalid rule specification at index [0]: failed to validate condition of alert rule AlwaysFiring: condition B not found in any query or expression: it should be one of: [A]", + expectedMessage: "invalid rule specification at index [0]: failed to validate condition of alert rule AlwaysFiring: condition B does not exist, must be one of [A]", }, } @@ -1074,7 +1074,7 @@ func TestAlertRuleCRUD(t *testing.T) { err = json.Unmarshal([]byte(body), &res) require.NoError(t, err) - assert.Equal(t, res.Message, tc.expectedMessage) + assert.Equal(t, tc.expectedMessage, res.Message) assert.Equal(t, http.StatusBadRequest, status) }) @@ -2264,7 +2264,7 @@ func TestEval(t *testing.T) { `, expectedStatusCode: func() int { return http.StatusBadRequest }, expectedMessage: func() string { - return "invalid condition: condition B not found in any query or expression: it should be one of: [A]" + return "invalid condition: condition B does not exist, must be one of [A]" }, expectedResponse: func() string { return "" }, }, @@ -2300,7 +2300,7 @@ func TestEval(t *testing.T) { if setting.IsEnterprise { return "user is not authorized to query one or many data sources used by the rule" } - return "invalid condition: invalid query A: data source not found: unknown" + return "invalid condition: failed to build query 'A': data source not found" }, expectedResponse: func() string { return "" }, }, @@ -2483,7 +2483,7 @@ func TestEval(t *testing.T) { if setting.IsEnterprise { return "user is not authorized to query one or many data sources used by the rule" } - return "invalid queries or expressions: invalid query A: data source not found: unknown" + return "Failed to evaluate queries and expressions: failed to execute conditions: failed to build query 'A': data source not found" }, }, }