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

Add an unused implementation NoOpTransportSocketCallbacks #4172

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions include/envoy/network/transport_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,11 @@ class TransportSocket {
* @return the const SSL connection data if this is an SSL connection, or nullptr if it is not.
*/
virtual const Ssl::Connection* ssl() const PURE;

/**
* @return the TransportSocketCallbacks object passed in through setTransportSocketCallbacks().
*/
virtual TransportSocketCallbacks* callbacks() PURE;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this? (see comments on NoOpTransportSocketCallbacks too)

};

typedef std::unique_ptr<TransportSocket> TransportSocketPtr;
Expand Down
1 change: 1 addition & 0 deletions source/common/network/raw_buffer_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class RawBufferSocket : public TransportSocket, protected Logger::Loggable<Logge
IoResult doWrite(Buffer::Instance& buffer, bool end_stream) override;
Ssl::Connection* ssl() override { return nullptr; }
const Ssl::Connection* ssl() const override { return nullptr; }
TransportSocketCallbacks* callbacks() { return callbacks_; }

private:
TransportSocketCallbacks* callbacks_{};
Expand Down
1 change: 1 addition & 0 deletions source/common/ssl/ssl_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class SslSocket : public Network::TransportSocket,
void onConnected() override;
Ssl::Connection* ssl() override { return this; }
const Ssl::Connection* ssl() const override { return this; }
Network::TransportSocketCallbacks* callbacks() { return callbacks_; }

SSL* rawSslForTest() { return ssl_.get(); }

Expand Down
6 changes: 6 additions & 0 deletions source/extensions/transport_sockets/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,9 @@ envoy_cc_library(
"//source/common/singleton:const_singleton",
],
)

envoy_cc_library(
name = "noop_transport_socket_callbacks_lib",
hdrs = ["noop_transport_socket_callbacks.h"],
deps = ["//include/envoy/network:transport_socket_interface"],
)
6 changes: 6 additions & 0 deletions source/extensions/transport_sockets/alts/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,9 @@ envoy_cc_library(
"//source/common/buffer:buffer_lib",
],
)

