make custom inline headers bootstrap configurable #17330

wbpcode · 2021-07-14T05:11:54Z

Signed-off-by: wbpcode [email protected]

Commit Message: make custom inline headers bootstrap configurable
Additional Description:

Make custom inline headers bootstrap configurable. Check #17234 for more background info.

Risk Level: Mid.
Testing: Added.
Docs Changes: N/A.
Release Notes: Wait.

Signed-off-by: wbpcode <[email protected]>

repokitteh-read-only · 2021-07-14T05:12:00Z

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

🐱

Caused by: #17330 was opened by wbpcode.

see: more, trace.

Signed-off-by: wbpcode <[email protected]>

adisuissa

Left an API comment and some drive-by code comments.

adisuissa · 2021-07-14T13:57:17Z

source/server/server.cc

@@ -294,6 +295,46 @@ void loadBootstrap(absl::optional<uint32_t> bootstrap_version,
    throw EnvoyException(fmt::format("Unknown bootstrap version {}.", *bootstrap_version));
  }
 }
+
+bool canBeRegisteredAsInlineHeaderOrNot(const Http::LowerCaseString& header_name) {


nit: canBeRegisteredAsInlineHeader

adisuissa · 2021-07-14T13:57:51Z

source/server/server.cc

+void registerCustomInlineHeadersFromBootstrap(
+    const envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
+  for (const auto& inline_header : bootstrap.inline_headers()) {
+    Http::LowerCaseString lower_case_name(inline_header.inline_header_name());


adisuissa · 2021-07-14T19:13:53Z

test/server/server_test.cc

+    // instantiation of InstanceImpl. If InstanceImpl is instantiated again and the inline header is
+    // registered again, the assertion will be triggered.


Is there a test that covers this scenario? If not, can you please add one.

adisuissa · 2021-07-14T19:17:20Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+  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;


I think that the following PGV constraint (validate.rules).enum.defined_only = true can be added.

Signed-off-by: wbpcode <[email protected]>

…e_headers

adisuissa

/lgtm api

Signed-off-by: wbpcode <[email protected]>

wbpcode · 2021-07-18T00:45:16Z

/retest

repokitteh-read-only · 2021-07-18T00:45:20Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17330 (comment) was created by @wbpcode.

see: more, trace.

adisuissa

/lgtm api

wbpcode · 2021-07-20T02:49:19Z

@alyssawilk Now the API is ready. When it is convenient for you, can you advance this work? 😄

alyssawilk

Looks great! this will be a really useful feature going forward :-)

api/envoy/config/bootstrap/v3/bootstrap.proto

alyssawilk · 2021-07-20T13:52:34Z

source/server/server.cc

@@ -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) {


call out in API?

alyssawilk · 2021-07-20T13:53:39Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+  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}];


vadd pgv for well_known_regex: HTTP_HEADER_NAME?

alyssawilk · 2021-07-20T13:54:31Z

source/server/server.cc

