Copilot commented on code in PR #12804: URL: https://github.com/apache/trafficserver/pull/12804#discussion_r2729606647
########## tests/gold_tests/traffic_ctl/traffic_ctl_server_debug.test.py: ########## @@ -0,0 +1,80 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +import sys + +# To include util classes +sys.path.insert(0, f'{Test.TestDirectory}') + +from traffic_ctl_test_utils import Make_traffic_ctl + +Test.Summary = ''' +Test traffic_ctl server debug enable/disable commands. +''' + +Test.ContinueOnFail = True + +records_yaml = ''' +diags: + debug: + enabled: 0 + tags: xyz +''' + +traffic_ctl = Make_traffic_ctl(Test, records_yaml) + +###### +# Test 1: Enable debug with tags +traffic_ctl.server().debug().enable(tags="http").exec() +# Test 2: Verify debug is enabled and tags are set +traffic_ctl.config().get("proxy.config.diags.debug.enabled").validate_with_text("proxy.config.diags.debug.enabled: 1") +# Test 3: Verify tags are set +traffic_ctl.config().get("proxy.config.diags.debug.tags").validate_with_text("proxy.config.diags.debug.tags: http") + +# Test 4: Disable debug +traffic_ctl.server().debug().disable().exec() +# Test 5: Verify debug is disabled +traffic_ctl.config().get("proxy.config.diags.debug.enabled").validate_with_text("proxy.config.diags.debug.enabled: 0") + +# Test 6: Enable debug with new tags (replace mode) +traffic_ctl.server().debug().enable(tags="cache").exec() +# Test 7: Verify tags are replaced +traffic_ctl.config().get("proxy.config.diags.debug.tags").validate_with_text("proxy.config.diags.debug.tags: cache") + +# Test 8: Enable debug with append mode - should combine with existing tags +traffic_ctl.server().debug().enable(tags="http", append=True).exec() +# Test 9: Verify tags are appended +traffic_ctl.config().get("proxy.config.diags.debug.tags").validate_with_text("proxy.config.diags.debug.tags: cache|http") + +# Test 10: Append another tag +traffic_ctl.server().debug().enable(tags="dns", append=True).exec() +# Test 11: Verify all tags are present +traffic_ctl.config().get("proxy.config.diags.debug.tags").validate_with_text("proxy.config.diags.debug.tags: cache|http|dns") Review Comment: The test expects tags in a specific order (cache|http|dns), but the current implementation simply concatenates tags without any guaranteed ordering. If the internal representation stores tags differently or if deduplication logic is added in the future, this test might become fragile. Consider using a more flexible validation approach that checks for the presence of all expected tags regardless of order, or document that tag order is guaranteed to be preserved. ```suggestion # Test 11: Verify all tags are present (order-independent) tr = Test.AddTestRun("verify all debug tags are present regardless of order") tr.Processes.Default.Env = traffic_ctl._ts.Env tr.Processes.Default.Command = "traffic_ctl config get proxy.config.diags.debug.tags" tr.Processes.Default.ReturnCode = 0 tr.Processes.Default.Streams.All = Testers.ContainsExpression( r"proxy.config.diags.debug.tags:\s*(?=.*cache)(?=.*http)(?=.*dns).*", "Should include cache, http, and dns tags regardless of order" ) tr.StillRunningAfter = traffic_ctl._ts ``` ########## src/traffic_ctl/CtrlCommands.cc: ########## @@ -695,12 +695,31 @@ ServerCommand::server_debug() { // Set ATS to enable or disable debug at runtime. const bool enable = get_parsed_arguments()->get(ENABLE_STR); + const bool append = get_parsed_arguments()->get(APPEND_STR); // If the following is not passed as options then the request will ignore them as default values // will be set. - const std::string tags = get_parsed_arguments()->get(TAGS_STR).value(); + std::string tags = get_parsed_arguments()->get(TAGS_STR).value(); const std::string client_ip = get_parsed_arguments()->get(CLIENT_IP_STR).value(); + // If append mode is enabled and tags are provided, fetch current tags and combine + if (append && !tags.empty()) { + shared::rpc::RecordLookupRequest lookup_request; + lookup_request.emplace_rec("proxy.config.diags.debug.tags", shared::rpc::NOT_REGEX, shared::rpc::CONFIG_REC_TYPES); + auto lookup_response = invoke_rpc(lookup_request); + + if (!lookup_response.is_error()) { + auto const &records = lookup_response.result.as<shared::rpc::RecordLookUpResponse>(); + if (!records.recordList.empty()) { + std::string current_tags = records.recordList[0].currentValue; + if (!current_tags.empty()) { + // Combine: current|new + tags = current_tags + "|" + tags; + } + } + } + } Review Comment: The append logic doesn't check for duplicate tags. If a user appends a tag that already exists in the current tags, it will be duplicated (e.g., "http|dns|http"). Consider adding deduplication logic to prevent duplicate tags in the combined string. This could be done by splitting the current and new tags, combining them into a set to remove duplicates, and then joining them back together. ########## src/tscore/ArgParser.cc: ########## @@ -574,6 +595,59 @@ ArgParser::Command::validate_mutex_groups(Arguments &ret) const } } +// Specify that the last added option requires another option +ArgParser::Command & +ArgParser::Command::with_required(std::string const &required_option) +{ + if (_last_added_option.empty()) { + std::cerr << "Error: with_required() must be called after add_option()" << std::endl; + ArgParser::do_exit(1); + } + + // Validate that required option exists + if (_option_list.find(required_option) == _option_list.end()) { + std::cerr << "Error: Required option '" << required_option << "' not found" << std::endl; + ArgParser::do_exit(1); + } + + _option_dependencies[_last_added_option].push_back(required_option); + + return *this; +} + +// Validate option dependencies +void +ArgParser::Command::validate_dependencies(Arguments &ret) const +{ + for (const auto &[dependent, required_list] : _option_dependencies) { + // Get the key for the dependent option + auto it = _option_list.find(dependent); + if (it == _option_list.end()) { + continue; + } + + const std::string &dep_key = it->second.key; + + // Check if dependent option was used + if (ret.get(dep_key)) { + // Dependent option was used, check all required options + for (const auto &required : required_list) { + auto req_it = _option_list.find(required); + if (req_it == _option_list.end()) { + continue; + } + + const std::string &req_key = req_it->second.key; + + if (!ret.get(req_key)) { + std::string error_msg = "Option '" + dependent + "' requires '" + required + "' to be specified"; Review Comment: The error message for dependency violations is inconsistent with other validation error messages. Mutex group validation errors (lines 574 and 586) prefix the message with "Error: ", but this dependency validation error does not. Consider adding the "Error: " prefix for consistency. ```suggestion std::string error_msg = "Error: Option '" + dependent + "' requires '" + required + "' to be specified"; ``` ########## src/traffic_ctl/CtrlCommands.cc: ########## @@ -695,12 +695,31 @@ ServerCommand::server_debug() { // Set ATS to enable or disable debug at runtime. const bool enable = get_parsed_arguments()->get(ENABLE_STR); + const bool append = get_parsed_arguments()->get(APPEND_STR); // If the following is not passed as options then the request will ignore them as default values // will be set. - const std::string tags = get_parsed_arguments()->get(TAGS_STR).value(); + std::string tags = get_parsed_arguments()->get(TAGS_STR).value(); const std::string client_ip = get_parsed_arguments()->get(CLIENT_IP_STR).value(); + // If append mode is enabled and tags are provided, fetch current tags and combine + if (append && !tags.empty()) { + shared::rpc::RecordLookupRequest lookup_request; + lookup_request.emplace_rec("proxy.config.diags.debug.tags", shared::rpc::NOT_REGEX, shared::rpc::CONFIG_REC_TYPES); + auto lookup_response = invoke_rpc(lookup_request); + + if (!lookup_response.is_error()) { + auto const &records = lookup_response.result.as<shared::rpc::RecordLookUpResponse>(); + if (!records.recordList.empty()) { + std::string current_tags = records.recordList[0].currentValue; + if (!current_tags.empty()) { + // Combine: current|new + tags = current_tags + "|" + tags; + } + } Review Comment: When the RPC lookup fails or returns an empty record list in append mode, the code silently falls back to using only the new tags. This might be confusing for users who expect append behavior. Consider logging a warning or providing feedback to the user that append mode couldn't fetch current tags and is falling back to replace mode. ```suggestion } } else if (_printer) { _printer->write_output( "Warning: append mode enabled, but no existing debug tags could be retrieved; using only new tags." ); } } else if (_printer) { _printer->write_output( "Warning: append mode enabled, but failed to fetch existing debug tags; using only new tags." ); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
