grpc_json_transcoder: Adhere to decoder and encoder buffer limits by nareddyt · Pull Request #14906 · envoyproxy/envoy

nareddyt · 2021-02-02T02:28:12Z

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 · 2021-02-02T02:31:36Z

@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

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

qiwzhang · 2021-02-02T21:21:08Z

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

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.

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

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.

Signed-off-by: Teju Nareddy <nareddyt@google.com>

… limit Signed-off-by: Teju Nareddy <nareddyt@google.com>

source/common/http/filter_manager.cc

Signed-off-by: Teju Nareddy <nareddyt@google.com>

antoniovicente · 2021-02-24T20:56:56Z

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

Signed-off-by: Teju Nareddy <nareddyt@google.com>

alyssawilk

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

mattklein123 · 2021-03-15T22:41:41Z

I can take a look.

mattklein123

Thanks, LGTM with small comments.

/wait

docs/root/version_history/current.rst

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

source/common/http/filter_manager.cc

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt · 2021-03-16T20:54:22Z

CI will fail until #15075 is merged.

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt · 2021-03-17T16:50:40Z

/retest

repokitteh-read-only · 2021-03-17T16:50:47Z

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

🐱

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

see: more, trace.

Signed-off-by: Teju Nareddy <nareddyt@google.com>

mattklein123

Thanks LGTM with small comment.

/wait

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc

This reverts commit 5be2a1e Signed-off-by: Teju Nareddy <nareddyt@google.com>

mattklein123

Thanks!

nareddyt requested a review from lizan as a code owner February 2, 2021 02:28

nareddyt marked this pull request as draft February 2, 2021 02:28

qiwzhang suggested changes Feb 2, 2021

View reviewed changes

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Feb 2, 2021

View reviewed changes

nareddyt added 3 commits February 10, 2021 14:58

Add internal buffer limits + unit tests

32ba870

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Integration tests

72f5ccb

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix format

5ce7e0a

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt force-pushed the transcoder-buffer-limits branch from 4bd75e7 to 5ce7e0a Compare February 11, 2021 18:29

Support and test sendLocalReply for server streaming exceeds buffer…

81eb653

… limit Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt commented Feb 12, 2021

View reviewed changes

source/common/http/filter_manager.cc Show resolved Hide resolved

nareddyt added 4 commits February 12, 2021 16:50

Merge branch 'main' into transcoder-buffer-limits

7826265

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix comments

36814b7

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Add one more integration test for http body

ca3c8f3

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix CI test failures

6db6f1c

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt mentioned this pull request Feb 17, 2021

hcm: Handle stream destroy during encodeData due to sendLocalReply #15075

Merged

nareddyt added 3 commits February 17, 2021 16:15

Add integration test for under buffer limit

24fbab2

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Fix format

7763383

Signed-off-by: Teju Nareddy <nareddyt@google.com>

Add release note

5eb8e59

Signed-off-by: Teju Nareddy <nareddyt@google.com>

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 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 marked this pull request as ready for review February 18, 2021 01:06

nareddyt requested review from antoniovicente and qiwzhang February 18, 2021 01:06

Fix release notes

1a72a01

Signed-off-by: Teju Nareddy <nareddyt@google.com>

qiwzhang previously approved these changes Feb 18, 2021

View reviewed changes

antoniovicente assigned qiwzhang and antoniovicente Feb 24, 2021

antoniovicente assigned alyssawilk Feb 25, 2021

Change function name

4aba8c5

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt requested a review from alyssawilk March 15, 2021 18:24

alyssawilk previously approved these changes Mar 15, 2021

View reviewed changes

mattklein123 self-assigned this Mar 15, 2021

mattklein123 requested changes Mar 16, 2021

View reviewed changes

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

repokitteh-read-only bot added the waiting label Mar 16, 2021

Review comments

b9452f7

Signed-off-by: Teju Nareddy <nareddyt@google.com>

nareddyt dismissed alyssawilk’s stale review via b9452f7 March 16, 2021 20:46

repokitteh-read-only bot added waiting and removed waiting labels Mar 16, 2021

nareddyt requested a review from mattklein123 March 16, 2021 20:54

Fix doc

ad963a2

Signed-off-by: Teju Nareddy <nareddyt@google.com>

repokitteh-read-only bot removed the waiting label Mar 16, 2021

mattklein123 added the waiting label Mar 17, 2021

Fix rc details

5be2a1e

Signed-off-by: Teju Nareddy <nareddyt@google.com>

repokitteh-read-only bot removed the waiting label Mar 17, 2021

mattklein123 requested changes Mar 17, 2021

View reviewed changes

source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Mar 17, 2021

nareddyt requested a review from mattklein123 March 17, 2021 22:55

Revert "Fix rc details"

5c2a043

This reverts commit 5be2a1e Signed-off-by: Teju Nareddy <nareddyt@google.com>

repokitteh-read-only bot removed the waiting label Mar 17, 2021

mattklein123 approved these changes Mar 18, 2021

View reviewed changes

mattklein123 merged commit dde4eb6 into envoyproxy:main Mar 18, 2021

nareddyt deleted the transcoder-buffer-limits branch March 18, 2021 17:14

nareddyt mentioned this pull request Mar 19, 2021

Upgrade security posture of grpc_json_transcoder #15576

Merged

alyssawilk mentioned this pull request Oct 5, 2021

envoy.reloadable_features.grpc_json_transcoder_adhere_to_buffer_limits deprecation #18437

Closed

Conversation

nareddyt commented Feb 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nareddyt commented Feb 2, 2021

Uh oh!

Uh oh!

qiwzhang Feb 2, 2021

Choose a reason for hiding this comment

Uh oh!

nareddyt Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

nareddyt Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antoniovicente commented Feb 24, 2021

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Mar 15, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nareddyt commented Mar 16, 2021

Uh oh!

nareddyt commented Mar 17, 2021

Uh oh!

repokitteh-read-only bot commented Mar 17, 2021

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nareddyt commented Feb 2, 2021 •

edited

Loading