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.

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

Jump to bottom

grpc_json_transcoder: Adhere to decoder and encoder buffer limits #14906

Merged
merged 26 commits into from
Mar 18, 2021
Merged

grpc_json_transcoder: Adhere to decoder and encoder buffer limits #14906

merged 26 commits into from
Mar 18, 2021

Conversation

nareddyt
Copy link
Contributor

@nareddyt nareddyt commented Feb 2, 2021 •
edited
Loading

Commit Message:
grpc_json_transcoder: Adhere to decoder and encoder buffer limits

Additional Description:
Currently, the transcoder has unbounded internal buffering. This is not allowed according to the Extension stability and security posture.

This PR introduces limits to internal request/response buffers. This follows the documentation for Flow Control, but the filter does not participate in watermarking. Flow control via watermarking will not help the filter: Data stuck in a buffer will never be transcoded until more data comes in. If watermarking occurs, then the data will be stuck forever. So we fail the request with HTTP 413 or HTTP 500 instead.

Note: The current implementation does not handle some edge cases with streaming requests/responses. Those will require special handling. I will make a follow-up PR for that. See review comments for more details.

Risk Level:
Medium.

  • Technically a breaking change, but default envoy buffer limits are 1 MiB. Unlikely users will hit this limit with typical requests.
  • Configurable by a runtime feature, true by default: envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits

Testing:

  • Unit tests for unary/streaming, request/response
  • Integration tests for unary request/responses over buffer limit.
  • Integration tests for streaming responses under/over buffer limit.

Docs Changes: None

Release Notes: Yes

Platform Specific Features: None

@nareddyt nareddyt requested a review from lizan as a code owner February 2, 2021 02:28
@nareddyt nareddyt marked this pull request as draft February 2, 2021 02:28
@nareddyt
Copy link
Contributor Author

nareddyt commented Feb 2, 2021

@qiwzhang Please take a quick look at the draft PR.
@htuch Can you give more context on the scary changes in b/155331763#comment11
cc @TAOXUY

qiwzhang
qiwzhang reviewed Feb 2, 2021
View reviewed changes
source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc Outdated
@@ -530,6 +534,9 @@ Http::FilterDataStatus JsonTranscoderFilter::decodeData(Buffer::Instance& data,
}
} else {
request_in_.move(data);
if (checkIfDecoderBufferLimitReached(request_in_.BytesStored())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not very accurate. incoming request bytes are converted on the flight to proto. It is stored here if I read it correctly.

We may have to count its size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Will spend some time evaluating if there are any other buffers internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a look at the transcoder library. This will only be a problem for the streaming cases. If two streaming messages occur in succession, then the byte limit will be inaccurate and the buffered data will exceed the allowed limit. This should be rare.

For unary request/response, the current implementation works for all cases. The buffer limit is checked before the message reaches any of those internal buffers. So the size unary messages will be counted correctly.

I would like to move forward with this PR and follow-up later. The diff is getting large. I am not worried about introducing a partial implementation here. There is never a case where the transcoder will incorrectly reject a request. It will always count the correct buffer usage or (in the streaming case) undercount buffer usage.

@nareddyt
Add internal buffer limits + unit tests
32ba870 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt
Integration tests
72f5ccb 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt
Fix format
5ce7e0a 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt nareddyt force-pushed the transcoder-buffer-limits branch from 4bd75e7 to 5ce7e0a Compare February 11, 2021 18:29
@nareddyt nareddyt mentioned this pull request Feb 17, 2021
Merged
@nareddyt
Add integration test for under buffer limit
24fbab2 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt
Fix format
7763383 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt
Add release note
5eb8e59 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt nareddyt changed the title [WIP] transcoder: Adhere to decoder and encoder buffer limits [DNS] transcoder: Adhere to decoder and encoder buffer limits Feb 18, 2021
@nareddyt nareddyt changed the title [DNS] transcoder: Adhere to decoder and encoder buffer limits [DNS] grpc_json_transcoder: Adhere to decoder and encoder buffer limits Feb 18, 2021
@nareddyt nareddyt marked this pull request as ready for review February 18, 2021 01:06
@nareddyt
Fix release notes
1a72a01 
Signed-off-by: Teju Nareddy <[email protected]>
qiwzhang
qiwzhang previously approved these changes Feb 18, 2021
View reviewed changes
@antoniovicente
Copy link
Contributor

What does [DNS] refer to in the PR title?

@nareddyt
Change function name
4aba8c5 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt nareddyt requested a review from alyssawilk March 15, 2021 18:24
alyssawilk
alyssawilk previously approved these changes Mar 15, 2021
View reviewed changes
Copy link
Contributor

@alyssawilk alyssawilk left a comment

Choose a reason for hiding this comment

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

@snowp or @mattklein123 would one of you be up for final pass?

@mattklein123 mattklein123 self-assigned this Mar 15, 2021
@mattklein123
Copy link
Member

I can take a look.

mattklein123
mattklein123 requested changes Mar 16, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM with small comments.

/wait

docs/root/version_history/current.rst Show resolved Hide resolved
source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc Show resolved Hide resolved
source/common/http/filter_manager.cc Outdated Show resolved Hide resolved
@nareddyt
Review comments
b9452f7 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt
Copy link
Contributor Author

CI will fail until #15075 is merged.

@nareddyt nareddyt requested a review from mattklein123 March 16, 2021 20:54
@nareddyt
Fix doc
ad963a2 
Signed-off-by: Teju Nareddy <[email protected]>
@nareddyt
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14906 (comment) was created by @nareddyt.

see: more, trace.

@nareddyt
Fix rc details
5be2a1e 
Signed-off-by: Teju Nareddy <[email protected]>
mattklein123
mattklein123 requested changes Mar 17, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM with small comment.

/wait

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc Outdated Show resolved Hide resolved
@nareddyt nareddyt requested a review from mattklein123 March 17, 2021 22:55
@nareddyt
Revert "Fix rc details"
5c2a043 
This reverts commit 5be2a1e

Signed-off-by: Teju Nareddy <[email protected]>
mattklein123
mattklein123 approved these changes Mar 18, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 merged commit dde4eb6 into envoyproxy:main Mar 18, 2021
@nareddyt nareddyt deleted the transcoder-buffer-limits branch March 18, 2021 17:14
@nareddyt nareddyt mentioned this pull request Mar 19, 2021
Merged
@alyssawilk alyssawilk mentioned this pull request Oct 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@alyssawilk alyssawilk alyssawilk left review comments

@lizan lizan Awaiting requested review from lizan

@antoniovicente antoniovicente Awaiting requested review from antoniovicente

@qiwzhang qiwzhang Awaiting requested review from qiwzhang

Assignees

@mattklein123 mattklein123

@qiwzhang qiwzhang

@alyssawilk alyssawilk

@antoniovicente antoniovicente

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nareddyt @antoniovicente @mattklein123 @qiwzhang @alyssawilk