From aee926993c2663d159185083cf5550c3d2e437a9 Mon Sep 17 00:00:00 2001 From: Owamoyo Evans Date: Tue, 7 Nov 2023 15:07:08 +0300 Subject: [PATCH 1/3] feat(api): added full trigger validation in api (#892) --- api/dto/triggers.go | 4 +- expression/expression.go | 58 +++++++++++----- expression/expression_test.go | 42 ++++++++--- go.mod | 5 +- go.sum | 2 + .../expression/expression_test.go | 69 ++++++++++++++++++- 6 files changed, 150 insertions(+), 30 deletions(-) diff --git a/api/dto/triggers.go b/api/dto/triggers.go index 78755b53f..a95831ec6 100644 --- a/api/dto/triggers.go +++ b/api/dto/triggers.go @@ -185,7 +185,7 @@ func (trigger *Trigger) Bind(request *http.Request) error { return err } - if err := checkTTLSanity(trigger, metricsSource); err != nil { + if err = checkTTLSanity(trigger, metricsSource); err != nil { return api.ErrInvalidRequestContent{ValidationError: err} } @@ -202,7 +202,7 @@ func (trigger *Trigger) Bind(request *http.Request) error { middleware.SetTimeSeriesNames(request, metricsDataNames) - if _, err := triggerExpression.Evaluate(); err != nil { + if err = triggerExpression.Validate(); err != nil { return err } diff --git a/expression/expression.go b/expression/expression.go index 690a4c486..6d7fdfec8 100644 --- a/expression/expression.go +++ b/expression/expression.go @@ -4,9 +4,11 @@ import ( "fmt" "strings" + "github.com/antonmedv/expr" "github.com/patrickmn/go-cache" "github.com/Knetic/govaluate" + "github.com/moira-alert/moira" ) @@ -77,6 +79,45 @@ func (triggerExpression TriggerExpression) Get(name string) (interface{}, error) } } +// Validate returns error if triggers of type moira.ExpressionTrigger are badly formatted otherwise nil +func (triggerExpression *TriggerExpression) Validate() error { + if triggerExpression.TriggerType != moira.ExpressionTrigger { + return nil + } + if triggerExpression.Expression == nil || *triggerExpression.Expression == "" { + return ErrInvalidExpression{ + internalError: fmt.Errorf("trigger_type set to expression, but no expression provided"), + } + } + expression := *triggerExpression.Expression + env := map[string]interface{}{ + "ok": moira.StateOK, + "error": moira.StateERROR, + "warn": moira.StateWARN, + "warning": moira.StateWARN, + "nodata": moira.StateNODATA, + "t1": triggerExpression.MainTargetValue, + "prev_state": triggerExpression.PreviousState, + } + if triggerExpression.WarnValue != nil { + env["warn_value"] = *triggerExpression.WarnValue + } + if triggerExpression.ErrorValue != nil { + env["error_value"] = *triggerExpression.ErrorValue + } + for k, v := range triggerExpression.AdditionalTargetsValues { + env[k] = v + } + if _, err := expr.Compile( + strings.ToLower(expression), + expr.Optimize(true), + expr.Env(env), + ); err != nil { + return ErrInvalidExpression{err} + } + return nil +} + // Evaluate gets trigger expression and evaluates it for given parameters using govaluate func (triggerExpression *TriggerExpression) Evaluate() (moira.State, error) { expr, err := getExpression(triggerExpression) @@ -95,27 +136,12 @@ func (triggerExpression *TriggerExpression) Evaluate() (moira.State, error) { } } -func validateUserExpression(triggerExpression *TriggerExpression, userExpression *govaluate.EvaluableExpression) (*govaluate.EvaluableExpression, error) { - for _, v := range userExpression.Vars() { - if _, err := triggerExpression.Get(v); err != nil { - return nil, fmt.Errorf("invalid variable value: %w", err) - } - } - return userExpression, nil -} - func getExpression(triggerExpression *TriggerExpression) (*govaluate.EvaluableExpression, error) { if triggerExpression.TriggerType == moira.ExpressionTrigger { if triggerExpression.Expression == nil || *triggerExpression.Expression == "" { return nil, fmt.Errorf("trigger_type set to expression, but no expression provided") } - - userExpression, err := getUserExpression(*triggerExpression.Expression) - if err != nil { - return nil, err - } - - return validateUserExpression(triggerExpression, userExpression) + return getUserExpression(*triggerExpression.Expression) } return getSimpleExpression(triggerExpression) } diff --git a/expression/expression_test.go b/expression/expression_test.go index 090f0d9b2..498768510 100644 --- a/expression/expression_test.go +++ b/expression/expression_test.go @@ -4,8 +4,9 @@ import ( "fmt" "testing" - "github.com/moira-alert/moira" . "github.com/smartystreets/goconvey/convey" + + "github.com/moira-alert/moira" ) type getExpressionValuesTest struct { @@ -86,16 +87,41 @@ func TestExpression(t *testing.T) { result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger, PreviousState: moira.StateNODATA}).Evaluate() So(err, ShouldBeNil) So(result, ShouldResemble, moira.StateNODATA) + }) +} - expression = "t1 > 10 && t2 > 3 ? OK : ddd" - result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Evaluate() - So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("invalid variable value: %w", fmt.Errorf("no value with name ddd"))}) - So(result, ShouldBeEmpty) +func TestValidate(t *testing.T) { + Convey("Test valid expressions", t, func() { + expression := "t1 > 10 && t2 > 3 ? OK : ERROR" + err := (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldBeNil) + + expression = "t1 <= 0 ? PREV_STATE : (t1 >= 20 ? ERROR : (t1 >= 10 ? WARN : OK))" + err = (&TriggerExpression{PreviousState: moira.StateNODATA, Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldBeNil) + + warnValue, errorValue := 60.0, 90.0 + err = (&TriggerExpression{PreviousState: moira.StateNODATA, Expression: nil, WarnValue: &warnValue, ErrorValue: &errorValue, TriggerType: moira.RisingTrigger, MainTargetValue: 5}).Validate() + SoMsg("validating simple expression", err, ShouldBeNil) + }) + Convey("Test bad expressions", t, func() { + err := (&TriggerExpression{Expression: nil, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("trigger_type set to expression, but no expression provided")}) + }) + Convey("Test invalid expressions", t, func() { + expression := "t1 > 10 && t2 > 3 ? OK : ddd" + err := (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldNotBeNil) + So(err.Error(), ShouldResemble, `unknown name ddd (1:26) + | t1 > 10 && t2 > 3 ? ok : ddd + | .........................^`) expression = "t1 > 10 ? OK : (t2 < 5 ? WARN : ERROR)" - result, err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Evaluate() - So(err, ShouldResemble, ErrInvalidExpression{fmt.Errorf("invalid variable value: %w", fmt.Errorf("no value with name t2"))}) - So(result, ShouldBeEmpty) + err = (&TriggerExpression{Expression: &expression, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{}, TriggerType: moira.ExpressionTrigger}).Validate() + So(err, ShouldNotBeNil) + So(err.Error(), ShouldResemble, `unknown name t2 (1:17) + | t1 > 10 ? ok : (t2 < 5 ? warn : error) + | ................^`) }) } diff --git a/go.mod b/go.mod index 37386c962..22caf6794 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,8 @@ require ( github.com/Knetic/govaluate v3.0.1-0.20171022003610-9aa49832a739+incompatible github.com/PagerDuty/go-pagerduty v1.5.1 github.com/ansel1/merry v1.6.2 - github.com/aws/aws-sdk-go v1.44.293 + github.com/antonmedv/expr v1.12.7 + github.com/aws/aws-sdk-go v1.44.219 github.com/blevesearch/bleve/v2 v2.3.8 github.com/bwmarrin/discordgo v0.25.0 github.com/carlosdp/twiliogo v0.0.0-20161027183705-b26045ebb9d1 @@ -26,6 +27,8 @@ require ( github.com/gotokatsuya/ipare v0.0.0-20161202043954-fd52c5b6c44b github.com/gregdel/pushover v1.1.0 github.com/karriereat/blackfriday-slack v0.1.0 + github.com/mattermost/mattermost-server/v6 v6.0.0-20230405170428-2a75f997ee6c // it is last commit of 7.9.2 (https://github.com/mattermost/mattermost-server/commits/v7.9.2). Can't use v7, issue https://github.com/mattermost/mattermost-server/issues/20817 + github.com/mitchellh/mapstructure v1.5.0 github.com/moira-alert/go-chart v0.0.0-20230220064910-812fb2829b9b github.com/opsgenie/opsgenie-go-sdk-v2 v1.2.13 github.com/patrickmn/go-cache v2.1.0+incompatible diff --git a/go.sum b/go.sum index f73e0f1bf..315598b26 100644 --- a/go.sum +++ b/go.sum @@ -440,6 +440,8 @@ github.com/ansel1/merry/v2 v2.1.1 h1:Ax0gQh7Z/GfimoVg2EDBAU6CJIieWwVvhtBKJdkCE1M github.com/ansel1/merry/v2 v2.1.1/go.mod h1:4p/FFyQbCgqlDbseWOVQaL5USpgkE9sr5xh4V6Ry0JU= github.com/ansel1/vespucci/v4 v4.1.1/go.mod h1:zzdrO4IgBfgcGMbGTk/qNGL8JPslmW3nPpcBHKReFYY= github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kdvxnR2qWY= +github.com/antonmedv/expr v1.12.7 h1:jfV/l/+dHWAadLwAtESXNxXdfbK9bE4+FNMHYCMntwk= +github.com/antonmedv/expr v1.12.7/go.mod h1:FPC8iWArxls7axbVLsW+kpg1mz29A1b2M6jt+hZfDkU= github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8= github.com/aws/aws-sdk-go v1.44.293 h1:oBPrQqsyMYe61Sl/xKVvQFflXjPwYH11aKi8QR3Nhts= github.com/aws/aws-sdk-go v1.44.293/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI= diff --git a/perfomance_tests/expression/expression_test.go b/perfomance_tests/expression/expression_test.go index d979a479a..92977e5b1 100644 --- a/perfomance_tests/expression/expression_test.go +++ b/perfomance_tests/expression/expression_test.go @@ -3,6 +3,7 @@ package expression import ( "testing" + "github.com/moira-alert/moira" "github.com/moira-alert/moira/expression" ) @@ -13,9 +14,13 @@ func BenchmarkDefault1Expr(b *testing.B) { MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, + TriggerType: moira.RisingTrigger, } for i := 0; i < b.N; i++ { - (expr).Evaluate() //nolint + _, err := expr.Evaluate() + if err != nil { + b.Log(err) + } } } @@ -26,9 +31,13 @@ func BenchmarkDefault2Expr(b *testing.B) { MainTargetValue: 10.0, WarnValue: &warnValue, ErrorValue: &errorValue, + TriggerType: moira.RisingTrigger, } for i := 0; i < b.N; i++ { - (expr).Evaluate() //nolint + _, err := expr.Evaluate() + if err != nil { + b.Log(err) + } } } @@ -36,9 +45,63 @@ func BenchmarkCustomExpr(b *testing.B) { expressionStr := "t1 > 10 && t2 > 3 ? ERROR : OK" expr := &expression.TriggerExpression{ Expression: &expressionStr, + TriggerType: moira.ExpressionTrigger, MainTargetValue: 11.0, AdditionalTargetsValues: map[string]float64{"t2": 4.0}} for i := 0; i < b.N; i++ { - (expr).Evaluate() //nolint + _, err := expr.Evaluate() + if err != nil { + b.Log(err) + } + } +} + +func BenchmarkValidateComplex(b *testing.B) { + expressionStr := "(t1 * 2 > t2 && t2 / 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" + expr := &expression.TriggerExpression{ + Expression: &expressionStr, + TriggerType: moira.ExpressionTrigger, + MainTargetValue: 4, + AdditionalTargetsValues: map[string]float64{ + "t2": 5, + "t3": 3, + "t4": 6, + "t5": 18, + "t6": 10, + "t7": 15, + "t8": 20, + "t9": 10, + }, + } + for i := 0; i < b.N; i++ { + err := expr.Validate() + if err != nil { + b.Log(err) + } + } +} + +func BenchmarkEvaluateComplex(b *testing.B) { + expressionStr := "(t1 * 2 > t2 && t2 / 2 != 0) || (t3 * t4 == t5 && t6 < t7) ? (t8 > t9 ? OK : WARN) : ERROR" + expr := &expression.TriggerExpression{ + Expression: &expressionStr, + TriggerType: moira.ExpressionTrigger, + MainTargetValue: 4, + AdditionalTargetsValues: map[string]float64{ + "t2": 5, + "t3": 3, + "t4": 6, + "t5": 18, + "t6": 10, + "t7": 15, + "t8": 20, + "t9": 10, + }, + } + for i := 0; i < b.N; i++ { + _, err := expr.Evaluate() + if err != nil { + b.Log(err) + } } } From 573c408ceaddf959c57e5f198575818065ca4140 Mon Sep 17 00:00:00 2001 From: Tetrergeru Date: Thu, 9 Nov 2023 09:38:55 +0100 Subject: [PATCH 2/3] fix: go mod download --- go.sum | 2 ++ 1 file changed, 2 insertions(+) diff --git a/go.sum b/go.sum index 315598b26..633bb0b30 100644 --- a/go.sum +++ b/go.sum @@ -443,6 +443,7 @@ github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kd github.com/antonmedv/expr v1.12.7 h1:jfV/l/+dHWAadLwAtESXNxXdfbK9bE4+FNMHYCMntwk= github.com/antonmedv/expr v1.12.7/go.mod h1:FPC8iWArxls7axbVLsW+kpg1mz29A1b2M6jt+hZfDkU= github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8= +github.com/aws/aws-sdk-go v1.44.219/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI= github.com/aws/aws-sdk-go v1.44.293 h1:oBPrQqsyMYe61Sl/xKVvQFflXjPwYH11aKi8QR3Nhts= github.com/aws/aws-sdk-go v1.44.293/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI= github.com/barkimedes/go-deepcopy v0.0.0-20220514131651-17c30cfc62df h1:GSoSVRLoBaFpOOds6QyY1L8AX7uoY+Ln3BHc22W40X0= @@ -872,6 +873,7 @@ github.com/mattermost/ldap v0.0.0-20201202150706-ee0e6284187d h1:/RJ/UV7M5c7L2TQ github.com/mattermost/ldap v0.0.0-20201202150706-ee0e6284187d/go.mod h1:HLbgMEI5K131jpxGazJ97AxfPDt31osq36YS1oxFQPQ= github.com/mattermost/logr/v2 v2.0.18 h1:qiznuwwKckZJoGtBYc4Y9FAY97/oQwV1Pq9oO5qP5nk= github.com/mattermost/logr/v2 v2.0.18/go.mod h1:1dm/YhTpozsqANXxo5Pi5zYLBsal2xY0pX+JZNbzYJY= +github.com/mattermost/mattermost-server/v6 v6.0.0-20230405170428-2a75f997ee6c/go.mod h1:o61MGMh7We01wGr1ydGDA5mmNpjTzaBVWUAlezsgx50= github.com/mattermost/mattermost/server/public v0.0.9 h1:Qsktgxx5dc8xVAUHP5MbSLi6Cf82iB/83r6S9bluHto= github.com/mattermost/mattermost/server/public v0.0.9/go.mod h1:sgXQrYzs+IJy51mB8E8OBljagk2u3YwQRoYlBH5goiw= github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU= From c82ed49c8b659e0236e9fb8492c65d7d6dfe43f4 Mon Sep 17 00:00:00 2001 From: Tetrergeru Date: Thu, 9 Nov 2023 09:58:15 +0100 Subject: [PATCH 3/3] fix: go get --- go.sum | 1 + 1 file changed, 1 insertion(+) diff --git a/go.sum b/go.sum index 633bb0b30..51d03911b 100644 --- a/go.sum +++ b/go.sum @@ -443,6 +443,7 @@ github.com/antihax/optional v1.0.0/go.mod h1:uupD/76wgC+ih3iEmQUL+0Ugr19nfwCT1kd github.com/antonmedv/expr v1.12.7 h1:jfV/l/+dHWAadLwAtESXNxXdfbK9bE4+FNMHYCMntwk= github.com/antonmedv/expr v1.12.7/go.mod h1:FPC8iWArxls7axbVLsW+kpg1mz29A1b2M6jt+hZfDkU= github.com/armon/go-radix v0.0.0-20180808171621-7fddfc383310/go.mod h1:ufUuZ+zHj4x4TnLV4JWEpy2hxWSpsRywHrMgIH9cCH8= +github.com/aws/aws-sdk-go v1.44.219 h1:YOFxTUQZvdRzgwb6XqLFRwNHxoUdKBuunITC7IFhvbc= github.com/aws/aws-sdk-go v1.44.219/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI= github.com/aws/aws-sdk-go v1.44.293 h1:oBPrQqsyMYe61Sl/xKVvQFflXjPwYH11aKi8QR3Nhts= github.com/aws/aws-sdk-go v1.44.293/go.mod h1:aVsgQcEevwlmQ7qHE9I3h+dtQgpqhFB+i8Phjh7fkwI=