Skip to content

Support nested prefixes in the LcTrie class #2614

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 5 commits into from
Feb 20, 2018
Merged

Support nested prefixes in the LcTrie class #2614

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
37adece
Support nested prefixes in the LcTrie class
Feb 14, 2018
8e15383
Merge remote-tracking branch 'upstream/master' into nested-prefix
Feb 15, 2018
1cb6ac0
cleanups based on code review feedback
Feb 16, 2018
6f958a4
Merge remote-tracking branch 'upstream/master' into nested-prefix
Feb 20, 2018
6ce091a
add a TODO comment about nested tries, and refactor `isPrefix` to use…
Feb 20, 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
114 changes: 57 additions & 57 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
* 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 All @@ -72,6 +70,9 @@ class LcTrie {
// By shifting the value to the left by p bits(and back), the bits between 0 and p-1 are zero'd
// out. Then to get the n bits, shift the IP back by the address_size minus the number of
// desired bits.
if (n == 0) {
return IpType(0);
}
return input << p >> (address_size - n);
}

Expand All @@ -94,31 +95,6 @@ class LcTrie {
typedef uint32_t Ipv4;
typedef absl::uint128 Ipv6;

// Helper methods to retrieve the string representation of the address in IpPrefix using
// inet_ntop. These strings are used in the nested prefixes exception messages.
// TODO(ccaraman): Remove once nested prefixes are supported.
static std::string toString(const Ipv4& input) {
sockaddr_in addr4;
addr4.sin_family = AF_INET;
addr4.sin_addr.s_addr = htonl(input);
addr4.sin_port = htons(0);

Address::Ipv4Instance address(&addr4);
return address.ip()->addressAsString();
}

static std::string toString(const Ipv6& input) {
sockaddr_in6 addr6;
addr6.sin6_family = AF_INET6;
addr6.sin6_port = htons(0);

// Ipv6 stores the values in host byte order.
absl::uint128 ipv6 = Utility::Ip6htonl(input);
memcpy(&addr6.sin6_addr.s6_addr, &ipv6, sizeof(absl::uint128));
Address::Ipv6Instance address(addr6);
return address.ip()->addressAsString();
}

/**
* Structure to hold a CIDR range and the tag associated with it.
*/
Expand Down Expand Up @@ -157,15 +133,36 @@ class LcTrie {
extractBits<IpType, address_size>(0, length_, other.ip_)));

@alyssawilk alyssawilk Feb 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

migrate over to using contains()?

}

/**
* @param address supplies an IP address to check against this prefix.
* @return bool true if this prefix contains the address.
*/
bool contains(const IpType& address) const {
return (extractBits<IpType, address_size>(0, length_, ip_) ==
extractBits<IpType, address_size>(0, length_, address));
}

std::string asString() { return fmt::format("{}/{}", toString(ip_), length_); }

void addNestedPrefix(const IpPrefix& other) {
if (nested_prefixes_ == nullptr) {
nested_prefixes_ = std::make_shared<std::vector<IpPrefix>>();
}
nested_prefixes_->push_back(other);
}

// The address represented either in Ipv4(uint32_t) or Ipv6(asbl::uint128).
IpType ip_{0};
// Length of the cidr range.
int length_;
// TODO(ccaraman): Support more than one tag per entry.
// Tag for this entry.
std::string tag_;
// Other prefixes nested under this one. If an LC trie lookup matches on this
// prefix, the lookup will scan the nested prefixes to see if any of them match,
// too. This situation is rare, so to save memory in the common case the
// nested_prefixes field is a pointer to a vector rather than an inline vector.
std::shared_ptr<std::vector<IpPrefix>> nested_prefixes_;

@ccaraman ccaraman Feb 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this meant to be a unique_ptr? addNestedPrefix is calling make_unique

@brian-pane brian-pane Feb 16, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching that. It started out as unique_ptr but then I found that the existing code assumed copyability of the containing struct. So I changed to shared_ptr, but I forgot to change addNestedPrefix. I’ll fix that today.

};

