This is an automated email from the ASF dual-hosted git repository.
bneradt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 482e0c62b3 healthchecks: fix event matching for directory events
(#12274)
482e0c62b3 is described below
commit 482e0c62b34a624b46de8547feb5e1f48789e376
Author: Brian Neradt <brian.ner...@gmail.com>
AuthorDate: Fri Jun 6 13:13:30 2025 -0500
healthchecks: fix event matching for directory events (#12274)
For directory changes, we compared the event's name (which indicates
which file in the directory changed) against the config's basename but
limited the check to basename_len via strncmp. Unwittingly, though, if
event->name was more bytes than basename_len, we could incorrectly match
on the substring.
For instance, with a config like this:
/acme /tmp/sb/healthchecks/acme text/plain 200 404
/acme-ssl /tmp/sb/healthchecks/acme-ssl text/plain 200 404
An event for acme-ssl changes would accidentally match for acme,
because acme's basename_len is 4, while event->name is acme-ssl, thus
strncmp on the event would incorrectly indicate a match because the
first four bytes were the same for each.
This extracts event/config file matching to a function so it is easier
to understand and ensures that event->name and basename are the same
length to avoid the accidental match.
This also adds an autest for healthchecks which reproduces the above
issue and verifies basic file event processing functionality.
---
plugins/healthchecks/healthchecks.cc | 48 +++++--
tests/gold_tests/pluginTest/healthchecks/acme | 17 +++
tests/gold_tests/pluginTest/healthchecks/acme-ssl | 17 +++
.../pluginTest/healthchecks/healthchecks.test.py | 148 +++++++++++++++++++++
4 files changed, 221 insertions(+), 9 deletions(-)
diff --git a/plugins/healthchecks/healthchecks.cc
b/plugins/healthchecks/healthchecks.cc
index c11a543662..f31a43d0cc 100644
--- a/plugins/healthchecks/healthchecks.cc
+++ b/plugins/healthchecks/healthchecks.cc
@@ -166,25 +166,54 @@ setup_watchers(int fd)
/* Separate thread to monitor status files for reload */
#define INOTIFY_BUFLEN (1024 * sizeof(struct inotify_event))
+/** Determine whether @a event applies to the @a finfo configured file. */
+static bool
+event_matches_config(struct inotify_event *event, HCFileInfo *finfo)
+{
+ if (!finfo || !event) {
+ return false;
+ }
+ if (event->wd == finfo->wd) {
+ // Easy: the event is for this configured file we are watching.
+ return true;
+ }
+ if (event->wd != finfo->dir->wd) {
+ // The event is not for this file, nor for the parent directory. No match.
+ return false;
+ }
+
+ // The event applies to a change in a directory that contains files we are
+ // configured to watch. Does the directory event apply to this file?
+
+ if (!finfo->basename || finfo->basename_len <= 0) {
+ // This configured finfo is not for a specific file.
+ return false;
+ }
+ if (strnlen(event->name, NAME_MAX) != finfo->basename_len) {
+ return false;
+ }
+ return strncmp(event->name, finfo->basename, finfo->basename_len) == 0;
+}
+
static void *
hc_thread(void *data ATS_UNUSED)
{
- int fd = inotify_init();
- HCFileData *fl_head = nullptr;
+ int inotify_fd = inotify_init();
+ HCFileData *fl_head = nullptr;
char buffer[INOTIFY_BUFLEN];
struct timeval last_free, now;
gettimeofday(&last_free, nullptr);
/* Setup watchers for the directories, these are a one time setup */
- setup_watchers(fd); // This is a leak, but since we enter an infinite loop
this is ok?
+ setup_watchers(inotify_fd); // This is a leak, but since we enter an
infinite loop this is ok?
- while (1) {
+ while (true) {
HCFileData *fdata = fl_head, *fdata_prev = nullptr;
gettimeofday(&now, nullptr);
/* Read the inotify events, blocking until we get something */
- int len = read(fd, buffer, INOTIFY_BUFLEN);
+ int len = read(inotify_fd, buffer, INOTIFY_BUFLEN);
/* The fl_head is a linked list of previously released data entries. They
are ordered "by time", so once we find one that is scheduled for
deletion,
@@ -220,8 +249,7 @@ hc_thread(void *data ATS_UNUSED)
struct inotify_event *event = (struct inotify_event *)&buffer[i];
HCFileInfo *finfo = g_config;
- while (finfo && !((event->wd == finfo->wd) ||
- ((event->wd == finfo->dir->wd) &&
!strncmp(event->name, finfo->basename, finfo->basename_len)))) {
+ while (finfo && !event_matches_config(event, finfo)) {
finfo = finfo->_next;
}
if (finfo) {
@@ -232,10 +260,12 @@ hc_thread(void *data ATS_UNUSED)
Dbg(dbg_ctl, "Modify file event (%d) on %s", event->mask,
finfo->fname);
} else if (event->mask & (IN_CREATE | IN_MOVED_TO)) {
Dbg(dbg_ctl, "Create file event (%d) on %s", event->mask,
finfo->fname);
- finfo->wd = inotify_add_watch(fd, finfo->fname, IN_DELETE_SELF |
IN_CLOSE_WRITE | IN_ATTRIB);
+ finfo->wd = inotify_add_watch(inotify_fd, finfo->fname,
IN_DELETE_SELF | IN_CLOSE_WRITE | IN_ATTRIB);
} else if (event->mask & (IN_DELETE_SELF | IN_MOVED_FROM)) {
Dbg(dbg_ctl, "Delete file event (%d) on %s", event->mask,
finfo->fname);
- finfo->wd = inotify_rm_watch(fd, finfo->wd);
+ finfo->wd = inotify_rm_watch(inotify_fd, finfo->wd);
+ } else {
+ Dbg(dbg_ctl, "Unhandled event (%d) on %s", event->mask,
finfo->fname);
}
/* Load the new data and then swap this atomically */
memset(new_data, 0, sizeof(HCFileData));
diff --git a/tests/gold_tests/pluginTest/healthchecks/acme
b/tests/gold_tests/pluginTest/healthchecks/acme
new file mode 100644
index 0000000000..a4ae271e6f
--- /dev/null
+++ b/tests/gold_tests/pluginTest/healthchecks/acme
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# 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.
+
+Some content.
diff --git a/tests/gold_tests/pluginTest/healthchecks/acme-ssl
b/tests/gold_tests/pluginTest/healthchecks/acme-ssl
new file mode 100644
index 0000000000..a4ae271e6f
--- /dev/null
+++ b/tests/gold_tests/pluginTest/healthchecks/acme-ssl
@@ -0,0 +1,17 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# 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.
+
+Some content.
diff --git a/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py
b/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py
new file mode 100644
index 0000000000..a0709c854a
--- /dev/null
+++ b/tests/gold_tests/pluginTest/healthchecks/healthchecks.test.py
@@ -0,0 +1,148 @@
+"""
+Verify healthchecks plugin behavior.
+"""
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# 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 healthchecks plugin behavior.'''
+
+Test.SkipUnless(Condition.PluginExists('healthchecks.so'))
+
+CONFIG = f'''
+/acme {Test.RunDirectory}/acme text/plain 200 404
+/acme-ssl {Test.RunDirectory}/acme-ssl text/plain 200 404
+'''
+
+CONTENT = 'Some generic content.'
+
+
+class TestFileChangeBehavior:
+ '''Verify healthchecks plugin file change detection behavior.'''
+
+ def __init__(self) -> None:
+ '''Initialize the TestRun.'''
+ self._ts_started = False
+ self._positive_hc_counter = 0
+ self._configure_global_ts()
+ self._expect_positive_healthchecks()
+ self._remove_acme_ssl()
+ self._expect_acme_ssl_404()
+ self._re_add_acme_ssl()
+ self._expect_positive_healthchecks()
+
+ def _configure_global_ts(self) -> None:
+ '''Configure a global Traffic Server instance for the test runs.
+ :param tr: The TestRun to associate the ATS instance with.
+ :return: The Traffic Server Process.
+ '''
+ ts = Test.MakeATSProcess('ts', enable_tls=True)
+ self._ts = ts
+
+ # healthchecks plugin configuration.
+ config_name = "healthchecks.config"
+ config_path = os.path.join(ts.Variables.CONFIGDIR, config_name)
+ ts.Disk.File(config_path, id='healthchecks_config',
typename="ats:config")
+ config = ts.Disk.healthchecks_config
+ config.AddLine(f'/acme {Test.RunDirectory}/acme text/plain 200 404')
+ config.AddLine(f'/acme-ssl {Test.RunDirectory}/acme-ssl text/plain 200
404')
+ ts.Setup.Copy('acme')
+ ts.Setup.Copy('acme-ssl')
+ ts.Disk.plugin_config.AddLine(f'healthchecks.so
{ts.Variables.CONFIGDIR}/healthchecks.config')
+
+ # TLS configuration.
+ ts.addDefaultSSLFiles()
+ ts.Disk.records_config.update(
+ {
+ "proxy.config.ssl.server.cert.path": f'{ts.Variables.SSLDir}',
+ "proxy.config.ssl.server.private_key.path":
f'{ts.Variables.SSLDir}',
+ "proxy.config.ssl.client.verify.server.policy": 'PERMISSIVE',
+ })
+ ts.Disk.ssl_multicert_config.AddLine('dest_ip=*
ssl_cert_name=server.pem ssl_key_name=server.key')
+
+ # Other configuration.
+ ts.Disk.records_config.update({
+ 'proxy.config.diags.debug.enabled': 1,
+ 'proxy.config.diags.debug.tags': 'healthchecks',
+ })
+ return ts
+
+ def _expect_positive_healthchecks(self) -> None:
+ '''Configure a positive healthcheck for the test runs.
+ :return: None
+ '''
+ self._positive_hc_counter += 1
+ counter = self._positive_hc_counter
+ tr = Test.AddTestRun(f'Positive acme healthchecks: {counter}')
+ tr.MakeCurlCommand(f'-v
http://127.0.0.1:{self._ts.Variables.port}/acme')
+ curl_acme = tr.Processes.Default
+ if not self._ts_started:
+ curl_acme.StartBefore(self._ts)
+ self._ts_started = True
+ curl_acme.Streams.All += Testers.ContainsExpression('HTTP/1.1 200',
'Verify 200 response for /acme')
+
+ # Repeat for acme-ssl
+ tr2 = Test.AddTestRun(f'Positive acme-ssl healthchecks: {counter}')
+ tr2.MakeCurlCommand(f'-kv
https://127.0.0.1:{self._ts.Variables.ssl_port}/acme-ssl')
+ curl_acme_ssl = tr2.Processes.Default
+ if not self._ts_started:
+ curl_acme_ssl.StartBefore(self._ts)
+ self._ts_started = True
+ curl_acme_ssl.Streams.All += Testers.ContainsExpression('HTTP/2 200',
'Verify 200 response for /acme-ssl')
+
+ def _remove_acme_ssl(self) -> None:
+ '''Remove the acme-ssl file to trigger a file change detection.
+ :return: None
+ '''
+ tr = Test.AddTestRun('Remove acme-ssl file')
+ p = tr.Processes.Default
+ p.Command = f'rm {Test.RunDirectory}/acme-ssl && sleep 1'
+ tr.Processes.Default.ReturnCode = 0
+
+ def _expect_acme_ssl_404(self) -> None:
+ '''Expect a 404 response for the removed acme-ssl file.
+ :return: None
+ '''
+ tr = Test.AddTestRun('Expect 200 for acme after acme-ssl removal')
+ tr.MakeCurlCommand(f'-v
http://127.0.0.1:{self._ts.Variables.port}/acme')
+ curl_acme = tr.Processes.Default
+ if not self._ts_started:
+ curl_acme.StartBefore(self._ts)
+ self._ts_started = True
+ curl_acme.Streams.All += Testers.ContainsExpression('HTTP/1.1 200',
'Verify 200 response for /acme after acme-ssl removal')
+
+ tr2 = Test.AddTestRun('Expect 404 for acme-ssl after removal')
+ tr2.MakeCurlCommand(f'-kv
https://127.0.0.1:{self._ts.Variables.ssl_port}/acme-ssl')
+ curl_acme_ssl = tr2.Processes.Default
+ if not self._ts_started:
+ curl_acme_ssl.StartBefore(self._ts)
+ self._ts_started = True
+ curl_acme_ssl.Streams.All += Testers.ContainsExpression('HTTP/2 404',
'Verify 404 response for /acme-ssl after removal')
+
+ def _re_add_acme_ssl(self) -> None:
+ '''Re-add the acme-ssl file to restore the healthcheck.
+ :return: None
+ '''
+ tr = Test.AddTestRun('Re-add acme-ssl file')
+ tr.Setup.Copy('acme-ssl', Test.RunDirectory)
+ p = tr.Processes.Default
+ p.Command = 'sleep 1'
+ p.ReturnCode = 0
+
+
+# Instantiate the test
+TestFileChangeBehavior()
