-
Notifications
You must be signed in to change notification settings - Fork 5.5k
local_reply_config : support content-type in SubstitutionFormatString #13019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
bfc45e4
84f641f
8d5f5fd
6207d6c
4a1fbe0
9ab7e4b
020eb52
48bd0ba
88e7856
0c9713b
db5486e
9dfaa86
95cc592
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,21 @@ message SubstitutionFormatString { | |
| // } | ||
| // | ||
| 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>`. | ||
| // | ||
| // .. code-block:: | ||
| // | ||
| // html_format: <p>%LOCAL_REPLY_BODY%:%RESPONSE_CODE%:path=$REQ(:path)%</p> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, the only difference is that you are setting content type? Would it be preferable to be able to just specify the content-type?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @htuch - do you mean adding just
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| // | ||
| // The following html text will be created: | ||
| // | ||
| // .. code-block:: | ||
| // | ||
| // <p>upstream connect error:204:path=/foo</p> | ||
| // | ||
| string html_format = 4 [(validate.rules).string = {min_bytes: 1}]; | ||
| } | ||
|
|
||
| // If set to true, when command operators are evaluated to null, | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -149,6 +149,24 @@ TEST_F(LocalReplyTest, TestDefaultJsonFormatter) { | |
| EXPECT_TRUE(TestUtility::jsonStringEqual(body_, expected)); | ||
| } | ||
|
|
||
| TEST_F(LocalReplyTest, TestDefaultHtmlFormatter) { | ||
| // Default html formatter without any mappers | ||
| const std::string yaml = R"( | ||
| body_format: | ||
| html_format: "<h1>Sample Html Body</h1>" | ||
| )"; | ||
| TestUtility::loadFromYaml(yaml, config_); | ||
| auto local = Factory::create(config_, context_); | ||
|
|
||
| local->rewrite(nullptr, response_headers_, stream_info_, code_, body_, content_type_); | ||
| EXPECT_EQ(code_, TestInitCode); | ||
| EXPECT_EQ(stream_info_.response_code_, static_cast<uint32_t>(TestInitCode)); | ||
| EXPECT_EQ(response_headers_.Status()->value().getStringView(), | ||
| std::to_string(enumToInt(TestInitCode))); | ||
| EXPECT_EQ(body_, "<h1>Sample Html Body</h1>"); | ||
| EXPECT_EQ(content_type_, "text/html; charset=UTF-8"); | ||
| } | ||
|
|
||
| TEST_F(LocalReplyTest, TestMapperRewrite) { | ||
| // Match with response_code, and rewrite the code and body. | ||
| const std::string yaml = R"( | ||
|
|
@@ -336,5 +354,60 @@ TEST_F(LocalReplyTest, TestHeaderAddition) { | |
| ASSERT_EQ(out[1], "append-bar3"); | ||
| } | ||
|
|
||
| TEST_F(LocalReplyTest, TestMapperWithHtmlFormat) { | ||
| // Match with response_code, and rewrite the code and body. | ||
| const std::string yaml = R"( | ||
| mappers: | ||
| - filter: | ||
| status_code_filter: | ||
| comparison: | ||
| op: EQ | ||
| value: | ||
| default_value: 400 | ||
| runtime_key: key_b | ||
| status_code: 401 | ||
| body: | ||
| inline_string: "401 body text" | ||
| body_format_override: | ||
| html_format: "<h1>%LOCAL_REPLY_BODY%</h1>" | ||
| - filter: | ||
| status_code_filter: | ||
| comparison: | ||
| op: EQ | ||
| value: | ||
| default_value: 410 | ||
| runtime_key: key_b | ||
| status_code: 411 | ||
| body: | ||
| inline_string: "411 body text" | ||
| body_format: | ||
| html_format: "<h1>%LOCAL_REPLY_BODY%</h1> %RESPONSE_CODE% default formatter" | ||
| )"; | ||
| TestUtility::loadFromYaml(yaml, config_); | ||
| auto local = Factory::create(config_, context_); | ||
|
|
||
| // code=400 matches the first filter; rewrite code and body | ||
| // has its own formatter | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit end with . |
||
| resetData(400); | ||
| local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); | ||
| EXPECT_EQ(code_, static_cast<Http::Code>(401)); | ||
| EXPECT_EQ(stream_info_.response_code_, 401U); | ||
| EXPECT_EQ(response_headers_.Status()->value().getStringView(), "401"); | ||
| EXPECT_EQ(content_type_, "text/html; charset=UTF-8"); | ||
|
|
||
| const std::string expected = R"(<h1>401 body text</h1>)"; | ||
| EXPECT_EQ(body_, expected); | ||
|
|
||
| // code=410 matches the second filter; rewrite code and body | ||
|
dio marked this conversation as resolved.
|
||
| // but using default formatter | ||
| resetData(410); | ||
| local->rewrite(&request_headers_, response_headers_, stream_info_, code_, body_, content_type_); | ||
| EXPECT_EQ(code_, static_cast<Http::Code>(411)); | ||
| EXPECT_EQ(stream_info_.response_code_, 411U); | ||
| EXPECT_EQ(response_headers_.Status()->value().getStringView(), "411"); | ||
| EXPECT_EQ(body_, "<h1>411 body text</h1> 411 default formatter"); | ||
| EXPECT_EQ(content_type_, "text/html; charset=UTF-8"); | ||
| } | ||
|
|
||
| } // namespace LocalReply | ||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update these docs with a description of HTML format.