Merge pull request #8389 from Icinga/feature/forbid-dep-cycles

Forbid dependency cycles

lib/cli/daemonutility.cpp

@@ -1,6 +1,8 @@
 /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */
 
 #include "cli/daemonutility.hpp"
+#include "base/configobject.hpp"
+#include "base/exception.hpp"
 #include "base/utility.hpp"
 #include "base/logger.hpp"
 #include "base/application.hpp"
@@ -8,6 +10,7 @@
 #include "config/configcompiler.hpp"
 #include "config/configcompilercontext.hpp"
 #include "config/configitembuilder.hpp"
+#include "icinga/dependency.hpp"
 #include <set>
 
 using namespace icinga;
@@ -248,6 +251,17 @@ bool DaemonUtility::LoadConfigFiles(const std::vector<std::string>& configs,
 	upq.SetName("DaemonUtility::LoadConfigFiles");
 	bool result = ConfigItem::CommitItems(ascope.GetContext(), upq, newItems);
 
+	if (result) {
+		try {
+			Dependency::AssertNoCycles();
+		} catch (...) {
+			Log(LogCritical, "config")
+				<< DiagnosticInformation(boost::current_exception(), false);
+
+			result = false;
+		}
+	}
+
 	if (!result) {
 		ConfigCompilerContext::GetInstance()->CancelObjectsFile();
 		return false;

lib/icinga/dependency.cpp

@@ -3,13 +3,114 @@
 #include "icinga/dependency.hpp"
 #include "icinga/dependency-ti.cpp"
 #include "icinga/service.hpp"
+#include "base/configobject.hpp"
+#include "base/initialize.hpp"
 #include "base/logger.hpp"
 #include "base/exception.hpp"
+#include <map>
+#include <sstream>
+#include <utility>
 
 using namespace icinga;
 
 REGISTER_TYPE(Dependency);
 
+bool Dependency::m_AssertNoCyclesForIndividualDeps = false;
+
+struct DependencyCycleNode
+{
+	bool Visited = false;
+	bool OnStack = false;
+};
+
+struct DependencyStackFrame
+{
+	ConfigObject::Ptr Node;
+	bool Implicit;
+
+	inline DependencyStackFrame(ConfigObject::Ptr node, bool implicit = false) : Node(std::move(node)), Implicit(implicit)
+	{ }
+};
+
+struct DependencyCycleGraph
+{
+	std::map<Checkable::Ptr, DependencyCycleNode> Nodes;
+	std::vector<DependencyStackFrame> Stack;
+};
+
+static void AssertNoDependencyCycle(const Checkable::Ptr& checkable, DependencyCycleGraph& graph, bool implicit = false);
+
+static void AssertNoParentDependencyCycle(const Checkable::Ptr& parent, DependencyCycleGraph& graph, bool implicit)
+{
+	if (graph.Nodes[parent].OnStack) {
+		std::ostringstream oss;
+		oss << "Dependency cycle:\n";
+
+		for (auto& frame : graph.Stack) {
+			oss << frame.Node->GetReflectionType()->GetName() << " '" << frame.Node->GetName() << "'";
+
+			if (frame.Implicit) {
+				oss << " (implicit)";
+			}
+
+			oss << "\n-> ";
+		}
+
+		oss << parent->GetReflectionType()->GetName() << " '" << parent->GetName() << "'";
+
+		if (implicit) {
+			oss << " (implicit)";
+		}
+
+		BOOST_THROW_EXCEPTION(ScriptError(oss.str()));
+	}
+
+	AssertNoDependencyCycle(parent, graph, implicit);
+}
+
+static void AssertNoDependencyCycle(const Checkable::Ptr& checkable, DependencyCycleGraph& graph, bool implicit)
+{
+	auto& node (graph.Nodes[checkable]);
+
+	if (!node.Visited) {
+		node.Visited = true;
+		node.OnStack = true;
+		graph.Stack.emplace_back(checkable, implicit);
+
+		for (auto& dep : checkable->GetDependencies()) {
+			graph.Stack.emplace_back(dep);
+			AssertNoParentDependencyCycle(dep->GetParent(), graph, false);
+			graph.Stack.pop_back();
+		}
+
+		{
+			auto service (dynamic_pointer_cast<Service>(checkable));
+
+			if (service) {
+				AssertNoParentDependencyCycle(service->GetHost(), graph, true);
+			}
+		}
+
+		graph.Stack.pop_back();
+		node.OnStack = false;
+	}
+}
+
+void Dependency::AssertNoCycles()
+{
+	DependencyCycleGraph graph;
+
+	for (auto& host : ConfigType::GetObjectsByType<Host>()) {
+		AssertNoDependencyCycle(host, graph);
+	}
+
+	for (auto& service : ConfigType::GetObjectsByType<Service>()) {
+		AssertNoDependencyCycle(service, graph);
+	}
+
+	m_AssertNoCyclesForIndividualDeps = true;
+}
+
 String DependencyNameComposer::MakeName(const String& shortName, const Object::Ptr& context) const
 {
 	Dependency::Ptr dependency = dynamic_pointer_cast<Dependency>(context);
@@ -89,6 +190,18 @@ void Dependency::OnAllConfigLoaded()
 
 	m_Child->AddDependency(this);
 	m_Parent->AddReverseDependency(this);
+
+	if (m_AssertNoCyclesForIndividualDeps) {
+		DependencyCycleGraph graph;
+
+		try {
+			AssertNoDependencyCycle(m_Parent, graph);
+		} catch (...) {
+			m_Child->RemoveDependency(this);
+			m_Parent->RemoveReverseDependency(this);
+			throw;
+		}
+	}
 }
 
 void Dependency::Stop(bool runtimeRemoved)
@@ -210,4 +323,3 @@ void Dependency::SetChild(intrusive_ptr<Checkable> child)
 {
 	m_Child = child;
 }
-

lib/icinga/dependency.hpp

@@ -36,6 +36,7 @@ public:
 
 	static void EvaluateApplyRules(const intrusive_ptr<Host>& host);
 	static void EvaluateApplyRules(const intrusive_ptr<Service>& service);
+	static void AssertNoCycles();
 
 	/* Note: Only use them for unit test mocks. Prefer OnConfigLoaded(). */
 	void SetParent(intrusive_ptr<Checkable> parent);
@@ -50,6 +51,8 @@ private:
 	Checkable::Ptr m_Parent;
 	Checkable::Ptr m_Child;
 
+	static bool m_AssertNoCyclesForIndividualDeps;
+
 	static bool EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule, bool skipFilter);
 	static bool EvaluateApplyRule(const Checkable::Ptr& checkable, const ApplyRule& rule, bool skipFilter = false);
 };