local_reply_config : support content-type in SubstitutionFormatString by deveshkandpal24121990 · Pull Request #13019 · envoyproxy/envoy

deveshkandpal24121990 · 2020-09-09T02:54:16Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: local_reply_config : support content-type in SubstitutionFormatString
Additional Description: Prior to this fix only text_format and json_format were supported which would result in text/plain or application/json content-type. This Introduces content_type field which supports setting content-type for body_format / body_format_override in local_reply_config.
Risk Level: low
Testing: Unit Testing, Manual Testing
Docs Changes: Introduces new field content_type in substitution_format_string.proto that supports setting content-type for body_format / body_format_override in local_reply_config.
Release Notes: Added release notes.
[Optional Fixes #Issue] Fixes #11313

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

repokitteh-read-only · 2020-09-09T02:54:22Z

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

🐱

Caused by: #13019 was opened by deveshkandpal24121990.

see: more, trace.

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

deveshkandpal24121990 · 2020-09-09T04:07:51Z

/retest

repokitteh-read-only · 2020-09-09T04:07:55Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit, please wait.

🐱

Caused by: a #13019 (comment) was created by @deveshkandpal24121990.

see: more, trace.

ramaraochavali

looks good high level,
Update docs https://github.com/envoyproxy/envoy/blob/master/docs/root/configuration/http/http_conn_man/local_reply.rst#local-reply-format-modification
and add release notes

source/common/local_reply/local_reply.cc

ramaraochavali · 2020-09-09T04:36:11Z

test/common/local_reply/local_reply_test.cc

+  auto local = Factory::create(config_, context_);
+
+  // code=400 matches the first filter; rewrite code and body
+  // has its own formatter


nit end with .

test/common/local_reply/local_reply_test.cc

dio · 2020-09-09T09:52:24Z

Instead of this, WDYT of adding a facility to override content_type (or headers, not sure if we want to override more headers) to ResponseMapper instead?

ramaraochavali · 2020-09-09T10:20:18Z

Sure. Is there any benefit of doing that rather than this? I could see one benefit i.e. not polluting SubstitutionFormatString with html_format which is not applicable for access logs? Is that what you are thinking too?

dio · 2020-09-09T10:31:01Z

@ramaraochavali I think that could be a reason, and setting content-type seems one of the mentioned suggestion here: #11313 (comment). And perhaps we can stay to have "structured" via JSON and "unstructured" via text (it seems text/html falls into this, along with e.g. text/xml).

ramaraochavali · 2020-09-09T10:36:01Z

Sure. I am fine with that. @deveshkandpal24121990 WDYT?

htuch · 2020-09-09T15:21:56Z

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

    google.protobuf.Struct json_format = 2 [(validate.rules).message = {required: true}];
+
+    // Specify a format with command operators to form a html string.
+    // Its details is described in :ref:`format string<config_access_log_format_strings>`.


Please update these docs with a description of HTML format.

htuch · 2020-09-09T15:22:22Z

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

+    //
+    // .. code-block::
+    //
+    //   html_format: <p>%LOCAL_REPLY_BODY%:%RESPONSE_CODE%:path=$REQ(:path)%</p>


IIUC, the only difference is that you are setting content type? Would it be preferable to be able to just specify the content-type?

@htuch - do you mean adding just content-type to ResponseMapper as is suggested here -> #13019 (comment) ?

Yeah. I'm thinking there's probably a lot of different variations on content type that might be asked for over time, so adding a special purpose field here is limiting. It makes sense to distinguish text/JSON, since they are structurally different.

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

ramaraochavali

Few comments. @htuch PTAL

ramaraochavali · 2020-09-14T09:30:38Z

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

  // * for ``json_format`` the keys with null values are omitted in the output structure.
  bool omit_empty_values = 3;
+
+  // Specify a content_type for text_format. This will be ignored for json_format


nit: end with .

ramaraochavali · 2020-09-14T09:32:34Z

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

+  //
+  //   content_type: "text/html; charset=UTF-8"
+  //
+  string content_type = 4 [(validate.rules).string = {min_bytes: 1}];


Do we need this validation since it is only valid for text_format?

Also specify currently supported content type values are text/html; charset=UTF-8 and text/plain defaulted to text/plain ?

ramaraochavali · 2020-09-14T09:33:58Z

docs/root/version_history/current.rst

 * access log: added support for :ref:`%DOWNSTREAM_PEER_FINGERPRINT_1% <config_access_log_format_response_flags>` as a response flag.
 * access log: added support for nested objects in :ref:`JSON logging mode <config_access_log_format_dictionaries>`.
 * access log: added :ref:`omit_empty_values<envoy_v3_api_field_config.core.v3.SubstitutionFormatString.omit_empty_values>` option to omit unset value from formatted log.
+* access log: added :ref:`content_type<envoy_v3_api_field_config.core.v3.SubstitutionFormatString.content_type>` option to set content-type for textformat.


I think this is applicable for local reply (not so for access log) - so may be we should release note it under local_reply config?

ramaraochavali · 2020-09-14T09:58:34Z

source/common/formatter/substitution_format_string.cc


+absl::string_view SubstitutionFormatStringUtils::getContentType(
+    const envoy::config::core::v3::SubstitutionFormatString& config) {
+  const std::string contentType = config.content_type();


Simplify this as follows as those are the only ones supported now ?

if (contentType == Http::Headers::get().ContentTypeValues.Html) return Http::Headers::get().ContentTypeValues.Html; else return Http::Headers::get().ContentTypeValues.Text;

Why can't we plumb through the content type directly, without having to rely on well-known values?

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

deveshkandpal24121990 · 2020-09-14T16:34:52Z

addressed comments. @ramaraochavali @htuch PTAL. Thanks!

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

deveshkandpal24121990 · 2020-09-14T21:32:20Z

/retest

repokitteh-read-only · 2020-09-14T21:32:23Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Cannot retry non-completed check: envoy-presubmit (Linux multi-arch docker), please wait.

🐱

Caused by: a #13019 (comment) was created by @deveshkandpal24121990.

see: more, trace.

deveshkandpal24121990 · 2020-09-14T21:33:17Z

the integration test logs complains about //test/extensions/filters/http/oauth2:oauth_integration_test failing. I ran this locally but seems to work just fine. https://dev.azure.com/cncf/4684fb3d-0389-4e0b-8251-221942316e06/_apis/build/builds/51207/logs/108

deveshkandpal24121990 · 2020-09-14T22:29:07Z

/retest

repokitteh-read-only · 2020-09-14T22:29:11Z

Retrying Azure Pipelines, to retry CircleCI checks, use /retest-circle.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #13019 (comment) was created by @deveshkandpal24121990.

see: more, trace.

dio · 2020-09-14T22:37:34Z

source/common/formatter/substitution_format_string.cc

+  if (config.content_type() == Http::Headers::get().ContentTypeValues.Html) {
+    return Http::Headers::get().ContentTypeValues.Html;
+  }


Not sure, but can we pass config.content_type() value as is? And yes, when it is not set and it is not JSON we return text/plain. WDYT @htuch?

@dio - I tried doing that initially, however config.content_type() returns std:string& and after the value would get assigned to absl:string_view content_type_ , the address it points to is not valid anymore after the scope of the method is over. My understanding about string_view is that it's a read-only pointer to the string object being assigned. One way to fix that was to change the datatype of content_type_ to std::string, however I wasn't sure if that is encouraged ? Any other ideas ?

Sure, make content_type_ a std::string.

dio · 2020-09-14T22:37:55Z

source/common/local_reply/local_reply.cc

                    envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat
                ? Http::Headers::get().ContentTypeValues.Json
-                : Http::Headers::get().ContentTypeValues.Text) {}
+                : Formatter::SubstitutionFormatStringUtils::getContentType(config)) {}


