Move Rule evaluation to Source

[oxzi]

This is a first version to move the rule evaluation from Icinga Notifications into the source, starting with Icinga DB.

For an end user or another computer, the /process-event API endpoint was slightly modified. Two new HTTP request headers were introduced to tell which rules match and to share the state.

Another new /event-rules API endpoint allows querying all rules.

---

Other PRs are

• Initial Icinga Notifications Source icingadb#998 and
• Import notifications packages icinga-go-library#145.

---

At the moment, there is no real support in Icinga Notifications Web. Thus, start by creating a rule through the current web interface and update the data in the relational database. Consider the following:

notifications=# select name, object_filter from rule;
-[ RECORD 1 ]-+------------------------------------------------------------------------------------------------
name          | hosts with 1
object_filter | select * from host where id = :host_id and environment_id = :environment_id and name like '%1%'
-[ RECORD 2 ]-+------------------------------------------------------------------------------------------------
name          | only services
object_filter | select * from service where id = :service_id and environment_id = :environment_id
-[ RECORD 3 ]-+------------------------------------------------------------------------------------------------
name          | fair dice
object_filter | select rand() as r having r >= 0.4

[yhabteab]

Likewise to the Icinga DB PR, I've changed quite a few things as well, so I'll summarize the changes here for you:

• You've previously used the event.ExtraTags field to store the matched event rule IDs received from source, but in order to completely get rid of the extra_tags term from the codebase, I've introduced a new field called MatchedRules on the Event struct. It serves the same purpose as event.ExtraTags, but it is more descriptive and aligns with the existing internal usages and is filled by the new method called event.LoadMatchedRulesFromString(). This method is called by listener whenever it receives an event from source with the X-Icinga-Rules-Id header. In the same commit, I've also renamed incident#evaluateRules() to incident#applyMatchingRules() to better reflect its purpose, since it no longer evaluates event rules, but rather applies the matched rules to its rules cache. The relevant commit for this change is 1530def.
• I've removed the extra_tags or ExtraTags term without a trace from our codebase, so you won't find any references to it in the code or database schema anymore. As part of this change, I've also removed all the example JSON files from the event package that contained the extra_tags field, since they are no longer relevant. You can find the relevant commit for this change in 39293b2.
• Likewise, I've removed the icinga2 client without a trace as well. The commit that contains this change is 30ae8e2.
• Just like in the Icinga DB PR, 8e02472 makes use of the now outsourced common, notifications-related code from our Icinga Go Library, and removes the now redundant event.Severity, event.Type etc. types. Apart from that, the Event struct now embeds the event.Event type from the Icinga Go Library, and it doesn't contain any fields that are required to be set by clients in way or another, it's just there to provide other internal non-JSON serializable fields used by Icinga Notifications only.
• You've previously used the highest changed_at timestamp of the event rules as their version, but since the hidden internal document requires that Icinga Notifications should maintain the version of the event rules that are relevant for any given source, the implementation for this has become quickly more complex. So, I've moved the version tracking logic to the runtime config type, since it has the knowledge about when the event rules change and it can easily track the version of the event rules of any given source. Instead of using time-based versioning, it now uses a simple monotonically increasing integer value for each source. With this change, changing the config of an event rule from source A will not affect the version of the event rules of source B, since they are now tracked separately. Consequently, the listener will only send the event rules to clients that are relevant for that source. To achieve this, I've introduced a source_id database column to the rule table to connect the event rules to their source. Due to this change, you can no longer create event rules via the UI anymore, since Icinga Notifications Web does not this feature yet. However, you can still populate the database manually with event rules, just like you did before. The relevant commit for this change is 26e7cd7.
• Finally, with 05d48e5 the listener rejects events with outdated event rules with HTTP 412 Precondition Failed status code, instead of the previously used 424 Failed Dependency status code. I think, this describes the situation better!

After applying the extra tags and Icinga 2 removal changes, Icinga Notifications Web will be broken, so you've to work around this with a temporary fix, by removing and commenting out some code parts:

Diff

diff --git a/application/controllers/EventRuleController.php b/application/controllers/EventRuleController.php
index c96c88b..3e0d7c2 100644
--- a/application/controllers/EventRuleController.php
+++ b/application/controllers/EventRuleController.php
@@ -10,7 +10,6 @@ use Icinga\Module\Notifications\Common\Links;
 use Icinga\Module\Notifications\Forms\EventRuleForm;
 use Icinga\Module\Notifications\Forms\SaveEventRuleForm;
 use Icinga\Module\Notifications\Model\Rule;
-use Icinga\Module\Notifications\Web\Control\SearchBar\ExtraTagSuggestions;
 use Icinga\Module\Notifications\Widget\EventRuleConfig;
 use Icinga\Web\Notification;
 use Icinga\Web\Session;
@@ -177,9 +176,6 @@ class EventRuleController extends CompatController
      */
     public function completeAction(): void
     {
-        $suggestions = new ExtraTagSuggestions();
-        $suggestions->forRequest($this->getServerRequest());
-        $this->getDocument()->add($suggestions);
     }


diff --git a/application/forms/SourceForm.php b/application/forms/SourceForm.php
index dcfbc63..b5f4523 100644
--- a/application/forms/SourceForm.php
+++ b/application/forms/SourceForm.php
@@ -71,7 +71,7 @@ class SourceForm extends CompatForm
             || ($chosenType === null && $configuredType === null)
             || ($chosenType === null && $configuredType === Source::ICINGA_TYPE_NAME);

