search

Icinga / icinga2

The core of our monitoring platform with a powerful configuration language and REST API.
https://icinga.com/docs/icinga2/latest
GNU General Public License v2.0
2k stars 574 forks source link

Unregister invalid config objects properly #10111

Closed yhabteab closed 1 month ago

yhabteab commented 1 month ago

When trying to create an object via the API, the ConfigItemBuilder will first generate a ConfigItem for this config object and register it in the ConfigItem::m_Items map. This config item generation includes some basic object validations, such as making sure that there is no already registered config item for that object name and type in ConfigItem::m_Items, which contains all the committed items of the Icinga 2 process. https://github.com/Icinga/icinga2/blob/07d253009a976f71f26e61a47d2d31488f3aba66/lib/config/vmops.hpp#L142-L150 https://github.com/Icinga/icinga2/blob/07d253009a976f71f26e61a47d2d31488f3aba66/lib/config/vmops.hpp#L172

ConfigItem::CommitItems() takes actually care of unregistering the newly created objects if CommitNewItems() returns false. However, since the elements from ConfigItem::m_Items are only copied to the newItems vector after they have been successfully committed, it is impossible to unregister them here and leaves a half-committed objects. https://github.com/Icinga/icinga2/blob/07d253009a976f71f26e61a47d2d31488f3aba66/lib/config/configitem.cpp#L631-L635

Fortunately, this has only been broken since v2.14.0 with 400117e and this PR reverts it to its original form.

fixes #10110

yhabteab commented 1 month ago

So the problem is that runtime objects get registered early (and don't vanish on failure)? Maybe we shall not register (and eff. publish) them too early in the first place?

That's not the same issue here! Are you aware of that ConfigObject::Register() ≠ ConfigItem::Register()? The PR you are referring to concerns the former one, while this PR focuses on the latter. While the former takes place after an object has been successfully committed, the latter is triggered long before an object is even validated/started to commit, namely when you call expr->Evaluate(frame) on the compiled configuration.