@lyft/network-team please review

@sbelair2 thanks I will take a look. Can you check the format CI job?

Regarding the failed test ci/circleci:format:

ERROR: From ./include/envoy/network/io_defs.h
ERROR: clang-format check failed for file: ./include/envoy/network/io_defs.h
ERROR: ./include/envoy/network/io_defs.h:56

And then:

tools/check_format.py fix ./include/envoy/network/io_defs.h
sh: clang-format-6.0: command not found
ERROR: From ././include/envoy/network/io_defs.h
ERROR: clang-format rewrite error: ././include/envoy/network/io_defs.h
ERROR: clang-format check failed for file: ././include/envoy/network/io_defs.h
ERROR:   ././include/envoy/network/io_defs.h:1
ERROR: check format failed. run 'tools/check_format.py fix'

Note that I have clang-format installed- I don’t know how to get clang-format-6.0 (?). Looking at io_defs.h, I see no format errors, either on line 1 or line 56.

@mattklein123: Suggestions?

Looking at the ci/circleci:asan failure, only one of 326 failed:

@envoy//test/integration:idle_timeout_integration_test FAILED in 28.9s
/build/tmp/_bazel_bazel/400406edc57d332f0b9b805d2b8e33a1/execroot/envoy/bazel-out/k8-dbg/testlogs/external/envoy/test/integration/idle_timeout_integration_test/test.log. The log has:

external/envoy/test/integration/idle_timeout_integration_test.cc:174: Failure
Expected equality of these values:
"aa"
response->body()
Which is: "a"
[ FAILED ] Protocols/IdleTimeoutIntegrationTest.PerStreamIdleTimeoutAfterBidiData/IPv4_HttpDownstream_Http2Upstream, where GetParam() = 12-byte object <00-00 00-00 00-00 00-00 01-00 00-00> (1099 ms)

I’m not sure what that failure is about…

I'm really excited about this effort; thanks for taking this on!

I'm wondering, though, if it would be more general to change widespread usage of integer handles to some abstract base class. IOHandle seems like a good name for such a class, and is consistent with some of your new naming. However, the PR as it stands still uses ints to pass around these descriptors.

I realize that the devil is in the details, especially when it comes to hooking it into libevent, which exposes the need for integer FDs. Nevertheless I think it might be possible to put the libevent/fd interface into virtual methods on a subclass of IOHandle (SocketIOHandle or something).

@cpakulski WDYT?

I'm wondering, though, if it would be more general to change widespread usage of integer handles to some abstract base class. IOHandle seems like a good name for such a class, and is consistent with some of your new naming. However, the PR as it stands still uses ints to pass around these descriptors.

+1, this is what I had in mind. I don't think continuing to pass around integer handles is really where we want to go. What @jmarantz mentions is where I thought we were heading with this change. Thoughts?

@jmarantz, @mattklein123: Thanks for the comments. To be honest, I wasn't sure what the ultimate intentions of the fd refactor were- all I had to go on was the original commit which introduced the fd. Let me get back to you on this- thanks!

Few thoughts after reviewing the changes. I tried to do something similar, but not exactly the same. My goal was to hide fd and talk to a socket via a class, let us name it EnvoySocket class. Such abstraction would allow to use real socket handles or simulate ones (handy in tests). I hit a wall at some point. As @jmarantz mentioned devil is hidden in details - infrastructure code calls system routines and they require "real" fds. Passing a simulated fd to such calls caused a crash. At some point I came to conclusion that abstraction must be taken one or two levels above and abstracting just Socket is not enough. By saying one or two levels up, I mean for example Dispatcher. Right now Dispatcher just uses system calls and expects real fd handles. It would have to be changed to accept EnvoySocket class and in your case ioHandle and would have to know how to act when type of IoHandle is IoHandle::SocketFd or IoHandle::SessionId. Good example is
address = Address::addressFromFd(socket.ioHandle());
It needs real socket descriptor handle. Moving forward would require abstracting Address class, so it works with all types of IoHandle. Meaning of Address itself would also change, it may not always be IP address.

