Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would remove this as I think where you need get callbacks is in the scope you set it. No strong opinion on it though. If this is needed internally, ping me on hangout.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

};

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() override { 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() override { return callbacks_; }

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

Expand Down
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
Copy Markdown
Member

Choose a reason for hiding this comment

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

#4172 (comment)

Yeah if you delay creating TransportSocketCallbacks, i.e. in parent setTransportSocketCallbacks, you should be able to just take TransportSocketCallbacks& here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good point! Done.
I still keep the callbacks() in TransportSocket interface, as it's convenient to access to callbacks_ for underlying socket.


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 {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm fine with this landing as is, but can you please loop me in when it is used? I have some concerns about these particular events being swallowed. Maybe we can add ASSERTs that they're not used so we'll at least catch it in debug builds if a mistake is made? Hopefully seeing where the code is used will allay my concerns :-)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ACK'ed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@alyssawilk I will merge master and use this in my PR #4153, ASSERT cannot be added here. In #4153, it have to swallow Connected event from RawBufferSocket, and raise it when handshake is done.


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() override { return callbacks_; }

private:
const std::string& path_prefix_;
Expand Down
5 changes: 1 addition & 4 deletions test/common/network/connection_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -879,13 +879,10 @@ class MockTransportConnectionImplTest : public testing::Test {
EXPECT_CALL(dispatcher_, createFileEvent_(0, _, _, _))
.WillOnce(DoAll(SaveArg<1>(&file_ready_cb_), Return(file_event_)));
transport_socket_ = new NiceMock<MockTransportSocket>;
EXPECT_CALL(*transport_socket_, setTransportSocketCallbacks(_))
.WillOnce(Invoke([this](TransportSocketCallbacks& callbacks) {
transport_socket_callbacks_ = &callbacks;
}));
connection_.reset(
new ConnectionImpl(dispatcher_, std::make_unique<ConnectionSocketImpl>(0, nullptr, nullptr),
TransportSocketPtr(transport_socket_), true));
transport_socket_callbacks_ = transport_socket_->callbacks();
connection_->addConnectionCallbacks(callbacks_);
}

Expand Down
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) {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if connection_ couldn't be null, make it a reference. also make constructor explicit

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done


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
8 changes: 7 additions & 1 deletion test/mocks/network/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,9 @@ class MockTransportSocket : public TransportSocket {
MockTransportSocket();
~MockTransportSocket();

MOCK_METHOD1(setTransportSocketCallbacks, void(TransportSocketCallbacks& callbacks));
void setTransportSocketCallbacks(TransportSocketCallbacks& callbacks) override {

@lizan lizan Aug 16, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Keep this (and callbacks below) as a MOCK and do ON_CALL(...).WillByDefault in constructor like:
https://github.com/envoyproxy/envoy/pull/4153/files#diff-f1d609046babf8223367556fbdda1721R206

This will allow test to do EXPECT_CALL etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

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

private:
Network::TransportSocketCallbacks* callbacks_;
};

class MockTransportSocketFactory : public TransportSocketFactory {
Expand Down