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

Add socket injection into client #147

Merged
merged 11 commits into from
Feb 15, 2022
Merged

Add socket injection into client #147

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c243cac
allow Socket injection into Client
itrofimow Jan 29, 2022
bcde298
add abstract base class for Socket
itrofimow Jan 31, 2022
14470fb
fix forwarding
itrofimow Jan 31, 2022
1bef85c
do handshake when initialized externally
itrofimow Feb 2, 2022
d612802
merge with upstream
itrofimow Feb 2, 2022
d7d5399
cleanup
itrofimow Feb 2, 2022
0dca48d
rework
itrofimow Feb 3, 2022
e26d785
oops
itrofimow Feb 3, 2022
aa8a7d0
build fix
itrofimow Feb 3, 2022
f0aa52b
cr fixes
itrofimow Feb 7, 2022
b173a4f
merge with upstream
itrofimow Feb 8, 2022
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
35 changes: 35 additions & 0 deletions clickhouse/base/socket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "socket.h"
#include "singleton.h"
#include "../client.h"

#include <assert.h>
#include <stdexcept>
Expand Down Expand Up @@ -213,6 +214,11 @@ const std::string & NetworkAddress::Host() const {
}


SocketBase::~SocketBase() = default;

SocketFactory::~SocketFactory() = default;


Socket::Socket(const NetworkAddress& addr)
: handle_(SocketConnect(addr))
{}
Expand Down Expand Up @@ -286,6 +292,35 @@ std::unique_ptr<OutputStream> Socket::makeOutputStream() const {
return std::make_unique<SocketOutput>(handle_);
}

NonSecureSocketFactory::~NonSecureSocketFactory() {}

std::unique_ptr<SocketBase> NonSecureSocketFactory::connect(const ClientOptions &opts) {
const auto address = NetworkAddress(opts.host, std::to_string(opts.port));

auto socket = doConnect(opts, address);
setSocketOptions(*socket, opts);

return socket;
}

std::unique_ptr<Socket> NonSecureSocketFactory::doConnect(const ClientOptions &opts,
const NetworkAddress& address) {
(void)opts;
return std::make_unique<Socket>(address);
}

void NonSecureSocketFactory::setSocketOptions(Socket &socket, const ClientOptions &opts) {
if (opts.tcp_keepalive) {
socket.SetTcpKeepAlive(
static_cast<int>(opts.tcp_keepalive_idle.count()),
static_cast<int>(opts.tcp_keepalive_intvl.count()),
static_cast<int>(opts.tcp_keepalive_cnt));
}
if (opts.tcp_nodelay) {
socket.SetTcpNoDelay(opts.tcp_nodelay);
}
}