-        if ($showIcingaApiConfig) {
+        if (false && $showIcingaApiConfig) {
             // TODO: Shouldn't be necessary: https://github.com/Icinga/ipl-html/issues/131
             $this->clearPopulatedValue('type');

@@ -203,11 +203,11 @@ class SourceForm extends CompatForm
             );

             // Preserves (some) entered data even if the user switches between types
-            $this->addElement('hidden', 'icinga2_base_url');
-            $this->addElement('hidden', 'icinga2_auth_user');
-            $this->addElement('hidden', 'icinga2_insecure_tls');
-            $this->addElement('hidden', 'icinga2_common_name');
-            $this->addElement('hidden', 'icinga2_ca_pem');
+            //$this->addElement('hidden', 'icinga2_base_url');
+            //$this->addElement('hidden', 'icinga2_auth_user');
+            //$this->addElement('hidden', 'icinga2_insecure_tls');
+            //$this->addElement('hidden', 'icinga2_common_name');
+            //$this->addElement('hidden', 'icinga2_ca_pem');
         }

         $this->addElement(
@@ -376,12 +376,12 @@ class SourceForm extends CompatForm
         return [
             'name'                  => $source->name,
             'type'                  => $source->type,
-            'icinga2_base_url'      => $source->icinga2_base_url,
-            'icinga2_auth_user'     => $source->icinga2_auth_user,
-            'icinga2_auth_pass'     => $source->icinga2_auth_pass,
-            'icinga2_ca_pem'        => $source->icinga2_ca_pem,
-            'icinga2_common_name'   => $source->icinga2_common_name,
-            'icinga2_insecure_tls'  => $source->icinga2_insecure_tls
+            //'icinga2_base_url'      => $source->icinga2_base_url,
+            //'icinga2_auth_user'     => $source->icinga2_auth_user,
+            //'icinga2_auth_pass'     => $source->icinga2_auth_pass,
+            //'icinga2_ca_pem'        => $source->icinga2_ca_pem,
+            //'icinga2_common_name'   => $source->icinga2_common_name,
+            //'icinga2_insecure_tls'  => $source->icinga2_insecure_tls
         ];
     }
 }
diff --git a/library/Notifications/Model/ExtraTag.php b/library/Notifications/Model/ExtraTag.php
deleted file mode 100644
index 07a27f8..0000000
--- a/library/Notifications/Model/ExtraTag.php
+++ /dev/null
@@ -1,28 +0,0 @@
-<?php
-
-/* Icinga Notifications Web | (c) 2023 Icinga GmbH | GPLv2 */
-
-namespace Icinga\Module\Notifications\Model;
-
-use Icinga\Module\Notifications\Model\Behavior\ObjectTags;
-use ipl\Orm\Behaviors;
-use ipl\Sql\Connection;
-
-class ExtraTag extends ObjectExtraTag
-{
-    /**
-     * @internal Don't use. This model acts only as relation target and is not supposed to be directly used as query
-     *           target. Use {@see ObjectExtraTag} instead.
-     */
-    public static function on(Connection $_)
-    {
-        throw new \LogicException('Documentation says: DO NOT USE. Can\'t you read?');
-    }
-
-    public function createBehaviors(Behaviors $behaviors): void
-    {
-        parent::createBehaviors($behaviors);
-
-        $behaviors->add(new ObjectTags());
-    }
-}
diff --git a/library/Notifications/Model/ObjectExtraTag.php b/library/Notifications/Model/ObjectExtraTag.php
deleted file mode 100644
index df7beb7..0000000
--- a/library/Notifications/Model/ObjectExtraTag.php
+++ /dev/null
@@ -1,52 +0,0 @@
-<?php
-
-/* Icinga Notifications Web | (c) 2023 Icinga GmbH | GPLv2 */
-
-namespace Icinga\Module\Notifications\Model;
-
-use ipl\Orm\Behavior\Binary;
-use ipl\Orm\Behaviors;
-use ipl\Orm\Model;
-use ipl\Orm\Query;
-use ipl\Orm\Relations;
-
-/**
- * ObjectExtraTag database model
- *
- * @property string $object_id
- * @property string $tag
- * @property string $value
- *
- * @property Query|Objects $object
- */
-class ObjectExtraTag extends Model
-{
-    public function getTableName(): string
-    {
-        return 'object_extra_tag';
-    }
-
-    public function getKeyName(): array
-    {
-        return ['object_id', 'tag'];
-    }
-
-    public function getColumns(): array
-    {
-        return [
-            'object_id',
-            'tag',
-            'value'
-        ];
-    }
-
-    public function createBehaviors(Behaviors $behaviors): void
-    {
-        $behaviors->add(new Binary(['object_id']));
-    }
-
-    public function createRelations(Relations $relations): void
-    {
-        $relations->belongsTo('object', Objects::class);
-    }
-}
diff --git a/library/Notifications/Model/Objects.php b/library/Notifications/Model/Objects.php
index c621056..3f37a27 100644
--- a/library/Notifications/Model/Objects.php
+++ b/library/Notifications/Model/Objects.php
@@ -25,8 +25,6 @@ use ipl\Orm\Relations;
  * @property Query|Event $event
  * @property Query|Incident $incident
  * @property Query|Tag $tag
- * @property Query|ObjectExtraTag $object_extra_tag
- * @property Query|ExtraTag $extra_tag
  * @property Query|Source $source
  * @property array<string, string> $id_tags
  */
@@ -81,10 +79,6 @@ class Objects extends Model

         $relations->hasMany('object_id_tag', ObjectIdTag::class);
         $relations->hasMany('tag', Tag::class);
-        $relations->hasMany('object_extra_tag', ObjectExtraTag::class)
-            ->setJoinType('LEFT');
-        $relations->hasMany('extra_tag', ExtraTag::class)
-            ->setJoinType('LEFT');

         $relations->belongsTo('source', Source::class)->setJoinType('LEFT');
     }
diff --git a/library/Notifications/Model/Source.php b/library/Notifications/Model/Source.php
index 973bbb7..e8059fc 100644
--- a/library/Notifications/Model/Source.php
+++ b/library/Notifications/Model/Source.php
@@ -19,12 +19,6 @@ use ipl\Web\Widget\Icon;
  * @property string $type Type identifier
  * @property string $name The user-defined name
  * @property ?string $listener_password_hash
- * @property ?string $icinga2_base_url
- * @property ?string $icinga2_auth_user
- * @property ?string $icinga2_auth_pass
- * @property ?string $icinga2_ca_pem
- * @property ?string $icinga2_common_name
- * @property string $icinga2_insecure_tls
  * @property DateTime $changed_at
  * @property bool $deleted
  *
@@ -51,12 +45,6 @@ class Source extends Model
             'type',
             'name',
             'listener_password_hash',
-            'icinga2_base_url',
-            'icinga2_auth_user',
-            'icinga2_auth_pass',
-            'icinga2_ca_pem',
-            'icinga2_common_name',
-            'icinga2_insecure_tls',
             'changed_at',
             'deleted'
         ];
diff --git a/library/Notifications/Web/Control/SearchBar/ExtraTagSuggestions.php b/library/Notifications/Web/Control/SearchBar/ExtraTagSuggestions.php
deleted file mode 100644
index 88f52c2..0000000
--- a/library/Notifications/Web/Control/SearchBar/ExtraTagSuggestions.php
+++ /dev/null
@@ -1,41 +0,0 @@
-<?php
-
-/* Icinga Notifications Web | (c) 2023 Icinga GmbH | GPLv2 */
-
-namespace Icinga\Module\Notifications\Web\Control\SearchBar;
-
-use Icinga\Module\Notifications\Common\Database;
-use Icinga\Module\Notifications\Model\ObjectExtraTag;
-use Icinga\Module\Notifications\Util\ObjectSuggestionsCursor;
-use ipl\Web\Control\SearchBar\Suggestions;
-use ipl\Stdlib\Filter;
-use Traversable;
-
-class ExtraTagSuggestions extends Suggestions
-{
-    protected function fetchValueSuggestions($column, $searchTerm, Filter\Chain $searchFilter)
-    {
-        yield;
-    }
-
-    protected function createQuickSearchFilter($searchTerm)
-    {
-        return Filter::any();
-    }
-
-    protected function fetchColumnSuggestions($searchTerm)
-    {
-        $searchColumns = (new ObjectSuggestionsCursor(
-            Database::get(),
-            (new ObjectExtraTag())::on(Database::get())
-                ->columns(['tag'])
-                ->assembleSelect()
-                ->distinct()
-        ));
-
-        // Object Extra Tags
-        foreach ($searchColumns as $column) {
-            yield $column->tag => $column->tag;
-        }
-    }
-}
diff --git a/library/Notifications/Web/Control/SearchBar/ObjectSuggestions.php b/library/Notifications/Web/Control/SearchBar/ObjectSuggestions.php
index 2a4d674..2bd1b82 100644
--- a/library/Notifications/Web/Control/SearchBar/ObjectSuggestions.php
+++ b/library/Notifications/Web/Control/SearchBar/ObjectSuggestions.php
@@ -6,7 +6,6 @@ namespace Icinga\Module\Notifications\Web\Control\SearchBar;

 use Icinga\Module\Notifications\Common\Auth;
 use Icinga\Module\Notifications\Common\Database;
-use Icinga\Module\Notifications\Model\ObjectExtraTag;
 use Icinga\Module\Notifications\Model\ObjectIdTag;
 use Icinga\Module\Notifications\Util\ObjectSuggestionsCursor;
 use ipl\Html\HtmlElement;
@@ -184,9 +183,9 @@ class ObjectSuggestions extends Suggestions
         }

         // Custom variables only after the columns are exhausted and there's actually a chance the user sees them
