listener: add filter chain match support for direct source address by ggreenway · Pull Request #17118 · envoyproxy/envoy

ggreenway · 2021-06-24T00:12:22Z

Signed-off-by: Greg Greenway ggreenway@apple.com

Commit Message:
Additional Description:
Risk Level: Low
Testing: Added tests
Docs Changes: In protos
Release Notes: added
Platform Specific Features: none
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only · 2021-06-24T00:12:29Z

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

🐱

Caused by: #17118 was opened by ggreenway.

see: more, trace.

ggreenway · 2021-06-24T00:16:59Z

I believe Matt is out this week; un-assigning him, and adding a different api shepherd.

ggreenway · 2021-06-24T00:28:47Z

Assigning @htuch or @lizan for API-shepherd, and @alyssawilk to review the code (since her and I briefly discussed it on slack).

ggreenway · 2021-06-24T04:04:15Z

/retest

repokitteh-read-only · 2021-06-24T04:04:18Z

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

🐱

Caused by: a #17118 (comment) was created by @ggreenway.

see: more, trace.

lizan

/lgtm api

alyssawilk

Sorry for lag - I'll do a full pass Monday. @lambdai git blame tells me you did much of this code - if so would you be up for first pass?

alyssawilk · 2021-06-24T18:33:52Z

source/server/filter_chain_manager_impl.cc

