From 99f9d5953eb78d4abc3933a2305b16e295b57908 Mon Sep 17 00:00:00 2001 From: Ferdinando Formica Date: Mon, 10 Mar 2025 15:19:00 +0000 Subject: [PATCH 1/3] PENG-6092: check if we can tolerate the race condition by returning success on the already exists error --- src/CoreServer.cpp | 3 +++ src/Policies.cpp | 10 ++++++---- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/CoreServer.cpp b/src/CoreServer.cpp index 0d31a1a9c..bfc6297f7 100644 --- a/src/CoreServer.cpp +++ b/src/CoreServer.cpp @@ -358,10 +358,13 @@ void CoreServer::_setup_routes(const PrometheusConfig &prom_config) res.status = 404; j["error"] = "policy does not exists"; res.set_content(j.dump(), "text/json"); + _logger->info("delete: policy not found: {}", name); return; } try { + _logger->info("delete: removing policy: {}", name); _registry->policy_manager()->remove_policy(name); + _logger->info("delete: removed policy: {}", name); res.status = 200; res.set_content(j.dump(), "text/json"); } catch (const std::exception &e) { diff --git a/src/Policies.cpp b/src/Policies.cpp index b292be7f2..55fa3c030 100644 --- a/src/Policies.cpp +++ b/src/Policies.cpp @@ -53,7 +53,13 @@ std::vector PolicyManager::load(const YAML::Node &policy_yaml, bool si // serialized policy loads std::unique_lock lock(_load_mutex); + // Ensure policy name isn't already defined auto policy_name = _get_policy_name(it); + if (module_exists(policy_name)) { + // ignore + spdlog::get("visor")->warn("ignoring policy with name '{}' already defined", policy_name); + continue; + } // Input Section auto input_node = it->second["input"]; auto [input_config, input_filter] = _registry->input_manager()->get_config_and_filter(input_node); @@ -225,10 +231,6 @@ std::string PolicyManager::_get_policy_name(YAML::const_iterator it) if (!it->second.IsMap()) { throw PolicyException("expecting policy configuration map"); } - // Ensure policy name isn't already defined - if (module_exists(policy_name)) { - throw PolicyException(fmt::format("policy with name '{}' already defined", policy_name)); - } // Policy kind defines schema if (!it->second["kind"] || !it->second["kind"].IsScalar()) { From 3b4bbc065d7a8fe5f6722dbfc6c1d2dffef852b9 Mon Sep 17 00:00:00 2001 From: Ferdinando Formica Date: Mon, 10 Mar 2025 15:43:46 +0000 Subject: [PATCH 2/3] Force policy name to std::string for logging --- src/CoreServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/CoreServer.cpp b/src/CoreServer.cpp index bfc6297f7..67e3bfb4a 100644 --- a/src/CoreServer.cpp +++ b/src/CoreServer.cpp @@ -353,7 +353,7 @@ void CoreServer::_setup_routes(const PrometheusConfig &prom_config) }); _svr.Delete(fmt::format("/api/v1/policies/({})", AbstractModule::MODULE_ID_REGEX).c_str(), [&](const httplib::Request &req, httplib::Response &res) { json j = json::object(); - auto name = req.matches[1]; + std::string name = req.matches[1]; if (!_registry->policy_manager()->module_exists(name)) { res.status = 404; j["error"] = "policy does not exists"; From bebbb38a0ccb6af33d84303ebd6c6ca0eed99c6f Mon Sep 17 00:00:00 2001 From: Ferdinando Formica Date: Mon, 10 Mar 2025 16:10:53 +0000 Subject: [PATCH 3/3] Align unit test --- src/tests/test_policies.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/tests/test_policies.cpp b/src/tests/test_policies.cpp index f0e483815..b32ac4b18 100644 --- a/src/tests/test_policies.cpp +++ b/src/tests/test_policies.cpp @@ -3,8 +3,8 @@ #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wmissing-declarations" #endif -#include "inputs/static_plugins.h" #include "handlers/static_plugins.h" +#include "inputs/static_plugins.h" #ifdef __GNUC__ #pragma GCC diagnostic pop #endif @@ -901,7 +901,9 @@ TEST_CASE("Policies", "[policies]") REQUIRE_NOTHROW(registry.tap_manager()->load(config_file["visor"]["taps"])); REQUIRE_NOTHROW(registry.policy_manager()->load(config_file["visor"]["policies"])); - REQUIRE_THROWS_WITH(registry.policy_manager()->load(config_file["visor"]["policies"]), "policy with name 'default_view' already defined"); + // tolerated for the time being + REQUIRE_NOTHROW(registry.policy_manager()->load(config_file["visor"]["policies"])); + // REQUIRE_THROWS_WITH(registry.policy_manager()->load(config_file["visor"]["policies"]), "policy with name 'default_view' already defined"); REQUIRE(registry.policy_manager()->module_exists("default_view")); auto [policy, lock] = registry.policy_manager()->module_get_locked("default_view");