-
Notifications
You must be signed in to change notification settings - Fork 5.5k
make custom inline headers bootstrap configurable #17330
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 7 commits
d7521c3
bbbe590
4fe85dc
cb0c6ac
5cc8281
72ec313
56b0e82
446214a
17686d9
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 |
|---|---|---|
|
|
@@ -40,7 +40,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; | |
| // <config_overview_bootstrap>` for more detail. | ||
|
|
||
| // Bootstrap :ref:`configuration overview <config_overview_bootstrap>`. | ||
| // [#next-free-field: 32] | ||
| // [#next-free-field: 33] | ||
| message Bootstrap { | ||
| option (udpa.annotations.versioning).previous_message_type = | ||
| "envoy.config.bootstrap.v2.Bootstrap"; | ||
|
|
@@ -322,6 +322,11 @@ message Bootstrap { | |
| // field. | ||
| // [#not-implemented-hide:] | ||
| map<string, core.v3.TypedExtensionConfig> certificate_provider_instances = 25; | ||
|
|
||
| // Specifies a set of headers that need to be registered as inline header. This configuration | ||
| // allows users to customize the inline headers on-demand at Envoy startup without modifying | ||
| // Envoy's source code. | ||
| repeated CustomInlineHeader inline_headers = 32; | ||
| } | ||
|
|
||
| // Administration interface :ref:`operations documentation | ||
|
|
@@ -595,3 +600,22 @@ message LayeredRuntime { | |
| // such that later layers in the list overlay earlier entries. | ||
| repeated RuntimeLayer layers = 1; | ||
| } | ||
|
|
||
| // Used to specify the header that needs to be registered as an inline header. | ||
| // Inline headers provide O(1) search performance, but each inline header imposes | ||
| // an additional memory overhead on all instances of the corresponding type of | ||
| // HeaderMap or TrailerMap. | ||
| message CustomInlineHeader { | ||
| enum InlineHeaderType { | ||
| REQUEST_HEADER = 0; | ||
| REQUEST_TRAILER = 1; | ||
| RESPONSE_HEADER = 2; | ||
| RESPONSE_TRAILER = 3; | ||
| } | ||
|
|
||
| // The name of the header that is expected to be set as the inline header. | ||
| string inline_header_name = 1 [(validate.rules).string = {min_len: 1}]; | ||
|
|
||
| // The type of the header that is expected to be set as the inline header. | ||
| InlineHeaderType inline_header_type = 2 [(validate.rules).enum = {defined_only: true}]; | ||
|
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. vadd pgv for well_known_regex: HTTP_HEADER_NAME?
Member
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. get it. |
||
| } | ||
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 | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -33,6 +33,7 @@ | |||||||||||||||||||||||||||||||
| #include "source/common/config/version_converter.h" | ||||||||||||||||||||||||||||||||
| #include "source/common/config/xds_resource.h" | ||||||||||||||||||||||||||||||||
| #include "source/common/http/codes.h" | ||||||||||||||||||||||||||||||||
| #include "source/common/http/headers.h" | ||||||||||||||||||||||||||||||||
| #include "source/common/local_info/local_info_impl.h" | ||||||||||||||||||||||||||||||||
| #include "source/common/memory/stats.h" | ||||||||||||||||||||||||||||||||
| #include "source/common/network/address_impl.h" | ||||||||||||||||||||||||||||||||
|
|
@@ -292,6 +293,46 @@ void loadBootstrap(absl::optional<uint32_t> bootstrap_version, | |||||||||||||||||||||||||||||||
| throw EnvoyException(fmt::format("Unknown bootstrap version {}.", *bootstrap_version)); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| bool canBeRegisteredAsInlineHeader(const Http::LowerCaseString& header_name) { | ||||||||||||||||||||||||||||||||
|
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. call out in API?
Member
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. get it. |
||||||||||||||||||||||||||||||||
| // 'set-cookie' cannot currently be registered as an inline header. | ||||||||||||||||||||||||||||||||
| if (header_name == Http::Headers::get().SetCookie) { | ||||||||||||||||||||||||||||||||
| return false; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return true; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| void registerCustomInlineHeadersFromBootstrap( | ||||||||||||||||||||||||||||||||
| const envoy::config::bootstrap::v3::Bootstrap& bootstrap) { | ||||||||||||||||||||||||||||||||
| for (const auto& inline_header : bootstrap.inline_headers()) { | ||||||||||||||||||||||||||||||||
| const Http::LowerCaseString lower_case_name(inline_header.inline_header_name()); | ||||||||||||||||||||||||||||||||
| if (!canBeRegisteredAsInlineHeader(lower_case_name)) { | ||||||||||||||||||||||||||||||||
| throw EnvoyException(fmt::format("Header {} cannot be registered as an inline header.", | ||||||||||||||||||||||||||||||||
| inline_header.inline_header_name())); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| switch (inline_header.inline_header_type()) { | ||||||||||||||||||||||||||||||||
| case envoy::config::bootstrap::v3::CustomInlineHeader::REQUEST_HEADER: | ||||||||||||||||||||||||||||||||
|
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. Can one add as a custom inline header a header which is already an inline header? I think it's worth a unit test for this.
Member
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.
Yes.
I think this scenario has been verified in the test of envoy/test/common/http/header_map_impl_test.cc Lines 365 to 368 in d483098
envoy/test/common/http/header_map_impl_test.cc Lines 389 to 399 in d483098
|
||||||||||||||||||||||||||||||||
| Http::CustomInlineHeaderRegistry::registerInlineHeader< | ||||||||||||||||||||||||||||||||
| Http::RequestHeaderMap::header_map_type>(lower_case_name); | ||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||
| case envoy::config::bootstrap::v3::CustomInlineHeader::REQUEST_TRAILER: | ||||||||||||||||||||||||||||||||
| Http::CustomInlineHeaderRegistry::registerInlineHeader< | ||||||||||||||||||||||||||||||||
| Http::RequestTrailerMap::header_map_type>(lower_case_name); | ||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||
| case envoy::config::bootstrap::v3::CustomInlineHeader::RESPONSE_HEADER: | ||||||||||||||||||||||||||||||||
| Http::CustomInlineHeaderRegistry::registerInlineHeader< | ||||||||||||||||||||||||||||||||
| Http::ResponseHeaderMap::header_map_type>(lower_case_name); | ||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||
| case envoy::config::bootstrap::v3::CustomInlineHeader::RESPONSE_TRAILER: | ||||||||||||||||||||||||||||||||
| Http::CustomInlineHeaderRegistry::registerInlineHeader< | ||||||||||||||||||||||||||||||||
| Http::ResponseTrailerMap::header_map_type>(lower_case_name); | ||||||||||||||||||||||||||||||||
| break; | ||||||||||||||||||||||||||||||||
| default: | ||||||||||||||||||||||||||||||||
| NOT_IMPLEMENTED_GCOVR_EXCL_LINE; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| } // namespace | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| void InstanceUtil::loadBootstrapConfig(envoy::config::bootstrap::v3::Bootstrap& bootstrap, | ||||||||||||||||||||||||||||||||
|
|
@@ -353,8 +394,10 @@ void InstanceImpl::initialize(const Options& options, | |||||||||||||||||||||||||||||||
| // setPrefix has a release assert verifying that setPrefix() is not called after prefix() | ||||||||||||||||||||||||||||||||
| ThreadSafeSingleton<Http::PrefixValue>::get().setPrefix(bootstrap_.header_prefix().c_str()); | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| // TODO(mattklein123): Custom O(1) headers can be registered at this point for creating/finalizing | ||||||||||||||||||||||||||||||||
| // any header maps. | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Register Custom O(1) headers from bootstrap. | ||||||||||||||||||||||||||||||||
| registerCustomInlineHeadersFromBootstrap(bootstrap_); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| ENVOY_LOG(info, "HTTP header map info:"); | ||||||||||||||||||||||||||||||||
| for (const auto& info : Http::HeaderMapImplUtility::getAllHeaderMapImplInfo()) { | ||||||||||||||||||||||||||||||||
| ENVOY_LOG(info, " {}: {} bytes: {}", info.name_, info.size_, | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -334,6 +334,60 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, ServerInstanceImplTest, | |
| testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), | ||
| TestUtility::ipTestParamsToString); | ||
|
|
||
| // The finalize() of Http::CustomInlineHeaderRegistry will be called When InstanceImpl is | ||
| // instantiated. For this reason, the test case for inline headers must precede all other test | ||
| // cases. | ||
|
|
||
| // Test the case where inline headers contain names that cannot be registered as inline headers. | ||
| TEST_P(ServerInstanceImplTest, WithErrorCustomInlineHeaders) { | ||
| EXPECT_THROW_WITH_MESSAGE( | ||
| initialize("test/server/test_data/server/bootstrap_inline_headers_error.yaml"), | ||
| EnvoyException, "Header set-cookie cannot be registered as an inline header."); | ||
| } | ||
|
|
||
| TEST_P(ServerInstanceImplTest, WithDeathCustomInlineHeaders) { | ||
| #if !defined(NDEBUG) | ||
| // The finalize() of Http::CustomInlineHeaderRegistry will be called after the first successful | ||
| // instantiation of InstanceImpl. If InstanceImpl is instantiated again and the inline header is | ||
| // registered again, the assertion will be triggered. | ||
| EXPECT_DEATH( | ||
| { | ||
| initialize("test/server/test_data/server/bootstrap_inline_headers.yaml"); | ||
| initialize("test/server/test_data/server/bootstrap_inline_headers.yaml"); | ||
| }, | ||
| ""); | ||
| #endif // !defined(NDEBUG) | ||
| } | ||
|
|
||
| // Test whether the custom inline headers can be registered correctly. | ||
| TEST_P(ServerInstanceImplTest, WithCustomInlineHeaders) { | ||
| static bool is_registered = false; | ||
|
|
||
| if (!is_registered) { | ||
| // Avoid repeated registration of custom inline headers in the current process after the | ||
| // finalize() of Http::CustomInlineHeaderRegistry has already been called. | ||
| EXPECT_NO_THROW(initialize("test/server/test_data/server/bootstrap_inline_headers.yaml")); | ||
| is_registered = true; | ||
| } | ||
|
|
||
| EXPECT_TRUE( | ||
|
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. how about an integration test (protocol integration test?) that the headers / trailers end up elided?
Member
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. get it. I think we can verify this problem with a few simple lines of tests in existing test cases.
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. Where did we land on this? Don't see an integration test |
||
| Http::CustomInlineHeaderRegistry::getInlineHeader<Http::RequestHeaderMap::header_map_type>( | ||
| Http::LowerCaseString("test1")) | ||
| .has_value()); | ||
| EXPECT_TRUE( | ||
| Http::CustomInlineHeaderRegistry::getInlineHeader<Http::RequestTrailerMap::header_map_type>( | ||
| Http::LowerCaseString("test2")) | ||
| .has_value()); | ||
| EXPECT_TRUE( | ||
| Http::CustomInlineHeaderRegistry::getInlineHeader<Http::ResponseHeaderMap::header_map_type>( | ||
| Http::LowerCaseString("test3")) | ||
| .has_value()); | ||
| EXPECT_TRUE( | ||
| Http::CustomInlineHeaderRegistry::getInlineHeader<Http::ResponseTrailerMap::header_map_type>( | ||
| Http::LowerCaseString("test4")) | ||
| .has_value()); | ||
| } | ||
|
|
||
| // Validates that server stats are flushed even when server is stuck with initialization. | ||
| TEST_P(ServerInstanceImplTest, StatsFlushWhenServerIsStillInitializing) { | ||
| CustomStatsSinkFactory factory; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| inline_headers: | ||
| - inline_header_name: test1 | ||
| inline_header_type: REQUEST_HEADER | ||
| - inline_header_name: test2 | ||
| inline_header_type: REQUEST_TRAILER | ||
| - inline_header_name: test3 | ||
| inline_header_type: RESPONSE_HEADER | ||
| - inline_header_name: test4 | ||
| inline_header_type: RESPONSE_TRAILER |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| inline_headers: | ||
| - inline_header_name: set-cookie | ||
| inline_header_type: RESPONSE_HEADER |
Uh oh!
There was an error while loading. Please reload this page.