Improve performance of finding SNI Actions

include/tscpp/util/Convert.h

@@ -0,0 +1,46 @@
+/** @file
+
+  Collection of utility functions for converting between different chars.
+
+  @section license License
+
+  Licensed to the Apache Software Foundation (ASF) under one
+  or more contributor license agreements.  See the NOTICE file
+  distributed with this work for additional information
+  regarding copyright ownership.  The ASF licenses this file
+  to you under the Apache License, Version 2.0 (the
+  "License"); you may not use this file except in compliance
+  with the License.  You may obtain a copy of the License at
+
+      http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+ */
+
+#pragma once
+
+#include "swoc/MemSpan.h"
+
+#include <string_view>
+
+namespace ts
+{
+/** Copy @a src to @a dst, transforming to lower case.
+ *
+ * @param src Input string.
+ * @param dst Output buffer.
+ */
+inline void
+transform_lower(std::string_view src, swoc::MemSpan<char> dst)
+{
+  if (src.size() > dst.size() - 1) { // clip @a src, reserving space for the terminal nul.
+    src = std::string_view{src.data(), dst.size() - 1};
+  }
+  auto final = std::transform(src.begin(), src.end(), dst.data(), [](char c) -> char { return std::tolower(c); });
+  *final++   = '\0';
+}
+} // namespace ts

iocore/net/SSLCertLookup.cc

@@ -35,6 +35,8 @@
 
 #include "tscore/TestBox.h"
 
+#include "tscpp/util/Convert.h"
+
 #include "I_EventSystem.h"
 
 #include "P_SSLUtils.h"
@@ -143,24 +145,6 @@ struct SSLContextStorage {
   int store(SSLCertContext const &cc);
 };
 
-namespace
-{
-/** Copy @a src to @a dst, transforming to lower case.
- *
- * @param src Input string.
- * @param dst Output buffer.
- */
-inline void
-transform_lower(std::string_view src, swoc::MemSpan<char> dst)
-{
-  if (src.size() > dst.size() - 1) { // clip @a src, reserving space for the terminal nul.
-    src = std::string_view{src.data(), dst.size() - 1};
-  }
-  auto final = std::transform(src.begin(), src.end(), dst.data(), [](char c) -> char { return std::tolower(c); });
-  *final++   = '\0';
-}
-} // namespace
-
 // Zero out and free the heap space allocated for ticket keys to avoid leaking secrets.
 // The first several bytes stores the number of keys and the rest stores the ticket keys.
 void
@@ -461,7 +445,7 @@ SSLContextStorage::insert(const char *name, int idx)
 {
   ats_wildcard_matcher wildcard;
   char lower_case_name[TS_MAX_HOST_NAME_LEN + 1];
-  transform_lower(name, lower_case_name);
+  ts::transform_lower(name, lower_case_name);
 
   shared_SSL_CTX ctx = this->ctx_store[idx].getCtx();
   if (wildcard.match(lower_case_name)) {
@@ -512,7 +496,7 @@ SSLContextStorage::lookup(const std::string &name)
   }
   // Try lower casing it
   char lower_case_name[TS_MAX_HOST_NAME_LEN + 1];
-  transform_lower(name, lower_case_name);
+  ts::transform_lower(name, lower_case_name);
   if (auto it_lower = this->hostnames.find(lower_case_name); it_lower != this->hostnames.end()) {
     return &(this->ctx_store[it_lower->second]);
   }
@@ -557,7 +541,7 @@ reverse_dns_name(const char *hostname, char (&reversed)[TS_MAX_HOST_NAME_LEN + 1
       *(--ptr) = '.';
     }
   }
-  transform_lower(ptr, {ptr, strlen(ptr) + 1});
+  ts::transform_lower(ptr, {ptr, strlen(ptr) + 1});
 
   return ptr;
 }
