Skip to content

lc trie: return a vector of tags #2612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alyssawilk merged 4 commits into from
Feb 20, 2018
Merged

lc trie: return a vector of tags #2612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1ab29d
Fix getTag to be getTags and return a vector of strings
ccaraman Feb 14, 2018
681c06a
Add another todo
ccaraman Feb 14, 2018
041791e
Merge branch 'master' into ret_vector
ccaraman Feb 15, 2018
a2e1a6e
Address a few comments.
ccaraman Feb 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion source/common/network/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ envoy_cc_library(
":cidr_range_lib",
":utility_lib",
"//source/common/common:assert_lib",
"//source/common/common:empty_string",
],
)

Expand Down
7 changes: 4 additions & 3 deletions source/common/network/lc_trie.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,14 @@ LcTrie::LcTrie(const std::vector<std::pair<std::string, std::vector<Address::Cid
ipv6_trie_.reset(new LcTrieInternal<Ipv6>(ipv6_prefixes, fill_factor, root_branching_factor));
}

std::string LcTrie::getTag(const Network::Address::InstanceConstSharedPtr& ip_address) const {
std::vector<std::string>
LcTrie::getTags(const Network::Address::InstanceConstSharedPtr& ip_address) const {
if (ip_address->ip()->version() == Address::IpVersion::v4) {
Ipv4 ip = ntohl(ip_address->ip()->ipv4()->address());
return ipv4_trie_->getTag(ip);
return ipv4_trie_->getTags(ip);
} else {
Ipv6 ip = Utility::Ip6ntohl(ip_address->ip()->ipv6()->address());
return ipv6_trie_->getTag(ip);
return ipv6_trie_->getTags(ip);
}
}

Expand Down
33 changes: 18 additions & 15 deletions source/common/network/lc_trie.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "envoy/network/address.h"

#include "common/common/assert.h"
#include "common/common/empty_string.h"
#include "common/network/address_impl.h"
#include "common/network/cidr_range.h"
#include "common/network/utility.h"
Expand Down Expand Up @@ -48,14 +47,13 @@ class LcTrie {
/**
* Retrieve the tag associated with the CIDR range that contains `ip_address`. Both IPv4 and IPv6
* addresses are supported.
* TODO(ccaraman): Change to getTags and std::vector<std::string> when nested prefixes are
* supported.
* @param ip_address supplies the IP address.
* @return tag from the CIDR range that contains 'ip_address'. An empty string is returned
* if no prefix contains 'ip_address' or there is no data for the IP version of the
* ip_address.
* @return a vector of tags from the CIDR ranges and IP addresses that contains 'ip_address'. An
Copy link
Contributor

@jmarantz jmarantz Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the pattern here was to give the type immediately after the @param or @return keyword, e.g.

 * @return std::vector<std::string> a vector of tags ...

at least I've seen this elsewhere in the codebase. I'm not sure what script or program processes this style though.

Copy link
Contributor Author

@ccaraman ccaraman Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question. I don't know what peoples preferences are these days. I'll find out.

Copy link
Contributor

@jmarantz jmarantz Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In our local chatter, no one understands why we are repeating the types in the comments; and I learned they are not needed for any scripts that generate API doc currently.

IMO your form is better and we should convert everything to it.

Copy link
Member

@ggreenway ggreenway Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has come up a couple times. Matt commented that he started the policy out of habit, because some script (super old doxygen from 1973??) output better docs if it was included.

I think we should remove those types from all the docs. They're not providing any value, and they can get out-of-date from the actual types on the function.

If not, we should make a linter that enforces them.

Copy link
Contributor Author

@ccaraman ccaraman Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, i'll keep it as is.

Copy link
Contributor

@alyssawilk alyssawilk Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly worth a call-out on envoy-dev or maintainers - I'm so happy to kill off that rule, and hadn't realized it was "outdated" so was enforcing.

* empty vector is returned if no prefix contains 'ip_address' or there is no data for the IP
* version of the ip_address.
*/
std::string getTag(const Network::Address::InstanceConstSharedPtr& ip_address) const;
std::vector<std::string>
getTags(const Network::Address::InstanceConstSharedPtr& ip_address) const;

private:
/**
Expand Down Expand Up @@ -198,10 +196,10 @@ class LcTrie {
/**
* Retrieve the tag associated with the CIDR range that contains `ip_address`.
* @param ip_address supplies the IP address in host byte order.
* @return the tag from the prefix that encompasses the input. An empty string is returned
* if no prefix in the LC Trie exists.
* @return a vector of tags from the CIDR ranges and IP addresses that encompasses the input. An
* empty vector is returned if the LC Trie is empty.
*/
std::string getTag(const IpType& ip_address) const;
std::vector<std::string> getTags(const IpType& ip_address) const;

private:
/**
Expand Down Expand Up @@ -510,9 +508,11 @@ LcTrie::LcTrieInternal<IpType, address_size>::LcTrieInternal(
}

template <class IpType, uint32_t address_size>
std::string LcTrie::LcTrieInternal<IpType, address_size>::getTag(const IpType& ip_address) const {
std::vector<std::string>
LcTrie::LcTrieInternal<IpType, address_size>::getTags(const IpType& ip_address) const {
std::vector<std::string> return_vector;
if (trie_.empty()) {
return EMPTY_STRING;
return return_vector;
}

LcNode node = trie_[0];
Expand All @@ -533,16 +533,19 @@ std::string LcTrie::LcTrieInternal<IpType, address_size>::getTag(const IpType& i

// /0 will match all IP addresses.
if (ip_prefixes_[address].length_ == 0) {
return ip_prefixes_[address].tag_;
return_vector.push_back(ip_prefixes_[address].tag_);
// TODO(ccaraman): When nested prefixes are supported, should the /0 case be handled better?
return return_vector;
}

// Check the input IP address is within the CIDR range stored in ip_prefixes_[adr] by XOR'ing the
// two values and checking that up until the length of the CIDR range the value is 0.
IpType bitmask = ip_prefixes_[address].ip_ ^ ip_address;
if (extractBits<IpType, address_size>(0, ip_prefixes_[address].length_, bitmask) == 0) {
return ip_prefixes_[address].tag_;
return_vector.push_back(ip_prefixes_[address].tag_);
}
return EMPTY_STRING;
// TODO(ccaraman): Search through the nested prefix structure.
return return_vector;
}

} // namespace LcTrie
Expand Down
143 changes: 75 additions & 68 deletions test/common/network/lc_trie_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ class LcTrieTest : public testing::Test {
}
}

void expectIPAndTag(const std::vector<std::pair<std::string, std::string>>& test_output) {
void expectIPAndTags(
const std::vector<std::pair<std::string, std::vector<std::string>>>& test_output) {
for (const auto& kv : test_output) {
EXPECT_EQ(kv.second, trie_->getTag(Utility::parseInternetAddress(kv.first)));
EXPECT_EQ(kv.second, trie_->getTags(Utility::parseInternetAddress(kv.first)));
}
}

Expand Down Expand Up @@ -64,17 +65,17 @@ TEST_F(LcTrieTest, IPv4Defaults) {
};
setup(cidr_range_strings);

std::vector<std::pair<std::string, std::string>> test_case = {
{"0.0.0.0", "tag_0"}, {"16.0.0.1", "tag_1"},
{"40.0.0.255", "tag_2"}, {"64.0.130.0", "tag_3"},
{"96.0.0.10", "tag_4"}, {"112.0.0.0", "tag_5"},
{"128.0.0.1", "tag_6"}, {"160.0.0.1", "tag_7"},
{"164.255.0.0", "tag_8"}, {"168.0.0.0", "tag_9"},
{"176.0.0.1", "tag_10"}, {"184.0.0.1", "tag_11"},
{"192.0.0.0", "tag_12"}, {"232.0.80.0", "tag_13"},
{"233.0.0.1", "tag_14"}, {"::1", ""},
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"0.0.0.0", {"tag_0"}}, {"16.0.0.1", {"tag_1"}},
{"40.0.0.255", {"tag_2"}}, {"64.0.130.0", {"tag_3"}},
{"96.0.0.10", {"tag_4"}}, {"112.0.0.0", {"tag_5"}},
{"128.0.0.1", {"tag_6"}}, {"160.0.0.1", {"tag_7"}},
{"164.255.0.0", {"tag_8"}}, {"168.0.0.0", {"tag_9"}},
{"176.0.0.1", {"tag_10"}}, {"184.0.0.1", {"tag_11"}},
{"192.0.0.0", {"tag_12"}}, {"232.0.80.0", {"tag_13"}},
{"233.0.0.1", {"tag_14"}}, {"::1", {}},
};
expectIPAndTag(test_case);
expectIPAndTags(test_case);
}

// There was a bug in the C++ port that didn't update the index for the next address in the trie.
Expand All @@ -101,17 +102,17 @@ TEST_F(LcTrieTest, RootBranchingFactor) {
};
setup(cidr_range_strings, fill_factor, root_branching_factor);

std::vector<std::pair<std::string, std::string>> test_case = {
{"0.0.0.0", "tag_0"}, {"16.0.0.1", "tag_1"},
{"40.0.0.255", "tag_2"}, {"64.0.130.0", "tag_3"},
{"96.0.0.10", "tag_4"}, {"112.0.0.0", "tag_5"},
{"128.0.0.1", "tag_6"}, {"160.0.0.1", "tag_7"},
{"164.255.0.0", "tag_8"}, {"168.0.0.0", "tag_9"},
{"176.0.0.1", "tag_10"}, {"184.0.0.1", "tag_11"},
{"192.0.0.0", "tag_12"}, {"232.0.80.0", "tag_13"},
{"233.0.0.1", "tag_14"}, {"::1", ""},
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"0.0.0.0", {"tag_0"}}, {"16.0.0.1", {"tag_1"}},
{"40.0.0.255", {"tag_2"}}, {"64.0.130.0", {"tag_3"}},
{"96.0.0.10", {"tag_4"}}, {"112.0.0.0", {"tag_5"}},
{"128.0.0.1", {"tag_6"}}, {"160.0.0.1", {"tag_7"}},
{"164.255.0.0", {"tag_8"}}, {"168.0.0.0", {"tag_9"}},
{"176.0.0.1", {"tag_10"}}, {"184.0.0.1", {"tag_11"}},
{"192.0.0.0", {"tag_12"}}, {"232.0.80.0", {"tag_13"}},
{"233.0.0.1", {"tag_14"}}, {"::1", {}},
};
expectIPAndTag(test_case);
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, IPv4AddressSizeBoundaries) {
Expand All @@ -122,11 +123,13 @@ TEST_F(LcTrieTest, IPv4AddressSizeBoundaries) {
};

setup(cidr_range_strings);
std::vector<std::pair<std::string, std::string>> test_case = {
{"205.251.192.100", "tag_1"}, {"18.232.0.255", ""}, {"10.255.255.255", "tag_0"},
{"52.220.191.10", "tag_1"}, {"10.255.255.254", "tag_2"},
};
expectIPAndTag(test_case);
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"205.251.192.100", {"tag_1"}},
{"10.255.255.255", {"tag_0"}},
{"52.220.191.10", {"tag_1"}},
{"10.255.255.254", {"tag_2"}},
{"18.232.0.255", {}}};
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, IPv4Boundaries) {
Expand All @@ -137,11 +140,11 @@ TEST_F(LcTrieTest, IPv4Boundaries) {
};

setup(cidr_range_strings);
std::vector<std::pair<std::string, std::string>> test_case = {
{"10.255.255.255", "tag_0"},
{"205.251.192.100", "tag_2"},
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"10.255.255.255", {"tag_0"}},
{"205.251.192.100", {"tag_2"}},
};
expectIPAndTag(test_case);
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, IPv6) {
Expand All @@ -151,14 +154,14 @@ TEST_F(LcTrieTest, IPv6) {
};
setup(cidr_range_strings);

std::vector<std::pair<std::string, std::string>> test_case = {
{"2400:ffff:ff00::", ""},
{"2406:da00:2000::1", "tag_0"},
{"2001:abcd:ef01:2345::1", "tag_1"},
{"::1", "tag_0"},
{"1.2.3.4", ""},
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"2406:da00:2000::1", {"tag_0"}},
{"2001:abcd:ef01:2345::1", {"tag_1"}},
{"::1", {"tag_0"}},
{"1.2.3.4", {}},
{"2400:ffff:ff00::", {}},
};
expectIPAndTag(test_case);
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, IPv6AddressSizeBoundaries) {
Expand All @@ -169,14 +172,14 @@ TEST_F(LcTrieTest, IPv6AddressSizeBoundaries) {
};
setup(cidr_range_strings);

std::vector<std::pair<std::string, std::string>> test_case = {
{"::1", "tag_0"},
{"2406:da00:2000::1", "tag_0"},
{"2001:abcd:ef01:2345::1", "tag_1"},
{"::", "tag_2"},
{"::2", ""},
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"::1", {"tag_0"}},
{"2406:da00:2000::1", {"tag_0"}},
{"2001:abcd:ef01:2345::1", {"tag_1"}},
{"::", {"tag_2"}},
{"::2", {}},
};
expectIPAndTag(test_case);
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, IPv6Boundaries) {
Expand All @@ -187,12 +190,12 @@ TEST_F(LcTrieTest, IPv6Boundaries) {
};
setup(cidr_range_strings);

std::vector<std::pair<std::string, std::string>> test_case = {
{"::1", "tag_2"},
{"::2", "tag_2"},
{"8000::1", "tag_0"},
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"::1", {"tag_2"}},
{"::2", {"tag_2"}},
{"8000::1", {"tag_0"}},
};
expectIPAndTag(test_case);
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, CatchAllIPv4Prefix) {
Expand All @@ -202,13 +205,13 @@ TEST_F(LcTrieTest, CatchAllIPv4Prefix) {
};
setup(cidr_range_strings);

std::vector<std::pair<std::string, std::string>> test_case = {
{"2001:abcd:ef01:2345::1", "tag_1"},
{"1.2.3.4", "tag_0"},
{"255.255.255.255", "tag_0"},
{"2400:ffff:ff00::", ""},
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"2001:abcd:ef01:2345::1", {"tag_1"}},
{"1.2.3.4", {"tag_0"}},
{"255.255.255.255", {"tag_0"}},
{"2400:ffff:ff00::", {}},
};
expectIPAndTag(test_case);
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, CatchAllIPv6Prefix) {
Expand All @@ -218,13 +221,12 @@ TEST_F(LcTrieTest, CatchAllIPv6Prefix) {
};
setup(cidr_range_strings);

std::vector<std::pair<std::string, std::string>> test_case = {
{"2001:abcd:ef01:2345::1", "tag_0"},
{"1.2.3.4", "tag_1"},
{"255.255.255.255", ""},
{"abcd::343", "tag_0"},
};
expectIPAndTag(test_case);
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"2001:abcd:ef01:2345::1", {"tag_0"}},
{"1.2.3.4", {"tag_1"}},
{"abcd::343", {"tag_0"}},
{"255.255.255.255", {}}};
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, BothIpvVersions) {
Expand All @@ -236,12 +238,17 @@ TEST_F(LcTrieTest, BothIpvVersions) {
};
setup(cidr_range_strings);

std::vector<std::pair<std::string, std::string>> test_case = {
{"205.251.192.100", "tag_3"}, {"18.232.0.255", ""}, {"10.255.255.255", "tag_2"},
{"52.220.191.10", "tag_3"}, {"2400:ffff:ff00::", ""}, {"2406:da00:2000::1", "tag_0"},
{"2001:abcd:ef01:2345::1", "tag_1"}, {"::1", "tag_0"},
std::vector<std::pair<std::string, std::vector<std::string>>> test_case = {
{"205.251.192.100", {"tag_3"}},
{"10.255.255.255", {"tag_2"}},
{"52.220.191.10", {"tag_3"}},
{"2406:da00:2000::1", {"tag_0"}},
{"2001:abcd:ef01:2345::1", {"tag_1"}},
{"::1", {"tag_0"}},
{"18.232.0.255", {}},
{"2400:ffff:ff00::", {}},
};
expectIPAndTag(test_case);
expectIPAndTags(test_case);
}

TEST_F(LcTrieTest, NestedPrefixes) {
Expand Down