Improve decompression within ParseProtoReader. #3682

cyriltovena · 2021-01-13T10:10:45Z

This uses the last weaveworks/common, which include a fix for the content-type size to be set correctly allowing to allocate buffers correctly instead of let them grow naturally.

I've also implemented an alternative decompression from a bytes.Buffer which avoids allocating and reading a new buffer when using httpgrpc since the underlying reader is already a bytes.Buffer.

I took the liberty to also improve the validation when using RawSnappy, I'm checking the length using the snappy header. This will avoid wasting CPU when the decompressed size is too big since we will know in advance.

I've added also missing tests that covers the old and new code.

I'm planning to use this in Loki, but it should also benefits Cortex, this method is used within the push and remote_read code.

Signed-off-by: Cyril Tovena [email protected]

This uses the last weaveworks, which include a fix for the content-type size to be set correctly which allows to allocate buffers correctly instead of let it grow naturally. I've also implemented an alternative decompression from a `bytes.Buffer` which avoids allocating and reading a new buffer when using httpgrpc since the underlaying reader is already a `bytes.Buffer`. I took the liberty to also improve the validation when using RawSnappy, I'm checking the length using the snappy header. This will avoid wasting CPU when the decompressed size is too big since we will know in advance. I've added also missing tests that covers the old and new code. Signed-off-by: Cyril Tovena <[email protected]>

Signed-off-by: Cyril Tovena <[email protected]>

bboreham · 2021-01-13T13:56:36Z

Should note weaveworks/common#204, Allow specifying JAEGER_ENDPOINT, in the changelog.

Do you think we could drop framed Snappy? It was only in a small range of Prometheus clients, a long time ago.

cyriltovena · 2021-01-13T14:19:16Z

Should note weaveworks/common#204, Allow specifying JAEGER_ENDPOINT, in the changelog.

Do you think we could drop framed Snappy? It was only in a small range of Prometheus clients, a long time ago.

We don't use it in Loki and it does make the code more complex, so I'm in for this, if you want.

Signed-off-by: Cyril Tovena <[email protected]>

cyriltovena · 2021-01-13T14:24:23Z

I also realized during writing tests, that FrameSnappy weight more.

bboreham · 2021-01-13T14:58:10Z

This was where framed Snappy was removed, before Prometheus 1.7: prometheus/prometheus#2696

cyriltovena · 2021-01-14T08:32:27Z

This was where framed Snappy was removed, before Prometheus 1.7: prometheus/prometheus#2696

I wonder if this is fine to remove ? I guess we won't support 1.7 then ? Is this fine ? I'm not totally sure.

@gouthamve @pracucci any chance you might have a reason to keep this ? Do we expect all users above 1.7 Prometheus ?

pracucci · 2021-01-14T08:41:46Z

@gouthamve @pracucci any chance you might have a reason to keep this ? Do we expect all users above 1.7 Prometheus ?

Yesterday we had a brief discussion in this Slack thread and LGTM from me and Goutham. As Bryan said:

I think it's exactly 1.6 that we would remove support for, since Cortex did not support <1.6 already
1.6 was April 2017 and 1.7 June 2017

As far as the CHANGELOG mention we're dropping support for Prometheus 1.7, LGTM.

Signed-off-by: Cyril Tovena <[email protected]>

cyriltovena · 2021-01-14T16:45:15Z

I would have prefer another PR. but ho well too late.

bboreham · 2021-01-14T16:48:40Z

Not clear why it's too late. I don't mind it being another PR.

Signed-off-by: Cyril Tovena <[email protected]>

pracucci

Good job! Changes make a lot of sense to me. I left few comments, but overall LGTM 👏

CHANGELOG.md

pkg/querier/remote_read.go

pracucci · 2021-01-15T14:01:50Z