@@ -572,8 +556,13 @@ REGRESSION_TEST(SSLWildcardMatch)(RegressionTest *t, int /* atype ATS_UNUSED */,
   box.check(wildcard.match("foo.com") == false, "foo.com is not a wildcard");
   box.check(wildcard.match("*.foo.com") == true, "*.foo.com is a wildcard");
   box.check(wildcard.match("bar*.foo.com") == false, "bar*.foo.com not a wildcard");
+  box.check(wildcard.match("*bar.foo.com") == false, "*bar.foo.com not a wildcard");
+  box.check(wildcard.match("b*ar.foo.com") == false, "*bar.foo.com not a wildcard");
+  box.check(wildcard.match("bar.*.foo.com") == false, "bar.*.foo.com not a wildcard");
+  box.check(wildcard.match("*.*.foo.com") == false, "multiple *");
   box.check(wildcard.match("*") == false, "* is not a wildcard");
   box.check(wildcard.match("") == false, "'' is not a wildcard");
+  box.check(wildcard.match("foo[0-9]+.example.com") == false, "regex is not wildcard");
 }
 
 REGRESSION_TEST(SSLReverseHostname)(RegressionTest *t, int /* atype ATS_UNUSED */, int *pstatus)

iocore/net/SSLSNIConfig.cc

@@ -40,6 +40,9 @@
 #include "tscore/I_Layout.h"
 
 #include "tscpp/util/ts_ip.h"
+#include "tscpp/util/Convert.h"
+
+#include "swoc/TextView.h"
 
 #include <netinet/in.h>
 
@@ -53,6 +56,16 @@ static constexpr int OVECSIZE{30};
 static DbgCtl dbg_ctl_ssl{"ssl"};
 static DbgCtl dbg_ctl_ssl_sni{"ssl_sni"};
 
+namespace
+{
+bool
+is_port_in_the_ranges(const std::vector<ts::port_range_t> &port_ranges, in_port_t port)
+{
+  return std::any_of(port_ranges.begin(), port_ranges.end(),
+                     [port](ts::port_range_t const &port_range) { return port_range.contains(port); });
+}
+} // namespace
+
 ////
 // NamedElement
 //