envoy_cc_library(
name = "noop_transport_socket_callbacks_lib",
hdrs = ["noop_transport_socket_callbacks.h"],
deps = ["//include/envoy/network:transport_socket_interface"],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#include "envoy/network/transport_socket.h"

namespace Envoy {
namespace Extensions {
namespace TransportSockets {
namespace Alts {

/**
* A TransportSocketCallbacks for wrapped TransportSocket object. Some
* TransportSocket implementation wraps another socket which does actual I/O.
* This class is used by the wrapped socket as its callbacks instead of the real
* connection to hold back callbacks from the underlying socket to connection.
*/
class NoOpTransportSocketCallbacks : public Network::TransportSocketCallbacks {
public:
explicit NoOpTransportSocketCallbacks(Network::TransportSocket* parent) : parent_(parent) {}
Copy link
Member

Choose a reason for hiding this comment

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

All you need is parent_->callbacks(), so why just not taking a Network::TransportSocketCallbacks& as parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on the timing when NoOpTransportSocketCallbacks is initialized and parent socket's setTransportSocketCallbacks() is called. In #4153 it is initialized before parent knows its callbacks. It can take a TransportSocketCallbacks** too, but I feel not much more simpler.


int fd() const override { return parent_->callbacks()->fd(); }
Network::Connection& connection() override { return parent_->callbacks()->connection(); }
bool shouldDrainReadBuffer() override { return false; }
/*
* No-op for these two methods to hold back the callbacks.
*/
void setReadBufferReady() override {}
void raiseEvent(Network::ConnectionEvent) override {}

private:
Network::TransportSocket* parent_;
};

} // namespace Alts
} // namespace TransportSockets
} // namespace Extensions
} // namespace Envoy
1 change: 1 addition & 0 deletions source/extensions/transport_sockets/capture/capture.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ class CaptureSocket : public Network::TransportSocket {
void onConnected() override;
Ssl::Connection* ssl() override;
const Ssl::Connection* ssl() const override;
Network::TransportSocketCallbacks* callbacks() { return callbacks_; }

private:
const std::string& path_prefix_;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#include "third_party/envoy/src/include/envoy/network/transport_socket.h"

namespace Envoy {
namespace Extensions {
namespace TransportSockets {

/**
* A TransportSocketCallbacks for wrapped TransportSocket object. Some
* TransportSocket implementation wraps another socket which does actual I/O.
* This class is used by the wrapped socket as its callbacks instead of the real
* connection to hold back callbacks from the underlying socket to connection.
*/
class NoOpTransportSocketCallbacks : public Network::TransportSocketCallbacks {
public:
explicit NoOpTransportSocketCallbacks(Network::TransportSocket* parent) : parent_(parent) {}

int fd() const override { return parent_->callbacks()->fd(); }
Network::Connection& connection() override { return parent_->callbacks()->connection(); }
bool shouldDrainReadBuffer() override { return false; }
/*
* No-op for these two methods to hold back the callbacks.
*/
void setReadBufferReady() override {}
void raiseEvent(Network::ConnectionEvent) override {}

private:
Network::TransportSocket* parent_;
};

} // namespace TransportSockets
} // namespace Extensions
} // Envoy
10 changes: 10 additions & 0 deletions test/extensions/transport_sockets/alts/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,13 @@ envoy_extension_cc_test(
"//test/mocks/event:event_mocks",
],
)

envoy_extension_cc_test(
name = "noop_transport_socket_callbacks_test",
srcs = ["noop_transport_socket_callbacks_test.cc"],
extension_name = "envoy.transport_sockets.alts",
deps = [
"//source/extensions/transport_sockets/alts:noop_transport_socket_callbacks_lib",
"//test/mocks/network:network_mocks",
],
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
#include "extensions/transport_sockets/alts/noop_transport_socket_callbacks.h"

#include "test/mocks/network/mocks.h"

#include "gtest/gtest.h"

namespace Envoy {
namespace Extensions {
namespace TransportSockets {
namespace Alts {
namespace {

class TestTransportSocketCallbacks : public Network::TransportSocketCallbacks {
public:
TestTransportSocketCallbacks(Network::Connection* connection) : connection_(connection) {}

int fd() const override { return 1; }
Network::Connection& connection() override { return *connection_; }
bool shouldDrainReadBuffer() override { return false; }
void setReadBufferReady() override { set_read_buffer_ready_ = true; }
void raiseEvent(Network::ConnectionEvent) override { event_raised_ = true; }

bool event_raised() const { return event_raised_; }
bool set_read_buffer_ready() const { return set_read_buffer_ready_; }

private:
bool event_raised_{false};
bool set_read_buffer_ready_{false};
Network::Connection* connection_;
};

class NoOpTransportSocketCallbacksTest : public testing::Test {
protected:
NoOpTransportSocketCallbacksTest()
: wrapper_callbacks_(&connection_), wrapped_callbacks_(&wrapper_socket_) {
wrapper_socket_.setTransportSocketCallbacks(wrapper_callbacks_);
}

Network::MockConnection connection_;
TestTransportSocketCallbacks wrapper_callbacks_;
Network::MockTransportSocket wrapper_socket_;
NoOpTransportSocketCallbacks wrapped_callbacks_;
};

TEST_F(NoOpTransportSocketCallbacksTest, TestAllCallbacks) {
EXPECT_EQ(wrapper_callbacks_.fd(), wrapped_callbacks_.fd());
EXPECT_EQ(&connection_, &wrapped_callbacks_.connection());
EXPECT_FALSE(wrapped_callbacks_.shouldDrainReadBuffer());

wrapped_callbacks_.setReadBufferReady();
EXPECT_FALSE(wrapper_callbacks_.set_read_buffer_ready());
wrapped_callbacks_.raiseEvent(Network::ConnectionEvent::Connected);
EXPECT_FALSE(wrapper_callbacks_.event_raised());
}

} // namespace
} // namespace Alts
} // namespace TransportSockets
} // namespace Extensions
} // namespace Envoy
6 changes: 5 additions & 1 deletion test/mocks/network/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ class MockTransportSocket : public TransportSocket {
MockTransportSocket();
~MockTransportSocket();

MOCK_METHOD1(setTransportSocketCallbacks, void(TransportSocketCallbacks& callbacks));
void setTransportSocketCallbacks(TransportSocketCallbacks& callbacks) { callbacks_ = &callbacks; }
MOCK_CONST_METHOD0(protocol, std::string());
MOCK_METHOD0(canFlushClose, bool());
MOCK_METHOD1(closeSocket, void(Network::ConnectionEvent event));
Expand All @@ -437,6 +437,10 @@ class MockTransportSocket : public TransportSocket {
MOCK_METHOD0(onConnected, void());
MOCK_METHOD0(ssl, Ssl::Connection*());
MOCK_CONST_METHOD0(ssl, const Ssl::Connection*());
Network::TransportSocketCallbacks* callbacks() { return callbacks_; }

private:
Network::TransportSocketCallbacks* callbacks_;
};

class MockTransportSocketFactory : public TransportSocketFactory {
Expand Down