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

esmet · 2020-12-04T16:54:37Z

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:

repokitteh-read-only · 2020-12-04T16:54:40Z

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: #14276 was opened by esmet.

see: more, trace.

repokitteh-read-only · 2020-12-04T16:54:45Z

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: #14276 was opened by esmet.

see: more, trace.

qiwzhang · 2020-12-04T18:38:44Z

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

+    DataSource text_format_source = 5;
+
+    // Specify fixed text with no command operators.
+    string fixed_text = 6;


what is the purpose of this format? Especially how it is used in file_access_log

That's a good question and I realize now that I forgot to call out the original motivation for this PR. I linked #14221 as context.

The idea here is that we want a generic way to define HTTP response body rewrites. One interesting use case is to replace a non-empty HTTP body with an empty one, or one with a fixed string, like "denied". We'd use this in both the response_map (PR above) and the local_reply config.

One clear alternative to fixed_text is to relax the minimum length validation on text_format so that it supports an empty string.

Yeah as I said in #14221, I prefer just relax the text format validation. Will we see performance improvement of this formatter over the normal text_format?

Great. I will relax the format validation and remove fixed_text.

re: performance, I think it stands to reason that fixed format would perform better for that use case but I don't currently have any data to say whether it will matter in practice

This is polluting SubstitutionFormatString class. I see 1) DataSource of text_format and 2) fixed_text should not be part of this SubstitutionFormatString class. They should be in the ResponseMapper message in #14221

While I think it could be correct to specify a DataSource field in the ResponseMapper message in #14221, I also think it's reasonable and correct to allow for the text_format to come from a DataSource on SubstitutionFormatString, since a DataSource supports basic strings (via inline_string). The semantics of SubstitutionFormatString remain the same, we just have a behavioral extension that allows it to come from a source, which is nice. What do you think about the tradeoff of having the additional behavior here versus having users of this class define their own DataSource field to replace it?

I think there's also a reasonable argument to be made that the implementation of a SubstitutionFormatString using a DataSouce field was really straightforward. To move the source up a layer of abstraction, we'd need the option of creating a format string by telling it where it is sourced from. This conflicts with the existing fields in the format string that tell it where it is sourced from (i.e. text_format and json_format).

It is an interesting idea of replacing json_format with text_format.

Now that I think about it, though, json_format strings are escaped for json while text_format strings are not. That's fairly important and prevents us from totally getting rid of the json_format field in favor of text_format until we have a way to guarantee that json-as-text can be escaped nicely. Maybe we could add/extend the command operator(s) to support escaping?

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

…FormatString` This commit also adds PlainStringFormatterImpl to implement `fixed_text` Signed-off-by: John Esmet <[email protected]>

lizan

Please don't force push, it will screw up review history.

lizan · 2020-12-04T22:47:34Z

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

+    DataSource text_format_source = 5;
+
+    // Specify fixed text with no command operators.
+    string fixed_text = 6;


Yeah as I said in #14221, I prefer just relax the text format validation. Will we see performance improvement of this formatter over the normal text_format?

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

esmet · 2020-12-04T22:52:20Z

Sure, makes sense.

…text_format add coverage to local_reply_test Signed-off-by: John Esmet <[email protected]>

qiwzhang

Need to fix tests.

qiwzhang · 2020-12-08T22:08:37Z

test/common/local_reply/local_reply_test.cc

+              default_value: 404
+              runtime_key: key_b
+      body_format_override:
+        text_format: ""


Isn't this removed?

it's now deprecated, but not removed. since I removed the validation, I wanted to make sure there was a test case to cover the empty string case.

I see. Thanks

lizan · 2020-12-09T23:31:19Z

can you check CI?

esmet · 2020-12-11T23:24:51Z

can you check CI?

Looks like tests are failing since we deprecated text_format.

How about we postpone the deprecation to another PR? This will allow us to move forward. If we do deprecate in this PR, then we'll need to update all of the tests to use inline_string, but then we won't be able to test the empty string case because we haven't related validation there (yet).

So, maybe we should deprecate both text_source and relax the inline_string validation in the next PR. What do you think?

lizan · 2020-12-12T00:04:48Z

How about we postpone the deprecation to another PR? This will allow us to move forward. If we do deprecate in this PR, then we'll need to update all of the tests to use inline_string, but then we won't be able to test the empty string case because we haven't related validation there (yet).

You can still test it by wrapping test name with DEPRECATED_FEATURE_TEST, see how other test does. Deprecating doesn't mean removing tests.

esmet · 2020-12-12T00:06:20Z

Cool let me try that

Signed-off-by: John Esmet <[email protected]>

…o local_reply to cover the deprecated text_format with an empty string Signed-off-by: John Esmet <[email protected]>

Signed-off-by: John Esmet <[email protected]>

lizan

Thanks, this code LGTM, can you add release note? Need a line in deprecated section.

Signed-off-by: John Esmet <[email protected]>

…ter-impl Signed-off-by: John Esmet <[email protected]>

lizan

LGTM moduolo one nit. Thank you!

lizan · 2020-12-14T20:47:17Z

docs/root/version_history/current.rst

@@ -91,6 +93,7 @@ New Features

 Deprecated
 ----------
+* formatter: :ref:`text_format <envoy_v3_api_field_config.core.v3.SubstitutionFormatString.text_format>` is now deprecated in favor of :ref:`text_format_source <envoy_v3_api_field_config.core.v3.SubstitutionFormatString.text_format_source>`. To migrate existing text format strings, use the :ref:`inline_string <envoy_v3_api_field_config.core.v3.DataSource.inline_string>` field.


nit: alphabetical order

Signed-off-by: John Esmet <[email protected]>

repokitteh-read-only bot added the api label Dec 4, 2020

repokitteh-read-only bot assigned lizan Dec 4, 2020

esmet mentioned this pull request Dec 4, 2020

Response map HTTP filter for custom upstream responses #14221

Closed

qiwzhang reviewed Dec 4, 2020

View reviewed changes

formatter: add fixed_text and text_format_source on `Substitution…

