Skip to content

Commit c9cbfa6

Browse files
mchromemchrome
authored
Add validation for brackets in plain metrics and regex error parsing for tagged metrics (#284)
1 parent 001be76 commit c9cbfa6

File tree

10 files changed

+303
-17
lines changed

10 files changed

+303
-17
lines changed

‎finder/index.go‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,13 @@ import (
44
"bytes"
55
"context"
66
"fmt"
7+
"net/http"
78
"strings"
89

910
"github.com/lomik/graphite-clickhouse/config"
1011
"github.com/lomik/graphite-clickhouse/helper/clickhouse"
1112
"github.com/lomik/graphite-clickhouse/helper/date"
13+
"github.com/lomik/graphite-clickhouse/helper/errs"
1214
"github.com/lomik/graphite-clickhouse/pkg/scope"
1315
"github.com/lomik/graphite-clickhouse/pkg/where"
1416
)
@@ -153,7 +155,18 @@ func (idx *IndexFinder) whereFilter(query string, from int64, until int64) *wher
153155
return w
154156
}
155157

158+
func (idx *IndexFinder) validatePlainQuery(query string) error {
159+
if where.HasUnmatchedBrackets(query) {
160+
return errs.NewErrorWithCode("query has unmatched brackets", http.StatusBadRequest)
161+
}
162+
return nil
163+
}
164+
156165
func (idx *IndexFinder) Execute(ctx context.Context, config *config.Config, query string, from int64, until int64, stat *FinderStat) (err error) {
166+
err = idx.validatePlainQuery(query)
167+
if err != nil {
168+
return err
169+
}
157170
w := idx.whereFilter(query, from, until)
158171

159172
idx.body, stat.ChReadRows, stat.ChReadBytes, err = clickhouse.Query(

‎finder/tagged.go‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
)
2020

2121
var (
22-
// ErrEmptyArgs = errors.New("empty arguments")
2322
ErrCostlySeriesByTag = errs.NewErrorWithCode("seriesByTag argument has too much wildcard and regex terms", http.StatusForbidden)
2423
)
2524

‎helper/clickhouse/clickhouse.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,9 @@ func extractClickhouseError(e string) (int, string) {
7575
if strings.HasPrefix(e, "clickhouse response status 404: Code: 60. DB::Exception: Table default.") {
7676
return http.StatusServiceUnavailable, "Storage default tables damaged"
7777
}
78+
if strings.HasPrefix(e, "clickhouse response status 500: Code: 427") || strings.HasPrefix(e, "clickhouse response status 400: Code: 427.") {
79+
return http.StatusBadRequest, "Incorrect regex syntax"
80+
}
7881
return http.StatusServiceUnavailable, "Storage unavailable"
7982
}
8083

‎helper/clickhouse/clickhouse_test.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,16 @@ func Test_extractClickhouseError(t *testing.T) {
3939
wantStatus: http.StatusServiceUnavailable,
4040
wantMessage: "Storage default tables damaged",
4141
},
42+
{
43+
errStr: `clickhouse response status 500: Code: 427, e.displayText() = DB::Exception: OptimizedRegularExpression: cannot compile re2: ^t=.**a*, error: bad repetition operator: **. Look at https://github.com/google/re2/wiki/Syntax for reference. Please note that if you specify regex as an SQL string literal, the slashes have to be additionally escaped. For example, to match an opening brace, write '\(' -- the first slash is for SQL and the second one is for regex: while executing 'FUNCTION match(x :: 0, '^t=.**a*' :: 2) -> match(x, '^t=.**a*') UInt8 : 1': while executing 'FUNCTION arrayExists(__lambda_11 :: 7, Tags :: 3) -> arrayExists(lambda(tuple(x), and(like(x, 't=%'), match(x, '^t=.**a*'))), Tags) UInt8 : 6' (version 21.3.20.1 (official build))`,
44+
wantStatus: http.StatusBadRequest,
45+
wantMessage: "Incorrect regex syntax",
46+
},
47+
{
48+
errStr: `clickhouse response status 500: Code: 427. DB::Exception: OptimizedRegularExpression: cannot compile re2: ^t=.**a*, error: bad repetition operator: **. Look at https://github.com/google/re2/wiki/Syntax for reference. Please note that if you specify regex as an SQL string literal, the slashes have to be additionally escaped. For example, to match an opening brace, write '\(' -- the first slash is for SQL and the second one is for regex: while executing 'FUNCTION and(like(x, 't=%') :: 3, match(x, '^t=.**a*') :: 1) -> and(like(x, 't=%'), match(x, '^t=.**a*')) UInt8 : 2': while executing 'FUNCTION and(and(greaterOrEquals(Date, '2024-10-28'), lessOrEquals(Date, '2024-10-28')) :: 0, and(equals(Tag1, '__name__=request_success_total.counter'), arrayExists(lambda(tuple(x), equals(x, 'app=test')), Tags), arrayExists(lambda(tuple(x), equals(x, 'project=Test')), Tags), arrayExists(lambda(tuple(x), equals(x, 'environment=TEST')), Tags), arrayExists(lambda(tuple(x), and(like(x, 't=%'), match(x, '^t=.**a*'))), Tags)) :: 3) -> and(and(greaterOrEquals(Date, '2024-10-28'), lessOrEquals(Date, '2024-10-28')), and(equals(Tag1, '__name__=request_success_total.counter'), arrayExists(lambda(tuple(x), equals(x, 'app=test')), Tags), arrayExists(lambda(tuple(x), equals(x, 'project=Test')), Tags), arrayExists(lambda(tuple(x), equals(x, 'environment=TEST')), Tags), arrayExists(lambda(tuple(x), and(like(x, 't=%'), match(x, '^t=.**a*'))), Tags))) UInt8 : 6'. (CANNOT_COMPILE_REGEXP) (version 22.8.21.38 (official build))`,
49+
wantStatus: http.StatusBadRequest,
50+
wantMessage: "Incorrect regex syntax",
51+
},
4252
{
4353
errStr: "Other error",
4454
wantStatus: http.StatusServiceUnavailable,

‎pkg/where/match.go‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,37 @@ func clearGlob(query string) string {
8585
return query
8686
}
8787

88+
func HasUnmatchedBrackets(query string) bool {
89+
matchingBracket := map[rune]rune{
90+
'}': '{',
91+
']': '[',
92+
}
93+
stack := make([]rune, 0)
94+
95+
nodeHasUnmatched := func(query string) bool {
96+
for _, c := range query {
97+
if c == '{' || c == '[' {
98+
stack = append(stack, c)
99+
}
100+
if c == '}' || c == ']' {
101+
if len(stack) == 0 || stack[len(stack)-1] != matchingBracket[c] {
102+
return true
103+
}
104+
stack = stack[:len(stack)-1]
105+
}
106+
}
107+
return len(stack) != 0
108+
}
109+
110+
for _, node := range strings.Split(query, ".") {
111+
if nodeHasUnmatched(node) {
112+
return true
113+
}
114+
}
115+
116+
return false
117+
}
118+
88119
func glob(field string, query string, optionalDotAtEnd bool) string {
89120
if query == "*" {
90121
return ""

‎pkg/where/match_test.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,36 @@ func Test_clearGlob(t *testing.T) {
2828
}
2929
}
3030

31+
func Test_HasUnmatchedBrackets(t *testing.T) {
32+
tests := []struct {
33+
query string
34+
want bool
35+
}{
36+
{"a.{a,b.te{s}t.b", true},
37+
{"a.{a,b}.te{s}t.b", false},
38+
{"a.{a,b}.te{s,t}}*.b", true},
39+
{"a.{a,b}.test*.b", false},
40+
{"a.a,b}.test*.b", true},
41+
{"a.{a,b.test*.b}", true},
42+
{"a.[a,b.test*.b]", true},
43+
{"a.[a,b].test*.b", false},
44+
{"a.[b].te{s}t.b", false},
45+
{"a.{[cd],[ef]}.b", false},
46+
{"a.[ab].te{s,t}*.b", false},
47+
{"a.{a,b.}.te{s,t}*.b", true}, // dots are not escaped inside curly brackets
48+
{"О.[б].те{s}t.b", false}, // utf-8 string
49+
{"О.[б.теs}t.b", true},
50+
{"О.[].те{}t.b", false}, // utf-8 string with empthy blocks
51+
}
52+
for _, tt := range tests {
53+
t.Run(tt.query, func(t *testing.T) {
54+
if got := HasUnmatchedBrackets(tt.query); got != tt.want {
55+
t.Errorf("HasUnmatchedBrackets() = %v, want %v", got, tt.want)
56+
}
57+
})
58+
}
59+
}
60+
3161
func TestGlob(t *testing.T) {
3262
field := "test"
3363
tests := []struct {

‎tests/feature_flags_both_true/test.toml‎

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,6 @@ points = [{value = 1.0, time = "rnow-10"}]
3737
name = "request_success_total.counter;app=test;project=Test;environment=TEST;t=cqa"
3838
points = [{value = 1.0, time = "rnow-10"}]
3939

40-
[[test.input]]
41-
name = "request_success_total.counter;app=test;project=Test;environment=TEST;t=cqa"
42-
points = [{value = 1.0, time = "rnow-10"}]
43-
4440
[[test.input]]
4541
name = "test;env=prod"
4642
points = [{value = 1.0, time = "rnow-10"}]
@@ -974,6 +970,60 @@ req_start = "rnow-10"
974970
req_stop = "rnow+10"
975971
values = [1.0, nan]
976972

973+
# ### seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*a*') ###
974+
975+
[[test.render_checks]]
976+
from = "rnow-10"
977+
until = "rnow+1"
978+
timeout = "1h"
979+
targets = [
980+
"seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*a*')",
981+
]
982+
error_regexp = "^400: Incorrect regex syntax"
983+
984+
# ### seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*') ###
985+
986+
[[test.render_checks]]
987+
from = "rnow-10"
988+
until = "rnow+1"
989+
timeout = "1h"
990+
targets = [
991+
"seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')",
992+
]
993+
994+
[[test.render_checks.result]]
995+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=cqa"
996+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
997+
consolidation = "avg"
998+
start = "rnow-10"
999+
stop = "rnow+10"
1000+
step = 10
1001+
req_start = "rnow-10"
1002+
req_stop = "rnow+10"
1003+
values = [1.0, nan]
1004+
1005+
[[test.render_checks.result]]
1006+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=q"
1007+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
1008+
consolidation = "avg"
1009+
start = "rnow-10"
1010+
stop = "rnow+10"
1011+
step = 10
1012+
req_start = "rnow-10"
1013+
req_stop = "rnow+10"
1014+
values = [1.0, nan]
1015+
1016+
[[test.render_checks.result]]
1017+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=qac"
1018+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
1019+
consolidation = "avg"
1020+
start = "rnow-10"
1021+
stop = "rnow+10"
1022+
step = 10
1023+
req_start = "rnow-10"
1024+
req_stop = "rnow+10"
1025+
values = [1.0, nan]
1026+
9771027
# # End - Test no flags
9781028
# #########################################################################
9791029

‎tests/feature_flags_dont_match_missing_tags/test.toml‎

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,6 @@ points = [{value = 1.0, time = "rnow-10"}]
3737
name = "request_success_total.counter;app=test;project=Test;environment=TEST;t=cqa"
3838
points = [{value = 1.0, time = "rnow-10"}]
3939

40-
[[test.input]]
41-
name = "request_success_total.counter;app=test;project=Test;environment=TEST;t=cqa"
42-
points = [{value = 1.0, time = "rnow-10"}]
43-
4440
[[test.input]]
4541
name = "test;env=prod"
4642
points = [{value = 1.0, time = "rnow-10"}]
@@ -930,6 +926,60 @@ req_start = "rnow-10"
930926
req_stop = "rnow+10"
931927
values = [1.0, nan]
932928

929+
# ### seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*a*') ###
930+
931+
[[test.render_checks]]
932+
from = "rnow-10"
933+
until = "rnow+1"
934+
timeout = "1h"
935+
targets = [
936+
"seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*a*')",
937+
]
938+
error_regexp = "^400: Incorrect regex syntax"
939+
940+
# ### seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*') ###
941+
942+
[[test.render_checks]]
943+
from = "rnow-10"
944+
until = "rnow+1"
945+
timeout = "1h"
946+
targets = [
947+
"seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')",
948+
]
949+
950+
[[test.render_checks.result]]
951+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=cqa"
952+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
953+
consolidation = "avg"
954+
start = "rnow-10"
955+
stop = "rnow+10"
956+
step = 10
957+
req_start = "rnow-10"
958+
req_stop = "rnow+10"
959+
values = [1.0, nan]
960+
961+
[[test.render_checks.result]]
962+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=q"
963+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
964+
consolidation = "avg"
965+
start = "rnow-10"
966+
stop = "rnow+10"
967+
step = 10
968+
req_start = "rnow-10"
969+
req_stop = "rnow+10"
970+
values = [1.0, nan]
971+
972+
[[test.render_checks.result]]
973+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=qac"
974+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
975+
consolidation = "avg"
976+
start = "rnow-10"
977+
stop = "rnow+10"
978+
step = 10
979+
req_start = "rnow-10"
980+
req_stop = "rnow+10"
981+
values = [1.0, nan]
982+
933983
# # End - Test no flags
934984
# #########################################################################
935985

‎tests/feature_flags_false/test.toml‎

Lines changed: 54 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -37,10 +37,6 @@ points = [{value = 1.0, time = "rnow-10"}]
3737
name = "request_success_total.counter;app=test;project=Test;environment=TEST;t=cqa"
3838
points = [{value = 1.0, time = "rnow-10"}]
3939

40-
[[test.input]]
41-
name = "request_success_total.counter;app=test;project=Test;environment=TEST;t=cqa"
42-
points = [{value = 1.0, time = "rnow-10"}]
43-
4440
[[test.input]]
4541
name = "test;env=prod"
4642
points = [{value = 1.0, time = "rnow-10"}]
@@ -1292,6 +1288,60 @@ req_start = "rnow-10"
12921288
req_stop = "rnow+10"
12931289
values = [1.0, nan]
12941290

1291+
# ### seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*a*') ###
1292+
1293+
[[test.render_checks]]
1294+
from = "rnow-10"
1295+
until = "rnow+1"
1296+
timeout = "1h"
1297+
targets = [
1298+
"seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*a*')",
1299+
]
1300+
error_regexp = "^400: Incorrect regex syntax"
1301+
1302+
# ### seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*') ###
1303+
1304+
[[test.render_checks]]
1305+
from = "rnow-10"
1306+
until = "rnow+1"
1307+
timeout = "1h"
1308+
targets = [
1309+
"seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')",
1310+
]
1311+
1312+
[[test.render_checks.result]]
1313+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=cqa"
1314+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
1315+
consolidation = "avg"
1316+
start = "rnow-10"
1317+
stop = "rnow+10"
1318+
step = 10
1319+
req_start = "rnow-10"
1320+
req_stop = "rnow+10"
1321+
values = [1.0, nan]
1322+
1323+
[[test.render_checks.result]]
1324+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=q"
1325+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
1326+
consolidation = "avg"
1327+
start = "rnow-10"
1328+
stop = "rnow+10"
1329+
step = 10
1330+
req_start = "rnow-10"
1331+
req_stop = "rnow+10"
1332+
values = [1.0, nan]
1333+
1334+
[[test.render_checks.result]]
1335+
name = "request_success_total.counter;app=test;environment=TEST;project=Test;t=qac"
1336+
path = "seriesByTag('name=request_success_total.counter', 'app=test', 'project=Test', 'environment=TEST', 't=~*')"
1337+
consolidation = "avg"
1338+
start = "rnow-10"
1339+
stop = "rnow+10"
1340+
step = 10
1341+
req_start = "rnow-10"
1342+
req_stop = "rnow+10"
1343+
values = [1.0, nan]
1344+
12951345
# # End - Test no flags
12961346
# #########################################################################
12971347

0 commit comments

Comments
 (0)