Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
12 changes: 7 additions & 5 deletions envoy/event/dispatcher.h
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,15 @@ class Dispatcher : public DispatcherBase, public ScopeTracker {
* @param transport_socket supplies a transport socket to be used by the connection.
* @param options the socket options to be set on the underlying socket before anything is sent
* on the socket.
* @param transport socket options used to create the transport socket.
* @return Network::ClientConnectionPtr a client connection that is owned by the caller.
*/
virtual Network::ClientConnectionPtr
createClientConnection(Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options) PURE;
virtual Network::ClientConnectionPtr createClientConnection(
Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options,
Network::TransportSocketOptionsConstSharedPtr transport_options = nullptr) PURE;

/**
* @return Filesystem::WatcherPtr a filesystem watcher owned by the caller.
Expand Down
12 changes: 6 additions & 6 deletions envoy/network/client_connection_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ class ClientConnectionFactory : public Config::UntypedFactory {
* @return Network::ClientConnectionPtr The created connection. It's never nullptr but the return
* connection may be closed upon return.
*/
virtual Network::ClientConnectionPtr
createClientConnection(Event::Dispatcher& dispatcher,
Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options) PURE;
virtual Network::ClientConnectionPtr createClientConnection(
Event::Dispatcher& dispatcher, Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options,
Network::TransportSocketOptionsConstSharedPtr transport_options = nullptr) PURE;
};

} // namespace Network
Expand Down
5 changes: 3 additions & 2 deletions envoy/network/transport_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,10 @@ class TransportSocketOptions {
virtual absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const PURE;

/**
* @return filter state from the downstream request or connection.
* @return filter state objects from the downstream request or connection
* that are marked as shared with the upstream.
*/
virtual const StreamInfo::FilterStateSharedPtr& filterState() const PURE;
virtual const StreamInfo::FilterState::Objects& filterStateObjects() const 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 think this function name should be more descriptive, maybe something like downstreamSharedFilterStateObjects().

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.

Renamed.

};

using TransportSocketOptionsConstSharedPtr = std::shared_ptr<const TransportSocketOptions>;
Expand Down
43 changes: 42 additions & 1 deletion envoy/stream_info/filter_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,24 @@ using FilterStateSharedPtr = std::shared_ptr<FilterState>;
*/
class FilterState {
public:
enum class StateType { ReadOnly, Mutable };
enum StateType {

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 think it's simpler to just leave this as the previous enum, and add another bool for SharedWithUpstreamConnection.

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.

When the upstream L7 filters get done, we'll also need to share with the upstream request I think. I could add another enum for sharing configuration I suppose?

// Readonly objects must be set once.
ReadOnly = 0x1,

// Mutable objects can be overwritten with the same state type, life span, and name.
Mutable = 0,

// Mark a filter state object as shared with the upstream connection.
// Shared filter state objects are copied by reference from the downstream
// requests and connections to the upstream connection filter state, and
// also participate in the hashing decisions in the transport socket
// options when connections are re-used between downstream requests.
SharedWithUpstreamConnection = 0x2,

// ATTENTION: MAKE SURE THIS REMAINS EQUAL TO THE LAST FLAG.
LastFlag = SharedWithUpstreamConnection,
};

// Objects stored in the FilterState may have different life span. Life span is what controls
// how long an object stored in FilterState lives. Implementation of this interface actually
// stores objects in a (reverse) tree manner - multiple FilterStateImpl with shorter life span may
Expand All @@ -45,6 +62,16 @@ class FilterState {
//
// Note that order matters in this enum because it's assumed that life span grows as enum number
// grows.
//
// Note that for more accurate book-keeping it is recommended to subscribe to
// the stream callbacks instead of relying on the destruction of the filter
// state.
//
// As a special case, objects that are marked as shared with the upstream
// become bound to the upstream connection life span, regardless of the

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.

clarify: the object will live as long as either the downstream or upstream connection is open. The current wording implies that the lifetime is strictly tied to the upstream connection.

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.

Clarified, LMK if it can be improved further.

// original life span. That means, for example, that a shared request span
// object may outlive the original request when it is shared, because it may
// be captured by an upstream connection for the original downstream request.
enum LifeSpan { FilterChain, Request, Connection, TopSpan = Connection };

class Object {
Expand All @@ -66,6 +93,15 @@ class FilterState {
virtual absl::optional<std::string> serializeAsString() const { return absl::nullopt; }
};

struct FilterObject {
std::shared_ptr<Object> data_;
FilterState::StateType state_type_;
std::string name_{""};

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.

The initialization can just be {} instead of {""}, but empty is the default so it can be omitted entirely.

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.

};

using Objects = std::vector<FilterObject>;
using ObjectsPtr = std::unique_ptr<Objects>;

virtual ~FilterState() = default;

/**
Expand Down Expand Up @@ -156,6 +192,11 @@ class FilterState {
* either the top LifeSpan or the parent is not yet created.
*/
virtual FilterStateSharedPtr parent() const PURE;

/**
* @return filter objects that are shared with the upstream connection.
**/
virtual ObjectsPtr objectsSharedWithUpstreamConnection() const PURE;
};

} // namespace StreamInfo
Expand Down
14 changes: 8 additions & 6 deletions source/common/event/dispatcher_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -155,11 +155,12 @@ DispatcherImpl::createServerConnection(Network::ConnectionSocketPtr&& socket,
*this, std::move(socket), std::move(transport_socket), stream_info, true);
}

Network::ClientConnectionPtr
DispatcherImpl::createClientConnection(Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options) {
Network::ClientConnectionPtr DispatcherImpl::createClientConnection(
Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options,
Network::TransportSocketOptionsConstSharedPtr transport_options) {
ASSERT(isThreadSafe());

auto* factory = Config::Utility::getFactoryByName<Network::ClientConnectionFactory>(
Expand All @@ -170,7 +171,8 @@ DispatcherImpl::createClientConnection(Network::Address::InstanceConstSharedPtr
// expects a non-null connection as of today so we cannot gracefully handle unsupported address
// type.
return factory->createClientConnection(*this, address, source_address,
std::move(transport_socket), options);
std::move(transport_socket), options,
std::move(transport_options));
}

FileEventPtr DispatcherImpl::createFileEvent(os_fd_t fd, FileReadyCb cb, FileTriggerType trigger,
Expand Down
11 changes: 6 additions & 5 deletions source/common/event/dispatcher_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -66,11 +66,12 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>,
createServerConnection(Network::ConnectionSocketPtr&& socket,
Network::TransportSocketPtr&& transport_socket,
StreamInfo::StreamInfo& stream_info) override;
Network::ClientConnectionPtr
createClientConnection(Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options) override;
Network::ClientConnectionPtr createClientConnection(
Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options,
Network::TransportSocketOptionsConstSharedPtr transport_options = nullptr) override;
FileEventPtr createFileEvent(os_fd_t fd, FileReadyCb cb, FileTriggerType trigger,
uint32_t events) override;
Filesystem::WatcherPtr createFilesystemWatcher() override;
Expand Down
1 change: 1 addition & 0 deletions source/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,7 @@ envoy_cc_library(
":proxy_protocol_filter_state_lib",
":upstream_server_name_lib",
":upstream_subject_alt_names_lib",
"//envoy/common:hashable_interface",
"//envoy/network:proxy_protocol_options_lib",
"//envoy/network:transport_socket_interface",
"//envoy/stream_info:filter_state_interface",
Expand Down
17 changes: 14 additions & 3 deletions source/common/network/connection_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -855,15 +855,18 @@ ClientConnectionImpl::ClientConnectionImpl(
Event::Dispatcher& dispatcher, const Address::InstanceConstSharedPtr& remote_address,
const Network::Address::InstanceConstSharedPtr& source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options)
const Network::ConnectionSocket::OptionsSharedPtr& options,
const Network::TransportSocketOptionsConstSharedPtr& transport_options)
: ClientConnectionImpl(dispatcher, std::make_unique<ClientSocketImpl>(remote_address, options),
source_address, std::move(transport_socket), options) {}
source_address, std::move(transport_socket), options,
transport_options) {}

ClientConnectionImpl::ClientConnectionImpl(
Event::Dispatcher& dispatcher, std::unique_ptr<ConnectionSocket> socket,
const Address::InstanceConstSharedPtr& source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options)
const Network::ConnectionSocket::OptionsSharedPtr& options,
const Network::TransportSocketOptionsConstSharedPtr& transport_options)
: ConnectionImpl(dispatcher, std::move(socket), std::move(transport_socket), stream_info_,
false),
stream_info_(dispatcher_.timeSource(), socket_->connectionInfoProviderSharedPtr()) {
Expand Down Expand Up @@ -905,6 +908,14 @@ ClientConnectionImpl::ClientConnectionImpl(
ioHandle().activateFileEvents(Event::FileReadyType::Write);
}
}

if (transport_options) {
for (const auto& object : transport_options->filterStateObjects()) {
// TODO: handle exception
stream_info_.filterState()->setData(object.name_, object.data_, object.state_type_,
StreamInfo::FilterState::LifeSpan::Connection);
}
}
}

