Conversation
Move ATS core off the remaining remap-only parsing path and route remap.config loading through the shared src/config remap parser. This lets traffic_ctl and traffic_server use the same conversion logic while leaving only runtime regex-mapping helpers in the remap loader. This builds off of the work to support remap.yaml here: apache#12997
bneradt
force-pushed
the
refactor-remapyaml-parsing
branch
from
5c17ee4 to
22db9c6
Compare
|
|
||
| constexpr swoc::Errata::Severity ERRATA_NOTE_SEV{static_cast<swoc::Errata::severity_type>(DL_Note)}; | ||
| constexpr swoc::Errata::Severity ERRATA_ERROR_SEV{static_cast<swoc::Errata::severity_type>(DL_Error)}; | ||
|
|
There was a problem hiding this comment.
I prefer YAML key names definition in the beginning.
| namespace config | ||
| { | ||
|
|
||
| using RemapConfig = YAML::Node; |
There was a problem hiding this comment.
Hmm, can we define some struct describes remap.yaml node tree? It'll help us understanding type of node - int, string, list, map.
| DbgCtl dbg_ctl_url_rewrite{"url_rewrite"}; | ||
|
|
||
| swoc::Errata | ||
| parse_yaml_volume(const YAML::Node &node, url_mapping *url_mapping) |
There was a problem hiding this comment.
I feel these parsers in this file should moved to config/remap.cc
There was a problem hiding this comment.
Pull request overview
This PR moves remap configuration parsing/conversion onto the shared src/config remap parser so traffic_ctl and traffic_server can reuse the same remap.config → remap.yaml normalization and reduce legacy-only parsing paths in core.
Changes:
- Add
traffic_ctl config convert remapand gold tests for legacy remap.config → remap.yaml conversion output. - Introduce shared
config::RemapParser/config::RemapMarshaller(src/config/remap.cc,include/config/remap.h) and corresponding unit tests. - Route
UrlRewrite::BuildTable()throughremap_parse_yaml()unconditionally, withremap_parse_yaml_bti()using the shared parser to accept legacy or YAML input.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/traffic_ctl/convert_remap/legacy_config/basic.config | Adds legacy remap.config input fixture for conversion tests. |
| tests/gold_tests/traffic_ctl/convert_remap/gold/basic.yaml | Adds expected remap.yaml output fixture for conversion tests. |
| tests/gold_tests/traffic_ctl/convert_remap/convert_remap.test.py | Adds gold test coverage for traffic_ctl config convert remap (stdout + file output). |
| src/traffic_ctl/traffic_ctl.cc | Registers the new traffic_ctl config convert remap subcommand. |
| src/traffic_ctl/ConvertConfigCommand.{h,cc} | Implements remap conversion using the shared config remap parser/marshaller. |
| src/config/remap.cc / include/config/remap.h | Adds shared remap parser/marshaller for legacy+YAML remap config. |
| src/config/unit_tests/test_remap.cc | Adds unit tests for legacy conversion, round-trip, and error reporting. |
| src/proxy/http/remap/RemapYamlConfig.cc | Switches YAML loader to use shared parser; adds YAML parsing for unique, tag, and volume. |
| src/proxy/http/remap/UrlRewrite.cc | Removes legacy parse path selection; always calls remap_parse_yaml(). |
| src/proxy/http/remap/RemapConfig.cc + include/proxy/http/remap/RemapConfig.h | Removes legacy remap.config parsing implementation and leaves runtime helpers / BUILD_TABLE_INFO cleanup. |
| src/proxy/http/remap/unit-tests/test_RemapRules*.cc | Updates remap unit tests to use the YAML parsing entry point and adds metadata coverage. |
| src/*/CMakeLists.txt | Wires new config module + tests into builds and links remap code to ts::config. |
| } else if (is_deactivate_filter_directive(directive)) { | ||
| entry["deactivate_filter"] = params[1]; | ||
| } else if (directive == ".include") { | ||
| entry["include"] = params[1]; |
There was a problem hiding this comment.
Legacy .include supports a list of file names (see remap.config docs), but this conversion only preserves params[1] and silently drops any additional include arguments. Consider emitting one include entry per provided path (i.e., loop params[1..] and push multiple include directives) so .include one.config two.config converts correctly.
| entry["include"] = params[1]; | |
| for (size_t idx = 1; idx < params.size(); ++idx) { | |
| YAML::Node include_entry{YAML::NodeType::Map}; | |
| include_entry["include"] = params[idx]; | |
| remap_entries.push_back(include_entry); | |
| } | |
| return errata; |
| bool parse_success = remap_parse_yaml(path, this); | ||
|
|
There was a problem hiding this comment.
BuildTable() now always routes through remap_parse_yaml(), which means legacy remap.config .include directives are handled by the YAML include path. That include path registers child files under ts::filename::REMAP_YAML, so changes to remap.config won't clear/update the registered children (FileManager only deletes children of the changed parent), potentially leaving stale include dependencies and causing spurious reloads. Consider registering included fragments under ts::filename::REMAP when the primary config being parsed is legacy remap.config (e.g., consult UrlRewrite::is_remap_yaml() when calling load_remap_file_cb).
Move ATS core off the remaining remap-only parsing path and route remap.config loading through the shared src/config remap parser. This lets traffic_ctl and traffic_server use the same conversion logic while leaving only runtime regex-mapping helpers in the remap loader.
This builds off of the work to support remap.yaml here: #12997