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 6 commits
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
24 changes: 23 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,27 @@ 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 (auto current = address_list.begin(); current != address_list.end(); ++current) {
// Find the first address with a different address family than the current address.
auto it = std::find_if(current, address_list.end(), [&](const auto& val) {
return (val->type() != Address::Type::Ip || (*current)->type() != Address::Type::Ip ||

@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.

kinda curious, does the spec say what to do with non IP addresses? Should we just remove those if we see any?

@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.

The specs refer specifically to IP addresses. I don't think they even consider non-IP addresses. The other address types are Pipe and EnvoyInternal. Are these valid addresses for passing into a Network::ClientConnection? From spelunking down in the code, it kinda looks like we end up in SocketInterfaceImpl::socket which seems to support non-IP addresses so I think we need to keep them in the list, but maybe we don't need them if we're doing Happy Eyeballs?

(*current)->ip()->version() != val->ip()->version());
});
// If there are no more addresses with different families the sorting is finished.
if (it == address_list.end()) {
break;
}
// Bubble the address up to the current position.
auto start = std::make_reverse_iterator(it + 1);
auto end = std::make_reverse_iterator(current + 1);
std::rotate(start, start + 1, end);
}
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

@ggreenway ggreenway 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.

"list must already BE sorted...

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

Done. PTAL?

// 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
47 changes: 42 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,40 @@ 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));
}

} // namespace Network
} // namespace Envoy