4296eee

…FormatString` This commit also adds PlainStringFormatterImpl to implement `fixed_text` Signed-off-by: John Esmet <[email protected]>

esmet force-pushed the plain-string-formatter-impl branch from d95c3a2 to 4296eee Compare December 4, 2020 19:12

lizan suggested changes Dec 4, 2020

View reviewed changes

qiwzhang previously approved these changes Dec 7, 2020

View reviewed changes

esmet changed the title ~~formatter: add fixed_text and text_format_source~~ formatter: add text_format_source, relax minimum string length on text_format Dec 8, 2020

remove fixed_text, deprecate text_format, relax length validation on …

c56d981

…text_format add coverage to local_reply_test Signed-off-by: John Esmet <[email protected]>

esmet dismissed qiwzhang’s stale review via c56d981 December 8, 2020 20:57

qiwzhang suggested changes Dec 8, 2020

View reviewed changes

qiwzhang previously approved these changes Dec 9, 2020

View reviewed changes

esmet added 2 commits December 12, 2020 02:33

Fix tests and sample configs

7cb9bad

Signed-off-by: John Esmet <[email protected]>

Update tests to use text_format_source.inline_string and add a test t…

9e00736

…o local_reply to cover the deprecated text_format with an empty string Signed-off-by: John Esmet <[email protected]>

esmet dismissed qiwzhang’s stale review via 9e00736 December 12, 2020 02:56

esmet added 3 commits December 12, 2020 20:42

Kick CI

0f2598a

Signed-off-by: John Esmet <[email protected]>

Update some more tests to use text_format_source.inline_string

781ca3b

Signed-off-by: John Esmet <[email protected]>

Format

b7ab922

Signed-off-by: John Esmet <[email protected]>

lizan reviewed Dec 13, 2020

View reviewed changes

esmet added 3 commits December 14, 2020 16:10

Add release notes

6720661

Signed-off-by: John Esmet <[email protected]>

Fix typo

4cb058e

Signed-off-by: John Esmet <[email protected]>

Fix links

081b015

Signed-off-by: John Esmet <[email protected]>

esmet requested a review from lizan December 14, 2020 17:57

esmet added 2 commits December 14, 2020 18:06

Update links in api protos

fe251ae

Signed-off-by: John Esmet <[email protected]>

Merge remote-tracking branch 'origin/master' into plain-string-format…

e111088

…ter-impl Signed-off-by: John Esmet <[email protected]>

qiwzhang previously approved these changes Dec 14, 2020

View reviewed changes

lizan reviewed Dec 14, 2020

View reviewed changes

Reorder Deprecated section

7afb821

Signed-off-by: John Esmet <[email protected]>

esmet dismissed qiwzhang’s stale review via 7afb821 December 14, 2020 21:25

lizan approved these changes Dec 15, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Dec 15, 2020

lizan merged commit 43bf707 into envoyproxy:master Dec 15, 2020

esmet deleted the plain-string-formatter-impl branch December 16, 2020 00:33

lizan mentioned this pull request Dec 17, 2020

api: relax inline_string length limitation in DataSource #14461

Merged

pims mentioned this pull request Dec 29, 2022

Add support for json_format_source to Access Logs #24710

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

esmet commented Dec 4, 2020 •

edited

Loading

repokitteh-read-only bot commented Dec 4, 2020

repokitteh-read-only bot commented Dec 4, 2020

qiwzhang Dec 4, 2020

esmet Dec 4, 2020 •

edited

Loading

lizan Dec 4, 2020

esmet Dec 4, 2020 •

edited

Loading

qiwzhang Dec 5, 2020

esmet Dec 7, 2020

esmet Dec 7, 2020

qiwzhang Dec 7, 2020

esmet Dec 7, 2020

lizan left a comment

lizan Dec 4, 2020

esmet commented Dec 4, 2020

qiwzhang left a comment

qiwzhang Dec 8, 2020

esmet Dec 9, 2020

qiwzhang Dec 9, 2020

lizan commented Dec 9, 2020

esmet commented Dec 11, 2020

lizan commented Dec 12, 2020

esmet commented Dec 12, 2020

lizan left a comment

lizan left a comment

lizan Dec 14, 2020

esmet Dec 14, 2020

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

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

Conversation

esmet commented Dec 4, 2020 • edited Loading

repokitteh-read-only bot commented Dec 4, 2020

repokitteh-read-only bot commented Dec 4, 2020

Choose a reason for hiding this comment

esmet Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esmet Dec 4, 2020 • edited Loading

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

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esmet commented Dec 4, 2020

qiwzhang 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

lizan commented Dec 9, 2020

esmet commented Dec 11, 2020

lizan commented Dec 12, 2020

esmet commented Dec 12, 2020

lizan left a comment

Choose a reason for hiding this comment

lizan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esmet commented Dec 4, 2020 •

edited

Loading

esmet Dec 4, 2020 •

edited

Loading

esmet Dec 4, 2020 •

edited

Loading