From 447753b2e94c6539bc017dee5e846d4f5de8d62d Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Thu, 26 Feb 2026 12:27:45 +0000 Subject: [PATCH 1/2] RecConfigOverrideFromEnvironment: return resolved value and override source Change the return type to std::pair so callers know whether a record was overridden and by whom (ENV or RUNROOT). When runroot is active, return the actual Layout path (e.g. Layout::bindir) instead of the old nullptr which caused undefined behaviour when converted to std::string. Replace the RecConfigOverrideFromRunroot() strcmp chain with a constexpr lookup table mapping record names to Layout members. All three callers now log the override source at debug level. Preserve nullptr for built-in defaults that are intentionally unset. Adds records_runroot_precedence autest. --- include/records/RecCore.h | 16 +- src/records/RecConfigParse.cc | 54 +++--- src/records/RecYAMLDecoder.cc | 9 +- src/records/RecordsConfigUtils.cc | 17 +- .../records_runroot_precedence.test.py | 167 ++++++++++++++++++ 5 files changed, 229 insertions(+), 34 deletions(-) create mode 100644 tests/gold_tests/records/records_runroot_precedence.test.py diff --git a/include/records/RecCore.h b/include/records/RecCore.h index d0a6ed5b4fb..17d2d62c3f1 100644 --- a/include/records/RecCore.h +++ b/include/records/RecCore.h @@ -25,6 +25,8 @@ #include #include +#include +#include #include "tscore/Diags.h" @@ -69,9 +71,17 @@ std::string RecConfigReadConfigPath(const char *file_variable, const char *defau // Return a copy of the persistent stats file. This is $RUNTIMEDIR/records.snap. std::string RecConfigReadPersistentStatsPath(); -// Test whether the named configuration value is overridden by an environment variable. Return either -// the overridden value, or the original value. Caller MUST NOT free the result. -const char *RecConfigOverrideFromEnvironment(const char *name, const char *value); +/// Indicates why RecConfigOverrideFromEnvironment() chose its returned value. +enum class RecConfigOverrideSource { + NONE, ///< No override — the original value was kept. + ENV, ///< Overridden by a PROXY_CONFIG_* environment variable. + RUNROOT, ///< Overridden with the resolved Layout path because runroot manages this record. +}; + +// Test whether the named configuration value is overridden by an environment +// variable or by the runroot mechanism. Returns the resolved value together +// with the source that produced it. +std::pair RecConfigOverrideFromEnvironment(const char *name, const char *value); //------------------------------------------------------------------------- // Stat Registration diff --git a/src/records/RecConfigParse.cc b/src/records/RecConfigParse.cc index 0819137f7d2..140c567eaa6 100644 --- a/src/records/RecConfigParse.cc +++ b/src/records/RecConfigParse.cc @@ -84,24 +84,20 @@ RecFileImport_Xmalloc(const char *file, char **file_buf, int *file_size) } //------------------------------------------------------------------------- -// RecConfigOverrideFromRunroot +// Records whose paths are managed by runroot. When runroot is active the +// value from records.yaml is replaced with the resolved Layout path. //------------------------------------------------------------------------- -bool -RecConfigOverrideFromRunroot(const char *name) -{ - if (!get_runroot().empty()) { - if (!strcmp(name, "proxy.config.bin_path") || !strcmp(name, "proxy.config.local_state_dir") || - !strcmp(name, "proxy.config.log.logfile_dir") || !strcmp(name, "proxy.config.plugin.plugin_dir")) { - return true; - } - } - return false; -} +static constexpr std::pair runroot_records[] = { + {"proxy.config.bin_path", &Layout::bindir }, + {"proxy.config.local_state_dir", &Layout::runtimedir}, + {"proxy.config.log.logfile_dir", &Layout::logdir }, + {"proxy.config.plugin.plugin_dir", &Layout::libexecdir}, +}; //------------------------------------------------------------------------- // RecConfigOverrideFromEnvironment //------------------------------------------------------------------------- -const char * +std::pair RecConfigOverrideFromEnvironment(const char *name, const char *value) { ats_scoped_str envname(ats_strdup(name)); @@ -121,12 +117,18 @@ RecConfigOverrideFromEnvironment(const char *name, const char *value) envval = getenv(envname.get()); if (envval) { - return envval; - } else if (RecConfigOverrideFromRunroot(name)) { - return nullptr; + return {envval, RecConfigOverrideSource::ENV}; + } + + if (!get_runroot().empty()) { + for (auto const &[rec_name, member] : runroot_records) { + if (rec_name == name) { + return {Layout::get()->*member, RecConfigOverrideSource::RUNROOT}; + } + } } - return value; + return {value ? value : "", RecConfigOverrideSource::NONE}; } //------------------------------------------------------------------------- @@ -141,10 +143,9 @@ RecConfigFileParse(const char *path, RecConfigEntryCallback handler) const char *line; int line_num; - char *rec_type_str, *name_str, *data_type_str, *data_str; - const char *value_str; - RecT rec_type; - RecDataT data_type; + char *rec_type_str, *name_str, *data_type_str, *data_str; + RecT rec_type; + RecDataT data_type; Tokenizer line_tok("\r\n"); tok_iter_state line_tok_state; @@ -245,8 +246,15 @@ RecConfigFileParse(const char *path, RecConfigEntryCallback handler) } // OK, we parsed the record, send it to the handler ... - value_str = RecConfigOverrideFromEnvironment(name_str, data_str); - handler(rec_type, data_type, name_str, value_str, value_str == data_str ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV); + { + auto [value_str, override_source] = RecConfigOverrideFromEnvironment(name_str, data_str); + if (override_source != RecConfigOverrideSource::NONE) { + RecDebug(DL_Debug, "'%s' overridden with '%s' by %s", name_str, value_str.c_str(), + override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot"); + } + handler(rec_type, data_type, name_str, value_str.c_str(), + override_source == RecConfigOverrideSource::NONE ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV); + } // update our g_rec_config_contents_xxx g_rec_config_contents_ht.emplace(name_str); diff --git a/src/records/RecYAMLDecoder.cc b/src/records/RecYAMLDecoder.cc index 29f2f12fa95..dadd9751c15 100644 --- a/src/records/RecYAMLDecoder.cc +++ b/src/records/RecYAMLDecoder.cc @@ -156,11 +156,12 @@ SetRecordFromYAMLNode(CfgNode const &field, swoc::Errata &errata) std::string field_value = field.value_node.as(); // in case of a string, the library will give us the literal // 'null' which is exactly what we want. - std::string value_str = RecConfigOverrideFromEnvironment(record_name.c_str(), field_value.c_str()); - RecSourceT source = (field_value == value_str ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV); + auto [value_str, override_source] = RecConfigOverrideFromEnvironment(record_name.c_str(), field_value.c_str()); + RecSourceT source = (override_source == RecConfigOverrideSource::NONE) ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV; - if (source == REC_SOURCE_ENV) { - errata.note(ERRATA_DEBUG, "'{}' was override with '{}' using an env variable", record_name, value_str); + if (override_source != RecConfigOverrideSource::NONE) { + errata.note(ERRATA_DEBUG, "'{}' overridden with '{}' by {}", record_name, value_str, + override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot"); } if (!check_expr.empty() && RecordValidityCheck(value_str.c_str(), check_type, check_expr.c_str()) == false) { diff --git a/src/records/RecordsConfigUtils.cc b/src/records/RecordsConfigUtils.cc index f572a957495..dc49a08e00a 100644 --- a/src/records/RecordsConfigUtils.cc +++ b/src/records/RecordsConfigUtils.cc @@ -49,9 +49,14 @@ initialize_record(const RecordElement *record, void *) access = record->access; if (REC_TYPE_IS_CONFIG(type)) { - const char *value = RecConfigOverrideFromEnvironment(record->name, record->value); - RecData data = {0}; - RecSourceT source = value == record->value ? REC_SOURCE_DEFAULT : REC_SOURCE_ENV; + auto [value, override_source] = RecConfigOverrideFromEnvironment(record->name, record->value); + RecData data = {0}; + RecSourceT source = (override_source == RecConfigOverrideSource::NONE) ? REC_SOURCE_DEFAULT : REC_SOURCE_ENV; + + if (override_source != RecConfigOverrideSource::NONE) { + RecDebug(DL_Debug, "'%s' overridden with '%s' by %s", record->name, value.c_str(), + override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot"); + } // If you specify a consistency check, you have to specify a regex expression. We abort here // so that this breaks QA completely. @@ -59,7 +64,11 @@ initialize_record(const RecordElement *record, void *) ink_fatal("%s has a consistency check but no regular expression", record->name); } - RecDataSetFromString(record->value_type, &data, value); + // When the built-in default is nullptr and no override was applied, preserve + // nullptr so optional records (e.g. keylog_file, groups_list) stay unset. + const char *value_ptr = + (override_source == RecConfigOverrideSource::NONE && record->value == nullptr) ? nullptr : value.c_str(); + RecDataSetFromString(record->value_type, &data, value_ptr); RecErrT reg_status{REC_ERR_FAIL}; switch (record->value_type) { case RECD_INT: diff --git a/tests/gold_tests/records/records_runroot_precedence.test.py b/tests/gold_tests/records/records_runroot_precedence.test.py new file mode 100644 index 00000000000..662de95039f --- /dev/null +++ b/tests/gold_tests/records/records_runroot_precedence.test.py @@ -0,0 +1,167 @@ +''' +''' +# 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 os + +Test.Summary = ''' +Verify that when runroot is active, path records from records.yaml are +overridden by the resolved Layout paths (precedence: env var > runroot > records.yaml). + +When --run-root (or TS_RUNROOT) is set and the PROXY_CONFIG_* environment +variables for path records are unset, RecConfigOverrideFromEnvironment() +returns the actual Layout path (e.g. Layout::bindir, Layout::logdir) which +was populated from runroot.yaml — effectively making runroot.yaml override +records.yaml for these path records. +''' +Test.ContinueOnFail = True +Test.SkipUnless( + Test.Variables.BINDIR.startswith(Test.Variables.PREFIX), "need to guarantee bin path starts with prefix for runroot") + +ts = Test.MakeATSProcess("ts") +ts_dir = os.path.join(Test.RunDirectory, "ts") + +# Set deliberately WRONG values in records.yaml for all 4 runroot-managed +# path records. If runroot override works, these values must NOT be used. +ts.Disk.records_config.append_to_document( + ''' + bin_path: wrong_bin_path + local_state_dir: wrong_runtime + log: + logfile_dir: wrong_log + plugin: + plugin_dir: wrong_plugin +''') + +# Build the ATS command: +# - Unset the 4 path env vars (the test framework always sets them, +# which masks the runroot code path). +# - Set TS_RUNROOT to the sandbox dir so the runroot mechanism activates. +original_cmd = ts.Command +ts.Command = ( + "env" + " -u PROXY_CONFIG_BIN_PATH" + " -u PROXY_CONFIG_LOCAL_STATE_DIR" + " -u PROXY_CONFIG_LOG_LOGFILE_DIR" + " -u PROXY_CONFIG_PLUGIN_PLUGIN_DIR" + f" TS_RUNROOT={ts_dir}" + f" {original_cmd}") + +# --------------------------------------------------------------------------- +# Test 0: Create runroot.yaml that maps to the sandbox layout, then start ATS. +# +# The runroot.yaml must exist before ATS starts because TS_RUNROOT triggers +# Layout::runroot_setup() during initialization. We write a runroot.yaml +# whose paths match the sandbox structure the test framework already created +# (traffic_layout init would create a different FHS-style layout that does +# not match the sandbox, so we write it manually). +# --------------------------------------------------------------------------- +runroot_yaml = os.path.join(ts_dir, 'runroot.yaml') + +runroot_lines = [ + f"prefix: {ts_dir}", + f"bindir: {os.path.join(ts_dir, 'bin')}", + f"sbindir: {os.path.join(ts_dir, 'bin')}", + f"sysconfdir: {os.path.join(ts_dir, 'config')}", + f"logdir: {os.path.join(ts_dir, 'log')}", + f"libexecdir: {os.path.join(ts_dir, 'plugin')}", + f"localstatedir: {os.path.join(ts_dir, 'runtime')}", + f"runtimedir: {os.path.join(ts_dir, 'runtime')}", + f"cachedir: {os.path.join(ts_dir, 'cache')}", +] +runroot_content = "\\n".join(runroot_lines) + "\\n" + +tr = Test.AddTestRun("Create runroot.yaml") +tr.Processes.Default.Command = f"mkdir -p {ts_dir} && printf '{runroot_content}' > {runroot_yaml}" +tr.Processes.Default.ReturnCode = 0 + +# --------------------------------------------------------------------------- +# Test 1: Start ATS with runroot active +# --------------------------------------------------------------------------- +tr = Test.AddTestRun("Start ATS with runroot") +tr.Processes.Default.Command = 'echo start' +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.StartBefore(ts) +tr.StillRunningAfter = ts + +# ATS must not crash (the original nullptr bug) and must complete startup. +ts.Disk.traffic_out.Content = Testers.ExcludesExpression( + "basic_string", "must not crash with 'basic_string: construction from null'") +ts.Disk.traffic_out.Content += Testers.ContainsExpression("records parsing completed", "ATS should complete records parsing") + +# Verify the override log messages appear in traffic.out. +# The errata notes from RecYAMLDecoder are printed by RecCoreInit(). +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "'proxy.config.bin_path' overridden with .* by runroot", "bin_path override by runroot must be logged") +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "'proxy.config.local_state_dir' overridden with .* by runroot", "local_state_dir override by runroot must be logged") +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "'proxy.config.log.logfile_dir' overridden with .* by runroot", "logfile_dir override by runroot must be logged") +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "'proxy.config.plugin.plugin_dir' overridden with .* by runroot", "plugin_dir override by runroot must be logged") +ts.Disk.traffic_out.Content += Testers.ContainsExpression( + "'proxy.config.diags.debug.tags' overridden with 'env_wins' by environment variable", + "diags.debug.tags override by environment variable must be logged") + +# --------------------------------------------------------------------------- +# Test 2: Verify path records do NOT contain the records.yaml values. +# +# Because runroot is active and env vars are unset, the records should hold +# the resolved Layout paths from runroot.yaml, not the records.yaml values. +# --------------------------------------------------------------------------- +tr = Test.AddTestRun("Verify runroot overrides records.yaml for path records") +tr.Processes.Default.Command = ( + 'traffic_ctl config get' + ' proxy.config.bin_path' + ' proxy.config.local_state_dir' + ' proxy.config.log.logfile_dir' + ' proxy.config.plugin.plugin_dir') +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 0 +tr.StillRunningAfter = ts + +# The deliberately wrong records.yaml values must NOT appear. +tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression( + 'wrong_bin_path', 'bin_path must be overridden by runroot, not records.yaml') +tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression( + 'wrong_runtime', 'local_state_dir must be overridden by runroot, not records.yaml') +tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression( + 'wrong_log', 'logfile_dir must be overridden by runroot, not records.yaml') +tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression( + 'wrong_plugin', 'plugin_dir must be overridden by runroot, not records.yaml') + +# --------------------------------------------------------------------------- +# Test 3: Verify env vars still take highest precedence over runroot. +# +# Set PROXY_CONFIG_DIAGS_DEBUG_TAGS via env and a different value in +# records.yaml. The env value must win regardless of runroot. +# --------------------------------------------------------------------------- +ts.Env['PROXY_CONFIG_DIAGS_DEBUG_TAGS'] = 'env_wins' +ts.Disk.records_config.update(''' + diags: + debug: + enabled: 0 + tags: config_value + ''') + +tr = Test.AddTestRun("Verify env var overrides both runroot and records.yaml") +tr.Processes.Default.Command = 'traffic_ctl config get proxy.config.diags.debug.tags' +tr.Processes.Default.Env = ts.Env +tr.Processes.Default.ReturnCode = 0 +tr.StillRunningAfter = ts +tr.Processes.Default.Streams.stdout = Testers.ContainsExpression( + 'proxy.config.diags.debug.tags: env_wins', 'Env var must override both runroot and records.yaml') From 48858ffc627efdaa7e6bedc00e5c3e8dce5c679f Mon Sep 17 00:00:00 2001 From: Damian Meden Date: Fri, 6 Mar 2026 09:27:44 +0000 Subject: [PATCH 2/2] clean up --- include/records/RecCore.h | 21 ++++++++++++++++--- src/records/RecConfigParse.cc | 2 +- src/records/RecYAMLDecoder.cc | 2 +- src/records/RecordsConfigUtils.cc | 2 +- .../records_runroot_precedence.test.py | 20 ++++++++---------- 5 files changed, 30 insertions(+), 17 deletions(-) diff --git a/include/records/RecCore.h b/include/records/RecCore.h index 17d2d62c3f1..40c7b32743e 100644 --- a/include/records/RecCore.h +++ b/include/records/RecCore.h @@ -78,9 +78,24 @@ enum class RecConfigOverrideSource { RUNROOT, ///< Overridden with the resolved Layout path because runroot manages this record. }; -// Test whether the named configuration value is overridden by an environment -// variable or by the runroot mechanism. Returns the resolved value together -// with the source that produced it. +/// Human-readable label for the override source (for logging). +constexpr const char * +RecConfigOverrideSourceName(RecConfigOverrideSource src) +{ + switch (src) { + case RecConfigOverrideSource::ENV: + return "environment variable"; + case RecConfigOverrideSource::RUNROOT: + return "runroot"; + default: + return "none"; + } +} + +// Test whether the named configuration value is overridden by the execution +// environment — either a PROXY_CONFIG_* environment variable or the runroot +// mechanism. Returns the resolved value together with the source that +// produced it. std::pair RecConfigOverrideFromEnvironment(const char *name, const char *value); //------------------------------------------------------------------------- diff --git a/src/records/RecConfigParse.cc b/src/records/RecConfigParse.cc index 140c567eaa6..d80a1237af5 100644 --- a/src/records/RecConfigParse.cc +++ b/src/records/RecConfigParse.cc @@ -250,7 +250,7 @@ RecConfigFileParse(const char *path, RecConfigEntryCallback handler) auto [value_str, override_source] = RecConfigOverrideFromEnvironment(name_str, data_str); if (override_source != RecConfigOverrideSource::NONE) { RecDebug(DL_Debug, "'%s' overridden with '%s' by %s", name_str, value_str.c_str(), - override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot"); + RecConfigOverrideSourceName(override_source)); } handler(rec_type, data_type, name_str, value_str.c_str(), override_source == RecConfigOverrideSource::NONE ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV); diff --git a/src/records/RecYAMLDecoder.cc b/src/records/RecYAMLDecoder.cc index dadd9751c15..1bb7b9e45e9 100644 --- a/src/records/RecYAMLDecoder.cc +++ b/src/records/RecYAMLDecoder.cc @@ -161,7 +161,7 @@ SetRecordFromYAMLNode(CfgNode const &field, swoc::Errata &errata) if (override_source != RecConfigOverrideSource::NONE) { errata.note(ERRATA_DEBUG, "'{}' overridden with '{}' by {}", record_name, value_str, - override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot"); + RecConfigOverrideSourceName(override_source)); } if (!check_expr.empty() && RecordValidityCheck(value_str.c_str(), check_type, check_expr.c_str()) == false) { diff --git a/src/records/RecordsConfigUtils.cc b/src/records/RecordsConfigUtils.cc index dc49a08e00a..9d244510616 100644 --- a/src/records/RecordsConfigUtils.cc +++ b/src/records/RecordsConfigUtils.cc @@ -55,7 +55,7 @@ initialize_record(const RecordElement *record, void *) if (override_source != RecConfigOverrideSource::NONE) { RecDebug(DL_Debug, "'%s' overridden with '%s' by %s", record->name, value.c_str(), - override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot"); + RecConfigOverrideSourceName(override_source)); } // If you specify a consistency check, you have to specify a regex expression. We abort here diff --git a/tests/gold_tests/records/records_runroot_precedence.test.py b/tests/gold_tests/records/records_runroot_precedence.test.py index 662de95039f..d1ce4362f9a 100644 --- a/tests/gold_tests/records/records_runroot_precedence.test.py +++ b/tests/gold_tests/records/records_runroot_precedence.test.py @@ -47,6 +47,15 @@ plugin_dir: wrong_plugin ''') +# Test 3 setup: env var that must win over both runroot and records.yaml. +ts.Env['PROXY_CONFIG_DIAGS_DEBUG_TAGS'] = 'env_wins' +ts.Disk.records_config.update(''' + diags: + debug: + enabled: 0 + tags: config_value + ''') + # Build the ATS command: # - Unset the 4 path env vars (the test framework always sets them, # which masks the runroot code path). @@ -146,18 +155,7 @@ # --------------------------------------------------------------------------- # Test 3: Verify env vars still take highest precedence over runroot. -# -# Set PROXY_CONFIG_DIAGS_DEBUG_TAGS via env and a different value in -# records.yaml. The env value must win regardless of runroot. # --------------------------------------------------------------------------- -ts.Env['PROXY_CONFIG_DIAGS_DEBUG_TAGS'] = 'env_wins' -ts.Disk.records_config.update(''' - diags: - debug: - enabled: 0 - tags: config_value - ''') - tr = Test.AddTestRun("Verify env var overrides both runroot and records.yaml") tr.Processes.Default.Command = 'traffic_ctl config get proxy.config.diags.debug.tags' tr.Processes.Default.Env = ts.Env