Enforce query response size limit after decompression in query-frontend #6607

afhassan · 2025-02-25T15:25:06Z

What this PR does:
Add new config max_query_response_size that limits the total response size of instant and range queries after decompression. The limit is applied to the total response size of all query shards.

Which issue(s) this PR fixes:
Fixes #

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

yeya24

If we apply the limit only on instant query and range query tripperware, then we don't have a way to protect us for large response from GetSeries and GetLabels API.

Do we plan to do something there? Or rely on the existing gRPC message size limit.

pkg/querier/tripperware/instantquery/instant_query.go

CHANGELOG.md

yeya24

Can we add an integration test?

harry671003

Thanks for the PR.
I believe the intend is to protect QFEs from OOMs. Could you test this and validate if it works as intended? Maybe reproduce a case where QFEs are getting OOMed and check whether the limit prevents this from happening.

pkg/querier/tripperware/instantquery/instant_query.go

afhassan · 2025-03-13T20:52:08Z

Added a new config for max_query_response_size that tracks the total response size of all query shards.

I will reproduce QFE OOM and test the new limit + add integration test

Thanks for the feedback!

afhassan · 2025-03-21T13:12:12Z

Addressed the previous feedback.

I think the PR is in a good place for a second review now

harry671003 · 2025-03-21T16:21:00Z

pkg/querier/tripperware/instantquery/instant_query.go

+
+	responseSizeLimiter := limiter.ResponseSizeLimiterFromContextWithFallback(ctx)
+
+	if strings.EqualFold(r.Header.Get("Content-Encoding"), "gzip") && len(buf.Bytes()) >= 4 {


nit: Extract the logic to estimate the uncompressed size into its own function.
What happens when it overflows? For example if the uncompressed size is 5GB, what will be these last 4 bytes be?

It is a 32-bit unsigned integer. When it overflows it should loop around so it will show 1 GB instead of 5 GB. It will not be a negative value.

The only risk in that case is not enforcing the limit when we should have.

pkg/querier/tripperware/instantquery/instant_query.go

harry671003

Overall great work! Left some optional nit comments.

harry671003 · 2025-03-25T14:18:18Z

pkg/querier/tripperware/query.go

 }

