[GitHub] [trafficserver] lzx404243 commented on a diff in pull request #9618: Use BWF for SNI tunnel configuration.

["lzx404243 (via GitHub)" <gi...@apache.org>]

lzx404243 commented on code in PR #9618:
URL: https://github.com/apache/trafficserver/pull/9618#discussion_r1173897404


##########
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:
   In the current code, specifying both the `inbound_local_port` and `proxy_protocol_port` results in an error. Looking at the changes it seems all available substitutions are applied by default. Is it possible to preserve the error for this misconfigured case? 



##########
tests/gold_tests/tls/tls_tunnel.test.py:
##########
@@ -96,19 +96,19 @@
     "- fqdn: bob.*.com",
     "  tunnel_route: localhost:{0}".format(server_foo.Variables.SSL_Port),
     "- fqdn: '*.match.com'",
-    "  tunnel_route: $1.testmatch:{0}".format(server_foo.Variables.SSL_Port),
+    '''  tunnel_route: "{{1}}.testmatch:{0}"'''.format(server_foo.Variables.SSL_Port),
     "- fqdn: '*.ok.*.com'",
-    "  tunnel_route: $2.example.$1:{0}".format(server_foo.Variables.SSL_Port),
+    '''  tunnel_route: "{{2}}.example.{{1}}:{0}"'''.format(server_foo.Variables.SSL_Port),
     "- fqdn: ''",  # No SNI sent
     "  tunnel_route: localhost:{0}".format(server_bar.Variables.SSL_Port),
     "- fqdn: 'incoming.port.com'",
-    "  tunnel_route: backend.incoming.port.com:{inbound_local_port}",
+    '''  tunnel_route: "backend.incoming.port.com:{inbound_local_port}"''',
     "- fqdn: 'proxy.protocol.port.com'",
-    "  tunnel_route: backend.proxy.protocol.port.com:{proxy_protocol_port}",
+    '''  tunnel_route: "backend.proxy.protocol.port.com:{proxy_protocol_port}"''',
     "- fqdn: '*.*.incoming.port.com'",
-    "  tunnel_route: backend.$1.$2.incoming.port.com:{inbound_local_port}",
+    '''  tunnel_route: "backend.{1}.{2}.incoming.port.com:{inbound_local_port}"''',

Review Comment:
   Is it valid for  `tunnel_route` url to have the `{}` characters?  I am not sure if  we support url that contains the `{}` at all in ATS since they are considered unsafe and need to be percent-encoded. But if so, there would be issues for:
   
   - When `{abc}` is in the url but `abc` is an invalid name for bwf so would become `~abc~`
   - When `{3}` is specified in the url but only two match groups are available.



##########
iocore/net/SNIActionPerformer.cc:
##########
@@ -92,3 +94,111 @@ SNI_IpAllow::TestClientSNIAction(char const *servrername, IpEndpoint const &ep,
 {
   return ip_addrs.contains(swoc::IPAddr(ep));
 }
+
+TunnelDestination::bwf_map_type TunnelDestination::bwf_map;
+
+TunnelDestination::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)
+{
+  // Get a view of the port text. If there is a substitution there, @a port will end up empty while
+  // rest (stuff after port) will be non-empty. If both are empty there is no port specified at all.
+  std::string_view port_text, rest;
+  swoc::IPEndpoint::tokenize(destination, nullptr, &port_text, &rest);
+  dynamic_port_p = port_text.empty() && !rest.empty();
+
+  Debug("ssl_sni", "port is %s", dynamic_port_p ? "true" : "false");

Review Comment:
   -> `"port is dynamic: %s"` ?



-- 
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: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org