/**
Expand Down Expand Up @@ -198,10 +195,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 All @@ -228,16 +225,12 @@ class LcTrie {
std::sort(tag_data.begin(), tag_data.end());
ip_prefixes_.push_back(tag_data[0]);

// Remove duplicate entries and check for nested prefixes.
// Set up ip_prefixes_, which should contain the supplied prefixes in sorted order,
// but with any nested prefixes encapsulated under their parent prefixes.
for (size_t i = 1; i < tag_data.size(); ++i) {
// TODO(ccaraman): Add support for nested prefixes.
if (tag_data[i - 1].isPrefix(tag_data[i])) {
throw EnvoyException(fmt::format(
"LcTrie does not support nested prefixes. '{0}' is a nested prefix of '{1}'.",
tag_data[i].asString(), tag_data[i - 1].asString()));
}

if (tag_data[i - 1] != tag_data[i]) {
if (ip_prefixes_[ip_prefixes_.size() - 1].isPrefix(tag_data[i])) {
ip_prefixes_[ip_prefixes_.size() - 1].addNestedPrefix(tag_data[i]);

@alyssawilk alyssawilk Feb 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, question time, if we're trying to build a trie, isn't this actually building a vector of maybe vectors? I'd expect from the class name if we had nested prefixes those could have their own nested prefix rather than being bundled together at depth 1. Is this just infrequent enough we do it the inefficient way and if so, mind commenting it up?

@ccaraman ccaraman Feb 20, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, it is building a vector of nested prefixes. From talking to @mattklein123, nested prefixes aren't that common and linearly traversing a sorted vector in these cases should be good enough for now. If we do notice there being an issue, we can always swap this to have tries for the nested prefix cases.

@brian-pane brian-pane Feb 20, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll add a TODO comment about how more general-purpose usage will require using sub-tries for the nested prefix cases.

@brian-pane brian-pane Feb 20, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Another solution, I think, would be to change the construction of the base LC Trie to allow nested prefixes. As far as I can tell, the data structure should be able to handle nested prefixes in a single trie; it's just the trie construction algorithm from the original paper that doesn't support nesting. For now, though, I just added the TODO comment.

} else {
ip_prefixes_.push_back(tag_data[i]);
}
}
Expand Down Expand Up @@ -510,9 +503,10 @@ 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 {
if (trie_.empty()) {
return EMPTY_STRING;
return std::vector<std::string>();
}

LcNode node = trie_[0];
Expand All @@ -531,18 +525,24 @@ std::string LcTrie::LcTrieInternal<IpType, address_size>::getTag(const IpType& i
address = node.address_;
}

// /0 will match all IP addresses.
if (ip_prefixes_[address].length_ == 0) {
return ip_prefixes_[address].tag_;
}

// 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_;
// The prefix table entry ip_prefixes_[address] contains either a single prefix or

@ccaraman ccaraman Feb 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add a todo assigned to me to look into if having a better way to handle /0 is ideal.

@brian-pane brian-pane Feb 16, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, added! With this PR, the code is relatively clean, because the /0 special case is encapsulated in the extractBits function, but the drawback is that the extra conditional in that function will make the common case (prefixes of nonzero length) a bit slower. I'll defer to you on whether to optimize for code simplicity or runtime speed.

// a parent prefix with a set of additional prefixes nested under it. In the latter
// case, we compare the supplied ip_address against all the prefixes in the entry
// and return the union of all the matches' tags.
// TODO(ccaraman): determine whether there's a more optimal way to handle "/0" prefixes.
std::unordered_set<std::string> unique_tags;
const auto& prefix = ip_prefixes_[address];
if (prefix.contains(ip_address)) {
unique_tags.insert(prefix.tag_);
if (prefix.nested_prefixes_ != nullptr) {
for (const auto& nested_prefix : *prefix.nested_prefixes_) {
if (nested_prefix.contains(ip_address)) {
unique_tags.insert(nested_prefix.tag_);
}
}
}
}
return EMPTY_STRING;
return std::vector<std::string>(unique_tags.begin(), unique_tags.end());
}

} // namespace LcTrie
Expand Down
Loading