ext_proc: Pass stream_info to gRPC streams#18190
Merged
snowp merged 3 commits intoenvoyproxy:mainfrom Sep 28, 2021
Merged
Conversation
The ext_proc filter was not passing the stream_info structure down to the gRPC stream that it creates. As a result, certain metadata keys could cause the filter to crash when trying to open the stream. Signed-off-by: Gregory Brail <gregbrail@google.com>
Contributor
|
/assign-from @envoyproxy/first-pass-reviewers |
|
@envoyproxy/first-pass-reviewers assignee is @tonya11en |
tonya11en
reviewed
Sep 22, 2021
Signed-off-by: Gregory Brail <gregbrail@google.com>
snowp
reviewed
Sep 24, 2021
Contributor
snowp
left a comment
snowp
left a comment
There was a problem hiding this comment.
Just one question on how this all fits together, but I think this looks good
Comment on lines
+72
to
+75
| // Insert some extra metadata. This ensures that we are actually passing the | ||
| // "stream info" from the original HTTP request all the way down to the | ||
| // ext_proc stream. | ||
| auto* metadata = proto_config_.mutable_grpc_service()->mutable_initial_metadata()->Add(); |
Contributor
There was a problem hiding this comment.
Just so I understand, how is this related to stream info? I would have expected this to be used when the grpc stream is created by reading the config, not sure how this would be plumbed though via StreamInfo?
Contributor
Author
|
I don't know the ins and outs of metadata and our gRPC client, but I think
that "%REQ" syntax means, "pull information from the original request and
add it to the stream." The grpc client seems to do that using the
stream_info, and since I wasn't setting that when opening the gRPC stream
to the external processor, it was crashing while trying to get that
information.
…On Fri, Sep 24, 2021 at 9:47 AM Snow Pettersen ***@***.***> wrote:
***@***.**** commented on this pull request.
Just one question on how this all fits together, but I think this looks
good
------------------------------
In test/extensions/filters/http/ext_proc/streaming_integration_test.cc
<#18190 (comment)>:
> + // Insert some extra metadata. This ensures that we are actually passing the
+ // "stream info" from the original HTTP request all the way down to the
+ // ext_proc stream.
+ auto* metadata = proto_config_.mutable_grpc_service()->mutable_initial_metadata()->Add();
Just so I understand, how is this related to stream info? I would have
expected this to be used when the grpc stream is created by reading the
config, not sure how this would be plumbed though via StreamInfo?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#18190 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I26NIP6MS2WERROC75DUDSTTPANCNFSM5EMYGCQQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Contributor
|
Got it, I didn't realize that the important bit was the REQ query, thanks! Seems like this will need a main merge |
Contributor
Author
|
No problem -- I had two in flight at the same time. Let me do that and
you'll see it pop up.
…On Fri, Sep 24, 2021 at 10:53 AM Snow Pettersen ***@***.***> wrote:
Got it, I didn't realize that the important bit was the REQ query, thanks!
Seems like this will need a main merge
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#18190 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I275CLFUEF6KNPK6WBLUDS3JHANCNFSM5EMYGCQQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
Signed-off-by: Gregory Brail <gregbrail@google.com>
mpuncel
added a commit
to mpuncel/envoy
that referenced
this pull request
Oct 29, 2021
* main: (114 commits) kafka: add header support to mesh-filter (envoyproxy#18248) rbac: add support for upstream ip policy. (envoyproxy#17645) SIPProxy BUGFIX UT failure for fastbuild/debug (envoyproxy#18303) quic: updating goaway code (envoyproxy#18291) various tiny fixes (envoyproxy#18287) dns cache: remove assert at this layer (envoyproxy#18301) [ext_authz]: ext_authz filter unit test that use real threading (envoyproxy#17742) signal action: fully disable sigaltstack on Apple (envoyproxy#18299) Add missing dependencies (envoyproxy#18297) ext_proc: Pass stream_info to gRPC streams (envoyproxy#18190) use clang 12 (envoyproxy#18220) Update PR template to include the "Fixes commit" message when reverting or fixing bad commits (envoyproxy#18298) [test] Fixing integration test to cleanup cleanly (envoyproxy#18293) test: moving grpc bridge tests out of core directory (envoyproxy#18227) runtime: disable deprecated extensions names by default (envoyproxy#18239) quiche: updating deps (envoyproxy#18272) sip_proxy: SIP protocol support in envoy (envoyproxy#18039) http: add core retry policy to route retry policy conversion utility (envoyproxy#17803) build: updating stale visibility (envoyproxy#18278) alternate_protocols_cache: Impose a max size limit on the alternate protocols cache (envoyproxy#18258) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message: The ext_proc filter was not passing the stream_info structure down to the gRPC stream that it creates. As a result, certain metadata keys could cause the filter to crash when trying to open the stream.
Additional Description:
Risk Level: Low. Makes the filter more resilient.
Testing: New code in integration test failed before this fix was applied and now passes.