quiche: handle multiple cookies and multiple set-cookie headers #14969

danzh2010 · 2021-02-06T05:29:10Z

Signed-off-by: Dan Zhang [email protected]

Commit Message: fix some issues around duplicated http headers:
Quiche doesn't coalesce duplicated headers, so we need to coalesce them in Envoy. Cookie headers after coalescing are semi-colon separated, while Set-Cookie headers shouldn't be coalesced.

Risk Level: low
Testing: added unit tests and integration tests.

Part of #2557

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-02-08T16:52:38Z

/retest

repokitteh-read-only · 2021-02-08T16:52:42Z

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

🐱

Caused by: a #14969 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-02-08T17:37:30Z

/assign @alyssawilk

antoniovicente

Thanks for the continued effort to add QUIC support to Envoy.

include/envoy/http/header_map.h

source/common/http/header_map_impl.cc

alyssawilk

Thanks for catching this one - cookie headers are easy to screw up!

include/envoy/http/header_map.h

source/common/http/header_map_impl.cc

alyssawilk · 2021-02-09T18:20:53Z

I think adding multiple entries is a fine behavior for the cookie header. Is there a reason you want to inline? My thought is we could fix appendToHeader to instead of having the default of ",", have a helper function which checked for cookie/set-cookie. Either that or we could fix appendCopy to make sure it did the checks, which would arguably be cheaper, and then ASSERT in appendToHeader that if it's cookie/set-cookie the correct delimiter applies.

…

On Tue, Feb 9, 2021 at 12:50 PM danzh ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/envoy/http/header_map.h <#14969 (comment)>: > @@ -294,7 +294,8 @@ class HeaderEntry { HEADER_FUNC(Protocol) \ HEADER_FUNC(Scheme) \ HEADER_FUNC(TE) \ - HEADER_FUNC(UserAgent) + HEADER_FUNC(UserAgent) \ + HEADER_FUNC(Cookie) There seems to be no set-cookie related changes in this PR. Is the set-cookie behavior already correct and just requires extra testing? set-cookie is already handled in Envoy HeaderMapImpl and is unit-tested and also tested in protocol integration tests. ------------------------------ In include/envoy/http/header_map.h <#14969 (comment)>: > @@ -294,7 +294,8 @@ class HeaderEntry { HEADER_FUNC(Protocol) \ HEADER_FUNC(Scheme) \ HEADER_FUNC(TE) \ - HEADER_FUNC(UserAgent) + HEADER_FUNC(UserAgent) \ + HEADER_FUNC(Cookie) I think this will result in Envoy inlining cookie headers, which is a pretty large change we should avoid if we can. According to the interface contract, HeaderMapImpl::insertByKey() only concatenate predefined inline headers. For other headers it will add multiple entries. If we don't prefer to inline cookie headers, the delimiter I added in insertByKey() won't work for cookie header. Maybe I should do what H2 does? Http2 codec coalesces cookie values into a class member variable via reconstituteCrumbledCookie() upon receiving each header and adds a "cookie" header at the end. Or change the contract to use concatenate for non-inlined headers as well. Which one is better? — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#14969 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AELALPIBBKWVXNZAPFLPTRTS6FYXHANCNFSM4XF5DU7A> .

danzh2010 · 2021-02-10T18:07:49Z

I think adding multiple entries is a fine behavior for the cookie header. Is there a reason you want to inline?

Multiple cookie headers are allowed according to the RFC, but H2 codec seems reconstitute them into a single header with semi-colon separated values. QUICHE is a bit different here. It splits all the cookie headers even if the client sends them in a single header. I want to coalesce them just because H2 does that.

alyssawilk · 2021-02-10T18:54:26Z

I don't mind us coalescing HTTP/3 if that keeps HTTP/2 and HTTP/3 closer, but my concern is I believe you're also changing HTTP/1, with no runtime guards or recourse which I don't think is a viable change (unless I'm wrong and something in http parser also coalesces cookie headers - you're welcome to test my assumptions here!)

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-02-10T21:44:45Z

I don't mind us coalescing HTTP/3 if that keeps HTTP/2 and HTTP/3 closer, but my concern is I believe you're also changing HTTP/1, with no runtime guards or recourse which I don't think is a viable change (unless I'm wrong and something in http parser also coalesces cookie headers - you're welcome to test my assumptions here!)

I added runtime guard to change in HeaderMapImpl. And also moved the change from insertByKey() to appendCopy(). PTAL!

alyssawilk

This looks much safer, thanks for the iteration!
It looks like the other 2 call sites for appendToHeader only apply to inline headers but I wouldn't mind a double check from @snowp
I also wonder if it's worth just making that argument non-optional and from those 2 call sites calling it for kDelimiterForInlineHeaders with a comment on definition that all inline headers use comma delimiter. Possibly overkill so I'll say that's 100% optional :-P

