Open
Conversation
Member
Author
TODO
|
This reverts commit 106ae7b.
This reverts commit aa1886d.
This reverts commit 33b2934.
This reverts commit f3870a3.
Since the event rules are dependent on their corresponding sources, we don't need to add another layer of indirection via an extra "source -> IDs" cache in the RuntimeConfig. Instead, we can directly store the rule IDs within the Source struct and thus bound to the source's lifecycle.
This effectively replaces the previous (prior to v0.2.0) implementation on the `Object` type.
3a06180 to
f2e3457
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR kind of reverts all the changes mad in v0.2.0 with a slight twist. We will evaluate the rules on our own again (just like pre v0.2.0) but the filter string is expected to have a different format. Icinga Notifications Web will now store the filter string as a JSON stringified object instead of a simple filter string format used before. This is done allegedly to make it easier to parse the filter string but the previous filter string parser is still there as it's used by the escalation filters. The new filter string format is as follows:
{ "op": "&/|/!", "rules": [ { "column": "service.state", "op": "==/!=/>/</~/!~", "value": "OK" }, { "op": "&/|/!", "rules": [ ... ] } ] }The JSON object for filter conditions may include other meta information used by Icinga Notifications Web/Sources but the only relevant information for us is the above-mentioned format. The
opfield is the operator used to combine the filter chain rules and can be either&(AND),|(OR) or!(NOT). Therulesfield is an array of rules which can be either a simple condition or a nested filter chain. A simple condition consists of acolumn, anopand avalue. Thecolumnis the column name used to extract values from requests as JSONPath expressions, theopis the operator used to compare the JSONPath expression result with the provided value and can be either==,!=,>,<,~(regex match) or!~(regex not match). If thecolumnis not a valid JSONPath expression, Icinga Notifications will just ignore that rule and won't be loading it from the database. This means that if you have an existing filter string in the old format, they won't be loaded, and you will have to ask the web colleagues if they provide a migration step for the existing filter strings.Furthermore, prior to v0.2.0, we were evaluating the rules against the
Objectrepresentation of the request, now they are evaluated against theEventtype itself. Specifically, the JSONPath expressions are evaluated against the newrelationsfield of theEventtype which is a map of generic key-value pairs extracted from the request. When a source doesn't include that field with all the necessary information in its requests, some or even all the configured rules might not match if they reference the missing information in theirobject_filter. If the source wishes to be notified about the missing information, it can include the newX-Icinga-Enable-Attributes-NegotiationHTTP header set totruein its requests and Icinga Notifications will respond with a422 Unprocessable Entitystatus code (not final, is still up to a discussion) and a JSON body containing the missing attributes. This is a new feature but similar to the previous behavior introduced in v0.2.0 where we were responding with a412 Precondition Failedstatus code when the used rules_version was outdated. The source can then extend its requests with the missing information and retry the request.In the referenced issue, it was mentioned that sources might only re-send the missing information as a follow-up request instead of retrying the entire request. However, it's not worth implementing this behavior just for the sake of this single use case as it would require a lot of additional work and complexity on both the source and Icinga Notifications sides.
Lastly, in a discussion with @nilmerg yesterday, we agreed that Icinga Notifications should abandon all the effects of the ongoing request if it can't successfully notify the source that the request was successfully processed. Currently, the incident starts a database transaction internally and commits it before trying to send the resulting notifications. However, the client will only receive
200 OKif the notifications were sent successfully, and that in turn might not reach the source since the connection might be gone in the meantime (errors are ignored blantantly).icinga-notifications/internal/listener/listener.go
Lines 181 to 185 in aad8cb2
icinga-notifications/internal/incident/incident.go
Lines 221 to 229 in aad8cb2
In that case the source will be left in a state where the request was processed but the source wasn't notified about it, which is not ideal and will definitely lead to some issues when we've added HA support. Icinga Notifications will treat every event as a unique one and won't have any mechanism to detect duplicate events, so if the source retries the request, it will be processed again and might lead to some unwanted side effects. To avoid that, we will need to roll back all the changes made by the request if we fail to notify the source about the successful processing of the request.
I didn't implement this behavior in this PR as I see it as a separate task that can be done in a follow-up PR, but if you think it should be included in this PR, please let me know and I will add it.Forget that, that's really something that should be done after or while implementing the HA support.resolves #406