diff --git a/CMakePresets.json b/CMakePresets.json index 68bbb2d4e4a..fd504ff15f4 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -267,6 +267,8 @@ "description": "CI Pipeline config for running clang-analyzer", "inherits": ["ci"], "cacheVariables": { + "CMAKE_C_COMPILER": "clang", + "CMAKE_CXX_COMPILER": "clang++", "CMAKE_EXPORT_COMPILE_COMMANDS": "ON", "ENABLE_CCACHE": "OFF", "ENABLE_EXAMPLE": "OFF", diff --git a/doc/appendices/command-line/traffic_ctl.en.rst b/doc/appendices/command-line/traffic_ctl.en.rst index 762dfdfaad5..64276e05c6a 100644 --- a/doc/appendices/command-line/traffic_ctl.en.rst +++ b/doc/appendices/command-line/traffic_ctl.en.rst @@ -416,6 +416,77 @@ Display the current value of a configuration record. will return an error for the corresponding key. The JSONRPC response will contain per-key error details. + .. option:: --directive, -D + + Pass a reload directive to a specific config handler. Directives are operational parameters + that modify how the handler performs the reload — for example, scoping a reload to a single + entry or enabling a dry-run mode. They are distinct from config content (``-d``). + + The format is ``config_key.directive_key=value``, parsed by splitting on the first ``.`` + and the first ``=``: + + - ``config_key`` — the registry key (e.g. ``ip_allow``, ``sni``) + - ``directive_key`` — the directive name understood by that handler + - ``value`` — the directive value (always passed as a string on the wire) + + Multiple directives are passed as space-separated values after a single ``-D``, or by + repeating the option. Both spellings accumulate, and they may be mixed: + + .. code-block:: bash + + # Single directive + $ traffic_ctl config reload -D myconfig.id=foo + + # Multiple directives for the same handler + $ traffic_ctl config reload -D myconfig.id=foo myconfig.dry_run=true + + # Directives for different handlers in the same reload + $ traffic_ctl config reload -D myconfig.id=foo sni.fqdn=example.com + + # The same, written as a repeated option + $ traffic_ctl config reload -D myconfig.id=foo -D sni.fqdn=example.com + + # Repeating it is how a directive is written after another option + $ traffic_ctl config reload -D myconfig.id=foo --monitor -D sni.fqdn=example.com + + On the wire, ``-D myconfig.id=foo`` translates to: + + .. code-block:: json + + { "configs": { "myconfig": { "_reload": { "id": "foo" } } } } + + For complex or nested directive values, use ``-d`` with full YAML instead: + + .. code-block:: bash + + $ traffic_ctl config reload -d 'myconfig: { _reload: { id: foo, options: { strict: true } } }' + + .. note:: + + ``-D`` accepts values until the next option or the end of the command line, so it + may appear anywhere among the options and can be combined with ``-d`` — directives + and inline content merge under the same config key: + + .. code-block:: bash + + $ traffic_ctl config reload -D myconfig.id=foo --monitor + $ traffic_ctl config reload -D myconfig.id=foo -d 'myconfig: {rules: [a]}' + + To pass a directive value that begins with ``-``, place ``--`` before it. Option + recognition then stays off for the rest of the line, so every remaining token becomes + a directive value and any option written afterwards is swallowed. Use + ``--directive=-value`` instead when options still have to follow: + + .. code-block:: bash + + $ traffic_ctl config reload --directive=-weird.id=foo --monitor + + .. note:: + + Available directives depend on the handler — consult each config's documentation for + supported directive keys. Directive values are strings on the wire; handlers use + yaml-cpp's ``as()`` to interpret them as needed. + .. option:: --force, -F Force a new reload even if one is already in progress. Without this flag, the server rejects @@ -511,6 +582,42 @@ Display the current value of a configuration record. Specifying the file name is not needed as `traffic_ctl` will try to use the build(or the runroot if used) information to figure out the path to the `records.yaml`. + ``-c`` accepts at most one file name, so it may be written before or after the record + names: + + .. code-block:: bash + + $ traffic_ctl config get -c records.yaml proxy.config.diags.debug.enabled + $ traffic_ctl config get proxy.config.diags.debug.enabled -c records.yaml + $ traffic_ctl config get --cold=records.yaml proxy.config.diags.debug.enabled + + When no file name is given, write ``-c`` last, or use the ``--cold=`` form for the + explicit file. A bare ``-c`` followed by a record name takes the record as the file name, + which leaves the command short of its own arguments. Each command reports this in terms of + what it was left without: + + .. code-block:: bash + + $ traffic_ctl config get proxy.config.diags.debug.enabled -c # default records.yaml + $ traffic_ctl config get -c proxy.config.diags.debug.enabled + Error: at least one argument expected by get + + $ traffic_ctl config set proxy.config.diags.debug.enabled 1 -c # default records.yaml + $ traffic_ctl config set -c proxy.config.diags.debug.enabled 1 + Error: 2 argument(s) expected by set + + An empty file name is not a file name, so it is reported rather than taken as a request for + the default file. This matters when the name comes from a variable that is unset, where + reading or writing the live :file:`records.yaml` is unlikely to be what was meant: + + .. code-block:: bash + + $ traffic_ctl config set -c "" proxy.config.diags.debug.enabled 1 + Error: missing argument for '-c' + + ``-c`` is also given at most once, so repeating it is a usage error rather than the last + file name silently winning. + If the file exists and is empty a new document will be created. If a file does not exist, an attempt to create a new file will be done. This option(only for the config file changes) lets you use the prefix `proxy.config.` or `ts.` for variable names, either would work. diff --git a/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst b/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst index f081acda953..66a12078d8a 100644 --- a/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst +++ b/doc/developer-guide/api/functions/TSSslClientCertUpdate.en.rst @@ -35,6 +35,11 @@ Description =========== :func:`TSSslClientCertUpdate` updates existing client certificates configured in :file:`sni.yaml` or -`proxy.config.ssl.client.cert.filename`. :arg:`cert_path` should be exact match as provided in -configurations. :func:`TSSslClientCertUpdate` returns :enumerator:`TS_SUCCESS` only if :arg:`cert_path` exists -in configuration and reloaded to update the context. +`proxy.config.ssl.client.cert.filename`. :arg:`cert_path` must match the resolved certificate path used by +Traffic Server. Relative certificate names in the configuration are resolved against +`proxy.config.ssl.client.cert.path`. :func:`TSSslClientCertUpdate` returns :enumerator:`TS_SUCCESS` only if +:arg:`cert_path` exists in the configuration and is reloaded into every matching context. + +Any certificate data cached for :arg:`cert_path` and :arg:`key_path` is discarded as well, so client +contexts that Traffic Server creates after the update also use the new certificate rather than the +previously cached one. diff --git a/doc/developer-guide/api/functions/TSSslClientContext.en.rst b/doc/developer-guide/api/functions/TSSslClientContext.en.rst index 9f685b2d486..e427734e0ca 100644 --- a/doc/developer-guide/api/functions/TSSslClientContext.en.rst +++ b/doc/developer-guide/api/functions/TSSslClientContext.en.rst @@ -37,8 +37,8 @@ Description These functions are used to explore the client contexts that |TS| uses to connect to upstreams. :func:`TSSslClientContextsNamesGet` can be used to retrieve the entire client context mappings. Note -that in |TS|, client contexts are stored in a 2-level mapping with ca paths and cert/key -paths as keys. Hence every 2 null-terminated string in :arg:`result` can be used to lookup one context. +that in |TS|, client contexts are stored in a 2-level mapping with CA paths and the resolved certificate +path as keys. Hence every 2 null-terminated string in :arg:`result` can be used to lookup one context. :arg:`result` points to an user allocated array that will hold pointers to lookup key strings and :arg:`n` is the size for :arg:`result` array. :arg:`actual`, if valid, will be filled with actual number of lookup keys (2 for each context). diff --git a/doc/developer-guide/config-reload-framework.en.rst b/doc/developer-guide/config-reload-framework.en.rst index 3eceee058cf..c91f5dda339 100644 --- a/doc/developer-guide/config-reload-framework.en.rst +++ b/doc/developer-guide/config-reload-framework.en.rst @@ -267,8 +267,90 @@ supplied_yaml() Returns the YAML node supplied via the RPC ``-d`` flag or ``configs`` parameter. If no inline content was provided, the returned node is undefined (``operator bool()`` returns ``false``). + The framework strips the reserved ``_reload`` key from the supplied YAML before delivering it + to the handler, so ``supplied_yaml()`` always contains pure config data. + +reload_directives() + Returns the YAML map extracted from the ``_reload`` key in the RPC-supplied content. If no + directives were provided, the returned node is Undefined (``operator bool()`` returns ``false``). + + Directives are operational parameters that modify **how** the handler performs the reload — + they are distinct from config **content**. Common uses include scoping a reload to a single + entry, enabling a dry-run mode, or passing a version constraint. + + On the wire, directives are nested under ``_reload`` inside the handler's ``configs`` node: + + .. code-block:: json + + { + "configs": { + "myconfig": { + "_reload": { "id": "foo", "dry_run": "true" }, + "rules": ["rule1", "rule2"] + } + } + } + + The framework extracts ``_reload`` before the handler runs, so: + + - ``reload_directives()`` returns ``{ "id": "foo", "dry_run": "true" }`` + - ``supplied_yaml()`` returns the remaining content (without ``_reload``) + - If ``_reload`` was the only key, ``supplied_yaml()`` is undefined + + Directives and content can coexist. The handler decides how to combine them — the framework + delivers both without interpretation. + + **Recommended handler pattern:** + + .. code-block:: cpp + + void MyConfig::reconfigure(ConfigContext ctx) { + ctx.in_progress(); + + if (auto directives = ctx.reload_directives()) { + if (auto id_node = directives["id"]; id_node.IsDefined()) { + std::string id = id_node.as(); + if (!reload_single_entry(id)) { + ctx.fail("Unknown entry: " + id); + return; + } + ctx.complete("Reloaded entry: " + id); + return; + } + } + + if (auto yaml = ctx.supplied_yaml()) { + if (!load_from_yaml(yaml)) { + ctx.fail("Invalid inline content"); + return; + } + ctx.complete("Loaded from inline content"); + return; + } + + if (!load_from_file(config_filename)) { + ctx.fail("Failed to parse " + config_filename); + return; + } + ctx.complete("Loaded from file"); + } + + From :program:`traffic_ctl`, directives are passed via ``--directive`` (``-D``): + + .. code-block:: bash + + $ traffic_ctl config reload -D myconfig.id=foo + + See the ``--directive`` option in :ref:`traffic_ctl ` for details. + + .. note:: + + Directive values are strings on the wire (the JSONRPC transport serializes all values as + double-quoted strings). Handlers use yaml-cpp's ``as()`` to interpret them as needed. + add_dependent_ctx(description) Create a child sub-task. The parent aggregates status from all its children. + Child contexts inherit both ``supplied_yaml()`` and ``reload_directives()`` from the parent. All methods support ``swoc::bwprint`` format strings: diff --git a/doc/developer-guide/internal-libraries/ArgParser.en.rst b/doc/developer-guide/internal-libraries/ArgParser.en.rst index c15a552ff5a..cdaf5cbe4f9 100644 --- a/doc/developer-guide/internal-libraries/ArgParser.en.rst +++ b/doc/developer-guide/internal-libraries/ArgParser.en.rst @@ -104,6 +104,48 @@ To add options to the parser or current command: This function call returns the new :class:`Option` instance. (0 is also number of arguments expected) +.. Note:: + + For options, the number of arguments may also be one of the following, which mirror the + ``nargs`` values of Python's ``argparse``: + + ================================ ======================================================= + Value Meaning + ================================ ======================================================= + ``AT_MOST_ONE_ARG_N`` Zero or one value (``argparse`` ``nargs='?'``) + ``MORE_THAN_ZERO_ARG_N`` Zero or more values (``argparse`` ``nargs='*'``) + ``MORE_THAN_ONE_ARG_N`` One or more values (``argparse`` ``nargs='+'``) + ================================ ======================================================= + + An option taking a variable number of values stops collecting when it reaches a token + naming another option of the same command, so options written afterwards keep their own + arguments. Use ``AT_MOST_ONE_ARG_N`` rather than ``MORE_THAN_ZERO_ARG_N`` for an option + whose value is optional, otherwise it also consumes the positional arguments of its + command. + + A token naming another option is not a value for a fixed number of arguments either. An + option written where a value is expected leaves the value missing, which is reported as a + usage error rather than the option being consumed and applied as the value. + + Because collection stops at the following option, an option taking an unbounded number of + values may be written more than once, and the occurrences accumulate. This matches the + ``--option=value`` form, which has always appended. An option taking a fixed number of + values keeps its last-one-wins behaviour instead, and ``AT_MOST_ONE_ARG_N`` reports a + repetition as a usage error since it permits only one value in total. + + An empty token is not a value for ``AT_MOST_ONE_ARG_N``. It is reported as a missing + argument rather than read as the option having been given without one, so a value taken + from an unset variable cannot silently select the declared default. + + A ``--`` token stops option recognition for the values being collected, which is how a + value beginning with ``-`` is passed. Note this differs from the POSIX ``--``: it does + not end the value list nor force the remainder to be positional arguments. + + Option recognition stays off for the rest of that collection, so for a variable number of + values every remaining token becomes a value and no later option is recognized. Use the + ``--option=value`` form instead when options still have to follow a value that begins with + ``-``. + We can also use the following chained way to add subcommand or option: .. code-block:: cpp diff --git a/include/mgmt/config/ConfigContext.h b/include/mgmt/config/ConfigContext.h index 30ebe886eb9..e788598872f 100644 --- a/include/mgmt/config/ConfigContext.h +++ b/include/mgmt/config/ConfigContext.h @@ -173,19 +173,35 @@ class ConfigContext [[nodiscard]] ConfigContext add_dependent_ctx(std::string_view description = "", std::string_view filename = ""); /// Get supplied YAML node (for RPC-based reloads). - /// A default-constructed YAML::Node is Undefined (operator bool() == false). + /// Returns Undefined when no content was provided (operator bool() == false). /// @code /// if (auto yaml = ctx.supplied_yaml()) { /* use yaml node */ } /// @endcode /// @return copy of the supplied YAML node (cheap — YAML::Node is internally reference-counted). [[nodiscard]] YAML::Node supplied_yaml() const; + /// Get reload directives extracted from the _reload key. + /// Directives are operational parameters that modify how the handler performs + /// the reload (e.g. scope to a single entry, dry-run) — distinct from config content. + /// The framework extracts _reload from the supplied node before passing content + /// to the handler, so supplied_yaml() never contains _reload. + /// Returns Undefined when no directives were provided (operator bool() == false). + /// @code + /// if (auto directives = ctx.reload_directives()) { /* use directives */ } + /// @endcode + /// @return copy of the directives YAML node (cheap — YAML::Node is internally reference-counted). + [[nodiscard]] YAML::Node reload_directives() const; + private: /// Set supplied YAML node. Only ConfigRegistry should call this during reload setup. void set_supplied_yaml(YAML::Node node); + /// Set reload directives. Only ConfigRegistry should call this during reload setup. + void set_reload_directives(YAML::Node node); + std::weak_ptr _task; - YAML::Node _supplied_yaml; ///< for no content, this will just be empty + YAML::Node _supplied_yaml{YAML::NodeType::Undefined}; + YAML::Node _reload_directives{YAML::NodeType::Undefined}; friend class ReloadCoordinator; friend class config::ConfigRegistry; diff --git a/include/mgmt/rpc/handlers/server/Server.h b/include/mgmt/rpc/handlers/server/Server.h index fec31c66bfe..40e9b2ca059 100644 --- a/include/mgmt/rpc/handlers/server/Server.h +++ b/include/mgmt/rpc/handlers/server/Server.h @@ -26,7 +26,6 @@ namespace rpc::handlers::server { swoc::Rv server_start_drain(std::string_view const &id, YAML::Node const ¶ms); swoc::Rv server_stop_drain(std::string_view const &id, YAML::Node const &); -void server_shutdown(YAML::Node const &); swoc::Rv get_server_status(std::string_view const &id, YAML::Node const &); swoc::Rv get_connection_tracker_info(std::string_view const &id, YAML::Node const ¶ms); diff --git a/include/tscore/ArgParser.h b/include/tscore/ArgParser.h index fdb6086eba0..1b29504a83f 100644 --- a/include/tscore/ArgParser.h +++ b/include/tscore/ArgParser.h @@ -34,10 +34,23 @@ constexpr unsigned MORE_THAN_ZERO_ARG_N = ~0; // more than one arguments constexpr unsigned MORE_THAN_ONE_ARG_N = ~0 - 1; +// zero or one argument +constexpr unsigned AT_MOST_ONE_ARG_N = ~0 - 2; // customizable indent for help message constexpr int INDENT_ONE = 32; constexpr int INDENT_TWO = 46; +/** Whether @a arg_num asks for a variable rather than a fixed number of values. + + Use this in preference to comparing against the sentinels, so that adding another + variable arity does not silently leave a sentinel being treated as a literal count. + */ +constexpr bool +is_variable_arg_num(unsigned arg_num) +{ + return arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N || arg_num == AT_MOST_ONE_ARG_N; +} + namespace ts { using AP_StrVec = std::vector; @@ -89,6 +102,12 @@ class Arguments ~Arguments(); ArgumentData get(std::string const &name); + /** Whether @a name has an entry. + + @return @c true when the command or option has been parsed. Unlike get(), the called + flag is left alone, so this can be asked while parsing. + */ + bool has(std::string const &name) const noexcept; void append(std::string const &key, ArgumentData const &value); // Append value to the arg to the map of key @@ -222,6 +241,10 @@ class ArgParser void version_message() const; // Helper method for parse() void append_option_data(Arguments &ret, AP_StrVec &args, int index); + // Helper method to collect the values of an option or command into @a ret + std::string handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) const; + // Whether @a token names an option registered on this command + bool is_registered_option(std::string const &token) const; // Helper method to validate mutually exclusive groups void validate_mutex_groups(Arguments &ret) const; // Helper method to validate option dependencies diff --git a/include/tsutil/Metrics.h b/include/tsutil/Metrics.h index 40cdef9522f..741b1e069a5 100644 --- a/include/tsutil/Metrics.h +++ b/include/tsutil/Metrics.h @@ -30,13 +30,12 @@ #include #include #include +#include #include #include #include #include -#include "swoc/MemSpan.h" - #include "tsutil/Assert.h" namespace ts @@ -85,8 +84,7 @@ class Metrics enum class MetricType : int { COUNTER = 0, GAUGE }; - using IdType = int32_t; // Could be a tuple, but one way or another, they have to be combined to an int32_t. - using SpanType = swoc::MemSpan; + using IdType = int32_t; // Could be a tuple, but one way or another, they have to be combined to an int32_t. static constexpr uint16_t MAX_BLOBS = 8192; static constexpr uint16_t MAX_SIZE = 1024; // For a total of 8M metrics @@ -147,12 +145,6 @@ class Metrics { return _storage->lookup(id, out_name, type); } - bool - rename(IdType id, const std::string_view name) - { - return _storage->rename(id, name); - } - AtomicType & operator[](IdType id) { @@ -267,9 +259,7 @@ class Metrics iterator end() const { - auto [blob, offset] = _storage->current(); - - return iterator(*this, _makeId(blob, offset, MetricType::COUNTER)); + return iterator(*this, _storage->next_free_id()); } iterator @@ -292,12 +282,6 @@ class Metrics return _storage->create(name, type); } - SpanType - _createSpan(size_t size, MetricType type, IdType *id = nullptr) - { - return _storage->createSpan(size, type, id); - } - // These are little helpers around managing the ID's static constexpr std::tuple _splitID(IdType value) @@ -308,7 +292,7 @@ class Metrics static constexpr MetricType _extractType(IdType value) { - return MetricType{value >> METRIC_TYPE_BITS}; + return MetricType{static_cast((static_cast(value) >> METRIC_TYPE_BITS) & 0x1)}; } static constexpr IdType @@ -318,13 +302,31 @@ class Metrics return (t << METRIC_TYPE_BITS | blob << 16 | offset); } + /// As @c _makeId, without the type bits. + static constexpr uint32_t + _pack(uint16_t blob, uint16_t offset) + { + return static_cast(blob) << 16 | offset; + } + + // A packed position must not reach the type bits, and an offset must fit its field. + static_assert(MAX_SIZE <= 0x10000); + static_assert(MAX_BLOBS <= (1 << (METRIC_TYPE_BITS - 16))); + class Storage { - BlobStorage _blobs; - uint16_t _cur_blob = 0; - uint16_t _cur_off = 0; - LookupTable _lookups; - mutable std::mutex _mutex; + /* The next free slot, packed as @c _makeId packs one. A single value because a reader that + * caught a new offset against an old blob index, or the reverse, would reject ids that exist + * or accept ids that do not. Release stored last, after the blob pointer or the slot's name it + * publishes. Only ever increases, so an id is allocated exactly when it packs below it. + * + * A slot's name is written once, before the store that publishes it, and never changes, which + * is what lets @c name and @c lookup hand out a view of it without the mutex. + */ + BlobStorage _blobs; + std::atomic _next_free{0}; + LookupTable _lookups; + mutable std::mutex _mutex; public: Storage(const Storage &) = delete; @@ -347,22 +349,43 @@ class Metrics AtomicType *lookup(Metrics::IdType id, std::string_view *out_name = nullptr, MetricType *out_type = nullptr) const; std::string_view name(IdType id) const; MetricType type(IdType id) const; - SpanType createSpan(size_t size, const MetricType type = MetricType::COUNTER, IdType *id = nullptr); - bool rename(IdType id, const std::string_view name); - std::pair - current() const + /// The id the next slot will get, which is also iteration's exclusive bound. + IdType + next_free_id() const { - std::lock_guard lock(_mutex); - return {_cur_blob, _cur_off}; + return static_cast(_next_free.load(std::memory_order_acquire)); } bool valid(IdType id) const { - auto [blob, entry] = _splitID(id); + return _is_allocated(id); + } + + private: + /** Whether @a id names an allocated slot. + * + * The gate for every id based accessor, since ids from the @c TSStat* API are untrusted. An id + * qualifies when it is non-negative, its offset is one @c _makeId could produce, and its slot + * has been handed out. + */ + bool + _is_allocated(IdType id) const + { + if (id < 0) { + return false; + } + + auto [blob_ix, offset] = _splitID(id); - return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob == _cur_blob && entry <= _cur_off))); + // Not implied below: an earlier blob can name an offset past MAX_SIZE and still pack under. + if (offset >= MAX_SIZE) { + return false; + } + + // Acquiring the bound acquires the blob install, so _blobs needs no check of its own. + return _pack(blob_ix, offset) < _next_free.load(std::memory_order_acquire); } }; @@ -376,7 +399,6 @@ class Metrics { public: using self_type = Gauge; - using SpanType = Metrics::SpanType; class AtomicType : public Metrics::AtomicType { @@ -452,14 +474,6 @@ class Metrics return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::GAUGE))); } - static Metrics::Gauge::SpanType - createSpan(size_t size, IdType *id = nullptr) - { - auto &instance = Metrics::instance(); - - return instance._createSpan(size, MetricType::GAUGE, id); - } - static void increment(AtomicType *metric, uint64_t val = 1) { @@ -494,7 +508,6 @@ class Metrics { public: using self_type = Counter; - using SpanType = Metrics::SpanType; class AtomicType : public Metrics::AtomicType { @@ -570,14 +583,6 @@ class Metrics return reinterpret_cast(instance.lookup(instance._create(tmpname, MetricType::COUNTER))); } - static Metrics::Counter::SpanType - createSpan(size_t size, IdType *id = nullptr) - { - auto &instance = Metrics::instance(); - - return instance._createSpan(size, MetricType::COUNTER, id); - } - static void increment(AtomicType *metric, uint64_t val = 1) { diff --git a/plugins/esi/lib/IncludeUrlValidator.cc b/plugins/esi/lib/IncludeUrlValidator.cc index 5bb9c0c51fe..33834b06fc4 100644 --- a/plugins/esi/lib/IncludeUrlValidator.cc +++ b/plugins/esi/lib/IncludeUrlValidator.cc @@ -39,7 +39,6 @@ namespace // Backtracking limit for the allowlist match. PCRE2 stops and reports // PCRE2_ERROR_MATCHLIMIT once this many match steps are taken, bounding // worst-case CPU per validation against attacker-influenced hostnames. - // Matches the value used by the regex_remap plugin. constexpr uint32_t ALLOW_REGEX_MATCH_LIMIT = 1750; bool diff --git a/plugins/healthchecks/healthchecks.cc b/plugins/healthchecks/healthchecks.cc index c5da85da9da..ece4319aee2 100644 --- a/plugins/healthchecks/healthchecks.cc +++ b/plugins/healthchecks/healthchecks.cc @@ -140,9 +140,9 @@ load_status_file(HCFileInfo *info) if (nullptr != (fd = fopen(info->fname, "r"))) { data->exists = 1; - size_t bytes_read; - while ((bytes_read = fread(data->body, 1, MAX_BODY_LEN, fd)) > 0) { - data->b_len = static_cast(bytes_read); + data->b_len = static_cast(fread(data->body, 1, MAX_BODY_LEN, fd)); + if (ferror(fd)) { + data->b_len = 0; } fclose(fd); } @@ -333,68 +333,66 @@ parse_configs(const char *fname) return nullptr; } - while (!feof(fd)) { + while (fgets(buf, sizeof(buf) - 1, fd) != nullptr) { char *str, *save; char *ok = nullptr, *miss = nullptr, *mime = nullptr; - if (fgets(buf, sizeof(buf) - 1, fd)) { - finfo = new HCFileInfo(); - - str = strtok_r(buf, SEPARATORS, &save); - int state = 0; - while (nullptr != str) { - if (strlen(str) > 0) { - switch (state) { - case 0: - if ('/' == *str) { - ++str; - } - strncpy(finfo->path, str, PATH_NAME_MAX - 1); - finfo->path[PATH_NAME_MAX - 1] = '\0'; - finfo->p_len = strlen(finfo->path); - break; - case 1: - strncpy(finfo->fname, str, MAX_PATH_LEN - 1); - finfo->fname[MAX_PATH_LEN - 1] = '\0'; - finfo->basename = strrchr(finfo->fname, '/'); - if (finfo->basename) { - ++(finfo->basename); - finfo->basename_len = strlen(finfo->basename); - } - break; - case 2: - mime = str; - break; - case 3: - ok = str; - break; - case 4: - miss = str; - break; + finfo = new HCFileInfo(); + + str = strtok_r(buf, SEPARATORS, &save); + int state = 0; + while (nullptr != str) { + if (strlen(str) > 0) { + switch (state) { + case 0: + if ('/' == *str) { + ++str; + } + strncpy(finfo->path, str, PATH_NAME_MAX - 1); + finfo->path[PATH_NAME_MAX - 1] = '\0'; + finfo->p_len = strlen(finfo->path); + break; + case 1: + strncpy(finfo->fname, str, MAX_PATH_LEN - 1); + finfo->fname[MAX_PATH_LEN - 1] = '\0'; + finfo->basename = strrchr(finfo->fname, '/'); + if (finfo->basename) { + ++(finfo->basename); + finfo->basename_len = strlen(finfo->basename); } - ++state; + break; + case 2: + mime = str; + break; + case 3: + ok = str; + break; + case 4: + miss = str; + break; } - str = strtok_r(nullptr, SEPARATORS, &save); + ++state; } + str = strtok_r(nullptr, SEPARATORS, &save); + } - /* Fill in the info if everything was ok */ - if (state > 4) { - Dbg(dbg_ctl, "Parsed: %s %s %s %s %s", finfo->path, finfo->fname, mime, ok, miss); - finfo->ok = gen_header(ok, mime, &finfo->o_len); - finfo->miss = gen_header(miss, mime, &finfo->m_len); - finfo->set_data(load_status_file(finfo)); - - /* Add it the linked list */ - Dbg(dbg_ctl, "Adding path=%s to linked list", finfo->path); - if (nullptr == head_finfo) { - head_finfo = finfo; - } else { - prev_finfo->_next = finfo; - } - prev_finfo = finfo; + /* Fill in the info if everything was ok */ + if (state > 4) { + Dbg(dbg_ctl, "Parsed: %s %s %s %s %s", finfo->path, finfo->fname, mime, ok, miss); + finfo->ok = gen_header(ok, mime, &finfo->o_len); + finfo->miss = gen_header(miss, mime, &finfo->m_len); + finfo->set_data(load_status_file(finfo)); + + /* Add it the linked list */ + Dbg(dbg_ctl, "Adding path=%s to linked list", finfo->path); + if (nullptr == head_finfo) { + head_finfo = finfo; } else { - delete finfo; + prev_finfo->_next = finfo; } + prev_finfo = finfo; + } else { + delete finfo; } } fclose(fd); diff --git a/plugins/regex_remap/regex_remap.cc b/plugins/regex_remap/regex_remap.cc index 86b2b2d2ba7..6b2d20d1636 100644 --- a/plugins/regex_remap/regex_remap.cc +++ b/plugins/regex_remap/regex_remap.cc @@ -55,9 +55,8 @@ static const char *PLUGIN_NAME = "regex_remap"; // Constants -static const int MATCHCOUNT = 15; // We support $0 - $9 x2 ints, and this needs to be 1.5x that -static const int MAX_SUBS = 32; // No more than 32 substitution variables in the subst string -static const int32_t REGEX_MATCH_LIMIT = 1750; // POOMA - also dependent on actual stack size. Crashes with previous value of 2047 +static const int MATCHCOUNT = 15; // We support $0 - $9 x2 ints, and this needs to be 1.5x that +static const int MAX_SUBS = 32; // No more than 32 substitution variables in the subst string // Substitutions other than regex matches enum ExtraSubstitutions { @@ -917,7 +916,6 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE if (!ri->rule_set) { return TS_ERROR; } - ri->match_context.set_match_limit(REGEX_MATCH_LIMIT); if (ri->profile) { ri->rule_hits.resize(ri->rule_set->rules().size()); } diff --git a/src/api/InkAPI.cc b/src/api/InkAPI.cc index 158c06fa72b..5914d96313a 100644 --- a/src/api/InkAPI.cc +++ b/src/api/InkAPI.cc @@ -28,6 +28,7 @@ #include #include #include +#include #include "iocore/net/NetVConnection.h" #include "iocore/net/NetHandler.h" @@ -8240,59 +8241,106 @@ TSSslClientCertUpdate(const char *cert_path, const char *key_path) return TS_ERROR; } - std::string key; - shared_SSL_CTX client_ctx = nullptr; - SSLConfigParams *params = SSLConfig::acquire(); + // --- Pin the active SSL configuration --- + // + // Keep this configuration generation alive across every early return and + // release it automatically when the update finishes. + std::string key{cert_path}; + SSLConfig::scoped_config params; - // Generate second level key for client context lookup - swoc::bwprint(key, "{}:{}", cert_path, key_path); + // The client context map is keyed by the resolved certificate path. Dbg(dbg_ctl_ssl_cert_update, "TSSslClientCertUpdate(): Use %.*s as key for lookup", static_cast(key.size()), key.data()); - if (nullptr != params) { - // Try to update client contexts maps - auto &ca_paths_map = params->top_level_ctx_map; - auto &map_lock = params->ctxMapLock; - std::string ca_paths_key; - // First try to locate the client context and its CA path (by top level) - ink_mutex_acquire(&map_lock); - for (auto &ca_paths_pair : ca_paths_map) { - auto &ctx_map = ca_paths_pair.second; - auto iter = ctx_map.find(key); - if (iter != ctx_map.end() && iter->second != nullptr) { - ca_paths_key = ca_paths_pair.first; - break; - } + if (!params) { + return TS_ERROR; + } + + auto &ca_paths_map = params->top_level_ctx_map; + auto &map_lock = params->ctxMapLock; + std::vector ca_paths_keys; + + // --- Find every matching CA bucket --- + // + // A certificate can be used with more than one CA configuration. Snapshot + // all matching bucket keys while holding the map lock, then release it + // before performing the expensive context construction. + ink_mutex_acquire(&map_lock); + for (auto const &[ca_paths_key, ctx_map] : ca_paths_map) { + if (ctx_map.contains(key)) { + ca_paths_keys.push_back(ca_paths_key); } - ink_mutex_release(&map_lock); + } + ink_mutex_release(&map_lock); + + if (ca_paths_keys.empty()) { + return TS_ERROR; + } + + // --- Drop the cached certificate data --- + // + // getCTX() builds contexts from the cached secret data rather than from the + // files. Drop the cached copies so that a context built later for a CA + // bucket that does not exist yet also picks up the updated certificate + // instead of the pre-update PEM. + params->secrets.invalidateSecret(key); + if (key_path != nullptr && key_path[0] != '\0') { + params->secrets.invalidateSecret(key_path); + } - // Only update on existing - if (ca_paths_key.empty()) { + std::vector> client_contexts; + + // --- Build every replacement context --- + // + // Build all replacements before changing the live map. If any construction + // fails, the existing working contexts remain installed. + client_contexts.reserve(ca_paths_keys.size()); + for (auto const &ca_paths_key : ca_paths_keys) { + size_t sep = ca_paths_key.find(':'); + std::string ca_bundle_file = ca_paths_key.substr(0, sep); + std::string ca_bundle_path = ca_paths_key.substr(sep + 1); + shared_SSL_CTX client_ctx(SSLCreateClientContext(params, ca_bundle_file.empty() ? nullptr : ca_bundle_file.c_str(), + ca_bundle_path.empty() ? nullptr : ca_bundle_path.c_str(), cert_path, + key_path), + SSL_CTX_free); + + if (!client_ctx) { return TS_ERROR; } + client_contexts.emplace_back(ca_paths_key, std::move(client_ctx)); + } - // Extract CA related paths - size_t sep = ca_paths_key.find(':'); - std::string ca_bundle_file = ca_paths_key.substr(0, sep); - std::string ca_bundle_path = ca_paths_key.substr(sep + 1); - - // Build new client context - client_ctx = - shared_SSL_CTX(SSLCreateClientContext(params, ca_bundle_path.empty() ? nullptr : ca_bundle_path.c_str(), - ca_bundle_file.empty() ? nullptr : ca_bundle_file.c_str(), cert_path, key_path), - SSL_CTX_free); - - // Successfully generates a client context, update in the map - ink_mutex_acquire(&map_lock); - auto iter = ca_paths_map.find(ca_paths_key); - if (iter != ca_paths_map.end() && iter->second.count(key)) { - iter->second[key] = client_ctx; - } else { - client_ctx = nullptr; + std::vector targets; + + // --- Install all replacement contexts --- + // + // Reacquire the map lock and locate every target before overwriting any of + // them, so that the live map is either updated completely or left untouched. + targets.reserve(client_contexts.size()); + ink_mutex_acquire(&map_lock); + for (auto const &client_context : client_contexts) { + auto ca_iter = ca_paths_map.find(client_context.first); + + if (ca_iter == ca_paths_map.end()) { + break; + } + auto ctx_iter = ca_iter->second.find(key); + + if (ctx_iter == ca_iter->second.end()) { + break; + } + targets.push_back(&ctx_iter->second); + } + + bool const updated_all = targets.size() == client_contexts.size(); + + if (updated_all) { + for (std::size_t i = 0; i < targets.size(); ++i) { + *targets[i] = std::move(client_contexts[i].second); } - ink_mutex_release(&map_lock); } + ink_mutex_release(&map_lock); - return client_ctx ? TS_SUCCESS : TS_ERROR; + return updated_all ? TS_SUCCESS : TS_ERROR; } TSReturnCode diff --git a/src/iocore/dns/DNS.cc b/src/iocore/dns/DNS.cc index 4c6cecdcca3..16e33e95f1f 100644 --- a/src/iocore/dns/DNS.cc +++ b/src/iocore/dns/DNS.cc @@ -1889,8 +1889,9 @@ dns_process(DNSHandler *handler, HostEnt *buf, int len) memcpy((*hap++ = bp), cp, n); Dbg(dbg_ctl_dns, "received %s = %s", QtypeName(type), inet_ntop(T_AAAA == type ? AF_INET6 : AF_INET, bp, ip_string, sizeof(ip_string))); - bp += n; - cp += n; + bp += n; + cp += n; + buflen = sizeof(buf->hostbuf) - (bp - buf->hostbuf); } } else { goto Lerror; diff --git a/src/iocore/hostdb/HostDB.cc b/src/iocore/hostdb/HostDB.cc index 6abda699462..6011d24b6b0 100644 --- a/src/iocore/hostdb/HostDB.cc +++ b/src/iocore/hostdb/HostDB.cc @@ -953,6 +953,10 @@ HostDBContinuation::dnsEvent(int event, HostEnt *e) ts::LocalBuffer q_buf(valid_records); SRV **q = q_buf.data(); ink_assert(valid_records <= static_cast(hostdb_round_robin_max_count)); + // The loop below assigns every element, but ts::LocalBuffer hands back raw storage and the + // static analyzer cannot follow the loop well enough to see that. Pre-fill so the sort below + // is never reported as reading an uninitialized pointer. + std::fill_n(q, valid_records, nullptr); for (int i = 0; i < valid_records; ++i) { q[i] = &e->srv_hosts.hosts[i]; } diff --git a/src/iocore/net/OCSPStapling.cc b/src/iocore/net/OCSPStapling.cc index 715e68d2f90..d4b2e7cf98e 100644 --- a/src/iocore/net/OCSPStapling.cc +++ b/src/iocore/net/OCSPStapling.cc @@ -23,6 +23,7 @@ #include #include +#include #include #include @@ -898,19 +899,22 @@ ssl_stapling_init_cert(SSL_CTX *ctx, X509 *cert, const char *certname, const cha Dbg(dbg_ctl_ssl_ocsp, "using OCSP prefetched response file %s", rsp_file); FILE *fp = fopen(rsp_file, "r"); if (fp) { - fseek(fp, 0, SEEK_END); - long rsp_buf_len = ftell(fp); - if (rsp_buf_len >= 0) { - rewind(fp); - unsigned char *rsp_buf = static_cast(malloc(rsp_buf_len)); - auto read_len = fread(rsp_buf, 1, rsp_buf_len, fp); + long rsp_buf_len = -1; + + if (fseek(fp, 0, SEEK_END) == 0) { + rsp_buf_len = ftell(fp); + } + + if (rsp_buf_len > 0 && fseek(fp, 0, SEEK_SET) == 0) { + std::vector rsp_buf(rsp_buf_len); + auto read_len = fread(rsp_buf.data(), 1, rsp_buf.size(), fp); + if (read_len == static_cast(rsp_buf_len)) { - const unsigned char *p = rsp_buf; + const unsigned char *p = rsp_buf.data(); rsp = d2i_TS_OCSP_RESPONSE(nullptr, &p, rsp_buf_len); } else { Error("stapling_refresh_response: failed to read prefetched response file: %s", rsp_file); } - free(rsp_buf); } else { Error("stapling_refresh_response: failed to check the size of prefetched response file: %s", rsp_file); } diff --git a/src/iocore/net/OpenSSLQUICNetVConnection.cc b/src/iocore/net/OpenSSLQUICNetVConnection.cc index 9a06fb94d26..d71fefd4e64 100644 --- a/src/iocore/net/OpenSSLQUICNetVConnection.cc +++ b/src/iocore/net/OpenSSLQUICNetVConnection.cc @@ -389,7 +389,10 @@ QUICNetVConnection::acceptEvent(int event, Event *e) MUTEX_TRY_LOCK(lock, h->mutex, t); if (!lock.is_locked()) { - if (event == EVENT_NONE) { + // The event system always hands over a non-null Event, so @a e is only null if this is called + // directly, which pairs with EVENT_NONE. Reschedule on the thread in that case rather than + // dereferencing @a e. + if (event == EVENT_NONE || e == nullptr) { t->schedule_in(this, HRTIME_MSECONDS(net_retry_delay)); return EVENT_DONE; } else { diff --git a/src/iocore/net/P_SSLConfig.h b/src/iocore/net/P_SSLConfig.h index 48dc665d3e2..08dafff0d25 100644 --- a/src/iocore/net/P_SSLConfig.h +++ b/src/iocore/net/P_SSLConfig.h @@ -152,7 +152,7 @@ struct SSLConfigParams : public ConfigInfo { // Client contexts are held by 2-level map: // The first level maps from CA bundle file&path to next level map; - // The second level maps from cert&key to actual SSL_CTX; + // The second level maps from the resolved certificate path to the actual SSL_CTX; // The second level map owns the client SSL_CTX objects and is responsible for cleaning them up using CTX_MAP = std::unordered_map; mutable std::unordered_map top_level_ctx_map; diff --git a/src/iocore/net/P_SSLSecret.h b/src/iocore/net/P_SSLSecret.h index 292f99ca7a7..d6b28c7e912 100644 --- a/src/iocore/net/P_SSLSecret.h +++ b/src/iocore/net/P_SSLSecret.h @@ -34,6 +34,13 @@ class SSLSecret void setSecret(const std::string &name, std::string_view data); void getOrLoadSecret(const std::string &name1, const std::string &name2, std::string &data, std::string &data2); + /** Drop any cached data for @a name. + * + * The next getOrLoadSecret() for @a name reloads the data, either from a + * TS_LIFECYCLE_SSL_SECRET_HOOK plugin or from the file itself. + */ + void invalidateSecret(const std::string &name); + private: void loadSecret(const std::string &name1, const std::string &name2, std::string &data_item, std::string &data_item2); std::string loadFile(const std::string &name); diff --git a/src/iocore/net/SSLCertLookup.cc b/src/iocore/net/SSLCertLookup.cc index bf33d0294f6..fdc0f9fdddd 100644 --- a/src/iocore/net/SSLCertLookup.cc +++ b/src/iocore/net/SSLCertLookup.cc @@ -34,6 +34,7 @@ #include "P_SSLUtils.h" #include +#include #include #include #include @@ -234,24 +235,24 @@ ssl_create_ticket_keyblock(const char *ticket_key_path) SSLCertContext::SSLCertContext(SSLCertContext const &other) { + std::shared_lock lock(other.ctx_mutex); opt = other.opt; userconfig = other.userconfig; keyblock = other.keyblock; ctx_type = other.ctx_type; - std::shared_lock lock(other.ctx_mutex); - ctx = other.ctx; + ctx = other.ctx; } SSLCertContext & SSLCertContext::operator=(SSLCertContext const &other) { if (&other != this) { + std::scoped_lock lock(this->ctx_mutex, other.ctx_mutex); this->opt = other.opt; this->userconfig = other.userconfig; this->keyblock = other.keyblock; this->ctx_type = other.ctx_type; - std::shared_lock lock(other.ctx_mutex); - this->ctx = other.ctx; + this->ctx = other.ctx; } return *this; } diff --git a/src/iocore/net/SSLClientUtils.cc b/src/iocore/net/SSLClientUtils.cc index a91b86c5cfd..0087188ead1 100644 --- a/src/iocore/net/SSLClientUtils.cc +++ b/src/iocore/net/SSLClientUtils.cc @@ -338,7 +338,7 @@ SSLInitClientContext(const SSLConfigParams *params) } SSL_CTX * -SSLCreateClientContext(const struct SSLConfigParams *params, const char *ca_bundle_path, const char *ca_bundle_file, +SSLCreateClientContext(const struct SSLConfigParams *params, const char *ca_bundle_file, const char *ca_bundle_path, const char *cert_path, const char *key_path) { std::unique_ptr ctx(nullptr, &SSL_CTX_free); diff --git a/src/iocore/net/SSLSecret.cc b/src/iocore/net/SSLSecret.cc index fd8204a1c47..7249681aa28 100644 --- a/src/iocore/net/SSLSecret.cc +++ b/src/iocore/net/SSLSecret.cc @@ -119,6 +119,16 @@ SSLSecret::setSecret(const std::string &name, std::string_view data) Dbg(dbg_ctl_ssl_secret, "Set secret for %s to %.*s", name.c_str(), int(data.size() > 50 ? 50 : data.size()), data.data()); } +void +SSLSecret::invalidateSecret(const std::string &name) +{ + std::scoped_lock lock(secret_map_mutex); + + if (secret_map.erase(name) > 0) { + Dbg(dbg_ctl_ssl_secret, "Invalidated cached secret for %s", name.c_str()); + } +} + std::string SSLSecret::getSecret(const std::string &name) const { diff --git a/src/mgmt/config/ConfigContext.cc b/src/mgmt/config/ConfigContext.cc index fc5d2772c7a..2f26c9b098a 100644 --- a/src/mgmt/config/ConfigContext.cc +++ b/src/mgmt/config/ConfigContext.cc @@ -149,7 +149,8 @@ ConfigContext::add_dependent_ctx(std::string_view description, std::string_view // child task will get the full content of the parent task // TODO: eventually we can have a "key" passed so child module // only gets their node of interest. - child._supplied_yaml = _supplied_yaml; + child._supplied_yaml = _supplied_yaml; + child._reload_directives = _reload_directives; return child; } return {}; @@ -167,6 +168,18 @@ ConfigContext::supplied_yaml() const return _supplied_yaml; } +void +ConfigContext::set_reload_directives(YAML::Node node) +{ + _reload_directives = node; +} + +YAML::Node +ConfigContext::reload_directives() const +{ + return _reload_directives; +} + namespace config { ConfigContext diff --git a/src/mgmt/config/ConfigRegistry.cc b/src/mgmt/config/ConfigRegistry.cc index 0a3382f8463..776fa265d2f 100644 --- a/src/mgmt/config/ConfigRegistry.cc +++ b/src/mgmt/config/ConfigRegistry.cc @@ -431,15 +431,17 @@ ConfigRegistry::execute_reload(const std::string &key) { Dbg(dbg_ctl, "Executing reload for config '%s'", key.c_str()); - // Single lock for both lookups: passed config (from RPC) and registry entry YAML::Node passed_config; + bool has_passed_config{false}; Entry entry_copy; { - std::shared_lock lock(_mutex); + std::unique_lock lock(_mutex); if (auto pc_it = _passed_configs.find(key); pc_it != _passed_configs.end()) { - passed_config = pc_it->second; - Dbg(dbg_ctl, "Retrieved passed config for '%s'", key.c_str()); + passed_config = pc_it->second; + has_passed_config = true; + _passed_configs.erase(pc_it); + Dbg(dbg_ctl, "Retrieved and consumed passed config for '%s'", key.c_str()); } if (auto it = _entries.find(key); it != _entries.end()) { @@ -455,14 +457,31 @@ ConfigRegistry::execute_reload(const std::string &key) // Create context with subtask tracking // For rpc reload: use key as description, no filename (source: rpc) // For file reload: use key as description, filename indicates source: file - std::string filename = passed_config.IsDefined() ? "" : entry_copy.resolve_filename(); + std::string filename = has_passed_config ? "" : entry_copy.resolve_filename(); auto ctx = ReloadCoordinator::Get_Instance().create_config_context(entry_copy.key, entry_copy.key, filename); ctx.in_progress(); - if (passed_config.IsDefined()) { - // Passed config mode: store YAML node directly for handler to use via supplied_yaml() + if (has_passed_config) { Dbg(dbg_ctl, "Config '%s' reloading from rpc-supplied content", entry_copy.key.c_str()); - ctx.set_supplied_yaml(passed_config); + + // Extract _reload directives before passing content to the handler. + // This keeps supplied_yaml() clean (pure config data) and provides + // reload_directives() as a separate accessor for operational parameters. + if (passed_config.IsMap() && passed_config["_reload"]) { + auto directives = passed_config["_reload"]; + if (!directives.IsMap()) { + Warning("Config '%s': _reload must be a YAML map, ignoring directives", entry_copy.key.c_str()); + } else { + Dbg(dbg_ctl, "Config '%s' has reload directives", entry_copy.key.c_str()); + ctx.set_reload_directives(directives); + } + passed_config.remove("_reload"); + } + + // After stripping _reload, pass remaining content (if any) as supplied_yaml + if (passed_config.size() > 0) { + ctx.set_supplied_yaml(passed_config); + } } else { Dbg(dbg_ctl, "Config '%s' reloading from file '%s'", entry_copy.key.c_str(), filename.c_str()); } diff --git a/src/mgmt/rpc/handlers/server/Server.cc b/src/mgmt/rpc/handlers/server/Server.cc index f429d0787cc..c5e440d4b54 100644 --- a/src/mgmt/rpc/handlers/server/Server.cc +++ b/src/mgmt/rpc/handlers/server/Server.cc @@ -18,7 +18,6 @@ limitations under the License. */ -#include "../../../../iocore/cache/P_CacheDir.h" #include "iocore/eventsystem/EventProcessor.h" #include "iocore/net/ConnectionTracker.h" #include "mgmt/rpc/handlers/server/Server.h" @@ -176,12 +175,6 @@ server_stop_drain(std::string_view const & /* id ATS_UNUSED */, YAML::Node const return resp; } -void -server_shutdown(YAML::Node const &) -{ - sync_cache_dir_on_shutdown(); -} - swoc::Rv get_server_status(std::string_view const & /* params ATS_UNUSED */, YAML::Node const & /* params ATS_UNUSED */) { diff --git a/src/proxy/http/HttpBodyFactory.cc b/src/proxy/http/HttpBodyFactory.cc index cdaaac582d8..fc91ce77825 100644 --- a/src/proxy/http/HttpBodyFactory.cc +++ b/src/proxy/http/HttpBodyFactory.cc @@ -261,6 +261,7 @@ HttpBodyFactory::reconfigure() unlock(); return; } // callbacks not setup right + unlock(); //////////////////////////////////////////// // extract relevant records.yaml values // @@ -272,14 +273,14 @@ HttpBodyFactory::reconfigure() // enable_customizations if records.yaml set auto e{RecGetRecordInt("proxy.config.body_factory.enable_customizations")}; - enable_customizations = (e.has_value() ? e.value() : 0); - all_found = all_found && e.has_value(); - Dbg(dbg_ctl_body_factory, "enable_customizations = %d (found = %d)", enable_customizations, e.has_value()); + int new_enable_customizations = (e.has_value() ? e.value() : 0); + all_found = all_found && e.has_value(); + Dbg(dbg_ctl_body_factory, "enable_customizations = %d (found = %d)", new_enable_customizations, e.has_value()); - e = RecGetRecordInt("proxy.config.body_factory.enable_logging"); - enable_logging = (e.has_value() ? (e.value() ? true : false) : false); - all_found = all_found && e.has_value(); - Dbg(dbg_ctl_body_factory, "enable_logging = %d (found = %d)", enable_logging, e.has_value()); + e = RecGetRecordInt("proxy.config.body_factory.enable_logging"); + bool new_enable_logging = (e.has_value() ? (e.value() ? true : false) : false); + all_found = all_found && e.has_value(); + Dbg(dbg_ctl_body_factory, "enable_logging = %d (found = %d)", new_enable_logging, e.has_value()); ats_scoped_str directory_of_template_sets; @@ -305,21 +306,16 @@ HttpBodyFactory::reconfigure() Warning("config changed, but can't fetch all proxy.config.body_factory values"); } - ///////////////////////////////////////////// - // clear out previous template hash tables // - ///////////////////////////////////////////// - - nuke_template_tables(); - - ///////////////////////////////////////////////////////////// - // at this point, the body hash table is gone, so we start // - // building a new one, by scanning the template directory. // - ///////////////////////////////////////////////////////////// - + std::unique_ptr new_table_of_sets; if (directory_of_template_sets) { - table_of_sets = load_sets_from_directory(directory_of_template_sets); + new_table_of_sets = load_sets_from_directory(directory_of_template_sets); } + lock(); + enable_customizations = new_enable_customizations; + enable_logging = new_enable_logging; + nuke_template_tables(); + table_of_sets = std::move(new_table_of_sets); unlock(); } @@ -727,7 +723,6 @@ HttpBodyFactory::nuke_template_tables() } } -// LOCKING: must be called with lock taken std::unique_ptr HttpBodyFactory::load_sets_from_directory(char *set_dir) { @@ -797,7 +792,6 @@ HttpBodyFactory::load_sets_from_directory(char *set_dir) return new_table_of_sets; } -// LOCKING: must be called with lock taken HttpBodySet * HttpBodyFactory::load_body_set_from_directory(char *set_name, char *tmpl_dir) { diff --git a/src/proxy/http/HttpProxyServerMain.cc b/src/proxy/http/HttpProxyServerMain.cc index 467fcad36a0..4701d9771cf 100644 --- a/src/proxy/http/HttpProxyServerMain.cc +++ b/src/proxy/http/HttpProxyServerMain.cc @@ -194,19 +194,30 @@ MakeHttpProxyAcceptor(HttpProxyAcceptor &acceptor, HttpProxyPort &port, unsigned // XXX the protocol probe should be a configuration option. - ProtocolProbeSessionAccept *probe = new ProtocolProbeSessionAccept(); + // A QUIC port is dispatched to the QUIC acceptor below, which has no probe fallback, so building a + // probe for one only leaks it. Every other port type ends up behind the probe, either directly or + // as the SSL acceptor's fallback. Without QUIC compiled in no port can be a QUIC port, so this is + // always true there. + bool const needs_probe = !port.isQUIC(); + + ProtocolProbeSessionAccept *probe = nullptr; HttpSessionAccept *http = nullptr; // don't allocate this unless it will be used. - probe->proxyPort = &port; - probe->proxy_protocol_ipmap = &HttpConfig::m_master.config_proxy_protocol_ip_addrs; - if (port.m_session_protocol_preference.intersects(HTTP_PROTOCOL_SET)) { - http = new HttpSessionAccept(accept_opt); - probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP, http); - } + if (needs_probe) { + probe = new ProtocolProbeSessionAccept(); + probe->proxyPort = &port; + probe->proxy_protocol_ipmap = &HttpConfig::m_master.config_proxy_protocol_ip_addrs; + + if (port.m_session_protocol_preference.intersects(HTTP_PROTOCOL_SET)) { + http = new HttpSessionAccept(accept_opt); + probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP, http); + } - if (port.m_session_protocol_preference.intersects(HTTP2_PROTOCOL_SET)) { - probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP2, new Http2SessionAccept(accept_opt)); + if (port.m_session_protocol_preference.intersects(HTTP2_PROTOCOL_SET)) { + probe->registerEndpoint(ProtocolProbeSessionAccept::ProtoGroupKey::HTTP2, new Http2SessionAccept(accept_opt)); + } } + ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_1_0, create_h1_server_session}); ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_1_1, create_h1_server_session}); ProtocolSessionCreateMap.insert({TS_ALPN_PROTOCOL_INDEX_HTTP_2_0, create_h2_server_session}); diff --git a/src/proxy/http3/Http3Frame.cc b/src/proxy/http3/Http3Frame.cc index 2c15ee03ef3..54cd6355699 100644 --- a/src/proxy/http3/Http3Frame.cc +++ b/src/proxy/http3/Http3Frame.cc @@ -599,8 +599,7 @@ Http3FrameFactory::create_headers_frame(IOBufferReader *header_block_reader, siz { ats_unique_buf buf = ats_unique_malloc(header_block_len); - int64_t nread; - while ((nread = header_block_reader->read(buf.get(), header_block_len)) > 0) { + while (header_block_reader->read(buf.get(), header_block_len) > 0) { ; } diff --git a/src/proxy/http3/Http3Session.cc b/src/proxy/http3/Http3Session.cc index 1c92f031b21..ed62c55dd2e 100644 --- a/src/proxy/http3/Http3Session.cc +++ b/src/proxy/http3/Http3Session.cc @@ -65,11 +65,12 @@ HQSession::remove_transaction(HQTransaction *trans) void HQSession::_close_transactions() { - while (this->_transaction_list.head != nullptr) { - auto *transaction = this->_transaction_list.head; + for (auto *transaction = this->_transaction_list.head; transaction != nullptr;) { + auto *next = static_cast(transaction->link.next); transaction->do_io_close(); delete transaction; + transaction = next; } } diff --git a/src/proxy/http3/QPACK.cc b/src/proxy/http3/QPACK.cc index 45da0384942..38aa3a5630f 100644 --- a/src/proxy/http3/QPACK.cc +++ b/src/proxy/http3/QPACK.cc @@ -291,11 +291,13 @@ QPACK::decode(uint64_t stream_id, const uint8_t *header_block, size_t header_blo if (largest_reference != 0 && (this->_dynamic_table.is_empty() || this->_dynamic_table.largest_index() < largest_reference)) { // Blocked - if (this->_add_to_blocked_list( - new DecodeRequest(largest_reference, thread, cont, stream_id, header_block, header_block_len, hdr))) { + auto *decode_request = new DecodeRequest(largest_reference, thread, cont, stream_id, header_block, header_block_len, hdr); + + if (this->_add_to_blocked_list(decode_request)) { return 1; } else { // Number of blocked streams exceed the limit + delete decode_request; return -2; } } diff --git a/src/proxy/logging/LogAccess.cc b/src/proxy/logging/LogAccess.cc index 33d2f2f06c5..4471d11af8f 100644 --- a/src/proxy/logging/LogAccess.cc +++ b/src/proxy/logging/LogAccess.cc @@ -1627,7 +1627,7 @@ LogAccess::marshal_version_string(char *buf) int LogAccess::marshal_proxy_protocol_version(char *buf) { - const char *version_str = "-"; + const char *version_str; switch (m_data->get_pp_version()) { case 1: version_str = "V1"; diff --git a/src/proxy/logging/LogFieldFallback.cc b/src/proxy/logging/LogFieldFallback.cc index 876ac722060..9a98574e122 100644 --- a/src/proxy/logging/LogFieldFallback.cc +++ b/src/proxy/logging/LogFieldFallback.cc @@ -83,13 +83,13 @@ constexpr bool test_find_field_fallback_separator() { static_assert(find_field_fallback_separator("") == nullptr); - constexpr char const *text1 = "{field}??default"; + [[maybe_unused]] constexpr char text1[] = "{field}??default"; static_assert(find_field_fallback_separator(text1) == text1 + 7); - constexpr char const *text2 = "??default"; + [[maybe_unused]] constexpr char text2[] = "??default"; static_assert(find_field_fallback_separator(text2) == text2); - constexpr char const *text3 = "{field}??def??ault"; + [[maybe_unused]] constexpr char text3[] = "{field}??def??ault"; static_assert(find_field_fallback_separator(text3) == text3 + 7); - constexpr char const *text4 = "{field}??\"def??ault\""; + [[maybe_unused]] constexpr char text4[] = "{field}??\"def??ault\""; static_assert(find_field_fallback_separator(text4) == text4 + 7); return true; } diff --git a/src/records/CMakeLists.txt b/src/records/CMakeLists.txt index f5e643ae692..6fd4ce94874 100644 --- a/src/records/CMakeLists.txt +++ b/src/records/CMakeLists.txt @@ -54,6 +54,7 @@ if(BUILD_TESTING) unit_tests/test_ConfigReloadTask.cc unit_tests/test_ConfigRegistry.cc unit_tests/test_RecHiddenMetricLookup.cc + unit_tests/test_ReloadDirectives.cc ) target_link_libraries(test_records PRIVATE records configmanager inkevent Catch2::Catch2 ts::tscore libswoc::libswoc) add_catch2_test(NAME test_records COMMAND test_records) diff --git a/src/records/unit_tests/test_RecRegister.cc b/src/records/unit_tests/test_RecRegister.cc index 38fb1adbf03..a406b6fbd6e 100644 --- a/src/records/unit_tests/test_RecRegister.cc +++ b/src/records/unit_tests/test_RecRegister.cc @@ -26,6 +26,7 @@ #include "test_Diags.h" #include +#include #include TEST_CASE("RecRegisterConfig - Type Dispatch", "[librecords][RecConfig]") @@ -103,7 +104,8 @@ TEST_CASE("RecLookupRecord - Concurrent metric registration", "[librecords][RecL std::atomic finished{false}; std::thread register_metrics([&]() { for (int i = 0; i < 100000; ++i) { - ts::Metrics::Counter::createSpan(1); + // Any registration will do; the point is to grow the store while lookups run. + ts::Metrics::Counter::create("proxy.test.concurrent.reg." + std::to_string(i)); } finished.store(true, std::memory_order_release); }); diff --git a/src/records/unit_tests/test_ReloadDirectives.cc b/src/records/unit_tests/test_ReloadDirectives.cc new file mode 100644 index 00000000000..5b90b607081 --- /dev/null +++ b/src/records/unit_tests/test_ReloadDirectives.cc @@ -0,0 +1,319 @@ +/** @file + + Unit tests for reload directives: ConfigContext directive accessors, + framework extraction logic, and CLI parse_directive() format. + + @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. +*/ + +#include + +#include "mgmt/config/ConfigContext.h" +#include "mgmt/config/ConfigReloadTrace.h" + +#include +#include +#include + +// ─── parse_directive: standalone copy of the traffic_ctl parsing logic ──────── +// +// The actual function lives in an anonymous namespace in CtrlCommands.cc. +// We reproduce the identical logic here so we can unit-test the format parsing +// without pulling in the full traffic_ctl binary and its dependencies. +namespace +{ +bool +parse_directive(std::string_view dir, YAML::Node &configs, std::string &error_out) +{ + auto dot = dir.find('.'); + if (dot == std::string_view::npos || dot == 0) { + error_out = "Invalid directive format '" + std::string(dir) + "'. Expected: config_key.directive_key=value"; + return false; + } + + auto eq = dir.find('=', dot + 1); + if (eq == std::string_view::npos || eq == dot + 1) { + error_out = "Invalid directive format '" + std::string(dir) + "'. Expected: config_key.directive_key=value"; + return false; + } + + std::string config_key{dir.substr(0, dot)}; + std::string directive_key{dir.substr(dot + 1, eq - dot - 1)}; + std::string value{dir.substr(eq + 1)}; + + configs[config_key]["_reload"][directive_key] = value; + return true; +} +} // namespace + +// ─── parse_directive format tests ───────────────────────────────────────────── + +TEST_CASE("parse_directive: valid single directive", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("myconfig.id=foo", configs, err)); + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); +} + +TEST_CASE("parse_directive: value with equals signs", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("plugin.url=http://x.com/a=b", configs, err)); + REQUIRE(configs["plugin"]["_reload"]["url"].as() == "http://x.com/a=b"); +} + +TEST_CASE("parse_directive: value with dots", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("plugin.fqdn=foo.example.com", configs, err)); + REQUIRE(configs["plugin"]["_reload"]["fqdn"].as() == "foo.example.com"); +} + +TEST_CASE("parse_directive: empty value is allowed", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("myconfig.flag=", configs, err)); + REQUIRE(configs["myconfig"]["_reload"]["flag"].as() == ""); +} + +TEST_CASE("parse_directive: multiple directives for same config", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("myconfig.id=foo", configs, err)); + REQUIRE(parse_directive("myconfig.dry_run=true", configs, err)); + + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); + REQUIRE(configs["myconfig"]["_reload"]["dry_run"].as() == "true"); +} + +TEST_CASE("parse_directive: multiple directives for different configs", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE(parse_directive("myconfig.id=foo", configs, err)); + REQUIRE(parse_directive("sni.fqdn=example.com", configs, err)); + + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); + REQUIRE(configs["sni"]["_reload"]["fqdn"].as() == "example.com"); +} + +TEST_CASE("parse_directive: rejects missing dot", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE_FALSE(parse_directive("nodot", configs, err)); + REQUIRE(err.find("Invalid directive format") != std::string::npos); +} + +TEST_CASE("parse_directive: rejects leading dot", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE_FALSE(parse_directive(".key=value", configs, err)); + REQUIRE(err.find("Invalid directive format") != std::string::npos); +} + +TEST_CASE("parse_directive: rejects missing equals", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE_FALSE(parse_directive("config.key", configs, err)); + REQUIRE(err.find("Invalid directive format") != std::string::npos); +} + +TEST_CASE("parse_directive: rejects empty directive key", "[config][directive][parse]") +{ + YAML::Node configs; + std::string err; + + REQUIRE_FALSE(parse_directive("config.=value", configs, err)); + REQUIRE(err.find("Invalid directive format") != std::string::npos); +} + +// ─── ConfigContext directive accessor tests ─────────────────────────────────── + +TEST_CASE("ConfigContext: reload_directives on default context has no keys", "[config][context][directive]") +{ + ConfigContext ctx; + + // Members are initialized as Undefined, so operator bool() is false. + YAML::Node const directives = ctx.reload_directives(); + REQUIRE_FALSE(directives.IsDefined()); + REQUIRE_FALSE(directives); + REQUIRE_FALSE(directives["id"].IsDefined()); +} + +TEST_CASE("ConfigContext: supplied_yaml on default context has no content", "[config][context][directive]") +{ + ConfigContext ctx; + + auto yaml = ctx.supplied_yaml(); + REQUIRE_FALSE(yaml.IsDefined()); + REQUIRE_FALSE(yaml); + REQUIRE_FALSE(yaml.IsMap()); + REQUIRE_FALSE(yaml.IsSequence()); +} + +TEST_CASE("ConfigContext: reload_directives round-trip via task", "[config][context][directive]") +{ + auto task = std::make_shared("test-dir-1", "test", false, nullptr); + ConfigContext ctx(task, "test_handler"); + + YAML::Node directives; + directives["id"] = "foo"; + directives["dry_run"] = "true"; + + // Use the private setter via a ConfigContext that has a live task. + // Since set_reload_directives is private and friend-accessible only from + // ConfigRegistry/ReloadCoordinator, we test through the public interface + // after setting up the state that execute_reload would create. + + // Simulate what ConfigRegistry::execute_reload() does: + // Build a passed_config with _reload, then manually extract + YAML::Node passed_config; + passed_config["_reload"]["id"] = "foo"; + passed_config["_reload"]["dry_run"] = "true"; + passed_config["data"] = "some content"; + + // Extract _reload (same logic as execute_reload) + if (passed_config.IsMap() && passed_config["_reload"]) { + auto dir = passed_config["_reload"]; + if (dir.IsMap()) { + // We can't call set_reload_directives directly (private). + // But we can verify the extraction logic works on the YAML node. + REQUIRE(dir["id"].as() == "foo"); + REQUIRE(dir["dry_run"].as() == "true"); + passed_config.remove("_reload"); + } + } + + // After extraction, passed_config should only have "data" + REQUIRE_FALSE(passed_config["_reload"].IsDefined()); + REQUIRE(passed_config["data"].as() == "some content"); + REQUIRE(passed_config.size() == 1); +} + +TEST_CASE("ConfigContext: _reload extraction with directives only", "[config][context][directive]") +{ + YAML::Node passed_config; + passed_config["_reload"]["id"] = "bar"; + + // Extract + YAML::Node directives; + if (passed_config.IsMap() && passed_config["_reload"]) { + directives = passed_config["_reload"]; + passed_config.remove("_reload"); + } + + REQUIRE(directives.IsDefined()); + REQUIRE(directives["id"].as() == "bar"); + + // After extraction with only _reload, the map should be empty + REQUIRE(passed_config.size() == 0); +} + +TEST_CASE("ConfigContext: _reload extraction with content only", "[config][context][directive]") +{ + YAML::Node passed_config; + passed_config["rules"].push_back("rule1"); + passed_config["rules"].push_back("rule2"); + + bool extracted = false; + if (passed_config.IsMap() && passed_config["_reload"]) { + extracted = true; + passed_config.remove("_reload"); + } + + // No _reload key present — extraction did not fire + REQUIRE_FALSE(extracted); + + // Content untouched + REQUIRE(passed_config["rules"].size() == 2); +} + +TEST_CASE("ConfigContext: _reload non-map is rejected", "[config][context][directive]") +{ + YAML::Node passed_config; + passed_config["_reload"] = "scalar_value"; + passed_config["data"] = "content"; + + bool extracted = false; + bool rejected = false; + if (passed_config.IsMap() && passed_config["_reload"]) { + auto dir = passed_config["_reload"]; + if (!dir.IsMap()) { + rejected = true; + } else { + extracted = true; + } + passed_config.remove("_reload"); + } + + REQUIRE(rejected); + REQUIRE_FALSE(extracted); + // _reload is still removed even when rejected + REQUIRE_FALSE(passed_config["_reload"].IsDefined()); + REQUIRE(passed_config["data"].as() == "content"); +} + +// ─── Wire format integration: -D flag produces correct YAML structure ───────── + +TEST_CASE("Wire format: -D produces _reload nested under config key", "[config][directive][wire]") +{ + YAML::Node configs; + std::string err; + + parse_directive("myconfig.id=foo", configs, err); + + // Verify the structure matches what the server expects + REQUIRE(configs.IsMap()); + REQUIRE(configs["myconfig"].IsMap()); + REQUIRE(configs["myconfig"]["_reload"].IsMap()); + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); +} + +TEST_CASE("Wire format: -D combined with -d content", "[config][directive][wire]") +{ + YAML::Node configs; + + // Simulate -d providing content + configs["myconfig"]["rules"].push_back("rule1"); + + // Then -D adding directives + std::string err; + parse_directive("myconfig.id=foo", configs, err); + + // Both coexist under the same config key + REQUIRE(configs["myconfig"]["rules"].size() == 1); + REQUIRE(configs["myconfig"]["_reload"]["id"].as() == "foo"); +} diff --git a/src/traffic_ctl/CtrlCommands.cc b/src/traffic_ctl/CtrlCommands.cc index b90d14c43c9..9a92ec3826a 100644 --- a/src/traffic_ctl/CtrlCommands.cc +++ b/src/traffic_ctl/CtrlCommands.cc @@ -51,6 +51,16 @@ const StringToFormatFlagsMap _Fmt_str_to_enum = { {"rpc", BasePrinter::Options::FormatFlags::RPC } }; +/// `-d ""`, or `-d $VAR` with VAR unset, yields an empty token that survives +/// ArgumentData::size() and is then skipped by the parse loops, so the reload would quietly +/// cover fewer configs than the operator asked for. Reported separately from the bare option +/// so the operator is told which of the two mistakes they made. +bool +has_empty_value(ts::ArgumentData const &args) +{ + return std::any_of(args.begin(), args.end(), [](std::string const &value) { return value.empty(); }); +} + constexpr std::string_view YAML_PREFIX{"records."}; constexpr std::string_view RECORD_PREFIX{"proxy.config."}; @@ -81,6 +91,33 @@ display_errors(BasePrinter *printer, std::vector co } } } + +/// Parse a single --directive (-D) argument "config_key.directive_key=value" +/// and inject into configs[config_key]["_reload"][directive_key]. +/// Returns true on success, sets error_out on parse failure. +bool +parse_directive(std::string_view dir, YAML::Node &configs, std::string &error_out) +{ + auto dot = dir.find('.'); + if (dot == std::string_view::npos || dot == 0) { + error_out = "Invalid directive format '" + std::string(dir) + "'. Expected: config_key.directive_key=value"; + return false; + } + + auto eq = dir.find('=', dot + 1); + if (eq == std::string_view::npos || eq == dot + 1) { + error_out = "Invalid directive format '" + std::string(dir) + "'. Expected: config_key.directive_key=value"; + return false; + } + + std::string config_key{dir.substr(0, dot)}; + std::string directive_key{dir.substr(dot + 1, eq - dot - 1)}; + std::string value{dir.substr(eq + 1)}; + + configs[config_key]["_reload"][directive_key] = value; + return true; +} + } // namespace BasePrinter::Options::FormatFlags @@ -527,6 +564,21 @@ ConfigCommand::config_reload() _printer->write_output(""); } + // Without content the request would silently degrade to a full reload of every handler, + // which is the opposite of the scoped reload the operator asked for. + if (data_args) { + if (data_args.size() == 0) { + _printer->write_output("Error: --data (-d) requires content: @file, @- or a YAML string"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + if (has_empty_value(data_args)) { + _printer->write_output("Error: --data (-d) received an empty value, so its config would be left out"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + } + // Parse inline config data if provided (supports multiple -d arguments) YAML::Node configs; for (auto const &data_arg : data_args) { @@ -557,6 +609,32 @@ ConfigCommand::config_reload() } } + // Parse --directive (-D) arguments into configs[key]["_reload"][directive] = value + auto dir_args = get_parsed_arguments()->get("directive"); + if (dir_args) { + if (dir_args.size() == 0) { + _printer->write_output("Error: --directive (-D) requires at least one config_key.directive_key=value"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + if (has_empty_value(dir_args)) { + _printer->write_output("Error: --directive (-D) received an empty value, so its directive would be left out"); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + } + for (auto const &dir : dir_args) { + if (dir.empty()) { + continue; + } + std::string err; + if (!parse_directive(dir, configs, err)) { + _printer->write_output("Error: " + err); + App_Exit_Status_Code = CTRL_EX_ERROR; + return; + } + } + using ConfigError = config::reload::errors::ConfigReloadError; auto contains_error = [](std::vector const &errors, ConfigError error) -> bool { diff --git a/src/traffic_ctl/traffic_ctl.cc b/src/traffic_ctl/traffic_ctl.cc index ba704f1745b..286461e556a 100644 --- a/src/traffic_ctl/traffic_ctl.cc +++ b/src/traffic_ctl/traffic_ctl.cc @@ -116,7 +116,7 @@ main([[maybe_unused]] int argc, const char **argv) .add_example_usage("traffic_ctl config get [OPTIONS] RECORD [RECORD ...]") .add_option("--cold", "-c", "Save the value in a configuration file. This does not save the value in TS. Local file change only", - "TS_RECORD_YAML", MORE_THAN_ZERO_ARG_N) + "TS_RECORD_YAML", AT_MOST_ONE_ARG_N) .add_option("--records", "", "Emit output in YAML format") .add_option("--default", "", "Include default value"); config_command.add_command("match", "Get configuration matching a regular expression", "", MORE_THAN_ONE_ARG_N, Command_Execute) @@ -164,6 +164,8 @@ main([[maybe_unused]] int argc, const char **argv) // -d @- - read config from stdin // -d "yaml: content" - inline yaml string .add_option("--data", "-d", "Inline config data (@file, @- for stdin, or yaml string)", "", MORE_THAN_ZERO_ARG_N, "") + .add_option("--directive", "-D", "Pass a reload directive to a config handler (format: config_key.directive_key=value)", "", + MORE_THAN_ZERO_ARG_N, "") .add_option( "--initial-wait", "-w", "Initial wait before first poll, giving the server time to schedule all handlers (seconds). Accepts fractional values", "", 1, @@ -182,7 +184,7 @@ main([[maybe_unused]] int argc, const char **argv) config_command.add_command("set", "Set a configuration value", "", 2, Command_Execute) .add_option("--cold", "-c", "Save the value in a configuration file. This does not save the value in TS. Local file change only", - "TS_RECORD_YAML", MORE_THAN_ZERO_ARG_N) + "TS_RECORD_YAML", AT_MOST_ONE_ARG_N) .add_option("--update", "-u", "Update a configuration value. [only relevant if --cold set]") .add_option( "--type", "-t", diff --git a/src/tscore/ArgParser.cc b/src/tscore/ArgParser.cc index 90c3c2f5e43..eff0c42e9ec 100644 --- a/src/tscore/ArgParser.cc +++ b/src/tscore/ArgParser.cc @@ -195,6 +195,9 @@ ArgParser::parse(const char **argv) if (!default_command.empty()) { args = _argv; args.insert(args.begin() + 1, default_command); + // The pass that failed may have collected options before it gave up. Those values would + // now accumulate on top of the ones the retry collects rather than be replaced. + ret = Arguments{}; _top_level_command.parse(ret, args); } }; @@ -418,6 +421,8 @@ ArgParser::Command::output_option() const return {" [ ...]"}; } else if (num == MORE_THAN_ONE_ARG_N) { return {" ..."}; + } else if (num == AT_MOST_ONE_ARG_N) { + return {" []"}; } else { return " ... "; } @@ -518,34 +523,113 @@ ArgParser::Command::output_option() const } } +bool +ArgParser::Command::is_registered_option(std::string const &token) const +{ + if (_option_list.find(token) != _option_list.end() || _option_map.find(token) != _option_map.end()) { + return true; + } + // The --option=value form. + if (token.size() > 2 && token[0] == '-' && token[1] == '-') { + if (auto const pos = token.find_first_of('='); pos != std::string::npos) { + return _option_list.find(token.substr(0, pos)) != _option_list.end(); + } + } + return false; +} + // helper method to handle the arguments and put them nicely in arguments // can be switched to ts::errata -static std::string -handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) +std::string +ArgParser::Command::handle_args(Arguments &ret, AP_StrVec &args, std::string const &name, unsigned arg_num, unsigned &index) const { - ArgumentData data; - ret.append(name, data); + // A repeated option taking an unbounded number of values accumulates, as the --option=value + // form always has, so an entry already written by this pass keeps the values it collected. A + // fixed arity option keeps its last-one-wins behaviour, which is a separate concern. + bool const accumulates = MORE_THAN_ZERO_ARG_N == arg_num || MORE_THAN_ONE_ARG_N == arg_num; + + if (!accumulates || !ret.has(name)) { + ArgumentData data; + ret.append(name, data); + } // handle the args - if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) { - // infinite arguments - if (arg_num == MORE_THAN_ONE_ARG_N && args.size() <= index + 1) { - return "at least one argument expected by " + name; + if (arg_num == AT_MOST_ONE_ARG_N) { + // Zero or one value. A value is taken only when the following token does not name + // another option of this command, which leaves this command's positional arguments + // in place. A "--" token makes whatever follows it a value rather than an option. + unsigned j{index + 1}; + bool takes_value{false}; + + if (j < args.size()) { + if (args[j] == "--") { + ++j; + takes_value = j < args.size(); + } else { + takes_value = !is_registered_option(args[j]); + } } - for (unsigned j = index + 1; j < args.size(); j++) { + if (takes_value) { + ret.append_arg(name, args[j]); + ++j; + } + args.erase(args.begin() + index, args.begin() + j); + index -= 1; + return ""; + } + if (arg_num == MORE_THAN_ZERO_ARG_N || arg_num == MORE_THAN_ONE_ARG_N) { + // Variable number of arguments. Stop collecting at a token that names another option of this + // command, so options written afterwards keep their own values. Every other token is taken as + // a value, including a positional argument of the command, which is why an option whose value + // is optional wants AT_MOST_ONE_ARG_N rather than MORE_THAN_ZERO_ARG_N. A "--" token ends + // option recognition, which is how a value that starts with '-' can be passed. + unsigned j{index + 1}; + unsigned collected{0}; + bool recognize_options{true}; + + for (; j < args.size(); j++) { + if (recognize_options) { + if (args[j] == "--") { + recognize_options = false; + continue; + } + if (is_registered_option(args[j])) { + break; + } + } ret.append_arg(name, args[j]); + ++collected; } - args.erase(args.begin() + index, args.end()); + if (arg_num == MORE_THAN_ONE_ARG_N && collected == 0) { + return "at least one argument expected by " + name; + } + args.erase(args.begin() + index, args.begin() + j); + index -= 1; return ""; } - // finite number of argument handling - for (unsigned j = 0; j < arg_num; j++) { - if (args.size() < index + j + 2 || args[index + j + 1].empty()) { + // Fixed number of arguments. A token naming another option of this command is not a value, so + // the missing value is reported rather than the following option being consumed as one. A "--" + // token ends option recognition, which is how a value that starts with '-' is passed. + unsigned j{index + 1}; + bool recognize_options{true}; + + for (unsigned collected{0}; collected < arg_num; ++j) { + if (j >= args.size() || args[j].empty()) { return std::to_string(arg_num) + " argument(s) expected by " + name; } - ret.append_arg(name, args[index + j + 1]); + if (recognize_options) { + if (args[j] == "--") { + recognize_options = false; + continue; + } + if (is_registered_option(args[j])) { + return std::to_string(arg_num) + " argument(s) expected by " + name; + } + } + ret.append_arg(name, args[j]); + ++collected; } // erase the used arguments and append the data to the return structure - args.erase(args.begin() + index, args.begin() + index + arg_num + 1); + args.erase(args.begin() + index, args.begin() + j); index -= 1; return ""; } @@ -658,7 +742,7 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde if (args[i][0] == '-' && args[i][1] == '-' && args[i].find('=') != std::string::npos) { // deal with --args= std::string option_name = args[i].substr(0, args[i].find_first_of('=')); - std::string value = args[i].substr(args[i].find_last_of('=') + 1); + std::string value = args[i].substr(args[i].find_first_of('=') + 1); if (value.empty()) { help_message("missing argument for '" + option_name + "'"); } @@ -705,6 +789,16 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde } else { cur_option = _option_list.at(short_it->second); } + // Counted for the same repetition check as the --option=value form, so that an option + // taking at most one value cannot be given more by mixing the two spellings. + if (cur_option.arg_num == AT_MOST_ONE_ARG_N) { + check_map[cur_option.long_option] += 1; + // An empty token is not a value, and the --option=value spelling already refuses one, + // so refuse it here rather than silently falling back to the declared default. + if (i + 1 < args.size() && args[i + 1].empty()) { + help_message("missing argument for '" + args[i] + "'"); + } + } // handle the arguments std::string err = handle_args(ret, args, cur_option.key, cur_option.arg_num, i); if (!err.empty()) { @@ -720,9 +814,15 @@ ArgParser::Command::append_option_data(Arguments &ret, AP_StrVec &args, int inde } // check for wrong number of arguments for --arg=... for (const auto &it : check_map) { - unsigned num = _option_list.at(it.first).arg_num; - if (num != it.second && num < MORE_THAN_ONE_ARG_N) { - help_message(std::to_string(_option_list.at(it.first).arg_num) + " arguments expected by " + it.first); + unsigned const num = _option_list.at(it.first).arg_num; + if (num == AT_MOST_ONE_ARG_N) { + // At most one, so a repeated option is as wrong as a repeated fixed arity one, which + // is_variable_arg_num() would otherwise wave through. + if (it.second > 1) { + help_message("at most one argument expected by " + it.first); + } + } else if (num != it.second && !is_variable_arg_num(num)) { + help_message(std::to_string(num) + " arguments expected by " + it.first); } } } @@ -841,6 +941,12 @@ Arguments::get(std::string const &name) return ArgumentData(); } +bool +Arguments::has(std::string const &name) const noexcept +{ + return _data_map.find(name) != _data_map.end(); +} + void Arguments::append(std::string const &key, ArgumentData const &value) { diff --git a/src/tscore/unit_tests/test_ArgParser.cc b/src/tscore/unit_tests/test_ArgParser.cc index 0a32502e964..8803fa2eff1 100644 --- a/src/tscore/unit_tests/test_ArgParser.cc +++ b/src/tscore/unit_tests/test_ArgParser.cc @@ -217,3 +217,271 @@ TEST_CASE("with_required does not trigger on default values", "[parse]") REQUIRE(parsed.get("threshold").value() == "300"); REQUIRE(parsed.get("verbose") == true); } + +TEST_CASE("Variable argument option stops at a following option", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--token", "-t", "a token", "", 1, ""); + cmd.add_option("--monitor", "-m", "monitor progress"); + + // A flag after a variable argument option is not swallowed as a value. + const char *argv1[] = {"test_prog", "reload", "-D", "a.id=1", "b.id=2", "-m", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + REQUIRE(parsed.get("monitor") == true); + + // A following option keeps its own argument. + const char *argv2[] = {"test_prog", "reload", "-D", "a.id=1", "-t", "my_token", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("token").value() == "my_token"); + + // The long form of the following option is recognized too. + const char *argv3[] = {"test_prog", "reload", "-D", "a.id=1", "--monitor", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("monitor") == true); + + // So is its --option=value form. + const char *argv4[] = {"test_prog", "reload", "-D", "a.id=1", "--token=my_token", nullptr}; + parsed = parser.parse(argv4); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("token").value() == "my_token"); +} + +TEST_CASE("Double dash ends option recognition for variable argument options", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--monitor", "-m", "monitor progress"); + + // After "--" a token that looks like an option is taken as a value instead. + const char *argv[] = {"test_prog", "reload", "-D", "--", "-m", "a.id=1", nullptr}; + ts::Arguments parsed = parser.parse(argv); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "-m"); + REQUIRE(parsed.get("directive")[1] == "a.id=1"); + REQUIRE(parsed.get("monitor") == false); +} + +TEST_CASE("Option value keeps embedded equal signs", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + + // Only the first '=' separates the option from its value. + const char *argv[] = {"test_prog", "reload", "--directive=ip_allow.id=foo", nullptr}; + ts::Arguments parsed = parser.parse(argv); + REQUIRE(parsed.get("directive").size() == 1); + REQUIRE(parsed.get("directive")[0] == "ip_allow.id=foo"); +} + +TEST_CASE("An option taking at most one argument leaves the positional arguments alone", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl config get [--cold [FILE]] RECORD [RECORD ...]". + ts::ArgParser::Command &cmd = parser.add_command("get", "get values", "", MORE_THAN_ONE_ARG_N, nullptr); + cmd.add_option("--cold", "-c", "read from a file", "", AT_MOST_ONE_ARG_N); + cmd.add_option("--records", "", "yaml output"); + + // The option takes its single value and stops, so the command keeps its own arguments. + const char *argv1[] = {"test_prog", "get", "-c", "records.yaml", "proxy.config.x", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); + + // Several positional arguments are unaffected. + const char *argv2[] = {"test_prog", "get", "-c", "records.yaml", "proxy.config.x", "proxy.config.y", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 2); + REQUIRE(parsed.get("get")[1] == "proxy.config.y"); + + // Trailing placement keeps working. + const char *argv3[] = {"test_prog", "get", "proxy.config.x", "-c", "records.yaml", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); + + // The --option=value form is not mistaken for a fixed arity mismatch. + const char *argv4[] = {"test_prog", "get", "--cold=records.yaml", "proxy.config.x", nullptr}; + parsed = parser.parse(argv4); + REQUIRE(parsed.get("cold").value() == "records.yaml"); + REQUIRE(parsed.get("get").size() == 1); +} + +TEST_CASE("An option taking at most one argument accepts no value at all", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + ts::ArgParser::Command &cmd = parser.add_command("get", "get values", "", MORE_THAN_ONE_ARG_N, nullptr); + cmd.add_option("--cold", "-c", "read from a file", "", AT_MOST_ONE_ARG_N); + cmd.add_option("--records", "", "yaml output"); + + // Called with no value, so the caller falls back to its own default. + const char *argv1[] = {"test_prog", "get", "proxy.config.x", "-c", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("cold") == true); + REQUIRE(parsed.get("cold").size() == 0); + REQUIRE(parsed.get("cold").value().empty()); + REQUIRE(parsed.get("get").size() == 1); + + // A following option is never taken as the value. + const char *argv2[] = {"test_prog", "get", "-c", "--records", "proxy.config.x", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("cold").size() == 0); + REQUIRE(parsed.get("records") == true); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); + + // After "--" even a token shaped like an option becomes the value. + const char *argv3[] = {"test_prog", "get", "-c", "--", "-weird-name.yaml", "proxy.config.x", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("cold").value() == "-weird-name.yaml"); + REQUIRE(parsed.get("get").size() == 1); + REQUIRE(parsed.get("get")[0] == "proxy.config.x"); +} + +TEST_CASE("An option taking a fixed number of arguments can be given a value shaped like an option", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl server debug enable [--tags TAGS] [--append]". + ts::ArgParser::Command &cmd = parser.add_command("enable", "enable debug"); + cmd.add_option("--tags", "-t", "debug tags", "", 1); + cmd.add_option("--append", "-a", "append to the existing tags"); + + // A value that starts with '-' is passed after "--", which is otherwise taken as naming an + // option and reported as a missing value. + const char *argv1[] = {"test_prog", "enable", "-t", "--", "-a", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("tags").value() == "-a"); + REQUIRE(parsed.get("append") == false); + + // The --option=value form needs no escape. + const char *argv2[] = {"test_prog", "enable", "--tags=-a", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("tags").value() == "-a"); + REQUIRE(parsed.get("append") == false); + + // An option written after the value keeps its own meaning. + const char *argv3[] = {"test_prog", "enable", "-t", "http", "-a", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("tags").value() == "http"); + REQUIRE(parsed.get("append") == true); +} + +TEST_CASE("A repeated variable argument option accumulates its values", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl config reload [-D DIRECTIVE...] [-d SOURCE...] [-m]". + ts::ArgParser::Command &cmd = parser.add_command("reload", "reload configs"); + cmd.add_option("--directive", "-D", "reload directives", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--data", "-d", "inline config data", "", MORE_THAN_ZERO_ARG_N, ""); + cmd.add_option("--monitor", "-m", "monitor progress"); + + // Collection stops at the second -D, so the values of the first must survive it. + const char *argv1[] = {"test_prog", "reload", "-D", "a.id=1", "-D", "b.id=2", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + + // Each occurrence keeps every value it collected, in the order written. + const char *argv2[] = {"test_prog", "reload", "-D", "a.id=1", "b.id=2", "-D", "c.id=3", "d.id=4", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("directive").size() == 4); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[3] == "d.id=4"); + + // An unrelated option written between the two occurrences keeps its own meaning. + const char *argv3[] = {"test_prog", "reload", "-D", "a.id=1", "-m", "-D", "b.id=2", nullptr}; + parsed = parser.parse(argv3); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + REQUIRE(parsed.get("monitor") == true); + + // The two spellings count against the same option, in either order. + const char *argv4[] = {"test_prog", "reload", "-D", "a.id=1", "--directive=b.id=2", nullptr}; + parsed = parser.parse(argv4); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("directive")[0] == "a.id=1"); + REQUIRE(parsed.get("directive")[1] == "b.id=2"); + + const char *argv5[] = {"test_prog", "reload", "--directive=a.id=1", "--directive=b.id=2", nullptr}; + parsed = parser.parse(argv5); + REQUIRE(parsed.get("directive").size() == 2); + + // Repeated -d merges the same way, which is what the documented multi-source reload needs. + const char *argv6[] = {"test_prog", "reload", "-d", "@ip_allow.yaml", "-d", "@sni.yaml", nullptr}; + parsed = parser.parse(argv6); + REQUIRE(parsed.get("data").size() == 2); + REQUIRE(parsed.get("data")[0] == "@ip_allow.yaml"); + REQUIRE(parsed.get("data")[1] == "@sni.yaml"); + + // Two different options each keep their own values. + const char *argv7[] = {"test_prog", "reload", "-D", "a.id=1", "-d", "@f.yaml", "-D", "b.id=2", nullptr}; + parsed = parser.parse(argv7); + REQUIRE(parsed.get("directive").size() == 2); + REQUIRE(parsed.get("data").size() == 1); + REQUIRE(parsed.get("data")[0] == "@f.yaml"); +} + +TEST_CASE("A repeated option requiring at least one argument accumulates too", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl rpc invoke [--params PARAM...]". + ts::ArgParser::Command &cmd = parser.add_command("invoke", "invoke a method"); + cmd.add_option("--params", "-p", "request parameters", "", MORE_THAN_ONE_ARG_N, ""); + cmd.add_option("--format", "-f", "output format", "", 1, ""); + + const char *argv1[] = {"test_prog", "invoke", "-p", "one", "-p", "two", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("params").size() == 2); + REQUIRE(parsed.get("params")[0] == "one"); + REQUIRE(parsed.get("params")[1] == "two"); + + // The arity is satisfied by the first occurrence, so a later one is not left short. + const char *argv2[] = {"test_prog", "invoke", "-p", "one", "-f", "json", "-p", "two", nullptr}; + parsed = parser.parse(argv2); + REQUIRE(parsed.get("params").size() == 2); + REQUIRE(parsed.get("format").value() == "json"); +} + +TEST_CASE("A repeated option taking a fixed number of arguments keeps the last value", "[parse]") +{ + ts::ArgParser parser; + parser.add_global_usage("test_prog [OPTIONS]"); + + // Mirrors "traffic_ctl server debug enable [--tags TAGS]". Only an unbounded arity + // accumulates; a fixed one keeps the behaviour it has always had. + ts::ArgParser::Command &cmd = parser.add_command("enable", "enable debug"); + cmd.add_option("--tags", "-t", "debug tags", "", 1); + + const char *argv1[] = {"test_prog", "enable", "-t", "http", "-t", "cache", nullptr}; + ts::Arguments parsed = parser.parse(argv1); + REQUIRE(parsed.get("tags").size() == 1); + REQUIRE(parsed.get("tags").value() == "cache"); +} diff --git a/src/tsutil/Metrics.cc b/src/tsutil/Metrics.cc index c339eea2df8..1168c7d2e44 100644 --- a/src/tsutil/Metrics.cc +++ b/src/tsutil/Metrics.cc @@ -58,12 +58,16 @@ Metrics::Storage::addBlob() // The mutex must be held before calling this! { auto blob = std::make_unique(); + auto const cur_blob = static_cast(_next_free.load(std::memory_order_relaxed) >> 16); + debug_assert(blob); - // The write below is to _blobs[_cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. - release_assert(_cur_blob < MAX_BLOBS - 1); + // The write below is to _blobs[cur_blob + 1], so the last usable blob index is MAX_BLOBS - 1. + release_assert(cur_blob < MAX_BLOBS - 1); + + _blobs[cur_blob + 1] = std::move(blob); - _blobs[++_cur_blob] = std::move(blob); - _cur_off = 0; + // One store publishes both; the install above is sequenced before it. + _next_free.store(_pack(cur_blob + 1, 0), std::memory_order_release); } Metrics::IdType @@ -76,22 +80,27 @@ Metrics::Storage::create(std::string_view name, const MetricType type) return it->second; } - // The slot is written below and the bookkeeping only then advances, calling addBlob() once - // _cur_off reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from - // ever being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE. - if (_cur_blob >= MAX_BLOBS - 1 && _cur_off >= MAX_SIZE - 1) { + // The slot is written below and the bookkeeping only then advances, calling addBlob() once the + // offset reaches MAX_SIZE. Refusing the final slot of the final blob keeps addBlob() from ever + // being reached in an exhausted store, at a cost of one slot out of MAX_BLOBS * MAX_SIZE. + auto const [cur_blob, cur_off] = _splitID(static_cast(_next_free.load(std::memory_order_relaxed))); + + if (cur_blob >= MAX_BLOBS - 1 && cur_off >= MAX_SIZE - 1) { return 0; // Slot 0 is the reserved bad_id. Cannot grow further. } - Metrics::IdType id = _makeId(_cur_blob, _cur_off, type); - Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get(); + Metrics::IdType id = _makeId(cur_blob, cur_off, type); + Metrics::NamesAndAtomics *blob = _blobs[cur_blob].get(); Metrics::NameStorage &names = std::get<0>(*blob); - names[_cur_off] = std::make_tuple(std::string(name), id); - _lookups.emplace(std::get<0>(names[_cur_off]), id); + names[cur_off] = std::make_tuple(std::string(name), id); + _lookups.emplace(std::get<0>(names[cur_off]), id); - if (++_cur_off >= MAX_SIZE) { - addBlob(); // This resets _cur_off to 0 as well + if (cur_off + 1 >= MAX_SIZE) { + addBlob(); + } else { + // Publishes the slot's name. + _next_free.store(_pack(cur_blob, cur_off + 1), std::memory_order_release); } return id; @@ -113,15 +122,16 @@ Metrics::Storage::lookup(const std::string_view name) const Metrics::AtomicType * Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name, Metrics::MetricType *out_type) const { - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + auto [blob_ix, offset] = _splitID(id); - // Do a sanity check on the ID, to make sure we don't index outside of the realm of possibility. - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { - blob = _blobs[0].get(); - offset = 0; + // Anything not naming an allocated slot resolves to the reserved bad_id slot. + if (!_is_allocated(id)) { + blob_ix = 0; + offset = 0; } + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + if (out_name) { *out_name = std::get<0>(std::get<0>(*blob)[offset]); } @@ -159,15 +169,16 @@ Metrics::Storage::lookup(const std::string_view name, Metrics::IdType *out_id, M std::string_view Metrics::Storage::name(Metrics::IdType id) const { - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + auto [blob_ix, offset] = _splitID(id); - // Do a sanity check on the ID, to make sure we don't index outside of the realm of possibility. - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { - blob = _blobs[0].get(); - offset = 0; + // Anything not naming an allocated slot resolves to the reserved bad_id slot. + if (!_is_allocated(id)) { + blob_ix = 0; + offset = 0; } + Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); + const std::string &result = std::get<0>(std::get<0>(*blob)[offset]); return result; @@ -179,72 +190,6 @@ Metrics::Storage::type(IdType id) const return _extractType(id); } -Metrics::SpanType -Metrics::Storage::createSpan(size_t size, Metrics::MetricType type, Metrics::IdType *id) -{ - release_assert(size <= MAX_SIZE); - std::lock_guard lock(_mutex); - - // On the final blob there is nowhere left to grow, so refuse a span that would fill or overflow - // it rather than letting addBlob() assert. Same intent as the guard in create(), and the same - // cost: some slots of the last blob go unused. - if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) { - if (id) { - *id = 0; // Slot 0 is the reserved bad_id. - } - return {}; - } - - // A span has to be contiguous, so one that does not fit in the current blob starts a new one. - if (_cur_off + size > MAX_SIZE) { - addBlob(); - } - - Metrics::IdType span_start = _makeId(_cur_blob, _cur_off, type); - Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get(); - Metrics::AtomicStorage &atomics = std::get<1>(*blob); - Metrics::SpanType span = Metrics::SpanType(&atomics[_cur_off], size); - - if (id) { - *id = span_start; - } - - _cur_off += size; - - // create() grows as soon as it consumes the last slot; do the same here. Otherwise a span ending - // exactly on the boundary leaves _cur_off at MAX_SIZE, and the next create() writes one past the - // end of the blob's name array. It also makes end() unreachable for iterator::next(), which - // wraps on ++offset == MAX_SIZE. - if (_cur_off >= MAX_SIZE) { - addBlob(); - } - - return span; -} - -bool -Metrics::Storage::rename(Metrics::IdType id, std::string_view name) -{ - auto [blob_ix, offset] = _splitID(id); - Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get(); - - // We can only rename Metrics that are already allocated - if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) { - return false; - } - - std::string &cur = std::get<0>(std::get<0>(*blob)[offset]); - std::lock_guard lock(_mutex); - - if (cur.length() > 0) { - _lookups.erase(cur); - } - cur = name; - _lookups.emplace(cur, id); - - return true; -} - // Iterator implementation void Metrics::iterator::next() diff --git a/src/tsutil/unit_tests/test_Metrics.cc b/src/tsutil/unit_tests/test_Metrics.cc index 960324997b0..f267097807f 100644 --- a/src/tsutil/unit_tests/test_Metrics.cc +++ b/src/tsutil/unit_tests/test_Metrics.cc @@ -24,8 +24,10 @@ #include #include +#include #include #include +#include #include #include #include @@ -93,37 +95,6 @@ TEST_CASE("Metrics", "[libtsapi][Metrics]") REQUIRE(m[storeid].load() == 42); } - SECTION("Span allocation") - { - ts::Metrics::IdType span_id; - auto fooid = m.lookup("foo"); - auto span = Metrics::Counter::createSpan(17, &span_id); - - REQUIRE(span.size() == 17); - // Not fixed offsets: those only hold against a virgin store. Assert instead that the span - // was allocated above the earlier metric and that every id in it is valid. Both ids are - // counters, so they are directly comparable -- ids encode the metric type, and so are not - // ordered across differing types. - REQUIRE(fooid != ts::Metrics::NOT_FOUND); - REQUIRE(span_id != ts::Metrics::NOT_FOUND); - REQUIRE(span_id > fooid); - for (size_t i = 0; i < span.size(); ++i) { - REQUIRE(m.valid(span_id + static_cast(i))); - } - - m.rename(span_id + 0, "span.0"); - m.rename(span_id + 1, "span.1"); - m.rename(span_id + 2, "span.2"); - REQUIRE(m.name(fooid) == "foo"); - REQUIRE(m.name(span_id + 0) == "span.0"); - REQUIRE(m.name(span_id + 1) == "span.1"); - REQUIRE(m.name(span_id + 2) == "span.2"); - m.rename(fooid, "foo-new"); - REQUIRE(m.name(fooid) == "foo-new"); - REQUIRE(m.lookup("foo") == ts::Metrics::NOT_FOUND); - REQUIRE(m.lookup("foo-new") == fooid); - } - SECTION("lookup") { auto nm = m.lookup("notametric"); @@ -608,35 +579,131 @@ TEST_CASE("Metrics blob growth boundary", "[libtsapi][Metrics]") REQUIRE(std::adjacent_find(sorted_ptrs.begin(), sorted_ptrs.end()) == sorted_ptrs.end()); } -TEST_CASE("Metrics span lands exactly on a blob boundary", "[libtsapi][Metrics]") +TEST_CASE("Metrics malformed id offsets resolve to bad_id", "[libtsapi][Metrics]") +{ + // An id's offset field is 16 bits but a real offset is below MAX_SIZE, so a malformed one must + // not index past a blob's arrays. Filling past one blob puts the ids below in earlier blobs, + // where they pack under the bound and only the MAX_SIZE test rejects them. + auto &h = Metrics::hidden_instance(); + + for (int i = 0; i < Metrics::MAX_SIZE + 8; ++i) { + REQUIRE(Metrics::Counter::createHiddenPtr("f1.fill." + std::to_string(i)) != nullptr); + } + + auto const *bad = h.lookup(Metrics::IdType{0}); // the reserved bad_id slot + REQUIRE(bad != nullptr); + + for (Metrics::IdType id : {Metrics::IdType{0x0000FFFF}, Metrics::IdType{0x00000400}, Metrics::IdType{0x0001FFFF}}) { + REQUIRE(h.valid(id) == false); + REQUIRE(h.lookup(id) == bad); + REQUIRE(h.name(id) == h.name(Metrics::IdType{0})); + } +} + +TEST_CASE("Metrics id lookup is safe against concurrent creation", "[libtsapi][Metrics]") { - // A span has to be contiguous, so createSpan(MAX_SIZE) always starts a fresh blob and then fills - // it completely, whatever the current offset was. That makes this the one span size that reaches - // the boundary case deterministically: the offset ends up at MAX_SIZE, and unlike create(), - // createSpan used not to grow a new blob afterwards. The next create() then indexed one past the - // end of the blob's name array, and end() became an id that iterator::next() can never reach - // because it wraps at ++offset == MAX_SIZE. + // The id based read paths take no lock, so resolving an id races a concurrent create. Run both + // sides at once, over enough metrics to cross several blob boundaries. Under the tsan preset a + // non-atomic allocation counter reports a data race here; relaxing the memory orders does not, + // since atomics are race free at any ordering. // - // createSpan only ever targets the published store, so this necessarily allocates there. - Metrics::IdType span_id = Metrics::NOT_FOUND; - auto span = Metrics::Counter::createSpan(Metrics::MAX_SIZE, &span_id); - - REQUIRE(span.size() == Metrics::MAX_SIZE); - REQUIRE(span_id != Metrics::NOT_FOUND); - REQUIRE(span_id != 0); // 0 is the reserved bad_id, returned only when the store cannot grow. - - // The store must still be usable, and the new metric must be a real, resolvable entry rather - // than something written past the end of a blob. - auto p = Metrics::Counter::createPtr("span.boundary.after"); - REQUIRE(p != nullptr); - - auto &m = Metrics::instance(); - auto id = m.lookup("span.boundary.after"); - REQUIRE(id != Metrics::NOT_FOUND); - REQUIRE(m.valid(id)); - - // And it must behave like any other metric. - Metrics::Counter::increment(p, 7); - REQUIRE(Metrics::Counter::load(p) == 7); - REQUIRE(Metrics::Counter::createPtr("span.boundary.after") == p); + // Readers take ids from what the writer has registered rather than counting integers: an id packs + // the blob index above the offset, so consecutive integers only ever name the first blob. The + // store is relaxed, so a reader can pick up an id whose slot is not published yet, which is the + // case of interest. + constexpr int N_READERS = 4; + constexpr int N_CREATE = Metrics::MAX_SIZE * 2 + 64; + + auto &h = Metrics::hidden_instance(); + std::atomic stop{false}; + std::atomic ready{0}; + std::atomic mismatches{0}; + std::atomic resolved{0}; + + std::vector> created(N_CREATE); + + for (auto &c : created) { + c.store(Metrics::NOT_FOUND, std::memory_order_relaxed); + } + + // A clamped lookup resolves to the bad_id slot, whose name is not empty, so only the expected + // name detects one. + std::vector names; + + names.reserve(N_CREATE); + for (int i = 0; i < N_CREATE; ++i) { + names.push_back("pub.order." + std::to_string(i)); + } + + std::vector readers; + + for (int t = 0; t < N_READERS; ++t) { + readers.emplace_back([&]() { + ready.fetch_add(1, std::memory_order_release); + + while (!stop.load(std::memory_order_relaxed)) { + for (int i = 0; i < N_CREATE; ++i) { + auto const id = created[i].load(std::memory_order_relaxed); + + if (id == Metrics::NOT_FOUND || !h.valid(id)) { + continue; + } + + // valid() accepted the id, so lookup() must return that metric and not clamp. + std::string_view name; + Metrics::MetricType type; + auto *m = h.lookup(id, &name, &type); + + if (m == nullptr || name != names[i]) { + mismatches.fetch_add(1, std::memory_order_relaxed); + } + + // Published as it happens so the writer can wait for one. + resolved.fetch_add(1, std::memory_order_relaxed); + } + } + }); + } + + // Readers must be in their loops before the writer starts, or nothing races. + while (ready.load(std::memory_order_acquire) < N_READERS) { + std::this_thread::yield(); + } + + for (int i = 0; i < N_CREATE; ++i) { + REQUIRE(Metrics::Counter::createHiddenPtr(names[i]) != nullptr); + created[i].store(h.lookup(names[i]), std::memory_order_relaxed); + } + + // Being in the loop is not doing work: on one CPU the writer can finish first and every reader + // would then see stop. Wait for a real resolution so the check below cannot pass vacuously. + while (resolved.load(std::memory_order_relaxed) == 0) { + std::this_thread::yield(); + } + + stop.store(true, std::memory_order_relaxed); + for (auto &r : readers) { + r.join(); + } + + CHECK(mismatches.load() == 0); + CHECK(resolved.load() > 0); + + auto lo = std::numeric_limits::max(); + auto hi = std::numeric_limits::min(); + + for (int i = 0; i < N_CREATE; ++i) { + auto const id = h.lookup(names[i]); + + REQUIRE(id != Metrics::NOT_FOUND); + REQUIRE(h.valid(id)); + REQUIRE(h.name(id) == names[i]); + + lo = std::min(lo, id); + hi = std::max(hi, id); + } + + // More metrics than fit in one blob, so the ids must span blobs. A spread no wider than a blob + // would mean the sweep above never left the first one. + REQUIRE(hi - lo > Metrics::MAX_SIZE); } diff --git a/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py b/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py new file mode 100644 index 00000000000..0cb1d443281 --- /dev/null +++ b/tests/gold_tests/jsonrpc/config_reload_directive_cli.test.py @@ -0,0 +1,195 @@ +''' +Verify traffic_ctl command line parsing for the reload options that take a +variable number of values, --directive (-D) and --data (-d). + +Options declared with MORE_THAN_ZERO_ARG_N used to consume every remaining +token, so any option written after -D was silently swallowed as a directive +value and never parsed. -D therefore had to be the last option, and -D could +not be combined with -d. Once collection stops at the following option, the +option can be written more than once, and each occurrence has to keep the +values it collected rather than replace the ones before it. These runs assert +on the JSONRPC request that traffic_ctl builds (printed by -f rpc), because the +subject under test is the command line parsing rather than the server side +handling of the reload. +''' +# 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. + +Test.Summary = 'Verify traffic_ctl -D/-d argument parsing for config reload' +Test.ContinueOnFail = True + +ts = Test.MakeATSProcess("ts") +ts.StartupTimeout = 30 + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'rpc|config.reload', +}) + +ts.Disk.ip_allow_yaml.AddLines([ + 'ip_allow:', + '- apply: in', + ' ip_addrs: 0/0', + ' action: allow', + ' methods: ALL', +]) + +# ============================================================================ +# Test 1: an option written after -D keeps its own argument +# ============================================================================ +tr = Test.AddTestRun("Option after -D is not consumed as a directive value") +tr.Processes.Default.StartBefore(ts) +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=foo -t cli_token_1 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"token": "cli_token_1"', "-t must survive after -D") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"id": "foo"', "the directive must still be parsed") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 2: several directives, then an option +# ============================================================================ +tr = Test.AddTestRun("Multiple directives followed by an option") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=1 sni.id=2 -t cli_token_2 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"token": "cli_token_2"', "-t must survive after -D") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "first directive key must be present") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "second directive key must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 3: -D combined with -d, which the parser previously made impossible +# ============================================================================ +tr = Test.AddTestRun("-D can be combined with -d") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=foo -d 'ip_allow: {rules: [x]}' -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"rules"', "inline content from -d must be present") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"_reload"', "directives from -D must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 4: --directive=value keeps a value that itself contains '=' +# ============================================================================ +tr = Test.AddTestRun("--directive=value preserves embedded equal signs") +tr.Processes.Default.Command = "traffic_ctl config reload --directive=ip_allow.id=foo -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"id": "foo"', "the whole value must reach the request") +tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression("Invalid directive format", "the value must parse cleanly") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 5: "--" ends option recognition, so the value is taken literally and +# then rejected by the directive format check +# ============================================================================ +tr = Test.AddTestRun("A value after -- is taken literally") +tr.Processes.Default.Command = "traffic_ctl config reload -D -- -m" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "Invalid directive format '-m'", "-m must be treated as a directive value, not as --monitor") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 6: -D without any directive would silently reload every handler +# ============================================================================ +tr = Test.AddTestRun("-D requires at least one directive") +tr.Processes.Default.Command = "traffic_ctl config reload -D" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression("requires at least one", "-D must not be a silent no-op") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 7: same for -d, where a silent full reload is especially misleading +# ============================================================================ +tr = Test.AddTestRun("-d requires content") +tr.Processes.Default.Command = "traffic_ctl config reload -d" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression("requires content", "-d must not be a silent no-op") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 8: a repeated -D keeps the directives of every occurrence +# ============================================================================ +tr = Test.AddTestRun("A repeated -D accumulates its directives") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=1 -D sni.id=2 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "the first occurrence must survive the second") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "the second occurrence must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 9: repeating the option is how a directive is written after another +# option, since collection stops at the option rather than at the value +# ============================================================================ +tr = Test.AddTestRun("A repeated -D survives an option written between the two") +tr.Processes.Default.Command = "traffic_ctl config reload -D ip_allow.id=1 -t cli_token_8 -D sni.id=2 -f rpc" +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"token": "cli_token_8"', "-t must keep its own value") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "the directive before -t must survive") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "the directive after -t must be parsed") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 10: the documented multi source reload, where dropping one -d would +# leave its handler out of the reload without reporting anything +# ============================================================================ +tr = Test.AddTestRun("A repeated -d merges every source") +tr.Processes.Default.Command = ("traffic_ctl config reload -d 'ip_allow: {rules: [x]}' -d 'sni: {rules: [y]}' -f rpc") +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"ip_allow"', "content from the first -d must be present") +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression('"sni"', "content from the second -d must be present") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 11: an empty -d token is no content. The token survives the argument +# count, so without the content check the request degrades to a full reload +# ============================================================================ +tr = Test.AddTestRun("-d with an empty argument is reported as empty") +tr.Processes.Default.Command = "traffic_ctl config reload -d ''" +# autest's shell detection indexes arg[0], so an empty argument needs the shell path. +tr.Processes.Default.ForceUseShell = True +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "received an empty value", "an empty -d must be reported as empty, not as a missing argument") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 12: same for -D +# ============================================================================ +tr = Test.AddTestRun("-D with an empty argument is reported as empty") +tr.Processes.Default.Command = "traffic_ctl config reload -D ''" +tr.Processes.Default.ForceUseShell = True +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "received an empty value", "an empty -D must be reported as empty, not as a missing argument") +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 13: an empty token is refused even next to a real one. This is what an +# unset variable in a script written with several -d looks like, and accepting +# it would reload fewer configs than were asked for without saying so +# ============================================================================ +tr = Test.AddTestRun("An empty -d token is refused next to a real one") +tr.Processes.Default.Command = ("traffic_ctl config reload -d '' -d 'ip_allow: {rules: [x]}'") +tr.Processes.Default.ForceUseShell = True +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 2 +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + "received an empty value", "a partial reload must not happen silently") +tr.StillRunningAfter = ts diff --git a/tests/gold_tests/jsonrpc/config_reload_rpc.test.py b/tests/gold_tests/jsonrpc/config_reload_rpc.test.py index bf5f3265f6d..55d38103452 100644 --- a/tests/gold_tests/jsonrpc/config_reload_rpc.test.py +++ b/tests/gold_tests/jsonrpc/config_reload_rpc.test.py @@ -395,3 +395,97 @@ def validate_large_config(resp: Response): tr.Processes.Default.Streams.stdout = Testers.CustomJSONRPCResponse(validate_large_config) tr.StillRunningAfter = ts + +# ============================================================================ +# Test 11: Reload directive for registered FileOnly config (sni) +# Directive-only request — sni is FileOnly, so the RPC handler rejects with 6011. +# Verifies the _reload structure is handled gracefully through the RPC stack. +# ============================================================================ +tr = Test.AddTestRun("Reload directive for FileOnly config (sni)") +tr.DelayStart = 2 +tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload(configs={"sni": {"_reload": {"fqdn": "*.example.com"}}})) + + +def validate_directive_fileonly(resp: Response): + '''sni is FileOnly — directive-only request rejected with 6011''' + result = resp.result + errors = result.get('errors', []) + + if not errors: + return (False, f"Expected rejection for FileOnly config, got: {result}") + + error_str = str(errors) + if '6011' in error_str: + return (True, f"Directive-only correctly rejected for FileOnly config: {errors}") + return (False, f"Expected error 6011, got: {errors}") + + +tr.Processes.Default.Streams.stdout = Testers.CustomJSONRPCResponse(validate_directive_fileonly) +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 12: Reload directive for unregistered config (virtualhost) +# virtualhost is not registered yet — should get 6010. +# This is the intended use case once the virtualhost handler is registered. +# ============================================================================ +tr = Test.AddTestRun("Reload directive for unregistered config (virtualhost)") +tr.DelayStart = 2 +tr.AddJsonRPCClientRequest(ts, Request.admin_config_reload(configs={"virtualhost": {"_reload": {"id": "myhost.example.com"}}})) + + +def validate_directive_unregistered(resp: Response): + '''virtualhost is not registered — rejected with 6010''' + result = resp.result + errors = result.get('errors', []) + + if not errors: + return (False, f"Expected error for unregistered config, got: {result}") + + error_str = str(errors) + if '6010' in error_str or 'not registered' in error_str: + return (True, f"Directive for unregistered config rejected: {errors}") + return (False, f"Expected error 6010, got: {errors}") + + +tr.Processes.Default.Streams.stdout = Testers.CustomJSONRPCResponse(validate_directive_unregistered) +tr.StillRunningAfter = ts + +# ============================================================================ +# Test 13: Directives mixed with content for FileOnly config (ip_allow) +# _reload directives alongside actual config content — still rejected with 6011. +# ============================================================================ +tr = Test.AddTestRun("Directives mixed with content for FileOnly config") +tr.DelayStart = 2 +tr.AddJsonRPCClientRequest( + ts, + Request.admin_config_reload( + configs={ + "ip_allow": { + "_reload": { + "validate_only": "true" + }, + "rules": [{ + "apply": "in", + "ip_addrs": "0/0", + "action": "allow" + }] + } + })) + + +def validate_directive_mixed(resp: Response): + '''ip_allow is FileOnly — mixed directive+content rejected with 6011''' + result = resp.result + errors = result.get('errors', []) + + if not errors: + return (False, f"Expected rejection, got: {result}") + + error_str = str(errors) + if '6011' in error_str: + return (True, f"Mixed directive+content correctly rejected: {errors}") + return (False, f"Expected error 6011, got: {errors}") + + +tr.Processes.Default.Streams.stdout = Testers.CustomJSONRPCResponse(validate_directive_mixed) +tr.StillRunningAfter = ts diff --git a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py index 3d2766a4add..0191bc77336 100644 --- a/tests/gold_tests/pluginTest/cert_update/cert_update.test.py +++ b/tests/gold_tests/pluginTest/cert_update/cert_update.test.py @@ -26,7 +26,7 @@ Test.SkipIf(Condition.CurlUsingUnixDomainSocket()) Test.SkipUnless( Condition.HasProgram("openssl", "Openssl need to be installed on system for this test to work"), - Condition.PluginExists('cert_update.so')) + Condition.PluginExists('cert_update.so'), Condition.PluginExists('conf_remap.so')) # Set up origin server server = Test.MakeOriginServer("server") @@ -54,6 +54,8 @@ 'proxy.config.ssl.server.private_key.path': '{0}'.format(ts.Variables.SSLDir), 'proxy.config.ssl.client.cert.path': '{0}'.format(ts.Variables.SSLDir), 'proxy.config.ssl.client.private_key.path': '{0}'.format(ts.Variables.SSLDir), + 'proxy.config.ssl.client.CA.cert.path': '{0}'.format(ts.Variables.SSLDir), + 'proxy.config.ssl.client.verify.server.policy': 'PERMISSIVE', 'proxy.config.url_remap.pristine_host_hdr': 1 }) @@ -62,6 +64,14 @@ ts.Disk.remap_config.AddLines( [ 'map https://bar.com http://127.0.0.1:{0}'.format(server.Variables.Port), + 'map https://foo.com/override-ca https://127.0.0.1:{0} @plugin=conf_remap.so ' + '@pparam=proxy.config.ssl.client.cert.filename=client1.pem ' + '@pparam=proxy.config.ssl.client.CA.cert.filename=server1.pem'.format(ts.Variables.s_server_port), + # This CA configuration is only used after the certificate is updated so + # that its client context is created from scratch post-update. + 'map https://foo.com/late-ca https://127.0.0.1:{0} @plugin=conf_remap.so ' + '@pparam=proxy.config.ssl.client.cert.filename=client1.pem ' + '@pparam=proxy.config.ssl.client.CA.cert.filename=server2.pem'.format(ts.Variables.s_server_port), 'map https://foo.com https://127.0.0.1:{0}'.format(ts.Variables.s_server_port), ]) @@ -90,7 +100,8 @@ tr.Processes.Default.Env = ts.Env tr.Processes.Default.Command = ( '{0}/traffic_ctl plugin msg cert_update.server {1}/server2.pem'.format(ts.Variables.BINDIR, ts.Variables.SSLDir)) -ts.Disk.traffic_out.Content = "gold/update.gold" +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "Successfully updated server cert", "The server certificate context should be updated") ts.StillRunningAfter = server # Server-Cert-After @@ -104,7 +115,7 @@ ts.StillRunningAfter = server # Client-Cert-Pre -# s_server should see client (Traffic Server) as alice.com +# s_server should see client (Traffic Server) as alice.com with the default CA configuration. tr = Test.AddTestRun("Client-Cert-Pre") s_server = tr.Processes.Process( "s_server", "openssl s_server -www -key {0}/server1.pem -cert {0}/server1.pem -accept {1} -Verify 1 -msg".format( @@ -112,7 +123,23 @@ s_server.Ready = When.PortReady(ts.Variables.s_server_port) tr.MakeCurlCommand('--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{}'.format(ts.Variables.ssl_port), ts=ts) tr.Processes.Default.StartBefore(s_server) -s_server.Streams.all = "gold/client-cert-pre.gold" +s_server.Streams.All = Testers.ContainsExpression( + "alice.com", "The default CA context should initially use the original client certificate") +tr.Processes.Default.ReturnCode = 0 +ts.StillRunningAfter = server + +# Client-Cert-Pre-CA-Override +# s_server should also see alice.com with the overridden CA configuration. +tr = Test.AddTestRun("Client-Cert-Pre-CA-Override") +s_server = tr.Processes.Process( + "s_server", "openssl s_server -www -key {0}/server1.pem -cert {0}/server1.pem -accept {1} -Verify 1 -msg".format( + ts.Variables.SSLDir, ts.Variables.s_server_port)) +s_server.Ready = When.PortReady(ts.Variables.s_server_port) +tr.MakeCurlCommand( + '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{}/override-ca'.format(ts.Variables.ssl_port), ts=ts) +tr.Processes.Default.StartBefore(s_server) +s_server.Streams.All = Testers.ContainsExpression( + "alice.com", "The CA override context should initially use the original client certificate") tr.Processes.Default.ReturnCode = 0 ts.StillRunningAfter = server @@ -122,7 +149,10 @@ tr.Processes.Default.Command = ( 'mv {0}/client2.pem {0}/client1.pem && {1}/traffic_ctl plugin msg cert_update.client {0}/client1.pem'.format( ts.Variables.SSLDir, ts.Variables.BINDIR)) -ts.Disk.traffic_out.Content = "gold/update.gold" +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "Successfully updated client cert", "The client certificate context should be updated") +ts.Disk.traffic_out.Content += Testers.ExcludesExpression( + "Failed to update client cert", "The client certificate context update should not fail") ts.StillRunningAfter = server # Client-Cert-After @@ -137,6 +167,39 @@ tr.MakeCurlCommand( '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{0}'.format(ts.Variables.ssl_port), ts=ts) tr.Processes.Default.StartBefore(s_server) -s_server.Streams.all = "gold/client-cert-after.gold" +s_server.Streams.All = Testers.ContainsExpression( + "bob.com", "The next outbound connection should use the replacement client certificate") +tr.Processes.Default.ReturnCode = 0 +ts.StillRunningAfter = server + +# Verify that the context under the overridden CA configuration was also updated. +tr = Test.AddTestRun("Client-Cert-After-CA-Override") +s_server = tr.Processes.Process( + "s_server", "openssl s_server -www -key {0}/server1.pem -cert {0}/server1.pem -accept {1} -Verify 1 -msg".format( + ts.Variables.SSLDir, ts.Variables.s_server_port)) +s_server.Ready = When.PortReady(ts.Variables.s_server_port) +tr.Processes.Default.Env = ts.Env +tr.MakeCurlCommand( + '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{0}/override-ca'.format(ts.Variables.ssl_port), ts=ts) +tr.Processes.Default.StartBefore(s_server) +s_server.Streams.All = Testers.ContainsExpression("bob.com", "The client certificate should be updated for every CA configuration") +tr.Processes.Default.ReturnCode = 0 +ts.StillRunningAfter = server + +# Client-Cert-After-Late-CA +# The /late-ca mapping has not been used yet, so its client context is built +# after the update. It must be built from the new certificate file rather than +# from the certificate data cached before the update. +tr = Test.AddTestRun("Client-Cert-After-Late-CA") +s_server = tr.Processes.Process( + "s_server", "openssl s_server -www -key {0}/server1.pem -cert {0}/server1.pem -accept {1} -Verify 1 -msg".format( + ts.Variables.SSLDir, ts.Variables.s_server_port)) +s_server.Ready = When.PortReady(ts.Variables.s_server_port) +tr.Processes.Default.Env = ts.Env +tr.MakeCurlCommand( + '--verbose --insecure --ipv4 --header "Host: foo.com" https://localhost:{0}/late-ca'.format(ts.Variables.ssl_port), ts=ts) +tr.Processes.Default.StartBefore(s_server) +s_server.Streams.All = Testers.ContainsExpression( + "bob.com", "A context created after the update should not use the cached pre-update certificate") tr.Processes.Default.ReturnCode = 0 ts.StillRunningAfter = server diff --git a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold b/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold deleted file mode 100644 index fef60f68d27..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-after.gold +++ /dev/null @@ -1 +0,0 @@ -``bob.com`` \ No newline at end of file diff --git a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold b/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold deleted file mode 100644 index 6a94425920f..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/client-cert-pre.gold +++ /dev/null @@ -1 +0,0 @@ -``alice.com`` \ No newline at end of file diff --git a/tests/gold_tests/pluginTest/cert_update/gold/update.gold b/tests/gold_tests/pluginTest/cert_update/gold/update.gold deleted file mode 100644 index 4160bb7dbf7..00000000000 --- a/tests/gold_tests/pluginTest/cert_update/gold/update.gold +++ /dev/null @@ -1,3 +0,0 @@ -`` -``Successfully updated`` -`` \ No newline at end of file diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py index 573de3e7730..ed6abcc6a34 100644 --- a/tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit.test.py @@ -21,6 +21,7 @@ Test rate_limit plugin: concurrent limit enforcement, queue drain, and independent limiters. ''' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) Test.ContinueOnFail = True server = Test.MakeOriginServer("server", delay=3) diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py index ba6003264d1..c22e064020c 100644 --- a/tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_iprep.test.py @@ -28,6 +28,7 @@ Test rate_limit ip-rep initialization: reserve() vs resize() regression (Finding #108). ''' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) Test.ContinueOnFail = True server = Test.MakeOriginServer("server") diff --git a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py index 5389bde7ed8..24be86bd77a 100644 --- a/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py +++ b/tests/gold_tests/pluginTest/rate_limit/rate_limit_sni.test.py @@ -27,6 +27,7 @@ Test rate_limit SNI queue expiry: active counter underflow regression (Finding #109). ''' +Test.SkipUnless(Condition.PluginExists('rate_limit.so')) Test.ContinueOnFail = True server = Test.MakeOriginServer("server", delay=4) diff --git a/tests/gold_tests/pluginTest/regex_remap/long_query.conf b/tests/gold_tests/pluginTest/regex_remap/long_query.conf new file mode 100644 index 00000000000..d151b5b2541 --- /dev/null +++ b/tests/gold_tests/pluginTest/regex_remap/long_query.conf @@ -0,0 +1,20 @@ +# 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. + +# Match a token within the query and preserve both capture groups. +^/cms(\?.*)TOKEN(.*)$ https://redirect.example/cms$1TOKEN$2 @status=302 +# Make a skipped rule observable without contacting an origin. +^/cms.*$ https://fallback.example/ @status=307 diff --git a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py index 91d4cb7ab3a..c6c30830127 100644 --- a/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py +++ b/tests/gold_tests/pluginTest/regex_remap/regex_remap.test.py @@ -45,6 +45,13 @@ # Define ATS and configure ts = Test.MakeATSProcess("ts", enable_cache=False) +# These two rules deliberately exercise resource limits. Replace the blanket +# error exclusion once, then append independent checks for each rule below. +ts.Disk.diags_log.Content = Testers.ExcludesExpression( + r'ERROR: (?!\[regex_remap\] Bad regular expression result ' + r'(?:-(?:46|47|53|63) .* from "\^/alpha/bravo/|-47 .* from "\^/match_limit/))', + "Only the deliberate resource-limit errors are allowed") + testName = "regex_remap" regex_remap_conf_path = os.path.join(ts.Variables.CONFIGDIR, 'regex_remap.conf') @@ -116,26 +123,29 @@ tr.Processes.Default.Streams.stdout = "gold/regex_remap_simple.gold" tr.StillRunningAfter = ts -# 3 Test - Match limit test 0 -tr = Test.AddTestRun("match limit 0") +# 3 Test - Preserve the original crash guard from #5762. This request must +# survive resource exhaustion without redirecting, regardless of which matching +# resource limit is reached (JIT stack, match work, depth, or heap). +tr = Test.AddTestRun("resource exhaustion does not crash ATS") creq = replay_txns[1]['client-request'] -tr.MakeCurlCommand(curl_and_args + \ - '--header "uuid: {}" '.format(creq["headers"]["fields"][1][1]) + '"{}"'.format(creq["url"]), ts=ts) +tr.MakeCurlCommand(curl_and_args + f"--header 'uuid: {creq['headers']['fields'][1][1]}' '{creq['url']}'", ts=ts) tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Streams.stdout = "gold/regex_remap_crash.gold" -ts.Disk.diags_log.Content = Testers.ContainsExpression( - 'ERROR: .regex_remap. Bad regular expression result -47', "Match limit exceeded") +ts.Disk.diags_log.Content += Testers.ContainsExpression( + r'ERROR: \[regex_remap\] Bad regular expression result -(?:46|47|53|63).*"\^/alpha/bravo/', + "The crash-guard rule must report resource exhaustion") tr.StillRunningAfter = ts -# 4 Test - Match limit test 1 -tr = Test.AddTestRun("match limit 1") +# 4 Test - The nested quantifiers must exceed PCRE2's default matching-work limit. +tr = Test.AddTestRun("excessive backtracking reaches the match limit") creq = replay_txns[2]['client-request'] tr.MakeCurlCommand(curl_and_args + \ '--header "uuid: {}" '.format(creq["headers"]["fields"][1][1]) + '"{}"'.format(creq["url"]), ts=ts) tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Streams.stdout = "gold/regex_remap_crash.gold" -ts.Disk.diags_log.Content = Testers.ContainsExpression( - 'ERROR: .regex_remap. Bad regular expression result -47', "Match limit exceeded") +ts.Disk.diags_log.Content += Testers.ContainsExpression( + r'ERROR: \[regex_remap\] Bad regular expression result -47.*\^/match_limit/', + "The excessive-backtracking rule must reach the match limit") tr.StillRunningAfter = ts diff --git a/tests/gold_tests/pluginTest/regex_remap/regex_remap_long_query.test.py b/tests/gold_tests/pluginTest/regex_remap/regex_remap_long_query.test.py new file mode 100644 index 00000000000..b430530da4e --- /dev/null +++ b/tests/gold_tests/pluginTest/regex_remap/regex_remap_long_query.test.py @@ -0,0 +1,19 @@ +# 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. + +Test.Summary = 'Verify regex_remap redirects with long query strings.' +Test.SkipUnless(Condition.PluginExists('regex_remap.so')) +Test.ATSReplayTest(replay_file='replay/long_query.replay.yaml') diff --git a/tests/gold_tests/pluginTest/regex_remap/replay/long_query.replay.yaml b/tests/gold_tests/pluginTest/regex_remap/replay/long_query.replay.yaml new file mode 100644 index 00000000000..2f6092ae426 --- /dev/null +++ b/tests/gold_tests/pluginTest/regex_remap/replay/long_query.replay.yaml @@ -0,0 +1,230 @@ +# 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. + +meta: + version: "1.0" + +autest: + description: 'Valid long query strings must not exhaust the match-work limit' + server: + name: 'server' + client: + name: 'client' + ats: + name: 'ts' + copy_to_config_dir: + - 'long_query.conf' + remap_config: + - from: 'http://example.com/' + to: 'http://127.0.0.1:{SERVER_HTTP_PORT}/' + plugins: + - name: 'regex_remap.so' + args: ['long_query.conf'] + log_validation: + diags_log: + excludes: + - expression: 'Bad regular expression result' + description: 'Simple query matching must not report any regex error' + +sessions: +- transactions: + - client-request: + method: GET + version: "1.1" + url: "/cms?partner=TOKEN&x=abc" + headers: + fields: + - [Host, example.com] + - [uuid, short-match] + proxy-request: + expect: absent + server-response: + status: 500 + headers: + fields: + - [Content-Length, "0"] + proxy-response: + status: 302 + headers: + fields: + - [Location, {value: "https://redirect.example/cms?partner=TOKEN&x=abc", as: equal}] + + # Regression for #13651: the 2021-byte subject exceeds the old 1750-work + # limit. Keep the 2000-byte suffix: shortening it can hide the regression. + # Only this case requires extensive backtracking; the other requests are + # controls for token placement and absence, not additional stress cases. + - client-request: + method: GET + version: "1.1" + url: "/cms?partner=TOKEN&x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaa" + headers: + fields: + - [Host, example.com] + - [uuid, long-match] + proxy-request: + expect: absent + server-response: + status: 500 + headers: + fields: + - [Content-Length, "0"] + proxy-response: + status: 302 + headers: + fields: + - [Location, {value: "https://redirect.example/cms?partner=TOKEN&x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", as: equal}] + + - client-request: + method: GET + version: "1.1" + url: "/cms?x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaa&partner=TOKEN" + headers: + fields: + - [Host, example.com] + - [uuid, token-at-end] + proxy-request: + expect: absent + server-response: + status: 500 + headers: + fields: + - [Content-Length, "0"] + proxy-response: + status: 302 + headers: + fields: + - [Location, {value: "https://redirect.example/cms?x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa&partner=TOKEN", as: equal}] + + - client-request: + method: GET + version: "1.1" + url: "/cms?partner=OTHER&x=aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\ + aaaaaaaaaaaaaaaaaaaaa" + headers: + fields: + - [Host, example.com] + - [uuid, no-token] + proxy-request: + expect: absent + server-response: + status: 500 + headers: + fields: + - [Content-Length, "0"] + proxy-response: + status: 307 + headers: + fields: + - [Location, {value: "https://fallback.example/", as: equal}] diff --git a/tests/gold_tests/pluginTest/regex_remap/replay/yts-2819.replay.json b/tests/gold_tests/pluginTest/regex_remap/replay/yts-2819.replay.json index 4361a9800ff..d492eb0ba9c 100644 --- a/tests/gold_tests/pluginTest/regex_remap/replay/yts-2819.replay.json +++ b/tests/gold_tests/pluginTest/regex_remap/replay/yts-2819.replay.json @@ -163,7 +163,7 @@ "version": "1.1", "scheme": "http", "method": "GET", - "url": "http://example.one/match_limit/aaaaaaaaaaaaaaaaaaaf", + "url": "http://example.one/match_limit/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaf", "headers": { "fields": [ [ diff --git a/tests/gold_tests/records/traffic_ctl_cold_config.test.py b/tests/gold_tests/records/traffic_ctl_cold_config.test.py index c27925baf2b..f605a4deac0 100644 --- a/tests/gold_tests/records/traffic_ctl_cold_config.test.py +++ b/tests/gold_tests/records/traffic_ctl_cold_config.test.py @@ -92,3 +92,106 @@ tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Env = ts.Env tr.Disk.File(file).Content = 'gold/records.yaml.cold_test5.gold' + +# --cold takes at most one file name, so it does not consume the record names that follow +# it. Before that was the case it had to be written after them, which the runs above do. +records_file = os.path.join(ts.Variables.CONFIGDIR, "records.yaml") + +# 6 +tr = Test.AddTestRun("Get a value with the file name given before the record") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The record must still be parsed as a record') + +# 7 +tr = Test.AddTestRun("Get several values with the file name given before them") +tr.Processes.Default.Command = ( + f'traffic_ctl config get -c {records_file} ' + 'proxy.config.diags.debug.tags proxy.config.cache.limits.http.max_alts') +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The first record must be parsed as a record') +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.cache.limits.http.max_alts: 1', 'The second record must be parsed as a record') + +# 8 +tr = Test.AddTestRun("Get a value using the --cold=FILE form") +tr.Processes.Default.Command = f'traffic_ctl config get --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: http', 'The record must still be parsed as a record') + +# 9 +file = os.path.join(ts.Variables.CONFIGDIR, "new_records3.yaml") +tr = Test.AddTestRun("Set a value with the file name given before the record and the value") +tr.Processes.Default.Command = f'traffic_ctl config set -c {file} proxy.config.cache.limits.http.max_alts 3' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Env = ts.Env +tr.Disk.File(file).Content = 'gold/records.yaml.cold_test5.gold' + +# 10 +tr = Test.AddTestRun("--cold takes at most one file name, so repeating it is an error") +tr.Processes.Default.Command = f'traffic_ctl config get --cold={records_file} --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'A repeated --cold must be reported rather than the last one winning') + +# 11 +tr = Test.AddTestRun("A repeated --cold is an error in the space-separated form too") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} -c {records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'A repeated -c must be reported rather than the last one winning') + +# 12 +tr = Test.AddTestRun("Mixing the two --cold spellings cannot smuggle in a second file name") +tr.Processes.Default.Command = f'traffic_ctl config get -c {records_file} --cold={records_file} proxy.config.diags.debug.tags' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + 'at most one argument expected by --cold', 'The two forms must be counted together') + +# An empty file name reaches traffic_ctl when it is taken from a variable that is unset. The +# --cold=FILE form has always rejected it; the space-separated form used to fall through to the +# default records.yaml instead, so a run meant for another file read or wrote the live one. +# These runs go through "sh -c" because autest indexes the first character of every argument +# it splits, so an empty argument written straight into Command raises IndexError before the +# process starts. + +# 13 +tr = Test.AddTestRun("An empty file name is reported rather than taken as the default file") +tr.Processes.Default.Command = """sh -c 'traffic_ctl config get -c "" proxy.config.diags.debug.tags'""" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "missing argument for '-c'", 'An empty -c value must be reported, naming the option as written') + +# 14 +tr = Test.AddTestRun("An empty file name is reported before anything is written") +tr.Processes.Default.Command = """sh -c 'traffic_ctl config set -c "" proxy.config.cache.limits.http.max_alts 9'""" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "missing argument for '-c'", 'A set with an empty -c value must not fall back to the live records.yaml') + +# 15 +tr = Test.AddTestRun("The long spelling of an empty file name is reported as written") +tr.Processes.Default.Command = """sh -c 'traffic_ctl config get --cold "" proxy.config.diags.debug.tags'""" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "missing argument for '--cold'", 'The error must name the spelling the caller used') + +# 16 +tr = Test.AddTestRun("A bare -c before the record leaves set short of its own arguments") +tr.Processes.Default.Command = 'traffic_ctl config set -c proxy.config.cache.limits.http.max_alts 9' +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + r'2 argument\(s\) expected by set', 'set must report what it was left without, as the docs show') diff --git a/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py b/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py index f899f980708..30b781732b2 100644 --- a/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py +++ b/tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py @@ -78,3 +78,21 @@ tr.Processes.Default.Streams.All = Testers.ContainsExpression( "Option \'--append\' requires \'--tags\' to be specified", "Should show error that --append requires --tags") tr.StillRunningAfter = traffic_ctl._ts + +# Test 15: An option written where the tags are expected leaves them missing, rather than being +# applied as the tags themselves. +tr = Test.AddTestRun("test --tags followed by another option") +tr.Processes.Default.Env = traffic_ctl._ts.Env +tr.Processes.Default.Command = "traffic_ctl server debug enable --tags --append" +tr.Processes.Default.ReturnCode = 64 # EX_USAGE - command line usage error +tr.Processes.Default.Streams.All = Testers.ContainsExpression( + "1 argument\\(s\\) expected by tags", "Should report the tags as missing") +tr.StillRunningAfter = traffic_ctl._ts + +# Test 16: Tags that are shaped like an option are passed after "--". +tr = Test.AddTestRun("test tags shaped like an option") +tr.Processes.Default.Env = traffic_ctl._ts.Env +tr.Processes.Default.Command = "traffic_ctl server debug enable --tags -- -a" +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.Streams.stdout = Testers.ContainsExpression('tags »"-a"«', "The value after -- must be taken as the tags") +tr.StillRunningAfter = traffic_ctl._ts diff --git a/tests/proxy-verifier-checksum.txt b/tests/proxy-verifier-checksum.txt index 9c2ec8528be..90b31901ff1 100644 --- a/tests/proxy-verifier-checksum.txt +++ b/tests/proxy-verifier-checksum.txt @@ -1 +1 @@ -342286244d441329c12de1122520da6c86c581fd +5485a3cea4359e86458bf6d369dc18cdcfbd433d diff --git a/tests/proxy-verifier-version.txt b/tests/proxy-verifier-version.txt index 66cfae52b28..6d260c3af09 100644 --- a/tests/proxy-verifier-version.txt +++ b/tests/proxy-verifier-version.txt @@ -1 +1 @@ -v3.1.3 +v3.2.0 diff --git a/tools/benchmark/CMakeLists.txt b/tools/benchmark/CMakeLists.txt index e58e8dcf26b..ba3816cf6be 100644 --- a/tools/benchmark/CMakeLists.txt +++ b/tools/benchmark/CMakeLists.txt @@ -33,6 +33,9 @@ target_link_libraries(benchmark_ProxyAllocator PRIVATE Catch2::Catch2WithMain ts add_executable(benchmark_SharedMutex benchmark_SharedMutex.cc) target_link_libraries(benchmark_SharedMutex PRIVATE Catch2::Catch2 ts::tscore libswoc::libswoc) +add_executable(benchmark_Metrics benchmark_Metrics.cc) +target_link_libraries(benchmark_Metrics PRIVATE Catch2::Catch2 ts::tsutil libswoc::libswoc) + add_executable(benchmark_Random benchmark_Random.cc) target_link_libraries(benchmark_Random PRIVATE Catch2::Catch2WithMain ts::tscore) diff --git a/tools/benchmark/benchmark_Metrics.cc b/tools/benchmark/benchmark_Metrics.cc new file mode 100644 index 00000000000..e42b092a7f4 --- /dev/null +++ b/tools/benchmark/benchmark_Metrics.cc @@ -0,0 +1,190 @@ +/** @file + + Micro benchmark tool for ts::Metrics + + Metric values are lock free atomics; reaching one from an id is not. Four cases, scaled by thread + count: + + increment(id) valid() then lookup(id) then fetch_add, as TSStatIntIncrement does + increment(ptr) a bare fetch_add on a cached pointer, as core does + lookup(id) lock free id resolution + lookup(name) the same resolution through the mutex guarded name map + + increment(id) against increment(ptr) is what an id costs a plugin. lookup(id) against + lookup(name) isolates the mutex, and serves as a control: it must degrade with thread count, or + the harness is not loading the machine. + + @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. + */ + +#define CATCH_CONFIG_ENABLE_BENCHMARKING + +#include +#include +#include + +#include "tsutil/Metrics.h" + +#include +#include +#include +#include + +using ts::Metrics; + +namespace +{ +// Args +struct Conf { + int nthreads = 1; + int nops = 1000; + int nmetrics = 64; +}; + +Conf conf; + +/// The metrics every case operates on, created once. +struct Fixture { + std::vector ids; + std::vector ptrs; + std::vector names; + + Fixture() + { + auto &m = Metrics::instance(); + + ids.reserve(conf.nmetrics); + ptrs.reserve(conf.nmetrics); + names.reserve(conf.nmetrics); + + for (int i = 0; i < conf.nmetrics; ++i) { + names.push_back("benchmark.metrics." + std::to_string(i)); + + // Registers the name too, so the id and name lookups resolve to the same metric. + ptrs.push_back(Metrics::Counter::createPtr(names.back())); + ids.push_back(m.lookup(names.back())); + } + } +}; + +Fixture *fixture = nullptr; + +/// Run @a op on every thread, @c nops times each. The return value only defeats optimization. +template +int64_t +run(F &&op) +{ + std::vector threads; + std::atomic sink{0}; + + threads.reserve(conf.nthreads); + + for (int t = 0; t < conf.nthreads; ++t) { + threads.emplace_back([t, &sink, &op]() { + int64_t local = 0; + + for (int i = 0; i < conf.nops; ++i) { + // Stride per thread, or this measures cacheline ping-pong on one atomic. + local += op((t + i) % conf.nmetrics); + } + sink.fetch_add(local, std::memory_order_relaxed); + }); + } + + for (auto &th : threads) { + th.join(); + } + + return sink.load(); +} + +} // namespace + +TEST_CASE("Micro benchmark of ts::Metrics", "") +{ + auto &m = Metrics::instance(); + + SECTION("increment by id") + { + BENCHMARK("increment(id)") + { + return run([&m](int i) -> int64_t { + auto id = fixture->ids[i]; + + return m.valid(id) ? m.increment(id, 1) : 0; + }); + }; + } + + SECTION("increment by cached pointer") + { + // The floor: no resolution at all. + BENCHMARK("increment(ptr)") + { + return run([](int i) -> int64_t { + Metrics::Counter::increment(fixture->ptrs[i], 1); + + return 1; + }); + }; + } + + SECTION("lookup by id") + { + BENCHMARK("lookup(id)") + { + return run([&m](int i) -> int64_t { return m.lookup(fixture->ids[i]) != nullptr; }); + }; + } + + SECTION("lookup by name") + { + BENCHMARK("lookup(name)") + { + return run([&m](int i) -> int64_t { return m.lookup(fixture->names[i]) != Metrics::NOT_FOUND; }); + }; + } +} + +int +main(int argc, char *argv[]) +{ + Catch::Session session; + + using namespace Catch::Clara; + + // clang-format off + auto cli = session.cli() | + Opt(conf.nthreads, "")["--ts-nthreads"]("number of threads (default: 1)") | + Opt(conf.nops, "")["--ts-nops"]("operations per thread per run (default: 1000)") | + Opt(conf.nmetrics, "")["--ts-nmetrics"]("distinct metrics to spread across (default: 64)"); + // clang-format on + + session.cli(cli); + + int returnCode = session.applyCommandLine(argc, argv); + if (returnCode != 0) { + return returnCode; + } + + Fixture f; + fixture = &f; + + return session.run(); +}