diff --git a/lib/remote/CMakeLists.txt b/lib/remote/CMakeLists.txt index 2c5a0326a31..740b112b42a 100644 --- a/lib/remote/CMakeLists.txt +++ b/lib/remote/CMakeLists.txt @@ -14,6 +14,7 @@ set(remote_SOURCES apilistener-authority.cpp apiuser.cpp apiuser.hpp apiuser-ti.hpp configfileshandler.cpp configfileshandler.hpp + configobjectslock.cpp configobjectslock.hpp configobjectutility.cpp configobjectutility.hpp configpackageshandler.cpp configpackageshandler.hpp configpackageutility.cpp configpackageutility.hpp diff --git a/lib/remote/apilistener-configsync.cpp b/lib/remote/apilistener-configsync.cpp index 627141166ea..4bd75410cde 100644 --- a/lib/remote/apilistener-configsync.cpp +++ b/lib/remote/apilistener-configsync.cpp @@ -8,6 +8,7 @@ #include "base/json.hpp" #include "base/convert.hpp" #include "config/vmops.hpp" +#include "remote/configobjectslock.hpp" #include using namespace icinga; @@ -104,6 +105,11 @@ Value ApiListener::ConfigUpdateObjectAPIHandler(const MessageOrigin::Ptr& origin return Empty; } + // Wait for the object name to become available for processing and block it immediately. + // Doing so guarantees that only one (create/update/delete) cluster event or API request of a + // given object is being processed at any given time. + ObjectNameLock objectNameLock(ptype, objName); + ConfigObject::Ptr object = ctype->GetObject(objName); String config = params->Get("config"); @@ -258,6 +264,11 @@ Value ApiListener::ConfigDeleteObjectAPIHandler(const MessageOrigin::Ptr& origin return Empty; } + // Wait for the object name to become available for processing and block it immediately. + // Doing so guarantees that only one (create/update/delete) cluster event or API request of a + // given object is being processed at any given time. + ObjectNameLock objectNameLock(ptype, objName); + ConfigObject::Ptr object = ctype->GetObject(objName); if (!object) { diff --git a/lib/remote/configobjectslock.cpp b/lib/remote/configobjectslock.cpp new file mode 100644 index 00000000000..ba59a0db3ad --- /dev/null +++ b/lib/remote/configobjectslock.cpp @@ -0,0 +1,39 @@ +/* Icinga 2 | (c) 2022 Icinga GmbH | GPLv2+ */ + +#include "remote/configobjectslock.hpp" + +using namespace icinga; + +std::mutex ObjectNameLock::m_Mutex; +std::condition_variable ObjectNameLock::m_CV; +std::map> ObjectNameLock::m_LockedObjectNames; + +/** + * Locks the specified object name of the given type and unlocks it upon destruction of the instance of this class. + * + * If it is already locked, the call blocks until the lock is released. + * + * @param Type::Ptr ptype The type of the object you want to lock + * @param String objName The object name you want to lock + */ +ObjectNameLock::ObjectNameLock(const Type::Ptr& ptype, const String& objName): m_ObjectName{objName}, m_Type{ptype} +{ + std::unique_lock lock(m_Mutex); + m_CV.wait(lock, [this]{ + auto& locked = m_LockedObjectNames[m_Type.get()]; + return locked.find(m_ObjectName) == locked.end(); + }); + + // Add the object name to the locked list to block all other threads that try + // to process a message affecting the same object. + m_LockedObjectNames[ptype.get()].emplace(objName); +} + +ObjectNameLock::~ObjectNameLock() +{ + { + std::unique_lock lock(m_Mutex); + m_LockedObjectNames[m_Type.get()].erase(m_ObjectName); + } + m_CV.notify_all(); +} diff --git a/lib/remote/configobjectslock.hpp b/lib/remote/configobjectslock.hpp new file mode 100644 index 00000000000..5add769d23e --- /dev/null +++ b/lib/remote/configobjectslock.hpp @@ -0,0 +1,39 @@ +/* Icinga 2 | (c) 2023 Icinga GmbH | GPLv2+ */ + +#pragma once + +#include "base/type.hpp" +#include "base/string.hpp" +#include +#include +#include +#include + +namespace icinga +{ + +/** + * Allows you to easily lock/unlock a specific object of a given type by its name. + * + * That way, locking an object "this" of type Host does not affect an object "this" of + * type "Service" nor an object "other" of type "Host". + * + * @ingroup remote + */ +class ObjectNameLock +{ +public: + ObjectNameLock(const Type::Ptr& ptype, const String& objName); + + ~ObjectNameLock(); + +private: + String m_ObjectName; + Type::Ptr m_Type; + + static std::mutex m_Mutex; + static std::condition_variable m_CV; + static std::map> m_LockedObjectNames; +}; + +} diff --git a/lib/remote/configobjectutility.cpp b/lib/remote/configobjectutility.cpp index 1faeb9a07e4..20d37b0ec93 100644 --- a/lib/remote/configobjectutility.cpp +++ b/lib/remote/configobjectutility.cpp @@ -5,7 +5,9 @@ #include "remote/apilistener.hpp" #include "config/configcompiler.hpp" #include "config/configitem.hpp" +#include "base/atomic-file.hpp" #include "base/configwriter.hpp" +#include "base/defer.hpp" #include "base/exception.hpp" #include "base/dependencygraph.hpp" #include "base/tlsutility.hpp" @@ -191,11 +193,16 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full return false; } + // AtomicFile doesn't create not yet existing directories, so we have to do it by ourselves. Utility::MkDirP(Utility::DirName(path), 0700); - std::ofstream fp(path.CStr(), std::ofstream::out | std::ostream::trunc); - fp << config; - fp.close(); + AtomicFile::Write(path, 0644, config); + + // Remove the just created config file in all the error cases and if the object creation + // succeeds the deferred callback will be cancelled. + Defer removeConfigPath([&path]{ + Utility::Remove(path); + }); std::unique_ptr expr = ConfigCompiler::CompileFile(path, String(), "_api"); @@ -220,8 +227,6 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full Log(LogNotice, "ConfigObjectUtility") << "Failed to commit config item '" << fullName << "'. Aborting and removing config path '" << path << "'."; - Utility::Remove(path); - for (const boost::exception_ptr& ex : upq.GetExceptions()) { errors->Add(DiagnosticInformation(ex, false)); @@ -243,8 +248,6 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full Log(LogNotice, "ConfigObjectUtility") << "Failed to activate config object '" << fullName << "'. Aborting and removing config path '" << path << "'."; - Utility::Remove(path); - for (const boost::exception_ptr& ex : upq.GetExceptions()) { errors->Add(DiagnosticInformation(ex, false)); @@ -268,16 +271,16 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full ConfigObject::Ptr obj = ctype->GetObject(fullName); if (obj) { + // Object is successfully created and activated, so don't remove its config. + removeConfigPath.Cancel(); + Log(LogInformation, "ConfigObjectUtility") << "Created and activated object '" << fullName << "' of type '" << type->GetName() << "'."; } else { Log(LogNotice, "ConfigObjectUtility") << "Object '" << fullName << "' was not created but ignored due to errors."; } - } catch (const std::exception& ex) { - Utility::Remove(path); - if (errors) errors->Add(DiagnosticInformation(ex, false)); diff --git a/lib/remote/createobjecthandler.cpp b/lib/remote/createobjecthandler.cpp index c01b2364156..6722118ea60 100644 --- a/lib/remote/createobjecthandler.cpp +++ b/lib/remote/createobjecthandler.cpp @@ -6,6 +6,7 @@ #include "remote/jsonrpcconnection.hpp" #include "remote/filterutility.hpp" #include "remote/apiaction.hpp" +#include "remote/configobjectslock.hpp" #include "remote/zone.hpp" #include "base/configtype.hpp" #include @@ -116,6 +117,9 @@ bool CreateObjectHandler::HandleRequest( return true; } + // Lock the object name of the given type to prevent from being created concurrently. + ObjectNameLock objectNameLock(type, name); + if (!ConfigObjectUtility::CreateObject(type, name, config, errors, diagnosticInformation)) { result1->Set("errors", errors); result1->Set("code", 500); diff --git a/lib/remote/deleteobjecthandler.cpp b/lib/remote/deleteobjecthandler.cpp index 2edb0e45519..92f0b84b402 100644 --- a/lib/remote/deleteobjecthandler.cpp +++ b/lib/remote/deleteobjecthandler.cpp @@ -5,6 +5,7 @@ #include "remote/httputility.hpp" #include "remote/filterutility.hpp" #include "remote/apiaction.hpp" +#include "remote/configobjectslock.hpp" #include "config/configitem.hpp" #include "base/exception.hpp" #include @@ -76,6 +77,9 @@ bool DeleteObjectHandler::HandleRequest( Array::Ptr errors = new Array(); Array::Ptr diagnosticInformation = new Array(); + // Lock the object name of the given type to prevent from being modified/deleted concurrently. + ObjectNameLock objectNameLock(type, obj->GetName()); + if (!ConfigObjectUtility::DeleteObject(obj, cascade, errors, diagnosticInformation)) { code = 500; status = "Object could not be deleted."; diff --git a/lib/remote/modifyobjecthandler.cpp b/lib/remote/modifyobjecthandler.cpp index cc008b90c53..2a8c70f68b0 100644 --- a/lib/remote/modifyobjecthandler.cpp +++ b/lib/remote/modifyobjecthandler.cpp @@ -4,6 +4,7 @@ #include "remote/httputility.hpp" #include "remote/filterutility.hpp" #include "remote/apiaction.hpp" +#include "remote/configobjectslock.hpp" #include "base/exception.hpp" #include #include @@ -87,6 +88,9 @@ bool ModifyObjectHandler::HandleRequest( String key; + // Lock the object name of the given type to prevent from being modified/deleted concurrently. + ObjectNameLock objectNameLock(type, obj->GetName()); + try { if (attrs) { ObjectLock olock(attrs);