Skip to content

redis cluster: fix ClusterSlot operator == #16116

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
snowp merged 11 commits into from
May 17, 2021
Merged

redis cluster: fix ClusterSlot operator == #16116

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5c326e9
redis cluster: fix ClusterSlot operator ==
gaoliangdut Apr 22, 2021
c6f431f
redis cluster: remove else
gaoliangdut Apr 23, 2021
6dbef99
redis cluster: fix replicas compare
gaoliangdut Apr 23, 2021
bb75fe2
redis cluster: use equality function
gaoliangdut Apr 25, 2021
4c19890
redis cluster: change flat_hash_set to map
gaoliangdut Apr 28, 2021
6594417
redis cluster: compare with std::equal
gaoliangdut Apr 29, 2021
732d8e7
change std::map to absl::btree_map
gaoliangdut May 10, 2021
46e9ff6
Kick CI
gaoliangdut May 11, 2021
ba8b162
add a comment for ClusterSlot compare
gaoliangdut May 13, 2021
61e7bc4
Merge branch 'envoyproxy:main' into main
gaoliangdut May 16, 2021
fa55163
Kick CI
gaoliangdut May 16, 2021
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
14 changes: 12 additions & 2 deletions source/extensions/clusters/redis/redis_cluster_lb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,18 @@ namespace Clusters {
namespace Redis {

bool ClusterSlot::operator==(const Envoy::Extensions::Clusters::Redis::ClusterSlot& rhs) const {
return start_ == rhs.start_ && end_ == rhs.end_ && primary_ == rhs.primary_ &&
replicas_ == rhs.replicas_;
if (start_ != rhs.start_ || end_ != rhs.end_ || *primary_ != *rhs.primary_ ||
replicas_.size() != rhs.replicas_.size()) {
return false;
}

for (auto const& replica : rhs.replicas_) {
if (replicas_.find(replica) == replicas_.end()) {
return false;
}
}

@rojkov rojkov Apr 26, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Isn't this loop equivalent to just

return replicas_ == rhs.replicas_;

@gaoliangdut gaoliangdut Apr 28, 2021

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.

In the test case, "replicas_ == rhs.replicas_" returns false, but In my code, it returns true. I'm trying to find the reason for this.

@gaoliangdut gaoliangdut Apr 28, 2021

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.

This is really not equal. Because in absl, "operator ==" uses "==" to compare the value, and "find" uses "eq_ref" to compare the value.
image
image


return true;
}

// RedisClusterLoadBalancerFactory
Expand Down
15 changes: 13 additions & 2 deletions source/extensions/clusters/redis/redis_cluster_lb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,18 @@ class ClusterSlot {
int64_t start() const { return start_; }
int64_t end() const { return end_; }
Network::Address::InstanceConstSharedPtr primary() const { return primary_; }
const absl::flat_hash_set<Network::Address::InstanceConstSharedPtr>& replicas() const {
struct Hash {
size_t operator()(const Network::Address::InstanceConstSharedPtr& address) const {
return absl::Hash<std::string>()(address->asString());

@jmarantz jmarantz Apr 26, 2021

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.

As a matter of good practice, you'd want to use return absl::Hash<absl::string_view>()(address->asStringView()); to avoid potentially creating a string temp while hashing. Though actually the name is held in a string in the main impl, so the asString() impl currently won't do that.

But in researching this I found a potential latent crash here. The doc for asString() says:

   * @return a human readable string for the address that represents the
   * physical/resolved address. (This will not necessarily include port
   * information, if applicable, since that may not be resolved until bind()).

This suggests to me that bind() might mutate the name. If that happens after we have already stored an address in a hash-table, I think this can crash in subsequent hash-table operations.

So my suggestion is to convert this to a map<string,Network::Address::InstanceConstSharedPtr>. This will result in string-copies for the map key, but will avoid crashing Envoy if bind() mutates the name. And if you do that you won't need this custom hasher/comparator.

@rojkov rojkov Apr 28, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

+1

That's a good catch! There's no good in hashing keys which can mutate.

@gaoliangdut gaoliangdut Apr 28, 2021

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 this suggestion, I have use map instead of flat_hash_set and create a commit.

}
};
struct Eq {
bool operator()(const Network::Address::InstanceConstSharedPtr& lhs,
const Network::Address::InstanceConstSharedPtr& rhs) const {
return *lhs == *rhs;
}
};
const absl::flat_hash_set<Network::Address::InstanceConstSharedPtr, Hash, Eq>& replicas() const {
return replicas_;
}
void addReplica(Network::Address::InstanceConstSharedPtr replica_address) {
Expand All @@ -50,7 +61,7 @@ class ClusterSlot {
int64_t start_;
int64_t end_;
Network::Address::InstanceConstSharedPtr primary_;
absl::flat_hash_set<Network::Address::InstanceConstSharedPtr> replicas_;
absl::flat_hash_set<Network::Address::InstanceConstSharedPtr, Hash, Eq> replicas_;
};

using ClusterSlotsPtr = std::unique_ptr<std::vector<ClusterSlot>>;
Expand Down
20 changes: 17 additions & 3 deletions test/extensions/clusters/redis/redis_cluster_lb_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,18 +349,30 @@ TEST_F(RedisClusterLoadBalancerTest, ClusterSlotUpdate) {

TEST_F(RedisClusterLoadBalancerTest, ClusterSlotNoUpdate) {
Upstream::HostVector hosts{Upstream::makeTestHost(info_, "tcp://127.0.0.1:90", simTime()),
Upstream::makeTestHost(info_, "tcp://127.0.0.1:91", simTime()),
Upstream::makeTestHost(info_, "tcp://127.0.0.1:92", simTime()),
Upstream::makeTestHost(info_, "tcp://127.0.0.1:90", simTime()),
Upstream::makeTestHost(info_, "tcp://127.0.0.1:91", simTime()),
Upstream::makeTestHost(info_, "tcp://127.0.0.1:92", simTime())};
Upstream::HostVector replicas{Upstream::makeTestHost(info_, "tcp://127.0.0.2:90", simTime()),
Upstream::makeTestHost(info_, "tcp://127.0.0.2:91", simTime()),
Upstream::makeTestHost(info_, "tcp://127.0.0.2:90", simTime()),
Upstream::makeTestHost(info_, "tcp://127.0.0.2:91", simTime())};

ClusterSlotsPtr slots = std::make_unique<std::vector<ClusterSlot>>(std::vector<ClusterSlot>{
ClusterSlot(0, 1000, hosts[0]->address()),
ClusterSlot(1001, 2000, hosts[1]->address()),
ClusterSlot(2001, 16383, hosts[2]->address()),
});

(*slots)[0].addReplica(replicas[0]->address());
(*slots)[0].addReplica(replicas[1]->address());
Upstream::HostMap all_hosts{
{hosts[0]->address()->asString(), hosts[0]},
{hosts[1]->address()->asString(), hosts[1]},
{hosts[2]->address()->asString(), hosts[2]},
{replicas[0]->address()->asString(), replicas[0]},
{replicas[1]->address()->asString(), replicas[1]},
};

// A list of (hash: host_index) pair.
Expand All @@ -373,10 +385,12 @@ TEST_F(RedisClusterLoadBalancerTest, ClusterSlotNoUpdate) {

// Calling cluster slot update without change should not change assignment.
std::vector<ClusterSlot> updated_slot{
ClusterSlot(0, 1000, hosts[0]->address()),
ClusterSlot(1001, 2000, hosts[1]->address()),
ClusterSlot(2001, 16383, hosts[2]->address()),
ClusterSlot(0, 1000, hosts[3]->address()),
ClusterSlot(1001, 2000, hosts[4]->address()),
ClusterSlot(2001, 16383, hosts[5]->address()),
};
updated_slot[0].addReplica(replicas[3]->address());
updated_slot[0].addReplica(replicas[2]->address());
EXPECT_EQ(false, factory_->onClusterSlotUpdate(
std::make_unique<std::vector<ClusterSlot>>(updated_slot), all_hosts));
validateAssignment(hosts, expected_assignments);
Expand Down