Skip to content

add response limits to default middleware for httpclient #1283

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

add response limits to default middleware for httpclient #1283

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
443a54f
add response limits to default middleware for httpclient
Multimo Mar 26, 2025
c2400ae
lint
Multimo Mar 26, 2025
300b02f
fix provisioning tests
Multimo Mar 26, 2025
f04a40b
fix unit test
Multimo Mar 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/httpclient/http_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,7 @@ func DefaultMiddlewares() []Middleware {
CustomHeadersMiddleware(),
ContextualMiddleware(),
ErrorSourceMiddleware(),
ResponseLimitMiddleware(0),
}
}

Expand Down
3 changes: 2 additions & 1 deletion backend/httpclient/http_client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,14 @@ func TestNewClient(t *testing.T) {
require.NoError(t, err)
require.NotNil(t, client)

require.Len(t, usedMiddlewares, 6)
require.Len(t, usedMiddlewares, 7)
require.Equal(t, TracingMiddlewareName, usedMiddlewares[0].(MiddlewareName).MiddlewareName())
require.Equal(t, DataSourceMetricsMiddlewareName, usedMiddlewares[1].(MiddlewareName).MiddlewareName())
require.Equal(t, BasicAuthenticationMiddlewareName, usedMiddlewares[2].(MiddlewareName).MiddlewareName())
require.Equal(t, CustomHeadersMiddlewareName, usedMiddlewares[3].(MiddlewareName).MiddlewareName())
require.Equal(t, ContextualMiddlewareName, usedMiddlewares[4].(MiddlewareName).MiddlewareName())
require.Equal(t, ErrorSourceMiddlewareName, usedMiddlewares[5].(MiddlewareName).MiddlewareName())
require.Equal(t, ResponseLimitMiddlewareName, usedMiddlewares[6].(MiddlewareName).MiddlewareName())
})