void ClientConnectionImpl::connect() {
Expand Down
7 changes: 5 additions & 2 deletions source/common/network/connection_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -262,11 +262,14 @@ class ClientConnectionImpl : public ConnectionImpl, virtual public ClientConnect
const Address::InstanceConstSharedPtr& remote_address,
const Address::InstanceConstSharedPtr& source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options);
const Network::ConnectionSocket::OptionsSharedPtr& options,
const Network::TransportSocketOptionsConstSharedPtr& transport_options);

ClientConnectionImpl(Event::Dispatcher& dispatcher, std::unique_ptr<ConnectionSocket> socket,
const Address::InstanceConstSharedPtr& source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options);
const Network::ConnectionSocket::OptionsSharedPtr& options,
const Network::TransportSocketOptionsConstSharedPtr& transport_options);

// Network::ClientConnection
void connect() override;
Expand Down
7 changes: 4 additions & 3 deletions source/common/network/default_client_connection_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,11 @@ Network::ClientConnectionPtr DefaultClientConnectionFactory::createClientConnect
Event::Dispatcher& dispatcher, Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options) {
const Network::ConnectionSocket::OptionsSharedPtr& options,
Network::TransportSocketOptionsConstSharedPtr transport_options) {
ASSERT(address->ip() || address->pipe());
return std::make_unique<Network::ClientConnectionImpl>(dispatcher, address, source_address,
std::move(transport_socket), options);
return std::make_unique<Network::ClientConnectionImpl>(
dispatcher, address, source_address, std::move(transport_socket), options, transport_options);
}
REGISTER_FACTORY(DefaultClientConnectionFactory, Network::ClientConnectionFactory);

Expand Down
12 changes: 6 additions & 6 deletions source/common/network/default_client_connection_factory.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ class DefaultClientConnectionFactory : public ClientConnectionFactory {
std::string name() const override { return "default"; }

// Network::ClientConnectionFactory
Network::ClientConnectionPtr
createClientConnection(Event::Dispatcher& dispatcher,
Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options) override;
Network::ClientConnectionPtr createClientConnection(
Event::Dispatcher& dispatcher, Network::Address::InstanceConstSharedPtr address,
Network::Address::InstanceConstSharedPtr source_address,
Network::TransportSocketPtr&& transport_socket,
const Network::ConnectionSocket::OptionsSharedPtr& options,
Network::TransportSocketOptionsConstSharedPtr transport_options = nullptr) override;
};

} // namespace Network
Expand Down
3 changes: 2 additions & 1 deletion source/common/network/happy_eyeballs_connection_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,8 @@ ClientConnectionPtr HappyEyeballsConnectionImpl::createNextConnection() {
address_list_[next_address_++], connection_construction_state_.source_address_,
connection_construction_state_.socket_factory_.createTransportSocket(
connection_construction_state_.transport_socket_options_),
connection_construction_state_.options_);
connection_construction_state_.options_,
connection_construction_state_.transport_socket_options_);
ENVOY_LOG_EVENT(debug, "happy_eyeballs_cx_attempt", "C[{}] address={}", id_, next_address_);
callbacks_wrappers_.push_back(std::make_unique<ConnectionCallbacksWrapper>(*this, *connection));
connection->addConnectionCallbacks(*callbacks_wrappers_.back());
Expand Down
13 changes: 12 additions & 1 deletion source/common/network/transport_socket_options_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#include <utility>
#include <vector>

#include "envoy/common/hashable.h"

