Merge pull request #9893 from Icinga/do-not-re-notify-if-filtered-states-don-t-change-4503

Discard likely duplicate problem notifications via Notification#last_notified_state_per_user

doc/19-technical-concepts.md

@@ -1514,6 +1514,76 @@ Message updates will be dropped when:
 * Notification does not exist.
 * Origin endpoint's zone is not allowed to access this checkable.
 
+#### event::UpdateLastNotifiedStatePerUser <a id="technical-concepts-json-rpc-messages-event-updatelastnotifiedstateperuser"></a>
+
+> Location: `clusterevents.cpp`
+
+##### Message Body
+
+Key       | Value
+----------|---------
+jsonrpc   | 2.0
+method    | event::UpdateLastNotifiedStatePerUser
+params    | Dictionary
+
+##### Params
+
+Key          | Type   | Description
+-------------|--------|------------------
+notification | String | Notification name
+user         | String | User name
+state        | Number | Checkable state the user just got a problem notification for
+
+Used to sync the state of a notification object within the same HA zone.
+
+##### Functions
+
+Event Sender: `Notification::OnLastNotifiedStatePerUserUpdated`
+Event Receiver: `LastNotifiedStatePerUserUpdatedAPIHandler`
+
+##### Permissions
+
+The receiver will not process messages from not configured endpoints.
+
+Message updates will be dropped when:
+
+* Notification does not exist.
+* Origin endpoint is not within the local zone.
+
+#### event::ClearLastNotifiedStatePerUser <a id="technical-concepts-json-rpc-messages-event-clearlastnotifiedstateperuser"></a>
+
+> Location: `clusterevents.cpp`
+
+##### Message Body
+
+Key       | Value
+----------|---------
+jsonrpc   | 2.0
+method    | event::ClearLastNotifiedStatePerUser
+params    | Dictionary
+
+##### Params
+
+Key          | Type   | Description
+-------------|--------|------------------
+notification | String | Notification name
+
+Used to sync the state of a notification object within the same HA zone.
+
+##### Functions
+
+Event Sender: `Notification::OnLastNotifiedStatePerUserCleared`
+Event Receiver: `LastNotifiedStatePerUserClearedAPIHandler`
+
+##### Permissions
+
+The receiver will not process messages from not configured endpoints.
+
+Message updates will be dropped when:
+
+* Notification does not exist.
+* Origin endpoint is not within the local zone.
+
 #### event::SetForceNextCheck <a id="technical-concepts-json-rpc-messages-event-setforcenextcheck"></a>
 
 > Location: `clusterevents.cpp`

lib/icinga/clusterevents.cpp

