Create a configurable list of TraceQL queries that are immediately 400'ed #3780
Conversation
maliciousbucket
requested review from
joe-elliott,
annanay25,
mdisibio,
mapno,
kvrhdn,
zalegrala,
electron0zero,
ie-pham and
stoewer
as code owners
|
|
modules/frontend/config.go
Outdated
| Timeout time.Duration `yaml:"timeout,omitempty"` | ||
| Sharder SearchSharderConfig `yaml:",inline"` | ||
| SLO SLOConfig `yaml:",inline"` | ||
| FilterPatterns []string `yaml:"filter_patterns,omitempty"` |
There was a problem hiding this comment.
nit: for clarity rename to BlockedQueries
let's add the same field and pipeline item to MetricsConfig.
these two http requests are parsed differently so perhaps NewTraceQueryFilterWareWithDenyList takes a func like: func (*http.Request) string ?
| } | ||
|
|
||
| func (c traceQueryFilterWare) RoundTrip(req *http.Request) (*http.Response, error) { | ||
| resp, err := c.next.RoundTrip(req) |
There was a problem hiding this comment.
let's work on the order we are doing things in this function. it should be something like:
// if no filters just return c.next.RoundTrip()
// parse http request to get current query
// if current query has match in blocked queries then return 400 with a relevant body
// return c.next.RoundTrip
refactor NewTraceQueryFilterWare to take a func for parsing the query. Cleanup and refactor the Next function to follow correct order of operations
|
Thanks for the feedback! I've done some refactoring and added some basic unit tests. Fairly sure I'm building the URL's wrong for the tests ( hence the use of the raw url in the ParseSearchRequetQuery function). What's the best way to make the traceql queries? |
| @@ -35,7 +35,7 @@ type SearchConfig struct { | |||
| Timeout time.Duration `yaml:"timeout,omitempty"` | |||
| Sharder SearchSharderConfig `yaml:",inline"` | |||
| SLO SLOConfig `yaml:",inline"` | |||
| FilterPatterns []string `yaml:"filter_patterns,omitempty"` | |||
| BlockedQueries []string `yaml:"blocked-queries_patterns,omitempty"` | |||
There was a problem hiding this comment.
| BlockedQueries []string `yaml:"blocked-queries_patterns,omitempty"` | |
| BlockedQueries []string `yaml:"blocked_queries,omitempty"` |
| u, err := api.ParseSearchRequest(req) | ||
| if err != nil { | ||
| return resp, err | ||
| if c.filters == nil || len(c.filters) == 0 { |
There was a problem hiding this comment.
in go len(<nil slice>) == 0
| if c.filters == nil || len(c.filters) == 0 { | |
| if len(c.filters) == 0 { |
|
|
||
| //query, _ := api.ParseSearchTagValuesRequestV2(req) | ||
|
|
||
| //query, _ := api.ParseSearchRequest(req) |
|
|
||
| match := make(chan bool, len(c.filters)) | ||
| wg := sync.WaitGroup{} | ||
| for range c.filters { |
There was a problem hiding this comment.
no need for concurrency. let's keep this simple and just check them one at a time inline
| @@ -97,7 +99,7 @@ func New(cfg Config, next http.RoundTripper, o overrides.Interface, reader tempo | |||
| multiTenantMiddleware(cfg, logger), | |||
| newAsyncSearchSharder(reader, o, cfg.Search.Sharder, logger), | |||
| }, | |||
| []pipeline.Middleware{cacheWare, statusCodeWare, retryWare}, | |||
| []pipeline.Middleware{cacheWare, statusCodeWare, retryWare, searchQueryFilterWare}, | |||
There was a problem hiding this comment.
searchQueryFilterWare should be the first in the list of the AsyncMiddleware. this will allow it to refuse a request before we do any work. it does mean it needs to be shaped like:
https://github.com/grafana/tempo/blob/main/modules/frontend/search_sharder.go#L51
|
We really appreciate your efforts here, but we have a sudden need for this internally and need to get this feature in. Thank you so much for giving this a shot! Closing in favor of #3963 |
What this PR does:
Adds a new filter middleware to the search and metrics endpoints that matches the search query against a set of regex expressions. If it matches, a 400 result and generic message are returned.
Regex patterns can be added tot he config under FilterRegexes.
I would appreciate some guidance with regards to refactoring it and writign proper unit tests.
Which issue(s) this PR fixes:
Fixes #3769
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]