+                                       inline_header.inline_header_name()));
+    }
+    switch (inline_header.inline_header_type()) {
+    case envoy::config::bootstrap::v3::CustomInlineHeader::REQUEST_HEADER:


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.

Can one add as a custom inline header a header which is already an inline header?

Yes.

I think it's worth a unit test for this.

I think this scenario has been verified in the test of CustomInlineHeaderRegistry::registerInlineHeader itself. We can register the header with the same name as the inline header repeatedly without any negative effects.

envoy/test/common/http/header_map_impl_test.cc

Lines 365 to 368 in d483098

Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>

custom_header_1(Http::LowerCaseString{"foo_custom_header"});

Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>

custom_header_1_copy(Http::LowerCaseString{"foo_custom_header"});

envoy/test/common/http/header_map_impl_test.cc

Lines 389 to 399 in d483098

// Make sure that the same header registered twice points to the same location.

TEST_P(HeaderMapImplTest, CustomRegisteredHeaders) {

TestRequestHeaderMapImpl headers;

EXPECT_EQ(custom_header_1.handle(), custom_header_1_copy.handle());

EXPECT_EQ(nullptr, headers.getInline(custom_header_1.handle()));

EXPECT_EQ(nullptr, headers.getInline(custom_header_1_copy.handle()));

headers.setInline(custom_header_1.handle(), 42);

EXPECT_EQ("42", headers.getInlineValue(custom_header_1_copy.handle()));

EXPECT_EQ("foo_custom_header",

headers.getInline(custom_header_1.handle())->key().getStringView());

}

alyssawilk · 2021-07-20T13:57:02Z

test/server/server_test.cc

+    is_registered = true;
+  }
+
+  EXPECT_TRUE(


how about an integration test (protocol integration test?) that the headers / trailers end up elided?

get it. I think we can verify this problem with a few simple lines of tests in existing test cases.

Where did we land on this? Don't see an integration test

Signed-off-by: wbpcode <[email protected]>

wbpcode · 2021-07-21T12:54:34Z

/retest

repokitteh-read-only · 2021-07-21T12:54:38Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17330 (comment) was created by @wbpcode.

see: more, trace.

alyssawilk

Looks great! Throwing over to @snowp for second pass

alyssawilk · 2021-07-22T13:00:50Z

Also @adisuissa can you do another API stamp?

adisuissa

/lgtm api

snowp

This LGTM, just one question re adding an integration test

wbpcode · 2021-07-24T00:30:35Z

@snowp Thanks.

@alyssawilk's suggestion is to verify that a header or trailer registered as an inline header can be folded correctly. In practice, integration tests are not necessarily required to verify this problem.
Instead of adding a whole new integration test to verify it, I did the same by adding some extra code to the test case.

https://github.com/wbpcode/envoy/blob/446214acbbce602b8b9a84ad1b0e342c65e47535/test/server/server_test.cc#L390-L427

{
    Http::TestRequestHeaderMapImpl headers{
        {"test1", "test1_value1"},
        {"test1", "test1_value2"},
        {"test3", "test3_value1"},
        {"test3", "test3_value2"},
    };

    // 'test1' is registered as the inline request header.
    auto test1_headers = headers.get(Http::LowerCaseString("test1"));
    EXPECT_EQ(1, test1_headers.size());
    EXPECT_EQ("test1_value1,test1_value2", headers.get_("test1"));

    // 'test3' is not registered as an inline request header.
    auto test3_headers = headers.get(Http::LowerCaseString("test3"));
    EXPECT_EQ(2, test3_headers.size());
    EXPECT_EQ("test3_value1", headers.get_("test3"));
  }

  {
    Http::TestResponseHeaderMapImpl headers{
        {"test1", "test1_value1"},
        {"test1", "test1_value2"},
        {"test3", "test3_value1"},
        {"test3", "test3_value2"},
    };

    // 'test1' is not registered as the inline response header.
    auto test1_headers = headers.get(Http::LowerCaseString("test1"));
    EXPECT_EQ(2, test1_headers.size());
    EXPECT_EQ("test1_value1", headers.get_("test1"));

    // 'test3' is registered as an inline response header.
    auto test3_headers = headers.get(Http::LowerCaseString("test3"));
    EXPECT_EQ(1, test3_headers.size());
    EXPECT_EQ("test3_value1,test3_value2", headers.get_("test3"));
  }
}

wbpcode · 2021-07-24T08:56:05Z

/retest

repokitteh-read-only · 2021-07-24T08:56:09Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17330 (comment) was created by @wbpcode.

see: more, trace.

wbpcode · 2021-07-24T13:45:16Z

/retest

repokitteh-read-only · 2021-07-24T13:45:20Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17330 (comment) was created by @wbpcode.

see: more, trace.

Signed-off-by: wbpcode <[email protected]>

snowp · 2021-07-27T15:33:30Z

I think normally we encourage adding an integration test even if it's not strictly necessary since they tend to catch unexpected issues (or provide more reliable regression tests). That said it seems like we're okay skipping it in this case, so let's get this merged

snowp

Thanks!

…bridge-stream * upstream/main: (140 commits) quiche: remove google quic support (envoyproxy#17465) runtime: removing envoy.reloadable_features.check_ocsp_policy (envoyproxy#17524) upstream: not trying to do HTTP/3 where not configured (envoyproxy#17454) api: Remove confusing line about auto-generation (envoyproxy#17536) v2: removing bootstrap (envoyproxy#17523) connpool: Fix crash in pool removal if the cluster was already deleted (envoyproxy#17522) Enhance the comments clearly (envoyproxy#17517) mysql proxy: connection attributes parsing (envoyproxy#17209) [ci] fix false positive CVE scan from node (envoyproxy#17510) Fixing Envoy Mobile factory strings (envoyproxy#17509) http3: validating codec (envoyproxy#17452) quic: add QUIC upstream stream reset error stats (envoyproxy#17496) thrift proxy: move UpstreamRequest into its own file (envoyproxy#17498) docs: Fixed FaultDelay docs. (envoyproxy#17495) updates links to jaegertracing-plugin.tar.gz (envoyproxy#17497) http: make custom inline headers bootstrap configurable (envoyproxy#17330) deps: update yaml-cpp to latest master (envoyproxy#17489) improving tracer coverage (envoyproxy#17493) Increase buffer size of `Win32RedirectRecords` (envoyproxy#17471) ext_proc: Fix problem with buffered body mode with empty or no body (envoyproxy#17430) ... Signed-off-by: Garrett Bourg <[email protected]>

Commit Message: ensure that the inline cookie header will be folded correctly Additional Description: This PR is a supplement to #17330 and #14969 and will finally close #17234. This PR mainly did following works: update insertByKey to choose suitable delimiter for inline header. update parseCookie to avoid unnecessary iteration for parsing cookie value. Risk Level: Low. Testing: Add. Docs Changes: N/A. Release Notes: N/A. Signed-off-by: wbpcode <[email protected]>

…7330) Signed-off-by: wbpcode <[email protected]>