@@ -29,6 +29,8 @@ REGISTER_APIFUNCTION(SetStateBeforeSuppression, event, &ClusterEvents::StateBefo
 REGISTER_APIFUNCTION(SetSuppressedNotifications, event, &ClusterEvents::SuppressedNotificationsChangedAPIHandler);
 REGISTER_APIFUNCTION(SetSuppressedNotificationTypes, event, &ClusterEvents::SuppressedNotificationTypesChangedAPIHandler);
 REGISTER_APIFUNCTION(SetNextNotification, event, &ClusterEvents::NextNotificationChangedAPIHandler);
+REGISTER_APIFUNCTION(UpdateLastNotifiedStatePerUser, event, &ClusterEvents::LastNotifiedStatePerUserUpdatedAPIHandler);
+REGISTER_APIFUNCTION(ClearLastNotifiedStatePerUser, event, &ClusterEvents::LastNotifiedStatePerUserClearedAPIHandler);
 REGISTER_APIFUNCTION(SetForceNextCheck, event, &ClusterEvents::ForceNextCheckChangedAPIHandler);
 REGISTER_APIFUNCTION(SetForceNextNotification, event, &ClusterEvents::ForceNextNotificationChangedAPIHandler);
 REGISTER_APIFUNCTION(SetAcknowledgement, event, &ClusterEvents::AcknowledgementSetAPIHandler);
@@ -50,6 +52,8 @@ void ClusterEvents::StaticInitialize()
 	Checkable::OnSuppressedNotificationsChanged.connect(&ClusterEvents::SuppressedNotificationsChangedHandler);
 	Notification::OnSuppressedNotificationsChanged.connect(&ClusterEvents::SuppressedNotificationTypesChangedHandler);
 	Notification::OnNextNotificationChanged.connect(&ClusterEvents::NextNotificationChangedHandler);
+	Notification::OnLastNotifiedStatePerUserUpdated.connect(&ClusterEvents::LastNotifiedStatePerUserUpdatedHandler);
+	Notification::OnLastNotifiedStatePerUserCleared.connect(&ClusterEvents::LastNotifiedStatePerUserClearedHandler);
 	Checkable::OnForceNextCheckChanged.connect(&ClusterEvents::ForceNextCheckChangedHandler);
 	Checkable::OnForceNextNotificationChanged.connect(&ClusterEvents::ForceNextNotificationChangedHandler);
 	Checkable::OnNotificationsRequested.connect(&ClusterEvents::SendNotificationsHandler);
@@ -529,6 +533,115 @@ Value ClusterEvents::NextNotificationChangedAPIHandler(const MessageOrigin::Ptr&
 	return Empty;
 }
 
+void ClusterEvents::LastNotifiedStatePerUserUpdatedHandler(const Notification::Ptr& notification, const String& user, uint_fast8_t state, const MessageOrigin::Ptr& origin)
+{
+	auto listener (ApiListener::GetInstance());
+
+	if (!listener) {
+		return;
+	}
+
+	Dictionary::Ptr params = new Dictionary();
+	params->Set("notification", notification->GetName());
+	params->Set("user", user);
+	params->Set("state", state);
+
+	Dictionary::Ptr message = new Dictionary();
+	message->Set("jsonrpc", "2.0");
+	message->Set("method", "event::UpdateLastNotifiedStatePerUser");
+	message->Set("params", params);
+
+	listener->RelayMessage(origin, notification, message, true);
+}
+
+Value ClusterEvents::LastNotifiedStatePerUserUpdatedAPIHandler(const MessageOrigin::Ptr& origin, const Dictionary::Ptr& params)
+{
+	auto endpoint (origin->FromClient->GetEndpoint());
+
+	if (!endpoint) {
+		Log(LogNotice, "ClusterEvents")
+			<< "Discarding 'last notified state of user updated' message from '" << origin->FromClient->GetIdentity() << "': Invalid endpoint origin (client not allowed).";
+
+		return Empty;
+	}
+
+	if (origin->FromZone && origin->FromZone != Zone::GetLocalZone()) {
+		Log(LogNotice, "ClusterEvents")
+			<< "Discarding 'last notified state of user updated' message from '"
+			<< origin->FromClient->GetIdentity() << "': Unauthorized access.";
+
+		return Empty;
+	}
+
+	auto notification (Notification::GetByName(params->Get("notification")));
+
+	if (!notification) {
+		return Empty;
+	}
+
+	auto state (params->Get("state"));
+
+	if (!state.IsNumber()) {
+		return Empty;
+	}
+
+	notification->GetLastNotifiedStatePerUser()->Set(params->Get("user"), state);
+	Notification::OnLastNotifiedStatePerUserUpdated(notification, params->Get("user"), state, origin);
+
+	return Empty;
+}
+
+void ClusterEvents::LastNotifiedStatePerUserClearedHandler(const Notification::Ptr& notification, const MessageOrigin::Ptr& origin)
+{
+	auto listener (ApiListener::GetInstance());
+
+	if (!listener) {
+		return;
+	}
+
+	Dictionary::Ptr params = new Dictionary();
+	params->Set("notification", notification->GetName());
+
+	Dictionary::Ptr message = new Dictionary();
+	message->Set("jsonrpc", "2.0");
+	message->Set("method", "event::ClearLastNotifiedStatePerUser");
+	message->Set("params", params);
+
+	listener->RelayMessage(origin, notification, message, true);
+}
+
+Value ClusterEvents::LastNotifiedStatePerUserClearedAPIHandler(const MessageOrigin::Ptr& origin, const Dictionary::Ptr& params)
+{
+	auto endpoint (origin->FromClient->GetEndpoint());
+
+	if (!endpoint) {
+		Log(LogNotice, "ClusterEvents")
+			<< "Discarding 'last notified state of user cleared' message from '"
+			<< origin->FromClient->GetIdentity() << "': Invalid endpoint origin (client not allowed).";
+
+		return Empty;
+	}
+
+	if (origin->FromZone && origin->FromZone != Zone::GetLocalZone()) {
+		Log(LogNotice, "ClusterEvents")
+			<< "Discarding 'last notified state of user cleared' message from '"
+			<< origin->FromClient->GetIdentity() << "': Unauthorized access.";
+
+		return Empty;
+	}
+
+	auto notification (Notification::GetByName(params->Get("notification")));
+
+	if (!notification) {
+		return Empty;
+	}
+
+	notification->GetLastNotifiedStatePerUser()->Clear();
+	Notification::OnLastNotifiedStatePerUserCleared(notification, origin);
+
+	return Empty;
+}
+
 void ClusterEvents::ForceNextCheckChangedHandler(const Checkable::Ptr& checkable, const MessageOrigin::Ptr& origin)
 {
 	ApiListener::Ptr listener = ApiListener::GetInstance();

lib/icinga/clusterevents.hpp

@@ -41,6 +41,12 @@ public:
 	static void NextNotificationChangedHandler(const Notification::Ptr& notification, const MessageOrigin::Ptr& origin);
 	static Value NextNotificationChangedAPIHandler(const MessageOrigin::Ptr& origin, const Dictionary::Ptr& params);
 
+	static void LastNotifiedStatePerUserUpdatedHandler(const Notification::Ptr& notification, const String& user, uint_fast8_t state, const MessageOrigin::Ptr& origin);
+	static Value LastNotifiedStatePerUserUpdatedAPIHandler(const MessageOrigin::Ptr& origin, const Dictionary::Ptr& params);
+
+	static void LastNotifiedStatePerUserClearedHandler(const Notification::Ptr& notification, const MessageOrigin::Ptr& origin);
+	static Value LastNotifiedStatePerUserClearedAPIHandler(const MessageOrigin::Ptr& origin, const Dictionary::Ptr& params);
+
 	static void ForceNextCheckChangedHandler(const Checkable::Ptr& checkable, const MessageOrigin::Ptr& origin);
 	static Value ForceNextCheckChangedAPIHandler(const MessageOrigin::Ptr& origin, const Dictionary::Ptr& params);
 

lib/icinga/notification.cpp

@@ -23,6 +23,8 @@ std::map<String, int> Notification::m_StateFilterMap;
 std::map<String, int> Notification::m_TypeFilterMap;
 
 boost::signals2::signal<void (const Notification::Ptr&, const MessageOrigin::Ptr&)> Notification::OnNextNotificationChanged;
+boost::signals2::signal<void (const Notification::Ptr&, const String&, uint_fast8_t, const MessageOrigin::Ptr&)> Notification::OnLastNotifiedStatePerUserUpdated;
+boost::signals2::signal<void (const Notification::Ptr&, const MessageOrigin::Ptr&)> Notification::OnLastNotifiedStatePerUserCleared;
 
 String NotificationNameComposer::MakeName(const String& shortName, const Object::Ptr& context) const
 {
@@ -231,6 +233,13 @@ void Notification::BeginExecuteNotification(NotificationType type, const CheckRe
 		<< "notifications of type '" << notificationTypeName
 		<< "' for notification object '" << notificationName << "'.";
 
+	if (type == NotificationRecovery) {
+		auto states (GetLastNotifiedStatePerUser());
+
+		states->Clear();
+		OnLastNotifiedStatePerUserCleared(this, nullptr);
+	}
+
 	Checkable::Ptr checkable = GetCheckable();
 
 	if (!force) {
@@ -439,6 +448,22 @@ void Notification::BeginExecuteNotification(NotificationType type, const CheckRe
 			}
 		}
 
+		if (type == NotificationProblem && !reminder && !checkable->GetVolatile()) {
+			auto [host, service] = GetHostService(checkable);
+			uint_fast8_t state = service ? service->GetState() : host->GetState();
+
+			if (state == (uint_fast8_t)GetLastNotifiedStatePerUser()->Get(userName)) {
+				auto stateStr (service ? NotificationServiceStateToString(service->GetState()) : NotificationHostStateToString(host->GetState()));
+
+				Log(LogNotice, "Notification")
+					<< "Notification object '" << notificationName << "': We already notified user '" << userName << "' for a " << stateStr
+					<< " problem. Likely after that another state change notification was filtered out by config. Not sending duplicate '"
+					<< stateStr << "' notification.";
+
+				continue;
+			}
+		}
+
 		Log(LogInformation, "Notification")
 			<< "Sending " << (reminder ? "reminder " : "") << "'" << NotificationTypeToString(type) << "' notification '"
 			<< notificationName << "' for user '" << userName << "'";
@@ -452,6 +477,16 @@ void Notification::BeginExecuteNotification(NotificationType type, const CheckRe
 		/* collect all notified users */
 		allNotifiedUsers.insert(user);
 
+		if (type == NotificationProblem) {
+			auto [host, service] = GetHostService(checkable);
+			uint_fast8_t state = service ? service->GetState() : host->GetState();
+
+			if (state != (uint_fast8_t)GetLastNotifiedStatePerUser()->Get(userName)) {
+				GetLastNotifiedStatePerUser()->Set(userName, state);
+				OnLastNotifiedStatePerUserUpdated(this, userName, state, nullptr);
+			}
+		}
+
 		/* store all notified users for later recovery checks */
 		if (type == NotificationProblem && !notifiedProblemUsers->Contains(userName))
 			notifiedProblemUsers->Add(userName);

lib/icinga/notification.hpp

@@ -13,6 +13,7 @@
 #include "remote/endpoint.hpp"
 #include "remote/messageorigin.hpp"
 #include "base/array.hpp"
+#include <cstdint>
 
 namespace icinga
 {
@@ -92,6 +93,8 @@ public:
 	static String NotificationHostStateToString(HostState state);
 
 	static boost::signals2::signal<void (const Notification::Ptr&, const MessageOrigin::Ptr&)> OnNextNotificationChanged;
+	static boost::signals2::signal<void (const Notification::Ptr&, const String&, uint_fast8_t, const MessageOrigin::Ptr&)> OnLastNotifiedStatePerUserUpdated;
+	static boost::signals2::signal<void (const Notification::Ptr&, const MessageOrigin::Ptr&)> OnLastNotifiedStatePerUserCleared;
 
 	void Validate(int types, const ValidationUtils& utils) override;
 
@@ -105,7 +108,6 @@ public:
 	static const std::map<String, int>& GetStateFilterMap();
 	static const std::map<String, int>& GetTypeFilterMap();
 
-protected:
 	void OnConfigLoaded() override;
 	void OnAllConfigLoaded() override;
 	void Start(bool runtimeCreated) override;

lib/icinga/notification.ti

@@ -90,6 +90,10 @@ class Notification : CustomVarObject < NotificationNameComposer
 		default {{{ return 0; }}}
 	};
 
+	[state, no_user_view, no_user_modify] Dictionary::Ptr last_notified_state_per_user {
+		default {{{ return new Dictionary(); }}}
+	};
+
 	[config, navigation] name(Endpoint) command_endpoint (CommandEndpointRaw) {
 		navigate {{{
 			return Endpoint::GetByName(GetCommandEndpointRaw());

test/CMakeLists.txt

@@ -138,6 +138,11 @@ add_boost_test(base
     icinga_notification/strings
     icinga_notification/state_filter
     icinga_notification/type_filter
+    icinga_notification/no_filter_problem_no_duplicate
+    icinga_notification/filter_problem_no_duplicate
+    icinga_notification/volatile_filter_problem_duplicate
+    icinga_notification/no_recovery_filter_no_duplicate
+    icinga_notification/recovery_filter_duplicate
     icinga_macros/simple
     icinga_legacytimeperiod/simple
     icinga_legacytimeperiod/advanced

test/icinga-notification.cpp

@@ -1,11 +1,74 @@
 /* Icinga 2 | (c) 2012 Icinga GmbH | GPLv2+ */
 
+#include "icinga/host.hpp"
 #include "icinga/notification.hpp"
+#include "icinga/notificationcommand.hpp"
+#include "icinga/service.hpp"
+#include "icinga/user.hpp"
 #include <BoostTestTargetConfig.h>
 #include <iostream>
 
 using namespace icinga;
 
+struct DuplicateDueToFilterHelper
+{
+	Host::Ptr h = new Host();
+	Service::Ptr s = new Service();
+	User::Ptr u = new User();
+	NotificationCommand::Ptr nc = new NotificationCommand();
+	Notification::Ptr n = new Notification();
+	unsigned int called = 0;
+
+	DuplicateDueToFilterHelper(int typeFilter, int stateFilter)
+	{
+		h->SetName("example.com", true);
+		h->Register();
+
+		s->SetShortName("disk", true);
+		h->AddService(s);
+
+		u->SetName("jdoe", true);
+		u->SetTypeFilter(~0);
+		u->SetStateFilter(~0);
+		u->Register();
+
+		nc->SetName("mail", true);
+		nc->SetExecute(new Function("", [this]() { ++called; }), true);
+		nc->Register();
+
+		n->SetFieldByName("host_name", "example.com", false, DebugInfo());
+		n->SetFieldByName("service_name", "disk", false, DebugInfo());
+		n->SetFieldByName("command", "mail", false, DebugInfo());
+		n->SetUsersRaw(new Array({"jdoe"}), true);
+		n->SetTypeFilter(typeFilter);
+		n->SetStateFilter(stateFilter);
+		n->OnAllConfigLoaded(); // link Service
+	}
+
+	~DuplicateDueToFilterHelper()
+	{
+		h->Unregister();
+		u->Unregister();
+		nc->Unregister();
+	}
+
+	void SendStateNotification(ServiceState state, bool isSent)
+	{
+		auto calledBefore (called);
+
+		s->SetStateRaw(state, true);
+		Application::GetTP().Start();
+
+		n->BeginExecuteNotification(
+			state == ServiceOK ? NotificationRecovery : NotificationProblem,
+			nullptr, false, false, "", ""
+		);
+
+		Application::GetTP().Stop();
+		BOOST_CHECK_EQUAL(called > calledBefore, isSent);
+	}
+};
+
 BOOST_AUTO_TEST_SUITE(icinga_notification)
 
 BOOST_AUTO_TEST_CASE(strings)
@@ -102,4 +165,51 @@ BOOST_AUTO_TEST_CASE(type_filter)
 	std::cout << "#4 Notification type: " << ftype << " against " << notification->GetTypeFilter() << " must fail." << std::endl;
 	BOOST_CHECK(!(notification->GetTypeFilter() & ftype));
 }
+
+BOOST_AUTO_TEST_CASE(no_filter_problem_no_duplicate)
+{
+	DuplicateDueToFilterHelper helper (~0, ~0);
+
+	helper.SendStateNotification(ServiceCritical, true);
+	helper.SendStateNotification(ServiceWarning, true);
+	helper.SendStateNotification(ServiceCritical, true);
+}
+
+BOOST_AUTO_TEST_CASE(filter_problem_no_duplicate)
+{
+	DuplicateDueToFilterHelper helper (~0, ~StateFilterWarning);
+
+	helper.SendStateNotification(ServiceCritical, true);
+	helper.SendStateNotification(ServiceWarning, false);
+	helper.SendStateNotification(ServiceCritical, false);
+}
+
+BOOST_AUTO_TEST_CASE(volatile_filter_problem_duplicate)
+{
+	DuplicateDueToFilterHelper helper (~0, ~StateFilterWarning);
+
+	helper.s->SetVolatile(true, true);
+	helper.SendStateNotification(ServiceCritical, true);
+	helper.SendStateNotification(ServiceWarning, false);
+	helper.SendStateNotification(ServiceCritical, true);
+}
+
+BOOST_AUTO_TEST_CASE(no_recovery_filter_no_duplicate)
+{
+	DuplicateDueToFilterHelper helper (~0, ~0);
+
+	helper.SendStateNotification(ServiceCritical, true);
+	helper.SendStateNotification(ServiceOK, true);
+	helper.SendStateNotification(ServiceCritical, true);
+}
+
+BOOST_AUTO_TEST_CASE(recovery_filter_duplicate)
+{
+	DuplicateDueToFilterHelper helper (~NotificationRecovery, ~0);
+
+	helper.SendStateNotification(ServiceCritical, true);
+	helper.SendStateNotification(ServiceOK, false);
+	helper.SendStateNotification(ServiceCritical, true);
+}
+
 BOOST_AUTO_TEST_SUITE_END()