IP Matcher on a list of CIDR ranges by aguinet · Pull Request #16592 · envoyproxy/envoy

aguinet · 2021-05-20T08:10:09Z

Commit Message: Input matcher that checks that an IP{v4,v6} belongs to a list of CIDR ranges
Additional Description:

This input matcher takes an IPv4 or v6 address as input and returns true if it belongs to a list of given CIDR ranges. It uses the existing LcTrie facility.

See issue #16568

Risk Level: Medium
Testing: Unit tests for the extension are added
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

repokitteh-read-only · 2021-05-20T08:10:17Z

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

🐱

Caused by: #16592 was opened by aguinet.

see: more, trace.

aguinet · 2021-05-20T08:18:47Z

I have a question about https://github.com/envoyproxy/envoy/pull/16592/files#diff-2b3a93adfb5a23333aaf5113d8a99f142a8187735140781c49aeacd8c6c03795R23 : according to

envoy/source/common/network/cidr_range.h

Lines 76 to 84 in d304a2f

    
             /** 
        
              * TODO(ccaraman): Update CidrRange::create to support only constructing valid ranges. 
        
              * @return a CidrRange instance with the specified address and length, modified so that the only 
        
              *         bits that might be non-zero are in the high-order length bits, and so that length is 
        
              *         in the appropriate range (0 to 32 for IPv4, 0 to 128 for IPv6). If the address or 
        
              *         length is invalid, then the range will be invalid (i.e. length == -1). 
        
              */ 
        
             static CidrRange create(InstanceConstSharedPtr address, int length); 
        
             static CidrRange create(const std::string& address, int length);

, CidrRange::create can return an invalid range, hence this check.

The "problem" is that I didn't manage to create such an invalid range. Indeed, if a invalid IP address prefix is given, Utility::parseInternetAddress throws an exception before CidrRange::create returns. If we give a too big prefix value, it is truncated by truncateIpAddressAndLength, and thus a valid range is returned. Finally, looking at

envoy/source/common/network/cidr_range.cc

Lines 104 to 114 in d304a2f

    
           // static 
        
           CidrRange CidrRange::create(InstanceConstSharedPtr address, int length) { 
        
             InstanceConstSharedPtr ptr = truncateIpAddressAndLength(std::move(address), &length); 
        
             return CidrRange(std::move(ptr), length); 
        
           } 
        
           // static 
        
           CidrRange CidrRange::create(const std::string& address, int length) { 
        
             return create(Utility::parseInternetAddress(address), length); 
        
           }

and truncateIpAddressAndLength, I don't see how an invalid range could be returned, because parseInternetAddress will throw if it doesn't return an ipv4/ipv6 address (according to

envoy/source/common/network/utility.cc

Lines 129 to 155 in d304a2f

    
           Address::InstanceConstSharedPtr Utility::parseInternetAddressNoThrow(const std::string& ip_address, 
        
                                                                                uint16_t port, bool v6only) { 
        
             sockaddr_in sa4; 
        
             if (inet_pton(AF_INET, ip_address.c_str(), &sa4.sin_addr) == 1) { 
        
               sa4.sin_family = AF_INET; 
        
               sa4.sin_port = htons(port); 
        
               return std::make_shared<Address::Ipv4Instance>(&sa4); 
        
             } 
        
             sockaddr_in6 sa6; 
        
             memset(&sa6, 0, sizeof(sa6)); 
        
             if (inet_pton(AF_INET6, ip_address.c_str(), &sa6.sin6_addr) == 1) { 
        
               sa6.sin6_family = AF_INET6; 
        
               sa6.sin6_port = htons(port); 
        
               return std::make_shared<Address::Ipv6Instance>(sa6, v6only); 
        
             } 
        
             return nullptr; 
        
           } 
        
           Address::InstanceConstSharedPtr Utility::parseInternetAddress(const std::string& ip_address, 
        
                                                                         uint16_t port, bool v6only) { 
        
             const Address::InstanceConstSharedPtr address = 
        
                 parseInternetAddressNoThrow(ip_address, port, v6only); 
        
             if (address == nullptr) { 
        
               throwWithMalformedIp(ip_address); 
        
             } 
        
             return address; 
        
           }

).

