SolidWallOfCode commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1175719239
##########
iocore/net/P_SNIActionPerformer.h:
##########
@@ -92,180 +95,62 @@ class HTTP2BufferWaterMark : public ActionItem
class TunnelDestination : public ActionItem
{
- // ID of the configured variable. This will be used to know which function
- // should be called when processing the tunnel destination.
- enum OpId : int32_t {
- MATCH_GROUPS, // Deal with configured groups.
- MAP_WITH_RECV_PORT, // Use port from inbound local
- MAP_WITH_PROXY_PROTOCOL_PORT, // Use port from the proxy protocol
- MAX // Always at the end and do not change the
value of the above items.
+ /// Context used for name binding on tunnel destination.
+ struct BWContext {
+ Context const &_action_ctx; ///< @c ActionItem context.
+ SSLNetVConnection *_vc; ///< Connection object.
};
- static constexpr std::string_view MAP_WITH_RECV_PORT_STR =
"{inbound_local_port}";
- static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR =
"{proxy_protocol_port}";
-
-public:
- TunnelDestination(const std::string_view &dest, SNIRoutingType type,
YamlSNIConfig::TunnelPreWarm prewarm,
- const std::vector<int> &alpn)
- : destination(dest), type(type), tunnel_prewarm(prewarm), alpn_ids(alpn)
+ /// The container for bound names.
+ using bwf_map_type = swoc::bwf::ContextNames<BWContext const>;
+ /// Argument pack for capture groups.
+ class CaptureArgs : public swoc::bwf::ArgPack
{
- // Check for port variable specification. Note that this is checked before
- // the match group so that the corresponding function can be applied before
- // the match group expansion(when the var_start_pos is still accurate).
- auto recv_port_start_pos = destination.find(MAP_WITH_RECV_PORT_STR);
- auto pp_port_start_pos =
destination.find(MAP_WITH_PROXY_PROTOCOL_PORT_STR);
- bool has_recv_port_var = recv_port_start_pos != std::string::npos;
- bool has_pp_port_var = pp_port_start_pos != std::string::npos;
- if (has_recv_port_var && has_pp_port_var) {
- Error("Invalid destination \"%.*s\" in SNI configuration - Only one port
variable can be specified.",
- static_cast<int>(destination.size()), destination.data());
- } else if (has_recv_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_RECV_PORT);
- var_start_pos = recv_port_start_pos;
- } else if (has_pp_port_var) {
- fnArrIndexes.push_back(OpId::MAP_WITH_PROXY_PROTOCOL_PORT);
- var_start_pos = pp_port_start_pos;
- }
- // Check for match groups as well.
- if (destination.find_first_of('$') != std::string::npos) {
- fnArrIndexes.push_back(OpId::MATCH_GROUPS);
- }
- }
- ~TunnelDestination() override {}
+ public:
+ CaptureArgs(std::optional<ActionItem::Context::CapturedGroupViewVec> const
&groups);
- int
- SNIAction(TLSSNISupport *snis, const Context &ctx) const override
- {
- // Set the netvc option?
- SSLNetVConnection *ssl_netvc = dynamic_cast<SSLNetVConnection *>(snis);
- const char *servername = snis->get_sni_server_name();
- if (ssl_netvc) {
- if (fnArrIndexes.empty()) {
- ssl_netvc->set_tunnel_destination(destination, type,
!TLSTunnelSupport::PORT_IS_DYNAMIC, tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], fqdn [%s]",
destination.c_str(), servername);
- } else {
- bool port_is_dynamic = false;
- auto fixed_dst{destination};
- // Apply mapping functions to get the final destination.
- for (auto fnArrIndex : fnArrIndexes) {
- // Dispatch to the correct tunnel destination port function.
- fixed_dst = fix_destination[fnArrIndex](fixed_dst, var_start_pos,
ctx, ssl_netvc, port_is_dynamic);
- }
- ssl_netvc->set_tunnel_destination(fixed_dst, type, port_is_dynamic,
tunnel_prewarm);
- Debug("ssl_sni", "Destination now is [%s], configured [%s], fqdn
[%s]", fixed_dst.c_str(), destination.c_str(), servername);
- }
+ virtual std::any capture(unsigned idx) const override;
- if (type == SNIRoutingType::BLIND) {
- ssl_netvc->attributes = HttpProxyPort::TRANSPORT_BLIND_TUNNEL;
- }
+ /// Call out from formatting when a replace group is referenced.
+ virtual swoc::BufferWriter &print(swoc::BufferWriter &w, swoc::bwf::Spec
const &spec, unsigned idx) const override;
- // ALPN
- for (int id : alpn_ids) {
- ssl_netvc->enableProtocol(id);
- }
- }
+ /// Number of arguments in the pack.
+ virtual unsigned count() const override;
- return SSL_TLSEXT_ERR_OK;
- }
+ private:
+ /// Empty group for when no captures exist.
+ static inline const ActionItem::Context::CapturedGroupViewVec NO_GROUPS;
+ ActionItem::Context::CapturedGroupViewVec const &_groups;
+ };
-private:
- static bool
- is_number(std::string_view s)
- {
- return !s.empty() &&
- std::find_if(std::begin(s), std::end(s),
[](std::string_view::value_type c) { return !std::isdigit(c); }) == std::end(s);
- }
+ static constexpr std::string_view MAP_WITH_RECV_PORT_STR =
"inbound_local_port";
+ static constexpr std::string_view MAP_WITH_PROXY_PROTOCOL_PORT_STR =
"proxy_protocol_port";
Review Comment:
Is that misconfigured? Currently it's a misconfiguration because one of them
will not be applied and therefore the behavior is not as expected. With this
change all instances are replaced and therefore there is no surprise if both
are used. I don't see any reason to forbid it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