#include "source/common/common/scalar_to_byte_vector.h"
#include "source/common/common/utility.h"
#include "source/common/network/application_protocol.h"
Expand Down Expand Up @@ -40,6 +42,14 @@ void CommonTransportSocketFactory::hashKey(std::vector<uint8_t>& key,
for (const auto& protocol : alpn_fallback) {
pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(protocol), key);
}

for (const auto& object : options->filterStateObjects()) {
if (auto hashable = dynamic_cast<const Hashable*>(object.data_.get()); hashable != nullptr) {

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.

What happens if an object isn't Hashable? Can we end up with bugs due to matching transport socket options hash even though it contains different filter state? Should FilterState::Object always inherit from Hashable? Or maybe make it required when the object opts in to sharing, via an accessor like getHashable()? We mostly try to avoid dynamic_cast in envoy for this type of feature detection.

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.

Hashable is only needed for L7->L4 stream transition. For tcp_proxy, it's already 1-1 so hashing is not necessary. I followed https://www.envoyproxy.io/docs/envoy/latest/api-v3/type/v3/hash_policy.proto#envoy-v3-api-msg-type-v3-hashpolicy-filterstate which does the same dynamic cast test.

if (auto hash = hashable->hash(); hash) {
pushScalarToByteVector(hash.value(), key);
}
}
}
}

TransportSocketOptionsConstSharedPtr TransportSocketOptionsUtility::fromFilterState(
Expand Down Expand Up @@ -79,7 +89,8 @@ TransportSocketOptionsConstSharedPtr TransportSocketOptionsUtility::fromFilterSt

return std::make_shared<Network::TransportSocketOptionsImpl>(
server_name, std::move(subject_alt_names), std::move(application_protocols),
std::move(alpn_fallback), proxy_protocol_options, filter_state);
std::move(alpn_fallback), proxy_protocol_options,
filter_state->objectsSharedWithUpstreamConnection());
}

} // namespace Network
Expand Down
16 changes: 10 additions & 6 deletions source/common/network/transport_socket_options_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ class AlpnDecoratingTransportSocketOptions : public TransportSocketOptions {
absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const override {
return inner_options_->proxyProtocolOptions();
}
const StreamInfo::FilterStateSharedPtr& filterState() const override {
return inner_options_->filterState();
const StreamInfo::FilterState::Objects& filterStateObjects() const override {
return inner_options_->filterStateObjects();
}

private:
Expand All @@ -45,13 +45,15 @@ class TransportSocketOptionsImpl : public TransportSocketOptions {
std::vector<std::string>&& override_verify_san_list = {},
std::vector<std::string>&& override_alpn = {}, std::vector<std::string>&& fallback_alpn = {},
absl::optional<Network::ProxyProtocolData> proxy_proto_options = absl::nullopt,
const StreamInfo::FilterStateSharedPtr filter_state = nullptr)
StreamInfo::FilterState::ObjectsPtr filter_state_objects =
std::make_unique<StreamInfo::FilterState::Objects>())
: override_server_name_(override_server_name.empty()
? absl::nullopt
: absl::optional<std::string>(override_server_name)),
override_verify_san_list_{std::move(override_verify_san_list)},
override_alpn_list_{std::move(override_alpn)}, alpn_fallback_{std::move(fallback_alpn)},
proxy_protocol_options_(proxy_proto_options), filter_state_(filter_state) {}
proxy_protocol_options_(proxy_proto_options),
filter_state_objects_(std::move(filter_state_objects)) {}

// Network::TransportSocketOptions
const absl::optional<std::string>& serverNameOverride() const override {
Expand All @@ -69,15 +71,17 @@ class TransportSocketOptionsImpl : public TransportSocketOptions {
absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const override {
return proxy_protocol_options_;
}
const StreamInfo::FilterStateSharedPtr& filterState() const override { return filter_state_; }
const StreamInfo::FilterState::Objects& filterStateObjects() const override {
return *filter_state_objects_;
}

private:
const absl::optional<std::string> override_server_name_;
const std::vector<std::string> override_verify_san_list_;
const std::vector<std::string> override_alpn_list_;
const std::vector<std::string> alpn_fallback_;
const absl::optional<Network::ProxyProtocolData> proxy_protocol_options_;
const StreamInfo::FilterStateSharedPtr filter_state_;
const StreamInfo::FilterState::ObjectsPtr filter_state_objects_;
};

class TransportSocketOptionsUtility {
Expand Down
Loading