Am I missing a corner case?

Thanks!

adisuissa

API review - overall looks good, couple of small comments.

adisuissa · 2021-05-26T03:32:05Z

api/envoy/extensions/matching/input_matchers/ip/v3/ip.proto

+// the paper `IP-address lookup using LC-tries
+// <https://www.nada.kth.se/~snilsson/publications/IP-address-lookup-using-LC-tries/>`_
+// by S. Nilsson and G. Karlsson.
+message IP {


Naming convention suggests this should be Ip.

Fixed in 16c5477

adisuissa · 2021-05-26T03:32:39Z

api/envoy/extensions/matching/input_matchers/ip/v3/ip.proto

+// <https://www.nada.kth.se/~snilsson/publications/IP-address-lookup-using-LC-tries/>`_
+// by S. Nilsson and G. Karlsson.
+message IP {
+  // Match if the IP belongs to any of these CIDR ranges


nit: end comments with a period.

Fixed in 16c5477 . Thanks for the review!

@adisuissa

Review by @adisuissa Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

adisuissa

API looks good to me, and it is ready for the next round of reviews.
Please take a look at CI, as it seems legit.

api/envoy/extensions/matching/input_matchers/ip/v3/ip.proto

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-06-02T16:58:29Z

There's something I'm not sure I should fix in the CI: https://dev.azure.com/cncf/envoy/_build/results?buildId=77178&view=logs&j=91e633a5-4907-5da1-5862-92a79fe2387a&t=f29e24e7-a093-5724-80bd-d79975d268cb&l=295 tells me that I shouldn't add envoy.matching.input_matchers.ip to source/extensions/extensions_build_config.bzl, but I added it here: https://github.com/envoyproxy/envoy/pull/16592/files#diff-5294c859ad138c7624de53e0f9566c0b630f5a9522ff357d23b2f0b17d6e3991R55

Is there an error message I am missing? cc @adisuissa

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

snowp

Thanks for adding this! Looks pretty good overall, just a few comments

api/envoy/extensions/matching/input_matchers/ip/v3/ip.proto

source/extensions/matching/input_matchers/ip/config.h

source/extensions/matching/input_matchers/ip/matcher.h

source/extensions/matching/input_matchers/ip/matcher.cc

test/extensions/matching/input_matchers/ip/matcher_test.cc

snowp · 2021-06-03T12:36:41Z

There's something I'm not sure I should fix in the CI: https://dev.azure.com/cncf/envoy/_build/results?buildId=77178&view=logs&j=91e633a5-4907-5da1-5862-92a79fe2387a&t=f29e24e7-a093-5724-80bd-d79975d268cb&l=295 tells me that I shouldn't add envoy.matching.input_matchers.ip to source/extensions/extensions_build_config.bzl, but I added it here: https://github.com/envoyproxy/envoy/pull/16592/files#diff-5294c859ad138c7624de53e0f9566c0b630f5a9522ff357d23b2f0b17d6e3991R55

Is there an error message I am missing? cc @adisuissa

cc @phlax

phlax · 2021-06-03T12:41:55Z

hi @aguinet - we can probs improve the error messaging here

the problem is that the extension needs to be added to source/extensions/extensions_metadata.yaml

the error is here https://dev.azure.com/cncf/envoy/_build/results?buildId=77574&view=logs&j=c5dd2866-6ab3-5f3c-3a44-4cef0ec909b5&t=a9eb66d6-8944-5769-b3f7-476949dadcb8&l=469

this interface/registration has just changed - so the ci link you posted is now out-of-date

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-06-05T12:09:15Z

Thanks a lot @phlax for the hints! I added the extension to source/extensions/extensions_metadata.yaml.

source/extensions/matching/input_matchers/ip/matcher.cc

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

snowp

Thanks, this is looking good just a few more comments.

Could you also add a release note for this extension?

snowp · 2021-06-09T15:52:20Z

source/extensions/matching/input_matchers/ip/config.cc

  }

-  return std::make_unique<Matcher>(std::move(ranges));
+  const auto stat_prefix = ip_config.stat_prefix();


nit: this incurs an additional copy, either const auto& or just pass this inline

Fixed in 551ddcb , by using an explicit string_view

snowp · 2021-06-09T15:53:59Z

source/extensions/matching/input_matchers/ip/matcher.cc

+    if (stats_) {
+      stats_->ip_parsing_failed_.inc();
+    }
+    ENVOY_LOG(warn, "IP matcher: unable to parse address '{}'", ip_str);


I'd suggest dropping down this down to debug or use one of the rate limited loggers, this could be extremely spammy if deployed with bad configuration which would impact proxy performance.

Dropped down to debug in 551ddcb

snowp · 2021-06-09T15:57:10Z

api/envoy/extensions/matching/input_matchers/ip/v3/ip.proto

+  // If specified, emits statistics using this prefix.
+  string stat_prefix = 2;


Could we include docs that explain what stats are emitted? Normally we'd have a RST page explaining this extension, but I think inlining these docs in the proto file is fine for now

Fixed in 7960aa3

snowp · 2021-06-09T16:00:01Z

source/extensions/matching/input_matchers/ip/matcher.cc

+  if (!stat_prefix.empty()) {
+    stats_.emplace(generateStats(stat_prefix, stat_scope));
+  }


I think its more common to emit stats by default for extensions, so maybe make stats_prefix required so that it's always set? Or is there a good reason to avoid emitting this stat?

I've seen this in another extension, so I just made the same :) I have no other rational than this, I'm happy to make it mandatory!

Done in 7960aa3 !

aguinet · 2021-06-16T17:15:30Z

@snowp I merged with main.

I have a question though. In ff91bae#diff-2b3a93adfb5a23333aaf5113d8a99f142a8187735140781c49aeacd8c6c03795R30, is the reference to scope still valid when the callback is called? Thanks!

Edit: release note in ea28e0d

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

snowp · 2021-06-17T12:47:28Z

Can you look at coverage? Seems like the build is failing due to this new code being slightly below the coverage threshold

/wait

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-06-17T16:30:17Z

Can you look at coverage? Seems like the build is failing due to this new code being slightly below the coverage threshold

/wait

So the coverage problem is that I don't reach https://github.com/envoyproxy/envoy/pull/16592/files#diff-2b3a93adfb5a23333aaf5113d8a99f142a8187735140781c49aeacd8c6c03795R24 . It is basically the issue I explain here #16592 (comment) . I can remove the check, but I would like another POV on this :)

snowp · 2021-06-18T02:04:24Z

It seems like isValid just checks that the length is greater than 0, is it possible to hit that? If not, I think we can just replace it with an ASSERT with a comment explaining why this can't happen

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-06-20T07:19:41Z

It seems like isValid just checks that the length is greater than 0, is it possible to hit that? If not, I think we can just replace it with an ASSERT with a comment explaining why this can't happen

I didn't manage to hit it. We always at least have one IP (with /32 on ipv4 for instance). I added an ASSERT with a comment here ff53fe4

adisuissa

/lgtm api

Added a suggestion to clarify docs.

adisuissa · 2021-06-22T12:08:47Z

api/envoy/extensions/matching/input_matchers/ip/v3/ip.proto

+
+  // The human readable prefix to use when emitting statistics for the IP input
+  // matcher.
+  //


Nit: clarify the the following counters will be concatenated to the stat_prefix

Done in 5aaaa86

snowp · 2021-06-23T13:54:34Z

LGTM with @adisuissa's nit

@adisuissa

Suggestion by @adisuissa Signed-off-by: Adrien Guinet <adrien@reblaze.com>

adisuissa · 2021-06-23T17:56:01Z

/lgtm api

aguinet · 2021-06-24T04:29:46Z

Okay I forgot to regenerate the API.. Will fix it soon

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet · 2021-06-24T06:29:21Z

Okay I forgot to regenerate the API.. Will fix it soon

Fixed in 13fd781

snowp

Thanks!

aguinet · 2021-06-25T13:29:18Z

Thanks for the merge and everyone for the reviews!

Input matcher that checks that an IP{v4,v6} belongs to a list of CIDR ranges Signed-off-by: Adrien Guinet <adrien@reblaze.com> Signed-off-by: chris.xin <xinchuantao@qq.com>

Input matcher that checks that an IP{v4,v6} belongs to a list of CIDR ranges Signed-off-by: Adrien Guinet <adrien@reblaze.com>

IP Matcher on a list of CIDR ranges

cd38be7

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

repokitteh-read-only bot added the api label May 20, 2021

repokitteh-read-only bot assigned adisuissa May 20, 2021

adisuissa reviewed May 26, 2021

View reviewed changes

aguinet added 2 commits May 28, 2021 09:07

Rename IP to Ip + nit fixes

16c5477

Review by @adisuissa Signed-off-by: Adrien Guinet <adrien@reblaze.com>

API: remove unused protobuf imports

ef8fe28

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

adisuissa reviewed Jun 1, 2021

View reviewed changes

api/envoy/extensions/matching/input_matchers/ip/v3/ip.proto Outdated Show resolved Hide resolved

Review: nit on doc

bdc5274

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

aguinet added 2 commits June 2, 2021 19:31

Merge branch 'main' into feature/ips_matcher

6d7f90b

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Cleanup extension BUILD definition

e940002

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

junr03 assigned snowp Jun 2, 2021

snowp suggested changes Jun 3, 2021

View reviewed changes

snowp added the waiting label Jun 3, 2021

Review by @snowp

a0360a7

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

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

Add extension metadata

e027a05

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

snowp reviewed Jun 7, 2021

View reviewed changes

source/extensions/matching/input_matchers/ip/matcher.cc Outdated Show resolved Hide resolved

snowp reviewed Jun 7, 2021

View reviewed changes

source/extensions/matching/input_matchers/ip/matcher.cc Outdated Show resolved Hide resolved

aguinet added 5 commits June 8, 2021 21:03

emit stat + log when unable to parse an IP

04b5afe

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Merge branch 'main' into feature/ips_matcher

c4b0f24

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

Adapt the build system

354ac87

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

synchronize ip matcher protobuf

d565e78

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

add config test with stat prefix

3b2391d

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

snowp suggested changes Jun 9, 2021

View reviewed changes

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

Add a release note

ea28e0d

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

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

Avoid a copy (fix clang-tidy warning)

700187a

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

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

snowp added the waiting label Jun 18, 2021

config: only ASSERT that ranges are valid

ff53fe4

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

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

adisuissa reviewed Jun 22, 2021

View reviewed changes

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

Doc: explicit the concatenation process in the names of the statistics

5aaaa86

Suggestion by @adisuissa Signed-off-by: Adrien Guinet <adrien@reblaze.com>

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

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

Fix generated api

13fd781

Signed-off-by: Adrien Guinet <adrien@reblaze.com>

snowp approved these changes Jun 25, 2021

View reviewed changes

snowp merged commit 6cd6a0c into envoyproxy:main Jun 25, 2021

aguinet deleted the feature/ips_matcher branch June 25, 2021 13:30

mattklein123 mentioned this pull request Jun 29, 2021

Revert "alpha matching: support generic action factory context (#17025)" #17191

Merged

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

IP Matcher on a list of CIDR ranges (envoyproxy#16592)

690342f

Input matcher that checks that an IP{v4,v6} belongs to a list of CIDR ranges Signed-off-by: Adrien Guinet <adrien@reblaze.com>

		// If specified, emits statistics using this prefix.
		string stat_prefix = 2;

Conversation

aguinet commented May 20, 2021

Uh oh!

repokitteh-read-only bot commented May 20, 2021

Uh oh!

aguinet commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

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

Uh oh!

aguinet commented Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

snowp commented Jun 3, 2021

Uh oh!

phlax commented Jun 3, 2021

Uh oh!

aguinet commented Jun 5, 2021

Uh oh!

Uh oh!

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aguinet Jun 12, 2021 • 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!

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!

aguinet commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snowp commented Jun 17, 2021

Uh oh!

aguinet commented Jun 17, 2021

Uh oh!

snowp commented Jun 18, 2021

Uh oh!

aguinet commented Jun 20, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

aguinet commented May 20, 2021 •

edited

Loading

adisuissa left a comment •

edited

Loading

aguinet commented Jun 2, 2021 •

edited

Loading

aguinet Jun 12, 2021 •

edited

Loading

aguinet commented Jun 16, 2021 •

edited

Loading