@jmarantz, @mattklein123, @cpakulski: thanks much for the discussion. I'm not sure if everyone saw @cpakulski's response:

"Few thoughts after reviewing the changes. I tried to do something similar, but not exactly the same. My goal was to hide fd and talk to a socket via a class, let us name it EnvoySocket class. Such abstraction would allow to use real socket handles or simulate ones (handy in tests). I hit a wall at some point. As @jmarantz mentioned devil is hidden in details - infrastructure code calls system routines and they require "real" fds. Passing a simulated fd to such calls caused a crash. At some point I came to conclusion that abstraction must be taken one or two levels above and abstracting just Socket is not enough. By saying one or two levels up, I mean for example Dispatcher. Right now Dispatcher just uses system calls and expects real fd handles. It would have to be changed to accept EnvoySocket class and in your case ioHandle and would have to know how to act when type of IoHandle is IoHandle::SocketFd or IoHandle::SessionId. Good example is
address = Address::addressFromFd(socket.ioHandle());
It needs real socket descriptor handle. Moving forward would require abstracting Address class, so it works with all types of IoHandle. Meaning of Address itself would also change, it may not always be IP address."

It seems to me that the requirements can grow without end as we try to accommodate all use cases if we aren't careful. I'm wondering if maybe we should make IoHandle be really opaque and customizable at the same time, something like below.

In trying to come up with a non-integer handle in a generic IoHandle class, I wondered what should the type actually be of the handle? We agree that it being an int is too restrictive, but what should it be? I came to think that an IoHandle class should accommodate: (1) a pointer to an opaque type as a generic handle and (2) an integer value that is used when we force a handle into one of its eigenvalues, so to speak. For socket types, that is an int. For VPP sessions, we'll see that the int is a session id.

So the point is that the IoHandle has two generic and opaque pieces of private data: a pointer to whatever type, and an integer field, along with getters for either variable. Some parts of Envoy code might use the pointer variable, others might use the integer variable. Sample definition below, but note that I'm using void * below for discussion purposes, bear with me please... (full disclosure: I have been programming in C my whole career, primarily- coming up to speed with c++11 chops)

class IoHandle {
void *handle_;
int intValue_;

public:
IoHandle(void *hdl = nullptr, int iv = -1) : handle_(hdl), intValue_(iv) {}
void *ioHandle() const { return handle_; }
int intValue() const { return intValue_; }
};

Ok, the void * part. Really what I think is required is a template class for IoHandle, where we take a T * pointer for a class T, and an int: template<Class T*, int> IoHandle { ... }. I haven't worked out the code yet :-), but what I don't like about this is that we'll always have to be passing in a pointer to a class when we declare and construct IoHandles. Hence the void * above- it's a placeholder for proper code, and it conveys the opacity.

What I was thinking was that for future use, calling code can store context in the handle if it wants or needs to. We can even add another private datum: int data_, with a getter for it. Thus the calling code can either store an extra int for private use, or a pointer to some class. Either way, I see no need for the IoHandleType anymore, since a private data field can store the owner-specific state if it needs to, in addition to the substitute-for-fd intValue.

So, the void */maybe templated class is now:

class IoHandle {
void *handle_;
int intValue_;
int data_;
public:
IoHandle(void *hdl = nullptr, int iv = -1, int d = 0) : handle_(hdl), intValue_(iv), data_(d) {}
void *ioHandle() const { return handle_; }
int intValue() const { return intValue_; }
int private() { return data_; }
};

So, the way this would integrate into the current diffs: all references to IoHandleType will be removed, and all places in the code that have handle.ioHandle() will be replaced by handle.intValue(); The rest is open for future use, or not. Is the use of a template class for IoHandle too much a pain?

I hope I'm not in left field on this :-), comments please.

