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

Merged

alyssawilk merged 5 commits intoenvoyproxy:mainfrom

wbpcode:inline-cookie

Aug 10, 2021

Member

wbpcode commented Aug 2, 2021 •

edited

Loading

Signed-off-by: wbpcode wbphub@live.com

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.

wbpcode added 2 commits

August 2, 2021 13:55


          inline cookie delimiter

0da881f

Signed-off-by: wbpcode <wbphub@live.com>


          remove header include added by clangd

0b12394

Signed-off-by: wbpcode <wbphub@live.com>

Member Author

wbpcode commented Aug 3, 2021


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

56027f0

…e-cookie

alyssawilk self-assigned this

alyssawilk reviewed

View reviewed changes

Contributor

alyssawilk left a comment

Nice catch! sorry I didn't call this out in the last PR.
/wait

source/common/http/header_map_impl.h Outdated

                 void insertByKey(HeaderString&& key, HeaderString&& value);
                 static uint64_t appendToHeader(HeaderString& header, absl::string_view data,
                                                absl::string_view delimiter = ",");
+                static absl::string_view delimiterByHeader(const LowerCaseString& key,

Contributor

alyssawilk Aug 3, 2021

If this is not used outside of the cc file it can be a helper function in empty namespace

Member Author

wbpcode Aug 4, 2021

get it.

source/common/http/header_map_impl.cc Outdated

+                                                                 bool correctly_coalesce_cookies) {
+                // TODO(wbpcode): 'correctly_coalesce_cookies' feature is enabled by default and is allowed to be
+                // disabled via runtime. But I doubt if any user will actually disable it. The comma separator is
+                // obviously problematic for cookies. Maybe we can consider removing it.

Contributor

alyssawilk Aug 3, 2021

It'll be removed in a few months per the process documented in CONTRIBUTING.md so I think you can take this TODO out.

Member Author

wbpcode Aug 4, 2021

get it.

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

repokitteh-read-only bot added the waiting label


          move delimiterByHeader to anonymous namespace

58659ad

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only bot removed the waiting label

alyssawilk assigned mattklein123

mattklein123 requested changes

View reviewed changes

Member

mattklein123 left a comment

Thanks this LGTM at a high level with some small comments.

/wait

source/common/http/header_map_impl.cc Outdated

Comment on lines +51 to +53

+                  return "; ";
+                }
+                return ",";

Member

mattklein123 Aug 4, 2021

perf nit: I don't know if the compiler is smart enough to make the string_view ahead of time, so this might require a strlen each time. I would define constant string_view and then return them.

Member Author

wbpcode Aug 5, 2021

Get it.

source/common/http/header_map_impl.cc Outdated

                   if (*lookup.value().entry_ == nullptr) {
                     maybeCreateInline(lookup.value().entry_, *lookup.value().key_, std::move(value));
                   } else {
+                    auto delimiter =

Member

mattklein123 Aug 4, 2021

nit: const

Member Author

wbpcode Aug 5, 2021

get it.

source/common/http/header_map_impl.cc Outdated

-                  const uint64_t added_size = header_map_correctly_coalesce_cookies_
-                                                  ? appendToHeader(entry[0]->value(), value, delimiter)
-                                                  : appendToHeader(entry[0]->value(), value);
+                  auto delimiter = delimiterByHeader(key, header_map_correctly_coalesce_cookies_);

Member

mattklein123 Aug 4, 2021

nit: const

Member Author

wbpcode Aug 5, 2021

get it.

repokitteh-read-only bot added the waiting label


          minor update

Signed-off-by: wbpcode <wbphub@live.com>

repokitteh-read-only bot removed the waiting label

Member Author

wbpcode commented Aug 5, 2021

/retest

repokitteh-read-only bot commented Aug 5, 2021

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

🐱

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

see: more, trace.

mattklein123 approved these changes

View reviewed changes

Member

mattklein123 left a comment

Thanks!

Member

mattklein123 commented Aug 6, 2021

(Will defer to @alyssawilk to merge on Monday in case she has further comments.)

alyssawilk approved these changes

View reviewed changes

alyssawilk merged commit 6576212 into envoyproxy:main

mpuncel added a commit to mpuncel/envoy that referenced this pull request


          Merge branch 'main' into mpuncel/sds-hc-sequence

5b140de

* main: (687 commits)
  ci: set build debug information from env (envoyproxy#17635)
  ext_authz: do the authentication even the direct response is set (envoyproxy#17546)
  upstream: various cleanups in connection pool code (envoyproxy#17644)
  owners: promote Dmitry to maintainer (envoyproxy#17642)
  quiche: client session supports creating bidi stream (envoyproxy#17543)
  Update HTTP/2 METADATA documentation. (envoyproxy#17637)
  ext_proc: Check validity of the :status header (envoyproxy#17596)
  test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614)
  ensure that the inline cookie header will be folded correctly  (envoyproxy#17560)
  cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616)
  owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641)
  quiche: update QUICHE dependency (envoyproxy#17618)
  Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623)
  REPO_LAYOUT.md: fix outdated link (envoyproxy#17626)
  hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558)
  thrift proxy: add request shadowing support (envoyproxy#17544)
  ext_proc: Ensure that timer is always cancelled (envoyproxy#17569)
  Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362)
  proto: fix verify to point at v3 only (envoyproxy#17622)
  api: move generic matcher proto to its own package (envoyproxy#17096)
  ...

mpuncel added a commit to mpuncel/envoy that referenced this pull request


          Merge branch 'main' into mpuncel/sds-hc-sequence

dab7720

* main: (687 commits)
  ci: set build debug information from env (envoyproxy#17635)
  ext_authz: do the authentication even the direct response is set (envoyproxy#17546)
  upstream: various cleanups in connection pool code (envoyproxy#17644)
  owners: promote Dmitry to maintainer (envoyproxy#17642)
  quiche: client session supports creating bidi stream (envoyproxy#17543)
  Update HTTP/2 METADATA documentation. (envoyproxy#17637)
  ext_proc: Check validity of the :status header (envoyproxy#17596)
  test: add ASSERT indicating that gRPC stream has not been started yet (envoyproxy#17614)
  ensure that the inline cookie header will be folded correctly  (envoyproxy#17560)
  cluster_manager: Make ClusterEntry a class instead of a struct (envoyproxy#17616)
  owners: make Raúl a Thrift senior extension maintainer (envoyproxy#17641)
  quiche: update QUICHE dependency (envoyproxy#17618)
  Delete mock for removed RouteEntry::perFilterConfig() method (envoyproxy#17623)
  REPO_LAYOUT.md: fix outdated link (envoyproxy#17626)
  hcm: forbid use of detection extensions with use_remote_addr/xff_num_trusted_hops (envoyproxy#17558)
  thrift proxy: add request shadowing support (envoyproxy#17544)
  ext_proc: Ensure that timer is always cancelled (envoyproxy#17569)
  Proposal: Add CachePolicy interface to allow for custom cache behavior (envoyproxy#17362)
  proto: fix verify to point at v3 only (envoyproxy#17622)
  api: move generic matcher proto to its own package (envoyproxy#17096)
  ...

Signed-off-by: Michael Puncel <mpuncel@squareup.com>

theshubhamp mentioned this pull request

common: support parsing cookies into a map #17811

Merged

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request


          ensure that the inline cookie header will be folded correctly (envoyp…

9ad8d42

…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 <wbphub@live.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet