ClassCompiler: Don't allow to define an int as a group name in groups attr

[yhabteab]

No description provided.

[Al2Klimov]

Post auto-generated code before/after.

[yhabteab]

Before:

if (avalue()) {
		ObjectLock olock(avalue());
		for (const Value& value : avalue()) {
	if (value.IsEmpty() || !utils.ValidateName("HostGroup", value))
		BOOST_THROW_EXCEPTION(ValidationError(dynamic_cast<ConfigObject *>(this), { "groups" }, "Object '" + value + "' of type 'HostGroup' does not exist."));
		}
	}

After:

if (avalue()) {
		ObjectLock olock(avalue());
		for (const Value& value : avalue()) {
	                if (!value.IsEmpty() && !value.IsString())
				BOOST_THROW_EXCEPTION(ValidationError(dynamic_cast<ConfigObject *>(this), { "groups" }, "Object '" + value + "' of type 'HostGroup' has an invalid name."));
	                if (value.IsEmpty() || !utils.ValidateName("HostGroup", value))
		                BOOST_THROW_EXCEPTION(ValidationError(dynamic_cast<ConfigObject *>(this), { "groups" }, "Object '" + value + "' of type 'HostGroup' does not exist."));
		}
	}

[Al2Klimov]

Share an example Icinga2 complaint.

[Al2Klimov]

An Icinga 2 complaint output, not the code that triggers the complaint.

[yhabteab]

object User:

[2021-11-02 16:17:57 +0100] critical/config: Error: Validation failed for object 'icingaadmin' of type 'User'; Attribute 'groups': It is not allowed to specify '12' of type 'Number' as 'UserGroup' name. Expected type of 'UserGroup' name 'String'.
Location: in /prefix/etc/icinga2/conf.d/custom-host.conf: 73:3-73:33
/prefix/etc/icinga2/conf.d/custom-host.conf(71): 
/prefix/etc/icinga2/conf.d/custom-host.conf(72):   display_name = "Icinga 2 Admin"
/prefix/etc/icinga2/conf.d/custom-host.conf(73):   groups = [ 12, "icingaadmins" ]
                                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

object Host:

[2021-11-02 16:20:42 +0100] critical/config: Error: Validation failed for object '49' of type 'Host'; Attribute 'groups': It is not allowed to specify '9' of type 'Number' as 'HostGroup' name. Expected type of 'HostGroup' name 'String'.
Location: in /prefix/etc/icinga2/conf.d/custom-host.conf: 54:9-54:30
/prefix/etc/icinga2/conf.d/custom-host.conf(52):         check_command = "dummy"
/prefix/etc/icinga2/conf.d/custom-host.conf(53):         enable_active_checks = 0
/prefix/etc/icinga2/conf.d/custom-host.conf(54):         groups = [ name % hg ]
                                                         ^^^^^^^^^^^^^^^^^^^^^^

object Service:

[2021-11-02 16:24:06 +0100] critical/config: Error: Validation failed for object '9!4' of type 'Service'; Attribute 'groups': It is not allowed to specify '9' of type 'Number' as 'ServiceGroup' name. Expected type of 'ServiceGroup' name 'String'.
Location: in /prefix/etc/icinga2/conf.d/custom-host.conf: 63:9-63:35
/prefix/etc/icinga2/conf.d/custom-host.conf(61):         check_command = "dummy"
/prefix/etc/icinga2/conf.d/custom-host.conf(62):         enable_active_checks = 0
/prefix/etc/icinga2/conf.d/custom-host.conf(63):         groups = [ host_name % sg ]
                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Al2Klimov]

Doesn’t Icinga 2 already show you the code location along with the wrong value and its type?

[Al2Klimov]

Share an example Icinga2 complaint.

And with the group being null, not a number?

[yhabteab]

And with the group being null, not a number?

Nothing happens. But if you explicitly define null as a value, the following will be raised.

[2021-11-02 17:01:59 +0100] critical/config: Error: Validation failed for object '27!1' of type 'Service'; Attribute 'groups': Object '' of type 'ServiceGroup' does not exist.
Location: in /prefix/etc/icinga2/conf.d/custom-host.conf: 63:9-63:25
/prefix/etc/icinga2/conf.d/custom-host.conf(61):         check_command = "dummy"
/prefix/etc/icinga2/conf.d/custom-host.conf(62):         enable_active_checks = 0
/prefix/etc/icinga2/conf.d/custom-host.conf(63):         groups = [ null ]
                                                         ^^^^^^^^^^^^^^^^^

Note:

This check applies also to notification's users and user_groups attrs.

[Al2Klimov]

Once done, let Julian review. (No need for squashing at all, will be done while merging.)

[julianbrost]

What's the issue this is supposed to solve? If I try the current master, I get the following which looks okay as well:

[2021-11-22 09:53:28 +0100] critical/config: Error: Validation failed for object 'foobar' of type 'Host'; Attribute 'groups': Object '42' of type 'HostGroup' does not exist.
Location: in data/etc/icinga2/conf.d/objects.conf: 70:5-70:17
data/etc/icinga2/conf.d/objects.conf(68): object Host "foobar" {
data/etc/icinga2/conf.d/objects.conf(69):     check_command = "dummy"
data/etc/icinga2/conf.d/objects.conf(70):     groups = [42]
                                              ^^^^^^^^^^^^^
data/etc/icinga2/conf.d/objects.conf(71): }

[yhabteab]

Generally for Icinga2 this doesn't matter, as the integers are implicitly cast to string, but when icinga2 writes to the IDO, this groups attribute is sorted with std::sort, which was introduced with this PR #8367. And there these cannot be implicitly cast, if you have e.g. in the groups attr [2, "1"] and falls simply through.

[julianbrost]

I see, so with this config

object HostGroup 9057 {}
object HostGroup "9057-str" {}

object Host "9057-host" {
	check_command = "dummy"
	groups = [9057, "9057-str"]
}

you eventually run into this error:

[2021-11-22 11:40:19 +0100] critical/IdoMysqlConnection: Exception during database operation: Verify that your database is operational!
[2021-11-22 11:40:19 +0100] debug/IdoMysqlConnection: Exception during database operation: Error: Operator < cannot be applied to values of type 'String' and 'Number'

Have you considered normalizing all array elements to be of type string, so that the config would just work? I think it's a bit inconsistent that you can declare a group as object HostGroup 9057 {} (without the string quotes) and then not being able to use it that way. On the other hand, I hope that no non-test config just uses integer values for their group names, so the exact behavior in that case is not too important I guess (as long as it's not totally broken as it is at the moment).

[Al2Klimov]

normalizing all array elements to be of type string, so that the config would just work

What for? After all:

I hope that no non-test config just uses integer values for their group names, so the exact behavior in that case is not too important

[julianbrost]

I'm just thinking out loud. It's not a request for changes. As I said, it would probably be more consistent but not practically relevant.

[julianbrost]

Just noticed strange behavior with empty strings as object names, but that not related to this PR so I created #9097 for it.

Apart from this, looks fine for me, just needs to be squashed on merge.

[julianbrost]

Won't be backported. Rationale: the current fix is slightly backwards-incompatible and most likely users don't have group names that can be represented by other types than string.