+    addFilterChainForSourceTypes(direct_source_ips_map[EMPTY_STRING], source_type, source_ips,
+                                 source_ports, filter_chain);
+  } else {
+    for (const auto& direct_source_ip : direct_source_ips) {


Should we (or do we) have API docs somewhere that talk about the O(size) addition here?

I'm not sure. @lambdai wdyt?

alternately @htuch / @lizan can weigh in here as shephards

Is the question on how do we describe hte memory overheads of building up the nested tries for matching?

If so, I think maybe we should have some discussion on this in the top-level FilterChainMatch message comment.

I think it may be worth documenting, but probably outside the scope of this PR; this doesn't fundamentally change how it scales, and adds negligible additional memory usage when the new feature isn't being used.

alyssawilk

sorry about the delay - here's first pass comments :-)

alyssawilk · 2021-06-28T14:49:02Z

source/server/filter_chain_manager_impl.cc

+    const Network::ConnectionSocket& socket) const {
+  auto address = socket.addressProvider().directRemoteAddress();
+  if (address->type() != Network::Address::Type::Ip) {
+    address = fakeAddress();


To test my understanding, this is just a placeholder address so the matching code won't crash and will always fail the match (if a match is requested) right?
I know it's not your code but if I understand it right can you update the comments at fakeAddress to expand UDS -> unix domain socket and note it's just creating an address which should never match?

Also do we have a unit test for this?

Yes, your understanding is correct. Nearly all of this code is copy/pasted from the SourceIP matching. I'll add comments and make sure there's a test for this.

lambdai

LGTM. Only nits. Feel free to take it or leave it!

lambdai · 2021-06-28T17:18:47Z

source/server/filter_chain_manager_impl.cc


 void FilterChainManagerImpl::addFilterChainForSourceTypes(
-    SourceTypesArray& source_types_array,
+    SourceTypesArraySharedPtr& source_types_array_ptr,


super nit: previous it‘s caller get the arg ready.

lambdai · 2021-06-28T17:19:25Z

source/server/filter_chain_manager_impl.cc

-      const auto& cidr_range = Network::Address::CidrRange::create(destination_ip);
-      destination_ips.push_back(cidr_range.asString());
-    }
+    auto createAddressVector = [](const auto& prefix_ranges) -> std::vector<std::string> {


lambdai · 2021-06-28T17:32:50Z

source/server/filter_chain_manager_impl.cc

+
+  SourceTypesArray& source_types_array = *source_types_array_ptr;
  if (source_ips.empty()) {
    addFilterChainForSourceIPs(source_types_array[source_type].first, EMPTY_STRING, source_ports,


Take me a few time to realize this mututes the the upper entry so the allocated SourceTypesArray is not destroyed by end of this function.

This is literally correct.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

CI appears to be failing on api_listener_impl.cc, even though I didn't touch it, so I'm pulling out the bits that it needs from the code I did touch to isolate the changes. Signed-off-by: Greg Greenway <ggreenway@apple.com>

…direct-source

ggreenway · 2021-06-28T22:15:50Z

I merged main, but didn't make any further api changes; no need for re-review by api-shepherds.

alyssawilk · 2021-06-29T19:02:07Z

Are CI failures expected/flakes or should I wait until those are sorted to take a look?
/wait

This reverts commit 48e2d28. Signed-off-by: Greg Greenway <ggreenway@apple.com>

@sotiris

Thanks to @sotiris for helping me fix this. Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2021-06-30T18:53:47Z

clang_cl error this time was NOT the same issue. Failed to download a dep from github.com; spurious error I'm sure.

ggreenway · 2021-06-30T18:54:04Z

/retest

repokitteh-read-only · 2021-06-30T18:54: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 #17118 (comment) was created by @ggreenway.

see: more, trace.

…direct-source

ggreenway · 2021-06-30T20:59:38Z

@alyssawilk CI looks good now; please review.

alyssawilk

LGTM with the caveat I'm not convinced I'd catch errors this deep in loops (though the tests make me happier this is WAI) =P

alyssawilk · 2021-07-01T13:30:37Z

source/server/filter_chain_manager_impl.h

wow, that must have been "fun" to find :-P

Big thank you to @wrowe and @davinci26 for helping me figure out what was going wrong and how to fix it!

alyssawilk · 2021-07-01T13:35:03Z

source/server/filter_chain_manager_impl.cc

+    addFilterChainForSourceTypes(direct_source_ips_map[EMPTY_STRING], source_type, source_ips,
+                                 source_ports, filter_chain);
+  } else {
+    for (const auto& direct_source_ip : direct_source_ips) {


alternately @htuch / @lizan can weigh in here as shephards

* main: (51 commits) listener: add filter chain match support for direct source address (envoyproxy#17118) Increase common/common coverage (envoyproxy#17193) crash_dump: Added local_end_stream_ to crash dump for H2. (envoyproxy#17199) codeql: improve Ubuntu dependency installation (envoyproxy#16556) ci: Move tooling tests to tooling job (envoyproxy#17071) Fix issue with Windows container image (envoyproxy#17113) fix filter linking urls (envoyproxy#17185) bug fix: fix bug that check_format.py will check files which are ignored (envoyproxy#17195) tls: moving the server name into SocketAddressProvider (envoyproxy#16574) network: Use std::make_unique and std::make_shared in source/common/network instead of bare new() (envoyproxy#17177) Revert "alpha matching: support generic action factory context (envoyproxy#17025)" (envoyproxy#17191) ci: Dont clone filter example where not required (envoyproxy#17182) alpha matching: support generic action factory context (envoyproxy#17025) xds: Clarify comment for RouteMatch.case_sensitive field. (envoyproxy#17176) ci: Only publish the required docker image (envoyproxy#17080) coverage: fixing flake (envoyproxy#17190) api: add cluster_specifier_plugin to RouteAction (envoyproxy#16944) wasm: update V8 to v9.2.230.13. (envoyproxy#17183) wasm: update Proxy-Wasm C++ Host and SDK to latest (2021-06-24). (envoyproxy#17174) owners: add Piotr as senior extension maintainer (envoyproxy#17175) ... Signed-off-by: Garrett Bourg <bourg@squareup.com>

…nvoyproxy#17118) Signed-off-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

…nvoyproxy#17118) Signed-off-by: Greg Greenway <ggreenway@apple.com>

listener: add filter chain match support for direct source address

4b33a33

Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot added the api label Jun 24, 2021

repokitteh-read-only bot assigned mattklein123 Jun 24, 2021

ggreenway assigned lizan, htuch and alyssawilk and unassigned mattklein123 Jun 24, 2021

lizan reviewed Jun 24, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Jun 24, 2021

alyssawilk assigned lambdai Jun 24, 2021

alyssawilk reviewed Jun 24, 2021

View reviewed changes

alyssawilk reviewed Jun 28, 2021

View reviewed changes

lambdai previously approved these changes Jun 28, 2021

View reviewed changes

ggreenway added 2 commits June 28, 2021 10:42

more test coverage

0d34c1f

Signed-off-by: Greg Greenway <ggreenway@apple.com>

more detailed comment about fakeAddress()

9c046fb

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway dismissed lambdai’s stale review via 9c046fb June 28, 2021 17:42

ggreenway added 2 commits June 28, 2021 15:11

Merge remote-tracking branch 'upstream/main' into filter-chain-match-…

f7d1b0e

…direct-source

repokitteh-read-only bot added the api label Jun 28, 2021

repokitteh-read-only bot added the waiting label Jun 29, 2021

Revert "Trying to fix, or get more information out of, windows clang-cl"

25f92b6

This reverts commit 48e2d28. Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Jun 29, 2021

alyssawilk added the waiting label Jun 30, 2021

Fix windows clang_cl build hopefully.

ab6a3c2

Thanks to @sotiris for helping me fix this. Signed-off-by: Greg Greenway <ggreenway@apple.com>

repokitteh-read-only bot removed the waiting label Jun 30, 2021

Merge remote-tracking branch 'upstream/main' into filter-chain-match-…

279116e

…direct-source

alyssawilk approved these changes Jul 1, 2021

View reviewed changes

ggreenway merged commit 97eb8e5 into envoyproxy:main Jul 1, 2021

soulxu mentioned this pull request Jul 4, 2021

listener: reset the file event in framework instead of listener filter doing itself #17227

Merged

chrisxrepo pushed a commit to chrisxrepo/envoy that referenced this pull request Jul 8, 2021

listener: add filter chain match support for direct source address (e…

ef1cb10

…nvoyproxy#17118) Signed-off-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

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

listener: add filter chain match support for direct source address (e…

496a0a8

…nvoyproxy#17118) Signed-off-by: Greg Greenway <ggreenway@apple.com>

Conversation

ggreenway commented Jun 24, 2021

Uh oh!

repokitteh-read-only bot commented Jun 24, 2021

Uh oh!

ggreenway commented Jun 24, 2021

Uh oh!

ggreenway commented Jun 24, 2021

Uh oh!

ggreenway commented Jun 24, 2021

Uh oh!

repokitteh-read-only bot commented Jun 24, 2021

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

alyssawilk 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!

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!

alyssawilk 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!

lambdai 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!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Jun 28, 2021

Uh oh!

alyssawilk commented Jun 29, 2021

Uh oh!

ggreenway commented Jun 30, 2021

Uh oh!

ggreenway commented Jun 30, 2021

Uh oh!

repokitteh-read-only bot commented Jun 30, 2021

Uh oh!

ggreenway commented Jun 30, 2021

Uh oh!

alyssawilk 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants