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.

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

network: address socket interface pointer instead of name #12550

Merged
merged 5 commits into from
Aug 12, 2020
Merged

network: address socket interface pointer instead of name #12550

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9686883
network: address socket interface pointer instead of name
florincoras Aug 7, 2020
9397307
use default socket interface if none configured
florincoras Aug 10, 2020
da42c44
sock interface reference instead of pointer
florincoras Aug 11, 2020
0baf93e
Merge branch 'master' into addr_sock
florincoras Aug 11, 2020
88bdf78
const ref InstanceBase SocketInterface field
florincoras Aug 11, 2020
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
8 changes: 6 additions & 2 deletions include/envoy/network/address.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@

namespace Envoy {
namespace Network {

/* Forward declaration */
class SocketInterface;

namespace Address {

/**
Expand Down Expand Up @@ -178,9 +182,9 @@ class Instance {
virtual Type type() const PURE;

/**
* @return name of socket interface that should be used with this address
* @return SocketInterface to be used with the address
*/
virtual const std::string& socketInterface() const PURE;
virtual const Network::SocketInterface* socketInterface() const PURE;
Copy link
Member

Choose a reason for hiding this comment

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

let's return reference, it's no longer nullable.

Copy link
Member Author

Choose a reason for hiding this comment

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

We'll have to keep the const because of how we pass the SocketInterface pointer to the Address. Is that okay or were you thinking about something more complex?

Copy link
Member

Choose a reason for hiding this comment

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

ah of couse, const wherever possible :)

};

using InstanceConstSharedPtr = std::shared_ptr<const Instance>;
Expand Down
44 changes: 24 additions & 20 deletions source/common/network/address_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ std::string friendlyNameFromAbstractPath(absl::string_view path) {

} // namespace

static inline const SocketInterface* sockInterfaceOrDefault(const SocketInterface* sock_interface) {
Copy link
Member

Choose a reason for hiding this comment

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

style nit: put this in unnamed namespace above, remove static, doesn't have to be inline?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a C reflex of mine that I use instead of macros. Goal would be to convince the compiler to avoid extra function calls when not needed. But obviously, no strong opinion here ...

Copy link
Member

Choose a reason for hiding this comment

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

C++ inline isn't guaranteed to be inlined, all are decided by compiler, it's more of working around one-definition rule.

Copy link
Member Author

Choose a reason for hiding this comment

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

Understood!

return sock_interface == nullptr ? &SocketInterfaceSingleton::get() : sock_interface;
}

Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss, socklen_t ss_len,
bool v6only) {
RELEASE_ASSERT(ss_len == 0 || static_cast<unsigned int>(ss_len) >= sizeof(sa_family_t), "");
Expand Down Expand Up @@ -91,8 +95,8 @@ Address::InstanceConstSharedPtr addressFromSockAddr(const sockaddr_storage& ss,
NOT_REACHED_GCOVR_EXCL_LINE;
}

Ipv4Instance::Ipv4Instance(const sockaddr_in* address, absl::string_view sock_interface)
: InstanceBase(Type::Ip, sock_interface) {
Ipv4Instance::Ipv4Instance(const sockaddr_in* address, const SocketInterface* sock_interface)
: InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) {
ip_.ipv4_.address_ = *address;
ip_.friendly_address_ = sockaddrToString(*address);

Expand All @@ -105,12 +109,12 @@ Ipv4Instance::Ipv4Instance(const sockaddr_in* address, absl::string_view sock_in
validateIpv4Supported(friendly_name_);
}

Ipv4Instance::Ipv4Instance(const std::string& address, absl::string_view sock_interface)
: Ipv4Instance(address, 0, sock_interface) {}
Ipv4Instance::Ipv4Instance(const std::string& address, const SocketInterface* sock_interface)
: Ipv4Instance(address, 0, sockInterfaceOrDefault(sock_interface)) {}

Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port,
absl::string_view sock_interface)
: InstanceBase(Type::Ip, sock_interface) {
const SocketInterface* sock_interface)
: InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) {
memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_));
ip_.ipv4_.address_.sin_family = AF_INET;
ip_.ipv4_.address_.sin_port = htons(port);
Expand All @@ -124,8 +128,8 @@ Ipv4Instance::Ipv4Instance(const std::string& address, uint32_t port,
ip_.friendly_address_ = address;
}

