Checker fails with "Expression result must be state value" on incorrect expressions

[borovskyav]

We need to fix advanced mode expressions to avoid situations when expression evaluation result is not a state value.

For example, a user can enter t1 < 300 ? ERROR : foobar as an expression. It is valid, and it produces ERROR when t1 is less than 300. But then t1 is greater or equal to 300, this expression produces result that is not mapped to any valid Moira state. A correct expression would be, for example, t1 < 300 ? ERROR : OK.

Worse of all, this leads to an error inside Moira checker, but not to any visible error message for user.

We propose two different solutions for this issue:

1. (a better one) Validate expressions on trigger save and disallow any expressions that can lead to incorrect values.
2. Switch trigger to special EXCEPTION state with an error message to show user that this expression is incorrect.

You can use https://github.com/moira-alert/moira/blob/master/expression/expression_test.go to test different expressions.

[kamaev]

Можно попробовать запретить вводить expression'ы без обоих операций ? и :

Если проверка будет строгой, то сломается паттерн триггера, который мониторит только nodata с "expression":"OK"

[iurimateus]

I'll take this one

[iurimateus]

I made some attempts to fix it, but I'm not sure if they're proper. Both work, although it doesn't address the evaluation of the expression, relying on parsing instead.

On api/controller/trigger.go/saveTrigger()

• set validTokens to a slice of valid/acceptable moira.State values and possible targets
• grab variables by constructing a govalute expression, then use the Vars() method
• for each variable, check if it's a validToken

Code: https://github.com/iurimateus/moira/commit/d2982df427ebaa15313f26648ca682369defda56 However, I don't like how it flows and the hard-coded moira.State values.

Another attempt: On expression/expression.go Evaluate() https://github.com/moira-alert/moira/blob/683fa52a6b6a980f2b3ffa37ba6ca031186beb0a/expression/expression.go#L78-L80

• For each variable from expr.Vars(), take advantage of the Get method, checking for errors

https://github.com/moira-alert/moira/blob/683fa52a6b6a980f2b3ffa37ba6ca031186beb0a/expression/expression.go#L44-L45

Cleaner, and doesn't depend on copying different moira.State values.

Code: https://github.com/iurimateus/moira/commit/37fe4c10a9c64169c85c0ecb47c486ce39b6e21c Problem: Not sure if it's intended scope for the function, or may cause unexpected behavior.

I've also thought about forcing the evaluation of different parameters, so as to satisfy both sides of a ternary condition, letting the Eval method of govaluate.EvaluableExpression deal with it, but I'm not sure how to go about it.

[beevee]

@iurimateus I think both your approaches fail to catch errors like t1<300 ? ERROR. This expression contains only valid variables, but still doesn't produce a valid state when t1 is greater than 300.

[beevee]

I think, in this task we should try to catch as many expression errors as possible. Ideally, users should not be able to save an erroneous expression.

And, as a last resort, if we failed to catch an error on trigger save, we should show user an exception state on failed trigger check (see https://github.com/moira-alert/moira/blob/8fb128733d66a2b2a4121c5cf147202dd9fecead/checker/check.go#L187-L190).

[shiv6146]

@beevee @borovskyav Can I take up this issue if its still open?

[hv7214]

@beevee I see two issues here:

1. When variables/states are undefined, t1 > 10 ? ERROR : WRONGSTATE.
2. When ternary expression syntax is wrong.

[hv7214]

And to be generic about second point, t1 < 10 ? ERROR is passed

[beevee]

@hv7214 these are two possible ways to make a broken expression, yes. But we need to validate all possible mistakes, not just these two.

[hv7214]

@beevee apart from these two, I think others are properly handled by expression.go. In fact, I have performed some erroneous tests, only the above ones passed without any issue.

[beevee]

@hv7214 okay. You can make a PR fixing these, and we'll try to come up with different ways to bypass your checks :-)

[hv7214]

@beevee sure ; )