add response limits to default middleware for httpclient #1283
Conversation
Multimo
requested review from
oshirohugo,
briangann and
xnyo
and removed request for
a team
|
|
| ) | ||
|
|
||
| // ResponseLimitMiddlewareName is the middleware name used by ResponseLimitMiddleware. | ||
| const ResponseLimitMiddlewareName = "response-limit" | ||
|
|
||
| func ResponseLimitMiddleware(limit int64) Middleware { | ||
| if limit <= 0 { | ||
| envLimit, ok := os.LookupEnv(ResponseLimitEnvVar) |
There was a problem hiding this comment.
The problem with this is that we don't propagate host env vars to plugins by default in Cloud so it would require extra plumbing. Is the list of problematic plugins so large that it's not easier to patch them individually?
It can be done like so in the prom datasource or else like the following:
// NewDatasource creates a new datasource instance.
func NewDatasource(ctx context.Context, settings backend.DataSourceInstanceSettings) (instancemgmt.Instance, error) {
opts, err := settings.HTTPClientOptions(ctx)
if err != nil {
return nil, fmt.Errorf("http client options: %w", err)
}
cfg := backend.GrafanaConfigFromContext(ctx)
opts.Middlewares = append(opts.Middlewares, httpclient.ResponseLimitMiddleware(cfg.ResponseLimit()))
cl, err := httpclient.New(opts)
if err != nil {
return nil, fmt.Errorf("httpclient new: %w", err)
}
return &Datasource{
settings: settings,
httpClient: cl,
}, nil
}If this is a big lift, then we can look at trying to apply this automatically!
There was a problem hiding this comment.
Ideally we should apply this to all data sources as it's unpredictable which data sources may end up returning very large responses and when.
There was a problem hiding this comment.
Totally appreciate that. I can work on something to try achieve this easily, but in the meantime I recommend applying this change manually!
There was a problem hiding this comment.
Can't you simply do that code below, in the RoundTripperFunc function? I don't know if reading the config for each request can be a performance problem though but we could cache the result I assume.
There was a problem hiding this comment.
It's the classic backend -> httpclient -> backend problem. I started a branch to address it and tried minimize the number of breaking changes (seems gorelease has trouble identify a few moved pieces but seems okay-ish for the most part)
There was a problem hiding this comment.
yeah, I don't think you can avoid the breaking change there but if you want to move forward with that, it sounds good to me
|
This pull request has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had activity in the last 2 weeks. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
What this PR does / why we need it:
This PR will add response limit middleware to the default Middleware but unless the
GF_DATASOURCE_RESPONSE_LIMITis set it there will be no functional difference if a datasource updates.The context around this change is we want to begin rolling out limits in our cloud environment for datasources which can occasionally return very very large responses.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
I would like to be able to configure this via settings but I am not sure if this is the best approach.