diff --git a/doc/admin-guide/plugins/rate_limit.en.rst b/doc/admin-guide/plugins/rate_limit.en.rst index c96f6a3684b..57c9d96c4c2 100644 --- a/doc/admin-guide/plugins/rate_limit.en.rst +++ b/doc/admin-guide/plugins/rate_limit.en.rst @@ -142,7 +142,14 @@ configuration file. The basic use is as:: The YAML configuration can have the following format, where the various sections -and nodes are documented below. +and nodes are documented below. Unknown keys at any level cause configuration +loading to fail, with a diagnostic identifying the key, node, and line number. +An invalid value, such as a non-numeric ``limit``, fails the load the same way. +A failed reload keeps the previous configuration active. Use ``max_age`` (with +an underscore) for the ``queue``, ``ip-rep``, and ``perma-block`` aging settings. + +The file must hold a YAML map. An empty file is an error. To load the plugin +with no rules, write ``selector: []``. .. code-block:: yaml diff --git a/doc/check_response_validity-empty-eos.md b/doc/check_response_validity-empty-eos.md new file mode 100644 index 00000000000..c76fa1f9397 --- /dev/null +++ b/doc/check_response_validity-empty-eos.md @@ -0,0 +1,72 @@ +# Empty origin response with EOS + +This note walks the code path for the case where the origin server sends no response bytes at all and then closes the connection with EOS. + +## Short answer + +- In the real transaction path, `check_response_validity()` is not called. +- The transaction reaches `HandleResponse()` with `current.state == CONNECTION_CLOSED`, so `is_response_valid()` returns `false` early and sets `response_error = CONNECTION_OPEN_FAILED`. +- If `check_response_validity()` were called directly on the untouched `server_response` header object anyway, it would return `ResponseError_t::MISSING_STATUS_CODE`. + +## Code path + +1. `HttpSM::setup_server_read_response_header()` resets the origin response header with: + - `t_state.hdr_info.server_response.destroy();` + - `t_state.hdr_info.server_response.create(HTTPType::RESPONSE);` + + That creates a valid `HTTPHdr` object with response polarity, but its fields are still zero-initialized. In particular, the response status is still `HTTPStatus::NONE`. + +2. `HttpSM::state_read_server_response_header()` receives `VC_EVENT_EOS` before any response bytes are read. + - It sets `server_entry->eos = true`. + - `server_response_hdr_bytes` is still `0`. + - It then calls `t_state.hdr_info.server_response.parse_resp(..., eof = true)`. + +3. `HTTPHdr::parse_resp(HTTPParser *, IOBufferReader *, int *, bool)` handles the empty-buffer EOF case specially: + - if `b_avail <= 0` + - and `eof == true` + - and `start == nullptr` + - then it returns `ParseResult::ERROR` immediately. + + So an empty origin response plus EOS is treated as a parse failure. + +4. Back in `HttpSM::state_read_server_response_header()`, this lands in the `ParseResult::ERROR` branch. + - It sets `t_state.current.state = HttpTransact::PARSE_ERROR`. + - Because the event was `VC_EVENT_EOS`, it calls `handle_server_setup_error(VC_EVENT_EOS, data)`. + +5. `HttpSM::handle_server_setup_error()` then converts the state to the connection-close path: + - `t_state.current.state = HttpTransact::CONNECTION_CLOSED` + - `t_state.set_connect_fail(EPIPE)` + - `call_transact_and_set_next_state(HttpTransact::HandleResponse)` + +6. `HttpTransact::HandleResponse()` calls: + + `HttpTransact::is_response_valid(s, &s->hdr_info.server_response)` + + But `is_response_valid()` starts with a guard: + + - if `s->current.state != CONNECTION_ALIVE` + - set `s->hdr_info.response_error = ResponseError_t::CONNECTION_OPEN_FAILED` + - return `false` + + Because the state is already `CONNECTION_CLOSED`, execution returns here. `check_response_validity()` is never reached. + +## What `check_response_validity()` would return if called directly + +Even though the live path does not call it, the return value is straightforward from the initialized header state: + +1. `incoming_hdr` is not null. +2. `incoming_hdr->type_get() == HTTPType::RESPONSE` because the header was created with `create(HTTPType::RESPONSE)`. +3. `incoming_hdr->status_get() == HTTPStatus::NONE` because no bytes were parsed into the header. +4. `HttpTransact::check_response_validity()` therefore hits: + + `if (incoming_status == HTTPStatus::NONE) { return ResponseError_t::MISSING_STATUS_CODE; }` + +So the helper's direct return would be `ResponseError_t::MISSING_STATUS_CODE`. + +## Conclusion + +For an origin response of "0 bytes, then EOS": + +- Actual transaction behavior: `is_response_valid()` returns `false` early with `response_error = CONNECTION_OPEN_FAILED`. +- `check_response_validity()` is not called on the live path. +- Hypothetical direct call to `check_response_validity(&server_response)`: `ResponseError_t::MISSING_STATUS_CODE`. diff --git a/doc/connection_error_keepalive.md b/doc/connection_error_keepalive.md new file mode 100644 index 00000000000..3b0d1c50725 --- /dev/null +++ b/doc/connection_error_keepalive.md @@ -0,0 +1,154 @@ +# Connection-Error Handling and Client Keep-Alive Shutdown + +This note traces the Traffic Server (TS) code path from a failed parent/origin connection through to the moment TS forces the downstream client connection into non-keepalive, and describes how the client reacts. Each section includes the exact code that participates in the decision. + +## 1. Detecting the Connect Failure + +`HttpSM::state_http_server_open` drives the non-blocking connect. Any connect timeout or socket error drops into the `VC_EVENT_*` branch below, which stamps the state and records the errno before closing the half-open `NetVConnection`: + +```c++ +int +HttpSM::state_http_server_open(int event, void *data) +{ + ... + case VC_EVENT_INACTIVITY_TIMEOUT: + case VC_EVENT_ACTIVE_TIMEOUT: + t_state.set_connect_fail(ETIMEDOUT); + /* fallthrough */ + case VC_EVENT_ERROR: + case VC_EVENT_EOS: + case NET_EVENT_OPEN_FAILED: { + t_state.current.state = HttpTransact::CONNECTION_ERROR; + ... + if (_netvc != nullptr) { + if (event == VC_EVENT_ERROR || event == NET_EVENT_OPEN_FAILED) { + t_state.set_connect_fail(_netvc->lerrno); + } + _netvc->do_io_close(); + _netvc = nullptr; + } + ... + } + ... +} +``` + +If the error happens later—while writing the request header or waiting for the response—`HttpSM::handle_server_setup_error` applies the same bookkeeping (`t_state.current.state = CONNECTION_ERROR`, `set_connect_fail`, close VC) before handing control back to Traffic Cop. + +## 2. Transact Logs the Failure + +`HttpTransact::handle_response_from_server` looks at `t_state.current.state`. When it sees `CONNECTION_ERROR` it logs the failure and either retries or bails: + +```c++ +void +HttpTransact::handle_response_from_server(State *s) +{ + ... + case CONNECTION_ERROR: + ... + if (is_request_retryable(s) && s->current.retry_attempts.get() < max_connect_retries && + !HttpTransact::is_response_valid(s, &s->hdr_info.server_response)) { + ... + retry_server_connection_not_open(s, s->current.state, max_connect_retries); + ... + } else { + error_log_connection_failure(s, s->current.state); + TxnDbg(..., "Error. No more retries."); + SET_VIA_STRING(VIA_DETAIL_SERVER_CONNECT, VIA_DETAIL_SERVER_FAILURE); + handle_server_connection_not_open(s); + } + break; + ... +} +``` + +Every retry first forces the failed server session out of the keep-alive pool: + +```c++ +void +HttpTransact::retry_server_connection_not_open(State *s, ServerState_t conn_state, unsigned max_retries) +{ + ... + s->current.server->keep_alive = HTTPKeepAlive::NO_KEEPALIVE; + s->current.retry_attempts.increment(); +} +``` + +## 3. Forcing the User-Agent Connection to Close + +When the retry budget is exhausted, `handle_server_connection_not_open` routes to `handle_parent_down` / `handle_server_down`, which build an internal error. `HttpTransact::build_error_response` deliberately disables user-agent keep-alive before those headers are emitted: + +```c++ +void +HttpTransact::build_error_response(State *s, HTTPStatus status_code, const char *reason, const char *body_type) +{ + ... + if (status_code == HTTPStatus::REQUEST_TIMEOUT || s->hdr_info.client_request.get_content_length() != 0 || + s->client_info.transfer_encoding == HttpTransact::TransferEncoding_t::CHUNKED) { + s->client_info.keep_alive = HTTPKeepAlive::NO_KEEPALIVE; + } + ... + if ((s->state_machine->get_ua_txn() && s->state_machine->get_ua_txn()->is_outbound_transparent()) && + (status_code == HTTPStatus::INTERNAL_SERVER_ERROR || status_code == HTTPStatus::GATEWAY_TIMEOUT || + status_code == HTTPStatus::BAD_GATEWAY || status_code == HTTPStatus::SERVICE_UNAVAILABLE)) { + s->client_info.keep_alive = HTTPKeepAlive::NO_KEEPALIVE; + } + ... +} +``` + +`handle_response_keep_alive_headers` then honors that flag by inserting `Connection: close` (or `Proxy-Connection: close`) into the proxy response and calling `set_close_connection` on the client session: + +```c++ +void +HttpTransact::handle_response_keep_alive_headers(State *s, HTTPVersion ver, HTTPHdr *heads) +{ + ... + if (s->client_info.keep_alive != HTTPKeepAlive::KEEPALIVE) { + ka_action = KA_Action_t::DISABLED; + } + ... + case KA_Action_t::CLOSE: + case KA_Action_t::DISABLED: + if (s->client_info.keep_alive != HTTPKeepAlive::NO_KEEPALIVE || (ver == HTTP_1_1)) { + if (s->client_info.proxy_connect_hdr) { + heads->value_set(..., "close"sv); + } else if (s->state_machine->get_ua_txn() != nullptr) { + s->state_machine->get_ua_txn()->set_close_connection(*heads); + } + s->client_info.keep_alive = HTTPKeepAlive::NO_KEEPALIVE; + } + break; + ... +} +``` + +## 4. Client-Side Behavior + +On the downstream (first-layer) side, `HttpSM::tunnel_handler_ua` trusts the `keep_alive` flag. Because the earlier logic set it to `NO_KEEPALIVE`, the write-complete handler leaves `close_connection = true`, which causes the user-agent session to terminate instead of re-entering the keep-alive pool: + +```c++ +int +HttpSM::tunnel_handler_ua(int event, HttpTunnelConsumer *c) +{ + ... + case VC_EVENT_WRITE_COMPLETE: + c->write_success = true; + t_state.client_info.abort = HttpTransact::DIDNOT_ABORT; + if (t_state.client_info.keep_alive == HTTPKeepAlive::KEEPALIVE) { + ... + close_connection = false; + } + break; + ... +} +``` + +Earlier, `HttpSM::do_drain_request_body` (and `set_close_connection`) already updated the downstream headers, so the first-layer ATS receives an explicit `Connection: close` and, per RFC 7230 and `Http1ClientSession::set_close_connection`, immediately closes its side instead of tracking the socket for reuse. + +## Summary + +1. Connect failure → `CONNECTION_ERROR` state (`HttpSM` sets it when the connect VC fails). +2. `HttpTransact` logs `CONNECT: … CONNECTION_ERROR`, optionally retries, and forces the failed server session out of the pool. +3. The generated error response clears `client_info.keep_alive`, inserts `Connection: close`, and calls `set_close_connection`. +4. The downstream client (the first-layer ATS) sees the header, closes the socket, and drops it from its keep-alive pool—ensuring sockets used for failed multi-layer connects can’t be reused. diff --git a/doc/hdrtoken-analysis.md b/doc/hdrtoken-analysis.md new file mode 100644 index 00000000000..f8bdd5bc2dc --- /dev/null +++ b/doc/hdrtoken-analysis.md @@ -0,0 +1,64 @@ +# HdrToken System Analysis + +This document describes the purpose and inner workings of the `HdrToken` system in Traffic Server, including the algorithms used for initialization and runtime parsing. + +## Purpose + +The `HdrToken` system is an optimization designed to efficiently handle frequently occurring strings in HTTP headers, such as header names (e.g., "Content-Type") and common values (e.g., "chunked"). + +Instead of repeatedly comparing strings, the system maps these "well-known strings" (WKS) to unique integer identifiers and stores them in a special memory heap. At runtime, incoming header strings can be quickly checked against this optimized data structure. If a match is found, the system can use the faster integer representation and canonical string pointer for internal logic instead of performing more expensive string operations. + +## Algorithms and Data Structures + +The `HdrToken` system uses different algorithms for its one-time initialization phase and its runtime parsing phase. + +### Initialization (`hdrtoken_init`) + +During server startup, the `hdrtoken_init` function is called once to set up the necessary data structures for runtime use. + +1. **Well-Known Strings Lists**: Several static, compile-time arrays define the strings and their metadata: + * `_hdrtoken_strs`: The master list of all well-known strings. + * `_hdrtoken_strs_type_initializers`: Associates certain strings with a `HdrTokenType` (e.g., "GET" is a METHOD). + * `_hdrtoken_strs_field_initializers`: Associates header field names with slot IDs, presence masks, and flags. + +2. **DFA (Deterministic Finite Automaton)**: + * **Algorithm**: A DFA is compiled from the master `_hdrtoken_strs` list. + * **Purpose**: The DFA's sole purpose is to act as a temporary lookup mechanism during the initialization phase. It provides a robust and consistent way to find the canonical index of a string from the metadata arrays (`_hdrtoken_strs_type_initializers`, etc.) within the master `_hdrtoken_strs` array. This is crucial for correctly linking metadata to the right string before the primary runtime data structures are built. For example, it's used to find the index of "GET" so its `HdrTokenType` can be set to `METHOD`. The DFA is configured to be case-insensitive. After initialization is complete, the DFA is **not** used for parsing headers at runtime. + +3. **Specialized Memory Heap**: + * All well-known strings from the master list are copied into a single contiguous block of memory (a dedicated heap). Each string in the heap is prefixed with a `HdrTokenHeapPrefix` struct that stores its length, index, and other metadata. + * This allows for a very fast runtime check: if a string's pointer falls within the address range of this heap, it is instantly identifiable as a well-known string without needing a hash or comparison. + +4. **Hash Table (`hdrtoken_hash_table`)**: + * **Algorithm**: FNV-1a (Fowler–Noll–Vo) hash. + * **Purpose**: This is the primary data structure for fast lookups of unknown strings at runtime. After the heap is populated, the `hdrtoken_hash_init` function iterates through the well-known strings, calculates the FNV-1a hash for each one, and stores the canonical string pointer (from the heap) and its hash in the `hdrtoken_hash_table`. + +### Runtime Parsing and Tokenization + +When the server is running, headers are parsed and strings are "tokenized" (converted to a WKS representation) on the fly. This process is handled by the `hdrtoken_tokenize` function. + +1. **Fast Path (Pointer Check)**: The function first checks if the string pointer `hdrtoken_is_wks()` is already a known token by seeing if its address falls within the specialized memory heap. If it is, the string has already been tokenized, and the function immediately returns its index. + +2. **Slow Path (Hash Lookup)**: If the string is not already a known token pointer (i.e., it's a fresh string from the network), the following occurs: + * **Algorithm**: FNV-1a hash and direct memory comparison. + * The FNV-1a hash of the input string is calculated. + * This hash is used to find a potential match in the `hdrtoken_hash_table`. + * A three-part check is performed to validate the match: + 1. The hash bucket must contain a valid string (`bucket->wks != nullptr`). + 2. The hash of the string in the bucket must match the hash of the input string (`bucket->hash == hash`). + 3. The length of the string in the bucket must match the length of the input string (`hdrtoken_wks_to_length(bucket->wks) == string_len`). + +This final length check is critical. The combination of a matching hash and matching length provides a very strong guarantee of a **whole-string match**. It prevents a shorter string like "Accept" from incorrectly matching a longer string like "Accept-Ranges". + +If all checks pass, the string has been successfully "tokenized". The function returns the WKS index. Other parts of the system can then use `hdrtoken_index_to_wks(index)` to get a pointer to the canonical, shared string from the WKS heap. If the checks fail, the string is not a well-known token. + +## Sequence of Steps for Parsing a Header String + +1. An unknown header string is passed to `hdrtoken_tokenize`. +2. A quick check (`hdrtoken_is_wks`) is performed to see if the string's memory address is already in the well-known string heap. If yes, the token is found, and the process ends. +3. If not, the string's FNV-1a hash is calculated. +4. A bucket in the `hdrtoken_hash_table` is located using the hash. +5. The hash stored in the bucket is compared to the input string's hash. +6. The length of the string in the bucket is compared to the input string's length. +7. If both the hash and the length match, the string is successfully tokenized. Its WKS index is returned for use by the caller. +8. If any of these checks fail, the string is not a well-known token. diff --git a/doc/release-notes/upgrading.en.rst b/doc/release-notes/upgrading.en.rst index fd9c145f7e7..e9e9f0da674 100644 --- a/doc/release-notes/upgrading.en.rst +++ b/doc/release-notes/upgrading.en.rst @@ -30,6 +30,24 @@ with :cpp:func:`TSPortDescriptorDestroy`. The descriptor can be destroyed immediately after :cpp:func:`TSPortDescriptorAccept` returns because the listener does not retain it. +Plugins +------- + +Changes to Features +~~~~~~~~~~~~~~~~~~~ +The following plugins have been changed in this version of ATS. + +* rate_limit - The YAML configuration is now validated strictly: + + * An unknown key at any level makes the configuration fail to load. Correct any + misspelled key, such as ``max-age`` in place of ``max_age``. + * An invalid value, such as a non-numeric ``limit``, also fails the load. + * An empty configuration file is an error. Write ``selector: []`` to load the + plugin with no rules. + + A failed reload keeps the previous configuration active. For more details, please + check :ref:`admin-plugins-rate-limit`. + Upgrading to ATS v10.x ====================== diff --git a/plugins/experimental/rate_limit/ip_reputation.cc b/plugins/experimental/rate_limit/ip_reputation.cc index 427806630c0..1313a791fa5 100644 --- a/plugins/experimental/rate_limit/ip_reputation.cc +++ b/plugins/experimental/rate_limit/ip_reputation.cc @@ -81,6 +81,10 @@ SieveLru::hasher(const std::string &ip, u_short family) // Mostly a convenience bool SieveLru::parseYaml(const YAML::Node &node) { + if (!validate_yaml_keys(node, "ip-rep", {"name", "buckets", "size", "percentage", "max_age", "perma-block"})) { + return false; + } + if (node["buckets"]) { _num_buckets = node["buckets"].as(); } @@ -100,21 +104,20 @@ SieveLru::parseYaml(const YAML::Node &node) if (node["perma-block"]) { const YAML::Node &perma = node["perma-block"]; - if (perma.IsMap()) { - if (perma["limit"]) { - _permablock_limit = perma["limit"].as(); - } + if (!validate_yaml_keys(perma, "perma-block", {"limit", "threshold", "max_age"})) { + return false; + } - if (perma["threshold"]) { - _permablock_threshold = perma["threshold"].as(); - } + if (perma["limit"]) { + _permablock_limit = perma["limit"].as(); + } - if (perma["max_age"]) { - _permablock_max_age = std::chrono::seconds(perma["max_age"].as()); - } - } else { - TSError("[%s] The perma-block node must be a map", PLUGIN_NAME); - return false; + if (perma["threshold"]) { + _permablock_threshold = perma["threshold"].as(); + } + + if (perma["max_age"]) { + _permablock_max_age = std::chrono::seconds(perma["max_age"].as()); } } diff --git a/plugins/experimental/rate_limit/limiter.h b/plugins/experimental/rate_limit/limiter.h index 715cb6316e6..791c7a578bd 100644 --- a/plugins/experimental/rate_limit/limiter.h +++ b/plugins/experimental/rate_limit/limiter.h @@ -202,7 +202,7 @@ template class RateLimiter } if (node["rate"]) { - _limit = node["rate"].as(); + _rate = node["rate"].as(); } // ToDo: One or both of these should be required @@ -211,6 +211,10 @@ template class RateLimiter // If enabled, we default to UINT32_MAX, but the object default is still 0 (no queue) if (queue) { + if (!validate_yaml_keys(queue, "queue", {"size", "max_age"})) { + return false; + } + _max_queue = queue["size"] ? queue["size"].as() : UINT32_MAX; if (queue["max_age"]) { @@ -221,6 +225,10 @@ template class RateLimiter const YAML::Node &metrics = node["metrics"]; if (metrics) { + if (!validate_yaml_keys(metrics, "metrics", {"prefix", "tag"})) { + return false; + } + std::string prefix = metrics["prefix"] ? metrics["prefix"].as() : RATE_LIMITER_METRIC_PREFIX; std::string tag = metrics["tag"] ? metrics["tag"].as() : name(); diff --git a/plugins/experimental/rate_limit/lists.cc b/plugins/experimental/rate_limit/lists.cc index 83daa08b5f1..afa40b1e938 100644 --- a/plugins/experimental/rate_limit/lists.cc +++ b/plugins/experimental/rate_limit/lists.cc @@ -22,6 +22,10 @@ bool List::IP::parseYaml(const YAML::Node &node) { + if (!validate_yaml_keys(node, "lists", {"name", "cidr"})) { + return false; + } + const YAML::Node &cidr = node["cidr"]; if (cidr && cidr.IsSequence()) { diff --git a/plugins/experimental/rate_limit/sni_limiter.cc b/plugins/experimental/rate_limit/sni_limiter.cc index 36bca010045..93486c9437a 100644 --- a/plugins/experimental/rate_limit/sni_limiter.cc +++ b/plugins/experimental/rate_limit/sni_limiter.cc @@ -30,7 +30,9 @@ int gVCIdx = -1; bool SniRateLimiter::parseYaml(const YAML::Node &node) { - super_type::parseYaml(node); + if (!super_type::parseYaml(node)) { + return false; + } if (node["ip-rep"]) { auto ipr_name = node["ip-rep"].as(); diff --git a/plugins/experimental/rate_limit/sni_selector.cc b/plugins/experimental/rate_limit/sni_selector.cc index 02fc08f15b9..fad36001737 100644 --- a/plugins/experimental/rate_limit/sni_selector.cc +++ b/plugins/experimental/rate_limit/sni_selector.cc @@ -42,6 +42,35 @@ SniSelector::yamlParser(const std::string &yaml_file) return false; } + // yaml-cpp throws out of as() on a malformed value, e.g. "limit: abc". Contain it here so such + // a configuration fails the load rather than terminating the process during a reload. + try { + return parseConfig(config, yaml_file); + } catch (YAML::Exception const &e) { + TSError("[%s] Invalid value in configuration file: %s.", PLUGIN_NAME, e.what()); + return false; + } +} + +bool +SniSelector::parseConfig(const YAML::Node &config, const std::string &yaml_file) +{ + if (config.IsNull()) { + TSError("[%s] The configuration file is empty, use 'selector: []' to configure no rules", PLUGIN_NAME); + return false; + } + + if (!validate_yaml_keys(config, "configuration", {"lists", "ip-rep", "selector"})) { + return false; + } + + for (const auto *key : {"lists", "ip-rep", "selector"}) { + if (config[key] && !config[key].IsSequence()) { + TSError("[%s] The %s node must be a sequence at line %d", PLUGIN_NAME, key, config[key].Mark().line + 1); + return false; + } + } + _yaml_file = yaml_file; // First build the Lists, if any @@ -113,7 +142,13 @@ SniSelector::yamlParser(const std::string &yaml_file) for (const auto &i : sel) { const YAML::Node &sni = i; - if (sni.IsMap() && !sni["sni"].IsSequence()) { + if (!validate_yaml_keys(sni, "selector", {"sni", "aliases", "limit", "rate", "queue", "metrics", "ip-rep", "exclude"})) { + return false; + } + + // On a const node, operator[] yields a zombie for a missing key, and IsScalar() throws on it. + // The boolean test is safe, so it has to come first. + if (sni["sni"] && sni["sni"].IsScalar()) { auto name = sni["sni"].as(); if (nullptr != findLimiter(name)) { @@ -167,7 +202,7 @@ SniSelector::yamlParser(const std::string &yaml_file) } } - Dbg(dbg_ctl, "Succesfully loaded YAML file: %s", yaml_file.c_str()); + Dbg(dbg_ctl, "Successfully loaded YAML file: %s", yaml_file.c_str()); return true; } diff --git a/plugins/experimental/rate_limit/sni_selector.h b/plugins/experimental/rate_limit/sni_selector.h index b25c913f62b..bbb1f0ce460 100644 --- a/plugins/experimental/rate_limit/sni_selector.h +++ b/plugins/experimental/rate_limit/sni_selector.h @@ -185,6 +185,8 @@ class SniSelector static void startup(const std::string &yaml_file); private: + bool parseConfig(const YAML::Node &config, const std::string &yaml_file); + std::string _yaml_file; bool _needs_queue_cont = false; TSCont _queue_cont = nullptr; // Continuation processing the queue periodically diff --git a/plugins/experimental/rate_limit/utilities.cc b/plugins/experimental/rate_limit/utilities.cc index 37b682dbcb8..1e21fcc0cdf 100644 --- a/plugins/experimental/rate_limit/utilities.cc +++ b/plugins/experimental/rate_limit/utilities.cc @@ -21,6 +21,9 @@ #include "ts/remap.h" #include "utilities.h" +#include +#include + namespace rate_limit_ns { DbgCtl dbg_ctl{PLUGIN_NAME}; @@ -122,3 +125,28 @@ getDescriptionFromUrl(const char *url) return description; } + +bool +validate_yaml_keys(const YAML::Node &node, const char *context, std::initializer_list keys) +{ + if (!node.IsMap()) { + TSError("[%s] The %s node must be a map", PLUGIN_NAME, context); + return false; + } + + for (const auto &entry : node) { + if (!entry.first.IsScalar()) { + TSError("[%s] The %s node has a non-scalar key at line %d", PLUGIN_NAME, context, entry.first.Mark().line + 1); + return false; + } + + const auto &key = entry.first.Scalar(); + + if (std::find(keys.begin(), keys.end(), key) == keys.end()) { + TSError("[%s] Unknown key '%s' in %s node at line %d", PLUGIN_NAME, key.c_str(), context, entry.first.Mark().line + 1); + return false; + } + } + + return true; +} diff --git a/plugins/experimental/rate_limit/utilities.h b/plugins/experimental/rate_limit/utilities.h index 6069fb171d8..4a4611d733e 100644 --- a/plugins/experimental/rate_limit/utilities.h +++ b/plugins/experimental/rate_limit/utilities.h @@ -17,13 +17,23 @@ */ #pragma once -#include #include +#include +#include +#include #include "ts/ts.h" +namespace YAML +{ +class Node; +} + constexpr char const PLUGIN_NAME[] = "rate_limit"; +/// Reject unknown keys and malformed YAML mappings with a configuration diagnostic. +bool validate_yaml_keys(const YAML::Node &node, const char *context, std::initializer_list keys); + void delayHeader(TSHttpTxn txnp, const std::string &header, std::chrono::milliseconds delay); void retryAfter(TSHttpTxn txnp, unsigned retry); std::string getDescriptionFromUrl(const char *url); diff --git a/plugins/prefetch/benchmark_evaluate b/plugins/prefetch/benchmark_evaluate new file mode 100755 index 00000000000..50b82b2df37 Binary files /dev/null and b/plugins/prefetch/benchmark_evaluate differ diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py new file mode 100644 index 00000000000..458fb65cb11 --- /dev/null +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_yaml_keys.test.py @@ -0,0 +1,128 @@ +# 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. + +import copy +import yaml + +Test.Summary = 'rate_limit rejects unknown YAML keys at every configuration level.' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) + + +class TestYamlKeys: + """Exercise configuration loading without sending traffic.""" + + def __init__(self) -> None: + config = { + 'lists': [{ + 'name': 'local', + 'cidr': ['127.0.0.1/32'] + }], + 'ip-rep': + [ + { + 'name': 'reputation', + 'buckets': 2, + 'size': 4, + 'percentage': 90, + 'max_age': 300, + 'perma-block': { + 'limit': 100, + 'threshold': 1, + 'max_age': 1800 + }, + } + ], + 'selector': + [ + { + 'sni': 'test.example.com', + 'aliases': ['alias.example.com'], + 'limit': 10, + 'rate': 0, + 'queue': { + 'size': 5, + 'max_age': 30 + }, + 'metrics': { + 'prefix': 'plugin.rate_limit', + 'tag': 'valid' + }, + 'ip-rep': 'reputation', + 'exclude': 'local', + } + ], + } + self._configure('valid', config) + for name, path, key, context in [ + ('root', (), 'selecter', 'configuration'), + ('list', ('lists', 0), 'cidrs', 'lists'), + ('selector', ('selector', 0), 'limti', 'selector'), + ('queue', ('selector', 0, 'queue'), 'max-age', 'queue'), + ('metrics', ('selector', 0, 'metrics'), 'prefxi', 'metrics'), + ('iprep', ('ip-rep', 0), 'max-age', 'ip-rep'), + ('perma', ('ip-rep', 0, 'perma-block'), 'max-age', 'perma-block'), + ]: + invalid = copy.deepcopy(config) + node = invalid + for part in path: + node = node[part] + node[key] = 1 + self._configure(name, invalid, f"Unknown key '{key}' in {context} node at line [0-9]+") + for name, config, error in [ + ('misspelled-sni', {'selector': [{'sin': 'test'}]}, "Unknown key 'sin' in selector node at line [0-9]+"), + ('no-sni', {'selector': [{'limit': 10}]}, 'selector node is not a map or without a name'), + ('bad-queue', {'selector': [{'sni': 'test', 'queue': []}]}, 'The queue node must be a map'), + ('bad-metrics', {'selector': [{'sni': 'test', 'metrics': []}]}, 'The metrics node must be a map'), + ('bad-selector', {'selector': {'sni': 'test'}}, 'The selector node must be a sequence'), + ('non-scalar-key', {'selector': [{'sni': 'test', 'queue': {('bad', 'key'): 1}}]}, + 'The queue node has a non-scalar key at line [0-9]+'), + ('bad-value', {'selector': [{'sni': 'test', 'limit': 'abc'}]}, 'Invalid value in configuration file'), + ('empty-config', None, 'The configuration file is empty'), + ]: + self._configure(name, config, error) + + @staticmethod + def _configure(name: str, config: dict | None, error: str | None = None) -> None: + ts = Test.MakeATSProcess(name, disable_log_checks=error is not None) + ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rate_limit', + }) + # A None config stands for a file that declares no rules at all. + lines = yaml.safe_dump(config).splitlines() if config is not None else ['# no rate limiting rules'] + ts.Disk.File(f'{ts.Variables.CONFIGDIR}/rate_limit.yaml', typename='ats:config').AddLines(lines) + ts.Disk.plugin_config.AddLine(f'rate_limit.so {ts.Variables.CONFIGDIR}/rate_limit.yaml') + tr = Test.AddTestRun(f'{name}: rate_limit YAML configuration') + tr.Processes.Default.Command = 'echo configuration checked' + tr.Processes.Default.ReturnCode = 0 + if error: + ts.ReturnCode = 70 # EX_SOFTWARE from TSFatal. + ts.Ready = 0 + ts.Disk.diags_log.Content = Testers.ContainsExpression(error, 'Report the invalid configuration') + ts.Disk.diags_log.Content += Testers.ExcludesExpression( + 'Traffic Server is fully initialized', 'Invalid configuration prevents startup') + watcher = Test.Processes.Process(f'{name}-watcher') + watcher.Command = 'sleep 10' + watcher.Ready = When.FileContains(ts.Disk.diags_log.Name, 'Failed to parse YAML file') + watcher.StartBefore(ts) + tr.TimeOut = 5 + tr.Processes.Default.StartBefore(watcher) + else: + ts.Disk.traffic_out.Content += Testers.ContainsExpression('Successfully loaded YAML file', 'Accept all supported keys') + tr.Processes.Default.StartBefore(ts) + + +TestYamlKeys()