@@ -117,13 +130,36 @@ SNIConfigParams::get_property_config(const std::string &servername) const
 bool
 SNIConfigParams::load_sni_config()
 {
+  uint32_t count = 0;
+  ats_wildcard_matcher wildcard;
+
   for (auto &item : yaml_sni.items) {
-    auto &ai = sni_action_list.emplace_back();
-    ai.set_glob_name(item.fqdn);
-    ai.inbound_port_ranges = item.inbound_port_ranges;
     Dbg(dbg_ctl_ssl, "name: %s", item.fqdn.data());
 
-    item.populate_sni_actions(ai.actions);
+    ActionElement *element = nullptr;
+
+    // servername is case-insensitive, store & find it in lower case
+    char lower_case_name[TS_MAX_HOST_NAME_LEN + 1];
+    ts::transform_lower(item.fqdn, lower_case_name);
+
+    if (wildcard.match(lower_case_name)) {
+      auto &ai = sni_action_list.emplace_back();
+      ai.set_glob_name(lower_case_name);
+      element = &ai;
+    } else {
+      auto it = sni_action_map.emplace(std::make_pair(lower_case_name, ActionElement()));
+      if (it == sni_action_map.end()) {
+        Error("error on loading sni yaml - fqdn=%s", item.fqdn.c_str());
+        return false;
+      }
+
+      element = &it->second;
+    }
+
+    element->inbound_port_ranges = item.inbound_port_ranges;
+    element->rank                = count++;
+
+    item.populate_sni_actions(element->actions);
     if (!set_next_hop_properties(item)) {
       return false;
     }
@@ -167,20 +203,50 @@ SNIConfigParams::load_certs_if_client_cert_specified(YamlSNIConfig::Item const &
   return true;
 }
 
+/**
+  CAVEAT: the "fqdn" field in the sni.yaml accepts wildcards (*), but it has a negative performance impact.
+  */
 std::pair<const ActionVector *, ActionItem::Context>
 SNIConfigParams::get(std::string_view servername, in_port_t dest_incoming_port) const
 {
+  const ActionElement *element = nullptr;
+
+  // Check for exact matches
+  char lower_case_name[TS_MAX_HOST_NAME_LEN + 1];
+  ts::transform_lower(servername, lower_case_name);
+
+  Debug("sni", "lower_case_name=%s", lower_case_name);
+
+  auto range = sni_action_map.equal_range(lower_case_name);
+  for (auto it = range.first; it != range.second; ++it) {
+    Debug("sni", "match with %s", it->first.c_str());
+
+    if (!is_port_in_the_ranges(it->second.inbound_port_ranges, dest_incoming_port)) {
+      continue;
+    }
+
+    const ActionElement *candidate = &it->second;
+    if (element == nullptr) {
+      element = candidate;
+    } else if (candidate->rank < element->rank) {
+      element = &it->second;
+    }
+  }
+
+  // Check for wildcard matches
   int ovector[OVECSIZE];
 
   for (auto const &retval : sni_action_list) {
+    if (element != nullptr && element->rank < retval.rank) {
+      break;
+    }
+
     int length = servername.length();
     if (retval.match == nullptr && length == 0) {
       return {&retval.actions, {}};
     } else if (auto offset = pcre_exec(retval.match.get(), nullptr, servername.data(), length, 0, 0, ovector, OVECSIZE);
                offset >= 0) {
-      if (std::none_of(
-            retval.inbound_port_ranges.begin(), retval.inbound_port_ranges.end(),
-            [dest_incoming_port](ts::port_range_t const &port_range) { return port_range.contains(dest_incoming_port); })) {
+      if (!is_port_in_the_ranges(retval.inbound_port_ranges, dest_incoming_port)) {
         continue;
       }
       if (offset == 1) {
@@ -209,7 +275,12 @@ SNIConfigParams::get(std::string_view servername, in_port_t dest_incoming_port)
       return {&retval.actions, {std::move(groups)}};
     }
   }
-  return {nullptr, {}};
+
+  if (element != nullptr) {
+    return {&element->actions, {}};
+  } else {
+    return {nullptr, {}};
+  }
 }
 
 bool
@@ -251,7 +322,7 @@ SNIConfigParams::initialize(std::string const &sni_filename)
 
 SNIConfigParams::~SNIConfigParams()
 {
-  // sni_action_list and next_hop_list should cleanup with the params object
+  // sni_action_map, sni_action_list and next_hop_list should cleanup with the params object
 }
 
 ////

iocore/net/SSLSNIConfig.h

@@ -30,6 +30,8 @@
  ****************************************************************************/
 #pragma once
 
+#include <string>
+#include <unordered_map>
 #include <vector>
 #include <string_view>
 #include <strings.h>
@@ -74,6 +76,8 @@ struct NamedElement {
   std::vector<ts::port_range_t> inbound_port_ranges;
 
   std::unique_ptr<pcre, PcreFreer> match;
+
+  uint32_t rank = 0; ///< order of the config. smaller is higher.
 };
 
 struct ActionElement : public NamedElement {
@@ -84,9 +88,6 @@ struct NextHopItem : public NamedElement {
   NextHopProperty prop;
 };
 
-using SNIList             = std::vector<ActionElement>;
-using NextHopPropertyList = std::vector<NextHopItem>;
-
 class SNIConfigParams : public ConfigInfo
 {
 public:
@@ -102,8 +103,9 @@ class SNIConfigParams : public ConfigInfo
   bool load_sni_config();
   std::pair<const ActionVector *, ActionItem::Context> get(std::string_view servername, uint16_t dest_incoming_port) const;
 
-  SNIList sni_action_list;
-  NextHopPropertyList next_hop_list;
+  std::unordered_multimap<std::string, ActionElement> sni_action_map; ///< for exact fqdn matching
+  std::vector<ActionElement> sni_action_list;                         ///< for regex fqdn matching
+  std::vector<NextHopItem> next_hop_list;
   YamlSNIConfig yaml_sni;
 
 private:

iocore/net/unit_tests/sni_conf_test.yaml

@@ -25,3 +25,14 @@ sni:
   inbound_port_ranges: 8080-65535
 - fqdn: oneport.com
   inbound_port_ranges: 433
+
+# order check
+- fqdn: foo.bar.com
+  http2: true
+  http2_buffer_water_mark: 256
+  http2_initial_window_size_in: 256
+- fqdn: "*.bar.com"
+  http2: true
+  http2_buffer_water_mark: 256
+- fqdn: foo.bar.com
+  http2: false

iocore/net/unit_tests/test_SSLSNIConfig.cc

@@ -37,7 +37,7 @@
 TEST_CASE("Test SSLSNIConfig")
 {
   SNIConfigParams params;
-  params.initialize(_XSTR(LIBINKNET_UNIT_TEST_DIR) "/sni_conf_test.yaml");
+  REQUIRE(params.initialize(_XSTR(LIBINKNET_UNIT_TEST_DIR) "/sni_conf_test.yaml"));
 
   SECTION("The config does not match any SNIs for someport.com:577")
   {
@@ -105,4 +105,12 @@ TEST_CASE("Test SSLSNIConfig")
     REQUIRE(actions.first);
     REQUIRE(actions.first->size() == 3);
   }
+
+  SECTION("Matching order")
+  {
+    std::string_view target = "foo.bar.com";
+    auto const &actions{params.get(target, 443)};
+    REQUIRE(actions.first);
+    REQUIRE(actions.first->size() == 5); ///< three H2 config + early data + fqdn
+  }
 }

iocore/net/unit_tests/test_YamlSNIConfig.cc

@@ -55,7 +55,7 @@ TEST_CASE("YamlSNIConfig sets port ranges appropriately")
     FAIL(errorstream.str());
   }
   REQUIRE(zret.isOK());
-  REQUIRE(conf.items.size() == 4);
+  REQUIRE(conf.items.size() == 7);
 
   SECTION("If no ports were specified, port range should contain all ports.")
   {

iocore/net/unit_tests/unit_test_main.cc

@@ -46,6 +46,11 @@ class EventProcessorListener final : public Catch::TestEventListenerBase
     Layout::create();
     BaseLogFile *base_log_file = new BaseLogFile("stderr");
     DiagsPtr::set(new Diags(testRunInfo.name, "" /* tags */, "" /* actions */, base_log_file));
+
+    diags()->activate_taglist("sni", DiagsTagType_Debug);
+    diags()->config.enabled(DiagsTagType_Debug, 0); // set 1 if you want to see debug log
+    diags()->show_location = SHOW_LOCATION_DEBUG;
+
     RecProcessInit();
     LibRecordsConfigInit();
 

tests/gold_tests/h2/h2disable.test.py

@@ -57,7 +57,7 @@
     'sni:',
     '- fqdn: bar.com',
     '  http2: off',
-    '- fqdn: bob.*.com',
+    '- fqdn: "*.foo.com"',
     '  http2: off',
 ])
 

tests/gold_tests/h2/h2disable_no_accept_threads.test.py

@@ -57,7 +57,7 @@
     'sni:',
     '- fqdn: bar.com',
     '  http2: off',
-    '- fqdn: bob.*.com',
+    '- fqdn: "*.foo.com"',
     '  http2: off',
 ])
 

tests/gold_tests/h2/h2enable.test.py

@@ -57,7 +57,7 @@
     'sni:',
     '- fqdn: bar.com',
     '  http2: on',
-    '- fqdn: bob.*.com',
+    '- fqdn: "*.foo.com"',
     '  http2: on',
 ])