alyssawilk · 2021-02-10T22:04:23Z

source/common/runtime/runtime_features.cc

@@ -89,6 +89,7 @@ constexpr const char* runtime_features[] = {
    "envoy.reloadable_features.unify_grpc_handling",
    "envoy.reloadable_features.upstream_http2_flood_checks",
    "envoy.restart_features.use_apple_api_for_dns_lookups",
+    "envoy.reloadable_features.header_map_coalesce_cookie_headers",


I don't think this will result in cookies coalescing, but in correctly coalescing cookies. WDYT?

renamed it!

alyssawilk · 2021-02-10T22:05:13Z

source/extensions/quic_listeners/quiche/envoy_quic_utils.h

-    // TODO(danzh): Avoid copy by referencing entry as header_list is already validated by QUIC.
-    headers->addCopy(Http::LowerCaseString(entry.first), entry.second);
+    auto key = Http::LowerCaseString(entry.first);
+    if (!Runtime::runtimeFeatureEnabled(


FWIW I don't think you need to runtime guard QUIC code if youdon't want to as it's still alpha

good point! Removed the runtime guard here but still checks it in tests.

danzh2010 · 2021-02-11T05:03:11Z

/retest

repokitteh-read-only · 2021-02-11T05:03:14Z

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

🐱

Caused by: a #14969 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-02-11T22:07:38Z

This looks much safer, thanks for the iteration!
It looks like the other 2 call sites for appendToHeader only apply to inline headers but I wouldn't mind a double check from @snowp
I also wonder if it's worth just making that argument non-optional and from those 2 call sites calling it for kDelimiterForInlineHeaders with a comment on definition that all inline headers use comma delimiter. Possibly overkill so I'll say that's 100% optional :-P

It's hard to tell if the following interface are used anywhere with a delimiter different from comma:

#define DEFINE_INLINE_HEADER_STRING_FUNCS(name)      
  ...
  void append##name(absl::string_view data, absl::string_view delimiter)

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-02-15T21:14:52Z

/retest

repokitteh-read-only · 2021-02-15T21:14:56Z

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

🐱

Caused by: a #14969 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-02-16T16:41:40Z

Ping?

alyssawilk

code LGTM.

Let's add release notes in docs/root/version_history/current.rst
(bug fix, include the runtime guard)

and a header map impl test, and I think you're good to go.

snowp

It looks like the other 2 call sites for appendToHeader only apply to inline headers but I wouldn't mind a double check from @snowp

This seems right to me, can't find any use cases besides the ones applying to inlined headers.

test/common/http/header_map_impl_test.cc

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-02-18T21:07:37Z

code LGTM.

Let's add release notes in docs/root/version_history/current.rst
(bug fix, include the runtime guard)

and a header map impl test, and I think you're good to go.

Updated current.rst. There is a new header map unit test for the new feature. What else need to be tested?

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-02-22T06:48:04Z

/retest

repokitteh-read-only · 2021-02-22T06:48:07Z

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14969 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-02-22T18:38:09Z

/retest

repokitteh-read-only · 2021-02-22T18:38:13Z

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

🐱

Caused by: a #14969 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-02-22T22:37:34Z

/retest

repokitteh-read-only · 2021-02-22T22:37:39Z

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

🐱

Caused by: a #14969 (comment) was created by @danzh2010.

see: more, trace.

alyssawilk · 2021-02-23T14:26:56Z

Argh - CI passed but there's conflicts. Needs a main merge :-/

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 · 2021-02-25T18:39:48Z

Argh - CI passed but there's conflicts. Needs a main merge :-/

Done with main merge. PTAL

danzh2010 · 2021-02-25T21:35:40Z

/retest

repokitteh-read-only · 2021-02-25T21:35:44Z

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

🐱

Caused by: a #14969 (comment) was created by @danzh2010.

see: more, trace.

danzh2010 · 2021-03-01T17:23:49Z

sync'ed with upstream. PTAL!

snowp

Thanks, this LGTM.

Seems like this PR no longer contains any changes to how we handle set-cookie? Do we no longer need to make any changes to properly support that in with QUIC?

danzh2010 · 2021-03-02T16:21:52Z

Thanks, this LGTM.

Seems like this PR no longer contains any changes to how we handle set-cookie? Do we no longer need to make any changes to properly support that in with QUIC?

set-cookie was handled correctly before. This change add tests for it.

danzh2010 · 2021-03-02T18:06:44Z

Any other thoughts on this change? @alyssawilk

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]>

…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]>

cookie and set-cookie

53fbef6

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 requested review from alyssawilk, ggreenway and mattklein123 as code owners February 6, 2021 05:29

repokitteh-read-only bot assigned alyssawilk Feb 8, 2021

antoniovicente reviewed Feb 9, 2021

View reviewed changes

include/envoy/http/header_map.h Outdated Show resolved Hide resolved

source/common/http/header_map_impl.cc Outdated Show resolved Hide resolved

alyssawilk reviewed Feb 9, 2021

View reviewed changes

include/envoy/http/header_map.h Outdated Show resolved Hide resolved

source/common/http/header_map_impl.cc Outdated Show resolved Hide resolved

danzh1989 added 2 commits February 10, 2021 16:33

add flag

a80c27f

Signed-off-by: Dan Zhang <[email protected]>

flag protect e2e test

57e94af

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk assigned snowp Feb 10, 2021

alyssawilk reviewed Feb 10, 2021

View reviewed changes

rename runtime guard

f0d1aad

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk reviewed Feb 16, 2021

View reviewed changes

snowp reviewed Feb 16, 2021

View reviewed changes

test/common/http/header_map_impl_test.cc Show resolved Hide resolved

document the fix

e9349a1

Signed-off-by: Dan Zhang <[email protected]>

format

d1be519

Signed-off-by: Dan Zhang <[email protected]>

danzh1989 added 2 commits February 22, 2021 00:31

Merge branch 'master' into cookieheader

ed80087

Signed-off-by: Dan Zhang <[email protected]>

fix doc

b0a0281

Signed-off-by: Dan Zhang <[email protected]>

alyssawilk previously approved these changes Feb 22, 2021

View reviewed changes

Merge branch 'master' into cookieheader

ca245c9

Signed-off-by: Dan Zhang <[email protected]>

danzh2010 dismissed alyssawilk’s stale review via ca245c9 February 24, 2021 23:52

danzh1989 added 2 commits February 25, 2021 00:02

reorder bug fixes

d432876

Signed-off-by: Dan Zhang <[email protected]>

reorder again

6b34146

Signed-off-by: Dan Zhang <[email protected]>

snowp approved these changes Mar 2, 2021

View reviewed changes

alyssawilk merged commit 3cc0ca0 into envoyproxy:main Mar 2, 2021

wbpcode mentioned this pull request Aug 2, 2021

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

Merged

alyssawilk mentioned this pull request Oct 5, 2021

envoy.reloadable_features.header_map_correctly_coalesce_cookies deprecation #18450

Closed

quiche: handle multiple cookies and multiple set-cookie headers #14969

quiche: handle multiple cookies and multiple set-cookie headers #14969

Conversation

danzh2010 commented Feb 6, 2021 • edited Loading

danzh2010 commented Feb 8, 2021

repokitteh-read-only bot commented Feb 8, 2021

danzh2010 commented Feb 8, 2021

antoniovicente left a comment

Choose a reason for hiding this comment

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk commented Feb 9, 2021 via email

danzh2010 commented Feb 10, 2021

alyssawilk commented Feb 10, 2021

danzh2010 commented Feb 10, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk Feb 10, 2021

Choose a reason for hiding this comment

danzh2010 Feb 12, 2021

Choose a reason for hiding this comment

alyssawilk Feb 10, 2021

Choose a reason for hiding this comment

danzh2010 Feb 12, 2021

Choose a reason for hiding this comment

danzh2010 commented Feb 11, 2021

repokitteh-read-only bot commented Feb 11, 2021

danzh2010 commented Feb 11, 2021

danzh2010 commented Feb 15, 2021

repokitteh-read-only bot commented Feb 15, 2021

danzh2010 commented Feb 16, 2021

alyssawilk left a comment

Choose a reason for hiding this comment

snowp left a comment

Choose a reason for hiding this comment

danzh2010 commented Feb 18, 2021

danzh2010 commented Feb 22, 2021

repokitteh-read-only bot commented Feb 22, 2021

danzh2010 commented Feb 22, 2021

repokitteh-read-only bot commented Feb 22, 2021

danzh2010 commented Feb 22, 2021

repokitteh-read-only bot commented Feb 22, 2021

alyssawilk commented Feb 23, 2021

danzh2010 commented Feb 25, 2021

danzh2010 commented Feb 25, 2021

repokitteh-read-only bot commented Feb 25, 2021

danzh2010 commented Mar 1, 2021

snowp left a comment

Choose a reason for hiding this comment

danzh2010 commented Mar 2, 2021

danzh2010 commented Mar 2, 2021

danzh2010 commented Feb 6, 2021 •

edited

Loading