Evaluate apply filters in the for loop

fixes Icinga#7610
red61wjt · Jan 29, 2015 · fb323ee · fb323ee
1 parent 8878868
commit fb323ee
Show file tree

Hide file tree

Showing 14 changed files with 136 additions and 75 deletions.
diff --git a/doc/3-monitoring-basics.md b/doc/3-monitoring-basics.md
@@ -427,8 +427,6 @@ You can also specifiy the check command that way.
       notes = "Interface check for Port " + string(vars.port) + " in VLAN " + vars.vlan + " on Address " + vars.address + " QoS " + vars.qos
       notes_url = "http://foreman.company.com/hosts/" + host.name
       action_url = "http://snmp.checker.company.com/" + host.name + "if-" + if_name
-
-      assign where host.vars.interfaces
     }
 
 Note that numbers must be explicitely casted to string when adding to strings.
@@ -481,8 +479,6 @@ values for any object attribute specified in that apply rule.
 
       notes_url = "http://foreman.company.com/hosts/" + host.name
       action_url = "http://snmp.checker.company.com/" + host.name + "/" + vars.customer_id
-
-      assign where host.vars.hosting
     }
 
 ### <a id="groups"></a> Groups
@@ -1003,8 +999,6 @@ string values for passing multiple partitions to the `check_disk` check plugin.
 
       vars.disk_wfree = 10
       vars.disk_cfree = 5
-
-      assign where host.vars.local_disks
     }
 
 

diff --git a/doc/4-configuring-icinga-2.md b/doc/4-configuring-icinga-2.md
@@ -366,8 +366,6 @@ Configuration example:
       check_command = "disk"
 
       vars += config
-
-      assign where host.vars.disks
     }
 
 A similar example is used for the `http` services. That way you can make your

diff --git a/etc/icinga2/conf.d/services.conf b/etc/icinga2/conf.d/services.conf
@@ -61,8 +61,6 @@ apply Service for (http_vhost => config in host.vars.http_vhosts) {
   check_command = "http"
 
   vars += config
-
-  assign where host.vars.http_vhosts
 }
 
 apply Service for (disk => config in host.vars.disks) {
@@ -71,8 +69,6 @@ apply Service for (disk => config in host.vars.disks) {
   check_command = "disk"
 
   vars += config
-
-  assign where host.vars.disks
 }
 
 apply Service "icinga" {

diff --git a/lib/base/dictionary.cpp b/lib/base/dictionary.cpp
@@ -215,3 +215,23 @@ Dictionary::Ptr Dictionary::ShallowClone(void) const
 	CopyTo(clone);
 	return clone;
 }
+
+/**
+ * Returns an array containing all keys
+ * which are currently set in this directory.
+ *
+ * @returns an array of key names
+ */
+std::vector<String> Dictionary::GetKeys(void) const
+{
+	ASSERT(!OwnsLock());
+	ObjectLock olock(this);
+
+	std::vector<String> keys;
+
+	BOOST_FOREACH(const Dictionary::Pair& kv, m_Data) {
+		keys.push_back(kv.first);
+	}
+
+	return keys;
+}
diff --git a/lib/base/dictionary.hpp b/lib/base/dictionary.hpp
@@ -25,6 +25,7 @@
 #include "base/value.hpp"
 #include <boost/range/iterator.hpp>
 #include <map>
+#include <vector>
 
 namespace icinga
 {
@@ -64,6 +65,8 @@ class I2_BASE_API Dictionary : public Object
 	void CopyTo(const Dictionary::Ptr& dest) const;
 	Dictionary::Ptr ShallowClone(void) const;
 
+	std::vector<String> GetKeys(void) const;
+
 	static Object::Ptr GetPrototype(void);
 
 private:

diff --git a/lib/config/config_parser.yy b/lib/config/config_parser.yy
@@ -996,16 +996,23 @@ apply:
 		DictExpression *exprl = dynamic_cast<DictExpression *>($8);
 		exprl->MakeInline();
 
-		// assign && !ignore
-		if (!context->m_SeenAssign.top())
-			BOOST_THROW_EXCEPTION(ScriptError("'apply' is missing 'assign'", DebugInfoRange(@2, @3)));
-
+		bool seen_assign = context->m_SeenAssign.top();
 		context->m_SeenAssign.pop();
 
+		// assign && !ignore
+		if (!seen_assign && !context->m_FTerm.top())
+			BOOST_THROW_EXCEPTION(ScriptError("'apply' is missing 'assign'/'for'", DebugInfoRange(@2, @3)));
+
 		Expression *ignore = context->m_Ignore.top();
 		context->m_Ignore.pop();
 
-		Expression *assign = context->m_Assign.top();
+		Expression *assign;
+
+		if (!seen_assign)
+			assign = MakeLiteral(true);
+		else
+			assign = context->m_Assign.top();
+
 		context->m_Assign.pop();
 
 		Expression *filter;

diff --git a/lib/icinga/dependency-apply.cpp b/lib/icinga/dependency-apply.cpp
@@ -41,8 +41,11 @@ void Dependency::RegisterApplyRuleHandler(void)
 	ApplyRule::RegisterType("Dependency", targets);
 }
 
-void Dependency::EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule)
+bool Dependency::EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule)
 {
+	if (!rule.EvaluateFilter(frame))
+		return false;
+
 	DebugInfo di = rule.GetDebugInfo();
 
 	Log(LogDebug, "Dependency")
@@ -72,6 +75,8 @@ void Dependency::EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, cons
 
 	ConfigItem::Ptr dependencyItem = builder->Compile();
 	dependencyItem->Commit();
+
+	return true;
 }
 
 bool Dependency::EvaluateApplyRule(const Checkable::Ptr& checkable, const ApplyRule& rule)