pkg/util/http.go

 	sp := opentracing.SpanFromContext(ctx)
 	if sp != nil {
 		sp.LogFields(otlog.String("event", "util.ParseProtoRequest[start reading]"))
+		defer func() {
+			sp.LogFields(otlog.String("event", "util.ParseProtoRequest[unmarshal]"),


I would move this to ParseProtoReader() for clarity.

I actually moved it here for clarity but still applied your feedback, it's more or less the same as you need to push down the span into all code branches.

My first implementation only start the usage of the span in the decompressRequest.

pkg/util/http.go

@pracucci

From Review feedback from @pracucci Signed-off-by: Cyril Tovena <[email protected]>

pracucci

Good job, LGTM!

bboreham · 2021-01-19T12:00:02Z

pkg/util/http.go

+		_, err = buf.ReadFrom(io.LimitReader(reader, int64(maxSize)+1))
+		body = buf.Bytes()
+	case RawSnappy:
+		_, err = buf.ReadFrom(reader)


Is this a possible DoS attack? (I see it is what the old code did)
Seems we could use the same ReadFrom in either case, then check compression after.

Could fix it up later; I don't mind merging this as-is.

Yeah I see the potential issue, not sure if there's a better option then using a limitReader in both case ?

Decoding the length still leave us open for hijacked/fake requests.

I made the change let me know what you think ?

I think using LimitReader in all paths is correct, if the point is to stop someone blowing up the process.

Is 'result bigger than max' actually detected in the NoCompression case now?
Suggest just having one ReadFrom call then check the len, before we get into a routine named decompress

yes because we read a bit more the defer in decompressRequest will return the error.

bboreham

Thanks!

pstibrany

LGTM.

Nit: Both decompressFromReader and decompressFromBuffer could benefit from using early returns.

pstibrany · 2021-01-19T12:42:32Z

pkg/util/http.go

@@ -163,6 +115,88 @@ func ParseProtoReader(ctx context.Context, reader io.Reader, expectedSize, maxSi
 	return nil
 }

+func decompressRequest(ctx context.Context, reader io.Reader, expectedSize, maxSize int, compression CompressionType, sp opentracing.Span) (body []byte, err error) {


Nit: ctx is unused, let's remove it.

Yep this was leftover from previous span extraction code.

pstibrany · 2021-01-19T12:46:00Z

pkg/util/http.go

+			err = fmt.Errorf(messageSizeLargerErrFmt, len(body), maxSize)
+		}
+	}()
+	if expectedSize > maxSize {


Nit: There is no need to use defer in this function, it just makes code more tricky to follow.

pstibrany · 2021-01-19T12:48:21Z

pkg/util/http.go

+		return nil, fmt.Errorf(messageSizeLargerErrFmt, expectedSize, maxSize)
+	}
+	buffer, ok := tryBufferFromReader(reader)
+	if ok {


I think it would be better to check for non-nil buffer. It would be slightly more robust, as reader implementing interface { BytesBuffer() *bytes.Buffer } can still return nil buffer, which currently leads to panic.

pstibrany · 2021-01-19T12:57:25Z

pkg/util/http.go

@@ -163,6 +115,88 @@ func ParseProtoReader(ctx context.Context, reader io.Reader, expectedSize, maxSi
 	return nil
 }

+func decompressRequest(ctx context.Context, reader io.Reader, expectedSize, maxSize int, compression CompressionType, sp opentracing.Span) (body []byte, err error) {
+	defer func() {
+		if len(body) > maxSize {


Nit: This will overwrite existing err, if one was already set. I can see some code paths how it can happen, but it's probably not important enough to fix.

Signed-off-by: Cyril Tovena <[email protected]>

pracucci · 2021-01-21T11:03:29Z

pkg/util/http.go

+		// Read from LimitReader with limit max+1. So if the underlying
+		// reader is over limit, the result will be bigger than max.


Move this comment above, where io.LimitReader is used, please.

Signed-off-by: Cyril Tovena <[email protected]>

pull-request-size bot added the size/L label Jan 13, 2021

missing go.sum update.

25f3e3b

Signed-off-by: Cyril Tovena <[email protected]>

Update changelog.

22e7809

Signed-off-by: Cyril Tovena <[email protected]>

Removes FramedSnappy encoding support.

d8d7185

Signed-off-by: Cyril Tovena <[email protected]>

update go.mod

7f900c6

Signed-off-by: Cyril Tovena <[email protected]>

pracucci reviewed Jan 15, 2021

View reviewed changes

Simplify implementation.

54213e7

From Review feedback from @pracucci Signed-off-by: Cyril Tovena <[email protected]>

pracucci approved these changes Jan 19, 2021

View reviewed changes

Merge branch 'master' into httpgrpc-body

bdb8f12

bboreham reviewed Jan 19, 2021

View reviewed changes

bboreham approved these changes Jan 19, 2021

View reviewed changes

pstibrany approved these changes Jan 19, 2021

View reviewed changes

cyriltovena added 2 commits January 19, 2021 20:43

Nits & limitReader in both cases when reading from a buffer.

ebfdda8

Signed-off-by: Cyril Tovena <[email protected]>

Merge branch 'master' into httpgrpc-body

7b51ccb

pracucci reviewed Jan 21, 2021

View reviewed changes

Moves comment.

beadfad

Signed-off-by: Cyril Tovena <[email protected]>

pracucci merged commit b33ae45 into cortexproject:master Jan 21, 2021

ethervoid mentioned this pull request Jan 29, 2021

JAEGER_ENDPOINT not supported #1843

Closed

chencs mentioned this pull request Mar 7, 2024

Check max encoded length before attempting snappy.Encode grafana/mimir#7520

Merged

4 tasks

damnever mentioned this pull request Mar 12, 2024

The 'alertmanager_max_alerts_count' is not functioning properly #5720

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve decompression within ParseProtoReader. #3682

Improve decompression within ParseProtoReader. #3682

cyriltovena commented Jan 13, 2021

bboreham commented Jan 13, 2021

cyriltovena commented Jan 13, 2021

cyriltovena commented Jan 13, 2021

bboreham commented Jan 13, 2021

cyriltovena commented Jan 14, 2021

pracucci commented Jan 14, 2021

cyriltovena commented Jan 14, 2021

bboreham commented Jan 14, 2021

pracucci left a comment

pracucci Jan 15, 2021

cyriltovena Jan 18, 2021 •

edited

Loading

pracucci left a comment

bboreham Jan 19, 2021

bboreham Jan 19, 2021

cyriltovena Jan 19, 2021

bboreham Jan 20, 2021

cyriltovena Jan 21, 2021

bboreham left a comment

pstibrany left a comment

pstibrany Jan 19, 2021 •

edited

Loading

cyriltovena Jan 19, 2021

pstibrany Jan 19, 2021

pstibrany Jan 19, 2021

pstibrany Jan 19, 2021

pracucci Jan 21, 2021

		// Read from LimitReader with limit max+1. So if the underlying
		// reader is over limit, the result will be bigger than max.

Improve decompression within ParseProtoReader. #3682

Improve decompression within ParseProtoReader. #3682

Conversation

cyriltovena commented Jan 13, 2021

bboreham commented Jan 13, 2021

cyriltovena commented Jan 13, 2021

cyriltovena commented Jan 13, 2021

bboreham commented Jan 13, 2021

cyriltovena commented Jan 14, 2021

pracucci commented Jan 14, 2021

cyriltovena commented Jan 14, 2021

bboreham commented Jan 14, 2021

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyriltovena Jan 18, 2021 • edited Loading

Choose a reason for hiding this comment

pracucci left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bboreham left a comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

pstibrany Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyriltovena Jan 18, 2021 •

edited

Loading

pstibrany Jan 19, 2021 •

edited

Loading