From d5f9619984698d4b4536345df1ce26cac2765a29 Mon Sep 17 00:00:00 2001 From: Dmytro Kozlov Date: Fri, 15 Sep 2023 13:15:23 +0200 Subject: [PATCH] vmagent: add validation of MetricsQL functions (#4991) Co-authored-by: Aliaksandr Valialkin --- app/vmselect/promql/exec_test.go | 2 +- docs/CHANGELOG.md | 1 + go.mod | 2 +- go.sum | 4 +- .../VictoriaMetrics/metricsql/aggr.go | 3 +- .../VictoriaMetrics/metricsql/optimizer.go | 2 +- .../VictoriaMetrics/metricsql/parser.go | 9 ++-- .../VictoriaMetrics/metricsql/utils.go | 42 ++++++++++++++++++- vendor/modules.txt | 2 +- 9 files changed, 56 insertions(+), 11 deletions(-) diff --git a/app/vmselect/promql/exec_test.go b/app/vmselect/promql/exec_test.go index 90d31e349..3290612a7 100644 --- a/app/vmselect/promql/exec_test.go +++ b/app/vmselect/promql/exec_test.go @@ -45,7 +45,7 @@ func TestEscapeDotsInRegexpLabelFilters(t *testing.T) { f("2", "2") f(`foo.bar + 123`, `foo.bar + 123`) f(`foo{bar=~"baz.xx.yyy"}`, `foo{bar=~"baz\\.xx\\.yyy"}`) - f(`foo(a.b{c="d.e",x=~"a.b.+[.a]",y!~"aaa.bb|cc.dd"}) + x.y(1,sum({x=~"aa.bb"}))`, `foo(a.b{c="d.e",x=~"a\\.b.+[\\.a]",y!~"aaa\\.bb|cc\\.dd"}) + x.y(1, sum({x=~"aa\\.bb"}))`) + f(`sum(a.b{c="d.e",x=~"a.b.+[.a]",y!~"aaa.bb|cc.dd"}) + avg_over_time(1,sum({x=~"aa.bb"}))`, `sum(a.b{c="d.e",x=~"a\\.b.+[\\.a]",y!~"aaa\\.bb|cc\\.dd"}) + avg_over_time(1, sum({x=~"aa\\.bb"}))`) } func TestExecSuccess(t *testing.T) { diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 115b61ac2..30c817879 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -42,6 +42,7 @@ The following `tip` changes can be tested by building VictoriaMetrics components ssue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4825) and [these docs](https://docs.victoriametrics.com/vmauth.html#auth-config) for details. * FEATURE: [vmauth](https://docs.victoriametrics.com/vmauth.html): add ability to retry requests to the [remaining backends](https://docs.victoriametrics.com/vmauth.html#load-balancing) if they return response status codes specified in the `retry_status_codes` list. See [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4893). * FEATURE: [vmalert](https://docs.victoriametrics.com/vmalert.html): add `eval_offset` attribute for [Groups](https://docs.victoriametrics.com/vmalert.html#groups). If specified, Group will be evaluated at the exact time offset on the range of [0...evaluationInterval]. The setting might be useful for cron-like rules which must be evaluated at specific moments of time. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3409) for details. +* FEATURE: [vmalert](https://docs.victoriametrics.com/vmalert.html): validate [MetricsQL](https://docs.victoriametrics.com/MetricsQL.html) function names in alerting and recording rules when `vmalert` runs with `-dryRun` command-line flag. Previously it was allowed to use unknown (aka invalid) MetricsQL function names there. For example, `foo()` was counted as a valid query. See [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/4933). * FEATURE: limit the length of string params in log messages to 500 chars. Longer string params are replaced with the `first_250_chars..last_250_chars`. This prevents from too long log lines, which can be emitted by VictoriaMetrics components. * BUGFIX: [Official Grafana dashboards for VictoriaMetrics](https://grafana.com/orgs/victoriametrics): fix display of ingested rows rate for `Samples ingested/s` and `Samples rate` panels for vmagent's dasbhoard. Previously, not all ingested protocols were accounted in these panels. An extra panel `Rows rate` was added to `Ingestion` section to display the split for rows ingested rate by protocol. diff --git a/go.mod b/go.mod index 603c4a567..a6ee13e10 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( // like https://github.com/valyala/fasthttp/commit/996610f021ff45fdc98c2ce7884d5fa4e7f9199b github.com/VictoriaMetrics/fasthttp v1.2.0 github.com/VictoriaMetrics/metrics v1.24.0 - github.com/VictoriaMetrics/metricsql v0.64.0 + github.com/VictoriaMetrics/metricsql v0.65.0 github.com/aws/aws-sdk-go-v2 v1.21.0 github.com/aws/aws-sdk-go-v2/config v1.18.39 github.com/aws/aws-sdk-go-v2/feature/s3/manager v1.11.83 diff --git a/go.sum b/go.sum index 0b5db09a0..9bfe01c62 100644 --- a/go.sum +++ b/go.sum @@ -70,8 +70,8 @@ github.com/VictoriaMetrics/fasthttp v1.2.0 h1:nd9Wng4DlNtaI27WlYh5mGXCJOmee/2c2b github.com/VictoriaMetrics/fasthttp v1.2.0/go.mod h1:zv5YSmasAoSyv8sBVexfArzFDIGGTN4TfCKAtAw7IfE= github.com/VictoriaMetrics/metrics v1.24.0 h1:ILavebReOjYctAGY5QU2F9X0MYvkcrG3aEn2RKa1Zkw= github.com/VictoriaMetrics/metrics v1.24.0/go.mod h1:eFT25kvsTidQFHb6U0oa0rTrDRdz4xTYjpL8+UPohys= -github.com/VictoriaMetrics/metricsql v0.64.0 h1:uty6AXQFY3OpQ+eopo1jDjCcTctuqkqYLnRbQVhukW8= -github.com/VictoriaMetrics/metricsql v0.64.0/go.mod h1:k4UaP/+CjuZslIjd+kCigNG9TQmUqh5v0TP/nMEy90I= +github.com/VictoriaMetrics/metricsql v0.65.0 h1:+/Oit3QycM8z/NbMHy4KENSUDS5q9QRx8h2x6cvoQOk= +github.com/VictoriaMetrics/metricsql v0.65.0/go.mod h1:k4UaP/+CjuZslIjd+kCigNG9TQmUqh5v0TP/nMEy90I= github.com/VividCortex/ewma v1.2.0 h1:f58SaIzcDXrSy3kWaHNvuJgJ3Nmz59Zji6XoJR/q1ow= github.com/VividCortex/ewma v1.2.0/go.mod h1:nz4BbCtbLyFDeC9SUHbtcT5644juEuWfUAUnGx7j5l4= github.com/alecthomas/template v0.0.0-20160405071501-a0175ee3bccc/go.mod h1:LOuyumcjzFXgccqObfd/Ljyb9UuFJ6TxHnclSeseNhc= diff --git a/vendor/github.com/VictoriaMetrics/metricsql/aggr.go b/vendor/github.com/VictoriaMetrics/metricsql/aggr.go index b8c77c349..51bc36a0f 100644 --- a/vendor/github.com/VictoriaMetrics/metricsql/aggr.go +++ b/vendor/github.com/VictoriaMetrics/metricsql/aggr.go @@ -43,7 +43,8 @@ var aggrFuncs = map[string]bool{ "zscore": true, } -func isAggrFunc(s string) bool { +// IsAggrFunc returns whether funcName is a known aggregate function. +func IsAggrFunc(s string) bool { s = strings.ToLower(s) return aggrFuncs[s] } diff --git a/vendor/github.com/VictoriaMetrics/metricsql/optimizer.go b/vendor/github.com/VictoriaMetrics/metricsql/optimizer.go index 8072667cd..046a8f1b8 100644 --- a/vendor/github.com/VictoriaMetrics/metricsql/optimizer.go +++ b/vendor/github.com/VictoriaMetrics/metricsql/optimizer.go @@ -362,7 +362,7 @@ func getFuncArgIdxForOptimization(funcName string, args []Expr) int { if IsTransformFunc(funcName) { return getTransformArgIdxForOptimization(funcName, args) } - if isAggrFunc(funcName) { + if IsAggrFunc(funcName) { return getAggrArgIdxForOptimization(funcName, args) } return -1 diff --git a/vendor/github.com/VictoriaMetrics/metricsql/parser.go b/vendor/github.com/VictoriaMetrics/metricsql/parser.go index bc3f83e7f..c40876857 100644 --- a/vendor/github.com/VictoriaMetrics/metricsql/parser.go +++ b/vendor/github.com/VictoriaMetrics/metricsql/parser.go @@ -31,6 +31,9 @@ func Parse(s string) (Expr, error) { } e = removeParensExpr(e) e = simplifyConstants(e) + if err := checkSupportedFunctions(e); err != nil { + return nil, err + } return e, nil } @@ -596,7 +599,7 @@ func (p *parser) parseParensExpr() (*parensExpr, error) { } func (p *parser) parseAggrFuncExpr() (*AggrFuncExpr, error) { - if !isAggrFunc(p.lex.Token) { + if !IsAggrFunc(p.lex.Token) { return nil, fmt.Errorf(`AggrFuncExpr: unexpected token %q; want aggregate func`, p.lex.Token) } @@ -1608,7 +1611,7 @@ func (p *parser) parseIdentExpr() (Expr, error) { } if isIdentPrefix(p.lex.Token) { p.lex.Prev() - if isAggrFunc(p.lex.Token) { + if IsAggrFunc(p.lex.Token) { return p.parseAggrFuncExpr() } return p.parseMetricExpr() @@ -1620,7 +1623,7 @@ func (p *parser) parseIdentExpr() (Expr, error) { switch p.lex.Token { case "(": p.lex.Prev() - if isAggrFunc(p.lex.Token) { + if IsAggrFunc(p.lex.Token) { return p.parseAggrFuncExpr() } return p.parseFuncExpr() diff --git a/vendor/github.com/VictoriaMetrics/metricsql/utils.go b/vendor/github.com/VictoriaMetrics/metricsql/utils.go index 11828f39f..2405b1fef 100644 --- a/vendor/github.com/VictoriaMetrics/metricsql/utils.go +++ b/vendor/github.com/VictoriaMetrics/metricsql/utils.go @@ -1,5 +1,10 @@ package metricsql +import ( + "fmt" + "strings" +) + // ExpandWithExprs expands WITH expressions inside q and returns the resulting // PromQL without WITH expressions. func ExpandWithExprs(q string) (string, error) { @@ -14,7 +19,7 @@ func ExpandWithExprs(q string) (string, error) { // VisitAll recursively calls f for all the Expr children in e. // // It visits leaf children at first and then visits parent nodes. -// It is safe modifying expr in f. +// It is safe modifying e in f. func VisitAll(e Expr, f func(expr Expr)) { switch expr := e.(type) { case *BinaryOpExpr: @@ -36,3 +41,38 @@ func VisitAll(e Expr, f func(expr Expr)) { } f(e) } + +// IsSupportedFunction returns true if funcName contains supported MetricsQL function +func IsSupportedFunction(funcName string) bool { + funcName = strings.ToLower(funcName) + if IsRollupFunc(funcName) { + return true + } + if IsTransformFunc(funcName) { + return true + } + if IsAggrFunc(funcName) { + return true + } + return false +} + +func checkSupportedFunctions(e Expr) error { + var err error + VisitAll(e, func(expr Expr) { + if err != nil { + return + } + switch t := expr.(type) { + case *FuncExpr: + if !IsRollupFunc(t.Name) && !IsTransformFunc(t.Name) { + err = fmt.Errorf("unsupported function %q", t.Name) + } + case *AggrFuncExpr: + if !IsAggrFunc(t.Name) { + err = fmt.Errorf("unsupported aggregate function %q", t.Name) + } + } + }) + return err +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 1dbceba9a..7c406cb47 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -99,7 +99,7 @@ github.com/VictoriaMetrics/fasthttp/stackless # github.com/VictoriaMetrics/metrics v1.24.0 ## explicit; go 1.20 github.com/VictoriaMetrics/metrics -# github.com/VictoriaMetrics/metricsql v0.64.0 +# github.com/VictoriaMetrics/metricsql v0.65.0 ## explicit; go 1.13 github.com/VictoriaMetrics/metricsql github.com/VictoriaMetrics/metricsql/binaryop