listener: filter chain selection based on destination IP/port. by PiotrSikora · Pull Request #3851 · envoyproxy/envoy

PiotrSikora · 2018-07-12T19:34:41Z

Risk Level: Medium
Testing: bazel test //test/...
Docs Changes: Minimal
Release Notes: Added

Signed-off-by: Piotr Sikora piotrsikora@google.com

*Risk Level*: Medium *Testing*: bazel test //test/... *Docs Changes*: Minimal *Release Notes*: Added Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2018-07-12T19:34:52Z

cc @ggreenway @rshriram

rshriram · 2018-07-12T19:42:56Z

source/server/listener_manager_impl.cc

+}
+
+void ListenerImpl::addFilterChainForDestinationPorts(
+    DestinationPortsMap& destination_ports_map, uint16_t destination_port,


filter_chains_ is converted to destination_ports_map ?

Not converted, filter_chains_ is of the DestinationPortsMap type.

is it possible to rename it for clarity? :).

rshriram · 2018-07-12T19:44:34Z

source/server/listener_manager_impl.cc

        filter_chain_match.application_protocols().end());

-    addFilterChain(server_names, filter_chain_match.transport_protocol(), application_protocols,
+    addFilterChain(filter_chain_match.destination_port().value(), destination_ips, server_names,


what if destination_port is missing?

It's 0 by default.

Omitting those fields in configuration didn't affect existing tests, and we have pretty good coverage there.

Other places in the codebase use PROTOBUF_GET_WRAPPED_OR_DEFAULT() for this behavior. I think this is a special case because the default for this type is 0, but it feels weird to me to get thing() when has_thing()== false.

rshriram

Looks good. Thanks for the PR.

ggreenway

This looks great overall. A few minor nits.

Also, I didn't see a test for clients connecting via UDS; please add one (or point me to it if I missed it).

ggreenway · 2018-07-12T20:00:57Z

source/server/listener_manager_impl.cc

+    std::vector<std::pair<ServerNamesMapSharedPtr, std::vector<Network::Address::CidrRange>>> list;
+    for (const auto& entry : destination_ips_map) {
+      std::vector<Network::Address::CidrRange> subnets;
+      if (entry.first == EMPTY_STRING) {


entry.first.empty() ?

Done, although I feel that the old check was a bit more correct, since we're checking if it's the map[EMPTY_STRING] entry.

Oh, right, I forgot that's what it is doing. In that case, I'd be ok with putting it back to how you had it.

ggreenway · 2018-07-12T20:02:39Z

source/server/listener_manager_impl.cc

+      }
    }
+    destination_ips_pair.second.reset(
+        new Network::LcTrie::LcTrie<ServerNamesMapSharedPtr>(list, true));


std::make_unique?

ggreenway · 2018-07-12T20:28:22Z

source/server/listener_manager_impl.cc

+
+  // Use invalid IP address (matching only filter chains without IP requirements) for UDS.
+  if (address->type() != Network::Address::Type::Ip) {
+    address = Network::Utility::parseInternetAddress("255.255.255.255");


Can we create a static object to use for this, instead of parsing and allocating for each connection?

ggreenway · 2018-07-12T20:35:42Z

source/server/listener_manager_impl.h

-      filter_chains_;
+  // Mapping of FilterChain's configured destination ports, IPs, server names, transport protocols
+  // and application protocols, using structures defined above.
+  DestinationPortsMap filter_chains_;


Maybe rename to destination_ports_map_? I both like the current name, and dislike it. From a high level, filter_chains_ feels like the right name, but when reading the functions that use it, it is hard to follow. So, I'm not sure what I'm suggesting with this comment.

As I commented above, the code would read better if we simply called this destination_ports_map (in the filter chain match section, which is the crux of this entire thing)

ggreenway · 2018-07-12T20:46:05Z

source/server/listener_manager_impl.cc

        filter_chain_match.application_protocols().end());

-    addFilterChain(server_names, filter_chain_match.transport_protocol(), application_protocols,
+    addFilterChain(filter_chain_match.destination_port().value(), destination_ips, server_names,


Other places in the codebase use PROTOBUF_GET_WRAPPED_OR_DEFAULT() for this behavior. I think this is a special case because the default for this type is 0, but it feels weird to me to get thing() when has_thing()== false.

ggreenway · 2018-07-12T20:47:53Z

source/server/listener_manager_impl.cc

+  }
+}
+
+void ListenerImpl::finishFilterChain() {


This function needs a more descriptive name. Maybe convert the comment above into the name:

// Convert DestinationIPsMap to DestinationIPsTrie for faster lookups.

Renamed to convertDestinationIPsMapToTrie().

I originally called it finishFilterChain(), since we'll need to convert more stuff in the future, if we're going to use LcTrie for source IPs, etc., but I guess we can rename it when needed.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2018-07-12T22:53:30Z

@ggreenway all nits addressed. I'll add test for UDS shortly (there isn't explicit one, but since findFilterChain() is exercised for all requests, other tests that were using UDS were failing because of no filter chain match).

This reverts commit 4cd5313. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora · 2018-07-12T23:43:13Z

@ggreenway tests for UDS clients added, they all fail (well, Envoy is crashing when calling LcTrie::getData()) if I comment out the fake address workaround.

ggreenway

This looks great. Thanks!

Edit: What do you mean by "fake address workaround"?

PiotrSikora · 2018-07-13T14:07:53Z

@ggreenway by "fake address workaround" I mean using 255.255.255.255 as the destination IP when client connected using UDS.

mattklein123 · 2018-07-13T15:57:22Z

@PiotrSikora can you merge master to see if it fixes the flakes?

…chain_destination

PiotrSikora · 2018-07-13T19:55:34Z

@mattklein123 yay, flakes fixed!

PiotrSikora · 2018-07-18T18:53:03Z

I'm still digging into this, but it seems that the overhead of LcTrie (for 0.0.0.0/0 and 0::/0) is ~16MB, which is the price we pay for each configured listener / filter chain.

I'd rather not revert this right away (but it's understandable if that's going to happen) and try to figure out whether:
a) I'm misusing LcTrie,
b) there is an obvious an bug in LcTrie,
c) we can migrate lookups to the "dumb" IP list, without reverting the whole change.

