Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a configurable list of TraceQL queries that are immediately 400'ed #3780

Closed
wants to merge 9 commits into from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* [ENHANCEMENT] Improve use of OTEL semantic conventions on the service graph [#3711](https://github.com/grafana/tempo/pull/3711) (@zalegrala)
* [ENHANCEMENT] Performance improvement for `rate() by ()` queries [#3719](https://github.com/grafana/tempo/pull/3719) (@mapno)
* [ENHANCEMENT] Use multiple goroutines to unmarshal responses in parallel in the query frontend. [#3713](https://github.com/grafana/tempo/pull/3713) (@joe-elliott)
* [ENHANCEMENT] Create a configurable list of TraceQL queries that are immediately 400'ed [#3769](https://github.com/grafana/tempo/issues/3769)(@maliciousbucket)
* [BUGFIX] Fix metrics queries when grouping by attributes that may not exist [#3734](https://github.com/grafana/tempo/pull/3734) (@mdisibio)
* [BUGFIX] Fix frontend parsing error on cached responses [#3759](https://github.com/grafana/tempo/pull/3759) (@mdisibio)
* [BUGFIX] max_global_traces_per_user: take into account ingestion.tenant_shard_size when converting to local limit [#3618](https://github.com/grafana/tempo/pull/3618) (@kvrhdn)
Expand Down
7 changes: 4 additions & 3 deletions modules/frontend/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ type Config struct {
}

type SearchConfig struct {
Timeout time.Duration `yaml:"timeout,omitempty"`
Sharder SearchSharderConfig `yaml:",inline"`
SLO SLOConfig `yaml:",inline"`
Timeout time.Duration `yaml:"timeout,omitempty"`
Sharder SearchSharderConfig `yaml:",inline"`
SLO SLOConfig `yaml:",inline"`
FilterPatterns []string `yaml:"filter_patterns,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

}

type TraceByIDConfig struct {
Expand Down
5 changes: 3 additions & 2 deletions modules/frontend/frontend.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func New(cfg Config, next http.RoundTripper, o overrides.Interface, reader tempo
cacheWare := pipeline.NewCachingWare(cacheProvider, cache.RoleFrontendSearch, logger)
statusCodeWare := pipeline.NewStatusCodeAdjustWare()
traceIDStatusCodeWare := pipeline.NewStatusCodeAdjustWareWithAllowedCode(http.StatusNotFound)
queryFilterWare := pipeline.NewTraceQueryFilterWareWithDenyList(cfg.Search.FilterPatterns)

tracePipeline := pipeline.Build(
[]pipeline.AsyncMiddleware[combiner.PipelineResponse]{
Expand All @@ -97,7 +98,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, queryFilterWare},
next)

searchTagsPipeline := pipeline.Build(
Expand Down Expand Up @@ -130,7 +131,7 @@ func New(cfg Config, next http.RoundTripper, o overrides.Interface, reader tempo
multiTenantMiddleware(cfg, logger),
newAsyncQueryRangeSharder(reader, o, cfg.Metrics.Sharder, logger),
},
[]pipeline.Middleware{cacheWare, statusCodeWare, retryWare},
[]pipeline.Middleware{cacheWare, statusCodeWare, retryWare, queryFilterWare},
next)

traces := newTraceIDHandler(cfg, o, tracePipeline, logger)
Expand Down
96 changes: 96 additions & 0 deletions modules/frontend/pipeline/trace_query_filter.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
package pipeline

import (
"github.com/grafana/tempo/pkg/api"
"io"
"net/http"
"regexp"
"strings"
"sync"
)

type traceQueryFilterWare struct {
next http.RoundTripper
filters []*regexp.Regexp
}

func NewTraceQueryFilterWare(next http.RoundTripper) http.RoundTripper {
return &traceQueryFilterWare{
next: next,
}
}

func NewTraceQueryFilterWareWithDenyList(denyList []string) Middleware {
filter := make([]*regexp.Regexp, len(denyList)+1)
for i := range denyList {
exp, err := regexp.Compile(denyList[i])
if err == nil {
filter[i] = exp
}
}

return MiddlewareFunc(func(next http.RoundTripper) http.RoundTripper {
return traceQueryFilterWare{
next: next,
filters: filter,
}
})
}

func (c traceQueryFilterWare) RoundTrip(req *http.Request) (*http.Response, error) {
resp, err := c.next.RoundTrip(req)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return resp, err
}
//Better way to do this?
if len(c.filters) == 0 {
return resp, nil
}
//need wait group
u, err := api.ParseSearchRequest(req)
if err != nil {
return resp, err
}

qry := u.Query

if len(qry) == 0 {
return resp, nil
}

//Not sure this is a good idea / the best way to do this
//Also probably not necessary

match := make(chan bool, len(c.filters))
wg := sync.WaitGroup{}
for range c.filters {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need for concurrency. let's keep this simple and just check them one at a time inline

wg.Add(1)
}

go func(qry string) {
defer wg.Done()
for _, re := range c.filters {
if re.MatchString(qry) {
match <- true
return
}
}
match <- false
}(qry)

go func() {
wg.Wait()
close(match)
}()

if <-match {

return &http.Response{
StatusCode: http.StatusBadRequest,
Status: http.StatusText(http.StatusBadRequest),
Body: io.NopCloser(strings.NewReader("Query is temporarily blocked by your administrator.")),
}, nil

}
return resp, nil
}
1 change: 1 addition & 0 deletions modules/frontend/pipeline/trace_query_filter_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
package pipeline
joe-elliott marked this conversation as resolved.
Show resolved Hide resolved
Loading