Ipv4Instance::Ipv4Instance(uint32_t port, absl::string_view sock_interface)
: InstanceBase(Type::Ip, sock_interface) {
Ipv4Instance::Ipv4Instance(uint32_t port, const SocketInterface* sock_interface)
: InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) {
memset(&ip_.ipv4_.address_, 0, sizeof(ip_.ipv4_.address_));
ip_.ipv4_.address_.sin_family = AF_INET;
ip_.ipv4_.address_.sin_port = htons(port);
Expand Down Expand Up @@ -187,21 +191,21 @@ std::string Ipv6Instance::Ipv6Helper::makeFriendlyAddress() const {
}

Ipv6Instance::Ipv6Instance(const sockaddr_in6& address, bool v6only,
absl::string_view sock_interface)
: InstanceBase(Type::Ip, sock_interface) {
const SocketInterface* sock_interface)
: InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) {
ip_.ipv6_.address_ = address;
ip_.friendly_address_ = ip_.ipv6_.makeFriendlyAddress();
ip_.ipv6_.v6only_ = v6only;
friendly_name_ = fmt::format("[{}]:{}", ip_.friendly_address_, ip_.port());
validateIpv6Supported(friendly_name_);
}

Ipv6Instance::Ipv6Instance(const std::string& address, absl::string_view sock_interface)
: Ipv6Instance(address, 0, sock_interface) {}
Ipv6Instance::Ipv6Instance(const std::string& address, const SocketInterface* sock_interface)
: Ipv6Instance(address, 0, sockInterfaceOrDefault(sock_interface)) {}

Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port,
absl::string_view sock_interface)
: InstanceBase(Type::Ip, sock_interface) {
const SocketInterface* sock_interface)
: InstanceBase(Type::Ip, sockInterfaceOrDefault(sock_interface)) {
ip_.ipv6_.address_.sin6_family = AF_INET6;
ip_.ipv6_.address_.sin6_port = htons(port);
if (!address.empty()) {
Expand All @@ -217,8 +221,8 @@ Ipv6Instance::Ipv6Instance(const std::string& address, uint32_t port,
validateIpv6Supported(friendly_name_);
}

Ipv6Instance::Ipv6Instance(uint32_t port, absl::string_view sock_interface)
: Ipv6Instance("", port, sock_interface) {}
Ipv6Instance::Ipv6Instance(uint32_t port, const SocketInterface* sock_interface)
: Ipv6Instance("", port, sockInterfaceOrDefault(sock_interface)) {}

bool Ipv6Instance::operator==(const Instance& rhs) const {
const auto* rhs_casted = dynamic_cast<const Ipv6Instance*>(&rhs);
Expand All @@ -227,8 +231,8 @@ bool Ipv6Instance::operator==(const Instance& rhs) const {
}

PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t mode,
absl::string_view sock_interface)
: InstanceBase(Type::Pipe, sock_interface) {
const SocketInterface* sock_interface)
: InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) {
if (address->sun_path[0] == '\0') {
#if !defined(__linux__)
throw EnvoyException("Abstract AF_UNIX sockets are only supported on linux.");
Expand All @@ -253,8 +257,8 @@ PipeInstance::PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t
}

PipeInstance::PipeInstance(const std::string& pipe_path, mode_t mode,
absl::string_view sock_interface)
: InstanceBase(Type::Pipe, sock_interface) {
const SocketInterface* sock_interface)
: InstanceBase(Type::Pipe, sockInterfaceOrDefault(sock_interface)) {
if (pipe_path.size() >= sizeof(pipe_.address_.sun_path)) {
throw EnvoyException(
fmt::format("Path \"{}\" exceeds maximum UNIX domain socket path size of {}.", pipe_path,
Expand Down
35 changes: 20 additions & 15 deletions source/common/network/address_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "envoy/common/platform.h"
#include "envoy/network/address.h"
#include "envoy/network/socket.h"

namespace Envoy {
namespace Network {
Expand Down Expand Up @@ -37,16 +38,15 @@ class InstanceBase : public Instance {
const std::string& logicalName() const override { return asString(); }
Type type() const override { return type_; }

const std::string& socketInterface() const override { return socket_interface_; }
const SocketInterface* socketInterface() const override { return socket_interface_; }

protected:
InstanceBase(Type type) : type_(type) {}
InstanceBase(Type type, absl::string_view sock_interface) : type_(type) {
socket_interface_ = std::string(sock_interface);
InstanceBase(Type type, const SocketInterface* sock_interface) : type_(type) {
socket_interface_ = sock_interface;
}

std::string friendly_name_;
std::string socket_interface_;
const SocketInterface* socket_interface_;
Copy link
Member

Choose a reason for hiding this comment

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

This can be const ref as well

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!


private:
const Type type_;
Expand All @@ -60,23 +60,26 @@ class Ipv4Instance : public InstanceBase {
/**
* Construct from an existing unix IPv4 socket address (IP v4 address and port).
*/
explicit Ipv4Instance(const sockaddr_in* address, absl::string_view sock_interface = "");
explicit Ipv4Instance(const sockaddr_in* address,
const SocketInterface* sock_interface = nullptr);

/**
* Construct from a string IPv4 address such as "1.2.3.4". Port will be unset/0.
*/
explicit Ipv4Instance(const std::string& address, absl::string_view sock_interface = "");
explicit Ipv4Instance(const std::string& address,
const SocketInterface* sock_interface = nullptr);

/**
* Construct from a string IPv4 address such as "1.2.3.4" as well as a port.
*/
Ipv4Instance(const std::string& address, uint32_t port, absl::string_view sock_interface = "");
Ipv4Instance(const std::string& address, uint32_t port,
const SocketInterface* sock_interface = nullptr);

/**
* Construct from a port. The IPv4 address will be set to "any" and is suitable for binding
* a port to any available address.
*/
explicit Ipv4Instance(uint32_t port, absl::string_view sock_interface = "");
explicit Ipv4Instance(uint32_t port, const SocketInterface* sock_interface = nullptr);

// Network::Address::Instance
bool operator==(const Instance& rhs) const override;
Expand Down Expand Up @@ -131,23 +134,25 @@ class Ipv6Instance : public InstanceBase {
* Construct from an existing unix IPv6 socket address (IP v6 address and port).
*/
Ipv6Instance(const sockaddr_in6& address, bool v6only = true,
absl::string_view sock_interface = "");
const SocketInterface* sock_interface = nullptr);

/**
* Construct from a string IPv6 address such as "12:34::5". Port will be unset/0.
*/
explicit Ipv6Instance(const std::string& address, absl::string_view sock_interface = "");
explicit Ipv6Instance(const std::string& address,
const SocketInterface* sock_interface = nullptr);

/**
* Construct from a string IPv6 address such as "12:34::5" as well as a port.
*/
Ipv6Instance(const std::string& address, uint32_t port, absl::string_view sock_interface = "");
Ipv6Instance(const std::string& address, uint32_t port,
const SocketInterface* sock_interface = nullptr);

/**
* Construct from a port. The IPv6 address will be set to "any" and is suitable for binding
* a port to any available address.
*/
explicit Ipv6Instance(uint32_t port, absl::string_view sock_interface = "");
explicit Ipv6Instance(uint32_t port, const SocketInterface* sock_interface = nullptr);

// Network::Address::Instance
bool operator==(const Instance& rhs) const override;
Expand Down Expand Up @@ -203,13 +208,13 @@ class PipeInstance : public InstanceBase {
* Construct from an existing unix address.
*/
explicit PipeInstance(const sockaddr_un* address, socklen_t ss_len, mode_t mode = 0,
absl::string_view sock_interface = "");
const SocketInterface* sock_interface = nullptr);

/**
* Construct from a string pipe path.
*/
explicit PipeInstance(const std::string& pipe_path, mode_t mode = 0,
absl::string_view sock_interface = "");
const SocketInterface* sock_interface = nullptr);

// Network::Address::Instance
bool operator==(const Instance& rhs) const override;
Expand Down
10 changes: 2 additions & 8 deletions source/common/network/socket_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,8 @@ using SocketInterfaceLoader = ScopedInjectableLoader<SocketInterface>;
*/
static inline IoHandlePtr ioHandleForAddr(Socket::Type type,
const Address::InstanceConstSharedPtr addr) {
auto sock_interface_name = addr->socketInterface();
if (!sock_interface_name.empty()) {
auto sock_interface = socketInterface(sock_interface_name);
RELEASE_ASSERT(sock_interface != nullptr,
fmt::format("missing socket interface {}", sock_interface_name));
return sock_interface->socket(type, addr);
}
return SocketInterfaceSingleton::get().socket(type, addr);
auto sock_interface = addr->socketInterface();
return sock_interface->socket(type, addr);
}

} // namespace Network
Expand Down
5 changes: 3 additions & 2 deletions source/common/network/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,11 +330,12 @@ bool Utility::isLoopbackAddress(const Address::Instance& address) {

Address::InstanceConstSharedPtr Utility::getCanonicalIpv4LoopbackAddress() {
CONSTRUCT_ON_FIRST_USE(Address::InstanceConstSharedPtr,
new Address::Ipv4Instance("127.0.0.1", 0));
new Address::Ipv4Instance("127.0.0.1", 0, nullptr));
}

Address::InstanceConstSharedPtr Utility::getIpv6LoopbackAddress() {
CONSTRUCT_ON_FIRST_USE(Address::InstanceConstSharedPtr, new Address::Ipv6Instance("::1", 0));
CONSTRUCT_ON_FIRST_USE(Address::InstanceConstSharedPtr,
new Address::Ipv6Instance("::1", 0, nullptr));
}

Address::InstanceConstSharedPtr Utility::getIpv4AnyAddress() {
Expand Down
12 changes: 6 additions & 6 deletions test/common/network/connection_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1082,11 +1082,11 @@ TEST_P(ConnectionImplTest, BindTest) {
std::string address_string = TestUtility::getIpv4Loopback();
if (GetParam() == Network::Address::IpVersion::v4) {
source_address_ = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv4Instance(address_string, 0)};
new Network::Address::Ipv4Instance(address_string, 0, nullptr)};
} else {
address_string = "::1";
source_address_ = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv6Instance(address_string, 0)};
new Network::Address::Ipv6Instance(address_string, 0, nullptr)};
}
setUpBasicConnection();
connect();
Expand All @@ -1100,11 +1100,11 @@ TEST_P(ConnectionImplTest, BindFromSocketTest) {
Address::InstanceConstSharedPtr new_source_address;
if (GetParam() == Network::Address::IpVersion::v4) {
new_source_address = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv4Instance(address_string, 0)};
new Network::Address::Ipv4Instance(address_string, 0, nullptr)};
} else {
address_string = "::1";
new_source_address = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv6Instance(address_string, 0)};
new Network::Address::Ipv6Instance(address_string, 0, nullptr)};
}
auto option = std::make_shared<NiceMock<MockSocketOption>>();
EXPECT_CALL(*option, setOption(_, Eq(envoy::config::core::v3::SocketOption::STATE_PREBIND)))
Expand All @@ -1126,11 +1126,11 @@ TEST_P(ConnectionImplTest, BindFailureTest) {
if (GetParam() == Network::Address::IpVersion::v6) {
const std::string address_string = TestUtility::getIpv4Loopback();
source_address_ = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv4Instance(address_string, 0)};
new Network::Address::Ipv4Instance(address_string, 0, nullptr)};
} else {
const std::string address_string = "::1";
source_address_ = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv6Instance(address_string, 0)};
new Network::Address::Ipv6Instance(address_string, 0, nullptr)};
}
dispatcher_ = api_->allocateDispatcher("test_thread");
socket_ = std::make_shared<Network::TcpListenSocket>(Network::Test::getAnyAddress(GetParam()),
Expand Down
4 changes: 2 additions & 2 deletions test/common/network/dns_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,12 +393,12 @@ class CustomInstance : public Address::Instance {
const sockaddr* sockAddr() const override { return instance_.sockAddr(); }
socklen_t sockAddrLen() const override { return instance_.sockAddrLen(); }
Address::Type type() const override { return instance_.type(); }
const std::string& socketInterface() const override { return socket_interface_; }
const SocketInterface* socketInterface() const override { return socket_interface_; }

private:
std::string antagonistic_name_;
Address::Ipv4Instance instance_;
std::string socket_interface_{""};
const SocketInterface* socket_interface_{nullptr};
};

TEST_F(DnsImplConstructor, SupportCustomAddressInstances) {
Expand Down
Loading