Response map HTTP filter for custom upstream responses by esmet · Pull Request #14221 · envoyproxy/envoy

esmet · 2020-11-30T21:18:32Z

Support custom upstream responses using a new response_map filter.

The response map filter behaves like the local reply code, but on the HTTP filter chain instead. This allows for modification of responses generated both locally and upstream.

The motivation and implementation strategy for this change is captured in https://docs.google.com/document/d/1THyRnepx89FbK2VVaqoEHXv2knbm_lsLx2XgDZ7FxDw/edit?usp=sharing

Commit Message:
Additional Description:
Risk Level: medium (new filter, refactored common code)
Testing: unit tests done, integration tests still needed.
Docs Changes: NEEDED
Release Notes: NEEDED
Platform Specific Features:
Fixes #12883

Signed-off-by: John Esmet <john.esmet@gmail.com>

The original ResponseMap implementation largely copied from local_reply.cc with some specializations. Now, LocalReply is a ResponseMap specialization whose configuration comes from type whose from an object on the HttpConnectionManager. Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only · 2020-11-30T21:18:36Z

Hi @esmet, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14221 was opened by esmet.

see: more, trace.

repokitteh-read-only · 2020-11-30T21:18:41Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @lizan
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14221 was opened by esmet.

see: more, trace.

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet · 2020-11-30T22:51:34Z

source/common/response_map/response_map.h

+                       Http::ResponseHeaderMap& response_headers,
+                       StreamInfo::StreamInfo& stream_info, Http::Code& code, std::string& body,
+                       absl::string_view& content_type) const PURE;
+


Still needs comments

esmet · 2020-11-30T22:52:00Z

source/common/response_map/response_map.h

+using ResponseMapPtr = std::unique_ptr<ResponseMap>;
+
+/**
+ * Access log filter factory that reads from proto.


This comment could be improved

esmet · 2020-11-30T22:53:16Z

source/extensions/filters/http/response_map/BUILD

+licenses(["notice"])  # Apache 2
+
+# L7 HTTP filter that implements response modifications using matchers.
+# Public docs: docs/root/configuration/http_filters/response_map_filter.rst


This doesn't exist yet.

esmet · 2020-11-30T22:55:42Z

source/extensions/filters/http/response_map/response_map_filter.cc

+ *
+ * Not guaranteed to be called. In particular, if Envoy sends a local reply with HTTP 426
+ * as an upgrade response for HTTP/1.0 requests, we may never decode any of the client's
+ * original headers, because the request was rejected at the initial HTTP/1.0 line.


This comment might be better off above the original decodeHeaders function declaration.

esmet · 2020-11-30T22:59:15Z

source/extensions/filters/http/response_map/response_map_filter.cc

+
+  const ResponseMap::ResponseMapPtr& response_map = *response_map_;
+
+  // Fill in new_body and new_content_type using the response map rewrite.


Extend this comment to mention that we capture the rewritten code here per the method signature, but we don't actually do anything with the new value.

esmet · 2020-11-30T23:00:13Z

source/extensions/filters/http/response_map/response_map_filter.cc

+  // Encoding buffer may be null here even if we saw data in encodeData above. This
+  // happens when sendLocalReply sends a response downstream. By adding encoded data
+  // here, we do successfully override the sendLocalReply body, but it's not clear
+  // how or why.


We should get some clarity around this condition here and rewrite the comment to explain what's going on instead of asking questions.

esmet · 2020-11-30T23:01:13Z

source/extensions/filters/http/response_map/response_map_filter.h

+    return Http::FilterTrailersStatus::Continue;
+  }
+  void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override {
+    decoder_callbacks_ = &callbacks;


We never use this, so it should probably be removed.

esmet · 2020-11-30T23:01:53Z

test/common/stream_info/test_util.h

  const absl::optional<std::string>& responseCodeDetails() const override {
    return response_code_details_;
  }
+  void setResponseCode(uint32_t code) override { response_code_ = code; }


I would be in favor of moving this small addition to another pull request if others think that's better.

alyssawilk · 2020-12-01T15:34:00Z

api/envoy/config/response_map/v3/response_map.proto

+// [#next-free-field: 6]
+message ResponseMapper {
+  // Filter to determine if this mapper should apply.
+  accesslog.v3.AccessLogFilter filter = 1 [(validate.rules).message = {required: true}];


@lizan can I ask you to do an API pass before I dig into the code?
I think a lot of the components of access logs apply but not all do - not sure how much API refactor we want to do here and how much we can just do via standard metadata matcher.

@snowp can you weigh in here re: the new matcher API fits here? I think the access log filter works well for this purpose but just want to double check.

I would think that this could be accomplished with the new matching API, though we would most likely spend some time to reach parity in terms of supporting all the same conditionals that the access log filter supports. The matching API would offer some benefit in terms of better performance when lots of matchers are used, due to being able to structure it more as a tree than as a list of complex matchers.

I think this falls into the category that we'd use the matching API if it was available today but since that work is ongoing it becomes a question of time lines. Maybe @mattklein123 has thoughts on whether we're okay with relying on the access log filters here.

I could imagine the access log filter eventually being subsumed by the new matching work, but that would be a longer term thing

Yeah, I'd initially hoped the new matcher code could be used, since using access filters just strikes me as suboptimal and I'd like the perf wins. Looks like it'd also do away with much of the shared code, so probably the way to go (if we opt to have a standalone filter for per-route) is stick with this for now, and move both the HCM and filter matchers over to the new API when it's more feature complete.

…t test Signed-off-by: John Esmet <john.esmet@gmail.com>

… string Signed-off-by: John Esmet <john.esmet@gmail.com>

Signed-off-by: John Esmet <john.esmet@gmail.com>

lizan · 2020-12-02T21:08:20Z

api/envoy/config/core/v3/substitution_format_string.proto

+    DataSource text_format_source = 5;
+
+    // The empty format string.
+    bool empty_format = 6 [(validate.rules).bool = {const: true}];


I don't see where this is used, shall we just relax the min_len: 1 constraint on text_format if we want an empty format string?

We use it here https://github.com/envoyproxy/envoy/pull/14221/files#diff-7a3f06d418ccdf0de0f51a473b6d3450baa5faf9ac8362aa6fe33bc1284b5152R27

We could just relax the constraint, but my initial idea was to avoid making changes to an existing field and figured the explicit "empty" option was clear enough.

I would slightly prefer relax the existing one, or we can have a field fixed_text which just do what PlainStringFormatterImpl does.

Signed-off-by: John Esmet <john.esmet@gmail.com>

lizan

can we split the matcher / substitution part to another PR?

lizan · 2020-12-04T11:24:55Z

api/envoy/config/core/v3/substitution_format_string.proto

+    DataSource text_format_source = 5;
+
+    // The empty format string.
+    bool empty_format = 6 [(validate.rules).bool = {const: true}];


I would slightly prefer relax the existing one, or we can have a field fixed_text which just do what PlainStringFormatterImpl does.

lizan · 2020-12-04T11:26:43Z

api/envoy/config/response_map/v3/response_map.proto

+// [#next-free-field: 6]
+message ResponseMapper {
+  // Filter to determine if this mapper should apply.
+  accesslog.v3.AccessLogFilter filter = 1 [(validate.rules).message = {required: true}];


@snowp can you weigh in here re: the new matcher API fits here? I think the access log filter works well for this purpose but just want to double check.

esmet · 2020-12-04T14:53:50Z

can we split the matcher / substitution part to another PR?

Yes, will do. I will also look into replacing empty_format with fixed_text and passing that text to PlainStringFormatterImpl.

esmet · 2020-12-04T17:03:36Z

@lizan #14276 for the fixed_text etc changes

lizan · 2020-12-08T06:58:50Z

api/envoy/config/response_map/v3/response_map.proto

@@ -0,0 +1,89 @@
+syntax = "proto3";


This is not where extension config should live, move to api/envoy/extensions/filters/http/response_map/v3

The intention here is for this to be non-extension config (for source/common/response_map/*) and to have separate extension config for the response map filter (source/extensions/http/filters/response_map/...).

Is this idea correct or misguided?

I looked harder at how the API is organized and I think I understand. My feeling now is that this should live under api/envoy/extensions/common/reponse_map since I intend for it to be shared, eventually, by both the HCM and ResponseMap filter. Does that make sense?

lizan · 2020-12-08T07:00:20Z

api/envoy/config/response_map/v3/response_map.proto

+
+// The configuration to filter and change HTTP responses.
+// [#next-free-field: 6]
+message ResponseMapper {


This isn't different from ResponseMapper in HCM, why not just use it?

The idea is to have a common config that could be shared across HCM and any other use case (like the response map http filter). I think we should deprecate the response mapper on the HCM to use this proto instead. I didn't do that here because I wasn't sure of the right/approved migration path. What do you think?

I captured my thoughts here https://docs.google.com/document/d/1THyRnepx89FbK2VVaqoEHXv2knbm_lsLx2XgDZ7FxDw and added a link to the PR description

Thanks, can you give us comment access to the doc?

lizan · 2020-12-08T07:00:48Z

api/envoy/config/response_map/v3/response_map.proto

+}
+
+// The configuration to customize HTTP responses read by Envoy.
+message ResponseMap {


This is same as LocalReplyConfig in HCM as well?

Yep. From the comment above, I intended for there to be a common proto shared across use cases and that we should (eventually) migrate HCM to use this common type. I would appreciate thoughts/guidance on a migration strategy.

lizan

Thanks for putting in the doc, let's discuss about design options there first and come back to the actual implementation.

The major concern I have is this configuration model as a filter could be very confusing, as the local reply by routers and filters are also going through encoder filter so it will affected by both LocalReplyConfig and this filter. Having these configuration in separate places doesn't seems a good design. cc @alyssawilk @envoyproxy/api-shepherds

htuch · 2020-12-09T00:28:23Z

Could we just add an option to the HCM response mapper to also apply to upstream responses?

lizan · 2020-12-09T01:30:01Z

Could we just add an option to the HCM response mapper to also apply to upstream responses?

Yeah I'm leaning to this as well.

mattklein123 · 2020-12-09T01:31:21Z

Yeah I'm leaning to this as well.

+1. I was just about to start looking through the doc, but I'm strongly against this amount of duplication and it seems like we can just extend the mapper to work with upstream responses also. Can we potentially re-orient the doc to that solution and then talk through any issues that come up in the doc?

esmet · 2020-12-09T01:45:54Z

Yeah I'm leaning to this as well.

+1. I was just about to start looking through the doc, but I'm strongly against this amount of duplication and it seems like we can just extend the mapper to work with upstream responses also. Can we potentially re-orient the doc to that solution and then talk through any issues that come up in the doc?

I'll look into how we could do this in the HCM and update the doc with the new research. When I originally designed this code, I was thinking that the filter chain would be invoked for all sendLocalReply calls but @lizan showed me that this isn't true in all cases (in particular, stream timeouts etc).

esmet · 2020-12-09T03:19:28Z

I updated the doc. It now tries to focus much more on the problem statement and possible solutions and less on the particulars of this pull request or even code in general. I will now start doing more code research and will update the doc with those findings as they become relevant.

alyssawilk · 2020-12-09T17:36:04Z

I think that having the HCM matcher be able to alter upstream responses is worth doing, especially for the common case where some server has global response tweaks they want to apply which can't be done via existing config knobs, but there's still the problem of per-route config to deal with. That feels like a better fit for the RouteEntryImplBase::perFilterConfig which currently requires an explicit filter, though if the @envoyproxy/api-shepherds have a way to generalize that it could address that particular concern.

mattklein123 · 2020-12-09T18:17:11Z

That feels like a better fit for the RouteEntryImplBase::perFilterConfig which currently requires an explicit filter, though if the @envoyproxy/api-shepherds have a way to generalize that it could address that particular concern.

Yeah agreed that we should have per-route config in addition to generic matching, though I think this is also an issue for local replies, since in many cases a local reply may be generated when a route is available. I would need to look into it in more detail but I wonder if we could just have a non-filter key for override config that could be looked up?

alyssawilk · 2020-12-09T18:32:18Z

If you're OK with a non-filter key that works for me. Alternately @esmet suggested offline that (with some possible tweaking to the per-filter-config code) we could arguably use the HCM network filter name (it is a filter, just not L7 =P) instead of an arbitrary string. WDYT?

mattklein123 · 2020-12-09T18:36:08Z

If you're OK with a non-filter key that works for me. Alternately @esmet suggested offline that (with some possible tweaking to the per-filter-config code) we could arguably use the HCM network filter name (it is a filter, just not L7 =P) instead of an arbitrary string. WDYT?

Either SGTM. I agree the latter sounds interesting and we could have a generic config message that might eventually allow overriding various pieces of HCM behavior on a per-route basis.

lizan · 2020-12-09T23:29:47Z

I think that having the HCM matcher be able to alter upstream responses is worth doing, especially for the common case where some server has global response tweaks they want to apply which can't be done via existing config knobs, but there's still the problem of per-route config to deal with. That feels like a better fit for the RouteEntryImplBase::perFilterConfig which currently requires an explicit filter, though if the @envoyproxy/api-shepherds have a way to generalize that it could address that particular concern.

If we're not implementing the response mapper in a filter, then it can be a field in Route message and shouldn't be part of typed_per_filter_config anyway? Then it won't use RouteEntryImplBase::perFilterConfig either.

alyssawilk · 2020-12-10T20:40:07Z

if we're not doing it in a filter, don't we want it all in the HCM/FM for behavioral consistency? it's not clear to me if we'd need the route specific logic if Envoy sent a local reply late in the response process, but the FM is the only entity which sees the direct responses.

…t_format (#14276) This change is motivated by #14221 where we use a SubstitutionFormatString as a way to define custom HTTP response body rewrites. Commit Message: formatter: add text_format_source, relax minimum string length on text_format in SubstitutionFormatString Additional Description: The relaxed field validation on text_format now allows a user to replace something with nothing, e.g. to replace a non-empty HTTP response body with an empty one. The text_format_source field allows for a DataSource to be used to supply text inside of providing it inline. Risk Level: low (new fields) Testing: unit test needed for text_format_source Docs Changes: NEEDED Release Notes: NEEDED Platform Specific Features: Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet · 2021-01-04T20:33:42Z

This is still on my plate. The next thing that I plan to do is to try moving the response mapping code out of an HTTP extension and into the HCM itself.

esmet · 2021-02-05T22:02:08Z

I submitted a new draft PR that implements this behavior in the FilterManager #14926 feedback welcome.

I'm keeping branch/PR unchanged so we have the HTTP filter implementation to compare to, and preserve history.

github-actions · 2021-03-08T00:06:49Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-03-15T00:06:56Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

alexandru-ersenie · 2021-04-06T19:07:15Z

@esmet : The way i look at it is that your work is still in progress, and there is no way to control the error pages for example, returned by the upstream.

As we are using envoy as a proxy to the Istio Mesh, my understanding is we'd have to wait a little bitt longer, am i correct to assume that ?

Thanks !

esmet · 2021-04-16T13:51:31Z

@alexandru-ersenie yes, this work is still in progress and I need to get it to a place where we achieve consensus around how it will be implemented. I will hopefully be able to pick this up again soon.

John Esmet and others added 2 commits November 30, 2020 18:10

response_map: add filter for matching and modifying HTTP responses

8d3bc9a

Signed-off-by: John Esmet <john.esmet@gmail.com>

repokitteh-read-only bot added the api label Nov 30, 2020

repokitteh-read-only bot assigned lizan Nov 30, 2020

esmet mentioned this pull request Nov 30, 2020

Custom upstrem/backend response #12883

Open

esmet marked this pull request as draft November 30, 2020 21:24

fix spelling

eac8a47

Signed-off-by: John Esmet <john.esmet@gmail.com>

esmet changed the title ~~[DRAFT] Response map HTTP filter for custom upstream responses~~ Response map HTTP filter for custom upstream responses Nov 30, 2020

esmet commented Nov 30, 2020

View reviewed changes

alyssawilk self-assigned this Dec 1, 2020

alyssawilk reviewed Dec 1, 2020

View reviewed changes

esmet added 3 commits December 1, 2020 22:13

test/extensions/filters/http/response_map: (WIP) add response map uni…

7e464d2

…t test Signed-off-by: John Esmet <john.esmet@gmail.com>

Support empty responses using PlainStringFormatterImpl with the empty…

955aa8a

… string Signed-off-by: John Esmet <john.esmet@gmail.com>

Refactor test, add more test cases

87ffaf5

Signed-off-by: John Esmet <john.esmet@gmail.com>

lizan reviewed Dec 2, 2020

View reviewed changes

esmet marked this pull request as ready for review December 3, 2020 17:49

esmet added 2 commits December 3, 2020 21:35

Add common/response_map/response_map_test.cc, update filter test

eb61815

Signed-off-by: John Esmet <john.esmet@gmail.com>

Fix format and spelling

5adaad9

Signed-off-by: John Esmet <john.esmet@gmail.com>

lizan reviewed Dec 4, 2020

View reviewed changes

esmet mentioned this pull request Dec 4, 2020

formatter: add text_format_source, relax minimum string length on text_format #14276

Merged

lizan suggested changes Dec 8, 2020

View reviewed changes

mattklein123 self-assigned this Dec 9, 2020

mattklein123 added the waiting label Dec 9, 2020

Base automatically changed from master to main January 15, 2021 23:02

esmet mentioned this pull request Feb 3, 2021

Local reply and upstream response rewrites using the FilterManager #14926

Closed

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Mar 8, 2021

github-actions bot closed this Mar 15, 2021

pims mentioned this pull request Dec 29, 2022

Add support for json_format_source to Access Logs #24710

Open


		const ResponseMap::ResponseMapPtr& response_map = *response_map_;

		// Fill in new_body and new_content_type using the response map rewrite.

Conversation

esmet commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Nov 30, 2020

Uh oh!

repokitteh-read-only bot commented Nov 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esmet commented Dec 4, 2020

Uh oh!

esmet commented Dec 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

esmet Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

htuch commented Dec 9, 2020

Uh oh!

lizan commented Dec 9, 2020

Uh oh!

mattklein123 commented Dec 9, 2020

Uh oh!

esmet commented Nov 30, 2020 •

edited

Loading

esmet Dec 8, 2020 •

edited

Loading