Skip to content

upstream: Implement Happy Eyeballs address sorting #18906

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
RyanTheOptimist merged 10 commits into from
Nov 16, 2021
Merged

upstream: Implement Happy Eyeballs address sorting #18906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7c4538c
upstream: Implement Happy Eyeballs address sorting
RyanTheOptimist Nov 4, 2021
e03104a
no logging
RyanTheOptimist Nov 5, 2021
ef769a6
WIP
RyanTheOptimist Nov 9, 2021
3f3a4ac
Cleanup
RyanTheOptimist Nov 9, 2021
0a3f5da
Format
RyanTheOptimist Nov 9, 2021
eb86577
Update source/common/network/happy_eyeballs_connection_impl.h
RyanTheOptimist Nov 9, 2021
0561593
Rewrite sortAddresses to be O(N)
RyanTheOptimist Nov 11, 2021
9c84f2b
format
RyanTheOptimist Nov 11, 2021
8fd62a2
Merge branch 'main' into sortAddresses
RyanTheOptimist Nov 16, 2021
545928d
typo
RyanTheOptimist Nov 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
38 changes: 37 additions & 1 deletion source/common/network/happy_eyeballs_connection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ HappyEyeballsConnectionImpl::HappyEyeballsConnectionImpl(
Address::InstanceConstSharedPtr source_address, TransportSocketFactory& socket_factory,
TransportSocketOptionsConstSharedPtr transport_socket_options,
const ConnectionSocket::OptionsSharedPtr options)
: id_(ConnectionImpl::next_global_id_++), dispatcher_(dispatcher), address_list_(address_list),
: id_(ConnectionImpl::next_global_id_++), dispatcher_(dispatcher),
address_list_(sortAddresses(address_list)),

@junr03 junr03 Nov 9, 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.

food for thought: do we care about making this behavior configurable? i.e., opt-out via config?

@RyanTheOptimist RyanTheOptimist Nov 9, 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.

Hm. I'm inclined to say "no", or at least "not unless someone asks for it". Happy Eyeballs itself is already configurable and I can't really imagine why this would be something we'd want to disable if Happy Eyeballs were in use. That being said, if someone ask for it then we could definitely make it configurable. WDYT?

@junr03 junr03 Nov 15, 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.

fair. sg!

connection_construction_state_(
{source_address, socket_factory, transport_socket_options, options}),
next_attempt_timer_(dispatcher_.createTimer([this]() -> void { tryAnotherConnection(); })) {
Expand Down Expand Up @@ -379,6 +380,41 @@ void HappyEyeballsConnectionImpl::dumpState(std::ostream& os, int indent_level)
}
}

std::vector<Address::InstanceConstSharedPtr>
HappyEyeballsConnectionImpl::sortAddresses(const std::vector<Address::InstanceConstSharedPtr>& in) {
std::vector<Address::InstanceConstSharedPtr> address_list = in;
for (size_t current = 1; current < address_list.size(); ++current) {
// If the current address has a different family than the previous address then
// it is in the correct position.
if (address_list[current]->type() != Address::Type::Ip ||
address_list[current - 1]->type() != Address::Type::Ip ||
address_list[current]->ip()->version() != address_list[current - 1]->ip()->version()) {
continue;
}
// Find the first address with a different address family than the current address.
size_t next = current + 1;
while (next < address_list.size()) {
Comment thread
RyanTheOptimist marked this conversation as resolved.
Outdated
if (address_list[next]->type() != Address::Type::Ip ||
address_list[current]->ip()->version() != address_list[next]->ip()->version()) {
break;
}
++next;
}
// There are no more addresses with different families so sorting is finished.
if (next >= address_list.size()) {
break;
}

// Bubble the address up to the current position.
for (size_t i = next; i > current; i--) {
Comment thread
davinci26 marked this conversation as resolved.
Outdated
using std::swap;

@RenjieTang RenjieTang Nov 5, 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.

Since std::swap is only called once here, would it be cleaner to just call std::swap without the using clause?

@RyanTheOptimist RyanTheOptimist Nov 5, 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.

Apparently not, as per go/using-std-swap. Who knew?! (Believe it or not, there is a potential behavior difference between the two)

@RenjieTang RenjieTang Nov 5, 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.

Amazing... Thanks for providing the context!

swap(address_list[i], address_list[i - 1]);
}
current = next - 1;
}
return address_list;
Comment thread
alyssawilk marked this conversation as resolved.
}

