From 008fcd1744a08342271ed374aeaf94b0433b7c82 Mon Sep 17 00:00:00 2001
From: Yonas Habteab <yonas.habteab@icinga.com>
Date: Thu, 25 Jan 2024 17:06:12 +0100
Subject: [PATCH] Preserve runtime objects in a tmp file for the entire
 validation process

Given that the internal `config::Update` cluster events are using this
as well to create received runtime objects, we don't want to persist
first the conf file and the load and validate it with `CompileFile`.
Otherwise, we are forced to remove the newly created file whenever we
can't validate, commit or activate it. This also would also have the
downside that two cluster events for the same object arriving at the
same moment from two different endpoints would result in two different
threads simultaneously creating and loading the same config file -
whereby only one of the surpasses the validation, while the other is
facing an object `re-definition` error and tries to remove that config
file it mistakenly thinks it has created. As a consequence, an object
successfully created by the former is implicitly deleted by the latter
thread, causing the objects to mysteriously disappear.
---
 lib/remote/configobjectutility.cpp | 28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/lib/remote/configobjectutility.cpp b/lib/remote/configobjectutility.cpp
index 62c910b41..9502bde95 100644
--- a/lib/remote/configobjectutility.cpp
+++ b/lib/remote/configobjectutility.cpp
@@ -5,6 +5,7 @@
 #include "remote/apilistener.hpp"
 #include "config/configcompiler.hpp"
 #include "config/configitem.hpp"
+#include "base/atomic-file.hpp"
 #include "base/configwriter.hpp"
 #include "base/exception.hpp"
 #include "base/dependencygraph.hpp"
@@ -198,13 +199,21 @@ 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);
+	// Using AtomicFile guarantees that two different threads simultaneously creating and loading the same
+	// configuration file do not interfere with each other, as the configuration is stored in a unique temp file.
+	// When one thread fails to pass object validation, it only deletes its temporary file and does not affect
+	// the other thread in any way.
+	AtomicFile fp(path, 0644);
 	fp << config;
-	fp.close();
+	// Flush the output buffer to catch any errors ASAP and handle them accordingly!
+	// Note: AtomicFile places these configs in a temp file and will be automatically
+	//       discarded when it is not committed before going out of scope.
+	fp.flush();
 
-	std::unique_ptr<Expression> expr = ConfigCompiler::CompileFile(path, String(), "_api");
+	std::unique_ptr<Expression> expr = ConfigCompiler::CompileText(path, config, String(), "_api");
 
 	try {
 		ActivationScope ascope;
@@ -225,9 +234,7 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 		if (!ConfigItem::CommitItems(ascope.GetContext(), upq, newItems, true)) {
 			if (errors) {
 				Log(LogNotice, "ConfigObjectUtility")
-					<< "Failed to commit config item '" << fullName << "'. Aborting and removing config path '" << path << "'.";
-
-				Utility::Remove(path);
+					<< "Failed to commit config item '" << fullName << "'.";
 
 				for (const boost::exception_ptr& ex : upq.GetExceptions()) {
 					errors->Add(DiagnosticInformation(ex, false));
@@ -248,9 +255,7 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 		if (!ConfigItem::ActivateItems(newItems, true, false, false, cookie)) {
 			if (errors) {
 				Log(LogNotice, "ConfigObjectUtility")
-					<< "Failed to activate config object '" << fullName << "'. Aborting and removing config path '" << path << "'.";
-
-				Utility::Remove(path);
+					<< "Failed to activate config object '" << fullName << "'.";
 
 				for (const boost::exception_ptr& ex : upq.GetExceptions()) {
 					errors->Add(DiagnosticInformation(ex, false));
@@ -275,6 +280,9 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 		ConfigObject::Ptr obj = ctype->GetObject(fullName);
 
 		if (obj) {
+			// Object has surpassed the compiling/validation processes, we can safely commit the file!
+			fp.Commit();
+
 			Log(LogInformation, "ConfigObjectUtility")
 				<< "Created and activated object '" << fullName << "' of type '" << type->GetName() << "'.";
 		} else {
@@ -283,8 +291,6 @@ bool ConfigObjectUtility::CreateObject(const Type::Ptr& type, const String& full
 		}
 
 	} catch (const std::exception& ex) {
-		Utility::Remove(path);
-
 		if (errors)
 			errors->Add(DiagnosticInformation(ex, false));