[http]: Fine grained path normalization controls implementation #15450

chaoqin-li1123 · 2021-03-11T23:56:31Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: Currently, envoy apply path normalization in http connection manager. When a path transformation is enabled, it is visible to all the filters and the upstream server. For fine grained control, we want some path normalization to be visible to filter but not the upstream server. The new hcm api(#15044) allow user to split the path transformation for filter use and for forwarding. This PR intends to implement fine grained path normalization control(#6589) based on the new api. This is a draft used to collect opinions early.
Additional Description:
Risk Level: high
Testing: by now only unit test
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

test/common/http/path_utility_test.cc

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

adisuissa

Thanks for working on this!
I've left a few comments.
I'm not sure if I missed them, but I did not see the tests for PathTransformer::transform.

source/common/http/path_utility.cc

source/common/http/path_utility.h

source/extensions/filters/network/http_connection_manager/config.h

source/common/http/path_utility.h

source/common/http/path_utility.cc

yanavlasov · 2021-03-12T04:12:46Z

I wonder if this should be moved into the HeaderMap API. I.e. the HeaderMap API would provide two methods. NormalizedPath would return path normalized for matching and the Path would return path for forwarding.

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 · 2021-03-12T05:42:02Z

I wonder if this should be moved into the HeaderMap API. I.e. the HeaderMap API would provide two methods. NormalizedPath would return path normalized for matching and the Path would return path for forwarding.

I think we want to configure it in http connection manager.

chaoqin-li1123 · 2021-03-12T05:43:32Z

I wonder if this should be moved into the HeaderMap API. I.e. the HeaderMap API would provide two methods. NormalizedPath would return path normalized for matching and the Path would return path for forwarding.

I see, I do plan to store both the path for filter use and path for forwarding in HeaderMap.

asraa

Thanks so much!!

source/common/http/path_utility.cc

source/common/http/path_utility.h

source/common/http/path_utility.cc

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

adisuissa

Thanks for working on this!
Added a few comments.

adisuissa · 2021-03-15T04:00:44Z

source/common/http/path_utility.cc

-    return;
+PathTransformer::PathTransformer(envoy::type::http::v3::PathTransformation path_transformation) {
+  const auto& operations = path_transformation.operations();
+  std::vector<uint64_t> operation_hashes;


Depending on the length, it might be better to use absl::flat_hash_set.

It might be overkill to use a hash set here since we expect a very small number of transformation.

source/common/http/path_utility.cc

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

source/common/http/path_utility.cc

test/common/http/path_utility_test.cc

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

…normaliztion

source/common/http/header_map_impl.h

dmitri-d · 2021-03-17T19:10:43Z

source/common/http/header_map_impl.h

@@ -480,7 +485,8 @@ class RequestHeaderMapImpl final : public TypedHeaderMapImpl<RequestHeaderMap>,
  };

  using HeaderHandles = ConstSingleton<HeaderHandleValues>;
-
+  std::string forwarding_path_;


Should the intended use for these two be documented?

Makes sense.

dmitri-d · 2021-03-17T19:14:42Z

source/common/http/path_utility.h

+
+  // Take a string_view as argument and return an optional string.
+  // The optional will be null if the transformation fail.
+  absl::optional<std::string> transform(const absl::string_view original_path) const;


Nit: could you mention that this will execute all configured transformations, or something along those lines?

Thanks, will add comments.

test/common/http/path_utility_test.cc

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

source/extensions/filters/network/http_connection_manager/config.h

include/envoy/http/header_map.h

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

…normaliztion

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

…normaliztion

yanavlasov · 2021-03-24T19:54:41Z

include/envoy/http/header_map.h

@@ -807,6 +814,14 @@ class RequestHeaderMap
 public:
  INLINE_REQ_STRING_HEADERS(DEFINE_INLINE_STRING_HEADER)
  INLINE_REQ_NUMERIC_HEADERS(DEFINE_INLINE_NUMERIC_HEADER)
+  virtual absl::string_view getForwardingPath() PURE;


Please add comments explaining what these methods are for.

Thanks, will fix.

source/common/http/conn_manager_utility.cc

source/extensions/filters/network/http_connection_manager/config.cc

source/extensions/filters/network/http_connection_manager/config.h

chaoqin-li1123 · 2021-03-25T19:23:33Z

cc @yangminzhu

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

…normaliztion

chaoqin-li1123 · 2021-03-27T15:32:45Z

/retest

repokitteh-read-only · 2021-03-27T15:32:49Z

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

🐱

Caused by: a #15450 (comment) was created by @chaoqin-li1123.

see: more, trace.

source/common/http/conn_manager_config.h

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

github-actions · 2021-05-09T04:01:22Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2021-05-16T04:09:34Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

chaoqin-li1123 · 2021-05-27T00:51:07Z

reopen

chaoqin-li1123 added 7 commits March 9, 2021 05:59

path transformer interface

d8e1b04

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

add normalization helper function

effbb99

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

fix format

579c5b1

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

add unit test

12cb83a

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

more unit tests

33e999f

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

transformer return optional string

d4330f3

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

build path transformer in hcm config

2de9ab6

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 requested review from alyssawilk and mattklein123 as code owners March 11, 2021 23:56

fix format

1540cdb

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

dmitri-d reviewed Mar 12, 2021

View reviewed changes

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

fix clang tidy and add test cases

b45e089

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

adisuissa reviewed Mar 12, 2021

View reviewed changes

yanavlasov self-assigned this Mar 12, 2021

avoid string copy

93d74e1

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

asraa reviewed Mar 12, 2021

View reviewed changes

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

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

source/common/http/path_utility.h Outdated Show resolved Hide resolved

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

chaoqin-li1123 added 4 commits March 12, 2021 15:55

refactor merge slash

7614247

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

reject duplicate transformation

0e08d96

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

reject duplicate transformation

0e6bca7

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

more refactor

e9bbff2

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

adisuissa reviewed Mar 15, 2021

View reviewed changes

test for path transformer

91aff85

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

asraa reviewed Mar 15, 2021

View reviewed changes

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

asraa reviewed Mar 15, 2021

View reviewed changes

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

chaoqin-li1123 added 3 commits March 15, 2021 16:42

store paths in header map, more test

044d820

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

add path restoration

996b786

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

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

d39afbe

…normaliztion

dmitri-d reviewed Mar 17, 2021

View reviewed changes

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

dmitri-d reviewed Mar 17, 2021

View reviewed changes

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

fix typo

c2810d9

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

asraa reviewed Mar 18, 2021

View reviewed changes

source/extensions/filters/network/http_connection_manager/config.h Outdated Show resolved Hide resolved

asraa reviewed Mar 18, 2021

View reviewed changes

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

chaoqin-li1123 added 5 commits March 18, 2021 16:44

change set and get filter path method to be private

3d37a0f

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

make set filter path and set forwarding path private

e24a7e7

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

add test for hcm config

50a227e

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

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

544755a

…normaliztion

add header integration tests

e9a483d

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

chaoqin-li1123 changed the title ~~[WIP]: Fine grained path normalization controls implementation~~ [http]: Fine grained path normalization controls implementation Mar 23, 2021

chaoqin-li1123 added 3 commits March 23, 2021 19:22

fix header integration tests

b40bebb

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

fix format

0d7f899

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

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

afb4b6d

…normaliztion

yanavlasov requested changes Mar 25, 2021

View reviewed changes

chaoqin-li1123 added 3 commits March 26, 2021 04:09

clean up code

ee8b9a3

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

format

4786f15

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

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

ba7f746

…normaliztion

yanavlasov requested changes Apr 6, 2021

View reviewed changes

source/common/http/conn_manager_config.h Outdated Show resolved Hide resolved

clean up

43b95f7

Signed-off-by: chaoqin-li1123 <chaoqinli@google.com>

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 9, 2021

github-actions bot closed this May 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[http]: Fine grained path normalization controls implementation #15450

[http]: Fine grained path normalization controls implementation #15450

chaoqin-li1123 commented Mar 11, 2021 •

edited

Loading

adisuissa left a comment

yanavlasov commented Mar 12, 2021

chaoqin-li1123 commented Mar 12, 2021

chaoqin-li1123 commented Mar 12, 2021

asraa left a comment

adisuissa left a comment

adisuissa Mar 15, 2021

chaoqin-li1123 Mar 15, 2021

dmitri-d Mar 17, 2021

chaoqin-li1123 Mar 18, 2021

dmitri-d Mar 17, 2021

chaoqin-li1123 Mar 18, 2021

yanavlasov Mar 24, 2021

chaoqin-li1123 Mar 27, 2021

chaoqin-li1123 commented Mar 25, 2021

chaoqin-li1123 commented Mar 27, 2021

repokitteh-read-only bot commented Mar 27, 2021

github-actions bot commented May 9, 2021

github-actions bot commented May 16, 2021

chaoqin-li1123 commented May 27, 2021

[http]: Fine grained path normalization controls implementation #15450

[http]: Fine grained path normalization controls implementation #15450

Conversation

chaoqin-li1123 commented Mar 11, 2021 • edited Loading

adisuissa left a comment

Choose a reason for hiding this comment

yanavlasov commented Mar 12, 2021

chaoqin-li1123 commented Mar 12, 2021

chaoqin-li1123 commented Mar 12, 2021

asraa left a comment

Choose a reason for hiding this comment

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

chaoqin-li1123 commented Mar 25, 2021

chaoqin-li1123 commented Mar 27, 2021

repokitteh-read-only bot commented Mar 27, 2021

github-actions bot commented May 9, 2021

github-actions bot commented May 16, 2021

chaoqin-li1123 commented May 27, 2021

chaoqin-li1123 commented Mar 11, 2021 •

edited

Loading