Add proxy.config.http.negative_revalidating_list (#12212)

* Add proxy.config.http.negative_revalidating_list

* Address comments

Author: masaori335

doc/admin-guide/files/records.yaml.en.rst

@@ -1928,6 +1928,12 @@ Negative Response Caching
 
    This configuration defaults to 1,800 seconds (30 minutes).
 
+.. ts:cv:: CONFIG proxy.config.http.negative_revalidating_list STRING 500 502 503 504
+   :reloadable:
+
+   The HTTP status codes for which the negative revalidating feature applies. Note that this is a
+   `STRING` configuration containing a space separated list of the desired HTTP status codes.
+
 Proxy User Variables
 ====================
 

include/proxy/http/HttpConfig.h

@@ -802,6 +802,9 @@ struct HttpConfigParams : public ConfigInfo {
   // bitset to hold the status codes that will BE cached with negative caching enabled
   HttpStatusBitset negative_caching_list;
 
+  // bitset to hold the status codes that will used by nagative revalidating enabled
+  HttpStatusBitset negative_revalidating_list;
+
   // All the overridable configurations goes into this class member, but they
   // are not copied over until needed ("lazy").
   OverridableHttpConfigParams oride;

src/proxy/http/HttpConfig.cc

@@ -635,33 +635,51 @@ register_stat_callbacks()
   });
 }
 
+/**
+  Parse list of HTTP status code and return HttpStatusBitset
+  - e.g. "204 305 403 404 414 500 501 502 503 504"
+ */
+static HttpStatusBitset
+parse_http_status_code_list(swoc::TextView status_list)
+{
+  HttpStatusBitset set;
+
+  auto is_sep{[](char c) { return isspace(c) || ',' == c || ';' == c; }};
+
+  while (!status_list.ltrim_if(is_sep).empty()) {
+    swoc::TextView span;
+    swoc::TextView token{status_list.take_prefix_if(is_sep)};
+    auto           n = swoc::svtoi(token, &span);
+    if (span.size() != token.size()) {
+      Error("Invalid status code '%.*s': not a number", static_cast<int>(token.size()), token.data());
+    } else if (n <= 0 || n >= HTTP_STATUS_NUMBER) {
+      Error("Invalid status code '%.*s': out of range", static_cast<int>(token.size()), token.data());
+    } else {
+      set[n] = true;
+    }
+  }
+
+  return set;
+}
+
 static bool
 set_negative_caching_list(const char *name, RecDataT dtype, RecData data, HttpConfigParams *c, bool update)
 {
   bool             ret = false;
   HttpStatusBitset set;
+
   // values from proxy.config.http.negative_caching_list
   if (0 == strcasecmp("proxy.config.http.negative_caching_list", name) && RECD_STRING == dtype && data.rec_string) {
     // parse the list of status codes
-    swoc::TextView status_list(data.rec_string, strlen(data.rec_string));
-    auto           is_sep{[](char c) { return isspace(c) || ',' == c || ';' == c; }};
-    while (!status_list.ltrim_if(is_sep).empty()) {
-      swoc::TextView span, token{status_list.take_prefix_if(is_sep)};
-      auto           n = swoc::svtoi(token, &span);
-      if (span.size() != token.size()) {
-        Error("Invalid status code '%.*s' for negative caching: not a number", static_cast<int>(token.size()), token.data());
-      } else if (n <= 0 || n >= HTTP_STATUS_NUMBER) {
-        Error("Invalid status code '%.*s' for negative caching: out of range", static_cast<int>(token.size()), token.data());
-      } else {
-        set[n] = true;
-      }
-    }
+    set = parse_http_status_code_list({data.rec_string, strlen(data.rec_string)});
   }
+
   // set the return value
   if (set != c->negative_caching_list) {
     c->negative_caching_list = set;
     ret                      = ret || update;
   }
+
   return ret;
 }
 
@@ -685,6 +703,47 @@ load_negative_caching_var(RecRecord const *r, void *cookie)
   set_negative_caching_list(r->name, r->data_type, r->data, c, false);
 }
 
+static bool
+set_negative_revalidating_list(const char *name, RecDataT dtype, RecData data, HttpConfigParams *c, bool update)
+{
+  bool             ret = false;
+  HttpStatusBitset set;
+
+  // values from proxy.config.http.negative_revalidating_list
+  if (0 == strcasecmp("proxy.config.http.negative_revalidating_list", name) && RECD_STRING == dtype && data.rec_string) {
+    // parse the list of status codes
+    set = parse_http_status_code_list({data.rec_string, strlen(data.rec_string)});
+  }
+
+  // set the return value
+  if (set != c->negative_revalidating_list) {
+    c->negative_revalidating_list = set;
+    ret                           = ret || update;
+  }
+
+  return ret;
+}
+
+// Method of getting the status code bitset
+static int
+negative_revalidating_list_cb(const char *name, RecDataT dtype, RecData data, void *cookie)
+{
+  HttpConfigParams *c = static_cast<HttpConfigParams *>(cookie);
+  // Signal an update if valid value arrived.
+  if (set_negative_revalidating_list(name, dtype, data, c, true)) {
+    http_config_cb(name, dtype, data, cookie);
+  }
+  return REC_ERR_OKAY;
+}
+
+// Method of loading the negative caching config bitset
+void
+load_negative_revalidating_var(RecRecord const *r, void *cookie)
+{
+  HttpConfigParams *c = static_cast<HttpConfigParams *>(cookie);
+  set_negative_revalidating_list(r->name, r->data_type, r->data, c, false);
+}
+
 /** Template for creating conversions and initialization for @c std::chrono based configuration variables.
  *
  * @tparam V The exact type of the configuration variable.
@@ -1020,6 +1079,8 @@ HttpConfig::startup()
   HttpEstablishStaticConfigLongLong(c.oride.negative_revalidating_lifetime, "proxy.config.http.negative_revalidating_lifetime");
   RecRegisterConfigUpdateCb("proxy.config.http.negative_caching_list", &negative_caching_list_cb, &c);
   RecLookupRecord("proxy.config.http.negative_caching_list", &load_negative_caching_var, &c, true);
+  RecRegisterConfigUpdateCb("proxy.config.http.negative_revalidating_list", &negative_revalidating_list_cb, &c);
+  RecLookupRecord("proxy.config.http.negative_revalidating_list", &load_negative_revalidating_var, &c, true);
 
   // Buffer size and watermark
   HttpEstablishStaticConfigLongLong(c.oride.default_buffer_size_index, "proxy.config.http.default_buffer_size");
@@ -1339,7 +1400,8 @@ HttpConfig::reconfigure()
   params->oride.ssl_client_sni_policy     = ats_strdup(m_master.oride.ssl_client_sni_policy);
   params->oride.ssl_client_alpn_protocols = ats_strdup(m_master.oride.ssl_client_alpn_protocols);
 
-  params->negative_caching_list = m_master.negative_caching_list;
+  params->negative_caching_list      = m_master.negative_caching_list;
+  params->negative_revalidating_list = m_master.negative_revalidating_list;
 
   params->oride.host_res_data            = m_master.oride.host_res_data;
   params->oride.host_res_data.conf_value = ats_strdup(m_master.oride.host_res_data.conf_value);

src/proxy/http/HttpTransact.cc

@@ -4319,15 +4319,12 @@ HttpTransact::handle_cache_operation_on_forward_server_response(State *s)
     SET_VIA_STRING(VIA_SERVER_RESULT, VIA_SERVER_SERVED);
     SET_VIA_STRING(VIA_PROXY_RESULT, VIA_PROXY_SERVED);
 
-    /* if we receive a 500, 502, 503 or 504 while revalidating
+    /* By default, if we receive a 500, 502, 503 or 504 while revalidating
        a document, treat the response as a 304 and in effect revalidate the document for
        negative_revalidating_lifetime. (negative revalidating)
      */
