From 6d6dd0239940e3ae7bc4166ed55d076ef7991435 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Fri, 8 Jul 2022 11:18:32 -0500 Subject: [PATCH 01/11] File change monitoring on s3_auth (#8905) --- configure.ac | 8 +- doc/admin-guide/plugins/s3_auth.en.rst | 8 +- .../testing/blackbox-testing.en.rst | 1 + include/ts/apidefs.h.in | 12 + include/ts/ts.h | 17 ++ include/tscore/ink_config.h.in | 1 + iocore/Makefile.am | 2 +- iocore/fs/FileChange.cc | 227 ++++++++++++++++++ iocore/fs/FileChange.h | 82 +++++++ iocore/fs/Makefile.am | 34 +++ plugins/s3_auth/s3_auth.cc | 193 ++++++++++++++- src/traffic_layout/info.cc | 1 + src/traffic_server/InkAPI.cc | 21 ++ src/traffic_server/Makefile.inc | 1 + src/traffic_server/traffic_server.cc | 4 + tests/README.md | 1 + .../s3_auth/gold/s3_auth_basic.gold | 34 +++ .../s3_auth/gold/traffic_server.gold | 2 + .../pluginTest/s3_auth/rules/region_map.conf | 17 ++ .../pluginTest/s3_auth/rules/v4-modified.conf | 20 ++ .../pluginTest/s3_auth/rules/v4.conf | 20 ++ .../s3_auth/s3_auth_watch_config.test.py | 87 +++++++ 22 files changed, 785 insertions(+), 8 deletions(-) create mode 100644 iocore/fs/FileChange.cc create mode 100644 iocore/fs/FileChange.h create mode 100644 iocore/fs/Makefile.am create mode 100644 tests/gold_tests/pluginTest/s3_auth/gold/s3_auth_basic.gold create mode 100644 tests/gold_tests/pluginTest/s3_auth/gold/traffic_server.gold create mode 100644 tests/gold_tests/pluginTest/s3_auth/rules/region_map.conf create mode 100644 tests/gold_tests/pluginTest/s3_auth/rules/v4-modified.conf create mode 100644 tests/gold_tests/pluginTest/s3_auth/rules/v4.conf create mode 100644 tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py diff --git a/configure.ac b/configure.ac index df4623d316b..bd20009e6ee 100644 --- a/configure.ac +++ b/configure.ac @@ -1818,6 +1818,10 @@ AC_LANG_POP([C++]) # Right now, the healthcheck plugins requires inotify_init (and friends) AM_CONDITIONAL([BUILD_HEALTHCHECK_PLUGIN], [ test "$ac_cv_func_inotify_init" = "yes" ]) +use_inotify=0 +AS_IF([test "x$ac_cv_func_inotify_init" = "xyes"], [use_inotify=1]) +AC_SUBST(use_inotify) + # # Check for tcmalloc, jemalloc and mimalloc TS_CHECK_TCMALLOC @@ -2262,7 +2266,8 @@ iocore_include_dirs="\ -I\$(abs_top_srcdir)/iocore/hostdb \ -I\$(abs_top_srcdir)/iocore/cache \ -I\$(abs_top_srcdir)/iocore/utils \ --I\$(abs_top_srcdir)/iocore/dns" +-I\$(abs_top_srcdir)/iocore/dns \ +-I\$(abs_top_srcdir)/iocore/fs" AC_SUBST([AM_CPPFLAGS]) AC_SUBST([AM_CFLAGS]) @@ -2310,6 +2315,7 @@ AC_CONFIG_FILES([ iocore/cache/Makefile iocore/dns/Makefile iocore/eventsystem/Makefile + iocore/fs/Makefile iocore/hostdb/Makefile iocore/net/Makefile iocore/net/quic/Makefile diff --git a/doc/admin-guide/plugins/s3_auth.en.rst b/doc/admin-guide/plugins/s3_auth.en.rst index 87f1a92097c..23da86ed698 100644 --- a/doc/admin-guide/plugins/s3_auth.en.rst +++ b/doc/admin-guide/plugins/s3_auth.en.rst @@ -44,7 +44,7 @@ Alternatively, you can store the access key and secret in an external configurat # remap.config - ... @plugin=s3_auth.so @pparam=--config @pparam=s3_auth_v2.config + ... @plugin=s3_auth.so @pparam=--config @pparam=s3_auth_v2.config @pparam=--watch-config @pparam=--ttl=5 Where ``s3.config`` could look like:: @@ -94,6 +94,8 @@ The ``s3_auth_v4.config`` config file could look like this:: v4-include-headers= v4-exclude-headers= v4-region-map=region_map.config + watch-config + ttl=20 Where the ``region_map.config`` defines the entry-point hostname to region mapping i.e.:: @@ -123,6 +125,10 @@ If ``--v4-include-headers`` is not specified all headers except those specified If ``--v4-include-headers`` is specified only the headers specified will be signed except those specified in ``--v4-exclude-headers`` +If ``--watch-config`` is specified, the plugin will reload the config file set in ``--config`` when it changes + +If ``--ttl`` is specified, the plugin will cache configs for the specified number of seconds. During the ttl period, manual config reloads and ``--watch-config`` will not cause the config to be updated. The default is 60 seconds. Setting ttl to zero causes all reloads to read from the config file. This option is useful if the config file is fetched from a service, and you wish to limit the fetch rate. + AWS Authentication version 2 ============================ diff --git a/doc/developer-guide/testing/blackbox-testing.en.rst b/doc/developer-guide/testing/blackbox-testing.en.rst index 25af57aec31..5ca6f3f579a 100644 --- a/doc/developer-guide/testing/blackbox-testing.en.rst +++ b/doc/developer-guide/testing/blackbox-testing.en.rst @@ -262,6 +262,7 @@ Condition Testing - TS_HAS_128BIT_CAS - TS_HAS_TESTS - TS_HAS_WCCP + - TS_USE_INOTIFY Examples: diff --git a/include/ts/apidefs.h.in b/include/ts/apidefs.h.in index 9baae2bfda0..fdeb648718f 100644 --- a/include/ts/apidefs.h.in +++ b/include/ts/apidefs.h.in @@ -554,6 +554,11 @@ typedef enum { TS_EVENT_SSL_CLIENT_HELLO = 60207, TS_EVENT_SSL_SECRET = 60208, + TS_EVENT_FILE_CREATED = 60300, + TS_EVENT_FILE_UPDATED = 60301, + TS_EVENT_FILE_DELETED = 60302, + TS_EVENT_FILE_IGNORED = 60303, + TS_EVENT_MGMT_UPDATE = 60300 } TSEvent; #define TS_EVENT_HTTP_READ_REQUEST_PRE_REMAP TS_EVENT_HTTP_PRE_REMAP /* backwards compat */ @@ -1460,6 +1465,13 @@ namespace ts } #endif +typedef int TSWatchDescriptor; +typedef enum { TS_WATCH_CREATE, TS_WATCH_DELETE, TS_WATCH_MODIFY } TSFileWatchKind; +typedef struct { + TSWatchDescriptor wd; + const char *name; +} TSFileWatchData; + #ifdef __cplusplus } #endif /* __cplusplus */ diff --git a/include/ts/ts.h b/include/ts/ts.h index 66af0983da1..2b1d19b6340 100644 --- a/include/ts/ts.h +++ b/include/ts/ts.h @@ -2755,6 +2755,23 @@ tsapi void TSHostStatusSet(const char *hostname, const size_t hostname_len, TSHo tsapi bool TSHttpTxnCntlGet(TSHttpTxn txnp, TSHttpCntlType ctrl); tsapi TSReturnCode TSHttpTxnCntlSet(TSHttpTxn txnp, TSHttpCntlType ctrl, bool data); +/* + * Get notified for file system events + * + * Currently, this only works in Linux using inotify. + * + * TODO: Fix multiple plugins watching the same path. + * + * The edata (a.k.a. cookie) field of the continuation handler will contain information + * depending on the type of file event. edata is always a pointer to a TSFileWatchData. + * If the event is TS_EVENT_FILE_CREATED, name is a pointer to a null-terminated string + * containing the file name. Otherwise, name is a nullptr. wd is the watch descriptor + * for the event. + * + */ +tsapi TSWatchDescriptor TSFileEventRegister(const char *filename, TSFileWatchKind kind, TSCont contp); +tsapi void TSFileEventUnRegister(TSWatchDescriptor wd); + #ifdef __cplusplus } #endif /* __cplusplus */ diff --git a/include/tscore/ink_config.h.in b/include/tscore/ink_config.h.in index 5b297efe7bf..b6e635f4f31 100644 --- a/include/tscore/ink_config.h.in +++ b/include/tscore/ink_config.h.in @@ -83,6 +83,7 @@ #define TS_HAS_TLS_EARLY_DATA @has_tls_early_data@ #define TS_HAS_TLS_SESSION_TICKET @has_tls_session_ticket@ #define TS_HAS_VERIFY_CERT_STORE @has_verify_cert_store@ +#define TS_USE_INOTIFY @use_inotify@ #define TS_USE_HRW_GEOIP @use_hrw_geoip@ #define TS_USE_HRW_MAXMINDDB @use_hrw_maxminddb@ diff --git a/iocore/Makefile.am b/iocore/Makefile.am index 5aae15ea552..f870d193ba5 100644 --- a/iocore/Makefile.am +++ b/iocore/Makefile.am @@ -16,4 +16,4 @@ # See the License for the specific language governing permissions and # limitations under the License. -SUBDIRS = eventsystem net aio dns hostdb utils cache +SUBDIRS = eventsystem net aio dns hostdb utils cache fs diff --git a/iocore/fs/FileChange.cc b/iocore/fs/FileChange.cc new file mode 100644 index 00000000000..864df401f39 --- /dev/null +++ b/iocore/fs/FileChange.cc @@ -0,0 +1,227 @@ +/** @file FileChange.cc + + Watch for file system changes. + + @section license License + + 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. + */ + +#include "FileChange.h" +#include "tscore/Diags.h" +#include "P_EventSystem.h" + +#include +#include +#include +#include + +// Globals +FileChangeManager fileChangeManager; +static constexpr auto TAG ATS_UNUSED = "FileChange"; + +// Wrap a continuation +class FileChangeCallback : public Continuation +{ +public: + explicit FileChangeCallback(Continuation *contp, TSEvent event) : Continuation(contp->mutex.get()), m_cont(contp), m_event(event) + { + SET_HANDLER(&FileChangeCallback::event_handler); + } + + int + event_handler(int, void *eventp) + { + Event *e = reinterpret_cast(eventp); + if (m_cont->mutex) { + MUTEX_TRY_LOCK(trylock, m_cont->mutex, this_ethread()); + if (!trylock.is_locked()) { + eventProcessor.schedule_in(this, HRTIME_MSECONDS(10), ET_TASK); + } else { + m_cont->handleEvent(m_event, e->cookie); + delete this; + } + } else { + m_cont->handleEvent(m_event, e->cookie); + delete this; + } + + return 0; + } + + std::string filename; // File name if the event is a file creation event. This is used in the cookie for a create event. + TSFileWatchData data; + +private: + Continuation *m_cont; + TSEvent m_event; +}; + +#if TS_USE_INOTIFY +static constexpr size_t INOTIFY_BUF_SIZE = 4096; + +static void +invoke(FileChangeCallback *cb) +{ + void *cookie = static_cast(&cb->data); + eventProcessor.schedule_imm(cb, ET_TASK, 1, cookie); +} + +void +FileChangeManager::process_file_event(struct inotify_event *event) +{ + std::shared_lock file_watches_read_lock(file_watches_mutex); + auto finfo_it = file_watches.find(event->wd); + if (finfo_it != file_watches.end()) { + TSEvent event_type = TS_EVENT_NONE; + const struct file_info &finfo = finfo_it->second; + Continuation *contp = finfo.contp; + + if (event->mask & (IN_DELETE_SELF | IN_MOVED_FROM)) { + Debug(TAG, "Delete file event (%d) on %s", event->mask, finfo.path.c_str()); + int rc2 = inotify_rm_watch(inotify_fd, event->wd); + if (rc2 == -1) { + Error("Failed to remove inotify watch on %s: %s (%d)", finfo.path.c_str(), strerror(errno), errno); + } + event_type = TS_EVENT_FILE_DELETED; + FileChangeCallback *cb = new FileChangeCallback(contp, event_type); + cb->data.wd = event->wd; + cb->data.name = nullptr; + invoke(cb); + } + + if (event->mask & (IN_CREATE | IN_MOVED_TO)) { + // Name may be padded with nul characters. Trim them. + auto len = strnlen(event->name, event->len); + std::string name{event->name, len}; + Debug(TAG, "Create file event (%d) on %s (wd = %d): %s", event->mask, finfo.path.c_str(), event->wd, name.c_str()); + event_type = TS_EVENT_FILE_CREATED; + + FileChangeCallback *cb = new FileChangeCallback(contp, event_type); + cb->filename = name; + cb->data.wd = event->wd; + cb->data.name = cb->filename.c_str(); + invoke(cb); + } + + if (event->mask & (IN_CLOSE_WRITE | IN_ATTRIB)) { + Debug(TAG, "Modify file event (%d) on %s (wd = %d)", event->mask, finfo.path.c_str(), event->wd); + event_type = TS_EVENT_FILE_UPDATED; + FileChangeCallback *cb = new FileChangeCallback(contp, event_type); + cb->data.wd = event->wd; + cb->data.name = nullptr; + invoke(cb); + } + + if (event->mask & (IN_IGNORED)) { + Debug(TAG, "Ignored file event (%d) on %s (wd = %d)", event->mask, finfo.path.c_str(), event->wd); + event_type = TS_EVENT_FILE_IGNORED; + FileChangeCallback *cb = new FileChangeCallback(contp, event_type); + cb->data.wd = event->wd; + cb->data.name = nullptr; + invoke(cb); + } + } +} +#endif + +void +FileChangeManager::init() +{ +#if TS_USE_INOTIFY + // TODO: auto configure based on whether inotify is available + inotify_fd = inotify_init1(IN_CLOEXEC); + if (inotify_fd == -1) { + Error("Failed to init inotify: %s (%d)", strerror(errno), errno); + return; + } + auto inotify_thread = [manager = this]() mutable { + for (;;) { + char inotify_buf[INOTIFY_BUF_SIZE]; + + // blocking read + ssize_t rc = read(manager->inotify_fd, inotify_buf, sizeof inotify_buf); + + if (rc == -1) { + Error("Failed to read inotify: %s (%d)", strerror(errno), errno); + if (errno == EINTR) { + continue; + } else { + break; + } + } + + ssize_t offset = 0; + while (offset < rc) { + struct inotify_event *event = reinterpret_cast(inotify_buf + offset); + + // Process file events + manager->process_file_event(event); + offset += sizeof(struct inotify_event) + event->len; + } + } + }; + poll_thread = std::thread(inotify_thread); + poll_thread.detach(); +#else + // Implement this +#endif +} + +watch_handle_t +FileChangeManager::add(const ts::file::path &path, TSFileWatchKind kind, Continuation *contp) +{ +#if TS_USE_INOTIFY + Debug(TAG, "Adding a watch on %s", path.c_str()); + watch_handle_t wd = 0; + + // Let the OS handle multiple watches on one file. + uint32_t mask = 0; + if (kind == TS_WATCH_CREATE) { + mask = IN_CREATE | IN_MOVED_TO | IN_ONLYDIR; + } else if (kind == TS_WATCH_DELETE) { + mask = IN_DELETE_SELF | IN_MOVED_FROM; + } else if (kind == TS_WATCH_MODIFY) { + mask = IN_CLOSE_WRITE | IN_ATTRIB; + } + wd = inotify_add_watch(inotify_fd, path.c_str(), mask); + if (wd == -1) { + Error("Failed to add file watch on %s: %s (%d)", path.c_str(), strerror(errno), errno); + return -1; + } else { + std::unique_lock file_watches_write_lock(file_watches_mutex); + file_watches[wd] = {path, contp}; + } + + Debug(TAG, "Watch handle = %d", wd); + return wd; +#else + Warning("File change notification is not supported on this OS."); + return 0; +#endif +} + +void +FileChangeManager::remove(watch_handle_t watch_handle) +{ +#if TS_USE_INOTIFY + Debug(TAG, "Deleting watch %d", watch_handle); + inotify_rm_watch(inotify_fd, watch_handle); + std::unique_lock file_watches_write_lock(file_watches_mutex); + file_watches.erase(watch_handle); +#endif +} diff --git a/iocore/fs/FileChange.h b/iocore/fs/FileChange.h new file mode 100644 index 00000000000..57aa17225e9 --- /dev/null +++ b/iocore/fs/FileChange.h @@ -0,0 +1,82 @@ +/** @file FileChange.h + + Watch for file system changes. + + @section license License + + 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. + */ + +#pragma once +#include "tscore/ink_config.h" + +#include +#include +#include +#include +#include +#include +#include "tscore/ts_file.h" +#include "P_EventSystem.h" + +#if TS_USE_INOTIFY +#include +#else +// implement this +#endif + +using watch_handle_t = int; + +// File watch info +struct file_info { + ts::file::path path; + Continuation *contp; +}; + +class FileChangeManager +{ +public: + FileChangeManager() {} + + void init(); + + /** + Add a file watch + + @return a watch handle, or -1 on error + */ + watch_handle_t add(const ts::file::path &path, TSFileWatchKind kind, Continuation *contp); + + /** + Remove a file watch + */ + void remove(watch_handle_t watch_handle); + +private: + std::thread poll_thread; + +#if TS_USE_INOTIFY + void process_file_event(struct inotify_event *event); + std::shared_mutex file_watches_mutex; + std::unordered_map file_watches; + int inotify_fd; +#else + // implement this +#endif +}; + +extern FileChangeManager fileChangeManager; diff --git a/iocore/fs/Makefile.am b/iocore/fs/Makefile.am new file mode 100644 index 00000000000..a851273eadc --- /dev/null +++ b/iocore/fs/Makefile.am @@ -0,0 +1,34 @@ +# Makefile.am for the traffic/iocore/fs hierarchy +# +# 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. + +AM_CPPFLAGS += \ + $(iocore_include_dirs) \ + -I$(abs_top_srcdir)/include \ + -I$(abs_top_srcdir)/lib \ + $(TS_INCLUDES) + +noinst_LIBRARIES = libinkfs.a + +libinkfs_a_SOURCES = \ + FileChange.cc \ + FileChange.h + +include $(top_srcdir)/build/tidy.mk + +clang-tidy-local: $(DIST_SOURCES) + $(CXX_Clang_Tidy) diff --git a/plugins/s3_auth/s3_auth.cc b/plugins/s3_auth/s3_auth.cc index df5c090779c..50498822a60 100644 --- a/plugins/s3_auth/s3_auth.cc +++ b/plugins/s3_auth/s3_auth.cc @@ -42,11 +42,13 @@ #include #include #include +#include #include #include #include #include "tscore/ink_config.h" +#include "tscore/ts_file.h" #include "aws_auth_v4.h" @@ -161,21 +163,22 @@ class ConfigCache // never indicate config is fresh when it isn't. std::atomic config; std::atomic load_time; + std::atomic ttl = 60; _ConfigData() {} - _ConfigData(S3Config *config_, time_t load_time_) : config(config_), load_time(load_time_) {} + _ConfigData(S3Config *config_, time_t load_time_, int ttl_) : config(config_), load_time(load_time_), ttl(ttl_) {} _ConfigData(_ConfigData &&lhs) { update_status = lhs.update_status.load(); config = lhs.config.load(); load_time = lhs.load_time.load(); + ttl = lhs.ttl.load(); } }; std::unordered_map _cache; - static const int _ttl = 60; }; ConfigCache gConfCache; @@ -185,6 +188,8 @@ ConfigCache gConfCache; // int event_handler(TSCont, TSEvent, void *); // Forward declaration int config_reloader(TSCont, TSEvent, void *); +int config_dir_watch(TSCont, TSEvent, void *); +int config_watch(TSCont, TSEvent, void *); class S3Config { @@ -197,6 +202,12 @@ class S3Config _conf_rld = TSContCreate(config_reloader, TSMutexCreate()); TSContDataSet(_conf_rld, static_cast(this)); + + _dir_watch = TSContCreate(config_dir_watch, TSMutexCreate()); + TSContDataSet(_dir_watch, static_cast(this)); + + _conf_watch = TSContCreate(config_watch, TSMutexCreate()); + TSContDataSet(_conf_watch, static_cast(this)); } } @@ -302,6 +313,10 @@ class S3Config TSfree(_conf_fname); _conf_fname = TSstrdup(src->_conf_fname); } + + if (src->_watch_config) { + _watch_config = src->_watch_config; + } } // Getters @@ -383,6 +398,18 @@ class S3Config return _conf_fname; } + bool + watch_config() const + { + return _watch_config; + } + + int + ttl() const + { + return _ttl; + } + int incr_conf_reload_count() { @@ -463,6 +490,18 @@ class S3Config _conf_fname = TSstrdup(s); } + void + set_watch_config() + { + _watch_config = true; + } + + void + set_ttl(const char *s) + { + _ttl = strtol(s, nullptr, 10); + } + void reset_conf_reload_count() { @@ -489,7 +528,70 @@ class S3Config _conf_rld_act = TSContScheduleOnPool(_conf_rld, delay * 1000, TS_THREAD_POOL_NET); } + void + start_watch_config() + { + std::unique_lock lock(wd_mutex); + ts::file::path fname{makeConfigPath(_conf_fname)}; + if (!_config_file_wd) { + _config_file_wd = TSFileEventRegister(fname.c_str(), TS_WATCH_MODIFY, _conf_watch); + if (_config_file_wd == -1) { + _config_file_wd.reset(); + TSDebug(PLUGIN_NAME, "Waiting for config file to be created: %s", fname.c_str()); + } else { + TSDebug(PLUGIN_NAME, "Watching config file: %s (%d)", fname.c_str(), _config_file_wd.value()); + } + } + + if (!_config_dir_wd) { + auto parent_dir = fname.parent_path(); + _config_dir_wd = TSFileEventRegister(parent_dir.c_str(), TS_WATCH_CREATE, _dir_watch); + if (_config_dir_wd == -1) { + _config_dir_wd.reset(); + TSError("s3_auth: failed to watch config file directory: %s", parent_dir.c_str()); + } else { + TSDebug(PLUGIN_NAME, "Watching config file directory: %s (%d)", parent_dir.c_str(), _config_file_wd.value()); + } + } + } + + void + stop_watch_config() + { + std::unique_lock lock(wd_mutex); + if (_config_file_wd) { + TSFileEventUnRegister(_config_file_wd.value()); + _config_file_wd.reset(); + } + + if (_config_dir_wd) { + TSFileEventUnRegister(_config_dir_wd.value()); + _config_dir_wd.reset(); + } + } + + void + config_file_watch_ignored(TSWatchDescriptor wd) + { + std::unique_lock lock(wd_mutex); + if (_config_file_wd == wd) { + TSFileEventUnRegister(_config_file_wd.value()); + _config_file_wd.reset(); + } + } + + void + config_dir_watch_ignored(TSWatchDescriptor wd) + { + std::unique_lock lock(wd_mutex); + if (_config_dir_wd == wd) { + TSFileEventUnRegister(_config_dir_wd.value()); + _config_dir_wd.reset(); + } + } + ts::shared_mutex reload_mutex; + ts::shared_mutex wd_mutex; private: char *_secret = nullptr; @@ -504,6 +606,8 @@ class S3Config bool _virt_host_modified = false; TSCont _cont = nullptr; TSCont _conf_rld = nullptr; + TSCont _conf_watch = nullptr; + TSCont _dir_watch = nullptr; TSAction _conf_rld_act = nullptr; StringSet _v4includeHeaders; bool _v4includeHeaders_modified = false; @@ -514,6 +618,10 @@ class S3Config long _expiration = 0; char *_conf_fname = nullptr; int _conf_reload_count = 0; + bool _watch_config = false; + std::optional _config_file_wd; + std::optional _config_dir_wd; + int _ttl = 60; }; bool @@ -577,6 +685,10 @@ S3Config::parse_config(const std::string &config_fname) set_region_map(val_str.c_str()); } else if (key_str == "expiration") { set_expiration(val_str.c_str()); + } else if (key_str == "ttl") { + set_ttl(val_str.c_str()); + } else if (key_str == "watch-config") { + set_watch_config(); } else { // ToDo: warnings? } @@ -611,7 +723,7 @@ ConfigCache::get(const char *fname) if (it != _cache.end()) { unsigned update_status = it->second.update_status; - if (tv.tv_sec > (it->second.load_time + _ttl)) { + if (tv.tv_sec > (it->second.load_time + it->second.ttl)) { if (!(update_status & 1) && it->second.update_status.compare_exchange_strong(update_status, update_status + 1)) { TSDebug(PLUGIN_NAME, "Configuration from %s is stale, reloading", config_fname.c_str()); s3 = new S3Config(false); // false == this config does not get the continuation @@ -624,10 +736,14 @@ ConfigCache::get(const char *fname) s3 = nullptr; TSAssert(!"Configuration parsing / caching failed"); } + TSDebug(PLUGIN_NAME, "Updated rule: access_key=%s, virtual_host=%s, version=%d", s3->keyid(), + s3->virt_host() ? "yes" : "no", s3->version()); delete it->second.config; it->second.config = s3; it->second.load_time = tv.tv_sec; + it->second.ttl = s3->ttl(); + TSDebug(PLUGIN_NAME, "Config ttl updated to %d seconds", it->second.ttl.load()); // Update is complete. ++it->second.update_status; @@ -649,10 +765,12 @@ ConfigCache::get(const char *fname) // Create a new cached file. s3 = new S3Config(false); // false == this config does not get the continuation - TSDebug(PLUGIN_NAME, "Parsing and caching configuration from %s, version:%d", config_fname.c_str(), s3->version()); + TSDebug(PLUGIN_NAME, "Parsing and caching configuration from %s", config_fname.c_str()); if (s3->parse_config(config_fname)) { s3->set_conf_fname(fname); - _cache.emplace(config_fname, _ConfigData(s3, tv.tv_sec)); + _cache.emplace(config_fname, _ConfigData(s3, tv.tv_sec, s3->ttl())); + _cache[config_fname].ttl = s3->ttl(); + TSDebug(PLUGIN_NAME, "Config ttl set to %d seconds", _cache[config_fname].ttl.load()); } else { delete s3; s3 = nullptr; @@ -1047,6 +1165,51 @@ cal_reload_delay(long time_diff) } } +int +config_dir_watch(TSCont cont, TSEvent event, void *edata) +{ + TSDebug(PLUGIN_NAME, "config directory watch handler"); + S3Config *s3 = static_cast(TSContDataGet(cont)); + TSAssert(edata != nullptr); + TSFileWatchData *fwd = reinterpret_cast(edata); + + if (event == TS_EVENT_FILE_IGNORED) { + TSDebug(PLUGIN_NAME, "Config directory lost. No longer watching for config changes."); + // Lost the config file's directory. We currently can't deal with this. Just stop watching for file changes. + s3->config_dir_watch_ignored(fwd->wd); + return TS_SUCCESS; + } + + TSAssert(event == TS_EVENT_FILE_CREATED); + TSDebug(PLUGIN_NAME, "File created: %s", fwd->name); + if (strncmp(s3->conf_fname(), fwd->name, strlen(s3->conf_fname())) == 0) { + TSDebug(PLUGIN_NAME, "config file created"); + config_reloader(cont, event, edata); + } + + return TS_SUCCESS; +} + +int +config_watch(TSCont cont, TSEvent event, void *edata) +{ + TSDebug(PLUGIN_NAME, "config watch handler"); + TSAssert(edata != nullptr); + TSFileWatchData *fwd = reinterpret_cast(edata); + + S3Config *s3 = static_cast(TSContDataGet(cont)); + if (event == TS_EVENT_FILE_IGNORED) { + TSDebug(PLUGIN_NAME, "Config file watch lost."); + // Probably deleted. Directory watch will see if it's re-created. + s3->config_file_watch_ignored(fwd->wd); + return TS_SUCCESS; + } + + config_reloader(cont, event, edata); + + return TS_SUCCESS; +} + int config_reloader(TSCont cont, TSEvent event, void *edata) { @@ -1084,6 +1247,12 @@ config_reloader(TSCont cont, TSEvent event, void *edata) } } + if (s3->watch_config()) { + s3->start_watch_config(); + } else { + s3->stop_watch_config(); + } + return TS_SUCCESS; } @@ -1124,6 +1293,8 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE {const_cast("v4-exclude-headers"), required_argument, nullptr, 'e'}, {const_cast("v4-region-map"), required_argument, nullptr, 'm'}, {const_cast("session_token"), required_argument, nullptr, 't'}, + {const_cast("watch-config"), no_argument, nullptr, 'w'}, + {const_cast("ttl"), no_argument, nullptr, 'T'}, {nullptr, no_argument, nullptr, '\0'}, }; @@ -1171,6 +1342,12 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE case 'm': s3->set_region_map(optarg); break; + case 'w': + s3->set_watch_config(); + break; + case 'T': + s3->set_ttl(optarg); + break; } if (opt == -1) { @@ -1207,6 +1384,12 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE } } + if (s3->watch_config()) { + s3->start_watch_config(); + } else { + s3->stop_watch_config(); + } + *ih = static_cast(s3); TSDebug(PLUGIN_NAME, "New rule: access_key=%s, virtual_host=%s, version=%d", s3->keyid(), s3->virt_host() ? "yes" : "no", s3->version()); diff --git a/src/traffic_layout/info.cc b/src/traffic_layout/info.cc index a54a1b10f49..7b911793da8 100644 --- a/src/traffic_layout/info.cc +++ b/src/traffic_layout/info.cc @@ -108,6 +108,7 @@ produce_features(bool json) print_feature("TS_USE_EPOLL", TS_USE_EPOLL, json); print_feature("TS_USE_KQUEUE", TS_USE_KQUEUE, json); print_feature("TS_USE_PORT", TS_USE_PORT, json); + print_feature("TS_USE_INOTIFY", TS_USE_INOTIFY, json); print_feature("TS_USE_POSIX_CAP", TS_USE_POSIX_CAP, json); print_feature("TS_USE_TPROXY", TS_USE_TPROXY, json); print_feature("TS_HAS_SO_MARK", TS_HAS_SO_MARK, json); diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc index f470bbb8d19..d73e6c6b2ad 100644 --- a/src/traffic_server/InkAPI.cc +++ b/src/traffic_server/InkAPI.cc @@ -25,6 +25,10 @@ #include #include +#if defined(__linux__) +#include +#endif + #include "tscore/ink_platform.h" #include "tscore/ink_base64.h" #include "tscore/PluginUserArgs.h" @@ -74,6 +78,7 @@ #include "I_Machine.h" #include "HttpProxyServerMain.h" #include "shared/overridable_txn_vars.h" +#include "FileChange.h" #include "ts/ts.h" @@ -10465,3 +10470,19 @@ TSDbgCtlCreate(char const *tag) return DbgCtl::_get_ptr(tag); } + +tsapi TSWatchDescriptor +TSFileEventRegister(const char *filename, TSFileWatchKind kind, TSCont contp) +{ + sdk_assert(sdk_sanity_check_iocore_structure(contp) == TS_SUCCESS); + sdk_assert(sdk_sanity_check_null_ptr((void *)this_ethread()) == TS_SUCCESS); + + Continuation *pCont = reinterpret_cast(contp); + return fileChangeManager.add(ts::file::path{filename}, kind, pCont); +} + +tsapi void +TSFileEventUnRegister(TSWatchDescriptor wd) +{ + fileChangeManager.remove(wd); +} diff --git a/src/traffic_server/Makefile.inc b/src/traffic_server/Makefile.inc index b908fea376a..c9f002ef3b9 100644 --- a/src/traffic_server/Makefile.inc +++ b/src/traffic_server/Makefile.inc @@ -82,6 +82,7 @@ traffic_server_traffic_server_LDADD = \ $(top_builddir)/iocore/net/libinknet.a \ $(top_builddir)/lib/records/librecords_p.a \ $(top_builddir)/iocore/eventsystem/libinkevent.a \ + $(top_builddir)/iocore/fs/libinkfs.a \ @HWLOC_LIBS@ \ @LIBPCRE@ \ @LIBRESOLV@ \ diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index 3a1a868a5dd..1214f550b85 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -103,6 +103,7 @@ extern "C" int plock(int); #include "HTTP2.h" #include "tscore/ink_config.h" #include "P_SSLClientUtils.h" +#include "FileChange.h" #if TS_USE_QUIC == 1 #include "Http3.h" @@ -2001,6 +2002,9 @@ main(int /* argc ATS_UNUSED */, const char **argv) proxyServerCheck.notify_one(); } + // Spawn a thread to do file system change notification + fileChangeManager.init(); + // !! ET_NET threads start here !! // This means any spawn scheduling must be done before this point. eventProcessor.start(num_of_net_threads, stacksize); diff --git a/tests/README.md b/tests/README.md index e28a6b90751..a97ba0221c3 100644 --- a/tests/README.md +++ b/tests/README.md @@ -321,6 +321,7 @@ ts.Disk.remap_config.AddLine( * TS_HAS_128BIT_CAS * TS_HAS_TESTS * TS_HAS_WCCP + * TS_USE_INOTIFY ### Example ```python diff --git a/tests/gold_tests/pluginTest/s3_auth/gold/s3_auth_basic.gold b/tests/gold_tests/pluginTest/s3_auth/gold/s3_auth_basic.gold new file mode 100644 index 00000000000..5ec9ba4d49e --- /dev/null +++ b/tests/gold_tests/pluginTest/s3_auth/gold/s3_auth_basic.gold @@ -0,0 +1,34 @@ +* Trying 127.0.0.1:``... +* Connected to 127.0.0.1 (127.0.0.1) port `` (#0) +> GET / HTTP/1.1 +> Host: www.example.com +> User-Agent: curl/`` +> Accept: */* +> +* Mark bundle as not supporting multiuse +< HTTP/1.1 200 OK +< Date: `` +< Age: 0 +< Transfer-Encoding: chunked +< Connection: keep-alive +< Server: `` +< +{ [5 bytes data] +* Connection #0 to host 127.0.0.1 left intact +* Trying 127.0.0.1:``... +* Connected to 127.0.0.1 (127.0.0.1) port `` (#0) +> GET / HTTP/1.1 +> Host: www.example.com +> User-Agent: curl/`` +> Accept: */* +> +* Mark bundle as not supporting multiuse +< HTTP/1.1 200 OK +< Date: `` +< Age: 0 +< Transfer-Encoding: chunked +< Connection: keep-alive +< Server: `` +< +{ [5 bytes data] +* Connection #0 to host 127.0.0.1 left intact diff --git a/tests/gold_tests/pluginTest/s3_auth/gold/traffic_server.gold b/tests/gold_tests/pluginTest/s3_auth/gold/traffic_server.gold new file mode 100644 index 00000000000..4c41e8d8c9e --- /dev/null +++ b/tests/gold_tests/pluginTest/s3_auth/gold/traffic_server.gold @@ -0,0 +1,2 @@ +`` DIAG: (s3_auth) Updated rule: access_key=1111111, virtual_host=no, version=4 +`` diff --git a/tests/gold_tests/pluginTest/s3_auth/rules/region_map.conf b/tests/gold_tests/pluginTest/s3_auth/rules/region_map.conf new file mode 100644 index 00000000000..1f663336a8b --- /dev/null +++ b/tests/gold_tests/pluginTest/s3_auth/rules/region_map.conf @@ -0,0 +1,17 @@ +# +# 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. +127.0.0.1 : us-east-1 diff --git a/tests/gold_tests/pluginTest/s3_auth/rules/v4-modified.conf b/tests/gold_tests/pluginTest/s3_auth/rules/v4-modified.conf new file mode 100644 index 00000000000..165d0b96309 --- /dev/null +++ b/tests/gold_tests/pluginTest/s3_auth/rules/v4-modified.conf @@ -0,0 +1,20 @@ +# +# 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. +access_key=1111111 +secret_key=5555555 +version=4 +ttl=0 diff --git a/tests/gold_tests/pluginTest/s3_auth/rules/v4.conf b/tests/gold_tests/pluginTest/s3_auth/rules/v4.conf new file mode 100644 index 00000000000..822b5346187 --- /dev/null +++ b/tests/gold_tests/pluginTest/s3_auth/rules/v4.conf @@ -0,0 +1,20 @@ +# +# 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. +access_key=1234567 +secret_key=9999999 +version=4 +ttl=0 diff --git a/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py b/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py new file mode 100644 index 00000000000..1a56543409c --- /dev/null +++ b/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py @@ -0,0 +1,87 @@ +''' +Test s3_auth config change watch function +''' +# 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. + +Test.ContinueOnFail = True + +Test.SkipUnless( + Condition.HasATSFeature('TS_USE_INOTIFY'), +) + +ts = Test.MakeATSProcess("ts") +server = Test.MakeOriginServer("server") + +Test.testName = "s3_auth: watch config" + +# define the request header and the desired response header +request_header = { + "headers": "GET / HTTP/1.1\r\nHost: www.example.com\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" +} + +# desired response form the origin server +response_header = { + "headers": "HTTP/1.1 200 OK\r\nConnection: close\r\n\r\n", + "timestamp": "1469733493.993", + "body": "" +} + +# add request/response +server.addResponse("sessionlog.log", request_header, response_header) + +ts.Disk.records_config.update({ + 'proxy.config.diags.debug.enabled': 1, + 'proxy.config.diags.debug.tags': 'FileChange|s3_auth', +}) + +ts.Setup.CopyAs('rules/v4.conf', Test.RunDirectory) +ts.Setup.CopyAs('rules/v4-modified.conf', Test.RunDirectory) +ts.Setup.CopyAs('rules/region_map.conf', Test.RunDirectory) + +ts.Disk.remap_config.AddLine( + 'map http://www.example.com http://127.0.0.1:{0} \ + @plugin=s3_auth.so \ + @pparam=--config @pparam={1}/v4.conf \ + @pparam=--v4-region-map @pparam={1}/region_map.conf \ + @pparam=--watch-config \ + ' + .format(server.Variables.Port, Test.RunDirectory) +) + +# Commands to get the following response headers +# 1. make a request +# 2. modify the config +# 3. make another request +curlRequest = ( + 'curl -s -v -H "Host: www.example.com" http://127.0.0.1:{0};' + 'sleep 1; cp {1}/v4-modified.conf {1}/v4.conf;' + 'sleep 1; curl -s -v -H "Host: www.example.com" http://127.0.0.1:{0};' +) + +# Test Case +tr = Test.AddTestRun() +tr.Processes.Default.Command = curlRequest.format(ts.Variables.port, Test.RunDirectory) +tr.Processes.Default.ReturnCode = 0 +tr.Processes.Default.StartBefore(server) +tr.Processes.Default.StartBefore(ts) +tr.Processes.Default.Streams.stderr = "gold/s3_auth_basic.gold" +tr.StillRunningAfter = server + +ts.Streams.stderr = "gold/traffic_server.gold" +ts.ReturnCode = 0 From adf98d3c983420c9ed519f523c2097f0648f0f73 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Wed, 13 Jul 2022 15:46:34 -0500 Subject: [PATCH 02/11] Fix autest 10.0 has a change where ATS logs are sent to traffic.log by default. --- .../gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py b/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py index 1a56543409c..dac3d8caba2 100644 --- a/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py +++ b/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py @@ -83,5 +83,5 @@ tr.Processes.Default.Streams.stderr = "gold/s3_auth_basic.gold" tr.StillRunningAfter = server -ts.Streams.stderr = "gold/traffic_server.gold" +ts.Disk.traffic_out.Content = "gold/traffic_server.gold" ts.ReturnCode = 0 From 109247b1bf9b54ddad6ab5a3cbb58d58fefb2f8c Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Wed, 13 Jul 2022 19:02:04 -0500 Subject: [PATCH 03/11] Fix merge error Move file monitoring events so they're not conflicting with MGMT. --- include/ts/apidefs.h.in | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/include/ts/apidefs.h.in b/include/ts/apidefs.h.in index fdeb648718f..faf475875d4 100644 --- a/include/ts/apidefs.h.in +++ b/include/ts/apidefs.h.in @@ -554,12 +554,12 @@ typedef enum { TS_EVENT_SSL_CLIENT_HELLO = 60207, TS_EVENT_SSL_SECRET = 60208, - TS_EVENT_FILE_CREATED = 60300, - TS_EVENT_FILE_UPDATED = 60301, - TS_EVENT_FILE_DELETED = 60302, - TS_EVENT_FILE_IGNORED = 60303, + TS_EVENT_MGMT_UPDATE = 60300, - TS_EVENT_MGMT_UPDATE = 60300 + TS_EVENT_FILE_CREATED = 60400, + TS_EVENT_FILE_UPDATED = 60401, + TS_EVENT_FILE_DELETED = 60402, + TS_EVENT_FILE_IGNORED = 60403 } TSEvent; #define TS_EVENT_HTTP_READ_REQUEST_PRE_REMAP TS_EVENT_HTTP_PRE_REMAP /* backwards compat */ From c63e72f9f1aad5310dbbba0fb8b1675101a23e96 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Thu, 4 Aug 2022 16:09:46 -0500 Subject: [PATCH 04/11] Initial BSD impl --- iocore/fs/FileChange.cc | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/iocore/fs/FileChange.cc b/iocore/fs/FileChange.cc index 864df401f39..5c0b9902414 100644 --- a/iocore/fs/FileChange.cc +++ b/iocore/fs/FileChange.cc @@ -24,11 +24,16 @@ #include "FileChange.h" #include "tscore/Diags.h" #include "P_EventSystem.h" +#include "tscore/ink_assert.h" #include +#include #include #include #include +#include +#include +#include // Globals FileChangeManager fileChangeManager; @@ -177,6 +182,11 @@ FileChangeManager::init() }; poll_thread = std::thread(inotify_thread); poll_thread.detach(); +#elif TS_USE_KQUEUE + auto kq = kqueue(); + if (kq < 0) { + Fatal("Failed to init kqueue: %s.", strerror(errno)); + } #else // Implement this #endif @@ -197,6 +207,9 @@ FileChangeManager::add(const ts::file::path &path, TSFileWatchKind kind, Continu mask = IN_DELETE_SELF | IN_MOVED_FROM; } else if (kind == TS_WATCH_MODIFY) { mask = IN_CLOSE_WRITE | IN_ATTRIB; + } else { + // Shouldn't get here + ink_release_assert(false); } wd = inotify_add_watch(inotify_fd, path.c_str(), mask); if (wd == -1) { @@ -209,6 +222,25 @@ FileChangeManager::add(const ts::file::path &path, TSFileWatchKind kind, Continu Debug(TAG, "Watch handle = %d", wd); return wd; +#elif TS_USE_KQUEUE + auto fd = open(path.c_str(), O_EVTONLY); + if (fd <= 0) { + Error("Failed to open %s for monitoring: %s.", path.c_str(), strerror(errno)); + return -1; + } + + unsigned int mask = 0; + if (kind == TS_WATCH_CREATE) { + mask = NOTE_WRITE; + } else if (kind == TS_WATCH_DELETE) { + mask = NOTE_DELETE | NOTE_RENAME; + } else if (kind == TS_WATCH_MODIFY) { + mask = NOTE_WRITE | NOTE_EXTEND | NOTE_ATTRIB; + } else { + // Shouldn't get here + ink_release_assert(false); + } + EV_SET() #else Warning("File change notification is not supported on this OS."); return 0; From fa6eb6ae8e3eacc67f95a71d18153211a93c766a Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Mon, 15 Aug 2022 17:45:39 -0500 Subject: [PATCH 05/11] Add kqueue support --- include/ts/ts.h | 8 +- iocore/fs/FileChange.cc | 227 +++++++++++++--- iocore/fs/FileChange.h | 29 +- plugins/s3_auth/s3_auth.cc | 249 +++++++++--------- .../s3_auth/s3_auth_watch_config.test.py | 2 +- 5 files changed, 336 insertions(+), 179 deletions(-) diff --git a/include/ts/ts.h b/include/ts/ts.h index 2b1d19b6340..2181ef29df7 100644 --- a/include/ts/ts.h +++ b/include/ts/ts.h @@ -2758,15 +2758,13 @@ tsapi TSReturnCode TSHttpTxnCntlSet(TSHttpTxn txnp, TSHttpCntlType ctrl, bool da /* * Get notified for file system events * - * Currently, this only works in Linux using inotify. - * * TODO: Fix multiple plugins watching the same path. * * The edata (a.k.a. cookie) field of the continuation handler will contain information * depending on the type of file event. edata is always a pointer to a TSFileWatchData. - * If the event is TS_EVENT_FILE_CREATED, name is a pointer to a null-terminated string - * containing the file name. Otherwise, name is a nullptr. wd is the watch descriptor - * for the event. + * If the event is TS_EVENT_FILE_CREATED, and ATS is running on Linux, name is a pointer + * to a null-terminated string containing the file name. Otherwise, name is a nullptr. + * wd is the watch descriptor for the event. * */ tsapi TSWatchDescriptor TSFileEventRegister(const char *filename, TSFileWatchKind kind, TSCont contp); diff --git a/iocore/fs/FileChange.cc b/iocore/fs/FileChange.cc index 5c0b9902414..370f7674a66 100644 --- a/iocore/fs/FileChange.cc +++ b/iocore/fs/FileChange.cc @@ -33,12 +33,35 @@ #include #include #include +#include #include +#include // Globals FileChangeManager fileChangeManager; static constexpr auto TAG ATS_UNUSED = "FileChange"; +#if TS_USE_KQUEUE +using namespace std::chrono_literals; + +// When using kqueue, new watches will take effect after at most this much time. +// This value should be greater than 0. Smaller values cost more CPU. +static constexpr auto latency = 1s; + +static constexpr timespec +chrono_to_timespec(std::chrono::nanoseconds duration) +{ + if (duration <= 0s) { + duration = 1s; + } + + auto seconds = std::chrono::duration_cast(duration); + duration -= seconds; + return timespec{seconds.count(), duration.count()}; +} + +#endif + // Wrap a continuation class FileChangeCallback : public Continuation { @@ -76,9 +99,6 @@ class FileChangeCallback : public Continuation TSEvent m_event; }; -#if TS_USE_INOTIFY -static constexpr size_t INOTIFY_BUF_SIZE = 4096; - static void invoke(FileChangeCallback *cb) { @@ -86,8 +106,11 @@ invoke(FileChangeCallback *cb) eventProcessor.schedule_imm(cb, ET_TASK, 1, cookie); } +#if TS_USE_INOTIFY +static constexpr size_t INOTIFY_BUF_SIZE = 4096; + void -FileChangeManager::process_file_event(struct inotify_event *event) +FileChangeManager::inotify_process_event(struct inotify_event *event) { std::shared_lock file_watches_read_lock(file_watches_mutex); auto finfo_it = file_watches.find(event->wd); @@ -98,10 +121,6 @@ FileChangeManager::process_file_event(struct inotify_event *event) if (event->mask & (IN_DELETE_SELF | IN_MOVED_FROM)) { Debug(TAG, "Delete file event (%d) on %s", event->mask, finfo.path.c_str()); - int rc2 = inotify_rm_watch(inotify_fd, event->wd); - if (rc2 == -1) { - Error("Failed to remove inotify watch on %s: %s (%d)", finfo.path.c_str(), strerror(errno), errno); - } event_type = TS_EVENT_FILE_DELETED; FileChangeCallback *cb = new FileChangeCallback(contp, event_type); cb->data.wd = event->wd; @@ -142,6 +161,115 @@ FileChangeManager::process_file_event(struct inotify_event *event) } } } +#elif TS_USE_KQUEUE +static void +kqueue_make_event(int fd, const FileChangeInfo &info, struct kevent *event) +{ + unsigned int mask = 0; + if (info.kind == TS_WATCH_CREATE) { + mask = NOTE_WRITE | NOTE_DELETE | NOTE_RENAME; + } else if (info.kind == TS_WATCH_DELETE) { + mask = NOTE_DELETE | NOTE_RENAME; + } else if (info.kind == TS_WATCH_MODIFY) { + mask = NOTE_WRITE | NOTE_DELETE | NOTE_RENAME; + } else { + // Shouldn't get here + ink_release_assert(false); + } + EV_SET(event, fd, EVFILT_VNODE, EV_ADD | EV_CLEAR, mask, 0, (void *)(uintptr_t)fd); +} + +void +FileChangeManager::kqueue_prepare_events() +{ + if (file_watches_dirty) { + // Make events for kqueue to monitor + Debug(TAG, "Updating kqueue event list."); + std::unique_lock file_watches_lock{file_watches_mutex}; // unique lock because file_watches_dirty will be written + auto nwatches = file_watches.size(); + events_to_monitor.resize(nwatches); + events_from_kqueue.resize(nwatches); + int i = 0; + for (const auto &[wd, info] : file_watches) { + // Add a file event to the list of events to monitor + kqueue_make_event(wd, info, &events_to_monitor[i]); + i++; + } + file_watches_dirty = false; + } +} + +int +FileChangeManager::kqueue_wait_for_events() +{ + // Wait for kqueue events + if (events_to_monitor.empty()) { + std::this_thread::sleep_for(latency); + } else { + // I couldn't find a clear answer as to whether the timeout value can be modified by kevent (e.g. on an interrupt) + constexpr timespec latency_timespec = chrono_to_timespec(latency); + return kevent(kq, events_to_monitor.data(), events_to_monitor.size(), events_from_kqueue.data(), events_from_kqueue.size(), + &latency_timespec); + } + + return 0; +} + +void +FileChangeManager::kqueue_process_event(const struct kevent &event) +{ + std::shared_lock file_watches_read_lock(file_watches_mutex); + auto fd64 = reinterpret_cast(event.udata); + auto fd = static_cast(fd64); // Intentionally truncating to an int. Casting twice to suppress compiler warnings. + auto finfo_it = file_watches.find(fd); + if (finfo_it != file_watches.end()) { + TSEvent event_type = TS_EVENT_NONE; + const FileChangeInfo &finfo = finfo_it->second; + Continuation *contp = finfo.contp; + + if (event.fflags & (NOTE_DELETE | NOTE_RENAME)) { + Debug(TAG, "Delete file event (%d) on %s", event.fflags, finfo.path.c_str()); + if (finfo.kind == TS_WATCH_DELETE) { + event_type = TS_EVENT_FILE_DELETED; + FileChangeCallback *cb = new FileChangeCallback(contp, event_type); + cb->data.wd = fd; + cb->data.name = nullptr; + invoke(cb); + } + + // kqueue doesn't notify us if a file watch no longer applies, so we do it. + event_type = TS_EVENT_FILE_IGNORED; + FileChangeCallback *cb = new FileChangeCallback(contp, event_type); + cb->data.wd = fd; + cb->data.name = nullptr; + invoke(cb); + } + + if (event.fflags & (NOTE_WRITE) && finfo.kind == TS_WATCH_CREATE) { + Debug(TAG, "Create file event (%d) on %s (wd = %d)", event.fflags, finfo.path.c_str(), fd); + event_type = TS_EVENT_FILE_CREATED; + + FileChangeCallback *cb = new FileChangeCallback(contp, event_type); + cb->data.wd = fd; + cb->data.name = nullptr; // a kqueue create has no name + invoke(cb); + } + + if (event.fflags & (NOTE_WRITE) && finfo.kind == TS_WATCH_MODIFY) { + Debug(TAG, "Modify file event (%d) on %s (wd = %d)", event.fflags, finfo.path.c_str(), fd); + event_type = TS_EVENT_FILE_UPDATED; + FileChangeCallback *cb = new FileChangeCallback(contp, event_type); + cb->data.wd = fd; + cb->data.name = nullptr; + invoke(cb); + } + } + if (event.flags & EV_ERROR) { + Error("kqueue error: %s (%" PRIxPTR ")", strerror(event.data), event.data); + return; + } +} + #endif void @@ -175,7 +303,7 @@ FileChangeManager::init() struct inotify_event *event = reinterpret_cast(inotify_buf + offset); // Process file events - manager->process_file_event(event); + manager->inotify_process_event(event); offset += sizeof(struct inotify_event) + event->len; } } @@ -183,10 +311,24 @@ FileChangeManager::init() poll_thread = std::thread(inotify_thread); poll_thread.detach(); #elif TS_USE_KQUEUE - auto kq = kqueue(); - if (kq < 0) { - Fatal("Failed to init kqueue: %s.", strerror(errno)); - } + auto kqueue_thread = [manager = this]() mutable { + manager->kq = kqueue(); + if (manager->kq < 0) { + Fatal("Failed to init kqueue: %s.", strerror(errno)); + } + for (;;) { + manager->kqueue_prepare_events(); + int event_count = manager->kqueue_wait_for_events(); + if (event_count == -1) { + Error("kqueue error: %s", strerror(errno)); + } + for (int i = 0; i < event_count; i++) { + manager->kqueue_process_event(manager->events_from_kqueue[i]); + } + } + }; + poll_thread = std::thread(kqueue_thread); + poll_thread.detach(); #else // Implement this #endif @@ -195,10 +337,11 @@ FileChangeManager::init() watch_handle_t FileChangeManager::add(const ts::file::path &path, TSFileWatchKind kind, Continuation *contp) { -#if TS_USE_INOTIFY - Debug(TAG, "Adding a watch on %s", path.c_str()); watch_handle_t wd = 0; + std::unique_lock file_watches_write_lock{file_watches_mutex}; + Debug(TAG, "Adding a watch on %s", path.c_str()); +#if TS_USE_INOTIFY // Let the OS handle multiple watches on one file. uint32_t mask = 0; if (kind == TS_WATCH_CREATE) { @@ -215,45 +358,53 @@ FileChangeManager::add(const ts::file::path &path, TSFileWatchKind kind, Continu if (wd == -1) { Error("Failed to add file watch on %s: %s (%d)", path.c_str(), strerror(errno), errno); return -1; - } else { - std::unique_lock file_watches_write_lock(file_watches_mutex); - file_watches[wd] = {path, contp}; } Debug(TAG, "Watch handle = %d", wd); - return wd; #elif TS_USE_KQUEUE - auto fd = open(path.c_str(), O_EVTONLY); - if (fd <= 0) { - Error("Failed to open %s for monitoring: %s.", path.c_str(), strerror(errno)); - return -1; - } + int o_flags = 0; - unsigned int mask = 0; +#ifdef O_SYMLINK + o_flags |= O_SYMLINK; +#endif + +#ifdef O_EVTONLY + // The descriptor is requested for event notifications only. + o_flags |= O_EVTONLY; +#else + o_flags |= O_RDONLY; +#endif + +#ifdef O_DIRECTORY if (kind == TS_WATCH_CREATE) { - mask = NOTE_WRITE; - } else if (kind == TS_WATCH_DELETE) { - mask = NOTE_DELETE | NOTE_RENAME; - } else if (kind == TS_WATCH_MODIFY) { - mask = NOTE_WRITE | NOTE_EXTEND | NOTE_ATTRIB; - } else { - // Shouldn't get here - ink_release_assert(false); + // file creation can only be monitored from the directory above + o_flags |= O_DIRECTORY; + } +#endif + + wd = open(path.c_str(), o_flags); + if (wd <= 0) { + Error("Failed to open %s for monitoring: %s.", path.c_str(), strerror(errno)); + return -1; } - EV_SET() #else Warning("File change notification is not supported on this OS."); - return 0; #endif + file_watches.try_emplace(wd, kind, path, contp); + file_watches_dirty = true; + return wd; } void FileChangeManager::remove(watch_handle_t watch_handle) { -#if TS_USE_INOTIFY + std::unique_lock file_watches_write_lock(file_watches_mutex); Debug(TAG, "Deleting watch %d", watch_handle); +#if TS_USE_INOTIFY inotify_rm_watch(inotify_fd, watch_handle); - std::unique_lock file_watches_write_lock(file_watches_mutex); - file_watches.erase(watch_handle); +#elif TS_USE_KQUEUE + close(watch_handle); #endif + file_watches.erase(watch_handle); + file_watches_dirty = true; } diff --git a/iocore/fs/FileChange.h b/iocore/fs/FileChange.h index 57aa17225e9..1437c0d36e9 100644 --- a/iocore/fs/FileChange.h +++ b/iocore/fs/FileChange.h @@ -22,16 +22,20 @@ */ #pragma once +#include "ts/apidefs.h" #include "tscore/ink_config.h" #include #include #include #include +#include #include #include #include "tscore/ts_file.h" #include "P_EventSystem.h" +#include +#include #if TS_USE_INOTIFY #include @@ -42,7 +46,14 @@ using watch_handle_t = int; // File watch info -struct file_info { +class FileChangeInfo +{ +public: + FileChangeInfo(TSFileWatchKind kind, ts::file::path path, Continuation *contp) : kind{kind}, path{std::move(path)}, contp{contp} + { + } + + TSFileWatchKind kind; ts::file::path path; Continuation *contp; }; @@ -68,12 +79,22 @@ class FileChangeManager private: std::thread poll_thread; + std::unordered_map file_watches; // protected by file_watches_mutex + std::shared_mutex file_watches_mutex; #if TS_USE_INOTIFY - void process_file_event(struct inotify_event *event); - std::shared_mutex file_watches_mutex; - std::unordered_map file_watches; + void inotify_process_event(struct inotify_event *event); int inotify_fd; +#elif TS_USE_KQUEUE + bool file_watches_dirty; // protected by file_watches_mutex + + std::vector events_to_monitor; + std::vector events_from_kqueue; + + int kq; + void kqueue_prepare_events(); + int kqueue_wait_for_events(); + void kqueue_process_event(const struct kevent &event); #else // implement this #endif diff --git a/plugins/s3_auth/s3_auth.cc b/plugins/s3_auth/s3_auth.cc index 50498822a60..74f958bca1c 100644 --- a/plugins/s3_auth/s3_auth.cc +++ b/plugins/s3_auth/s3_auth.cc @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -140,49 +141,6 @@ loadRegionMap(StringMap &m, const String &filename) return true; } -/////////////////////////////////////////////////////////////////////////////// -// Cache for the secrets file, to avoid reading / loading them repeatedly on -// a reload of remap.config. This gets cached for 60s (not configurable). -// -class S3Config; - -class ConfigCache -{ -public: - S3Config *get(const char *fname); - -private: - struct _ConfigData { - // This is incremented before and after cnf and load_time are set. - // Thus, an odd value indicates an update is in progress. - std::atomic update_status{0}; - - // A config from a file and the last time it was loaded. - // config should be written before load_time. That way, - // if config is read after load_time, the load time will - // never indicate config is fresh when it isn't. - std::atomic config; - std::atomic load_time; - std::atomic ttl = 60; - - _ConfigData() {} - - _ConfigData(S3Config *config_, time_t load_time_, int ttl_) : config(config_), load_time(load_time_), ttl(ttl_) {} - - _ConfigData(_ConfigData &&lhs) - { - update_status = lhs.update_status.load(); - config = lhs.config.load(); - load_time = lhs.load_time.load(); - ttl = lhs.ttl.load(); - } - }; - - std::unordered_map _cache; -}; - -ConfigCache gConfCache; - /////////////////////////////////////////////////////////////////////////////// // One configuration setup // @@ -220,6 +178,7 @@ class S3Config TSfree(_conf_fname); if (_conf_rld_act) { TSActionCancel(_conf_rld_act); + _conf_rld_act = nullptr; } if (_conf_rld) { TSContDestroy(_conf_rld); @@ -227,6 +186,14 @@ class S3Config if (_cont) { TSContDestroy(_cont); } + + if (_dir_watch) { + TSContDestroy(_dir_watch); + } + + if (_conf_watch) { + TSContDestroy(_conf_watch); + } } // Is this configuration usable? @@ -404,12 +371,6 @@ class S3Config return _watch_config; } - int - ttl() const - { - return _ttl; - } - int incr_conf_reload_count() { @@ -497,15 +458,15 @@ class S3Config } void - set_ttl(const char *s) + reset_conf_reload_count() { - _ttl = strtol(s, nullptr, 10); + _conf_reload_count = 0; } void - reset_conf_reload_count() + clear_reload_action() { - _conf_reload_count = 0; + _conf_rld_act = nullptr; } // Parse configs from an external file @@ -523,7 +484,9 @@ class S3Config schedule_conf_reload(long delay) { if (_conf_rld_act != nullptr && !TSActionDone(_conf_rld_act)) { + TSDebug(PLUGIN_NAME, "_conf_rld_act = %p, cancelling...", _conf_rld_act); TSActionCancel(_conf_rld_act); + _conf_rld_act = nullptr; } _conf_rld_act = TSContScheduleOnPool(_conf_rld, delay * 1000, TS_THREAD_POOL_NET); } @@ -548,7 +511,7 @@ class S3Config _config_dir_wd = TSFileEventRegister(parent_dir.c_str(), TS_WATCH_CREATE, _dir_watch); if (_config_dir_wd == -1) { _config_dir_wd.reset(); - TSError("s3_auth: failed to watch config file directory: %s", parent_dir.c_str()); + TSError("[%s]: failed to watch config file directory: %s", PLUGIN_NAME, parent_dir.c_str()); } else { TSDebug(PLUGIN_NAME, "Watching config file directory: %s (%d)", parent_dir.c_str(), _config_file_wd.value()); } @@ -590,6 +553,18 @@ class S3Config } } + void + set_watch_retry(int retries) + { + watch_retry_count = retries; + } + + int + decrement_retry() + { + return --watch_retry_count; + } + ts::shared_mutex reload_mutex; ts::shared_mutex wd_mutex; @@ -621,9 +596,47 @@ class S3Config bool _watch_config = false; std::optional _config_file_wd; std::optional _config_dir_wd; - int _ttl = 60; + std::atomic watch_retry_count = 0; }; +static bool +try_flock(int fd) +{ + struct flock lck; + lck.l_start = 0; + lck.l_len = 0; + lck.l_type = F_RDLCK; + lck.l_whence = SEEK_SET; + auto res = fcntl(fd, F_SETLK, &lck); + if (res < 0) { + switch (errno) { + case EAGAIN: + case EINTR: + TSDebug(PLUGIN_NAME, "flock is busy."); + break; + default: + TSError("[%s] Failed to flock(): %s!", PLUGIN_NAME, strerror(errno)); + break; + } + return false; + } + return true; +} + +static void +funlock(int fd) +{ + struct flock lck; + lck.l_start = 0; + lck.l_len = 0; + lck.l_type = F_UNLCK; + lck.l_whence = SEEK_SET; + auto res = fcntl(fd, F_SETLK, &lck); + if (res < 0) { + TSError("[%s] Failed to funlock(): %s!", PLUGIN_NAME, strerror(errno)); + } +} + bool S3Config::parse_config(const std::string &config_fname) { @@ -632,13 +645,19 @@ S3Config::parse_config(const std::string &config_fname) return false; } else { char line[512]; // These are long lines ... - FILE *file = fopen(config_fname.c_str(), "r"); + int fd = open(config_fname.c_str(), O_RDONLY); + FILE *file = fdopen(fd, "r"); if (nullptr == file) { TSError("[%s] unable to open %s", PLUGIN_NAME, config_fname.c_str()); return false; } + if (!try_flock(fd)) { + fclose(file); + return false; + } + while (fgets(line, sizeof(line), file) != nullptr) { char *pos1, *pos2; @@ -663,7 +682,7 @@ S3Config::parse_config(const std::string &config_fname) // Identify the keys (and values if appropriate) std::string key_val(pos2, pos1 - pos2 + 1); - size_t eq_pos = key_val.find_first_of("="); + size_t eq_pos = key_val.find_first_of('='); std::string key_str = trimWhiteSpaces(key_val.substr(0, eq_pos == String::npos ? key_val.size() : eq_pos)); std::string val_str = eq_pos == String::npos ? "" : trimWhiteSpaces(key_val.substr(eq_pos + 1, key_val.size())); @@ -685,8 +704,6 @@ S3Config::parse_config(const std::string &config_fname) set_region_map(val_str.c_str()); } else if (key_str == "expiration") { set_expiration(val_str.c_str()); - } else if (key_str == "ttl") { - set_ttl(val_str.c_str()); } else if (key_str == "watch-config") { set_watch_config(); } else { @@ -694,6 +711,7 @@ S3Config::parse_config(const std::string &config_fname) } } + funlock(fd); fclose(file); } @@ -707,10 +725,10 @@ S3Config::parse_config(const std::string &config_fname) // has to copy the relevant portions, but should not use the returned object // directly (i.e. it must be copied). // -S3Config * -ConfigCache::get(const char *fname) +static S3Config * +load_config(const char *fname) { - S3Config *s3; + S3Config *s3 = nullptr; struct timeval tv; @@ -719,64 +737,17 @@ ConfigCache::get(const char *fname) // Make sure the filename is an absolute path, prepending the config dir if needed std::string config_fname = makeConfigPath(fname); - auto it = _cache.find(config_fname); - - if (it != _cache.end()) { - unsigned update_status = it->second.update_status; - if (tv.tv_sec > (it->second.load_time + it->second.ttl)) { - if (!(update_status & 1) && it->second.update_status.compare_exchange_strong(update_status, update_status + 1)) { - TSDebug(PLUGIN_NAME, "Configuration from %s is stale, reloading", config_fname.c_str()); - s3 = new S3Config(false); // false == this config does not get the continuation - - if (s3->parse_config(config_fname)) { - s3->set_conf_fname(fname); - } else { - // Failed the configuration parse... Set the cache response to nullptr - delete s3; - s3 = nullptr; - TSAssert(!"Configuration parsing / caching failed"); - } - TSDebug(PLUGIN_NAME, "Updated rule: access_key=%s, virtual_host=%s, version=%d", s3->keyid(), - s3->virt_host() ? "yes" : "no", s3->version()); - - delete it->second.config; - it->second.config = s3; - it->second.load_time = tv.tv_sec; - it->second.ttl = s3->ttl(); - TSDebug(PLUGIN_NAME, "Config ttl updated to %d seconds", it->second.ttl.load()); + s3 = new S3Config(false); // false == this config does not get the continuation - // Update is complete. - ++it->second.update_status; - } else { - // This thread lost the race with another thread that is also reloading - // the config for this file. Wait for the other thread to finish reloading. - while (it->second.update_status & 1) { - // Hopefully yielding will sleep the thread at least until the next - // scheduler interrupt, preventing a busy wait. - std::this_thread::yield(); - } - s3 = it->second.config; - } - } else { - TSDebug(PLUGIN_NAME, "Configuration from %s is fresh, reusing", config_fname.c_str()); - s3 = it->second.config; - } + if (s3->parse_config(config_fname)) { + s3->set_conf_fname(fname); + TSDebug(PLUGIN_NAME, "Updated rule: access_key=%s, virtual_host=%s, version=%d", s3->keyid(), s3->virt_host() ? "yes" : "no", + s3->version()); } else { - // Create a new cached file. - s3 = new S3Config(false); // false == this config does not get the continuation - - TSDebug(PLUGIN_NAME, "Parsing and caching configuration from %s", config_fname.c_str()); - if (s3->parse_config(config_fname)) { - s3->set_conf_fname(fname); - _cache.emplace(config_fname, _ConfigData(s3, tv.tv_sec, s3->ttl())); - _cache[config_fname].ttl = s3->ttl(); - TSDebug(PLUGIN_NAME, "Config ttl set to %d seconds", _cache[config_fname].ttl.load()); - } else { - delete s3; - s3 = nullptr; - TSAssert(!"Configuration parsing / caching failed"); - } + delete s3; + s3 = nullptr; } + return s3; } @@ -1181,12 +1152,19 @@ config_dir_watch(TSCont cont, TSEvent event, void *edata) } TSAssert(event == TS_EVENT_FILE_CREATED); - TSDebug(PLUGIN_NAME, "File created: %s", fwd->name); - if (strncmp(s3->conf_fname(), fwd->name, strlen(s3->conf_fname())) == 0) { - TSDebug(PLUGIN_NAME, "config file created"); - config_reloader(cont, event, edata); + // On some platforms, we get a file name on create. + if (fwd->name != nullptr) { + TSDebug(PLUGIN_NAME, "File created: %s", fwd->name); + if (strncmp(s3->conf_fname(), fwd->name, strlen(s3->conf_fname())) == 0) { + TSDebug(PLUGIN_NAME, "config file created"); + } else { + // Some other file was created. + return TS_SUCCESS; + } } + s3->set_watch_retry(5); + config_reloader(cont, event, edata); return TS_SUCCESS; } @@ -1205,6 +1183,7 @@ config_watch(TSCont cont, TSEvent event, void *edata) return TS_SUCCESS; } + s3->set_watch_retry(5); config_reloader(cont, event, edata); return TS_SUCCESS; @@ -1214,12 +1193,24 @@ int config_reloader(TSCont cont, TSEvent event, void *edata) { TSDebug(PLUGIN_NAME, "reloading configs"); - S3Config *s3 = static_cast(TSContDataGet(cont)); - S3Config *file_config = gConfCache.get(s3->conf_fname()); + S3Config *s3 = static_cast(TSContDataGet(cont)); + s3->clear_reload_action(); + S3Config *file_config = load_config(s3->conf_fname()); if (!file_config || !file_config->valid()) { - TSError("[%s] requires both shared and AWS secret configuration", PLUGIN_NAME); - return TS_ERROR; + // An invalid config is not necessarily an error. + // Sometimes a file notification is sent before a file finishes writing. + // It's also possible that the advisory lock on the file is still being held. + TSDebug(PLUGIN_NAME, "Failed to reload config."); + auto remaining = s3->decrement_retry(); + if (remaining > 0) { + TSDebug(PLUGIN_NAME, "Retring config reload in 1 second. %d tries remaining.", remaining); + s3->schedule_conf_reload(1); + return TS_SUCCESS; + } else { + TSError("[%s] Giving up config reload. %d tries remaining.", PLUGIN_NAME, remaining); + return TS_ERROR; + } } { @@ -1294,7 +1285,6 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE {const_cast("v4-region-map"), required_argument, nullptr, 'm'}, {const_cast("session_token"), required_argument, nullptr, 't'}, {const_cast("watch-config"), no_argument, nullptr, 'w'}, - {const_cast("ttl"), no_argument, nullptr, 'T'}, {nullptr, no_argument, nullptr, '\0'}, }; @@ -1311,8 +1301,8 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE switch (opt) { case 'c': - file_config = gConfCache.get(optarg); // Get cached, or new, config object, from a file - if (!file_config) { + file_config = load_config(optarg); // Get cached, or new, config object, from a file + if (!file_config || !file_config->valid()) { TSError("[%s] invalid configuration file, %s", PLUGIN_NAME, optarg); *ih = nullptr; return TS_ERROR; @@ -1345,9 +1335,6 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE case 'w': s3->set_watch_config(); break; - case 'T': - s3->set_ttl(optarg); - break; } if (opt == -1) { diff --git a/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py b/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py index dac3d8caba2..7453ad1e664 100644 --- a/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py +++ b/tests/gold_tests/pluginTest/s3_auth/s3_auth_watch_config.test.py @@ -20,7 +20,7 @@ Test.ContinueOnFail = True Test.SkipUnless( - Condition.HasATSFeature('TS_USE_INOTIFY'), + Condition.HasATSFeature('TS_USE_INOTIFY') or Condition.HasATSFeature('TS_USE_KQUEUE'), ) ts = Test.MakeATSProcess("ts") From 05d1a8b00eaca77c2a8351cee81dc42aa893e79e Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Thu, 18 Aug 2022 16:57:52 -0500 Subject: [PATCH 06/11] Fix up broken Linux build from kqueue changes --- iocore/fs/FileChange.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/iocore/fs/FileChange.cc b/iocore/fs/FileChange.cc index 370f7674a66..eb742bb5bce 100644 --- a/iocore/fs/FileChange.cc +++ b/iocore/fs/FileChange.cc @@ -31,11 +31,13 @@ #include #include #include -#include #include #include #include #include +#if TS_USE_KQUEUE +#include +#endif // Globals FileChangeManager fileChangeManager; @@ -115,9 +117,9 @@ FileChangeManager::inotify_process_event(struct inotify_event *event) std::shared_lock file_watches_read_lock(file_watches_mutex); auto finfo_it = file_watches.find(event->wd); if (finfo_it != file_watches.end()) { - TSEvent event_type = TS_EVENT_NONE; - const struct file_info &finfo = finfo_it->second; - Continuation *contp = finfo.contp; + TSEvent event_type = TS_EVENT_NONE; + const FileChangeInfo &finfo = finfo_it->second; + Continuation *contp = finfo.contp; if (event->mask & (IN_DELETE_SELF | IN_MOVED_FROM)) { Debug(TAG, "Delete file event (%d) on %s", event->mask, finfo.path.c_str()); @@ -387,11 +389,11 @@ FileChangeManager::add(const ts::file::path &path, TSFileWatchKind kind, Continu Error("Failed to open %s for monitoring: %s.", path.c_str(), strerror(errno)); return -1; } + file_watches_dirty = true; #else Warning("File change notification is not supported on this OS."); #endif file_watches.try_emplace(wd, kind, path, contp); - file_watches_dirty = true; return wd; } @@ -404,7 +406,7 @@ FileChangeManager::remove(watch_handle_t watch_handle) inotify_rm_watch(inotify_fd, watch_handle); #elif TS_USE_KQUEUE close(watch_handle); + file_watches_dirty = true; #endif file_watches.erase(watch_handle); - file_watches_dirty = true; } From d4c621de4821b888500c985b00fd2bf1844d3c72 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Fri, 19 Aug 2022 12:26:43 -0500 Subject: [PATCH 07/11] Fix a leak of file_config --- plugins/s3_auth/s3_auth.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/plugins/s3_auth/s3_auth.cc b/plugins/s3_auth/s3_auth.cc index 74f958bca1c..d9967da4863 100644 --- a/plugins/s3_auth/s3_auth.cc +++ b/plugins/s3_auth/s3_auth.cc @@ -1202,6 +1202,8 @@ config_reloader(TSCont cont, TSEvent event, void *edata) // Sometimes a file notification is sent before a file finishes writing. // It's also possible that the advisory lock on the file is still being held. TSDebug(PLUGIN_NAME, "Failed to reload config."); + delete file_config; + file_config = nullptr; auto remaining = s3->decrement_retry(); if (remaining > 0) { TSDebug(PLUGIN_NAME, "Retring config reload in 1 second. %d tries remaining.", remaining); @@ -1304,7 +1306,9 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE file_config = load_config(optarg); // Get cached, or new, config object, from a file if (!file_config || !file_config->valid()) { TSError("[%s] invalid configuration file, %s", PLUGIN_NAME, optarg); - *ih = nullptr; + delete file_config; + file_config = nullptr; + *ih = nullptr; return TS_ERROR; } break; From adb5255046a25862dd9f8c0754b9207f1b6a3909 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Fri, 26 Aug 2022 12:09:17 -0500 Subject: [PATCH 08/11] Add Docs, rename unregister --- .../api/functions/TSFileEventRegister.en.rst | 85 +++++++++++++++++++ .../functions/TSFileEventUnregister.en.rst | 37 ++++++++ include/ts/ts.h | 4 +- plugins/s3_auth/s3_auth.cc | 8 +- src/traffic_server/InkAPI.cc | 6 +- 5 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 doc/developer-guide/api/functions/TSFileEventRegister.en.rst create mode 100644 doc/developer-guide/api/functions/TSFileEventUnregister.en.rst diff --git a/doc/developer-guide/api/functions/TSFileEventRegister.en.rst b/doc/developer-guide/api/functions/TSFileEventRegister.en.rst new file mode 100644 index 00000000000..5e3f611fb3a --- /dev/null +++ b/doc/developer-guide/api/functions/TSFileEventRegister.en.rst @@ -0,0 +1,85 @@ +.. 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. + +.. include:: ../../../common.defs + +.. default-domain:: c + +TSFileEventRegister +******************* + +Synopsis +======== + +.. code-block:: cpp + + #include + +.. function:: TSWatchDescriptor TSFileEventRegister(const char *path, TSFileWatchKind kind, TSCont contp) + +Description +=========== + +Attempt to register a watch on a file or directory. :arg:`contp` will be called when +the OS reports a change in the file system at :arg:`path`. + +Types +===== + +.. enum:: TSFileWatchKind + + The kind of changes to watch on the path. + + .. enumerator:: TS_WATCH_CREATE + + Only valid on directories. :arg:`contp` is called after a file or directory has been + created under :arg:`path`. Some operating systems, such as Linux, will supply a file + name of the newly created file or directory. This name is passed to :arg:`contp` when + it's available. + + .. enumerator:: TS_WATCH_DELETE + + Valid on files and directories. :arg:`contp` is called after :arg:`path` is deleted. + + .. enumerator:: TS_WATCH_MODIFY + + Valid on files and directories. :arg:`contp` is called after :arg:`path` has been modified. + +.. struct:: TSFileWatchData + + A class that holds information for the callback. :arg:`contp` will be called back with + :arg:`edata` pointing to one of these. + + .. member:: TSWatchDescriptor wd + + The watch descriptor for that was previously returned from :func:`TSFileEventRegister`. + + .. member:: const char *name + + Only sometimes populated for :enumerator:`TS_WATCH_CREATE`. The name of the created + file. Note that this name is not always available. When it's unavailable, the value will + be :code:`nullptr`. + +.. type:: TSWatchDescriptor + + An opaque type that identifies a file system watch. + +Return Value +============ + +Returns a TSWatchDescriptor on success, or -1 on failure. The caller should store the +returned watch descriptor . + diff --git a/doc/developer-guide/api/functions/TSFileEventUnregister.en.rst b/doc/developer-guide/api/functions/TSFileEventUnregister.en.rst new file mode 100644 index 00000000000..5803a4d8667 --- /dev/null +++ b/doc/developer-guide/api/functions/TSFileEventUnregister.en.rst @@ -0,0 +1,37 @@ +.. 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. + +.. include:: ../../../common.defs + +.. default-domain:: c + +TSFileEventUnregister +********************* + +Synopsis +======== + +.. code-block:: cpp + + #include + +.. function:: void TSFileEventUnregister(TSWatchDescriptor wd) + +Description +=========== + +Unregister a watch that was registered by :func:`TSFileEventRegister`. :arg:`wd` is the return value from :func:`TSFileEventRegister`. + diff --git a/include/ts/ts.h b/include/ts/ts.h index 2181ef29df7..7af5339c155 100644 --- a/include/ts/ts.h +++ b/include/ts/ts.h @@ -2767,8 +2767,8 @@ tsapi TSReturnCode TSHttpTxnCntlSet(TSHttpTxn txnp, TSHttpCntlType ctrl, bool da * wd is the watch descriptor for the event. * */ -tsapi TSWatchDescriptor TSFileEventRegister(const char *filename, TSFileWatchKind kind, TSCont contp); -tsapi void TSFileEventUnRegister(TSWatchDescriptor wd); +tsapi TSWatchDescriptor TSFileEventRegister(const char *path, TSFileWatchKind kind, TSCont contp); +tsapi void TSFileEventUnregister(TSWatchDescriptor wd); #ifdef __cplusplus } diff --git a/plugins/s3_auth/s3_auth.cc b/plugins/s3_auth/s3_auth.cc index d9967da4863..fdc92fdfb64 100644 --- a/plugins/s3_auth/s3_auth.cc +++ b/plugins/s3_auth/s3_auth.cc @@ -523,12 +523,12 @@ class S3Config { std::unique_lock lock(wd_mutex); if (_config_file_wd) { - TSFileEventUnRegister(_config_file_wd.value()); + TSFileEventUnregister(_config_file_wd.value()); _config_file_wd.reset(); } if (_config_dir_wd) { - TSFileEventUnRegister(_config_dir_wd.value()); + TSFileEventUnregister(_config_dir_wd.value()); _config_dir_wd.reset(); } } @@ -538,7 +538,7 @@ class S3Config { std::unique_lock lock(wd_mutex); if (_config_file_wd == wd) { - TSFileEventUnRegister(_config_file_wd.value()); + TSFileEventUnregister(_config_file_wd.value()); _config_file_wd.reset(); } } @@ -548,7 +548,7 @@ class S3Config { std::unique_lock lock(wd_mutex); if (_config_dir_wd == wd) { - TSFileEventUnRegister(_config_dir_wd.value()); + TSFileEventUnregister(_config_dir_wd.value()); _config_dir_wd.reset(); } } diff --git a/src/traffic_server/InkAPI.cc b/src/traffic_server/InkAPI.cc index d73e6c6b2ad..abf8c94ee19 100644 --- a/src/traffic_server/InkAPI.cc +++ b/src/traffic_server/InkAPI.cc @@ -10472,17 +10472,17 @@ TSDbgCtlCreate(char const *tag) } tsapi TSWatchDescriptor -TSFileEventRegister(const char *filename, TSFileWatchKind kind, TSCont contp) +TSFileEventRegister(const char *path, TSFileWatchKind kind, TSCont contp) { sdk_assert(sdk_sanity_check_iocore_structure(contp) == TS_SUCCESS); sdk_assert(sdk_sanity_check_null_ptr((void *)this_ethread()) == TS_SUCCESS); Continuation *pCont = reinterpret_cast(contp); - return fileChangeManager.add(ts::file::path{filename}, kind, pCont); + return fileChangeManager.add(ts::file::path{path}, kind, pCont); } tsapi void -TSFileEventUnRegister(TSWatchDescriptor wd) +TSFileEventUnregister(TSWatchDescriptor wd) { fileChangeManager.remove(wd); } From 01cb9873f69b111d0accc9841b8ff9dd8284cfbb Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Fri, 26 Aug 2022 15:52:29 -0500 Subject: [PATCH 09/11] Fix a mem leak of file_config --- plugins/s3_auth/s3_auth.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/plugins/s3_auth/s3_auth.cc b/plugins/s3_auth/s3_auth.cc index fdc92fdfb64..74dd024d609 100644 --- a/plugins/s3_auth/s3_auth.cc +++ b/plugins/s3_auth/s3_auth.cc @@ -1220,6 +1220,9 @@ config_reloader(TSCont cont, TSEvent event, void *edata) s3->copy_changes_from(file_config); } + delete file_config; + file_config = nullptr; + if (s3->expiration() == 0) { TSDebug(PLUGIN_NAME, "disabling auto config reload"); } else { @@ -1349,6 +1352,8 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE // Copy the config file secret into our instance of the configuration. if (file_config) { s3->copy_changes_from(file_config); + delete file_config; + file_config = nullptr; } // Make sure we got both the shared secret and the AWS secret From ff3b4f22f6a9656eaf0564a0ba304839808d282d Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Wed, 31 Aug 2022 16:01:20 -0600 Subject: [PATCH 10/11] Fix clang-analyzer reported error --- plugins/s3_auth/s3_auth.cc | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/plugins/s3_auth/s3_auth.cc b/plugins/s3_auth/s3_auth.cc index 74dd024d609..d39e33a8a0e 100644 --- a/plugins/s3_auth/s3_auth.cc +++ b/plugins/s3_auth/s3_auth.cc @@ -1313,7 +1313,13 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE file_config = nullptr; *ih = nullptr; return TS_ERROR; + } else { + s3->copy_changes_from(file_config); + // Copy the config file secret into our instance of the configuration. + delete file_config; + file_config = nullptr; } + break; case 'a': s3->set_keyid(optarg); @@ -1349,13 +1355,6 @@ TSRemapNewInstance(int argc, char *argv[], void **ih, char * /* errbuf ATS_UNUSE } } - // Copy the config file secret into our instance of the configuration. - if (file_config) { - s3->copy_changes_from(file_config); - delete file_config; - file_config = nullptr; - } - // Make sure we got both the shared secret and the AWS secret if (!s3->valid()) { TSError("[%s] requires both shared and AWS secret configuration", PLUGIN_NAME); From 481aa04a8ddef9c954df8d039faff24ff99db754 Mon Sep 17 00:00:00 2001 From: Mo Chen Date: Tue, 13 Sep 2022 10:46:05 -0500 Subject: [PATCH 11/11] Remove flock-based locking -Reader and writer need to agree on a method -flock isn't supported by C++ standard library file I/O methods --- plugins/s3_auth/s3_auth.cc | 45 +------------------------------------- 1 file changed, 1 insertion(+), 44 deletions(-) diff --git a/plugins/s3_auth/s3_auth.cc b/plugins/s3_auth/s3_auth.cc index d39e33a8a0e..e4f488f1bbe 100644 --- a/plugins/s3_auth/s3_auth.cc +++ b/plugins/s3_auth/s3_auth.cc @@ -599,44 +599,6 @@ class S3Config std::atomic watch_retry_count = 0; }; -static bool -try_flock(int fd) -{ - struct flock lck; - lck.l_start = 0; - lck.l_len = 0; - lck.l_type = F_RDLCK; - lck.l_whence = SEEK_SET; - auto res = fcntl(fd, F_SETLK, &lck); - if (res < 0) { - switch (errno) { - case EAGAIN: - case EINTR: - TSDebug(PLUGIN_NAME, "flock is busy."); - break; - default: - TSError("[%s] Failed to flock(): %s!", PLUGIN_NAME, strerror(errno)); - break; - } - return false; - } - return true; -} - -static void -funlock(int fd) -{ - struct flock lck; - lck.l_start = 0; - lck.l_len = 0; - lck.l_type = F_UNLCK; - lck.l_whence = SEEK_SET; - auto res = fcntl(fd, F_SETLK, &lck); - if (res < 0) { - TSError("[%s] Failed to funlock(): %s!", PLUGIN_NAME, strerror(errno)); - } -} - bool S3Config::parse_config(const std::string &config_fname) { @@ -652,11 +614,7 @@ S3Config::parse_config(const std::string &config_fname) TSError("[%s] unable to open %s", PLUGIN_NAME, config_fname.c_str()); return false; } - - if (!try_flock(fd)) { - fclose(file); - return false; - } + // TODO: we really should have some kind of file locking strategy here. while (fgets(line, sizeof(line), file) != nullptr) { char *pos1, *pos2; @@ -711,7 +669,6 @@ S3Config::parse_config(const std::string &config_fname) } } - funlock(fd); fclose(file); }