…roxy#17560) Commit Message: ensure that the inline cookie header will be folded correctly Additional Description: This PR is a supplement to envoyproxy#17330 and envoyproxy#14969 and will finally close envoyproxy#17234. This PR mainly did following works: update insertByKey to choose suitable delimiter for inline header. update parseCookie to avoid unnecessary iteration for parsing cookie value. Risk Level: Low. Testing: Add. Docs Changes: N/A. Release Notes: N/A. Signed-off-by: wbpcode <[email protected]>

make custom inline headers bootstrap configurable

d7521c3

Signed-off-by: wbpcode <[email protected]>

repokitteh-read-only bot added the api label Jul 14, 2021

repokitteh-read-only bot assigned adisuissa Jul 14, 2021

minor update

bbbe590

Signed-off-by: wbpcode <[email protected]>

adisuissa reviewed Jul 14, 2021

View reviewed changes

wbpcode added 3 commits July 15, 2021 21:44

minor update

4fe85dc

Signed-off-by: wbpcode <[email protected]>

fix test

cb0c6ac

Signed-off-by: wbpcode <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into inlin…

5cc8281

…e_headers

adisuissa reviewed Jul 16, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 16, 2021

merged main and fix conflict

72ec313

Signed-off-by: wbpcode <[email protected]>

repokitteh-read-only bot added the api label Jul 16, 2021

add death tesst

56b0e82

Signed-off-by: wbpcode <[email protected]>

adisuissa reviewed Jul 19, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 19, 2021

alyssawilk self-assigned this Jul 20, 2021

alyssawilk reviewed Jul 20, 2021

View reviewed changes

alyssawilk added the waiting label Jul 20, 2021

add test & update comments & update docs

446214a

Signed-off-by: wbpcode <[email protected]>

repokitteh-read-only bot added api and removed waiting labels Jul 21, 2021

alyssawilk previously approved these changes Jul 21, 2021

View reviewed changes

alyssawilk assigned snowp Jul 21, 2021

adisuissa reviewed Jul 22, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jul 22, 2021

snowp reviewed Jul 23, 2021

View reviewed changes

wbpcode closed this Jul 24, 2021

wbpcode reopened this Jul 24, 2021

Kick CI

17686d9

Signed-off-by: wbpcode <[email protected]>

snowp dismissed alyssawilk’s stale review via 17686d9 July 26, 2021 19:44

wbpcode requested review from snowp and alyssawilk July 27, 2021 13:32

snowp approved these changes Jul 27, 2021

View reviewed changes

snowp merged commit 6ace04d into envoyproxy:main Jul 27, 2021

wbpcode mentioned this pull request Aug 2, 2021

ensure that the inline cookie header will be folded correctly #17560

Merged

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

http: make custom inline headers bootstrap configurable (envoyproxy#1…

82c0b66

…7330) Signed-off-by: wbpcode <[email protected]>

		// instantiation of InstanceImpl. If InstanceImpl is instantiated again and the inline header is
		// registered again, the assertion will be triggered.

	Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>
	custom_header_1(Http::LowerCaseString{"foo_custom_header"});
	Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>
	custom_header_1_copy(Http::LowerCaseString{"foo_custom_header"});

	// Make sure that the same header registered twice points to the same location.
	TEST_P(HeaderMapImplTest, CustomRegisteredHeaders) {
	TestRequestHeaderMapImpl headers;
	EXPECT_EQ(custom_header_1.handle(), custom_header_1_copy.handle());
	EXPECT_EQ(nullptr, headers.getInline(custom_header_1.handle()));
	EXPECT_EQ(nullptr, headers.getInline(custom_header_1_copy.handle()));
	headers.setInline(custom_header_1.handle(), 42);
	EXPECT_EQ("42", headers.getInlineValue(custom_header_1_copy.handle()));
	EXPECT_EQ("foo_custom_header",
	headers.getInline(custom_header_1.handle())->key().getStringView());
	}

make custom inline headers bootstrap configurable #17330

make custom inline headers bootstrap configurable #17330

Conversation

wbpcode commented Jul 14, 2021

repokitteh-read-only bot commented Jul 14, 2021

adisuissa 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

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

adisuissa left a comment

Choose a reason for hiding this comment

wbpcode commented Jul 18, 2021

repokitteh-read-only bot commented Jul 18, 2021

adisuissa left a comment

Choose a reason for hiding this comment

wbpcode commented Jul 20, 2021

alyssawilk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Jul 21, 2021 • 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

wbpcode commented Jul 21, 2021

repokitteh-read-only bot commented Jul 21, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk commented Jul 22, 2021

adisuissa left a comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

wbpcode commented Jul 24, 2021 • edited Loading

wbpcode commented Jul 24, 2021

repokitteh-read-only bot commented Jul 24, 2021

wbpcode commented Jul 24, 2021

repokitteh-read-only bot commented Jul 24, 2021

snowp commented Jul 27, 2021

snowp left a comment

Choose a reason for hiding this comment

wbpcode Jul 21, 2021 •

edited

Loading

wbpcode commented Jul 24, 2021 •

edited

Loading