@@ -93,52 +98,58 @@ bool Dependency::EvaluateApplyRule(const Checkable::Ptr& checkable, const ApplyR
 	if (service)
 		frame.Locals->Set("service", service);
 
-	if (!rule.EvaluateFilter(frame))
-		return false;
-
 	Value vinstances;
 
 	if (rule.GetFTerm()) {
-		vinstances = rule.GetFTerm()->Evaluate(frame);
+		try {
+			vinstances = rule.GetFTerm()->Evaluate(frame);
+		} catch (const std::exception&) {
+			/* Silently ignore errors here and assume there are no instances. */
+			return false;
+		}
 	} else {
 		Array::Ptr instances = new Array();
 		instances->Add("");
 		vinstances = instances;
 	}
 
+	bool match = false;
+
 	if (vinstances.IsObjectType<Array>()) {
 		if (!rule.GetFVVar().IsEmpty())
 			BOOST_THROW_EXCEPTION(ScriptError("Array iterator requires value to be an array.", di));
 
 		Array::Ptr arr = vinstances;
+		Array::Ptr arrclone = arr->ShallowClone();
 
-		ObjectLock olock(arr);
-		BOOST_FOREACH(const String& instance, arr) {
+		ObjectLock olock(arrclone);
+		BOOST_FOREACH(const String& instance, arrclone) {
 			String name = rule.GetName();
 
 			if (!rule.GetFKVar().IsEmpty()) {
 				frame.Locals->Set(rule.GetFKVar(), instance);
 				name += instance;
 			}
 
-			EvaluateApplyRuleInstance(checkable, name, frame, rule);
+			if (EvaluateApplyRuleInstance(checkable, name, frame, rule))
+				match = true;
 		}
 	} else if (vinstances.IsObjectType<Dictionary>()) {
 		if (rule.GetFVVar().IsEmpty())
 			BOOST_THROW_EXCEPTION(ScriptError("Dictionary iterator requires value to be a dictionary.", di));
 
 		Dictionary::Ptr dict = vinstances;
 
-		ObjectLock olock(dict);
-		BOOST_FOREACH(const Dictionary::Pair& kv, dict) {
-			frame.Locals->Set(rule.GetFKVar(), kv.first);
-			frame.Locals->Set(rule.GetFVVar(), kv.second);
+		BOOST_FOREACH(const String& key, dict->GetKeys()) {
+			frame.Locals->Set(rule.GetFKVar(), key);
+			frame.Locals->Set(rule.GetFVVar(), dict->Get(key));
 
-			EvaluateApplyRuleInstance(checkable, rule.GetName() + kv.first, frame, rule);
+			if (EvaluateApplyRuleInstance(checkable, rule.GetName() + key, frame, rule))
+				match = true;
 		}
 	}
 
-	return true;
+	return match;
 }
 
 void Dependency::EvaluateApplyRules(const Host::Ptr& host)

diff --git a/lib/icinga/dependency.hpp b/lib/icinga/dependency.hpp
@@ -65,7 +65,7 @@ class I2_ICINGA_API Dependency : public ObjectImpl<Dependency>
 	Checkable::Ptr m_Parent;
 	Checkable::Ptr m_Child;
 
-	static void EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule);
+	static bool EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule);
 	static bool EvaluateApplyRule(const Checkable::Ptr& checkable, const ApplyRule& rule);
 };
 

