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

Network::Connection: Add L4 crash dumping support #14509

Merged
merged 5 commits into from
Jan 6, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -90,3 +90,8 @@ envoy_cc_library(
name = "scope_tracker_interface",
hdrs = ["scope_tracker.h"],
)

envoy_cc_library(
name = "dumpable_interface",
hdrs = ["dumpable.h"],
)
28 changes: 28 additions & 0 deletions include/envoy/common/dumpable.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#pragma once

#include <ostream>

#include "envoy/common/pure.h"

namespace Envoy {

/*
* Interface for classes that can dump their state.
* This is similar to ScopeTrackedObject interface, but cannot be registered
* for tracking work.
*/
class Dumpable {
public:
virtual ~Dumpable() = default;
/**
* Dump debug state of the object in question to the provided ostream.
*
* This is called on Envoy fatal errors, so should do minimal memory allocation.
*
* @param os the ostream to output to.
* @param indent_level how far to indent, for pretty-printed classes and subclasses.
*/
virtual void dumpState(std::ostream& os, int indent_level = 0) const PURE;
};

} // namespace Envoy
11 changes: 7 additions & 4 deletions include/envoy/common/scope_tracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,19 @@
namespace Envoy {

/*
* A class for tracking the scope of work.
* Currently this is only used for best-effort tracking the L7 stream doing
* work if a fatal error occurs.
* An interface for tracking the scope of work. Implementors of this interface
* can be registered to the dispatcher when they're active on the stack. If a
* fatal error occurs while they were active, the dumpState method will be
* called.
*
* Currently this is only used for the L4 network connection and L7 stream.
Copy link
Contributor

Choose a reason for hiding this comment

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

class ScopeTrackedObject : public Dumpable {}

But are the two interfaces really needed? Why is it important to distinguish between objects that can be registered and ones that simply have a dump method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is to decrease cognitive load and have it be more explicit than implicit i.e. why is foo a trackedobject that gets registered, but bar a trackedobject that doesn't.

If we enforce this via the type system, the intent is a bit more clear instead of "maybe they forgot to register bar to get dumped".

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is not being able to register the object useful? Having 2 interfaces that are so similar adds to cognitive load.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I would tend to agree that the two interfaces do not feel that useful to me, but I don't feel strongly about it. At minimum I would derive from the other class as @antoniovicente suggests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, will just go with ScopeTrackedObject to keep it simple

*/
class ScopeTrackedObject {
public:
virtual ~ScopeTrackedObject() = default;

/**
* Dump debug state of the object in question to the provided ostream
* Dump debug state of the object in question to the provided ostream.
*
* This is called on Envoy fatal errors, so should do minimal memory allocation.
*
Expand Down
1 change: 1 addition & 0 deletions include/envoy/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ envoy_cc_library(
deps = [
":io_handle_interface",
":socket_interface",
"//include/envoy/common:dumpable_interface",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)
Expand Down
3 changes: 2 additions & 1 deletion include/envoy/network/listen_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include <tuple>
#include <vector>

#include "envoy/common/dumpable.h"
#include "envoy/common/exception.h"
#include "envoy/common/pure.h"
#include "envoy/config/core/v3/base.pb.h"
Expand All @@ -25,7 +26,7 @@ namespace Network {
* TODO(jrajahalme): Hide internals (e.g., fd) from listener filters by providing callbacks filters
* may need (set/getsockopt(), peek(), recv(), etc.)
*/
class ConnectionSocket : public virtual Socket {
class ConnectionSocket : public virtual Socket, public virtual Dumpable {
public:
~ConnectionSocket() override = default;

Expand Down
3 changes: 3 additions & 0 deletions source/common/common/dump_state_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ namespace Envoy {
#define DUMP_OPTIONAL_MEMBER(member) \
", " #member ": " << ((member).has_value() ? absl::StrCat((member).value()) : "null")

#define DUMP_NULLABLE_MEMBER(member, value) \
", " #member ": " << ((member) != nullptr ? value : "null")

// Macro assumes local member variables
// os (ostream)
// indent_level (int)
Expand Down
3 changes: 3 additions & 0 deletions source/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,10 @@ envoy_cc_library(
hdrs = ["connection_impl_base.h"],
deps = [
":filter_manager_lib",
"//include/envoy/common:scope_tracker_interface",
"//include/envoy/event:dispatcher_interface",
"//source/common/common:assert_lib",
"//source/common/common:dump_state_utils",
],
)

Expand Down Expand Up @@ -246,6 +248,7 @@ envoy_cc_library(
"//include/envoy/network:exception_interface",
"//include/envoy/network:listen_socket_interface",
"//source/common/common:assert_lib",
"//source/common/common:dump_state_utils",
"@envoy_api//envoy/config/core/v3:pkg_cc_proto",
],
)
Expand Down
24 changes: 24 additions & 0 deletions source/common/network/connection_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@
#include "envoy/network/socket.h"

#include "common/common/assert.h"
#include "common/common/dump_state_utils.h"
#include "common/common/empty_string.h"
#include "common/common/enum_to_int.h"
#include "common/common/scope_tracker.h"
#include "common/network/address_impl.h"
#include "common/network/listen_socket_impl.h"
#include "common/network/raw_buffer_socket.h"
Expand Down Expand Up @@ -530,6 +532,7 @@ void ConnectionImpl::onWriteBufferHighWatermark() {
}

void ConnectionImpl::onFileEvent(uint32_t events) {
ScopeTrackerScopeState scope(this, this->dispatcher_);
ENVOY_CONN_LOG(trace, "socket event: {}", *this, events);

if (immediate_error_event_ != ConnectionEvent::Connected) {
Expand Down Expand Up @@ -753,6 +756,27 @@ void ConnectionImpl::flushWriteBuffer() {
}
}

std::ostream& operator<<(std::ostream& os, Connection::State connection_state) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Implementing this operator in the middle of this cc file is a bit odd. Consider moving to an anonymous namespace at the top of the file or to the library that defines the Connection::State enum class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved it to anonymous namespace at the top of this file. Done.

switch (connection_state) {
case Connection::State::Open:
return os << "Open";
case Connection::State::Closing:
return os << "Closing";
case Connection::State::Closed:
return os << "Closed";
}
return os;
}

// ScopeTrackedObject
KBaichoo marked this conversation as resolved.
Show resolved Hide resolved
void ConnectionImpl::dumpState(std::ostream& os, int indent_level) const {
const char* spaces = spacesForLevel(indent_level);
os << spaces << "ConnectionImpl " << this << DUMP_MEMBER(connecting_) << DUMP_MEMBER(bind_error_)
<< DUMP_MEMBER(state()) << DUMP_MEMBER(read_buffer_limit_) << "\n";

DUMP_DETAILS(socket_);
}

ServerConnectionImpl::ServerConnectionImpl(Event::Dispatcher& dispatcher,
ConnectionSocketPtr&& socket,
TransportSocketPtr&& transport_socket,
Expand Down
11 changes: 9 additions & 2 deletions source/common/network/connection_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include <memory>
#include <string>

#include "envoy/common/scope_tracker.h"
#include "envoy/network/transport_socket.h"

#include "common/buffer/watermark_buffer.h"
Expand Down Expand Up @@ -41,9 +42,12 @@ class ConnectionImplUtility {
};

/**
* Implementation of Network::Connection and Network::FilterManagerConnection.
* Implementation of Network::Connection, Network::FilterManagerConnection and
* Envoy::ScopeTrackedObject.
*/
class ConnectionImpl : public ConnectionImplBase, public TransportSocketCallbacks {
class ConnectionImpl : public ConnectionImplBase,
public TransportSocketCallbacks,
public ScopeTrackedObject {
public:
ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPtr&& socket,
TransportSocketPtr&& transport_socket, StreamInfo::StreamInfo& stream_info,
Expand Down Expand Up @@ -126,6 +130,9 @@ class ConnectionImpl : public ConnectionImplBase, public TransportSocketCallback
// Obtain global next connection ID. This should only be used in tests.
static uint64_t nextGlobalIdForTest() { return next_global_id_; }

// ScopeTrackedObject
void dumpState(std::ostream& os, int indent_level) const override;

protected:
// A convenience function which returns true if
// 1) The read disable count is zero or
Expand Down
10 changes: 10 additions & 0 deletions source/common/network/listen_socket_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "envoy/network/socket_interface.h"

#include "common/common/assert.h"
#include "common/common/dump_state_utils.h"
#include "common/network/socket_impl.h"

namespace Envoy {
Expand Down Expand Up @@ -137,6 +138,15 @@ class ConnectionSocketImpl : public SocketImpl, public ConnectionSocket {
return ioHandle().lastRoundTripTime();
}

void dumpState(std::ostream& os, int indent_level) const override {
const char* spaces = spacesForLevel(indent_level);
os << spaces << "ListenSocketImpl " << this
<< DUMP_NULLABLE_MEMBER(remote_address_, remote_address_->asStringView())
<< DUMP_NULLABLE_MEMBER(direct_remote_address_, direct_remote_address_->asStringView())
<< DUMP_NULLABLE_MEMBER(local_address_, local_address_->asStringView())
<< DUMP_MEMBER(transport_protocol_) << DUMP_MEMBER(server_name_) << "\n";
}

protected:
Address::InstanceConstSharedPtr remote_address_;
const Address::InstanceConstSharedPtr direct_remote_address_;
Expand Down
74 changes: 74 additions & 0 deletions test/common/network/connection_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,13 @@

#include "envoy/common/platform.h"
#include "envoy/config/core/v3/base.pb.h"
#include "envoy/network/address.h"

#include "common/api/os_sys_calls_impl.h"
#include "common/buffer/buffer_impl.h"
#include "common/common/empty_string.h"
#include "common/common/fmt.h"
#include "common/common/utility.h"
#include "common/event/dispatcher_impl.h"
#include "common/network/address_impl.h"
#include "common/network/connection_impl.h"
Expand Down Expand Up @@ -1851,6 +1853,76 @@ TEST_P(ConnectionImplTest, DelayedCloseTimeoutNullStats) {
server_connection->close(ConnectionCloseType::NoFlush);
}

// Test DumpState methods.
TEST_P(ConnectionImplTest, NetworkSocketDumpsWithoutAllocatingMemory) {
std::array<char, 1024> buffer;
OutputBufferStream ostream{buffer.data(), buffer.size()};
IoHandlePtr io_handle = std::make_unique<IoSocketHandleImpl>(0);
Address::InstanceConstSharedPtr server_addr;
Address::InstanceConstSharedPtr local_addr;
if (GetParam() == Network::Address::IpVersion::v4) {
server_addr = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv4Instance("1.1.1.1", 80, nullptr)};
local_addr = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv4Instance("1.2.3.4", 56789, nullptr)};
} else {
server_addr = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv6Instance("::1", 80, nullptr)};
local_addr = Network::Address::InstanceConstSharedPtr{
new Network::Address::Ipv6Instance("::1:2:3:4", 56789, nullptr)};
}

auto connection_socket =
std::make_unique<ConnectionSocketImpl>(std::move(io_handle), local_addr, server_addr);
connection_socket->setRequestedServerName("envoyproxy.io");

// Start measuring memory and dump state.
Stats::TestUtil::MemoryTest memory_test;
connection_socket->dumpState(ostream, 0);
EXPECT_EQ(memory_test.consumedBytes(), 0);

// Check socket dump
EXPECT_THAT(ostream.contents(), HasSubstr("ListenSocketImpl"));
if (GetParam() == Network::Address::IpVersion::v4) {
EXPECT_THAT(
ostream.contents(),
HasSubstr(
"remote_address_: 1.1.1.1:80, direct_remote_address_: 1.1.1.1:80, local_address_: "
"1.2.3.4:56789, transport_protocol_: , server_name_: envoyproxy.io"));
} else {
EXPECT_THAT(
ostream.contents(),
HasSubstr("remote_address_: [::1]:80, direct_remote_address_: [::1]:80, local_address_: "
"[::1:2:3:4]:56789, transport_protocol_: , server_name_: envoyproxy.io"));
}
}

TEST_P(ConnectionImplTest, NetworkConnectionDumpsWithoutAllocatingMemory) {
std::array<char, 1024> buffer;
OutputBufferStream ostream{buffer.data(), buffer.size()};
ConnectionMocks mocks = createConnectionMocks(false);
IoHandlePtr io_handle = std::make_unique<IoSocketHandleImpl>(0);

auto server_connection = std::make_unique<Network::ServerConnectionImpl>(
*mocks.dispatcher_,
std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
std::move(mocks.transport_socket_), stream_info_, true);

// Start measuring memory and dump state.
Stats::TestUtil::MemoryTest memory_test;
server_connection->dumpState(ostream, 0);
EXPECT_EQ(memory_test.consumedBytes(), 0);

// Check connection data
EXPECT_THAT(ostream.contents(), HasSubstr("ConnectionImpl"));
EXPECT_THAT(ostream.contents(),
HasSubstr("connecting_: 0, bind_error_: 0, state(): Open, read_buffer_limit_: 0"));
// Check socket starts dumping
EXPECT_THAT(ostream.contents(), HasSubstr("ListenSocketImpl"));

server_connection->close(ConnectionCloseType::NoFlush);
}

class FakeReadFilter : public Network::ReadFilter {
public:
FakeReadFilter() = default;
Expand Down Expand Up @@ -1898,6 +1970,8 @@ class MockTransportConnectionImplTest : public testing::Test {
dispatcher_, std::make_unique<ConnectionSocketImpl>(std::move(io_handle), nullptr, nullptr),
TransportSocketPtr(transport_socket_), stream_info_, true);
connection_->addConnectionCallbacks(callbacks_);
// File events will trigger setTrackedObject on the dispatcher.
EXPECT_CALL(dispatcher_, setTrackedObject(_)).WillRepeatedly(Return(nullptr));
}

~MockTransportConnectionImplTest() override { connection_->close(ConnectionCloseType::NoFlush); }
Expand Down
2 changes: 2 additions & 0 deletions test/mocks/network/mocks.h
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include <cstdint>
#include <list>
#include <ostream>
#include <string>
#include <vector>

Expand Down Expand Up @@ -305,6 +306,7 @@ class MockConnectionSocket : public ConnectionSocket {
MOCK_METHOD(Api::SysCallIntResult, getSocketOption, (int, int, void*, socklen_t*), (const));
MOCK_METHOD(Api::SysCallIntResult, setBlockingForTest, (bool));
MOCK_METHOD(absl::optional<std::chrono::milliseconds>, lastRoundTripTime, ());
MOCK_METHOD(void, dumpState, (std::ostream&, int), (const));

IoHandlePtr io_handle_;
Address::InstanceConstSharedPtr local_address_;
Expand Down
2 changes: 2 additions & 0 deletions test/server/filter_chain_benchmark_test.cc
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <iostream>
#include <ostream>

#include "envoy/config/listener/v3/listener.pb.h"
#include "envoy/config/listener/v3/listener_components.pb.h"
Expand Down Expand Up @@ -118,6 +119,7 @@ class MockConnectionSocket : public Network::ConnectionSocket {
}
Api::SysCallIntResult setBlockingForTest(bool) override { return {0, 0}; }
absl::optional<std::chrono::milliseconds> lastRoundTripTime() override { return {}; }
void dumpState(std::ostream&, int) const override {}

private:
Network::IoHandlePtr io_handle_;
Expand Down