hcm: Add DependencyManager for http filterchain validation by aunu53 · Pull Request #15109 · envoyproxy/envoy

aunu53 · 2021-02-18T20:22:36Z

Commit Message:

Add DependencyManager class to handle http filterchain validation
This class currently only supports decode path validation, but encode path validation will be trivial to add afterwards
This class is directly scoped to the hcm extension and http filters, but it can be abstracted out when filterchain dependency is implemented for l4.

Risk Level: Low, this only adds a class and unit tests

See design doc attached to #14470 for more details.

Signed-off-by: Auni Ahsan <auni@google.com>

htuch · 2021-02-18T20:46:47Z

@htuch

Signed-off-by: Auni Ahsan <auni@google.com>

source/extensions/filters/network/http_connection_manager/BUILD

Signed-off-by: Auni Ahsan <auni@google.com>

htuch

Thanks, make sense and looks good, just a few nits.
/wait

source/extensions/filters/network/http_connection_manager/BUILD

htuch · 2021-02-19T21:57:18Z

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

+   * from the filter factory. Filters must be registered in decode path order.
+   */
+  void registerFilter(
+      std::string name,


Nit: absl::string_view name

Is there any memory access risk to having a std::map keyed by absl::string_view? Does this introduce lifetime requirements on the filter factory?

You probably want the map itself to have a string key, but use string_view here. This is why absl::flat_hash_map is great, as it has these polymorphic lookup methods.

I misspoke, this is a std::vector<absl::string_view,...> not a map. Any issue with that?

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

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

htuch · 2021-02-19T21:59:58Z

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

+
+bool DependencyManager::decodePathIsValid() {
+  auto cmp = [](Dependency a, Dependency b) { return a.name() != b.name(); };
+  std::set<Dependency, decltype(cmp)> satisfied(cmp);


Nit: prefer absl::flat_hash_set.

While I'd also prefer the absl library, it seems not to support the custom comparator API in the same way, and I'm going to avoid the hassle of figuring that out unless you feel strongly.

htuch · 2021-02-19T22:01:09Z

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

+
+bool DependencyManager::decodePathIsValid() {
+  auto cmp = [](Dependency a, Dependency b) { return a.name() != b.name(); };
+  std::set<Dependency, decltype(cmp)> satisfied(cmp);


Why not just make this a absl::flat_hash_set<std::string>?

Proto comparison doesn't work properly. In the future I'll probably switch this over to use message_differencer.h.

For both this and the above point, I don't think we need to store the full dependency in the set, but just its name?

I think I see what the issue is. You ideally are showing that the dependency matches the provider. You need to compare both name and also the DependencyType. However in this PR so far you only have name.

I suggest following the pattern of LocalityWeightsMap in eds.h for a pattern here around custom comparators, Absl, and proto keys.

I looked at LocalityWeightsMap and talked to Ashley about it, and I think got a simpler way to do this. Since it seems an intermediary type is unavoidable, I went with this:

using DependencyTuple = std::tuple<const std::string&, int>; absl::flat_hash_set<DependencyTuple> satisfied;

This isn't resilient to modifications to the Dependency proto but I don't really anticipate more fields being added so I think this works for now.

Signed-off-by: Auni Ahsan <auni@google.com>

htuch · 2021-02-24T02:08:55Z

@auni53 looks good, I think maybe if you can add support for dealing with dependency type matching that would complete this for the decode path. Thanks.
/wait

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53

OK, was able to incorporate absl flat hash set and simplified the comparator logic. I reordered/renamed the unit tests while adding functionality around dependency type checking.

aunu53 · 2021-02-24T19:04:34Z

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

+   * from the filter factory. Filters must be registered in decode path order.
+   */
+  void registerFilter(
+      std::string name,


I misspoke, this is a std::vector<absl::string_view,...> not a map. Any issue with that?

Signed-off-by: Auni Ahsan <auni@google.com>

htuch

LGTM modulo small nit. Thanks for the cleanups.
/wait

htuch · 2021-02-25T03:08:27Z

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

+  bool validDecodeDependencies();
+
+private:
+  std::vector<std::pair<absl::string_view,


For safety I'd make this an std::string. What I mentioned before was about parameters, the persistent structure should avoid holding a string view.

Yeah the polymorphism is not working here:

dependency_manager.h:26:19: error: no matching member function for call to 'push_back' filter_chain_.push_back({name, dependencies});

Probably I'd have to call whatever function copies the underlying string from absl::string_view, so I just changed it to const std::string& both in the function signature and the vector definition.

Signed-off-by: Auni Ahsan <auni@google.com>

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

Signed-off-by: Auni Ahsan <auni@google.com>

htuch

LGTM, thanks!

aunu53 · 2021-03-02T19:59:59Z

/retest

repokitteh-read-only · 2021-03-02T20:00:03Z

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

🐱

Caused by: a #15109 (comment) was created by @auni53.

see: more, trace.

Add dependency manager

dfa7b00

Signed-off-by: Auni Ahsan <auni@google.com>

Auni Ahsan added 5 commits February 18, 2021 21:48

Add dependency manager logic

d0a1d9b

Signed-off-by: Auni Ahsan <auni@google.com>

Remove log statement

8c8e247

Signed-off-by: Auni Ahsan <auni@google.com>

Fix format

e3bbb30

Signed-off-by: Auni Ahsan <auni@google.com>

Add a test, rename a var, remove ifdef

e545a8d

Signed-off-by: Auni Ahsan <auni@google.com>

Add class comments and specify decode-only

64f577c

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 changed the title ~~http connection manager: Add dependency manager~~ hcm: Add dependency manager class for filterchain validation Feb 19, 2021

aunu53 changed the title ~~hcm: Add dependency manager class for filterchain validation~~ hcm: Add DependencyManager for http filterchain validation Feb 19, 2021

Add TODOs

29029fb

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 commented Feb 19, 2021

View reviewed changes

source/extensions/filters/network/http_connection_manager/BUILD Outdated Show resolved Hide resolved

Auni Ahsan added 2 commits February 19, 2021 15:10

Fix format

1b0a21f

Signed-off-by: Auni Ahsan <auni@google.com>

Remove unused message differencer

21916fb

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 marked this pull request as ready for review February 19, 2021 15:22

aunu53 requested review from alyssawilk and mattklein123 as code owners February 19, 2021 15:22

Auni Ahsan added 4 commits February 19, 2021 18:43

Update comments

58f9a95

Signed-off-by: Auni Ahsan <auni@google.com>

Merge branch 'main' into manager

fb943ac

Signed-off-by: Auni Ahsan <auni@google.com>

Remove extraneous depe

ac04af8

Signed-off-by: Auni Ahsan <auni@google.com>

Satisfy clang tidy

ea89218

Signed-off-by: Auni Ahsan <auni@google.com>

junr03 assigned htuch Feb 19, 2021

htuch suggested changes Feb 19, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 19, 2021

Auni Ahsan added 4 commits February 22, 2021 16:05

Merge branch 'main' into manager

bd93e91

Signed-off-by: Auni Ahsan <auni@google.com>

Modify BUILD targets

0d56532

Signed-off-by: Auni Ahsan <auni@google.com>

Const, string view, rename function

ec4bd66

Signed-off-by: Auni Ahsan <auni@google.com>

Fix format

af5cc58

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only bot removed the waiting label Feb 23, 2021

Merge branch 'main' into manager

07b5c44

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only bot added the waiting label Feb 24, 2021

Auni Ahsan added 2 commits February 24, 2021 18:00

Merge branch 'main' into manager

b44cd36

Signed-off-by: Auni Ahsan <auni@google.com>

Add absl flat hash set

863114a

Signed-off-by: Auni Ahsan <auni@google.com>

aunu53 commented Feb 24, 2021

View reviewed changes

repokitteh-read-only bot removed the waiting label Feb 24, 2021

Remove unused using

5534eaa

Signed-off-by: Auni Ahsan <auni@google.com>

htuch suggested changes Feb 25, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 25, 2021

absl::string_view -> const std::string&

c21b6d7

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only bot removed the waiting label Feb 25, 2021

htuch reviewed Feb 26, 2021

View reviewed changes

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

htuch added the waiting label Feb 28, 2021

Auni Ahsan added 2 commits March 2, 2021 17:28

Merge branch 'main' into manager

86e8d3d

Signed-off-by: Auni Ahsan <auni@google.com>

Deconstify& internal reference to std::string

fe4dda1

Signed-off-by: Auni Ahsan <auni@google.com>

repokitteh-read-only bot removed the waiting label Mar 2, 2021

htuch approved these changes Mar 2, 2021

View reviewed changes

htuch merged commit 314f632 into envoyproxy:main Mar 2, 2021

aunu53 deleted the manager branch March 2, 2021 22:29

Conversation

aunu53 commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented Feb 18, 2021

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Feb 24, 2021

Uh oh!

aunu53 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

aunu53 commented Mar 2, 2021

Uh oh!

repokitteh-read-only bot commented Mar 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aunu53 commented Feb 18, 2021 •

edited

Loading