-        foreach ([new ObjectIdTag(), new ObjectExtraTag()] as $model) {
+        foreach ([new ObjectIdTag()] as $model) {
             $titleAdded = false;
-            /** @var ObjectIdTag|ObjectExtraTag $tag */
+            /** @var ObjectIdTag $tag */
             foreach ($this->queryTags($model, $searchTerm) as $tag) {
                 $isIdTag = $tag instanceof ObjectIdTag;

diff --git a/library/Notifications/Widget/Detail/IncidentDetail.php b/library/Notifications/Widget/Detail/IncidentDetail.php
index 46e6bed..a9c370b 100644
--- a/library/Notifications/Widget/Detail/IncidentDetail.php
+++ b/library/Notifications/Widget/Detail/IncidentDetail.php
@@ -105,19 +105,10 @@ class IncidentDetail extends BaseHtmlElement

     protected function createObjectTag(): array
     {
-        $tags = [];
-        foreach ($this->incident->object->object_extra_tag as $extraTag) {
-            $tags[] = Table::row([$extraTag->tag, $extraTag->value]);
-        }
-
-        if (! $tags) {
-            return $tags;
-        }
-
         return [
             new HtmlElement('h2', null, new Text(t('Object Tags'))),
             (new Table())
-                ->addHtml(...$tags)
+                ->addHtml()
                 ->addAttributes(['class' => 'object-tags-table'])
         ];
     }

Also, all the individual commits are self-contained, and includes the relevant commit message with some details about
the changes, so you can always refer to them to see the justifications for the changes.

[julianbrost]

This should also mention how rules are passed now (would also be helpful to get a glance over it while reviewing).

[oxzi]

This one is still open. I would like to update the documentation if everything else looks fine.

[oxzi]

The following diff against Icinga Notification Web "works" if your source has the ID 1. It is based on @yhabteab's diff from above. It should apply on top of today's main branch, being at 6d193d1058ad153330a37b70fea4992b9683822a.

CLICK HERE FOR A FREE DOWNLOAD

diff --git a/application/controllers/EventRuleController.php b/application/controllers/EventRuleController.php
index 9d07d6b..6ffdf3c 100644
--- a/application/controllers/EventRuleController.php
+++ b/application/controllers/EventRuleController.php
@@ -12,7 +12,6 @@ use Icinga\Module\Notifications\Forms\EventRuleConfigElements\NotificationConfig
 use Icinga\Module\Notifications\Forms\EventRuleConfigForm;
 use Icinga\Module\Notifications\Forms\EventRuleForm;
 use Icinga\Module\Notifications\Model\Rule;
-use Icinga\Module\Notifications\Web\Control\SearchBar\ExtraTagSuggestions;
 use Icinga\Web\Notification;
 use Icinga\Web\Session;
 use ipl\Html\Form;
@@ -157,9 +156,6 @@ class EventRuleController extends CompatController
      */
     public function completeAction(): void
     {
-        $suggestions = new ExtraTagSuggestions();
-        $suggestions->forRequest($this->getServerRequest());
-        $this->getDocument()->add($suggestions);
     }
 
     /**
diff --git a/application/forms/EventRuleConfigForm.php b/application/forms/EventRuleConfigForm.php
index 34da1ac..2652535 100644
--- a/application/forms/EventRuleConfigForm.php
+++ b/application/forms/EventRuleConfigForm.php
@@ -291,6 +291,7 @@ class EventRuleConfigForm extends Form
             $db->insert('rule', [
                 'name'          => $this->getValue('name'),
                 'timeperiod_id' => null,
+                'source_id'     => 1, // TODO :^)
                 'object_filter' => $this->getValue('object_filter'),
                 'changed_at'    => (int) (new DateTime())->format("Uv"),
                 'deleted'       => 'n'
diff --git a/application/forms/SourceForm.php b/application/forms/SourceForm.php
index dcfbc63..a57f626 100644
--- a/application/forms/SourceForm.php
+++ b/application/forms/SourceForm.php
@@ -71,144 +71,48 @@ class SourceForm extends CompatForm
             || ($chosenType === null && $configuredType === null)
             || ($chosenType === null && $configuredType === Source::ICINGA_TYPE_NAME);
 
-        if ($showIcingaApiConfig) {
-            // TODO: Shouldn't be necessary: https://github.com/Icinga/ipl-html/issues/131
-            $this->clearPopulatedValue('type');
+        $this->getElement('icinga_or_other')->setValue('other');
 
-            $this->addElement(
-                'hidden',
-                'type',
-                [
-                    'required'  => true,
-                    'disabled'  => true,
-                    'value'     => Source::ICINGA_TYPE_NAME
-                ]
-            );
-            $this->addElement(
-                'text',
-                'icinga2_base_url',
-                [
-                    'required'  => true,
-                    'label'     => $this->translate('API URL')
-                ]
-            );
-            $this->addElement(
-                'text',
-                'icinga2_auth_user',
-                [
-                    'required'      => true,
-                    'label'         => $this->translate('API Username'),
-                    'autocomplete'  => 'off'
-                ]
-            );
-            $this->addElement(
-                'password',
-                'icinga2_auth_pass',
-                [
-                    'required'      => true,
-                    'label'         => $this->translate('API Password'),
-                    'autocomplete'  => 'new-password'
-                ]
-            );
-            $this->addElement(
-                'checkbox',
-                'icinga2_insecure_tls',
-                [
-                    'class'             => 'autosubmit',
-                    'label'             => $this->translate('Verify API Certificate'),
-                    'checkedValue'      => 'n',
-                    'uncheckedValue'    => 'y',
-                    'value'             => true
-                ]
-            );
-
-            /** @var CheckboxElement $insecureBox */
-            $insecureBox = $this->getElement('icinga2_insecure_tls');
-            if ($insecureBox->isChecked()) {
-                $this->addElement(
-                    'text',
-                    'icinga2_common_name',
-                    [
-                        'label'         => $this->translate('Common Name'),
-                        'description'   => $this->translate(
-                            'The CN of the API certificate. Only required if it differs from the FQDN of the API URL'
-                        )
-                    ]
-                );
-
-                $this->addElement(
-                    'textarea',
-                    'icinga2_ca_pem',
-                    [
-                        'cols'          => 64,
-                        'rows'          => 28,
-                        'required'      => true,
-                        'validators'    => [new X509CertValidator()],
-                        'label'         => $this->translate('CA Certificate'),
-                        'description'   => $this->translate('The certificate of the Icinga CA')
-                    ]
-                );
-            }
-        } else {
-            $this->getElement('icinga_or_other')->setValue('other');
-
-            $this->addElement(
-                'text',
-                'type',
-                [
-                    'required'      => true,
-                    'label'         => $this->translate('Type Name'),
-                    'validators'    => [new CallbackValidator(function (string $value, CallbackValidator $validator) {
-                        if ($value === Source::ICINGA_TYPE_NAME) {
-                            $validator->addMessage($this->translate('This name is reserved and cannot be used'));
-
-                            return false;
-                        }
-
-                        return true;
-                    })]
-                ]
-            );
-            $this->addElement(
-                'password',
-                'listener_password',
-                [
-                    'ignore'        => true,
-                    'required'      => $this->sourceId === null,
-                    'label'         => $this->sourceId !== null
-                        ? $this->translate('New Password')
-                        : $this->translate('Password'),
-                    'autocomplete'  => 'new-password',
-                    'validators'    => [['name' => 'StringLength', 'options' => ['min' => 16]]]
-                ]
-            );
-            $this->addElement(
-                'password',
-                'listener_password_dupe',
-                [
-                    'ignore'        => true,
-                    'required'      => $this->sourceId === null,
-                    'label'         => $this->translate('Repeat Password'),
-                    'autocomplete'  => 'new-password',
-                    'validators'    => [new CallbackValidator(function (string $value, CallbackValidator $validator) {
-                        if ($value !== $this->getValue('listener_password')) {
-                            $validator->addMessage($this->translate('Passwords do not match'));
-
-                            return false;
-                        }
-
-                        return true;
-                    })]
-                ]
-            );
-
-            // Preserves (some) entered data even if the user switches between types
-            $this->addElement('hidden', 'icinga2_base_url');
-            $this->addElement('hidden', 'icinga2_auth_user');
-            $this->addElement('hidden', 'icinga2_insecure_tls');
-            $this->addElement('hidden', 'icinga2_common_name');
-            $this->addElement('hidden', 'icinga2_ca_pem');
-        }
+        $this->addElement(
+            'text',
+            'type',
+            [
+                'required'      => true,
+                'label'         => $this->translate('Type Name'),
+            ]
+        );
+        $this->addElement(
+            'password',
+            'listener_password',
+            [
+                'ignore'        => true,
+                'required'      => $this->sourceId === null,
+                'label'         => $this->sourceId !== null
+                    ? $this->translate('New Password')
+                    : $this->translate('Password'),
+                'autocomplete'  => 'new-password',
+                'validators'    => [['name' => 'StringLength', 'options' => ['min' => 16]]]
+            ]
+        );
+        $this->addElement(
+            'password',
+            'listener_password_dupe',
+            [
+                'ignore'        => true,
+                'required'      => $this->sourceId === null,
+                'label'         => $this->translate('Repeat Password'),
+                'autocomplete'  => 'new-password',
+                'validators'    => [new CallbackValidator(function (string $value, CallbackValidator $validator) {
+                    if ($value !== $this->getValue('listener_password')) {
+                        $validator->addMessage($this->translate('Passwords do not match'));
+
+                        return false;
+                    }
+
+                    return true;
+                })]
+            ]
+        );
 
         $this->addElement(
             'submit',
@@ -376,12 +280,6 @@ class SourceForm extends CompatForm
         return [
             'name'                  => $source->name,
             'type'                  => $source->type,
-            'icinga2_base_url'      => $source->icinga2_base_url,
-            'icinga2_auth_user'     => $source->icinga2_auth_user,
-            'icinga2_auth_pass'     => $source->icinga2_auth_pass,
-            'icinga2_ca_pem'        => $source->icinga2_ca_pem,
-            'icinga2_common_name'   => $source->icinga2_common_name,
-            'icinga2_insecure_tls'  => $source->icinga2_insecure_tls
         ];
     }
 }
diff --git a/library/Notifications/Model/ExtraTag.php b/library/Notifications/Model/ExtraTag.php
deleted file mode 100644
index 07a27f8..0000000
--- a/library/Notifications/Model/ExtraTag.php
+++ /dev/null
@@ -1,28 +0,0 @@
-<?php
-
-/* Icinga Notifications Web | (c) 2023 Icinga GmbH | GPLv2 */
-
-namespace Icinga\Module\Notifications\Model;
-
-use Icinga\Module\Notifications\Model\Behavior\ObjectTags;
-use ipl\Orm\Behaviors;
-use ipl\Sql\Connection;
-
-class ExtraTag extends ObjectExtraTag
-{
-    /**
-     * @internal Don't use. This model acts only as relation target and is not supposed to be directly used as query
-     *           target. Use {@see ObjectExtraTag} instead.
-     */
-    public static function on(Connection $_)
-    {
-        throw new \LogicException('Documentation says: DO NOT USE. Can\'t you read?');
-    }
-
-    public function createBehaviors(Behaviors $behaviors): void
-    {
-        parent::createBehaviors($behaviors);
-
-        $behaviors->add(new ObjectTags());
-    }
-}
diff --git a/library/Notifications/Model/ObjectExtraTag.php b/library/Notifications/Model/ObjectExtraTag.php
deleted file mode 100644
index df7beb7..0000000
--- a/library/Notifications/Model/ObjectExtraTag.php
+++ /dev/null
@@ -1,52 +0,0 @@
-<?php
-
-/* Icinga Notifications Web | (c) 2023 Icinga GmbH | GPLv2 */
-
-namespace Icinga\Module\Notifications\Model;
-
-use ipl\Orm\Behavior\Binary;
-use ipl\Orm\Behaviors;
-use ipl\Orm\Model;
-use ipl\Orm\Query;
-use ipl\Orm\Relations;
-
-/**
- * ObjectExtraTag database model
- *
- * @property string $object_id
- * @property string $tag
- * @property string $value
- *
- * @property Query|Objects $object
- */
-class ObjectExtraTag extends Model
-{
-    public function getTableName(): string
-    {
-        return 'object_extra_tag';
-    }
-
-    public function getKeyName(): array
-    {
-        return ['object_id', 'tag'];
-    }
-
-    public function getColumns(): array
-    {
-        return [
-            'object_id',
-            'tag',
-            'value'
-        ];
-    }
-
-    public function createBehaviors(Behaviors $behaviors): void
-    {
-        $behaviors->add(new Binary(['object_id']));
-    }
-
-    public function createRelations(Relations $relations): void
-    {
-        $relations->belongsTo('object', Objects::class);
-    }
-}
diff --git a/library/Notifications/Model/Objects.php b/library/Notifications/Model/Objects.php
index c621056..3f37a27 100644
--- a/library/Notifications/Model/Objects.php
+++ b/library/Notifications/Model/Objects.php
@@ -25,8 +25,6 @@ use ipl\Orm\Relations;
  * @property Query|Event $event
  * @property Query|Incident $incident
  * @property Query|Tag $tag
- * @property Query|ObjectExtraTag $object_extra_tag
- * @property Query|ExtraTag $extra_tag
  * @property Query|Source $source
  * @property array<string, string> $id_tags
  */
@@ -81,10 +79,6 @@ class Objects extends Model
 
         $relations->hasMany('object_id_tag', ObjectIdTag::class);
         $relations->hasMany('tag', Tag::class);
-        $relations->hasMany('object_extra_tag', ObjectExtraTag::class)
-            ->setJoinType('LEFT');
-        $relations->hasMany('extra_tag', ExtraTag::class)
-            ->setJoinType('LEFT');
 
         $relations->belongsTo('source', Source::class)->setJoinType('LEFT');
     }
diff --git a/library/Notifications/Model/Rule.php b/library/Notifications/Model/Rule.php
index 8e69a01..00cd5f8 100644
--- a/library/Notifications/Model/Rule.php
+++ b/library/Notifications/Model/Rule.php
@@ -16,6 +16,7 @@ use ipl\Orm\Relations;
  * @property int $id
  * @property string $name
  * @property ?int $timeperiod_id
+ * @property ?int $source_id
  * @property ?string $object_filter
  * @property DateTime $changed_at
  * @property bool $deleted
@@ -41,6 +42,7 @@ class Rule extends Model
         return [
             'name',
             'timeperiod_id',
+            'source_id',
             'object_filter',
             'changed_at',
             'deleted'
@@ -51,6 +53,7 @@ class Rule extends Model
     {
         return [
             'name'          => t('Name'),
+            'source_id'     => t('Source ID'),
             'timeperiod_id' => t('Timeperiod ID'),
             'object_filter' => t('Object Filter'),
             'changed_at'    => t('Changed At')
diff --git a/library/Notifications/Model/Source.php b/library/Notifications/Model/Source.php
index 973bbb7..8558ace 100644
--- a/library/Notifications/Model/Source.php
+++ b/library/Notifications/Model/Source.php
@@ -19,12 +19,6 @@ use ipl\Web\Widget\Icon;
  * @property string $type Type identifier
  * @property string $name The user-defined name
  * @property ?string $listener_password_hash
- * @property ?string $icinga2_base_url
- * @property ?string $icinga2_auth_user
- * @property ?string $icinga2_auth_pass
- * @property ?string $icinga2_ca_pem
- * @property ?string $icinga2_common_name
- * @property string $icinga2_insecure_tls
  * @property DateTime $changed_at
  * @property bool $deleted
  *
@@ -33,7 +27,7 @@ use ipl\Web\Widget\Icon;
 class Source extends Model
 {
     /** @var string The type name used by Icinga sources */
-    public const ICINGA_TYPE_NAME = 'icinga2';
+    public const ICINGA_TYPE_NAME = 'icingadb';
 
     public function getTableName(): string
     {
@@ -51,12 +45,6 @@ class Source extends Model
             'type',
             'name',
             'listener_password_hash',
-            'icinga2_base_url',
-            'icinga2_auth_user',
-            'icinga2_auth_pass',
-            'icinga2_ca_pem',
-            'icinga2_common_name',
-            'icinga2_insecure_tls',
             'changed_at',
             'deleted'
         ];
diff --git a/library/Notifications/Web/Control/SearchBar/ExtraTagSuggestions.php b/library/Notifications/Web/Control/SearchBar/ExtraTagSuggestions.php
deleted file mode 100644
index 88f52c2..0000000
--- a/library/Notifications/Web/Control/SearchBar/ExtraTagSuggestions.php
+++ /dev/null
@@ -1,41 +0,0 @@
-<?php
-
-/* Icinga Notifications Web | (c) 2023 Icinga GmbH | GPLv2 */
-
-namespace Icinga\Module\Notifications\Web\Control\SearchBar;
-
-use Icinga\Module\Notifications\Common\Database;
-use Icinga\Module\Notifications\Model\ObjectExtraTag;
-use Icinga\Module\Notifications\Util\ObjectSuggestionsCursor;
-use ipl\Web\Control\SearchBar\Suggestions;
-use ipl\Stdlib\Filter;
-use Traversable;
-
-class ExtraTagSuggestions extends Suggestions
-{
-    protected function fetchValueSuggestions($column, $searchTerm, Filter\Chain $searchFilter)
-    {
-        yield;
-    }
-
-    protected function createQuickSearchFilter($searchTerm)
-    {
-        return Filter::any();
-    }
-
-    protected function fetchColumnSuggestions($searchTerm)
-    {
-        $searchColumns = (new ObjectSuggestionsCursor(
-            Database::get(),
-            (new ObjectExtraTag())::on(Database::get())
-                ->columns(['tag'])
-                ->assembleSelect()
-                ->distinct()
-        ));
-
-        // Object Extra Tags
-        foreach ($searchColumns as $column) {
-            yield $column->tag => $column->tag;
-        }
-    }
-}
diff --git a/library/Notifications/Web/Control/SearchBar/ObjectSuggestions.php b/library/Notifications/Web/Control/SearchBar/ObjectSuggestions.php
index 2a4d674..2bd1b82 100644
--- a/library/Notifications/Web/Control/SearchBar/ObjectSuggestions.php
+++ b/library/Notifications/Web/Control/SearchBar/ObjectSuggestions.php
@@ -6,7 +6,6 @@ namespace Icinga\Module\Notifications\Web\Control\SearchBar;
 
 use Icinga\Module\Notifications\Common\Auth;
 use Icinga\Module\Notifications\Common\Database;
-use Icinga\Module\Notifications\Model\ObjectExtraTag;
 use Icinga\Module\Notifications\Model\ObjectIdTag;
 use Icinga\Module\Notifications\Util\ObjectSuggestionsCursor;
 use ipl\Html\HtmlElement;
@@ -184,9 +183,9 @@ class ObjectSuggestions extends Suggestions
         }
 
         // Custom variables only after the columns are exhausted and there's actually a chance the user sees them
-        foreach ([new ObjectIdTag(), new ObjectExtraTag()] as $model) {
+        foreach ([new ObjectIdTag()] as $model) {
             $titleAdded = false;
-            /** @var ObjectIdTag|ObjectExtraTag $tag */
+            /** @var ObjectIdTag $tag */
             foreach ($this->queryTags($model, $searchTerm) as $tag) {
                 $isIdTag = $tag instanceof ObjectIdTag;
 
diff --git a/library/Notifications/Widget/Detail/IncidentDetail.php b/library/Notifications/Widget/Detail/IncidentDetail.php
index 46e6bed..a9c370b 100644
--- a/library/Notifications/Widget/Detail/IncidentDetail.php
+++ b/library/Notifications/Widget/Detail/IncidentDetail.php
@@ -105,19 +105,10 @@ class IncidentDetail extends BaseHtmlElement
 
     protected function createObjectTag(): array
     {
-        $tags = [];
-        foreach ($this->incident->object->object_extra_tag as $extraTag) {
-            $tags[] = Table::row([$extraTag->tag, $extraTag->value]);
-        }
-
-        if (! $tags) {
-            return $tags;
-        }
-
         return [
             new HtmlElement('h2', null, new Text(t('Object Tags'))),
             (new Table())
-                ->addHtml(...$tags)
+                ->addHtml()
                 ->addAttributes(['class' => 'object-tags-table'])
         ];
     }

[sukhwinder33445]

This column was previously nullable, and I already have an entry with a null value. This means that I cannot perform this upgrade. The following is missing:

UPDATE source SET listener_password_hash = '$2_$%' WHERE listener_password_hash IS NULL;

[oxzi]

Honestly, this unveils a bigger issue in the migration. With dropping the icinga2 source and changing the ck_source_bcrypt_listener_password_hash check constraint, setups previously using icinga2 sources cannot be migrated without loss.

Since this very support is being removed in this PR and manual intervention is required, I am going to delete such rows.

[julianbrost]

Since this very support is being removed in this PR and manual intervention is required, I am going to delete such rows.

Doesn't sound like the best idea, as you'd basically have to wipe everything then given the foreign key constraints.

Two other options:

1. Make it a manual migration step that you have to set password hashes for all before applying the schema upgrade. Adding that constraint should then be the first step in the migration so that if the manual step wasn't done, it fails immediately.
2. Allow it to stay NULL for the time being, which means the source remains in a non-working state until a password was set for it. That would probably be less annoying migration path.

[julianbrost]

Option 2 sounds most reasonable to me. Requires minimal changes to the code (if at all) and has the most graceful migration path.

[julianbrost]

I'm not sure about RuleSet. That type seems to be only used for embedding it here. What's the idea behind that? I'd suggest keeping it more like the others, i.e. keep the Rules field as-is and add the second additional lookup structure directly to this struct.

[oxzi]

I cannot answer about the initial motivation, but changing this to a field results in some very long access paths.

[julianbrost]

What exactly do you mean by "very long access paths"?

I'm mainly wondering what the point for RuleSet is, i.e. I didn't find a pointer to a RuleSet anywhere or something that copies the whole thing. The alternative would be to just have Rules and RulesBySource defined directly in here.

[yhabteab]

The point is that I didn't want to bloat the ConfigSet struct with even mor fields that are actually all rule related, thus I've used RuleSet to group all the fields concerning event rules. -

[julianbrost]

Unsupported type? Can that even happen? Isn't the effect of moving that check to the UnmarshalJSON() method that the whole JSON parsing operation would fail (so that you wouldn't even end up here in that case)?

[oxzi]

Unsure if this can happen. Which UnmarshalJSON method are you referring to? In my understanding, this method is called for submitted events in Listener.ProcessEvent.

[julianbrost]

I was referring to that one: https://github.com/Icinga/icinga-go-library/blob/6600889adc38ffe2088948411c852cb387b5762f/notifications/event/type.go#L42-L61

So far, it was parsed into a string, allowing any string value, but now it's parsed into a type that does its own validation. So if it was indeed a unknown type, that should already trigger an error before getting here and this should only happen for "type":null (or it missing entirely in the JSON).

[yhabteab]

that should already trigger an error before getting here and this should only happen for "type":null (or it missing entirely in the JSON).

Since the ParseType() function doesn't have such constraint t != TypeUnknown, the JSON string can also contain such strings "type":"unknown" and would pass the validation on the Type. However, I don't get what your actual point is, are you suggesting to remove this check entierely or something else? Why would you want this basic validation to depend on something else when you can easily bypass the validation on the Type struct?

[julianbrost]

However, I don't get what your actual point is, are you suggesting to remove this check entierely or something else?

Whether the error message actually describes the situation that's checked for. Maybe something like "invalid event: missing type" would better describe it. Using e.Type in the error message is also questionable given that it would be normalized to TypeUnknown already. (I'm also not completely sure what %q will do in this scenario, i.e. whether it will print "unknown" or - worse - try to interpret the underlying integer representation as a character.

Since the ParseType() function doesn't have such constraint t != TypeUnknown, the JSON string can also contain such strings "type":"unknown"

So {"type":null} and {"type":"unknown"} will parse the same? Not sure if that even sounds like desirable behavior, "unknown" probably shouldn't be considered a valid value if TypeUnknown is supposed to represent JSON null.

[yhabteab]

I'm also not completely sure what %q will do in this scenario, i.e. whether it will print "unknown" or - worse - try to interpret the underlying integer representation as a character.

Is never going to happen! The Type struct implements the String method.

So {"type":null} and {"type":"unknown"} will parse the same? Not sure if that even sounds like desirable behavior, "unknown" probably shouldn't be considered a valid value if TypeUnknown is supposed to represent JSON null.

Then both the ParseType and ParseSeverity functions need to be fixed (add the t/s != TypeUnknown/SeverityNone constraint.

[julianbrost]

Either way: is there any reason why this shouldn't just do return errors.New("invalid event: missing type")?

This clearly looks like a copy & paste remainder from the old implementation, having e.Type as part of the log message inside if e.Type == baseEv.TypeUnknown (that condition was different before) just makes little sense.

[yhabteab]

Either way: is there any reason why this shouldn't just do return errors.New("invalid event: missing type")?

I already gave you the reason for that. Why do you want to blindly trust on the validation from the Type itself and assume that it'd be a missing type, when one can just construct even some invalid ones like this Type(100)? The condition is just wrong for checking for TypeUnkknown, instead we should maybe introduce a IsValid() method on the Type struct that reports whether the type represented by t is within the valid range of the type.

Again, I'm just answering your questions since I wrote that code, but you can change it however you want if this isn't a good enough reason for you.

[julianbrost]

Either way: is there any reason why this shouldn't just do return errors.New("invalid event: missing type")?

I already gave you the reason for that.

Where?

Why do you want to blindly trust on the validation from the Type itself and assume that it'd be a missing type,

Blindly trusting sounds like I'd want to remove the check, which I do not. All I'm trying to say so far is that the current message is bullshit. Is anyone trying to argue with that?

when one can just construct even some invalid ones like this Type(100)? The condition is just wrong for checking for TypeUnkknown, instead we should maybe introduce a IsValid() method on the Type struct that reports whether the type represented by t is within the valid range of the type.

Now that's another issue which boils down to the question what this code here is supposed to do: is it supposed to detect any possible inconsistency, including these possibly resulting from manually constructing the object? Or just verify the additional constraints that aren't check during JSON parsing? The latter is how this function is currently used (by the way, all the errors returned use the JSON field names, not the struct field names). For this, just ensuring that e.Type != baseEv.TypeUnknown is enough, because that checks that a field was set, which encoding/json can't check by itself. If the field was set, I find it reasonable to assume that parsing it ensured that it's set to a valid type.

Again, I'm just answering your questions since I wrote that code, but you can change it however you want if this isn't a good enough reason for you.

Okay, but who will address this now? For me, it'd be enough if @oxzi just fixed the error message as I suggested (after agreeing that this change makes sense).

If you (@yhabteab) think a validation function for the Type type, this can also be added after this PR, in particular, this saves this PR from requiring another cross-repo change.

[julianbrost]

The existing imports shouldn't be regrouped.

[julianbrost]

"👀" but unchanged? 👀

[oxzi]

Sorry, I have planned addressing this when everything else is done as otherwise this just might creep in again.

[julianbrost]

Since this very support is being removed in this PR and manual intervention is required, I am going to delete such rows.

Doesn't sound like the best idea, as you'd basically have to wipe everything then given the foreign key constraints.

Two other options:

1. Make it a manual migration step that you have to set password hashes for all before applying the schema upgrade. Adding that constraint should then be the first step in the migration so that if the manual step wasn't done, it fails immediately.
2. Allow it to stay NULL for the time being, which means the source remains in a non-working state until a password was set for it. That would probably be less annoying migration path.