http/filter: Add a callback for sendLocalReply() and encode gRPC message for local responses when the request is gRPC #3299

jrajahalme · 2018-05-05T01:32:19Z

Some gRPC clients get confused when they get non-gRPC responses to
their gRPC requests. While such clients should be fixed, we can play
nice and format the local responses returned by Envoy router filter as
gRPC responses when the request is gRPC.

Risk Level: Medium
Testing: A HTTP2 integration test is added to exercise the new behavior.
None of the existing tests were affected.
Docs Changes: version_history.rst updated.
Fixes: #1934
Signed-off-by: Jarno Rajahalme [email protected]

TODO:

Noted in a comment that grpc-message header value should be %-encoded.

htuch

Thanks for the contribution, I think this makes a lot of sense.

htuch · 2018-05-07T02:47:24Z

source/common/router/router.cc

-        {Http::Headers::get().Status,
-         std::to_string(enumToInt(route_entry_->clusterNotFoundResponseCode()))}}};
-    callbacks_->encodeHeaders(std::move(response_headers), true);
+    sendLocalReply(route_entry_->clusterNotFoundResponseCode(), "");


Could we make this a meaningful message?

Tried it, made some tests fail (likely due to body in the response being somehow unexpected), so did not look at it further.

htuch · 2018-05-07T02:47:42Z

source/common/grpc/common.cc

+      {Http::Headers::get().ContentType, Http::Headers::get().ContentTypeValues.Grpc},
+      {Http::Headers::get().GrpcStatus, std::to_string(enumToInt(grpc_status))}}};
+  if (!message.empty()) {
+    response_headers->insertGrpcMessage().value(message);


We could just punt on this for now and leave it as a TODO if not needed. FWIW, I don't recall percent encoding being mandatory, but maybe this has changed, since the wire format at https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md specifies it, with some confusing language (e.g. UTF-8 followed by percent encoding?!). @lizan for thoughts here.

Yeah I would just TODO it. I don't think it's explicitly required at the h2 level AFAIK. Maybe it is for gRPC.

Yeah I think a TODO here is fine (open an issue to track it) for now. We don't have local messages requires % encoding, also it should just work since h2 doesn't require this.

Added TODO, which is now in http/utility.cc

htuch · 2018-05-07T02:55:49Z

test/integration/http_integration.cc

@@ -626,6 +626,21 @@ void HttpIntegrationTest::testRetry() {
  EXPECT_EQ(512U, response_->body().size());
 }

+// Change the default route to be restrictive, and send a request to an alternate route.
+void HttpIntegrationTest::testGrpcRouterNotFound() {


Could also consider adding a unit test in router_test. I don't think it would bring any increased functional or code coverage, but it's the unit test counterpart, would complement existing gRPC tests there and should be simple. Up to you IMHO.

The functionality is now in http/utility.cc, added a new unit test to test/common/http/utility_test.cc.

mattklein123

Thanks, this is a longstanding point of confusion for people. One high level comment is IMO we should do this everywhere we send local replies. E.g., https://github.com/envoyproxy/envoy/blob/master/source/extensions/filters/http/buffer/buffer_filter.cc#L116 (there are other examples).

In a perfect world we would push this functionality directly into the send local reply utility which is already used everywhere, but, that has the downside that every filter now has to check if its dealing with gRPC.

In a perfect, perfect world, I think we would make sendLocalReply() a filter API, such that the connection manager would handle this logic as something built-in.

At minimum, I think we should TODO this, and not call the underlying issue fixed yet, but if you want to tackle the better solution feel free. :)

mattklein123 · 2018-05-07T14:07:18Z

source/common/grpc/common.cc

+      {Http::Headers::get().ContentType, Http::Headers::get().ContentTypeValues.Grpc},
+      {Http::Headers::get().GrpcStatus, std::to_string(enumToInt(grpc_status))}}};
+  if (!message.empty()) {
+    response_headers->insertGrpcMessage().value(message);


Yeah I would just TODO it. I don't think it's explicitly required at the h2 level AFAIK. Maybe it is for gRPC.

alyssawilk · 2018-05-07T14:47:55Z

Thanks for tackling this - it'll be great to have our response paths be consistent about the application layer :-)

I had the same suggestion as Matt here but he beat me to it - we're going to eventually want this to apply to all local replies and I agree that it doing this one step at a time makes sense. One thought - it would be nice if the first step moved us towards a reusable function rather than adding custom code to the router filter. What do we think of moving the wrapper code for "gRPC or normal sendLocalReply" to somewhere generally accessible (http connection manager is fine by me) even if we don't move any other callers of sendLocalReply over to it in this PR?

lizan · 2018-05-07T17:32:18Z

In a perfect, perfect world, I think we would make sendLocalReply() a filter API, such that the connection manager would handle this logic as something built-in.

WDYT about making this to a grpc-status-converter filter that can applied at the first of the HTTP filter chain? The filter can catch all non-200 status response from all local replies as well as from backend. It basically does 3. in #1934 (comment)

The gRPC spec does say clients should expect non-200 status and accept it. When they doesn't, the filter can be used to convert all non-200 status to a gRPC error that doesn't confuse clients.

htuch · 2018-05-07T17:45:26Z

+1 to @lizan suggestion, it would simplify Envoy internals here. In general, I wonder how much we can remove from the router and migrate to a filter.

mattklein123 · 2018-05-07T18:17:24Z

I like the idea of moving certain functionality to a filter, but it will be a little hard because the connection manager itself sends local replies, so in practice I'm not sure how well it will work in this case...

jrajahalme · 2018-05-07T19:02:53Z

Agree that this should be done for all local replies. I first tried to do this at http/utility, but that failed due to circular bazel dependency with grpc/common. One solution to this would be to move the needed status mapping functionality to http/utility. Then, if we passed request headers to sendLocalReply() we could have callers be unaware of gRPC with the minimal cost of inspecting content type for gRPC also when it is already known that the request is gRPC. Like the idea of extending the filter callbacks to send local replies.

If implemented as a filter, then (at least we) would need to configure all http filter chains with the filter as we don't know if/when/what clients have problems with non gRPC responses.

mattklein123 · 2018-05-07T20:31:03Z

Yeah, for this particular case I think I'm for inclined to add this to the filter API directly vs. do it as a filter, since IMO gRPC is a core concept that Envoy should just support out of the box.

htuch · 2018-05-07T21:21:08Z

OK, so the only actionable feedback is to add TODO/open issue and my above comments?

jrajahalme · 2018-05-07T21:24:39Z

I'll take a stab to place the new code to http/utility and maybe use the new functionality from more/all the callers of sendLocalReply() as well.

jrajahalme · 2018-05-08T22:18:34Z

@mattklein123 Now looking into adding this to the filter API. Currently local responses sent by filters are passed to the the encodeHeaders() callback in decoder filter callbacks, either directly, or via one of the utility functions. This is implemented so that encodeHeaders() encoder filter member of every encoder filter is called. I would have expected only the encoders downstream of the current decoder filter to see the local response, but apparently all of them, from first to last get the encodeHeaders() call. This may be confusing if later filters that have not seen the request somehow transform the response.

My question is: should we keep this behavior for local responses sent via the new API? If the local response is sent by a encoder/decoder filter it should be possible to locate that filter from the encoder chain and start the encoding iteration from there, or from the previous encoder, rather than from the beginning of the encoder chain. This may get complicated if the local response is sent by a decoder that is not a encoder, as it will not be as easy to figure out which encoders should see the local response. At the minimum all the decoders that are also encoders that have seen the request should also see the local response. Thoughts?

mattklein123 · 2018-05-09T00:14:19Z

@jrajahalme yeah agreed this is a problem in the current filter iteration system. Great callout! I agree that it would be possible for filters to get confused and only filters downstream should see the response. But I also agree with your assessment that the code really needs to only do this for dual encoder/decoder filters. IMO it's likely fine to skip this for now with some clear comments/TODO somewhere (and even better maybe some type of ASSERT if we are about to do something we might later regret). @alyssawilk thoughts here?

alyssawilk · 2018-05-09T13:57:56Z

Yeah, I think logically we should make that change, and definitely have docs explaining it since I suspect folks may get confused either way :-)

Some gRPC clients get confused when they get non-gRPC responses to their gRPC requests. While such clients should be fixed, we can play nice and format the local responses returned by Envoy router filter as gRPC responses when the request is gRPC. Fixes: envoyproxy#1934 Signed-off-by: Jarno Rajahalme <[email protected]>

Refactor by moving gRPC utility functions that only depend on http to http/utility. This allows making Http::Utility::sendLocalReply() gRPC aware without changing all the callers to be aware of gRPC. Signed-off-by: Jarno Rajahalme <[email protected]>

Add either an 'is_grpc' or 'request_headers' parameter to sendLocalResponse() in order to allow the implementation to issue a gRPC response to gRPC requests. Signed-off-by: Jarno Rajahalme <[email protected]>

Add a StreamDecoderFilter callback sendLocalReply() and convert current users of Utility::sendLocalReply() to use the callback instead. The new callback is implemented using Utility::sendLocalReply(), but making use of locally available state allows simplifying the API, enabling support for gRPC responses without requiring filters to become gRPC aware. Note that the local responses are still implemented via the encodeHeaders() and encodeData() callbacks that call the corresponding member functions of all encoders in the filter chain, not just the ones downstream of the filter issuing the sendLocalReply(). This has the notable effect of the filter issuing the local reply itself receiving a encodeHeaders() call for the local reply it issued. Signed-off-by: Jarno Rajahalme <[email protected]>

jrajahalme · 2018-05-10T01:40:57Z

Sorry for the manual rebase! First commit is same (but rebased) as before, other commits address comments and implement the new callback we discussed above.

jrajahalme · 2018-05-10T01:44:58Z

@htuch Addressed your review comments, thanks!

jrajahalme · 2018-05-10T01:46:49Z

@mattklein123 IMO it would be best to add a new GH issue for the local responses being traversed through all encoders?

Include for http/utility.h went missing in a master rebase, add it back. Signed-off-by: Jarno Rajahalme <[email protected]>

mattklein123 · 2018-05-10T16:18:02Z

@jrajahalme can you check CI errors and then I will review?

jrajahalme · 2018-05-10T19:55:04Z

Sure, will take care of the CI. It passed locally, so did not expect it to fail...

jrajahalme · 2018-05-11T18:08:13Z

lc_trie_test fails on me locally as well, but the log is not very helpful:

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
-----------------------------------------------------------------------------
[==========] Running 14 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 14 tests from LcTrieTest
[ RUN      ] LcTrieTest.IPv4Defaults
[       OK ] LcTrieTest.IPv4Defaults (63 ms)
[ RUN      ] LcTrieTest.RootBranchingFactor
[       OK ] LcTrieTest.RootBranchingFactor (67 ms)
[ RUN      ] LcTrieTest.IPv4AddressSizeBoundaries
[       OK ] LcTrieTest.IPv4AddressSizeBoundaries (62 ms)
[ RUN      ] LcTrieTest.IPv4Boundaries
[       OK ] LcTrieTest.IPv4Boundaries (124 ms)
[ RUN      ] LcTrieTest.IPv6
[       OK ] LcTrieTest.IPv6 (66 ms)
[ RUN      ] LcTrieTest.IPv6AddressSizeBoundaries
[       OK ] LcTrieTest.IPv6AddressSizeBoundaries (72 ms)
[ RUN      ] LcTrieTest.IPv6Boundaries
[       OK ] LcTrieTest.IPv6Boundaries (144 ms)
[ RUN      ] LcTrieTest.CatchAllIPv4Prefix
[       OK ] LcTrieTest.CatchAllIPv4Prefix (122 ms)
[ RUN      ] LcTrieTest.CatchAllIPv6Prefix
[       OK ] LcTrieTest.CatchAllIPv6Prefix (133 ms)
[ RUN      ] LcTrieTest.BothIpvVersions
[       OK ] LcTrieTest.BothIpvVersions (136 ms)
[ RUN      ] LcTrieTest.NestedPrefixes
[       OK ] LcTrieTest.NestedPrefixes (131 ms)
[ RUN      ] LcTrieTest.NestedPrefixesWithCatchAll
[       OK ] LcTrieTest.NestedPrefixesWithCatchAll (138 ms)
[ RUN      ] LcTrieTest.MaximumEntriesException
[       OK ] LcTrieTest.MaximumEntriesException (40706 ms)
[ RUN      ] LcTrieTest.ExceedingAllocatedTrieNodesException

mattklein123 · 2018-05-11T18:10:34Z

@jrajahalme you are hitting #3298. Don't worry about it for now.

jrajahalme · 2018-05-11T18:11:18Z

Yes, just verified it fails the same way locally on master as well.

…Impl. Move sendLocalReply base implementation to ActiveStream, so that it is usable also from ActiveStream. Use sendLocaslReply() instead of encoding headers directly so that we get gRPC responses in all cases when the request was gRPC. Signed-off-by: Jarno Rajahalme <[email protected]>

Signed-off-by: Jarno Rajahalme <[email protected]>

jrajahalme · 2018-05-11T20:03:27Z

Just about to add a commit that changes all encodeHeaders() calls that are used to send local responses to use sendLocalReply() instead. This requires moving the base implementation of sendLocalReply() to ActiveStream.

mattklein123 · 2018-05-11T20:04:42Z

@jrajahalme I was about to review, but will hold off if you are still making major changes. Please ping me when I should review.

jrajahalme · 2018-05-11T20:05:26Z

@mattklein123 Now, just merged the new commits to the branch :-)

jrajahalme · 2018-05-11T20:06:49Z

@mattklein123 CI passed before the last two commits, and those passed all tests for me locally, so IMO it's good to review.

Signed-off-by: Jarno Rajahalme <[email protected]>

jrajahalme · 2018-05-11T20:09:49Z

Except for that I messed the format, just pushed a fix for that.

htuch · 2018-05-11T20:51:42Z

source/common/http/utility.cc

@@ -186,6 +186,88 @@ bool Utility::isWebSocketUpgradeRequest(const HeaderMap& headers) {
                    Http::Headers::get().UpgradeValues.WebSocket.c_str())));
 }

