diff --git a/plugins/experimental/cache_fill/configs.cc b/plugins/experimental/cache_fill/configs.cc index c9d1b7b3ff9..cf09a61a384 100644 --- a/plugins/experimental/cache_fill/configs.cc +++ b/plugins/experimental/cache_fill/configs.cc @@ -168,6 +168,10 @@ BgFetchConfig::readConfig(const char *config_file) continue; } ++cfg_value; // Drop leading character. + if (cfg_value.empty()) { + TSError("[%s] missing Content-Length size value, skipping config value", PLUGIN_NAME); + continue; + } swoc::TextView parsed; auto n = swoc::svtou(cfg_value, &parsed); if (parsed.size() != cfg_value.size()) { @@ -213,11 +217,14 @@ BgFetchConfig::bgFetchAllowed(TSHttpTxn txnp) const if (TSHttpTxnClientReqGet(txnp, &bufp, &hdr_loc) == TS_SUCCESS) { bool hasRangeHdrs = false; for (auto const &header : FILTER_HEADERS) { - if (TSMimeHdrFieldFind(bufp, hdr_loc, header.data(), header.size() == TS_SUCCESS)) { + TSMLoc field_loc = TSMimeHdrFieldFind(bufp, hdr_loc, header.data(), static_cast(header.size())); + if (field_loc != TS_NULL_MLOC) { + TSHandleMLocRelease(bufp, hdr_loc, field_loc); hasRangeHdrs = true; break; } } + TSHandleMLocRelease(bufp, TS_NULL_MLOC, hdr_loc); if (!hasRangeHdrs && _range_req_only) { Dbg(dbg_ctl, "_range_req_only=true; This transaction is not a range request"); return false; diff --git a/tests/gold_tests/pluginTest/cache_fill/cache_fill.test.py b/tests/gold_tests/pluginTest/cache_fill/cache_fill.test.py index 5584a602695..29647d98a29 100644 --- a/tests/gold_tests/pluginTest/cache_fill/cache_fill.test.py +++ b/tests/gold_tests/pluginTest/cache_fill/cache_fill.test.py @@ -229,4 +229,141 @@ def run(self): self.runTraffic() +class CacheFillRangeReqOnlyTest: + ''' + Cover the two options that hinge on detecting a range request: + --range-req-only=true (fill ONLY for a range request) and + --cache-range-req=false (do NOT fill for a range request). + + This runs in its own ATS process rather than sharing the one above, + because that one loads cache_fill globally with default options. A global + instance hooks every transaction, which would background fill these paths + regardless of the per-remap options and make the negative cases vacuous. + ''' + + def __init__(self): + self.setUpOriginServer() + self.setUpTS() + self.curl_and_args = '-s -D /dev/stdout -v -x localhost:{} -H "x-debug: x-cache,x-cache-key"'.format(self.ts.Variables.port) + + def addCacheableResponse(self, path, etag): + req = { + "headers": "GET {} HTTP/1.1\r\n".format(path) + "Host: www.example.com\r\n" + "\r\n", + "timestamp": "1469733493.993", + "body": "" + } + # No explicit Date: a fixed date in the past plus max-age makes the + # stored object stale on arrival, and every hit assertion would fail. + res = { + "headers": + "HTTP/1.1 200 OK\r\n" + "Cache-Control: max-age=300\r\n" + "Connection: close\r\n" + 'Etag: {}\r\n'.format(etag), + "timestamp": "1469733493.993", + "body": "hello hello" + } + self.server.addResponse("sessionlog.json", req, res) + + def setUpOriginServer(self): + self.server = Test.MakeOriginServer("server-range") + # Distinct paths that are not a prefix of one another, so a remap rule + # cannot match the wrong one. + self.addCacheableResponse("/fill_on_range", "994324f6-78f6bc3e8d639") + self.addCacheableResponse("/skip_when_plain", "772102f4-56f4bc1e6d417") + self.addCacheableResponse("/decline_on_range", "883213f5-67f5bc2e7d528") + + def setUpTS(self): + self.ts = Test.MakeATSProcess("ts-range") + + port = self.server.Variables.Port + self.ts.Disk.remap_config.AddLines( + [ + 'map http://www.example.com/fill_on_range http://127.0.0.1:{}/fill_on_range'.format(port) + + ' @plugin=cache_fill.so @pparam=--range-req-only=true', + 'map http://www.example.com/skip_when_plain http://127.0.0.1:{}/skip_when_plain'.format(port) + + ' @plugin=cache_fill.so @pparam=--range-req-only=true', + 'map http://www.example.com/decline_on_range http://127.0.0.1:{}/decline_on_range'.format(port) + + ' @plugin=cache_fill.so @pparam=--cache-range-req=false', + ]) + + self.ts.Disk.plugin_config.AddLine('xdebug.so --enable=x-cache,x-cache-key') + + self.ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'cache_fill', + }) + + def test_rangeRequestPrimesCache(self): + # A range request is what --range-req-only asks for, so the background + # fetch should run and pull the whole object into cache. The client + # still sees the origin's 200 on this first pass. + tr = Test.AddTestRun("range-req-only: range request starts a background fill") + ps = tr.Processes.Default + ps.StartBefore(self.server, ready=When.PortOpen(self.server.Variables.Port)) + ps.StartBefore(self.ts) + tr.MakeCurlCommand(self.curl_and_args + ' http://www.example.com/fill_on_range -r 0-4', ts=self.ts) + ps.ReturnCode = 0 + ps.Streams.stdout.Content = Testers.ContainsExpression("X-Cache: miss", "expected cache miss") + ps.Streams.stdout.Content += Testers.ContainsExpression("200 OK", "expected the origin's 200") + tr.StillRunningAfter = self.ts + + def test_rangeRequestFilledCache(self): + # The fill above must have happened: the object is now served from + # cache, and ATS synthesizes the 206 from the cached full body. This is + # the assertion that fails when the range header lookup is broken. + tr = Test.AddTestRun("range-req-only: second range request is served from the filled cache") + ps = tr.Processes.Default + tr.DelayStart = 2 # let the background fetch finish writing to cache + tr.MakeCurlCommand(self.curl_and_args + ' http://www.example.com/fill_on_range -r 0-4', ts=self.ts) + ps.ReturnCode = 0 + ps.Streams.stdout.Content = Testers.ContainsExpression( + "X-Cache: hit-fresh", "expected a cache hit from the background fill") + ps.Streams.stdout.Content += Testers.ContainsExpression("206 Partial Content", "expected 206 from the cached object") + ps.Streams.stdout.Content += Testers.ContainsExpression( + "Content-Range: bytes 0-4/11", "expected Content-Range: bytes 0-4/11") + tr.StillRunningAfter = self.ts + + def test_plainRequestIsAMiss(self): + tr = Test.AddTestRun("range-req-only: non-range request is a miss") + ps = tr.Processes.Default + tr.MakeCurlCommand(self.curl_and_args + ' http://www.example.com/skip_when_plain', ts=self.ts) + ps.ReturnCode = 0 + ps.Streams.stdout.Content = Testers.ContainsExpression("X-Cache: miss", "expected cache miss") + tr.StillRunningAfter = self.ts + + def test_cacheRangeReqFalseDeclinesRange(self): + # The mirror option: --cache-range-req=false must decline the fill for a + # range request. Under the broken lookup hasRangeHdrs was always false, + # so this branch never ran either -- which makes this case an + # independent check on the same fix. + tr = Test.AddTestRun("cache-range-req=false: range request is declined") + ps = tr.Processes.Default + tr.MakeCurlCommand(self.curl_and_args + ' http://www.example.com/decline_on_range -r 0-4', ts=self.ts) + ps.ReturnCode = 0 + ps.Streams.stdout.Content = Testers.ContainsExpression("X-Cache: miss", "expected cache miss") + tr.StillRunningAfter = self.ts + + def check_pluginDecisions(self): + # Cache state cannot show the negative cases: a plain cacheable response + # is stored by ordinary proxy caching whether or not the plugin + # background fills, so both outcomes look the same from the client. + # Assert on the plugin's own decisions instead. Under a fix that simply + # always filled, neither decline line would ever appear. + self.ts.Disk.traffic_out.Content = Testers.ContainsExpression( + "_range_req_only=true; This transaction is not a range request", + "expected the plain request to be declined as a non-range request") + self.ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "_cache_range_req=false; This transaction is a range request", + "expected the range request to be declined when --cache-range-req=false") + # ...and the --range-req-only range request must have been accepted. + self.ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "scheduling background fetch", "expected the range request to schedule a background fetch") + + def run(self): + self.test_rangeRequestPrimesCache() + self.test_rangeRequestFilledCache() + self.test_plainRequestIsAMiss() + self.test_cacheRangeReqFalseDeclinesRange() + self.check_pluginDecisions() + + CacheFillTest().run() +CacheFillRangeReqOnlyTest().run()