diff --git a/lib/icinga/notification-apply.cpp b/lib/icinga/notification-apply.cpp
@@ -41,8 +41,11 @@ void Notification::RegisterApplyRuleHandler(void)
 	ApplyRule::RegisterType("Notification", targets);
 }
 
-void Notification::EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule)
+bool Notification::EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule)
 {
+	if (!rule.EvaluateFilter(frame))
+		return false;
+
 	DebugInfo di = rule.GetDebugInfo();
 
 	Log(LogDebug, "Notification")
@@ -71,6 +74,8 @@ void Notification::EvaluateApplyRuleInstance(const Checkable::Ptr& checkable, co
 
 	ConfigItem::Ptr notificationItem = builder->Compile();
 	notificationItem->Commit();
+
+	return true;
 }
 
 bool Notification::EvaluateApplyRule(const Checkable::Ptr& checkable, const ApplyRule& rule)
@@ -92,19 +97,23 @@ bool Notification::EvaluateApplyRule(const Checkable::Ptr& checkable, const Appl
 	if (service)
 		frame.Locals->Set("service", service);
 
-	if (!rule.EvaluateFilter(frame))
-		return false;
-
 	Value vinstances;
 
 	if (rule.GetFTerm()) {
-		vinstances = rule.GetFTerm()->Evaluate(frame);
+		try {
+			vinstances = rule.GetFTerm()->Evaluate(frame);
+		} catch (const std::exception&) {
+			/* Silently ignore errors here and assume there are no instances. */
+			return false;
+		}
 	} else {
 		Array::Ptr instances = new Array();
 		instances->Add("");
 		vinstances = instances;
 	}
 
+	bool match = false;
+
 	if (vinstances.IsObjectType<Array>()) {
 		if (!rule.GetFVVar().IsEmpty())
 			BOOST_THROW_EXCEPTION(ScriptError("Array iterator requires value to be an array.", di));
@@ -120,24 +129,25 @@ bool Notification::EvaluateApplyRule(const Checkable::Ptr& checkable, const Appl
 				name += instance;
 			}
 
-			EvaluateApplyRuleInstance(checkable, name, frame, rule);
+			if (EvaluateApplyRuleInstance(checkable, name, frame, rule))
+				match = true;
 		}
 	} else if (vinstances.IsObjectType<Dictionary>()) {
 		if (rule.GetFVVar().IsEmpty())
 			BOOST_THROW_EXCEPTION(ScriptError("Dictionary iterator requires value to be a dictionary.", di));
 
 		Dictionary::Ptr dict = vinstances;
 
-		ObjectLock olock(dict);
-		BOOST_FOREACH(const Dictionary::Pair& kv, dict) {
-			frame.Locals->Set(rule.GetFKVar(), kv.first);
-			frame.Locals->Set(rule.GetFVVar(), kv.second);
+		BOOST_FOREACH(const String& key, dict->GetKeys()) {
+			frame.Locals->Set(rule.GetFKVar(), key);
+			frame.Locals->Set(rule.GetFVVar(), dict->Get(key));
 
-			EvaluateApplyRuleInstance(checkable, rule.GetName() + kv.first, frame, rule);
+			if (EvaluateApplyRuleInstance(checkable, rule.GetName() + key, frame, rule))
+				match = true;
 		}
 	}
 
-	return true;
+	return match;
 }
 
 void Notification::EvaluateApplyRules(const Host::Ptr& host)

diff --git a/lib/icinga/notification.hpp b/lib/icinga/notification.hpp
@@ -123,7 +123,7 @@ class I2_ICINGA_API Notification : public ObjectImpl<Notification>
 private:
 	void ExecuteNotificationHelper(NotificationType type, const User::Ptr& user, const CheckResult::Ptr& cr, bool force, const String& author = "", const String& text = "");
 
-	static void EvaluateApplyRuleInstance(const intrusive_ptr<Checkable>& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule);
+	static bool EvaluateApplyRuleInstance(const intrusive_ptr<Checkable>& checkable, const String& name, ScriptFrame& frame, const ApplyRule& rule);
 	static bool EvaluateApplyRule(const intrusive_ptr<Checkable>& checkable, const ApplyRule& rule);
 };