Skip to content
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.

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

listener: fix ipv6 error #3912

Merged
merged 4 commits into from
Jul 20, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
22 changes: 11 additions & 11 deletions source/common/network/address_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,19 +21,10 @@ namespace Address {

namespace {

// Check if an IP family is supported on this machine.
bool ipFamilySupported(int domain) {
const int fd = ::socket(domain, SOCK_STREAM, 0);
if (fd >= 0) {
RELEASE_ASSERT(::close(fd) == 0, "");
}
return fd != -1;
}

// Validate that IPv4 is supported on this platform, raise an exception for the
// given address if not.
void validateIpv4Supported(const std::string& address) {
static const bool supported = ipFamilySupported(AF_INET);
static const bool supported = Network::Address::ipFamilySupported(AF_INET);
if (!supported) {
throw EnvoyException(
fmt::format("IPv4 addresses are not supported on this machine: {}", address));
Expand All @@ -43,7 +34,7 @@ void validateIpv4Supported(const std::string& address) {
// Validate that IPv6 is supported on this platform, raise an exception for the
// given address if not.
void validateIpv6Supported(const std::string& address) {
static const bool supported = ipFamilySupported(AF_INET6);
static const bool supported = Network::Address::ipFamilySupported(AF_INET6);
if (!supported) {
throw EnvoyException(
fmt::format("IPv6 addresses are not supported on this machine: {}", address));
Expand All @@ -52,6 +43,15 @@ void validateIpv6Supported(const std::string& address) {

} // namespace

// Check if an IP family is supported on this machine.
bool ipFamilySupported(int domain) {
const int fd = ::socket(domain, SOCK_STREAM, 0);
if (fd >= 0) {
RELEASE_ASSERT(::close(fd) == 0, "");
}
return fd != -1;
}

Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, socklen_t ss_len,
bool v6only) {
RELEASE_ASSERT(ss_len == 0 || ss_len >= sizeof(sa_family_t), "");
Expand Down
6 changes: 6 additions & 0 deletions source/common/network/address_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ namespace Envoy {
namespace Network {
namespace Address {

/**
* Returns true if the given family is supported on this machine.
* @param domain the IP family.
*/
bool ipFamilySupported(int domain);

/**
* Convert an address in the form of the socket address struct defined by Posix, Linux, etc. into
* a Network::Address::Instance and return a pointer to it. Raises an EnvoyException on failure.
Expand Down
13 changes: 8 additions & 5 deletions source/server/listener_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -380,11 +380,14 @@ void ListenerImpl::convertDestinationIPsMapToTrie() {
for (const auto& entry : destination_ips_map) {
std::vector<Network::Address::CidrRange> subnets;
if (entry.first == EMPTY_STRING) {
list.push_back(
std::make_pair<ServerNamesMapSharedPtr, std::vector<Network::Address::CidrRange>>(
std::make_shared<ServerNamesMap>(entry.second),
{Network::Address::CidrRange::create("0.0.0.0/0"),
Network::Address::CidrRange::create("::/0")}));
std::vector<Network::Address::CidrRange> cidr_ranges;
if (Network::Address::ipFamilySupported(AF_INET)) {
cidr_ranges.push_back(Network::Address::CidrRange::create("0.0.0.0/0"));
}
if (Network::Address::ipFamilySupported(AF_INET6)) {
cidr_ranges.push_back(Network::Address::CidrRange::create("::/0"));
}
Copy link
Contributor

@PiotrSikora PiotrSikora Jul 20, 2018

Choose a reason for hiding this comment

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

I think we need support IPv6-only setups as well:

diff --git a/source/server/listener_manager_impl.cc b/source/server/listener_manager_impl.cc
index 393fdfe1e..e69bebe4b 100644
--- a/source/server/listener_manager_impl.cc
+++ b/source/server/listener_manager_impl.cc
@@ -380,17 +380,19 @@ void ListenerImpl::convertDestinationIPsMapToTrie() {
     for (const auto& entry : destination_ips_map) {
       std::vector<Network::Address::CidrRange> subnets;
       if (entry.first == EMPTY_STRING) {
-        list.push_back(
-            std::make_pair<ServerNamesMapSharedPtr, std::vector<Network::Address::CidrRange>>(
-                std::make_shared<ServerNamesMap>(entry.second),
-                {Network::Address::CidrRange::create("0.0.0.0/0"),
-                 Network::Address::CidrRange::create("::/0")}));
+        if (Network::Address::ipFamilySupported(AF_INET)) {
+          subnets.push_back(Network::Address::CidrRange::create("0.0.0.0/0"));
+        }
+        if (Network::Address::ipFamilySupported(AF_INET6)) {
+          subnets.push_back(Network::Address::CidrRange::create("::/0"));
+        }
       } else {
-        list.push_back(
-            std::make_pair<ServerNamesMapSharedPtr, std::vector<Network::Address::CidrRange>>(
-                std::make_shared<ServerNamesMap>(entry.second),
-                {Network::Address::CidrRange::create(entry.first)}));
+        subnets.push_back(Network::Address::CidrRange::create(entry.first));
       }
+      list.push_back(
+          std::make_pair<ServerNamesMapSharedPtr, std::vector<Network::Address::CidrRange>>(
+              std::make_shared<ServerNamesMap>(entry.second),
+              std::vector<Network::Address::CidrRange>(subnets)));
     }
     destination_ips_pair.second = std::make_unique<DestinationIPsTrie>(list, true);
   }

(diff against master branch, since it's more readable)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @PiotrSikora

list.push_back({std::make_shared<ServerNamesMap>(entry.second), cidr_ranges});
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use the patch I provided (using unused subnets and sharing more of the code with the != EMPTY_STRING path)? It's a bit more readable, IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please check

} else {
list.push_back(
std::make_pair<ServerNamesMapSharedPtr, std::vector<Network::Address::CidrRange>>(
Expand Down