cc @rshriram @mattklein123 @ggreenway @ccaraman @brian-pane @htuch

brian-pane · 2018-07-18T19:01:48Z

If I remember correctly, there's still a step in the LcTrie construction code that pre-allocates a very large buffer to hold nodes (because it doesn't know the exact node count until it has finished constructing and level-compressing the trie) and then tries to realloc it down to just the needed size. That's a holdover from the reference implementation in the original LC Trie paper, which wasn't what I'd consider production-quality code.

If that's the problem, it's definitely fixable. I can set aside some time to work on it this weekend if nobody else gets to it first.

PiotrSikora · 2018-07-18T19:04:07Z

@brian-pane yeah, the leak appears to originate in LcTrieInternal::build(), perhaps realloc never frees the memory?

PiotrSikora · 2018-07-18T19:13:17Z

@brian-pane shrink_to_fit() is a one line fix for that (see: #3890), thanks for pointing me in the right direction!

listener: filter chain selection based on destination IP/port.

496da24

*Risk Level*: Medium *Testing*: bazel test //test/... *Docs Changes*: Minimal *Release Notes*: Added Signed-off-by: Piotr Sikora <piotrsikora@google.com>

rshriram reviewed Jul 12, 2018

View reviewed changes

ggreenway requested changes Jul 12, 2018

View reviewed changes

ggreenway self-assigned this Jul 12, 2018

PiotrSikora added 6 commits July 12, 2018 22:05

review: rename filter_chains_ to destination_ports_map_.

b2c8bcf

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use PROTOBUF_GET_WRAPPED_OR_DEFAULT().

d2744aa

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use std::make_unique.

e43be82

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: use static fake address.

e3666e0

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: replace EMPTY_STRING with empty() check.

4cd5313

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

review: rename finishFilterChains() to convertDestinationIPsMapToTrie().

dd76278

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

Revert "review: replace EMPTY_STRING with empty() check."

019a602

This reverts commit 4cd5313. Signed-off-by: Piotr Sikora <piotrsikora@google.com>

PiotrSikora force-pushed the filter_chain_destination branch from 567bb68 to 019a602 Compare July 12, 2018 22:59

review: add tests for UDS clients.

1fbd85d

Signed-off-by: Piotr Sikora <piotrsikora@google.com>

ggreenway approved these changes Jul 12, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into PiotrSikora/filter_…

f8b2ea6

…chain_destination

mattklein123 approved these changes Jul 13, 2018

View reviewed changes

mattklein123 merged commit 01d2e16 into envoyproxy:master Jul 13, 2018

ramaraochavali mentioned this pull request Jul 20, 2018

Error Creating Listeners on Machines that does not support IPv6 Family #3911

Closed

Conversation

PiotrSikora commented Jul 12, 2018

Uh oh!

PiotrSikora commented Jul 12, 2018

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!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

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

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!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented Jul 12, 2018

Uh oh!

PiotrSikora commented Jul 12, 2018

Uh oh!

ggreenway left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PiotrSikora commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Jul 13, 2018

Uh oh!

PiotrSikora commented Jul 13, 2018

Uh oh!

PiotrSikora commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brian-pane commented Jul 18, 2018

Uh oh!

PiotrSikora commented Jul 18, 2018

Uh oh!

PiotrSikora commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ggreenway left a comment •

edited

Loading

PiotrSikora commented Jul 13, 2018 •

edited

Loading

PiotrSikora commented Jul 18, 2018 •

edited

Loading

PiotrSikora commented Jul 18, 2018 •

edited

Loading