So, given: https://github.com/envoyproxy/envoy/pull/13019/files#r488273786, here we can have: config.content_type().empty() ? Http::Headers::get().ContentTypeValues.Text : config.content_type().

htuch

Thanks, API LGTM modulo comments.
/wait

htuch · 2020-09-14T23:49:48Z

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

+
+  // Specify a content_type for text_format. This will be ignored for json_format.
+  // Currently supported content-type value includes ``text/html; charset=UTF-8`` and ``text/plain``
+  // with default value being ``text/plain``.


I think we can avoid relying on well-known values, see below comments. Other than this, this API is fine.

htuch · 2020-09-14T23:50:08Z

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

  // * for ``json_format`` the keys with null values are omitted in the output structure.
  bool omit_empty_values = 3;
+
+  // Specify a content_type for text_format. This will be ignored for json_format.


Please specify defaults when no content_type is specified.

htuch · 2020-09-14T23:50:31Z

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

  // * for ``json_format`` the keys with null values are omitted in the output structure.
  bool omit_empty_values = 3;
+
+  // Specify a content_type for text_format. This will be ignored for json_format.


Nit: *content_type* for *text_format*

htuch · 2020-09-14T23:51:00Z

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

  // * for ``json_format`` the keys with null values are omitted in the output structure.
  bool omit_empty_values = 3;
