icinga2
icinga2 copied to clipboard
Icinga
Disallow nested config objects
➜ icinga2 git:(bugfix/nested-objects-8633) cat 8633ok.conf
include <itl>
object Host "a" {
check_command = "dummy"
}
apply Service "b" {
check_command = "dummy"
assign where true
}
➜ icinga2 git:(bugfix/nested-objects-8633) prefix/sbin/icinga2 daemon -C -c 8633ok.conf
[2021-03-04 12:23:56 +0100] information/cli: Icinga application loader (version: v2.12.0-507-gb9aee216d)
[2021-03-04 12:23:56 +0100] information/cli: Loading configuration file(s).
[2021-03-04 12:23:56 +0100] information/ConfigItem: Committing config item(s).
[2021-03-04 12:23:56 +0100] information/ConfigItem: Instantiated 8 CheckCommands.
[2021-03-04 12:23:56 +0100] information/ConfigItem: Instantiated 1 Host.
[2021-03-04 12:23:56 +0100] information/ConfigItem: Instantiated 1 IcingaApplication.
[2021-03-04 12:23:56 +0100] information/ConfigItem: Instantiated 1 Service.
[2021-03-04 12:23:56 +0100] information/ScriptGlobal: Dumping variables to file '/Users/aklimov/NET/WS/icinga2/prefix/var/cache/icinga2/icinga2.vars'
[2021-03-04 12:23:56 +0100] information/cli: Finished validating the configuration file(s).
➜ icinga2 git:(bugfix/nested-objects-8633) cat 8633oo.conf
include <itl>
object Host "a" {
check_command = "dummy"
object Host "b" {
check_command = "dummy"
}
}
➜ icinga2 git:(bugfix/nested-objects-8633) prefix/sbin/icinga2 daemon -C -c 8633oo.conf
[2021-03-04 12:24:19 +0100] information/cli: Icinga application loader (version: v2.12.0-507-gb9aee216d)
[2021-03-04 12:24:19 +0100] information/cli: Loading configuration file(s).
[2021-03-04 12:24:19 +0100] information/ConfigItem: Committing config item(s).
[2021-03-04 12:24:19 +0100] critical/config: Error: Nested objects are not allowed
Location: in 8633oo.conf: 6:2-6:16
8633oo.conf(4): check_command = "dummy"
8633oo.conf(5):
8633oo.conf(6): object Host "b" {
^^^^^^^^^^^^^^^
8633oo.conf(7): check_command = "dummy"
8633oo.conf(8): }
[2021-03-04 12:24:19 +0100] critical/config: 1 error
[2021-03-04 12:24:19 +0100] critical/cli: Config validation failed. Re-run with 'icinga2 daemon -C' after fixing the config.
➜ icinga2 git:(bugfix/nested-objects-8633) cat 8633aa.conf
include <itl>
object Host "a" {
check_command = "dummy"
}
apply Service "b" {
check_command = "dummy"
assign where true
apply Service "c" {
check_command = "dummy"
assign where true
}
}
➜ icinga2 git:(bugfix/nested-objects-8633) prefix/sbin/icinga2 daemon -C -c 8633aa.conf
[2021-03-04 12:24:39 +0100] information/cli: Icinga application loader (version: v2.12.0-507-gb9aee216d)
[2021-03-04 12:24:39 +0100] information/cli: Loading configuration file(s).
[2021-03-04 12:24:39 +0100] information/ConfigItem: Committing config item(s).
[2021-03-04 12:24:39 +0100] critical/config: Error: Nested objects are not allowed
Location: in 8633aa.conf: 11:2-11:18
8633aa.conf(9): assign where true
8633aa.conf(10):
8633aa.conf(11): apply Service "c" {
^^^^^^^^^^^^^^^^^
8633aa.conf(12): check_command = "dummy"
8633aa.conf(13): assign where true
[2021-03-04 12:24:39 +0100] critical/config: 1 error
[2021-03-04 12:24:39 +0100] critical/cli: Config validation failed. Re-run with 'icinga2 daemon -C' after fixing the config.
➜ icinga2 git:(bugfix/nested-objects-8633) cat 8633oa.conf
include <itl>
object Host "a" {
check_command = "dummy"
apply Service "b" {
check_command = "dummy"
assign where true
}
}
➜ icinga2 git:(bugfix/nested-objects-8633) prefix/sbin/icinga2 daemon -C -c 8633oa.conf
[2021-03-04 12:25:03 +0100] information/cli: Icinga application loader (version: v2.12.0-507-gb9aee216d)
[2021-03-04 12:25:03 +0100] information/cli: Loading configuration file(s).
[2021-03-04 12:25:03 +0100] information/ConfigItem: Committing config item(s).
[2021-03-04 12:25:03 +0100] critical/config: Error: Nested objects are not allowed
Location: in 8633oa.conf: 6:2-6:18
8633oa.conf(4): check_command = "dummy"
8633oa.conf(5):
8633oa.conf(6): apply Service "b" {
^^^^^^^^^^^^^^^^^
8633oa.conf(7): check_command = "dummy"
8633oa.conf(8): assign where true
[2021-03-04 12:25:03 +0100] critical/config: 1 error
[2021-03-04 12:25:03 +0100] critical/cli: Config validation failed. Re-run with 'icinga2 daemon -C' after fixing the config.
➜ icinga2 git:(bugfix/nested-objects-8633) cat 8633ao.conf
include <itl>
object Host "a" {
check_command = "dummy"
}
apply Service "b" {
check_command = "dummy"
assign where true
object Host "c" {
check_command = "dummy"
}
}
➜ icinga2 git:(bugfix/nested-objects-8633) prefix/sbin/icinga2 daemon -C -c 8633ao.conf
[2021-03-04 12:25:42 +0100] information/cli: Icinga application loader (version: v2.12.0-507-gb9aee216d)
[2021-03-04 12:25:42 +0100] information/cli: Loading configuration file(s).
[2021-03-04 12:25:42 +0100] information/ConfigItem: Committing config item(s).
[2021-03-04 12:25:42 +0100] critical/config: Error: Nested objects are not allowed
Location: in 8633ao.conf: 11:2-11:16
8633ao.conf(9): assign where true
8633ao.conf(10):
8633ao.conf(11): object Host "c" {
^^^^^^^^^^^^^^^
8633ao.conf(12): check_command = "dummy"
8633ao.conf(13): }
[2021-03-04 12:25:42 +0100] critical/config: 1 error
[2021-03-04 12:25:42 +0100] critical/cli: Config validation failed. Re-run with 'icinga2 daemon -C' after fixing the config.
➜ icinga2 git:(bugfix/nested-objects-8633)
First impression when looking at the diff: the condition is detected quite late.
I think this situation is comparable to how assign is handled in the parser:
https://github.com/Icinga/icinga2/blob/f679ea4d9f5c051b14e724cd6cf30f5945795c46/lib/config/config_parser.yy#L502-L503
Similarly, it should be possible there to add some inside apply/object flag and checking/updating it when entering/exiting such a scope.
That's for a reason:
- Create a function which declares an object
- Call it inside an object declaration
Interesting point. Is that something that is supposed/expected to work? Potential alternative would be to restrict apply/object to the global scope (including in conditionals/loops). At least that's what I expected how it's supposed to be used.
Is that something that is supposed/expected to work?
Why not?
- Create a function which declares an object
- Call it
Well, it's all about the language design and specification. https://icinga.com/docs/icinga-2/latest/doc/17-language-reference/ makes no statements about where object/apply is allowed and the examples show no context.
After all, we could also ask why not allow something like this:
object Host "foo" {
object Service "bar" {
object Notification "baz" {
# note: this is an example of non-working config, please don't do this because you've seen it here.
}
}
}
So the question is: is anyone aware that you can register new (global) objects in functions? Is anyone using it? Do we want to support it?
I'm currently looking at this PR due to #9434: unfortunately I'm still waiting for user feedback but given the stack trace, it looks like some unexpected nesting of object and apply to me (icinga::ApplyExpression::DoEvaluate called from within icinga::ConfigItem::Commit) and the user says that there was no problem with older versions (my current hypothesis is that #9362 changed the timing enough to that it makes race-conditions in code that's not expected to be called concurrently more apparent in this situation).
This kind of explains my motivation behind the idea of restricting the use of apply/object even more: such uncommon and little known constructs are likely to be overlooked. Every developer will remember that you can register an object on a global scope, but doing it within a function? Not sure. I haven't gone through the git history but I wouldn't be surprised if nesting objects like in my example above worked at some point, then parallelism was added to the commit phase, introducing the race condition that now leads to the crash in the linked issue because nobody thought about this when adding the parallelism.
Which brings back the question: is there a good reason to actually define objects in a function so that it's worth keeping and supporting this? Or are we better of with disallowing it and having fewer surprises in real world configs?
I see just no reason to forbid it.
- Create a function which declares an object
- Call it
Yes, this would be an alternative:
apply/objectto the global scope (including in conditionals/loops)
But I know at least one dev (just among us) like this:
You're giving me bad ideas. Your PR still allows the following (you can't use object, that's prevented somewhere else, search for "Objects may not be created outside of an activation context"):
object Host "gh-8655" {
check_command = "dummy"
vars.dummy_text = {{
apply Service "gh-8655-inner" {
assign where true
}
return "what could possibly go wrong?"
}}
}
Do this in many checkables, then the lambdas are evaluated concurrently, still triggering the race condition in #9434:
include <itl>
object CheckerComponent "checker" {}
for (i in range(10000)) {
object Host i {
check_command = "dummy"
check_interval = 1
vars.dummy_text = {{
apply Service "inner" {
assign where false
}
return "what could possibly go wrong?"
}}
}
}
You were right,
include <itl>
object CheckerComponent "checker" { }
object Host "h" {
check_interval = 1s
check_command = "dummy"
vars.dummy_text = {{
object Host get_time() {
check_command = "dummy"
}
return get_time()
}}
}
gave me
[2022-07-13 18:31:22 +0200] critical/checker: Exception occurred while checking 'h': Error: Error while evaluating expression: Objects may not be created outside of an activation context.
Location: in 8655a.conf: 9:3-9:24
8655a.conf(7): check_command = "dummy"
8655a.conf(8): vars.dummy_text = {{
8655a.conf(9): object Host get_time() {
^^^^^^^^^^^^^^^^^^^^^^
8655a.conf(10): check_command = "dummy"
8655a.conf(11): }
Context:
(0) Resolving macros for string '$dummy_text$'
(1) Executing check for object 'h'
whereas
include <itl>
object CheckerComponent "checker" { }
object Host "h" {
check_interval = 1s
check_command = "dummy"
vars.dummy_text = {{
apply Service get_time() {
check_command = "dummy"
assign where true
}
return get_time()
}}
}
apply Service get_time() {
check_command = "dummy"
assign where true
}
just worked
[2022-07-13 18:34:27 +0200] debug/CheckerComponent: Scheduling info for checkable 'h' (2022-07-13 18:34:27 +0200): Object 'h', Next Check: 2022-07-13 18:34:27 +0200(1.65773e+09).
[2022-07-13 18:34:27 +0200] debug/CheckerComponent: Executing check for 'h'
[2022-07-13 18:34:27 +0200] debug/Checkable: Update checkable 'h' with check interval '1' from last check time at 2022-07-13 18:34:26 +0200 (1.65773e+09) to next check time at 2022-07-13 18:34:28 +0200 (1.65773e+09).
[2022-07-13 18:34:27 +0200] debug/Checkable: Update checkable 'h' with check interval '1' from last check time at 2022-07-13 18:34:27 +0200 (1.65773e+09) to next check time at 2022-07-13 18:34:28 +0200 (1.65773e+09).
[2022-07-13 18:34:27 +0200] debug/Checkable: Flapping: Checkable h was: 0 is: 0 threshold low: 25 threshold high: 30% current: 5.3%.
[2022-07-13 18:34:27 +0200] debug/CheckerComponent: Check finished for object 'h'
until now:
[2022-07-13 18:45:22 +0200] critical/checker: Exception occurred while checking 'h': Error: Error while evaluating expression: Objects may not be created outside of an activation context.
Location: in 8655b.conf: 9:3-9:26
8655b.conf(7): check_command = "dummy"
8655b.conf(8): vars.dummy_text = {{
8655b.conf(9): apply Service get_time() {
^^^^^^^^^^^^^^^^^^^^^^^^
8655b.conf(10): check_command = "dummy"
8655b.conf(11): assign where true
Context:
(0) Resolving macros for string '$dummy_text$'
(1) Executing check for object 'h'
I still haven't figured out why ActivationContext uses a stack but your ConfigItem::m_CommitInProgress is supposed to be fine just being a boolean. But I haven't observed stack sizes greater than one so far, so ActivationContext might just support recursive use when it's not actually needed?
I've added logging to ActivationScope and it never reported a stack size greater than one. So looks like it's called without an outer context there.
Anyways, m_CommitInProgress would only require support for recursive use if it was possible to trigger a commit from within the expression of a config item, basically what this PR is trying to prevent in the first place. So should be fine, I still don't fully understand ActivationContext though.