Local reply mapper - implementation by lukidzi · Pull Request #8921 · envoyproxy/envoy

lukidzi · 2019-11-06T23:00:47Z

This is implementation of local reply mapper with ability to change format of response to custom Json. I haven't written any docs yet because I want to ensure you are ok with implementation proposition.

Description: Allows to create custom mapper of response code based on filters. Filter based on response headers might not work because when there is local reply we don't have any response headers. Also added possibility to map response to custom Text or Json format.
Risk Level: Low
Testing: I will add after implementation proposition review.
Docs Changes:
Release Notes:
Fixes #7537

Follow up #8126

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

repokitteh-read-only · 2019-11-06T23:00:53Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8921 was opened by lukidzi.

see: more, trace.

htuch

Awesome, some comments to kick this off. Tests will be required; I think you can start work on integration tests without further feedback, since these will look the same regardless of implementation. API looks good, as previously agreed.
/wait

htuch · 2019-11-07T23:53:41Z

api/envoy/config/filter/accesslog/v2/accesslog.proto

  }
 }

+// [#next-major-version: In the v3 API, we should consider renaming


Nit: please stick to 110 character line lengths, so this shouldn't wrap.

htuch · 2019-11-07T23:54:06Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

  // `HTTP spec <https://tools.ietf.org/html/rfc3986>` and is provided for convenience.
  bool merge_slashes = 33;
+
+  // [#not-implemented-hide:]


Please uncomment if you are going to land this in this PR.

source/common/access_log/access_log_formatter.h

htuch · 2019-11-08T00:00:09Z

source/common/http/utility.cc

+}
+
+void Utility::sendLocalReply(
+    bool is_grpc, std::function<void(HeaderMapPtr&& headers, bool end_stream)> encode_headers,


This method signature keeps growing and growing :) Can we wrap the parameters up into one or more structs that group things together?

htuch · 2019-11-08T00:00:55Z

source/common/local_reply/local_reply.cc

+}
+
+ResponseMapper::ResponseMapper(AccessLog::FilterPtr&& filter, ResponseRewriterPtr&& rewriter) {
+  filter_ = std::move(filter);


Nit: prefer using initialization args.

htuch · 2019-11-08T00:04:15Z

source/common/access_log/access_log_formatter.cc

-                                      const StreamInfo::StreamInfo& stream_info) const {
-  const auto output_map = toMap(request_headers, response_headers, response_trailers, stream_info);
+                                      const StreamInfo::StreamInfo& stream_info,
+                                      const absl::string_view& body) const {


This only applies to locally generated responses, right? We don't have to worry about the remote having a 100MB response? What if someone specifies RESP_BODY on a regular access log formatter?

Body is passed only for local_reply, for access log I'm passing empty string so this shouldn't be a problem.

I think the direct response route action also goes through sendLocalReply. So, keep in mind that potentially largeish files could be present.

source/extensions/filters/network/http_connection_manager/config.cc

euroelessar · 2019-11-08T03:01:44Z

api/envoy/type/format.proto

+    //
+    //   string_format: "My custom message"
+    //
+    string string_format = 1;


should this field be DataSource to allow fetching body from the control plane/disk?

Actually, it looks like string_format assumes formatting.
In this case my ask is to add 3rd option with non-formattable DataSource.

I guess one reasonable question is "which API fields in general should be data sources?". Do we have a governing principle? Is it basically anything you might want to load from a disk? Do you have a concrete needs for this @euroelessar?

Do you have a concrete needs for this @euroelessar?

We want an ability to return custom hard-coded user-facing html page on errors generated by envoy.
In addition to that we want to be able to serve compile-time compressed version of it for clients which support gzip/brotli.

Combination of this two factors implies that we need an ability to return potentially large binary body.
Adding this bodies to the config itself is potentially non-practical, therefore the ask for DataSource.

To clarify, I'm fine with it not being part of this particular pull request, we can contribute the necessary pieces as follow ups, assuming the API can be extended to this needs.

I think it would be nice to get the API clean here, what you're asking for isn't unreasonable. The actual code to consume from data source is a 1 liner, so if folks are in agreement let's do this.

euroelessar · 2019-11-08T03:08:15Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+}
+
+// Configuration of new value for matched local response.
+message ResponseRewriter {


Should there be an ability to override header values?
For example to set content-type or some application-specific chillout header.

It might be added later.

euroelessar · 2019-11-08T03:08:34Z

api/envoy/config/filter/network/http_connection_manager/v2/http_connection_manager.proto

+  // Allows to define custom format of local reply.
+  // It is allowed to use :ref:`command operators <config_access_log_command_operators>`
+  // which will be resolved to actual data.
+  type.StringOrJson format = 2;


Can the format be on per mapper basis, please?
Example: we want to return different body and/or content type depending on the generated status code.

During config proposition we aggred that this might be added later. So I might add it in next PR.

Have read the comments for prior version of pull request. It resolves the comment so no concerns from my side here.

euroelessar · 2019-11-08T03:12:28Z

api/envoy/type/format.proto

+    //
+    //   string_format: "My custom message"
+    //
+    string string_format = 1;


Actually, it looks like string_format assumes formatting.
In this case my ask is to add 3rd option with non-formattable DataSource.

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

mattklein123 · 2019-11-13T00:43:09Z

@lukidzi can you fix format and merge master? Is this ready for full review? Thank you.

/wait

lukidzi · 2019-11-13T07:40:05Z

I'm working on docs so I'll write when it's ready : )

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

alyssawilk

One discussion point while you're doing clean-up...

alyssawilk · 2019-11-14T15:17:53Z

source/common/http/utility.h

+
+struct LocalReplyData {
+  // Tells if this is a response to a gRPC request.
+  bool is_grpc_;


As long as we're doing an invasive refactor here, I wonder if we want to keep these special cased, or clean this up as well.
Arguably sendLocalReply could use request headers to determine is grpc and is head request. Or we could rewrite the two special cases we have as modification functions rather than preserving them as hard-coded one off special cases.

@mattklein123 any thoughts here?

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

repokitteh-read-only · 2019-11-26T06:45:24Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #8921 was synchronize by lukidzi.

see: more, trace.

htuch · 2019-11-26T23:26:52Z

@lukidzi can you ping the reviewers on this PR when this is ready for another pass? It's a bit unclear with the current activity whether reviewer feedback is addressed.

lukidzi · 2019-11-27T07:49:01Z

I think it is ready for first review. I am trying to fix asan build but maybe you will see something that I missed. @htuch @alyssawilk @mattklein123

htuch

Generally looks good, I'm curious to see if we can get a fuzzer also in this PR.

htuch · 2019-11-27T18:13:45Z

docs/root/configuration/http/http_conn_man/local_reply.rst

+Local reply modification
+========================
+
+The :ref:`HTTP connection manager <arch_overview_http_conn_man>` supports modification of local reply which is response returned by Envoy itself.


Nit: "which is the response"

htuch · 2019-11-27T18:14:20Z

docs/root/configuration/http/http_conn_man/local_reply.rst

+--------------------------------
+
+There is support for modification of local replies. You can specify list of :ref:`mappers <envoy_api_field_config.filter.network.http_connection_manager.v2.LocalReplyConfig.mapper>` which contains 
+pairs of :ref:`filter <envoy_api_msg_config.filter.accesslog.v2.AccessLogFilter>` and :ref:`rewriter <envoy_api_msg_config.filter.network.http_connection_manager.v2.ResponseRewriter>`. Both elements in pair has to be


Nit: "Both elements in the pair have to be specified."

htuch · 2019-11-27T18:25:13Z

source/common/http/conn_manager_impl.cc

-          CodeUtility::toString(Http::Code::InternalServerError), absl::nullopt,
-          parent_.is_head_request_);
+          parent_.state_.destroyed_,
+          Utility::EncodeFunctions{[&](HeaderMapPtr&& response_headers, bool end_stream) -> void {


Nit: this isn't your code, but if you can remove the wildcard & capture here, all the better.

htuch · 2019-11-27T18:25:57Z

source/common/http/utility.cc

-    response_headers->setReferenceContentType(Headers::get().ContentTypeValues.Text);
+  HeaderMapPtr response_headers{new HeaderMapImpl{
+      {Headers::get().Status, std::to_string(enumToInt(local_reply_data.response_code_))}}};
+  std::string formatted_body{};


Nit: no need for {} here.

source/extensions/filters/network/http_connection_manager/config.cc

stale · 2019-12-04T19:21:06Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

keontang · 2019-12-23T06:17:17Z

any progress？@lukidzi

stale · 2020-02-02T20:23:47Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 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!

stale · 2020-02-09T20:39:28Z

This pull request has been automatically closed because it has not had activity in the last 14 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!

SaveTheRbtz · 2020-02-22T19:31:49Z

@lukidzi, friendly ping =)

qiwzhang · 2020-04-23T21:48:12Z

@lukidzi we need this feature, are you going to finish this? or you prefer someone else to take over?

qiwzhang · 2020-04-23T21:48:48Z

@mattklein123 @htuch can you re-open this PR?

mattklein123 · 2020-04-23T22:03:22Z

@qiwzhang I want this finished also. If you want to finish it can you just open a new PR if @lukidzi doesn't respond?

qiwzhang · 2020-04-23T22:07:09Z

ok, sure. I will wait for a couple days then.

mattklein123 · 2020-04-23T22:18:42Z

cc @Augustyniak also ^ as you recently asked for this.

Lukasz Dziedziak added 2 commits November 5, 2019 22:22

Added api proposition

44762f6

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Basic implementation with current test working

2d14f71

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

lukidzi requested review from alyssawilk and mattklein123 as code owners November 6, 2019 23:00

repokitteh-read-only bot added the api label Nov 6, 2019

mattklein123 assigned htuch, mattklein123 and alyssawilk Nov 7, 2019

htuch suggested changes Nov 8, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 8, 2019

euroelessar reviewed Nov 8, 2019

View reviewed changes

htuch mentioned this pull request Nov 11, 2019

Enable Configurability of Internal Redirect HTTP Codes #8966

Closed

Added tests and fixed comments from code review

54f3ff4

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

repokitteh-read-only bot removed the waiting label Nov 11, 2019

lukidzi and others added 4 commits November 11, 2019 21:38

Fixed format

8d5bd74

Signed-off-by: Lukasz Dziedziak <lukidzi@gmail.com>

Fixed spelling

525956e

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Merge branch 'master' into local_reply_mapper_impl

2b3d7de

Fixed missing parameter in method after merge

b71957b

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

repokitteh-read-only bot added the waiting label Nov 13, 2019

lizan mentioned this pull request Nov 13, 2019

Use response_headers_to_add for Envoy-generated error responses #8944

Open

Lukasz Dziedziak added 3 commits November 17, 2019 13:48

Added docs and more tests

84660a6

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Merge branch 'master' into local_reply_mapper_impl

f9d6086

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Fix import

3e0d621

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

repokitteh-read-only bot removed the waiting label Nov 17, 2019

Fixed format

bdcaa07

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

alyssawilk reviewed Nov 18, 2019

View reviewed changes

Lukasz Dziedziak added 7 commits November 24, 2019 23:48

Nullptr bcs of header not initialized

ed817a5

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Fix format

b4c82b6

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Merge branch 'master' into local_reply_mapper_impl

b18f1cb

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Fix format

2407530

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Fix problem with empty headers

f4d9a07

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Another try to fix nullptr

613203d

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

Fix test

7af677d

Signed-off-by: Lukasz Dziedziak <lukasz.dziedziak@allegro.pl>

htuch reviewed Nov 27, 2019

View reviewed changes

mattklein123 mentioned this pull request Dec 2, 2019

Fault filter: allows the user to configure the header and body returned by 'FaultAbort' #9178

Closed

mattklein123 added the waiting label Dec 2, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 4, 2019

mattklein123 added the no stalebot Disables stalebot from closing an issue label Dec 4, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 4, 2019

mattklein123 removed the no stalebot Disables stalebot from closing an issue label Jan 26, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 2, 2020

stale bot closed this Feb 9, 2020

qiwzhang mentioned this pull request Apr 30, 2020

http: local reply mapper #11007

Merged

Conversation

lukidzi commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Nov 6, 2019

Uh oh!

htuch 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!

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!

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!

mattklein123 commented Nov 13, 2019

Uh oh!

lukidzi commented Nov 13, 2019

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Nov 26, 2019

Uh oh!

htuch commented Nov 26, 2019

Uh oh!

lukidzi commented Nov 27, 2019

Uh oh!

htuch 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stale bot commented Dec 4, 2019

Uh oh!

keontang commented Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

lukidzi commented Nov 6, 2019 •

edited

Loading

keontang commented Dec 23, 2019 •

edited

Loading