+Grpc::Status::GrpcStatus Utility::httpToGrpcStatus(uint64_t http_response_status) {


I'd also suggest leaving this function and the one below under source/common/grpc.

As it is, it is not possible due to circular Bazel dependencies, i.e., grpc/common depends on http:utility. Would have to make callers of Utility::sendLocalReply() replicate the gRPC status conversion.

+1 to @htuch, I think you should be able to move it to source/common/grpc:status, then it won't be circular dependencies.

OK, adding source/common/grpc/status_lib.

mattklein123

Thanks, this is great. I have a few comments and I would like @alyssawilk to also take a look on Monday.

mattklein123 · 2018-05-11T22:57:52Z

include/envoy/http/filter.h

@@ -190,6 +190,17 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks {
   */
  virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE;

+  /**
+   * Create a locally generated response using the provided lambdas.


Can you provide some docs/indication that this may be transparently converted to a gRPC response in certain cases? Some of the param text might also need to be altered so that it describes gRPC also.

mattklein123 · 2018-05-11T23:03:07Z

source/common/http/utility.cc

  HeaderMapPtr response_headers{
      new HeaderMapImpl{{Headers::get().Status, std::to_string(enumToInt(response_code))}}};
  if (!body_text.empty()) {
    response_headers->insertContentLength().value(body_text.size());
    response_headers->insertContentType().value(Headers::get().ContentTypeValues.Text);
  }
-
+  // Somehow it's OK to encode headers even if 'is_reset' is true?


What does this comment mean? The issue here is calling encode_headers() may result in a reset. You should be able to ASSERT(!is_reset); before this call. Can you add that and change the comment to be a bit better?

I did not understand why the following encode_data() call is only done if !is_reset, but this preceding encode_headers() is not conditional on is_reset at all. Also, if ASSERT before this is proper, then the conditional on !is_reset before the following encode_data() is not needed any more, and the whole purpose of the is_reset parameter is for us to ASSERT on it?

It's because the stream can get reset recursively as a result of the encodeHeaders() call. I realize this is probably not intuitive, but that's why this whole dance exists. Can you update comments and add relevant asserts? I think it will make it more clear.

Done, took a while for me realize that is_reset can change before the encode_data() call as it is a reference. I'll add a comment to clarify that as well.

mattklein123 · 2018-05-11T23:04:17Z

source/extensions/filters/http/buffer/buffer_filter.cc

@@ -113,8 +113,7 @@ void BufferFilter::onDestroy() {
 }

 void BufferFilter::onRequestTimeout() {
-  Http::Utility::sendLocalReply(*callbacks_, stream_destroyed_, Http::Code::RequestTimeout,


I think the only reason we are tracking stream_destroyed_ is because of this call. Can you remove that member variable? this comment likely applies to other filters also.

mattklein123 · 2018-05-11T23:05:33Z

source/common/router/router.cc

@@ -198,6 +181,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e

  downstream_headers_ = &headers;

+  grpc_request_ = Grpc::Common::hasGrpcContentType(headers);


I wonder, should we have this be a callback API so the filter doesn't have to recompute/track this? We are making gRPC a fundamental part of request processing at this point which IMO makes sense.

Can you add a TODO here about possibly adding a filter API for this?

mattklein123 · 2018-05-11T23:09:11Z

source/common/http/conn_manager_impl.cc

@@ -653,7 +651,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte
  for (; entry != decoder_filters_.end(); entry++) {
    ASSERT(!(state_.filter_call_state_ & FilterCallState::DecodeHeaders));
    state_.filter_call_state_ |= FilterCallState::DecodeHeaders;
-    FilterHeadersStatus status = (*entry)->handle_->decodeHeaders(
+    FilterHeadersStatus status = (*entry)->decodeHeaders(


This change is not intuitive. Can you add some comments. Is the idea that you want to recompute gRPC status before each filter? I think that makes sense, but worth some comments here and in the wrapper to forwards to parent.

Right, in case a filter bridges to/from gRPC, for example.

Will add comments.

mattklein123 · 2018-05-11T23:13:21Z

source/common/http/conn_manager_impl.h

@@ -257,6 +269,9 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
    void decodeTrailers(ActiveStreamDecoderFilter* filter, HeaderMap& trailers);
    void maybeEndDecode(bool end_stream);
    void addEncodedData(ActiveStreamEncoderFilter& filter, Buffer::Instance& data, bool streaming);
+    void sendLocalReply(ActiveStreamEncoderFilter* filter, bool is_grpc_request, Code code,


I think the first param here is always set to nullptr? What is the intention to eventually allow proper restart for filters downstream? For now, I would just remove the parameter and add TODOs around allowing proper restart if the filter is also an encoding filter?

Right, but as it is still a TODO, I'll remove it and add the comments to the encodeHeaders() and encodeData() calls that take the nullptr filter pointer as the first parameter in the implementation of sendLocalReply().

alyssawilk · 2018-05-14T13:54:00Z

source/common/http/conn_manager_impl.cc

@@ -821,6 +819,24 @@ void ConnectionManagerImpl::ActiveStream::refreshCachedRoute() {
  cached_route_ = std::move(route);
 }

+void ConnectionManagerImpl::ActiveStream::sendLocalReply(
+    ActiveStreamEncoderFilter* filter, bool is_grpc_request, Code code, const std::string& body,


Oh, I really like where this is going!

My one question for APIs is id we think sendLocalReply should instead take a reference to the request headers so is_grpc_request can be calculated locally in just one place, and in case we eventually end up with other transformations which might be based on request headers. We can just push this as-is and iterate, but I'd be interested in @mattklein123's take now since I think this is likely to be used in many places here and in downstream filters so it'd be nice to get it right on first pass.

@alyssawilk I think this is internal code and not the main API, right? (So I think the code does what you are asking?)

Yeah, this is what happens when I do reviews before caffeine :-/

I think we could avoid latching is_grpc_request_ in the two places we do, but that's a smaller request.

You mean having is_grpc_request_ in both ConnManagerImpl and AsyncStreamImpl? Right now I don't have a clear picture of the relation between the two, so I don't know if tracking in one of them can be eliminated or not.

Now that sendLocalReply() callback takes care of stream status tracking itself, such tracking is not needed in the filters any more. Suggested-by: Matt Klein <[email protected]> Signed-off-by: Jarno Rajahalme <[email protected]>

Clarify the comments regarding the referenced 'is_reset' parameter to sendLocalReply(). ASSERT that it is initially false. Suggested-by: Matt Klein <[email protected]> Signed-off-by: Jarno Rajahalme <[email protected]>

Remove the first parameter from sendLocalReply() as it currently is always nullptr. Add comments on the endoceHeaders() and decodeHeaders() calls to note about the filter that should start encoding. Signed-off-by: Jarno Rajahalme <[email protected]>

Add comments to sendLocalReply() about gRPC encoding of the sent replies. Signed-off-by: Jarno Rajahalme <[email protected]>

Keep gRPC status code conversion utilities in common/grpc, but put them into a separate bazel lib from common_lib to avoid circular bazel dependency between common/http/utility_lib and common/grpc/common_lib. Signed-off-by: Jarno Rajahalme <[email protected]>

htuch

LGTM, just one minor comment.

htuch · 2018-05-15T10:32:36Z

test/mocks/http/mocks.h

@@ -206,6 +221,8 @@ class MockStreamDecoderFilterCallbacks : public StreamDecoderFilterCallbacks,
  MOCK_METHOD0(continueDecoding, void());
  MOCK_METHOD2(addDecodedData, void(Buffer::Instance& data, bool streaming));
  MOCK_METHOD0(decodingBuffer, const Buffer::Instance*());
+  MOCK_METHOD3(sendLocalReply_, void(Code code, const std::string& body,


How is this used? Looks like sendLocalReply is overriden above, but doesn't call into the mock method.

My intention was to call the mock method from the real implementation (which is needed to keep existing tests expecting encodeHeaders() calls working), but forgot to do so. I'll remove the unused mock method for now.

Signed-off-by: Jarno Rajahalme <[email protected]>

lizan · 2018-05-15T18:03:54Z

source/common/http/conn_manager_impl.cc

-          {Headers::get().Status, std::to_string(enumToInt(Code::UpgradeRequired))}};
-      encodeHeaders(nullptr, headers, true);
+      // Send "Upgrade Required" if HTTP/1.0 support is not explictly configured on.
+      sendLocalReply(Grpc::Common::hasGrpcContentType(*request_headers_), Code::UpgradeRequired, "",


I don't think you need to check Content-Type in this case, it's HTTP/1.0 so just set to false.

mattklein123

LGTM. Few small comments + @lizan comment. Awesome!

mattklein123 · 2018-05-15T18:43:31Z

include/envoy/http/filter.h

@@ -190,6 +190,21 @@ class StreamDecoderFilterCallbacks : public virtual StreamFilterCallbacks {
   */
  virtual void addDecodedData(Buffer::Instance& data, bool streaming_filter) PURE;

+  /**
+   * Create a locally generated response using the provided response_code and body_text paramewters.


typo "paramewters"

Fixes, thanks.

mattklein123 · 2018-05-15T18:44:12Z

include/envoy/http/filter.h

+   *
+   * @param response_code supplies the HTTP response code.
+   * @param body_text supplies the optional body text which is sent using the text/plain content
+   *                  type.


nit: "... or encoded in the grpc-message header."

mattklein123 · 2018-05-15T18:47:29Z

source/common/http/async_client_impl.h

+    Utility::sendLocalReply(
+        is_grpc_request_,
+        [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
+          if (headers != nullptr && modify_headers != nullptr) {


can headers ever be nullptr here?

Correct, removed test for non-null.

mattklein123 · 2018-05-15T18:48:36Z

source/common/http/conn_manager_impl.cc

+    std::function<void(HeaderMap& headers)> modify_headers) {
+  Utility::sendLocalReply(is_grpc_request,
+                          [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
+                            if (headers != nullptr && modify_headers != nullptr) {


same comment can headers be nullptr? I think not?

Right, it can't, removed the test for non-null.

mattklein123 · 2018-05-15T18:50:53Z

source/common/router/router.cc

@@ -198,6 +181,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e

  downstream_headers_ = &headers;

+  grpc_request_ = Grpc::Common::hasGrpcContentType(headers);


Can you add a TODO here about possibly adding a filter API for this?

mattklein123 · 2018-05-15T18:52:44Z

test/mocks/http/mocks.h

+    Utility::sendLocalReply(
+        is_grpc_request_,
+        [this, modify_headers](HeaderMapPtr&& headers, bool end_stream) -> void {
+          if (headers != nullptr && modify_headers != nullptr) {


headers can't be nullptr?

Ditto. Also added the TODO comment you asked for above (GitHub would not let me comment up there).

Signed-off-by: Jarno Rajahalme <[email protected]>

mattklein123

Thank you!

jrajahalme · 2018-05-16T01:08:19Z

Thanks for the timely reviews!

htuch self-assigned this May 7, 2018

htuch requested a review from alyssawilk May 7, 2018 02:33

htuch assigned alyssawilk and lizan and unassigned alyssawilk May 7, 2018

htuch removed the request for review from alyssawilk May 7, 2018 02:37

htuch suggested changes May 7, 2018

View reviewed changes

mattklein123 reviewed May 7, 2018

View reviewed changes

jrajahalme added 4 commits May 9, 2018 18:34

http: Make Utility::sendLocalResponse() aware of gRPC.

29ba376

Add either an 'is_grpc' or 'request_headers' parameter to sendLocalResponse() in order to allow the implementation to issue a gRPC response to gRPC requests. Signed-off-by: Jarno Rajahalme <[email protected]>

jrajahalme force-pushed the grpc-local-responses branch from a8bbb2e to 2c264bf Compare May 10, 2018 01:38

http: Add a missing include.

c21e507

Include for http/utility.h went missing in a master rebase, add it back. Signed-off-by: Jarno Rajahalme <[email protected]>

jrajahalme added 2 commits May 11, 2018 11:42

filters: Use sendLocalReply() decoder callback for local replies.

a47b2a1

Signed-off-by: Jarno Rajahalme <[email protected]>

conn_manager_impl: Fix format.

6bd512a

Signed-off-by: Jarno Rajahalme <[email protected]>

htuch reviewed May 11, 2018

View reviewed changes

mattklein123 reviewed May 11, 2018

View reviewed changes

alyssawilk reviewed May 14, 2018

View reviewed changes

jrajahalme added 5 commits May 14, 2018 14:50

filters: Remove unnecessary tracking of stream status.

26aa335

Now that sendLocalReply() callback takes care of stream status tracking itself, such tracking is not needed in the filters any more. Suggested-by: Matt Klein <[email protected]> Signed-off-by: Jarno Rajahalme <[email protected]>

http: Clarify comments on sendLocalReply()

dea2787

Clarify the comments regarding the referenced 'is_reset' parameter to sendLocalReply(). ASSERT that it is initially false. Suggested-by: Matt Klein <[email protected]> Signed-off-by: Jarno Rajahalme <[email protected]>

filter: Add comment about gRPC encoding of local responses.

e125f63

Add comments to sendLocalReply() about gRPC encoding of the sent replies. Signed-off-by: Jarno Rajahalme <[email protected]>

htuch reviewed May 15, 2018

View reviewed changes

test: http mock cleanup.

74724fd

Signed-off-by: Jarno Rajahalme <[email protected]>

htuch previously approved these changes May 15, 2018

View reviewed changes

lizan reviewed May 15, 2018

View reviewed changes

mattklein123 reviewed May 15, 2018

View reviewed changes

http: Final review comment fixes.

2b57fc4

Signed-off-by: Jarno Rajahalme <[email protected]>

jrajahalme dismissed htuch’s stale review via 2b57fc4 May 15, 2018 21:11

mattklein123 approved these changes May 15, 2018

View reviewed changes

mattklein123 merged commit 5843954 into envoyproxy:master May 15, 2018

junr03 mentioned this pull request Jun 4, 2018

access log: add response flag filter #3536

Merged

		@@ -198,6 +181,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e

		downstream_headers_ = &headers;

		grpc_request_ = Grpc::Common::hasGrpcContentType(headers);

http/filter: Add a callback for sendLocalReply() and encode gRPC message for local responses when the request is gRPC #3299

http/filter: Add a callback for sendLocalReply() and encode gRPC message for local responses when the request is gRPC #3299

Conversation

jrajahalme commented May 5, 2018 • edited Loading

htuch 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented May 7, 2018

lizan commented May 7, 2018

htuch commented May 7, 2018

mattklein123 commented May 7, 2018

jrajahalme commented May 7, 2018

mattklein123 commented May 7, 2018

htuch commented May 7, 2018

jrajahalme commented May 7, 2018

jrajahalme commented May 8, 2018 • edited Loading

mattklein123 commented May 9, 2018

alyssawilk commented May 9, 2018

jrajahalme commented May 10, 2018

jrajahalme commented May 10, 2018

jrajahalme commented May 10, 2018

mattklein123 commented May 10, 2018

jrajahalme commented May 10, 2018

jrajahalme commented May 11, 2018

mattklein123 commented May 11, 2018

jrajahalme commented May 11, 2018

jrajahalme commented May 11, 2018

mattklein123 commented May 11, 2018

jrajahalme commented May 11, 2018

jrajahalme commented May 11, 2018

jrajahalme commented May 11, 2018

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

mattklein123 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

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

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

Choose a reason for hiding this comment

htuch 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

mattklein123 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrajahalme commented May 5, 2018 •

edited

Loading

jrajahalme commented May 8, 2018 •

edited

Loading