t.Run("New() with opts middleware should return expected http.Client", func(t *testing.T) {
Expand Down
9 changes: 6 additions & 3 deletions backend/httpclient/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,27 +25,29 @@ func TestProvider(t *testing.T) {
client, err := ctx.provider.New()
require.NoError(t, err)
require.NotNil(t, client)
require.Len(t, ctx.usedMiddlewares, 6)
require.Len(t, ctx.usedMiddlewares, 7)
require.Equal(t, TracingMiddlewareName, ctx.usedMiddlewares[0].(MiddlewareName).MiddlewareName())
require.Equal(t, DataSourceMetricsMiddlewareName, ctx.usedMiddlewares[1].(MiddlewareName).MiddlewareName())
require.Equal(t, BasicAuthenticationMiddlewareName, ctx.usedMiddlewares[2].(MiddlewareName).MiddlewareName())
require.Equal(t, CustomHeadersMiddlewareName, ctx.usedMiddlewares[3].(MiddlewareName).MiddlewareName())
require.Equal(t, ContextualMiddlewareName, ctx.usedMiddlewares[4].(MiddlewareName).MiddlewareName())
require.Equal(t, ErrorSourceMiddlewareName, ctx.usedMiddlewares[5].(MiddlewareName).MiddlewareName())
require.Equal(t, ResponseLimitMiddlewareName, ctx.usedMiddlewares[6].(MiddlewareName).MiddlewareName())
})

t.Run("Transport should use default middlewares", func(t *testing.T) {
ctx := newProviderTestContext(t)
transport, err := ctx.provider.GetTransport()
require.NoError(t, err)
require.NotNil(t, transport)
require.Len(t, ctx.usedMiddlewares, 6)
require.Len(t, ctx.usedMiddlewares, 7)
require.Equal(t, TracingMiddlewareName, ctx.usedMiddlewares[0].(MiddlewareName).MiddlewareName())
require.Equal(t, DataSourceMetricsMiddlewareName, ctx.usedMiddlewares[1].(MiddlewareName).MiddlewareName())
require.Equal(t, BasicAuthenticationMiddlewareName, ctx.usedMiddlewares[2].(MiddlewareName).MiddlewareName())
require.Equal(t, CustomHeadersMiddlewareName, ctx.usedMiddlewares[3].(MiddlewareName).MiddlewareName())
require.Equal(t, ContextualMiddlewareName, ctx.usedMiddlewares[4].(MiddlewareName).MiddlewareName())
require.Equal(t, ErrorSourceMiddlewareName, ctx.usedMiddlewares[5].(MiddlewareName).MiddlewareName())
require.Equal(t, ResponseLimitMiddlewareName, ctx.usedMiddlewares[6].(MiddlewareName).MiddlewareName())
})

t.Run("New() with options and no middleware should return expected http client and transport", func(t *testing.T) {
Expand Down Expand Up @@ -86,7 +88,7 @@ func TestProvider(t *testing.T) {
require.Equal(t, DefaultTimeoutOptions.Timeout, client.Timeout)

t.Run("Should use configured middlewares and implement MiddlewareName", func(t *testing.T) {
require.Len(t, pCtx.usedMiddlewares, 9)
require.Len(t, pCtx.usedMiddlewares, 10)
require.Equal(t, "mw1", pCtx.usedMiddlewares[0].(MiddlewareName).MiddlewareName())
require.Equal(t, "mw2", pCtx.usedMiddlewares[1].(MiddlewareName).MiddlewareName())
require.Equal(t, "mw3", pCtx.usedMiddlewares[2].(MiddlewareName).MiddlewareName())
Expand All @@ -96,6 +98,7 @@ func TestProvider(t *testing.T) {
require.Equal(t, CustomHeadersMiddlewareName, pCtx.usedMiddlewares[6].(MiddlewareName).MiddlewareName())
require.Equal(t, ContextualMiddlewareName, pCtx.usedMiddlewares[7].(MiddlewareName).MiddlewareName())
require.Equal(t, ErrorSourceMiddlewareName, pCtx.usedMiddlewares[8].(MiddlewareName).MiddlewareName())
require.Equal(t, ResponseLimitMiddlewareName, pCtx.usedMiddlewares[9].(MiddlewareName).MiddlewareName())
})

t.Run("When roundtrip should call expected middlewares", func(t *testing.T) {
Expand Down
20 changes: 20 additions & 0 deletions backend/httpclient/response_limit_middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,32 @@ package httpclient

import (
"net/http"
"os"
"strconv"

"github.com/grafana/grafana-plugin-sdk-go/backend/log"
)

const (
ResponseLimitEnvVar = "GF_DATAPROXY_RESPONSE_LIMIT"
)

// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally appreciate that. I can work on something to try achieve this easily, but in the meantime I recommend applying this change manually!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Circular dependencies 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

if ok && envLimit != "" {
limitInt, err := strconv.ParseInt(envLimit, 10, 64)
if err == nil && limitInt > 0 {
limit = limitInt
}

log.DefaultLogger.Error("failed to parse GF_DATAPROXY_RESPONSE_LIMIT", "error", err)
}
}

return NamedMiddlewareFunc(ResponseLimitMiddlewareName, func(_ Options, next http.RoundTripper) http.RoundTripper {
return RoundTripperFunc(func(req *http.Request) (*http.Response, error) {
res, err := next.RoundTrip(req)
Expand Down
19 changes: 15 additions & 4 deletions backend/httpclient/response_limit_middleware_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"fmt"
"io"
"net/http"
"os"
"strings"
"testing"

Expand All @@ -18,17 +19,27 @@ func TestResponseLimitMiddleware(t *testing.T) {
expectedBodyLength int
expectedBody string
err error
envLimit string
}{
{limit: 1, expectedBodyLength: 1, expectedBody: "d", err: errors.New("error: http: response body too large, response limit is set to: 1")},
{limit: 1000000, expectedBodyLength: 5, expectedBody: "dummy", err: nil},
{limit: 0, expectedBodyLength: 5, expectedBody: "dummy", err: nil},
// Test that the limit is set from arguments
{limit: 1, expectedBodyLength: 1, expectedBody: "d", err: errors.New("error: http: response body too large, response limit is set to: 1"), envLimit: ""},
{limit: 1000000, expectedBodyLength: 5, expectedBody: "dummy", err: nil, envLimit: ""},
{limit: 0, expectedBodyLength: 5, expectedBody: "dummy", err: nil, envLimit: ""},
// Test that the limit is set from the environment variable
{limit: 0, expectedBodyLength: 1, expectedBody: "d", err: errors.New("error: http: response body too large, response limit is set to: 1"), envLimit: "1"},
{limit: 0, expectedBodyLength: 5, expectedBody: "dummy", err: nil, envLimit: "1000000"},
{limit: 0, expectedBodyLength: 5, expectedBody: "dummy", err: nil, envLimit: "-1"},
{limit: 0, expectedBodyLength: 5, expectedBody: "dummy", err: nil, envLimit: "0"},
}
for _, tc := range tcs {
t.Run(fmt.Sprintf("Test ResponseLimitMiddleware with limit: %d", tc.limit), func(t *testing.T) {
t.Run(fmt.Sprintf("Test ResponseLimitMiddleware with limit: %d and envLimit: %s", tc.limit, tc.envLimit), func(t *testing.T) {
finalRoundTripper := RoundTripperFunc(func(req *http.Request) (*http.Response, error) {
return &http.Response{StatusCode: http.StatusOK, Request: req, Body: io.NopCloser(strings.NewReader("dummy"))}, nil
})

os.Setenv(ResponseLimitEnvVar, tc.envLimit)
defer os.Unsetenv(ResponseLimitEnvVar)

mw := ResponseLimitMiddleware(tc.limit)
rt := mw.CreateMiddleware(Options{}, finalRoundTripper)
require.NotNil(t, rt)
Expand Down