Disallow nested config objects #8655

Al2Klimov · 2021-03-04T11:18:56Z

Al2Klimov · 2021-03-04T11:26:31Z

➜  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)

Al2Klimov · 2021-08-04T12:21:15Z

@cla-bot check

julianbrost · 2022-07-12T15:12:26Z

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:

icinga2/lib/config/config_parser.yy

Lines 502 to 503 in f679ea4

    
           if ((context->m_Apply.empty() || !context->m_Apply.top()) && (context->m_ObjectAssign.empty() || !context->m_ObjectAssign.top())) 
        
           	BOOST_THROW_EXCEPTION(ScriptError("'assign' keyword not valid in this context.", @$));

Similarly, it should be possible there to add some inside apply/object flag and checking/updating it when entering/exiting such a scope.

Al2Klimov · 2022-07-13T09:38:31Z

That's for a reason:

Create a function which declares an object
Call it inside an object declaration

julianbrost · 2022-07-13T09:48:30Z

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.

Al2Klimov · 2022-07-13T10:59:52Z

Is that something that is supposed/expected to work?

Why not?

Create a function which declares an object
Call it

julianbrost · 2022-07-13T14:59:59Z

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?

Al2Klimov · 2022-07-13T15:08:11Z

I see just no reason to forbid it.

Create a function which declares an object

Call it

Yes, this would be an alternative:

apply/object to the global scope (including in conditionals/loops)

But I know at least one dev (just among us) like this:

julianbrost · 2022-07-13T15:55:14Z

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?"
		}}
	}
}

Al2Klimov · 2022-07-13T17:01:05Z

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'

julianbrost · 2022-07-21T14:12:53Z

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?

Al2Klimov · 2022-07-21T14:27:15Z

CommitNewItems() does CreateChildObjects() on a second stack level.

julianbrost · 2022-07-21T14:42:35Z

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.

julianbrost · 2022-07-22T11:50:07Z

lib/config/applyrule.cpp

@@ -75,6 +76,8 @@ void ApplyRule::AddRule(const String& sourceType, const String& targetType, cons
 	const Expression::Ptr& expression, const Expression::Ptr& filter, const String& package, const String& fkvar,
 	const String& fvvar, const Expression::Ptr& fterm, bool ignoreOnError, const DebugInfo& di, const Dictionary::Ptr& scope)
 {
+	ActivationContext::AssertOnContext();
+
 	m_Rules[sourceType].push_back(ApplyRule(targetType, name, expression, filter, package, fkvar, fvvar, fterm, ignoreOnError, di, scope));
 }


Why do this here and not also within ApplyExpression::DoEvaluate()?

DoEvaluate evaluates other expressions before (indirectly) calling AddRule, so when doing something like apply Service f() ..., this would now still evaluate f() before telling you that you can't even use apply there.

Check out this creative use of things crashing with a segfault:

object Host "h" { enable_active_checks = false check_command = "dummy" var h = this vars.dummy_text = () use (h) => { apply Service h.vars.dummy_text() { assign where false } } }

Why do this here and not also within ApplyExpression::DoEvaluate()?

DoEvaluate evaluates other expressions ...

As I said in the second commit's message:

Prevent apply rule creation outside of an activation context

just like object creation.

Rationale: make the behaviours as similar as possible.

Check out this creative use of things crashing with a segfault:

C'mon. That trick is old.

So you're saying with object instead of apply in my example, it would still crash? I think it would be nicer to - in both cases - detect that this is an invalid use of apply or object earlier, rather than after starting to evaluate the expression.

Yes.

C'mon! You're calling a function recursively w/o an exit condition.

refs #8633

just like object creation.

Al2Klimov added bug Something isn't working area/configuration DSL, parser, compiler, error handling labels Mar 4, 2021

Al2Klimov self-assigned this Mar 4, 2021

icinga-probot bot added this to the 2.13.0 milestone Mar 4, 2021

icinga-probot bot added the core/crash Shouldn't happen, requires attention label Mar 4, 2021

Al2Klimov removed their assignment Mar 4, 2021

Al2Klimov marked this pull request as ready for review March 4, 2021 11:27

Al2Klimov modified the milestones: 2.13.0, 2.14.0 Jun 2, 2021

cla-bot bot added the cla/signed label Aug 4, 2021

julianbrost self-requested a review July 12, 2022 10:38

julianbrost requested changes Jul 22, 2022

View reviewed changes

lippserd removed this from the 2.14.0 milestone Mar 30, 2023

Al2Klimov force-pushed the bugfix/nested-objects-8633 branch from 51930dc to 8ddaa73 Compare August 15, 2023 16:10

Al2Klimov requested a review from julianbrost August 15, 2023 16:11

Al2Klimov added this to the 2.15.0 milestone Jan 21, 2025

Al2Klimov force-pushed the bugfix/nested-objects-8633 branch from 8ddaa73 to 911d18b Compare January 21, 2025 10:39

Al2Klimov added 2 commits April 22, 2025 14:05

Disallow nested config objects

924068c

refs #8633

Prevent apply rule creation outside of an activation context

1c49db3

just like object creation.

Al2Klimov force-pushed the bugfix/nested-objects-8633 branch from 911d18b to 1c49db3 Compare April 22, 2025 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow nested config objects #8655

Disallow nested config objects #8655

Al2Klimov commented Mar 4, 2021 •

edited

Loading

Al2Klimov commented Mar 4, 2021

Al2Klimov commented Aug 4, 2021

julianbrost commented Jul 12, 2022

Al2Klimov commented Jul 13, 2022

julianbrost commented Jul 13, 2022

Al2Klimov commented Jul 13, 2022

julianbrost commented Jul 13, 2022

Al2Klimov commented Jul 13, 2022

julianbrost commented Jul 13, 2022

Al2Klimov commented Jul 13, 2022

julianbrost commented Jul 21, 2022

Al2Klimov commented Jul 21, 2022

julianbrost commented Jul 21, 2022

julianbrost Jul 22, 2022

julianbrost Jul 22, 2022

Al2Klimov Jul 25, 2022

julianbrost Jul 25, 2022

Al2Klimov Jul 25, 2022

Disallow nested config objects #8655

Are you sure you want to change the base?

Disallow nested config objects #8655

Conversation

Al2Klimov commented Mar 4, 2021 • edited Loading

Al2Klimov commented Mar 4, 2021

Al2Klimov commented Aug 4, 2021

julianbrost commented Jul 12, 2022

Al2Klimov commented Jul 13, 2022

julianbrost commented Jul 13, 2022

Al2Klimov commented Jul 13, 2022

julianbrost commented Jul 13, 2022

Al2Klimov commented Jul 13, 2022

julianbrost commented Jul 13, 2022

Al2Klimov commented Jul 13, 2022

julianbrost commented Jul 21, 2022

Al2Klimov commented Jul 21, 2022

julianbrost commented Jul 21, 2022

julianbrost Jul 22, 2022

Choose a reason for hiding this comment

julianbrost Jul 22, 2022

Choose a reason for hiding this comment

Al2Klimov Jul 25, 2022

Choose a reason for hiding this comment

julianbrost Jul 25, 2022

Choose a reason for hiding this comment

Al2Klimov Jul 25, 2022

Choose a reason for hiding this comment

Al2Klimov commented Mar 4, 2021 •

edited

Loading