-
Notifications
You must be signed in to change notification settings - Fork 5.3k
transport socket: refactor to capture the filter state #19809
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
Changes from all commits
feca4fc
120dd52
1966cdb
43179d3
bfd25ab
a242a23
88ff73a
5108c78
82ba840
8aec921
8f849ae
7121ce6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,92 +15,71 @@ | |
|
|
||
| namespace Envoy { | ||
| namespace Network { | ||
| namespace { | ||
| void commonHashKey(const TransportSocketOptions& options, std::vector<std::uint8_t>& key, | ||
| const Network::TransportSocketFactory& factory) { | ||
| const auto& server_name_overide = options.serverNameOverride(); | ||
|
|
||
| void CommonTransportSocketFactory::hashKey(std::vector<uint8_t>& key, | ||
| TransportSocketOptionsConstSharedPtr options) const { | ||
| if (!options) { | ||
| return; | ||
| } | ||
| const auto& server_name_overide = options->serverNameOverride(); | ||
| if (server_name_overide.has_value()) { | ||
| pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(server_name_overide.value()), key); | ||
| } | ||
|
|
||
| const auto& verify_san_list = options.verifySubjectAltNameListOverride(); | ||
| const auto& verify_san_list = options->verifySubjectAltNameListOverride(); | ||
| for (const auto& san : verify_san_list) { | ||
| pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(san), key); | ||
| } | ||
|
|
||
| const auto& alpn_list = options.applicationProtocolListOverride(); | ||
| const auto& alpn_list = options->applicationProtocolListOverride(); | ||
| for (const auto& protocol : alpn_list) { | ||
| pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(protocol), key); | ||
| } | ||
|
|
||
| const auto& alpn_fallback = options.applicationProtocolFallback(); | ||
| const auto& alpn_fallback = options->applicationProtocolFallback(); | ||
| for (const auto& protocol : alpn_fallback) { | ||
| pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(protocol), key); | ||
| } | ||
|
|
||
| // Proxy protocol options should only be included in the hash if the upstream | ||
| // socket intends to use them. | ||
| const auto& proxy_protocol_options = options.proxyProtocolOptions(); | ||
| if (proxy_protocol_options.has_value() && factory.usesProxyProtocolOptions()) { | ||
| pushScalarToByteVector( | ||
| StringUtil::CaseInsensitiveHash()(proxy_protocol_options.value().asStringForHash()), key); | ||
| } | ||
| } | ||
| } // namespace | ||
|
|
||
| void AlpnDecoratingTransportSocketOptions::hashKey( | ||
| std::vector<uint8_t>& key, const Network::TransportSocketFactory& factory) const { | ||
| commonHashKey(*this, key, factory); | ||
| } | ||
|
|
||
| void TransportSocketOptionsImpl::hashKey(std::vector<uint8_t>& key, | ||
| const Network::TransportSocketFactory& factory) const { | ||
| commonHashKey(*this, key, factory); | ||
| } | ||
|
|
||
| TransportSocketOptionsConstSharedPtr | ||
| TransportSocketOptionsUtility::fromFilterState(const StreamInfo::FilterState& filter_state) { | ||
| TransportSocketOptionsConstSharedPtr TransportSocketOptionsUtility::fromFilterState( | ||
| const StreamInfo::FilterStateSharedPtr& filter_state) { | ||
| if (!filter_state) { | ||
| return nullptr; | ||
| } | ||
| absl::string_view server_name; | ||
| std::vector<std::string> application_protocols; | ||
| std::vector<std::string> subject_alt_names; | ||
| std::vector<std::string> alpn_fallback; | ||
| absl::optional<Network::ProxyProtocolData> proxy_protocol_options; | ||
|
|
||
| bool needs_transport_socket_options = false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could easily be misremembering but I thought previously this being null vs non-null meant that connections would or would not be hashed together in connection pools. have you made sure it's safe to always return this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I assumed this is just some optimization but if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked over and I think there is no risk in returning an empty options struct vs PS. I added some additional refactor to make it more clear the goal of this PR. I want to swap out |
||
| if (auto typed_data = filter_state.getDataReadOnly<UpstreamServerName>(UpstreamServerName::key()); | ||
| if (auto typed_data = | ||
| filter_state->getDataReadOnly<UpstreamServerName>(UpstreamServerName::key()); | ||
| typed_data != nullptr) { | ||
| server_name = typed_data->value(); | ||
| needs_transport_socket_options = true; | ||
| } | ||
|
|
||
| if (auto typed_data = filter_state.getDataReadOnly<Network::ApplicationProtocols>( | ||
| if (auto typed_data = filter_state->getDataReadOnly<Network::ApplicationProtocols>( | ||
| Network::ApplicationProtocols::key()); | ||
| typed_data != nullptr) { | ||
| application_protocols = typed_data->value(); | ||
| needs_transport_socket_options = true; | ||
| } | ||
|
|
||
| if (auto typed_data = | ||
| filter_state.getDataReadOnly<UpstreamSubjectAltNames>(UpstreamSubjectAltNames::key()); | ||
| filter_state->getDataReadOnly<UpstreamSubjectAltNames>(UpstreamSubjectAltNames::key()); | ||
| typed_data != nullptr) { | ||
| subject_alt_names = typed_data->value(); | ||
| needs_transport_socket_options = true; | ||
| } | ||
|
|
||
| if (auto typed_data = | ||
| filter_state.getDataReadOnly<ProxyProtocolFilterState>(ProxyProtocolFilterState::key()); | ||
| filter_state->getDataReadOnly<ProxyProtocolFilterState>(ProxyProtocolFilterState::key()); | ||
| typed_data != nullptr) { | ||
| proxy_protocol_options.emplace(typed_data->value()); | ||
| needs_transport_socket_options = true; | ||
| } | ||
|
|
||
| if (needs_transport_socket_options) { | ||
| return std::make_shared<Network::TransportSocketOptionsImpl>( | ||
| server_name, std::move(subject_alt_names), std::move(application_protocols), | ||
| std::move(alpn_fallback), proxy_protocol_options); | ||
| } else { | ||
| return nullptr; | ||
| } | ||
| 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); | ||
| } | ||
|
|
||
| } // namespace Network | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What in the implementation makes this downstream-specific?
and aren't transport sockets per-connection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It specifies which filter state to capture when creating transport socket options. That determines whether downstream requests/connections map to the same upstream connection once we use the filter state in
hashKey. This is basically a generalization ofProxyProtocolDatabut for arbitrary filter state objects.