I think that's pretty close, but the C++-ish way to do that is make IOHandle a base class, with pure virtual methods covering all the system-calls you'd want to do with FDs (getsocketopt, read, write, libevent file-events).

There would be an FD-based derived class from IOHandle that would have a single member var int fd_; and implement all those pure methods.

Then let's say you were building a Windows version and the network primitives were some kind of abstract handle provided by that OS. You'd make another derived class that implements all those methods. You wouldn't need to use a void*; you could be specific about types in each derived class.

And you could do the same for a testing discipline which avoided making any OS calls. E.g. in the past I've built a MemFileSystem for use in tests, using a similar architecture, and a hash-table to represent files and their contents.

One tricky part is where we are dependent on using code we don't control (libevent) that has an FD interface. One solution I'd throw out there -- not that I love it but it's the first thing that comes to mind -- is to use threads to poll for non-fd related file/network events, and then get libevent to wake up the right callback via a timer.

What @jmarantz mentions is what I was envisioning. Basically, the internals of IOHandle are abstract, with methods on it to do various things.

I agree there are various details here that are tricky, for example the libevent case. IMO the way to handle this is actually to build IOHandle directly into Event::Dispatcher which is how we completely hide libevent from the rest of the code. Thus, the dispatcher can spit out IOHandles, addresses, connections, etc. If we do this I suspect it mostly "just works." I think this is what we want to do anyway for VPP since I'm pretty sure that ultimately VPP would need to implement its own dispatcher and move away from libevent, right? This would be the same for a DPDK solution.

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@mattklein123, @jmarantz, @htuch, @cpakulski: Re-opening this PR which was closed because it had become stale. My apologies for that- it took some time to understand the comments received. There is no new code, just proposed changes with the goal of continuing the discussion and the feedback.

The issue with refactoring the fd is the implicit assumption that the main interface to Envoy from the application is a socket. In this PR, our purpose is to abstract away sockets and all system calls pertaining to sockets from the I/O interface. The IoHandle represents the abstract interface for all I/O operations and the details of any particular interface (socket, VPP, etc) are contained in classes derived from IoHandle. To this end, we also define the IoAddress class and some derivatives, as well as the IoDescriptor class, which will replace the fd() calls when dealing with actual sockets.

These changes in this PR are a proposal and are intended to get feedback for the direction we are taking. Rather than show all the changes required to implement this new direction, we will also discuss towards the end how we anticipate further changes rippling through the code.

Note also that this PR does not address events yet, as previous comments suggested. The intention is to introduce further PRs that will address events and the dispatcher.

I. IoHandle and derivative classes

The following definitions would be contained in a new file io_defs.h.
io_defs.h (envoy/include/envoy/network):

// Generalize the SysCallResult to accommodate I/O operations that are not
// carried out by the kernel stack, hence not accomplished through a syscall

template struct IoCallResult : public struct IoResult {
T rc_;
int errno_;
};
typedef IoCallResult IoCallIntResult;
typedef IoCallResult<ssize_t> IoCallSizeResult;
typedef IoCallResult<void*> IoCallPtrResult;

template class IoDescriptor {
public:
IoDescriptor(T descriptor):descriptor_(descriptor) { }
virtual ~IoDescriptor() { };

T descriptor() const { return(descriptor_); }

operator(T) const { return(descriptor_);
bool operator == (const T& lhs) { return(descriptor_== lhs); }
bool operator != (const T& lhs) { return(descriptor_!=lhs); }

protected:
T descriptor_;
};

typedef IoDescriptor IoSocketDescriptor; // fd for sockets
typedef IoDescriptor<uint32_t> IoVppDescriptor; // for example…

// I/O options

struct IoOptionName { };
struct IoOptionValue { };

struct IoOptionNameSimple : public IoOptionName {
std::string name_;
};
struct IoOptionValueSimple : public IoOptionValue {
std::string value_;
};

struct IoSocketOptionName : public IoOptionName {
Int level_;
int name_;
};

struct IoSocketOptionValue : public IoOptionValue {
int value_;
};

class IoHandle {
public:
IoHandle() { }
virtual ~IoHandle() { }

// I/O Options API
virtual IoCallResult setOption(IoOptionName name, IoOptionValue value) PURE;
virtual const IoOptionValue& option(IoOptionName name) PURE;
virtual size_t options(vector<std::pair<IoOptionName,IoOptionValue>>& options) PURE;
virtual IoCallResult applyOptions() PURE;

virtual void setTransport(IoTransportPtr transport) PURE;
virtual IoTransportPtr transport() PURE;

// Calls made from Envoy::Network::Address::Instance
virtual IoCallResult bind() PURE;
virtual IoCallResult connect() PURE;
virtual IoCallResult close() PURE;

// Read/Write operations
virtual IoCallResult read(Buffer::Instance& buffer) PURE;
virtual IoCallResult write(Buffer::Instance& buffer, bool end_stream) PURE;

// Descriptor
const IoDescriptor& descriptor() const PURE;
};

// Socket derivative
class IoSocketHandle : public IoHandle {
public:
IoSocketHandle(const IoDescriptor& descriptor) : descriptor_(descriptor) { }
IoSocketHandle(const IoDescriptor& descriptor, const IoAddress& local_address)
:descriptor_(descriptor),
local_addr_(local_addr){ }

IoSocketHandle(const IoDescriptor& descriptor, const IoAddress& local_addr,
const IoAddress& remote_addr) : descriptor_(descriptor),
local_addr_(local_addr), remote_addr_(remote_addr) { }
virtual IoSocketHandle() { }
virtual ~IoSocketHandle() { }

// I/O Options API
virtual IoCallResult setOption(IoOptionName name, IoOptionValue value) override ;
virtual const IoOptionValue& option(IoOptionName name) override ;
virtual size_t options(vector<std::pair<IoOptionName,IoOptionValue>>& options) override ;
virtual IoCallResult applyOptions() override ;
virtual void setTransport(IoTransportPtr transport) override {
transport_socket_ = transport);
virtual IoTransportPtr transport() override { return transport_socket_; }

// Operations in Envoy::Network::Address::Instance
virtual IoCallResult bind() override ;
virtual IoCallResult connect() override ;
virtual IoCallResult close() override;
virtual IoCallResult socket() PURE;

// Read/Write operations
virtual IoCallResult read(Buffer::Instance& buffer) override ;
virtual IoCallResult write(Buffer::Instance& buffer, bool end_stream) override ;

const IoDescriptor& descriptor() const override { return(descriptor_); }

// Addresses (see include/network/address.h below)
const IoAdress& localAddress() const { return(local_addr_); }
const IoAdress& remoteAddress() const { return(remote_addr_); }
bool hasRemoteAddr() const { return !remote_addr_.isEmpty(); }

protected:
IoSocketDescriptor descriptor_;
IoAddress local_addr_;
IoAddress remote_addr_;
IoTransportSocket transport_socket_;
};

Similarly, we would define a new class IoHandleVpp that would accommodate differences required to interface to VPP. And it would have a customized IoTransportVpp member.

II. IoAddress and derivative classes

For the address classes, the intention was to remove all the methods using the fd. The IoAddress class is intended to replace the Instance class defined in address.h.

// in envoy/include/network/address.h:

enum class IoType { Stream, Datagram };
enum class IpVersion { v4, v6 };
enum class IoAddressType { Ipv4, Ipv6, Pipe };

class IoAddress // equivalent to class Instance in address,h
{
public:
virtual ~IoAddress() {}

virtual bool operator==(const IoAddress& rhs) const PURE;
bool operator!=(const IoAddress& rhs) const { return !operator==(rhs); }

virtual const std::string& asString() const PURE;
virtual const std::string& logicalName() const PURE;
  
virtual IoAddressType type() const PURE;
virtual bool isEmpty() const PURE;

}
typedef std::shared_ptr IoAddressConstSharedPtr;

// Replaces InstanceBase, InstanceIpv4, InstanceIpv6, IpHelper

template <typename T, typename U > class IoAddressBase : public IoAddress {
public:
IoAddressBase() { memset(&address_, 0, sizeof(address_)); }
IoAddressBase(T address) : address_(address) { }
IoAddressBase(const std::string& address) : friendly_address_(address)
{ memset(&address_, 0, sizeof(address_)); }
virtual ~IoAddressBase() { };

virtual const T& address() const { return address_; }
virtual U addressBytes() const PURE;
virtual uint32_t port() const PURE;

virtual const std::string& asString() const override { return friendly_name_; }

 virtual const std::string& logicalName() const override { return asString(); }
virtual T addressFromString(const std::string& address, uint32_t port=0) PURE;
virtual const std::string& addressToString() PURE;
virtual const std::string& addressAsString() const PURE;

virtual bool isAnyAddress() const PURE;
virtual bool isUnicastAddress() const PURE;

virtual IpVersion version() const PURE;

operator(T) const { return(address_); }
operator(std::string) const { return(asString()); }

bool operator == (const T& lhs) {
return (address_.addressBytes() == lhs.addressBytes()); }
bool operator != (const T& lhs) {
return address_.addressBytes() != lhs.addressBytes()); }

protected:

T    		 	          address_;
std::string    	 	friendly_address_;
std::string    	 	friendly_name_;
const IoAddressType 	type_;

};

