[GitHub] [trafficserver] bneradt commented on a change in pull request #6645: Traffic Dump: Adding an SNI filtering option.

[GitBox <gi...@apache.org>]

bneradt commented on a change in pull request #6645:
URL: https://github.com/apache/trafficserver/pull/6645#discussion_r413039761



##########
File path: plugins/experimental/traffic_dump/traffic_dump.cc
##########
@@ -597,9 +601,29 @@ global_ssn_handler(TSCont contp, TSEvent event, void *edata)
     return TS_SUCCESS;
   }
   case TS_EVENT_HTTP_SSN_START: {
-    // Grab session id to do sampling
+    // Grab session id for logging against a global value rather than the local
+    // session_counter.
     int64_t id = TSHttpSsnIdGet(ssnp);
-    if (id % sample_pool_size != 0) {
+
+    // If the user has asked for SNI filtering, filter on that first because
+    // any sampling will apply just to that subset of connections that match
+    // that SNI.
+    if (!sni_filter.empty()) {
+      TSVConn ssn_vc           = TSHttpSsnClientVConnGet(ssnp);
+      TSSslConnection ssl_conn = TSVConnSslConnectionGet(ssn_vc);
+      SSL *ssl_obj             = (SSL *)ssl_conn;
+      if (ssl_obj == nullptr) {
+        TSDebug(PLUGIN_NAME, "global_ssn_handler(): Ignore non-HTTPS session %" PRId64 "...", id);
+        break;
+      }
+      const std::string sni = SSL_get_servername(ssl_obj, TLSEXT_NAMETYPE_host_name);
+      if (sni != sni_filter) {
+        TSDebug(PLUGIN_NAME, "global_ssn_handler(): Ignore HTTPS session with non-filtered SNI: %s", sni.c_str());
+        break;
+      }
+    }
+    const auto this_session_count = session_counter++;
+    if (this_session_count % sample_pool_size != 0) {

Review comment:
       Thanks for the thoughts. I think we should keep this local variable though. This use of the local variable was actually only accidentally a workaround to the bug concerning the (lack of) unique values for the global session id. That is, I made this change before finding the bug with the global session id. We need this local counter because, now that we may filter on SNI, our sampling should be based on a local counter of sessions after the application of the SNI filter rather than the global one that applies before the filter (and potentially any other filters we may add in the future). That is, if the sampling is set to 20, the user wants to dump 1/20 sessions matching the given SNI, not 1/20 of the global session id that no longer directly corresponds with the sessions after the filter.




----------------------------------------------------------------
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.

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