diff --git a/lib/remote/apilistener-configsync.cpp b/lib/remote/apilistener-configsync.cpp index a12db0bca..04436ad8b 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 index e529c832b..f2165f2ce 100644 --- a/lib/remote/configobjectslock.cpp +++ b/lib/remote/configobjectslock.cpp @@ -1,13 +1,16 @@ /* Icinga 2 | (c) 2022 Icinga GmbH | GPLv2+ */ -#ifndef _WIN32 - -#include "base/shared-memory.hpp" #include "remote/configobjectslock.hpp" + +#ifndef _WIN32 +#include "base/shared-memory.hpp" #include +#endif /* _WIN32 */ using namespace icinga; +#ifndef _WIN32 + // On *nix one process may write config objects while another is loading the config, so this uses IPC. static SharedMemory l_ConfigObjectsMutex; @@ -22,3 +25,37 @@ ConfigObjectsSharedLock::ConfigObjectsSharedLock(std::try_to_lock_t) } #endif /* _WIN32 */ + +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 index ee909815f..6b75139b6 100644 --- a/lib/remote/configobjectslock.hpp +++ b/lib/remote/configobjectslock.hpp @@ -2,7 +2,12 @@ #pragma once +#include "base/type.hpp" +#include "base/string.hpp" +#include +#include #include +#include #ifndef _WIN32 #include @@ -69,4 +74,29 @@ private: #endif /* _WIN32 */ + +/** + * 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 62c910b41..60268f6e1 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" @@ -198,11 +200,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"); @@ -227,8 +234,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)); @@ -250,8 +255,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)); @@ -275,16 +278,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 598eeec3b..89977a3d3 100644 --- a/lib/remote/createobjecthandler.cpp +++ b/lib/remote/createobjecthandler.cpp @@ -124,6 +124,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 a4fd98d9a..0c6e85a97 100644 --- a/lib/remote/deleteobjecthandler.cpp +++ b/lib/remote/deleteobjecthandler.cpp @@ -84,6 +84,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 d6fa98b2e..a817faad8 100644 --- a/lib/remote/modifyobjecthandler.cpp +++ b/lib/remote/modifyobjecthandler.cpp @@ -112,6 +112,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 (restoreAttrs) { ObjectLock oLock (restoreAttrs);