-
-    if ((server_response_code == HTTP_STATUS_INTERNAL_SERVER_ERROR || server_response_code == HTTP_STATUS_GATEWAY_TIMEOUT ||
-         server_response_code == HTTP_STATUS_BAD_GATEWAY || server_response_code == HTTP_STATUS_SERVICE_UNAVAILABLE) &&
-        s->cache_info.action == CACHE_DO_UPDATE && s->txn_conf->negative_revalidating_enabled &&
-        is_stale_cache_response_returnable(s)) {
+    if (s->txn_conf->negative_revalidating_enabled && s->http_config_param->negative_revalidating_list[server_response_code] &&
+        s->cache_info.action == CACHE_DO_UPDATE && is_stale_cache_response_returnable(s)) {
       HTTPStatus cached_response_code = s->cache_info.object_read->response_get()->status_get();
       if (!(cached_response_code == HTTP_STATUS_INTERNAL_SERVER_ERROR || cached_response_code == HTTP_STATUS_GATEWAY_TIMEOUT ||
             cached_response_code == HTTP_STATUS_BAD_GATEWAY || cached_response_code == HTTP_STATUS_SERVICE_UNAVAILABLE)) {

src/records/RecordsConfig.cc

@@ -488,6 +488,8 @@ static const RecordElement RecordsConfig[] =
   ,
   {RECT_CONFIG, "proxy.config.http.negative_caching_list", RECD_STRING, "204 305 403 404 414 500 501 502 503 504", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
   ,
+  {RECT_CONFIG, "proxy.config.http.negative_revalidating_list", RECD_STRING, "500 502 503 504", RECU_DYNAMIC, RR_NULL, RECC_NULL, nullptr, RECA_NULL}
+  ,
 
   //        #########################
   //        # proxy users variables #

tests/gold_tests/cache/negative-revalidating.test.py

@@ -21,47 +21,78 @@
 Test the negative revalidating feature.
 '''
 
+
+class NegativeRevalidatingTest:
+    _test_num: int = 0
+
+    def __init__(self, name: str, records_config: dict, replay_file: str):
+        self._tr = Test.AddTestRun(name)
+        self._replay_file = replay_file
+
+        self.__setupOriginServer()
+        self.__setupTS(records_config)
+        self.__setupClient()
+
+        NegativeRevalidatingTest._test_num += 1
+
+    def __setupClient(self):
+        self._tr.AddVerifierClientProcess(
+            f"client-{NegativeRevalidatingTest._test_num}", self._replay_file, http_ports=[self._ts.Variables.port])
+
+    def __setupOriginServer(self):
+        self._server = self._tr.AddVerifierServerProcess(f"server-{NegativeRevalidatingTest._test_num}", self._replay_file)
+
+    def __setupTS(self, records_config):
+        self._ts = Test.MakeATSProcess(f"ts-{NegativeRevalidatingTest._test_num}")
+        self._ts.Disk.records_config.update(records_config)
+        self._ts.Disk.remap_config.AddLine('map / http://127.0.0.1:{0}'.format(self._server.Variables.http_port))
+
+    def run(self):
+        self._tr.Processes.Default.StartBefore(self._ts)
+        self._tr.StillRunningAfter = self._ts
+
+
 #
 # Verify disabled negative_revalidating behavior.
 #
-ts = Test.MakeATSProcess("ts-negative-revalidating-disabled")
-ts.Disk.records_config.update(
-    {
+NegativeRevalidatingTest(
+    "Verify negative revalidating disabled", {
         'proxy.config.diags.debug.enabled': 1,
         'proxy.config.diags.debug.tags': 'http|cache',
         'proxy.config.http.insert_age_in_response': 0,
+        'proxy.config.http.insert_response_via_str': 2,
         'proxy.config.http.negative_revalidating_enabled': 0,
         'proxy.config.http.cache.max_stale_age': 6
-    })
-tr = Test.AddTestRun("Verify disabled negative revalidating behavior.")
-replay_file = "replay/negative-revalidating-disabled.replay.yaml"
-server = tr.AddVerifierServerProcess("server1", replay_file)
-server_port = server.Variables.http_port
-tr.AddVerifierClientProcess("client1", replay_file, http_ports=[ts.Variables.port])
-ts.Disk.remap_config.AddLine('map / http://127.0.0.1:{0}'.format(server_port))
-tr.Processes.Default.StartBefore(ts)
-tr.StillRunningAfter = ts
+    }, "replay/negative-revalidating-disabled.replay.yaml").run()
 
 #
 # Verify enabled negative_revalidating behavior.
 #
-ts = Test.MakeATSProcess("ts-negative-revalidating-enabled")
-ts.Disk.records_config.update(
+NegativeRevalidatingTest(
+    "Verify negative revalidating enabled",
     {
         'proxy.config.diags.debug.enabled': 1,
         'proxy.config.diags.debug.tags': 'http|cache',
         'proxy.config.http.insert_age_in_response': 0,
+        'proxy.config.http.insert_response_via_str': 2,
 
         # Negative revalidating is on by default. Verify this by leaving out the
         # following line and expect negative_revalidating to be enabled.
         # 'proxy.config.http.negative_revalidating_enabled': 1,
         'proxy.config.http.cache.max_stale_age': 6
-    })
-tr = Test.AddTestRun("Verify negative revalidating behavior.")
-replay_file = "replay/negative-revalidating-enabled.replay.yaml"
-server = tr.AddVerifierServerProcess("server2", replay_file)
-server_port = server.Variables.http_port
-tr.AddVerifierClientProcess("client2", replay_file, http_ports=[ts.Variables.port])
-ts.Disk.remap_config.AddLine('map / http://127.0.0.1:{0}'.format(server_port))
-tr.Processes.Default.StartBefore(ts)
-tr.StillRunningAfter = ts
+    },
+    "replay/negative-revalidating-enabled.replay.yaml").run()
+
+#
+# Verify negative_revalidating list behavior.
+#
+NegativeRevalidatingTest(
+    "Verify negative_revalidating_list behavior", {
+        'proxy.config.diags.debug.enabled': 1,
+        'proxy.config.diags.debug.tags': 'http|cache',
+        'proxy.config.http.insert_age_in_response': 0,
+        'proxy.config.http.insert_response_via_str': 2,
+        'proxy.config.http.cache.max_stale_age': 6,
+        'proxy.config.http.negative_revalidating_enabled': 1,
+        'proxy.config.http.negative_revalidating_list': "403 404"
+    }, "replay/negative-revalidating-list.replay.yaml").run()