// Implementations for IoAddressType. The details for Ipv4 sockets are given
// below, and the v6 socket address implementation is similar. The IoAddressPipe
// requires more thought still and is in progress.

class IoSockAddrIpv4 : public IoAddressBase<sockaddr_in, uint32_t> {
public:
explicit IoSockAddrIpv4(const sockaddr_in& address)
: public IoAddressBase(address),type_(Ipv4) { addressToString(); }
explicit IoSockAddrIpv4(const sockaddr_in* address)
: public IoAddressBase(*address),type_(Ipv4) { addressToString(); }
explicit IoSockAddrIpv4(const std::string& address)
: public IoAddressBase(address),type_(Ipv4)
{ address_= addressFromString(address); }
IoSockAddrIpv4(const std::string& address, uint32_t port)
: public IoAddressBase(address),type_(Ipv4)
{ address_= addressFromString(address,port); }
explicit IoSockAddrIpv4(uint32_t port):type_(Ipv4)
{ address_= addressFromString("",port); }

sockaddr_in address() const { return address_; }
uint32_t addressBytes() const { return address_.sin_addr.s_addr; }
uint32_t port() const override { return ntohs(address_.sin_port); }
bool isEmpty() const override { return addressBytes()==0 ; }
IoAddressType type() const override { return type_; }

// The following is unclear- this next method replaces socketFromSocketType() but
// maybe it should be somewhere else?
int socketFromIoType(IoType type) const;

const sockaddr_in& addressFromString(const std::string& address,
uint32_t port=0) override;
const std::string& addressToString() override;
const std::string& addressAsString() const override { return friendly_address_;
};

Note also that the methods addressFromFd() and peerAddressFromFd() should be methods in a derived class from a base class that has virtual methods addressFromIoHandle() and peerAddressFromIoHandle(), as opposed to addressFromSockAddr() which pertains only to sockets.

III. Transport Sockets

The transport sockets will be redefined by the class IoTransport, with derivatives for sockets and for VPP.

In envoy/include/envoy/network, replacing the file transport_socket.h with the new file io_transport.h:

class IoTransportCallbacks {
public:
virtual ~IoTransportCallbacks() {}

virtual IoDescriptor descriptor() const PURE;
virtual bool shouldDrainReadBuffer() PURE;
virtual void setReadBufferReady() PURE;

virtual Network::Connection& connection() PURE;
virtual void raiseEvent(ConnectionEvent event) PURE;
};
typedef std::unique_ptr IoTransportCallbacksPtr;

class IoTransport {
public:
virtual ~IoTransport() {}

virtual void setTransportCallbacks(IoTransportCallbacks& callbacks) PURE;
virtual std::string protocol() const PURE;

virtual bool canFlushClose() PURE;
virtual void close(Network::ConnectionEvent event) PURE;

virtual IoResult doRead(Buffer::Instance& buffer) PURE;
virtual IoResult doWrite(Buffer::Instance& buffer, bool end_stream) PURE;
virtual void onConnected() PURE;
};
typedef std::unique_ptr IoTransportPtr;

class IoTransportFactory {
public:
virtual ~IoTransportFactory() {}

virtual bool implementsSecureTransport() const PURE;
virtual IoTransportPtr createTransport() const PURE;
};
typedef std::unique_ptr IoTransportFactoryPtr;

Definitions of IoTransport for sockets (in a file in envoy/source/common/network/io_transport_socket.h?):

class IoTransportSocket : public IoTransport {
public:
virtual ~IoTransportSocket() {}
virtual const Ssl::Connection* ssl() const PURE;
};
typedef std::unique_ptr IoTransportSocketPtr;

And then, for example, in envoy/source/common/network/raw_buffer_socket.h;

class RawBufferSocket : public IoTransportSocket,
protected Logger::LoggableLogger::Id::connection {
public:
void setTransportCallbacks(IoTransportCallbacks& callbacks) override;
std::string protocol() const override;
bool canFlushClose() override { return true; }
void close(Network::ConnectionEvent) override {}
void onConnected() override;
IoResult doRead(Buffer::Instance& buffer) override;
IoResult doWrite(Buffer::Instance& buffer, bool end_stream) override;
const Ssl::Connection* ssl() const override { return nullptr; }

private:
IoTransportSocketCallbacks* callbacks_{};
bool shutdown_{};
};
typedef std::unique_ptr RawBufferSocketPtr;

IV. Further Notes

We anticipate that these proposed changes will require changes to the current existing classes:

Socket
SocketImpl (+derivatives)
TransportSocketFactory (+derivatives)
TransportSocket (+derivatives)
TransportSocketCallbacks (+derivatives)
RawBufferSocket
FilterManager (+derivatives)
FilterChainManager (+derivatives)
FilterChainFactory (+derivatives)
FilterChain (+derivatives)
Connection (+derivatives)
ConnectionImpl (+derivatives)
Listener (+derivatives)
ListenerImpl
AddressImpl
InstanceBase
InstanceIpv4
InstanceIpv6
IpHelper

2. Listener code and connection_handler code that moves newly-created sockets and transport sockets will change to do so with IoHandles. A few examples:

In ConnectionHandlerImpl::ActiveListener::newConnection:

Network::ConnectionPtr new_connection = parent_.dispatcher_.createServerConnection(std::move(ioHandle));

Note that an IoTransportPtr is not passed in because the IoTransport is contained in the IoHandle.

  b. FilterChainManager::findFilterChain(const Network::ConnectionSocket& socket) to findFilterChain(const Network::IoHandle& ioHandle) will have to use IoHandle methods, such as:

const auto filter_chain = config_.filterChainManager().findFilterChain(*ioHandle);

@mattklein123, @jmarantz, @htuch, @cpakulski: I tried to re-open the PR and saw no way to do it. Your advice is welcome.

@sbelair2 thanks for the proposal. Can you potentially code format it so it's easier to read? Or put into a gist or new PR that we can comment on? Even if only psuedo header files? Thank you! (I can reopen this PR but I don't think it's worth it).

@mattklein123: Sure, happy to. My apologies- I thought this was readable enough to get the idea across. I will generate a new PR today.