SocketInput::SocketInput(SOCKET s)
: s_(s)
{
Expand Down
42 changes: 38 additions & 4 deletions clickhouse/base/socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ struct addrinfo;

namespace clickhouse {

struct ClientOptions;

/** Address of a host to establish connection to.
*
*/
Expand Down Expand Up @@ -57,13 +59,31 @@ class windowsErrorCategory : public std::error_category {

#endif

class Socket {

class SocketBase {
public:
virtual ~SocketBase();

virtual std::unique_ptr<InputStream> makeInputStream() const = 0;
virtual std::unique_ptr<OutputStream> makeOutputStream() const = 0;
};


class SocketFactory {
Copy link
Collaborator

Choose a reason for hiding this comment

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

To simplify issue with reconnecting and timeouts (and to provide a framework for upcoming multiple-hosts #139 ). Please consider updating the interface:

class SocketFactory {
        public:
    virtual ~SocketFactory();

    // TODO: move connection-related options to ConnectionOptions structure.

    virtual std::unique_ptr<SocketBase> Connect(const NetworkAddress &, const ClientOptions &) = 0;
    // reconnects to a given address, maybe with a timeout or with any special procedures.
    virtual std::unique_ptr<SocketBase> ReConnect(const NetworkAddress &, const ClientOptions &) = 0;
};

Copy link
Contributor Author

@itrofimow itrofimow Feb 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There's a problem with NetworkAddress - its constructor uses blocking getaddrinfo; one way of soling this is to make some base class for NetworkAddress, but i feel like this would complicate the interface too much (and we already have some Java-style abstract factories here)

Introducing ConnectionOptions seems like a good enhancement for me, but i believe it has to be done in #139, or at least on the receiving end of upcoming merge conflict. Since SocketFactory is a new feature, there shouldn't be a concern with backwards compatibility

public:
virtual ~SocketFactory();

virtual std::unique_ptr<SocketBase> connect(const ClientOptions& opts) = 0;
};


class Socket : public SocketBase {
public:
Socket(const NetworkAddress& addr);
Socket(Socket&& other) noexcept;
Socket& operator=(Socket&& other) noexcept;

virtual ~Socket();
~Socket() override;

/// @params idle the time (in seconds) the connection needs to remain
/// idle before TCP starts sending keepalive probes.
Expand All @@ -75,8 +95,8 @@ class Socket {
/// @params nodelay whether to enable TCP_NODELAY
void SetTcpNoDelay(bool nodelay) noexcept;

virtual std::unique_ptr<InputStream> makeInputStream() const;
virtual std::unique_ptr<OutputStream> makeOutputStream() const;
std::unique_ptr<InputStream> makeInputStream() const override;
std::unique_ptr<OutputStream> makeOutputStream() const override;

protected:
Socket(const Socket&) = delete;
Expand All @@ -87,6 +107,20 @@ class Socket {
};


class NonSecureSocketFactory : public SocketFactory {
public:
~NonSecureSocketFactory() override;

std::unique_ptr<SocketBase> connect(const ClientOptions& opts) override;

protected:
virtual std::unique_ptr<Socket> doConnect(const ClientOptions& opts,
const NetworkAddress& address);

void setSocketOptions(Socket& socket, const ClientOptions& opts);
};


class SocketInput : public InputStream {
public:
explicit SocketInput(SOCKET s);
Expand Down
34 changes: 32 additions & 2 deletions clickhouse/base/sslsocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include "sslsocket.h"
#include "../client.h"

#include <stdexcept>

Expand Down Expand Up @@ -135,9 +136,11 @@ SSL_CTX * SSLContext::getContext() {
<< "\n\t handshake state: " << SSL_get_state(ssl_) \
<< std::endl
*/
SSLSocket::SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params, SSLContext& context)
SSLSocket::SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params,
std::unique_ptr<SSLContext> context)
: Socket(addr)
, ssl_(SSL_new(context.getContext()), &SSL_free)
, context_(std::move(context))
, ssl_(SSL_new(context_->getContext()), &SSL_free)
{
auto ssl = ssl_.get();
if (!ssl)
Expand Down Expand Up @@ -181,6 +184,33 @@ SSLSocket::SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params, S
}
}

SSLSocketFactory::~SSLSocketFactory() {}

#if defined(WITH_OPENSSL)
std::unique_ptr<Socket> SSLSocketFactory::doConnect(const ClientOptions& opts,
const NetworkAddress& address) {
std::unique_ptr<SSLContext> ssl_context;
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO it would make sense to store SSLContext in a SSLSocketFactory and create it only once: in constructor.

const auto ssl_options = opts.ssl_options;
const auto ssl_params = SSLParams{
ssl_options.path_to_ca_files,
ssl_options.path_to_ca_directory,
ssl_options.use_default_ca_locations,
ssl_options.context_options,
ssl_options.min_protocol_version,
ssl_options.max_protocol_version,
ssl_options.use_sni
};

if (ssl_options.ssl_context)
ssl_context = std::make_unique<SSLContext>(*ssl_options.ssl_context);
else {
ssl_context = std::make_unique<SSLContext>(ssl_params);
}

return std::make_unique<SSLSocket>(address, ssl_params, std::move(ssl_context));
}
#endif

std::unique_ptr<InputStream> SSLSocket::makeInputStream() const {
return std::make_unique<SSLSocketInput>(ssl_.get());
}
Expand Down
17 changes: 15 additions & 2 deletions clickhouse/base/sslsocket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,10 @@ class SSLContext

class SSLSocket : public Socket {
public:
explicit SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params, SSLContext& context);
explicit SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params,
std::unique_ptr<SSLContext> context);
SSLSocket(SSLSocket &&) = default;
~SSLSocket() = default;
~SSLSocket() override = default;

SSLSocket(const SSLSocket & ) = delete;
SSLSocket& operator=(const SSLSocket & ) = delete;
Expand All @@ -53,9 +54,21 @@ class SSLSocket : public Socket {
std::unique_ptr<OutputStream> makeOutputStream() const override;

private:
std::unique_ptr<SSLContext> context_;
std::unique_ptr<SSL, void (*)(SSL *s)> ssl_;
};

class SSLSocketFactory : public NonSecureSocketFactory {
public:
~SSLSocketFactory() override;

#if defined(WITH_OPENSSL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The whole file is not event visible to the build-system, if WITH_OPENSSL is not defined. Please remove the #if defined/#endif braces.

protected:
std::unique_ptr<Socket> doConnect(const ClientOptions& opts,
const NetworkAddress& address) override;
#endif
};

class SSLSocketInput : public InputStream {
public:
explicit SSLSocketInput(SSL *ssl);
Expand Down
Loading