ClientConnectionPtr HappyEyeballsConnectionImpl::createNextConnection() {
ASSERT(next_address_ < address_list_.size());
auto connection = dispatcher_.createClientConnection(
Expand Down
9 changes: 8 additions & 1 deletion source/common/network/happy_eyeballs_connection_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,13 @@ class HappyEyeballsConnectionImpl : public ClientConnection,
void hashKey(std::vector<uint8_t>& hash_key) const override;
void dumpState(std::ostream& os, int indent_level) const override;

// Returns a new vector containing the contents of |address_list| sorted
// with address families interleaved, as per Section 4 of RFC 8305, Happy
// Eyeballs v2. It is assumed that The list must already sorted as per
Comment thread
RyanTheOptimist marked this conversation as resolved.
Outdated
// Section 6 of RFC6724, which happens in ares_getaddrinfo().

@junr03 junr03 Nov 9, 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.

does the Apple implementation also do this? If not, then we would be breaking this requirement for iOS

@RyanTheOptimist RyanTheOptimist Nov 9, 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.

Yes, it does, according to what I read online.

@alyssawilk alyssawilk Nov 10, 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.

maybe comment as such.
is there any way to CHECK on this, in case there are other DNS impls that might not realize the constraint?

@RyanTheOptimist RyanTheOptimist Nov 10, 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.

I added a comment about Apple DNS. Good idea. As for detecting that this sorting might be missing, I'm not sure how we would do that short of implementing the sorting rules and checking that addr[i] < addr[i+1] but that's non-trivial. I'm open to suggestions though!

static std::vector<Address::InstanceConstSharedPtr>
sortAddresses(const std::vector<Address::InstanceConstSharedPtr>& address_list);

private:
// ConnectionCallbacks which will be set on an ClientConnection which
// sends connection events back to the HappyEyeballsConnectionImpl.
Expand Down Expand Up @@ -196,7 +203,7 @@ class HappyEyeballsConnectionImpl : public ClientConnection,
Event::Dispatcher& dispatcher_;

// List of addresses to attempt to connect to.
const std::vector<Address::InstanceConstSharedPtr>& address_list_;
const std::vector<Address::InstanceConstSharedPtr> address_list_;
// Index of the next address to use.
size_t next_address_ = 0;

Expand Down
54 changes: 49 additions & 5 deletions test/common/network/happy_eyeballs_connection_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,19 @@ class HappyEyeballsConnectionImplTest : public testing::Test {
: failover_timer_(new testing::StrictMock<Event::MockTimer>(&dispatcher_)),
transport_socket_options_(std::make_shared<TransportSocketOptionsImpl>()),
options_(std::make_shared<ConnectionSocket::Options>()),
address_list_({std::make_shared<Address::Ipv4Instance>("127.0.0.1"),
std::make_shared<Address::Ipv4Instance>("127.0.0.2"),
std::make_shared<Address::Ipv4Instance>("127.0.0.3")}) {
raw_address_list_({std::make_shared<Address::Ipv4Instance>("127.0.0.1"),
std::make_shared<Address::Ipv4Instance>("127.0.0.2"),
std::make_shared<Address::Ipv6Instance>("ff02::1", 0)}),
address_list_({raw_address_list_[0], raw_address_list_[2], raw_address_list_[1]}) {
EXPECT_CALL(transport_socket_factory_, createTransportSocket(_));
EXPECT_CALL(dispatcher_, createClientConnection_(address_list_[0], _, _, _))
.WillOnce(testing::InvokeWithoutArgs(
this, &HappyEyeballsConnectionImplTest::createNextConnection));

next_connections_.push_back(std::make_unique<StrictMock<MockClientConnection>>());
impl_ = std::make_unique<HappyEyeballsConnectionImpl>(
dispatcher_, address_list_, Address::InstanceConstSharedPtr(), transport_socket_factory_,
transport_socket_options_, options_);
dispatcher_, raw_address_list_, Address::InstanceConstSharedPtr(),
transport_socket_factory_, transport_socket_options_, options_);
}

// Called by the dispatcher to return a MockClientConnection. In order to allow expectations to
Expand Down Expand Up @@ -94,6 +95,7 @@ class HappyEyeballsConnectionImplTest : public testing::Test {
MockTransportSocketFactory transport_socket_factory_;
TransportSocketOptionsConstSharedPtr transport_socket_options_;
const ConnectionSocket::OptionsSharedPtr options_;
const std::vector<Address::InstanceConstSharedPtr> raw_address_list_;
const std::vector<Address::InstanceConstSharedPtr> address_list_;
std::vector<StrictMock<MockClientConnection>*> created_connections_;
std::vector<ConnectionCallbacks*> connection_callbacks_;
Expand Down Expand Up @@ -1049,5 +1051,47 @@ TEST_F(HappyEyeballsConnectionImplTest, LastRoundTripTime) {
EXPECT_EQ(rtt, impl_->lastRoundTripTime());
}

TEST_F(HappyEyeballsConnectionImplTest, SortAddresses) {
auto ip_v4_1 = std::make_shared<Address::Ipv4Instance>("127.0.0.1");
auto ip_v4_2 = std::make_shared<Address::Ipv4Instance>("127.0.0.2");
auto ip_v4_3 = std::make_shared<Address::Ipv4Instance>("127.0.0.3");
auto ip_v4_4 = std::make_shared<Address::Ipv4Instance>("127.0.0.4");

auto ip_v6_1 = std::make_shared<Address::Ipv6Instance>("ff02::1", 0);
auto ip_v6_2 = std::make_shared<Address::Ipv6Instance>("ff02::2", 0);
auto ip_v6_3 = std::make_shared<Address::Ipv6Instance>("ff02::3", 0);
auto ip_v6_4 = std::make_shared<Address::Ipv6Instance>("ff02::4", 0);

// All v4 address so unchanged.
std::vector<Address::InstanceConstSharedPtr> v4_list = {ip_v4_1, ip_v4_2, ip_v4_3, ip_v4_4};
EXPECT_EQ(v4_list, HappyEyeballsConnectionImpl::sortAddresses(v4_list));

// All v6 address so unchanged.
std::vector<Address::InstanceConstSharedPtr> v6_list = {ip_v6_1, ip_v6_2, ip_v6_3, ip_v6_4};
EXPECT_EQ(v6_list, HappyEyeballsConnectionImpl::sortAddresses(v6_list));

std::vector<Address::InstanceConstSharedPtr> v6_then_v4 = {ip_v6_1, ip_v6_2, ip_v4_1, ip_v4_2};
std::vector<Address::InstanceConstSharedPtr> interleaved = {ip_v6_1, ip_v4_1, ip_v6_2, ip_v4_2};
EXPECT_EQ(interleaved, HappyEyeballsConnectionImpl::sortAddresses(v6_then_v4));

std::vector<Address::InstanceConstSharedPtr> v6_then_single_v4 = {ip_v6_1, ip_v6_2, ip_v6_3,
ip_v4_1};
std::vector<Address::InstanceConstSharedPtr> interleaved2 = {ip_v6_1, ip_v4_1, ip_v6_2, ip_v6_3};
EXPECT_EQ(interleaved2, HappyEyeballsConnectionImpl::sortAddresses(v6_then_single_v4));

std::vector<Address::InstanceConstSharedPtr> mixed = {ip_v6_1, ip_v6_2, ip_v6_3, ip_v4_1,
ip_v4_2, ip_v4_3, ip_v4_4, ip_v6_4};
std::vector<Address::InstanceConstSharedPtr> interleaved3 = {ip_v6_1, ip_v4_1, ip_v6_2, ip_v4_2,
ip_v6_3, ip_v4_3, ip_v6_4, ip_v4_4};
EXPECT_EQ(interleaved3, HappyEyeballsConnectionImpl::sortAddresses(mixed));
for (size_t i = 0; i < interleaved3.size(); ++i) {
std::cout << i << " " << interleaved3[i]->asString() << "\n";

@RenjieTang RenjieTang Nov 4, 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.

debug log?

@RyanTheOptimist RyanTheOptimist Nov 5, 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.

Whoops! Done.

}
interleaved3 = HappyEyeballsConnectionImpl::sortAddresses(mixed);
for (size_t i = 0; i < interleaved3.size(); ++i) {
std::cout << i << " " << interleaved3[i]->asString() << "\n";
}
}

} // namespace Network
} // namespace Envoy