Skip to content

Commit 447753b

Browse files
brbzull0brbzull0
committed
RecConfigOverrideFromEnvironment: return resolved value and override source
Change the return type to std::pair<std::string, RecConfigOverrideSource> 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.
1 parent 00604e1 commit 447753b

5 files changed

Lines changed: 229 additions & 34 deletions

File tree

‎include/records/RecCore.h‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,8 @@
2525

2626
#include <functional>
2727
#include <optional>
28+
#include <string>
29+
#include <utility>
2830

2931
#include "tscore/Diags.h"
3032

@@ -69,9 +71,17 @@ std::string RecConfigReadConfigPath(const char *file_variable, const char *defau
6971
// Return a copy of the persistent stats file. This is $RUNTIMEDIR/records.snap.
7072
std::string RecConfigReadPersistentStatsPath();
7173

72-
// Test whether the named configuration value is overridden by an environment variable. Return either
73-
// the overridden value, or the original value. Caller MUST NOT free the result.
74-
const char *RecConfigOverrideFromEnvironment(const char *name, const char *value);
74+
/// Indicates why RecConfigOverrideFromEnvironment() chose its returned value.
75+
enum class RecConfigOverrideSource {
76+
NONE, ///< No override — the original value was kept.
77+
ENV, ///< Overridden by a PROXY_CONFIG_* environment variable.
78+
RUNROOT, ///< Overridden with the resolved Layout path because runroot manages this record.
79+
};
80+
81+
// Test whether the named configuration value is overridden by an environment
82+
// variable or by the runroot mechanism. Returns the resolved value together
83+
// with the source that produced it.
84+
std::pair<std::string, RecConfigOverrideSource> RecConfigOverrideFromEnvironment(const char *name, const char *value);
7585

7686
//-------------------------------------------------------------------------
7787
// Stat Registration

‎src/records/RecConfigParse.cc‎

Lines changed: 31 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -84,24 +84,20 @@ RecFileImport_Xmalloc(const char *file, char **file_buf, int *file_size)
8484
}
8585

8686
//-------------------------------------------------------------------------
87-
// RecConfigOverrideFromRunroot
87+
// Records whose paths are managed by runroot. When runroot is active the
88+
// value from records.yaml is replaced with the resolved Layout path.
8889
//-------------------------------------------------------------------------
89-
bool
90-
RecConfigOverrideFromRunroot(const char *name)
91-
{
92-
if (!get_runroot().empty()) {
93-
if (!strcmp(name, "proxy.config.bin_path") || !strcmp(name, "proxy.config.local_state_dir") ||
94-
!strcmp(name, "proxy.config.log.logfile_dir") || !strcmp(name, "proxy.config.plugin.plugin_dir")) {
95-
return true;
96-
}
97-
}
98-
return false;
99-
}
90+
static constexpr std::pair<std::string_view, std::string Layout::*> runroot_records[] = {
91+
{"proxy.config.bin_path", &Layout::bindir },
92+
{"proxy.config.local_state_dir", &Layout::runtimedir},
93+
{"proxy.config.log.logfile_dir", &Layout::logdir },
94+
{"proxy.config.plugin.plugin_dir", &Layout::libexecdir},
95+
};
10096

10197
//-------------------------------------------------------------------------
10298
// RecConfigOverrideFromEnvironment
10399
//-------------------------------------------------------------------------
104-
const char *
100+
std::pair<std::string, RecConfigOverrideSource>
105101
RecConfigOverrideFromEnvironment(const char *name, const char *value)
106102
{
107103
ats_scoped_str envname(ats_strdup(name));
@@ -121,12 +117,18 @@ RecConfigOverrideFromEnvironment(const char *name, const char *value)
121117

122118
envval = getenv(envname.get());
123119
if (envval) {
124-
return envval;
125-
} else if (RecConfigOverrideFromRunroot(name)) {
126-
return nullptr;
120+
return {envval, RecConfigOverrideSource::ENV};
121+
}
122+
123+
if (!get_runroot().empty()) {
124+
for (auto const &[rec_name, member] : runroot_records) {
125+
if (rec_name == name) {
126+
return {Layout::get()->*member, RecConfigOverrideSource::RUNROOT};
127+
}
128+
}
127129
}
128130

129-
return value;
131+
return {value ? value : "", RecConfigOverrideSource::NONE};
130132
}
131133

132134
//-------------------------------------------------------------------------
@@ -141,10 +143,9 @@ RecConfigFileParse(const char *path, RecConfigEntryCallback handler)
141143
const char *line;
142144
int line_num;
143145

144-
char *rec_type_str, *name_str, *data_type_str, *data_str;
145-
const char *value_str;
146-
RecT rec_type;
147-
RecDataT data_type;
146+
char *rec_type_str, *name_str, *data_type_str, *data_str;
147+
RecT rec_type;
148+
RecDataT data_type;
148149

149150
Tokenizer line_tok("\r\n");
150151
tok_iter_state line_tok_state;
@@ -245,8 +246,15 @@ RecConfigFileParse(const char *path, RecConfigEntryCallback handler)
245246
}
246247

247248
// OK, we parsed the record, send it to the handler ...
248-
value_str = RecConfigOverrideFromEnvironment(name_str, data_str);
249-
handler(rec_type, data_type, name_str, value_str, value_str == data_str ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV);
249+
{
250+
auto [value_str, override_source] = RecConfigOverrideFromEnvironment(name_str, data_str);
251+
if (override_source != RecConfigOverrideSource::NONE) {
252+
RecDebug(DL_Debug, "'%s' overridden with '%s' by %s", name_str, value_str.c_str(),
253+
override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot");
254+
}
255+
handler(rec_type, data_type, name_str, value_str.c_str(),
256+
override_source == RecConfigOverrideSource::NONE ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV);
257+
}
250258

251259
// update our g_rec_config_contents_xxx
252260
g_rec_config_contents_ht.emplace(name_str);

‎src/records/RecYAMLDecoder.cc‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -156,11 +156,12 @@ SetRecordFromYAMLNode(CfgNode const &field, swoc::Errata &errata)
156156
std::string field_value = field.value_node.as<std::string>(); // in case of a string, the library will give us the literal
157157
// 'null' which is exactly what we want.
158158

159-
std::string value_str = RecConfigOverrideFromEnvironment(record_name.c_str(), field_value.c_str());
160-
RecSourceT source = (field_value == value_str ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV);
159+
auto [value_str, override_source] = RecConfigOverrideFromEnvironment(record_name.c_str(), field_value.c_str());
160+
RecSourceT source = (override_source == RecConfigOverrideSource::NONE) ? REC_SOURCE_EXPLICIT : REC_SOURCE_ENV;
161161

162-
if (source == REC_SOURCE_ENV) {
163-
errata.note(ERRATA_DEBUG, "'{}' was override with '{}' using an env variable", record_name, value_str);
162+
if (override_source != RecConfigOverrideSource::NONE) {
163+
errata.note(ERRATA_DEBUG, "'{}' overridden with '{}' by {}", record_name, value_str,
164+
override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot");
164165
}
165166

166167
if (!check_expr.empty() && RecordValidityCheck(value_str.c_str(), check_type, check_expr.c_str()) == false) {

‎src/records/RecordsConfigUtils.cc‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,17 +49,26 @@ initialize_record(const RecordElement *record, void *)
4949
access = record->access;
5050

5151
if (REC_TYPE_IS_CONFIG(type)) {
52-
const char *value = RecConfigOverrideFromEnvironment(record->name, record->value);
53-
RecData data = {0};
54-
RecSourceT source = value == record->value ? REC_SOURCE_DEFAULT : REC_SOURCE_ENV;
52+
auto [value, override_source] = RecConfigOverrideFromEnvironment(record->name, record->value);
53+
RecData data = {0};
54+
RecSourceT source = (override_source == RecConfigOverrideSource::NONE) ? REC_SOURCE_DEFAULT : REC_SOURCE_ENV;
55+
56+
if (override_source != RecConfigOverrideSource::NONE) {
57+
RecDebug(DL_Debug, "'%s' overridden with '%s' by %s", record->name, value.c_str(),
58+
override_source == RecConfigOverrideSource::ENV ? "environment variable" : "runroot");
59+
}
5560

5661
// If you specify a consistency check, you have to specify a regex expression. We abort here
5762
// so that this breaks QA completely.
5863
if (record->check != RECC_NULL && record->regex == nullptr) {
5964
ink_fatal("%s has a consistency check but no regular expression", record->name);
6065
}
6166

62-
RecDataSetFromString(record->value_type, &data, value);
67+
// When the built-in default is nullptr and no override was applied, preserve
68+
// nullptr so optional records (e.g. keylog_file, groups_list) stay unset.
69+
const char *value_ptr =
70+
(override_source == RecConfigOverrideSource::NONE && record->value == nullptr) ? nullptr : value.c_str();
71+
RecDataSetFromString(record->value_type, &data, value_ptr);
6372
RecErrT reg_status{REC_ERR_FAIL};
6473
switch (record->value_type) {
6574
case RECD_INT:

‎tests/gold_tests/records/records_runroot_precedence.test.py‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
'''
2+
'''
3+
# Licensed to the Apache Software Foundation (ASF) under one
4+
# or more contributor license agreements. See the NOTICE file
5+
# distributed with this work for additional information
6+
# regarding copyright ownership. The ASF licenses this file
7+
# to you under the Apache License, Version 2.0 (the
8+
# "License"); you may not use this file except in compliance
9+
# with the License. You may obtain a copy of the License at
10+
#
11+
# http://www.apache.org/licenses/LICENSE-2.0
12+
#
13+
# Unless required by applicable law or agreed to in writing, software
14+
# distributed under the License is distributed on an "AS IS" BASIS,
15+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
16+
# See the License for the specific language governing permissions and
17+
# limitations under the License.
18+
19+
import os
20+
21+
Test.Summary = '''
22+
Verify that when runroot is active, path records from records.yaml are
23+
overridden by the resolved Layout paths (precedence: env var > runroot > records.yaml).
24+
25+
When --run-root (or TS_RUNROOT) is set and the PROXY_CONFIG_* environment
26+
variables for path records are unset, RecConfigOverrideFromEnvironment()
27+
returns the actual Layout path (e.g. Layout::bindir, Layout::logdir) which
28+
was populated from runroot.yaml — effectively making runroot.yaml override
29+
records.yaml for these path records.
30+
'''
31+
Test.ContinueOnFail = True
32+
Test.SkipUnless(
33+
Test.Variables.BINDIR.startswith(Test.Variables.PREFIX), "need to guarantee bin path starts with prefix for runroot")
34+
35+
ts = Test.MakeATSProcess("ts")
36+
ts_dir = os.path.join(Test.RunDirectory, "ts")
37+
38+
# Set deliberately WRONG values in records.yaml for all 4 runroot-managed
39+
# path records. If runroot override works, these values must NOT be used.
40+
ts.Disk.records_config.append_to_document(
41+
'''
42+
bin_path: wrong_bin_path
43+
local_state_dir: wrong_runtime
44+
log:
45+
logfile_dir: wrong_log
46+
plugin:
47+
plugin_dir: wrong_plugin
48+
''')
49+
50+
# Build the ATS command:
51+
# - Unset the 4 path env vars (the test framework always sets them,
52+
# which masks the runroot code path).
53+
# - Set TS_RUNROOT to the sandbox dir so the runroot mechanism activates.
54+
original_cmd = ts.Command
55+
ts.Command = (
56+
"env"
57+
" -u PROXY_CONFIG_BIN_PATH"
58+
" -u PROXY_CONFIG_LOCAL_STATE_DIR"
59+
" -u PROXY_CONFIG_LOG_LOGFILE_DIR"
60+
" -u PROXY_CONFIG_PLUGIN_PLUGIN_DIR"
61+
f" TS_RUNROOT={ts_dir}"
62+
f" {original_cmd}")
63+
64+
# ---------------------------------------------------------------------------
65+
# Test 0: Create runroot.yaml that maps to the sandbox layout, then start ATS.
66+
#
67+
# The runroot.yaml must exist before ATS starts because TS_RUNROOT triggers
68+
# Layout::runroot_setup() during initialization. We write a runroot.yaml
69+
# whose paths match the sandbox structure the test framework already created
70+
# (traffic_layout init would create a different FHS-style layout that does
71+
# not match the sandbox, so we write it manually).
72+
# ---------------------------------------------------------------------------
73+
runroot_yaml = os.path.join(ts_dir, 'runroot.yaml')
74+
75+
runroot_lines = [
76+
f"prefix: {ts_dir}",
77+
f"bindir: {os.path.join(ts_dir, 'bin')}",
78+
f"sbindir: {os.path.join(ts_dir, 'bin')}",
79+
f"sysconfdir: {os.path.join(ts_dir, 'config')}",
80+
f"logdir: {os.path.join(ts_dir, 'log')}",
81+
f"libexecdir: {os.path.join(ts_dir, 'plugin')}",
82+
f"localstatedir: {os.path.join(ts_dir, 'runtime')}",
83+
f"runtimedir: {os.path.join(ts_dir, 'runtime')}",
84+
f"cachedir: {os.path.join(ts_dir, 'cache')}",
85+
]
86+
runroot_content = "\\n".join(runroot_lines) + "\\n"
87+
88+
tr = Test.AddTestRun("Create runroot.yaml")
89+
tr.Processes.Default.Command = f"mkdir -p {ts_dir} && printf '{runroot_content}' > {runroot_yaml}"
90+
tr.Processes.Default.ReturnCode = 0
91+
92+
# ---------------------------------------------------------------------------
93+
# Test 1: Start ATS with runroot active
94+
# ---------------------------------------------------------------------------
95+
tr = Test.AddTestRun("Start ATS with runroot")
96+
tr.Processes.Default.Command = 'echo start'
97+
tr.Processes.Default.ReturnCode = 0
98+
tr.Processes.Default.StartBefore(ts)
99+
tr.StillRunningAfter = ts
100+
101+
# ATS must not crash (the original nullptr bug) and must complete startup.
102+
ts.Disk.traffic_out.Content = Testers.ExcludesExpression(
103+
"basic_string", "must not crash with 'basic_string: construction from null'")
104+
ts.Disk.traffic_out.Content += Testers.ContainsExpression("records parsing completed", "ATS should complete records parsing")
105+
106+
# Verify the override log messages appear in traffic.out.
107+
# The errata notes from RecYAMLDecoder are printed by RecCoreInit().
108+
ts.Disk.traffic_out.Content += Testers.ContainsExpression(
109+
"'proxy.config.bin_path' overridden with .* by runroot", "bin_path override by runroot must be logged")
110+
ts.Disk.traffic_out.Content += Testers.ContainsExpression(
111+
"'proxy.config.local_state_dir' overridden with .* by runroot", "local_state_dir override by runroot must be logged")
112+
ts.Disk.traffic_out.Content += Testers.ContainsExpression(
113+
"'proxy.config.log.logfile_dir' overridden with .* by runroot", "logfile_dir override by runroot must be logged")
114+
ts.Disk.traffic_out.Content += Testers.ContainsExpression(
115+
"'proxy.config.plugin.plugin_dir' overridden with .* by runroot", "plugin_dir override by runroot must be logged")
116+
ts.Disk.traffic_out.Content += Testers.ContainsExpression(
117+
"'proxy.config.diags.debug.tags' overridden with 'env_wins' by environment variable",
118+
"diags.debug.tags override by environment variable must be logged")
119+
120+
# ---------------------------------------------------------------------------
121+
# Test 2: Verify path records do NOT contain the records.yaml values.
122+
#
123+
# Because runroot is active and env vars are unset, the records should hold
124+
# the resolved Layout paths from runroot.yaml, not the records.yaml values.
125+
# ---------------------------------------------------------------------------
126+
tr = Test.AddTestRun("Verify runroot overrides records.yaml for path records")
127+
tr.Processes.Default.Command = (
128+
'traffic_ctl config get'
129+
' proxy.config.bin_path'
130+
' proxy.config.local_state_dir'
131+
' proxy.config.log.logfile_dir'
132+
' proxy.config.plugin.plugin_dir')
133+
tr.Processes.Default.Env = ts.Env
134+
tr.Processes.Default.ReturnCode = 0
135+
tr.StillRunningAfter = ts
136+
137+
# The deliberately wrong records.yaml values must NOT appear.
138+
tr.Processes.Default.Streams.stdout = Testers.ExcludesExpression(
139+
'wrong_bin_path', 'bin_path must be overridden by runroot, not records.yaml')
140+
tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression(
141+
'wrong_runtime', 'local_state_dir must be overridden by runroot, not records.yaml')
142+
tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression(
143+
'wrong_log', 'logfile_dir must be overridden by runroot, not records.yaml')
144+
tr.Processes.Default.Streams.stdout += Testers.ExcludesExpression(
145+
'wrong_plugin', 'plugin_dir must be overridden by runroot, not records.yaml')
146+
147+
# ---------------------------------------------------------------------------
148+
# Test 3: Verify env vars still take highest precedence over runroot.
149+
#
150+
# Set PROXY_CONFIG_DIAGS_DEBUG_TAGS via env and a different value in
151+
# records.yaml. The env value must win regardless of runroot.
152+
# ---------------------------------------------------------------------------
153+
ts.Env['PROXY_CONFIG_DIAGS_DEBUG_TAGS'] = 'env_wins'
154+
ts.Disk.records_config.update('''
155+
diags:
156+
debug:
157+
enabled: 0
158+
tags: config_value
159+
''')
160+
161+
tr = Test.AddTestRun("Verify env var overrides both runroot and records.yaml")
162+
tr.Processes.Default.Command = 'traffic_ctl config get proxy.config.diags.debug.tags'
163+
tr.Processes.Default.Env = ts.Env
164+
tr.Processes.Default.ReturnCode = 0
165+
tr.StillRunningAfter = ts
166+
tr.Processes.Default.Streams.stdout = Testers.ContainsExpression(
167+
'proxy.config.diags.debug.tags: env_wins', 'Env var must override both runroot and records.yaml')

0 commit comments

Comments
 (0)