-
Notifications
You must be signed in to change notification settings - Fork 845
traffic_ctl: Add config reset command #12752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Adds `traffic_ctl config reset` to reset configuration records to their default values. Supports both record name format (proxy.config.*) and YAML format (records.*) for path matching. There is no new ATS rpc handler needed it just use the existing API and checks for differences between current value and default value. This includes Autests. Examples: traffic_ctl config reset records traffic_ctl config reset proxy.config.diags.debug.enabled traffic_ctl config reset records.diags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new traffic_ctl config reset command that allows resetting configuration records to their default values. The implementation leverages existing RPC APIs by fetching records that match the provided path patterns, comparing current values against defaults, and using the existing set API to restore default values. The command supports both traditional record name format (proxy.config.*) and YAML format (records.*), with automatic conversion between the two.
Key changes:
- Implements
config_reset()method that uses regex-based record lookup to identify modified records and resets them to defaults - Adds helper function
yaml_to_record_name()to convert YAML-style paths to record name format - Refactors test utilities to use a cleaner inheritance model with shared
_finish()method and adds new validation helpers
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/traffic_ctl/CtrlCommands.cc | Implements core reset logic: lookup matching records, filter modified ones, and reset to defaults via existing set API |
| src/traffic_ctl/CtrlCommands.h | Adds RESET_STR constant and config_reset() method declaration |
| src/traffic_ctl/traffic_ctl.cc | Registers new "reset" subcommand with CLI parser |
| src/mgmt/rpc/handlers/config/Configuration.cc | Adds blank line for formatting consistency |
| tests/gold_tests/traffic_ctl/traffic_ctl_test_utils.py | Refactors test utilities with cleaner inheritance, adds validate_contains_all() and exec() methods, updates validate_with_text() to handle empty output |
| tests/gold_tests/traffic_ctl/traffic_ctl_config_output.test.py | Adds comprehensive test coverage for reset command including single record, partial path, all records, and YAML-style path scenarios |
| doc/appendices/command-line/traffic_ctl.en.rst | Documents reset command with examples showing both record name and YAML format usage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .. option:: reset PATH [PATH ...] | ||
|
|
||
| Reset configuration record(s) to their default values. The PATH argument is used as a | ||
| regex pattern to match against record names. Multiple paths at once can be provided. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really a regex pattern right? It's just prefix of the config. Can users put a full regex expression in the argument?
|
I like this idea but I wonder if it might be confusing that values return to default values as opposed to init values (what the user defined in files which got changed by traffic_ctl). When I think of reset, I think about returning values to the startup state, not the default state (as if I didn't configure anything). This might be a bit nitpicky, just wanted to share my thoughts. |
Let me add a bit of context: note the How this is useful? This actually came from a need I had when testing some cve bugs, a simple flow would be. 1 - I have a working ATS running, with my own config. Without this feature, you would need to copy/save/edit your records.yaml just to do the test, instead of just run reset. Thanks.
|
|
Ah I see. Thank you for providing that context and explanation! |
Adds
traffic_ctl config resetto reset configuration records to their default values. Supports both record name format (proxy.config.) and YAML format (records.) for path matching.There is no new ATS rpc handler needed, it just use the existing API and checks for differences between current value and default value. This can be used eventually to reset any config without changing the semantic, just use whatever config node you need(if able to) to change.
This includes Autests.
Examples: