Merge pull request #7695 from Icinga/bugfix/double-ack

Refuse acking an already acked checkable

doc/16-upgrading-icinga-2.md

@@ -8,6 +8,13 @@ Specific version upgrades are described below. Please note that version
 updates are incremental. An upgrade from v2.6 to v2.8 requires to
 follow the instructions for v2.7 too.
 
+## Upgrading to v2.12 <a id="upgrading-to-2-12"></a>
+
+### Breaking changes <a id="upgrading-to-2-12-breaking-changes"></a>
+
+As of v2.12 our [API](12-icinga2-api.md) URL endpoint [`/v1/actions/acknowledge-problem`](12-icinga2-api.md#icinga2-api-actions-acknowledge-problem) refuses acknowledging an already acknowledged checkable by overwriting the acknowledgement.
+To replace an acknowledgement you have to remove the old one before adding the new one.
+
 ## Upgrading to v2.11 <a id="upgrading-to-2-11"></a>
 
 ### Bugfixes for 2.11 <a id="upgrading-to-2-11-bugfixes"></a>

lib/icinga/apiactions.cpp

@@ -214,6 +214,8 @@ Dictionary::Ptr ApiActions::AcknowledgeProblem(const ConfigObject::Ptr& object,
 	} else
 		timestamp = 0;
 
+	ObjectLock oLock (checkable);
+
 	Host::Ptr host;
 	Service::Ptr service;
 	tie(host, service) = GetHostService(checkable);
@@ -226,6 +228,10 @@ Dictionary::Ptr ApiActions::AcknowledgeProblem(const ConfigObject::Ptr& object,
 			return ApiActions::CreateResult(409, "Service " + checkable->GetName() + " is OK.");
 	}
 
+	if (checkable->IsAcknowledged()) {
+		return ApiActions::CreateResult(409, (service ? "Service " : "Host ") + checkable->GetName() + " is already acknowledged.");
+	}
+
 	Comment::AddComment(checkable, CommentAcknowledgement, HttpUtility::GetLastParameter(params, "author"),
 		HttpUtility::GetLastParameter(params, "comment"), persistent, timestamp);
 	checkable->AcknowledgeProblem(HttpUtility::GetLastParameter(params, "author"),

lib/icinga/checkable.cpp

@@ -138,13 +138,27 @@ void Checkable::AcknowledgeProblem(const String& author, const String& comment,
 
 void Checkable::ClearAcknowledgement(const String& removedBy, const MessageOrigin::Ptr& origin)
 {
+	ObjectLock oLock (this);
+
+	bool wasAcked;
+
+	if (GetAcknowledgementRaw() == AcknowledgementNone) {
+		wasAcked = false;
+	} else {
+		double expiry = GetAcknowledgementExpiry();
+
+		wasAcked = expiry == 0 || expiry >= Utility::GetTime();
+	}
+
 	SetAcknowledgementRaw(AcknowledgementNone);
 	SetAcknowledgementExpiry(0);
 
 	Log(LogInformation, "Checkable")
 		<< "Acknowledgement cleared for checkable '" << GetName() << "'.";
 
-	OnAcknowledgementCleared(this, removedBy, origin);
+	if (wasAcked) {
+		OnAcknowledgementCleared(this, removedBy, origin);
+	}
 }
 
 Endpoint::Ptr Checkable::GetCommandEndpoint() const

lib/icinga/clusterevents.cpp

@@ -535,6 +535,15 @@ Value ClusterEvents::AcknowledgementSetAPIHandler(const MessageOrigin::Ptr& orig
 		return Empty;
 	}
 
+	ObjectLock oLock (checkable);
+
+	if (checkable->IsAcknowledged()) {
+		Log(LogWarning, "ClusterEvents")
+			<< "Discarding 'acknowledgement set' message for checkable '" << checkable->GetName()
+			<< "' from '" << origin->FromClient->GetIdentity() << "': Checkable is already acknowledged.";
+		return Empty;
+	}
+
 	checkable->AcknowledgeProblem(params->Get("author"), params->Get("comment"),
 		static_cast<AcknowledgementType>(static_cast<int>(params->Get("acktype"))),
 		params->Get("notify"), params->Get("persistent"), params->Get("expiry"), origin);

lib/icinga/externalcommandprocessor.cpp

@@ -588,6 +588,7 @@ void ExternalCommandProcessor::AcknowledgeSvcProblem(double, const std::vector<S
 	bool persistent = (Convert::ToLong(arguments[4]) > 0 ? true : false);
 
 	Service::Ptr service = Service::GetByNamePair(arguments[0], arguments[1]);
+	ObjectLock oLock (service);
 
 	if (!service)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Cannot acknowledge service problem for non-existent service '" + arguments[1] + "' on host '" + arguments[0] + "'"));
@@ -595,6 +596,10 @@ void ExternalCommandProcessor::AcknowledgeSvcProblem(double, const std::vector<S
 	if (service->GetState() == ServiceOK)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("The service '" + arguments[1] + "' is OK."));
 
+	if (service->IsAcknowledged()) {
+		BOOST_THROW_EXCEPTION(std::invalid_argument("The service '" + arguments[1] + "' is already acknowledged."));
+	}
+
 	Log(LogNotice, "ExternalCommandProcessor")
 		<< "Setting acknowledgement for service '" << service->GetName() << "'" << (notify ? "" : ". Disabled notification");
 
@@ -610,6 +615,7 @@ void ExternalCommandProcessor::AcknowledgeSvcProblemExpire(double, const std::ve
 	double timestamp = Convert::ToDouble(arguments[5]);
 
 	Service::Ptr service = Service::GetByNamePair(arguments[0], arguments[1]);
+	ObjectLock oLock (service);
 
 	if (!service)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Cannot acknowledge service problem with expire time for non-existent service '" + arguments[1] + "' on host '" + arguments[0] + "'"));
@@ -620,6 +626,10 @@ void ExternalCommandProcessor::AcknowledgeSvcProblemExpire(double, const std::ve
 	if (timestamp != 0 && timestamp <= Utility::GetTime())
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Acknowledgement expire time must be in the future for service '" + arguments[1] + "' on host '" + arguments[0] + "'"));
 
+	if (service->IsAcknowledged()) {
+		BOOST_THROW_EXCEPTION(std::invalid_argument("The service '" + arguments[1] + "' is already acknowledged."));
+	}
+
 	Log(LogNotice, "ExternalCommandProcessor")
 		<< "Setting timed acknowledgement for service '" << service->GetName() << "'" << (notify ? "" : ". Disabled notification");
 
@@ -652,6 +662,7 @@ void ExternalCommandProcessor::AcknowledgeHostProblem(double, const std::vector<
 	bool persistent = (Convert::ToLong(arguments[3]) > 0 ? true : false);
 
 	Host::Ptr host = Host::GetByName(arguments[0]);
+	ObjectLock oLock (host);
 
 	if (!host)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Cannot acknowledge host problem for non-existent host '" + arguments[0] + "'"));
@@ -662,6 +673,10 @@ void ExternalCommandProcessor::AcknowledgeHostProblem(double, const std::vector<
 	if (host->GetState() == HostUp)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("The host '" + arguments[0] + "' is OK."));
 
+	if (host->IsAcknowledged()) {
+		BOOST_THROW_EXCEPTION(std::invalid_argument("The host '" + arguments[1] + "' is already acknowledged."));
+	}
+
 	Comment::AddComment(host, CommentAcknowledgement, arguments[4], arguments[5], persistent, 0);
 	host->AcknowledgeProblem(arguments[4], arguments[5], sticky ? AcknowledgementSticky : AcknowledgementNormal, notify, persistent);
 }
@@ -674,6 +689,7 @@ void ExternalCommandProcessor::AcknowledgeHostProblemExpire(double, const std::v
 	double timestamp = Convert::ToDouble(arguments[4]);
 
 	Host::Ptr host = Host::GetByName(arguments[0]);
+	ObjectLock oLock (host);
 
 	if (!host)
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Cannot acknowledge host problem with expire time for non-existent host '" + arguments[0] + "'"));
@@ -687,6 +703,10 @@ void ExternalCommandProcessor::AcknowledgeHostProblemExpire(double, const std::v
 	if (timestamp != 0 && timestamp <= Utility::GetTime())
 		BOOST_THROW_EXCEPTION(std::invalid_argument("Acknowledgement expire time must be in the future for host '" + arguments[0] + "'"));
 
+	if (host->IsAcknowledged()) {
+		BOOST_THROW_EXCEPTION(std::invalid_argument("The host '" + arguments[1] + "' is already acknowledged."));
+	}
+
 	Comment::AddComment(host, CommentAcknowledgement, arguments[5], arguments[6], persistent, timestamp);
 	host->AcknowledgeProblem(arguments[5], arguments[6], sticky ? AcknowledgementSticky : AcknowledgementNormal, notify, persistent, timestamp);
 }