+
+  // Specify a content_type for text_format. This will be ignored for json_format.


Why ignore for JSON? I feel there are JSON content type variants you might want to support, e.g. https://stackoverflow.com/questions/477816/what-is-the-correct-json-content-type

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

deveshkandpal24121990 · 2020-09-15T04:06:43Z

@htuch @ramaraochavali @dio - PTAL. Thanks!

ramaraochavali

LGTM with minor nits

ramaraochavali · 2020-09-15T04:23:07Z

source/common/local_reply/local_reply.cc

-                    envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat
-                ? Http::Headers::get().ContentTypeValues.Json
-                : Http::Headers::get().ContentTypeValues.Text) {}
+        content_type_(Formatter::SubstitutionFormatStringUtils::getContentType(config)) {}


nit: may be you can inline this initialization

!config.content_type().empty()? config.content_type() : config.format_case() ==
envoy::config::core::v3::SubstitutionFormatString::FormatCase::kJsonFormat
? Http::Headers::get().ContentTypeValues.Json
: Http::Headers::get().ContentTypeValues.Text;
and avoid the method?

ramaraochavali · 2020-09-15T04:26:12Z

docs/root/version_history/current.rst

 * load balancer: added a :ref:`configuration<envoy_v3_api_msg_config.cluster.v3.Cluster.LeastRequestLbConfig>` option to specify the active request bias used by the least request load balancer.
 * load balancer: added an :ref:`option <envoy_v3_api_field_config.cluster.v3.Cluster.LbSubsetConfig.LbSubsetSelector.single_host_per_subset>` to optimize subset load balancing when there is only one host per subset.
 * load balancer: added support for bounded load per host for consistent hash load balancers via :ref:`hash_balance_factor <envoy_api_field_Cluster.CommonLbConfig.consistent_hashing_lb_config>`.
+* local_reply config: added :ref:`content_type<envoy_v3_api_field_config.core.v3.SubstitutionFormatString.content_type>` option to set appropriate content-type for text_format or json_format.


nit: suggestion
added :ref:content_type<envoy_v3_api_field_config.core.v3.SubstitutionFormatString.content_type> option to set content-type.

Specifying text_format or json_format here is redundant here as we support for all formats now.

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

deveshkandpal24121990 · 2020-09-16T01:00:07Z

@htuch - whenever you get a chance, could you PTAL ? Thanks in advance!

htuch

LGTM, thanks!

…envoyproxy#13019) Prior to this fix only text_format and json_format were supported which would result in text/plain or application/json content-type. This Introduces content_type field which supports setting content-type for body_format / body_format_override in local_reply_config. Risk Level: low Testing: Unit Testing, Manual Testing Docs Changes: Introduces new field content_type in substitution_format_string.proto that supports setting content-type for body_format / body_format_override in local_reply_config. Release Notes: Added release notes. Fixes envoyproxy#11313 Co-authored-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com> (cherry picked from commit a8b946e)

Devesh Kandpal added 2 commits September 8, 2020 21:20

local_reply_config : support html format

bfc45e4

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

fix clang-format

84f641f

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

repokitteh-read-only bot added the api label Sep 9, 2020

deveshkandpal24121990 changed the title ~~Devesh24/custom error page~~ local_reply_config : support html format Sep 9, 2020

Devesh Kandpal added 2 commits September 8, 2020 22:34

Fix format in local_reply + substitution_format_string_test

8d5f5fd

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

Fix clang-format again

6207d6c

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

ramaraochavali reviewed Sep 9, 2020

View reviewed changes

htuch reviewed Sep 9, 2020

View reviewed changes

htuch self-assigned this Sep 9, 2020

Devesh Kandpal added 2 commits September 14, 2020 00:16

add content-type to substitution_format_string.proto

4a1fbe0

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

minor fixes

9ab7e4b

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

ramaraochavali reviewed Sep 14, 2020

View reviewed changes

Devesh Kandpal added 3 commits September 14, 2020 07:53

Merge branch 'master' into devesh24/custom-error-page

020eb52

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

Updated doc + address comments

48bd0ba

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

fix typo

88e7856

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

deveshkandpal24121990 changed the title ~~local_reply_config : support html format~~ local_reply_config : support html content-type for textformat Sep 14, 2020

Devesh Kandpal added 2 commits September 14, 2020 13:35

fix clang-tidy check

0c9713b

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

re: clang-format fix

db5486e

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

dio reviewed Sep 14, 2020

View reviewed changes

htuch suggested changes Sep 14, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 14, 2020

deveshkandpal24121990 changed the title ~~local_reply_config : support html content-type for textformat~~ local_reply_config : support content-type in SubstitutionFormatString Sep 15, 2020

change data type for content_type_ + address comments

9dfaa86

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

repokitteh-read-only bot removed the waiting label Sep 15, 2020

ramaraochavali reviewed Sep 15, 2020

View reviewed changes

remove utility method + address comments

95cc592

Signed-off-by: Devesh Kandpal <devesh.kandpal@dkandpal-ltm.internal.salesforce.com>

deveshkandpal24121990 requested a review from htuch September 16, 2020 12:59

htuch approved these changes Sep 16, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 16, 2020

htuch merged commit a8b946e into envoyproxy:master Sep 16, 2020

deveshkandpal24121990 deleted the devesh24/custom-error-page branch September 16, 2020 19:41

Conversation

deveshkandpal24121990 commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 9, 2020

Uh oh!

deveshkandpal24121990 commented Sep 9, 2020

Uh oh!

repokitteh-read-only bot commented Sep 9, 2020

Uh oh!

ramaraochavali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dio commented Sep 9, 2020

Uh oh!

ramaraochavali commented Sep 9, 2020

Uh oh!

dio commented Sep 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramaraochavali commented Sep 9, 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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deveshkandpal24121990 commented Sep 14, 2020

Uh oh!

deveshkandpal24121990 commented Sep 14, 2020

Uh oh!

repokitteh-read-only bot commented Sep 14, 2020

Uh oh!

deveshkandpal24121990 commented Sep 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deveshkandpal24121990 commented Sep 14, 2020

Uh oh!

repokitteh-read-only bot commented Sep 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deveshkandpal24121990 Sep 14, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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

deveshkandpal24121990 commented Sep 9, 2020 •

edited

Loading

dio commented Sep 9, 2020 •

edited

Loading

deveshkandpal24121990 commented Sep 14, 2020 •

edited

Loading

deveshkandpal24121990 Sep 14, 2020 •

edited

Loading