feat(api): added full trigger validation in api (#892)

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
 	}
 

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)
 }

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)
+ | ................^`)
 	})
 }
 

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

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=

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,19 +31,77 @@ 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)
+		}
 	}
 }
 
 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)
+		}
 	}
 }