-func BodyBuffer(res *http.Response, logger log.Logger) ([]byte, error) {
+func BodyBytes(res *http.Response, responseSizeLimiter *limiter.ResponseSizeLimiter, logger log.Logger) ([]byte, error) {


nit: Does this method need to be renamed?

The function does not return body buffer. It returns body bytes array. I made that distinction when I split it into two functions, but even as one function it didn't make sense to me that it is called BodyBuffer() when it does not return a body buffer.

I am okay with keeping the name or changing it to something else, but what is the reason for using BodyBuffer()

pkg/querier/tripperware/query.go

justinjung04

Thanks for a great feature! I just have few minor comments

CHANGELOG.md

justinjung04 · 2025-03-25T15:39:42Z

pkg/querier/tripperware/query.go

-	} else if strings.EqualFold(res.Header.Get("Content-Encoding"), "snappy") {
-		sReader := snappy.NewReader(buf)
-		return io.ReadAll(sReader)


How do we double check if this is safe to be removed?

We set the compression headers for instant and range query requests here https://github.com/afhassan/cortex/blob/be0fc7f216589a3e9cb66af85ee25cf192fd533b/pkg/querier/tripperware/query.go#L762

We currently only support gzip compression (default) or no compression. We never set any other Accept-Encoding header.

Use compression for metrics query API or instant and range query APIs.
Supports 'gzip' and '' (disable compression)
CLI flag: -querier.response-compression
[response_compression: | default = "gzip"]

Snappy was added because we planned to potentially support it as well, but I think that can be left for a future PR to do.

pkg/querier/tripperware/query.go

Signed-off-by: Ahmed Hassan <[email protected]>

afhassan · 2025-03-25T19:39:43Z

Rebased the changes and force pushed

Signed-off-by: Ahmed Hassan <[email protected]>

justinjung04

Nice! Thanks a lot!

yeya24

Thanks and great work. Most of the comments are nits I think

CHANGELOG.md

yeya24 · 2025-03-26T16:10:31Z

pkg/util/limiter/response_size_limiter.go

+
+var (
+	responseLimiterCtxKey = &responseSizeLimiterCtxKey{}
+	ErrMaxResponseSizeHit = "the query response size exceeds limit (limit: %d bytes)"


Is the error message clear enough to tell it is the total response size for sharded queries?

Maybe the total received query response size exceeds limit (limit: %d bytes)? II don't know if we should explicitly mention shards since it is an internal detail.

I am a bit worried that the query response size exceeds limit might be a bit misleading to users as it reads as the query response size in their result. People will be confused that how their query exceeds several GBs even though the final size is only several MBs.

If we don't have a better error message we can go with what you have now

pkg/util/limiter/response_size_limiter.go

pkg/querier/tripperware/query.go

pkg/frontend/transport/handler.go

pkg/querier/tripperware/query.go

Signed-off-by: Ahmed Hassan <[email protected]>

…nd (cortexproject#6607)

pull-request-size bot added the size/M label Feb 25, 2025

dosubot bot added the component/query-frontend label Feb 25, 2025

pull-request-size bot added size/L and removed size/M labels Feb 26, 2025

afhassan force-pushed the qfe-byte-limit branch from 4b1de34 to 967ef4f Compare February 27, 2025 01:13

yeya24 reviewed Mar 3, 2025

View reviewed changes

pkg/querier/tripperware/instantquery/instant_query.go Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

yeya24 reviewed Mar 3, 2025

View reviewed changes

harry671003 reviewed Mar 13, 2025

View reviewed changes

pkg/querier/tripperware/instantquery/instant_query.go Outdated Show resolved Hide resolved

pkg/querier/tripperware/instantquery/instant_query.go Outdated Show resolved Hide resolved

harry671003 reviewed Mar 21, 2025

View reviewed changes

harry671003 approved these changes Mar 25, 2025

View reviewed changes

justinjung04 reviewed Mar 25, 2025

View reviewed changes

afhassan added 16 commits March 25, 2025 12:32

add query response size limit after decompression in query-frontend

de3764a

Signed-off-by: Ahmed Hassan <[email protected]>

update changelog

fe4cc56

Signed-off-by: Ahmed Hassan <[email protected]>

fix formatting

e6497e7

Signed-off-by: Ahmed Hassan <[email protected]>

inject orgId to ctx for unit tests

1fa4fd0

Signed-off-by: Ahmed Hassan <[email protected]>

fix formatting

8092511

Signed-off-by: Ahmed Hassan <[email protected]>

fix lint tenantID issue

9eb4884

Signed-off-by: Ahmed Hassan <[email protected]>

apply response size limit for the total size of all query shards

5456170

Signed-off-by: Ahmed Hassan <[email protected]>

formatting

2664621

Signed-off-by: Ahmed Hassan <[email protected]>

apply response size limit before decompressing gzip response in QFE

2a85bd7

Signed-off-by: Ahmed Hassan <[email protected]>

add integration test for QFE response size limit

8e6ddb9

Signed-off-by: Ahmed Hassan <[email protected]>

refactor response size limiter

7432331

Signed-off-by: Ahmed Hassan <[email protected]>

fix error formatting

fe3952c

Signed-off-by: Ahmed Hassan <[email protected]>

refactor updating response size limiter

50e9aaf

Signed-off-by: Ahmed Hassan <[email protected]>

rerun tests

b30a088

Signed-off-by: Ahmed Hassan <[email protected]>

update changelog

ca25461

Signed-off-by: Ahmed Hassan <[email protected]>

update comment

295d4b7

Signed-off-by: Ahmed Hassan <[email protected]>

afhassan force-pushed the qfe-byte-limit branch from 99f8630 to 295d4b7 Compare March 25, 2025 19:35

afhassan added 2 commits March 25, 2025 12:45

refactor getResponseSize

278ce54

Signed-off-by: Ahmed Hassan <[email protected]>

add reference for gzip footer specifications

386b727

Signed-off-by: Ahmed Hassan <[email protected]>

justinjung04 approved these changes Mar 25, 2025

View reviewed changes

yeya24 reviewed Mar 26, 2025

View reviewed changes

afhassan and others added 2 commits March 26, 2025 14:44

add unit test for getResponseSize

20461fb

Signed-off-by: Ahmed Hassan <[email protected]>

Merge branch 'master' into qfe-byte-limit

b83bc71

Signed-off-by: Ahmed Hassan <[email protected]>

yeya24 approved these changes Mar 27, 2025

View reviewed changes

yeya24 merged commit f97ec76 into cortexproject:master Mar 27, 2025
17 checks passed

justinjung04 pushed a commit to justinjung04/cortex that referenced this pull request Mar 27, 2025

Enforce query response size limit after decompression in query-fronte…

44dc6a6

…nd (cortexproject#6607)

CharlieTLe mentioned this pull request Jul 26, 2025

docs: fix typos in architecture.md CharlieTLe/cortex#92

Closed

CharlieTLe mentioned this pull request Aug 10, 2025

cortex config json schema CharlieTLe/cortex#99

Closed

3 tasks


		responseSizeLimiter := limiter.ResponseSizeLimiterFromContextWithFallback(ctx)

		if strings.EqualFold(r.Header.Get("Content-Encoding"), "gzip") && len(buf.Bytes()) >= 4 {

Enforce query response size limit after decompression in query-frontend #6607

Enforce query response size limit after decompression in query-frontend #6607

Uh oh!

Conversation

afhassan commented Feb 25, 2025 • edited by yeya24 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

harry671003 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

afhassan commented Mar 13, 2025

Uh oh!

afhassan commented Mar 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

harry671003 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

justinjung04 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

afhassan commented Mar 25, 2025

Uh oh!

justinjung04 left a comment

Choose a reason for hiding this comment

Uh oh!

yeya24 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

afhassan commented Feb 25, 2025 •

edited by yeya24

Loading

harry671003 left a comment •

edited

Loading

justinjung04 left a comment •

edited

Loading