diff --git a/include/records/RecCore.h b/include/records/RecCore.h index d0a6ed5b4fb..77148c386aa 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 (blanked) because runroot manages this path 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..a4dfbd400ec 100644 --- a/src/records/RecConfigParse.cc +++ b/src/records/RecConfigParse.cc @@ -101,7 +101,7 @@ RecConfigOverrideFromRunroot(const char *name) //------------------------------------------------------------------------- // RecConfigOverrideFromEnvironment //------------------------------------------------------------------------- -const char * +std::pair RecConfigOverrideFromEnvironment(const char *name, const char *value) { ats_scoped_str envname(ats_strdup(name)); @@ -121,12 +121,15 @@ RecConfigOverrideFromEnvironment(const char *name, const char *value) envval = getenv(envname.get()); if (envval) { - return envval; + return {envval, RecConfigOverrideSource::ENV}; } else if (RecConfigOverrideFromRunroot(name)) { - return nullptr; + // Return empty to blank this record. The actual path will be resolved + // at runtime by RecConfigReadBinDir() / ReadLogDir() / etc., which fall + // through to the Layout paths populated from runroot.yaml. + return {"", RecConfigOverrideSource::RUNROOT}; } - return value; + return {value ? value : "", RecConfigOverrideSource::NONE}; } //------------------------------------------------------------------------- @@ -141,10 +144,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 +247,11 @@ 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); + 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..29df8da44d7 100644 --- a/src/records/RecYAMLDecoder.cc +++ b/src/records/RecYAMLDecoder.cc @@ -156,14 +156,20 @@ 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 (!check_expr.empty() && RecordValidityCheck(value_str.c_str(), check_type, check_expr.c_str()) == false) { + // When runroot intentionally blanks a path record (returning ""), skip + // validation — some records (e.g. logfile_dir) have regexes that reject + // empty strings. The blank value is expected: at runtime the path- + // resolution helpers (RecConfigReadBinDir, ReadLogDir, …) see the empty + // record and fall through to the Layout paths set from runroot.yaml. + if (override_source != RecConfigOverrideSource::RUNROOT && !check_expr.empty() && + RecordValidityCheck(value_str.c_str(), check_type, check_expr.c_str()) == false) { errata.note(ERRATA_WARN, "{} - Validity Check error {}. Pattern '{}' failed against '{}'. Default value will be used", record_name, field.mark_as_view("at line={}, col={}"), check_expr, value_str); return; diff --git a/src/records/RecordsConfigUtils.cc b/src/records/RecordsConfigUtils.cc index f572a957495..a5773ba5c63 100644 --- a/src/records/RecordsConfigUtils.cc +++ b/src/records/RecordsConfigUtils.cc @@ -49,9 +49,9 @@ 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 you specify a consistency check, you have to specify a regex expression. We abort here // so that this breaks QA completely. @@ -59,7 +59,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..e3ae02b7094 --- /dev/null +++ b/tests/gold_tests/records/records_runroot_precedence.test.py @@ -0,0 +1,154 @@ +''' +''' +# 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 runroot 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 an empty string. This causes the runtime path-resolution functions +(RecConfigReadBinDir, etc.) to fall through to Layout::bindir / logdir / ... +which are 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") + +# --------------------------------------------------------------------------- +# Test 2: Verify path records do NOT contain the records.yaml values. +# +# Because runroot is active and env vars are unset, the records should have +# been blanked by RecConfigOverrideFromEnvironment() returning "". +# --------------------------------------------------------------------------- +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')