IcingaDB: handle null (Empty) for value/set_if/separator in command arguments #9371

julianbrost · 2022-05-16T12:04:16Z

Icinga 2 treats null (Empty) as if the corresponding attribute is not specified. However, without this commit, it would serialize the value as "null" (i.e. type string), so that it ends up in the database as this string instead of NULL. This commit adds handling for ValueEmpty so that is serialized as JSON null value and ends up in the database as NULL.

Note: this PR isn't based on #7919 but once both are merged, it affects separator in the same way.

Tests

Note: to see updates to checkcommand_argument, you have to manually TRUNCATE checkcommand_argument; and restart Icinga DB. This is because rows are only updated when the checksum changes. The checksum is obtained from the full dictionary specified in the config, rather than from just what's written to Redis. As the config doesn't change, both versions generate the same checksum even though they serialize the values differently.

Config snippet:

object CheckCommand "command-arguments-null" {
	command = ["true"]
	arguments = {
		#"--separator_default" = {
		#	value = "x"
		#}
		#"--separator_null" = {
		#	value = "x"
		#	separator = null
		#}
		#"--separator_nullstr" = {
		#	value = "x"
		#	separator = "null"
		#}
		#"--separator_empty" = {
		#	value = "x"
		#	separator = ""
		#}
		#"--separator_something" = {
		#	value = "x"
		#	separator = "="
		#}

		"--set_if_default" = {
			value = "x"
		}
		"--set_if_null" = {
			value = "x"
			set_if = null
		}
		"--set_if_nullstr" = {
			value = "x"
			set_if = "null"
		}
		"--set_if_true" = {
			value = "x"
			set_if = true
		}
		"--set_if_false" = {
			value = "x"
			set_if = false
		}

		"--value_default" = {
		}
		"--value_null" = {
			value = null
		}
		"--value_nullstr" = {
			value = "null"
		}
		"--value_empty" = {
			value = ""
		}
		"--value_something" = {
			value = "x"
		}
	}
}

object Host "command-arguments-null" {
	check_command = "command-arguments-null"
}

-set_if_nullstr generates a warning ("warning/PluginUtility: Error evaluating set_if value 'null' used in argument '--set_if_nullstr': Can't convert 'null' to a floating point number.") and is just added here for completeness to see how it behaves apart from that.

Before

For the executed command, not specifying set_if/value (--set_if_default/--value_default) or setting it to null explicitly (--set_if_null/--set_if_null) shows identical behavior:

$ curl -skSu root:icinga -H 'Accept: application/json' 'https://localhost:5665/v1/objects/hosts/command-arguments-null' | jq '.results[0].attrs.last_check_result.command'
[
  "true",
  "--set_if_default",
  "x",
  "--set_if_null",
  "x",
  "--set_if_true",
  "x",
  "--value_default",
  "--value_empty",
  "--value_null",
  "--value_nullstr",
  "null",
  "--value_something",
  "x"
]

However, in Redis both are serialized differently:

# set_if
{
  "argument_key": "--set_if_default",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": "x"
}
{
  "argument_key": "--set_if_null",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "set_if": "null",
  "value": "x"
}

# value
{
  "argument_key": "--value_default",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146"
}
{
  "argument_key": "--value_null",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": "null"
}

# others for completeness
{
  "argument_key": "--set_if_false",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "set_if": "false",
  "value": "x"
}
{
  "argument_key": "--set_if_nullstr",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "set_if": "null",
  "value": "x"
}
{
  "argument_key": "--set_if_true",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "set_if": "true",
  "value": "x"
}
{
  "argument_key": "--value_empty",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": ""
}
{
  "argument_key": "--value_nullstr",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": "null"
}
{
  "argument_key": "--value_something",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": "x"
}

This then also shows in the database:

MariaDB [icingadb]> SELECT argument_key, argument_value, set_if FROM checkcommand_argument WHERE checkcommand_id = 0x86d0f0cbc0eab9e72e825dd621120417de11f2ca ORDER BY argument_key;
+-------------------+----------------+--------+
| argument_key      | argument_value | set_if |
+-------------------+----------------+--------+
| --set_if_default  | x              | NULL   |
| --set_if_false    | x              | false  |
| --set_if_null     | x              | null   |
| --set_if_nullstr  | x              | null   |
| --set_if_true     | x              | true   |
| --value_default   | NULL           | NULL   |
| --value_empty     |                | NULL   |
| --value_null      | null           | NULL   |
| --value_nullstr   | null           | NULL   |
| --value_something | x              | NULL   |
+-------------------+----------------+--------+
10 rows in set (0.001 sec)

After

Executed command stayed the same:

curl -skSu root:icinga -H 'Accept: application/json' 'https://localhost:5665/v1/objects/hosts/command-arguments-null' | jq '.results[0].attrs.last_check_result.command'
[
  "true",
  "--set_if_default",
  "x",
  "--set_if_null",
  "x",
  "--set_if_true",
  "x",
  "--value_default",
  "--value_empty",
  "--value_null",
  "--value_nullstr",
  "null",
  "--value_something",
  "x"
]

Things are written slightly different to Redis now, "null" became null (so value/set_if is missing if it's not set, but set to JSON null if it's specified as Icinga DSL null):

# set_if
{
  "argument_key": "--set_if_default",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": "x"
}
{
  "argument_key": "--set_if_null",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "set_if": null,
  "value": "x"
}

# value
{
  "argument_key": "--value_default",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146"
}
{
  "argument_key": "--value_null",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": null
}

# others for completeness
{
  "argument_key": "--set_if_false",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "set_if": "false",
  "value": "x"
}
{
  "argument_key": "--set_if_nullstr",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "set_if": "null",
  "value": "x"
}
{
  "argument_key": "--set_if_true",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "set_if": "true",
  "value": "x"
}
{
  "argument_key": "--value_empty",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": ""
}
{
  "argument_key": "--value_nullstr",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": "null"
}
{
  "argument_key": "--value_something",
  "checkcommand_id": "86d0f0cbc0eab9e72e825dd621120417de11f2ca",
  "environment_id": "6cc4013ece4ffdbb275a4a763460507d17203146",
  "value": "x"
}

And now, there is also proper NULL in the database:

MariaDB [icingadb]> SELECT argument_key, argument_value, set_if FROM checkcommand_argument WHERE checkcommand_id = 0x86d0f0cbc0eab9e72e825dd621120417de11f2ca ORDER BY argument_key;
+-------------------+----------------+--------+
| argument_key      | argument_value | set_if |
+-------------------+----------------+--------+
| --set_if_default  | x              | NULL   |
| --set_if_false    | x              | false  |
| --set_if_null     | x              | NULL   |
| --set_if_nullstr  | x              | null   |
| --set_if_true     | x              | true   |
| --value_default   | NULL           | NULL   |
| --value_empty     |                | NULL   |
| --value_null      | NULL           | NULL   |
| --value_nullstr   | null           | NULL   |
| --value_something | x              | NULL   |
+-------------------+----------------+--------+
10 rows in set (0.001 sec)

Al2Klimov · 2022-05-19T10:33:40Z

lib/icingadb/icingadb-objects.cpp

@@ -996,6 +996,8 @@ void IcingaDB::InsertObjectDependencies(const ConfigObject::Ptr& object, const S
 					// Stringify if set.
 					if (values->Get(attr, &value)) {
 						switch (value.GetType()) {
+							case ValueEmpty:
+								break;


To me break is redundant, but that's a matter of preference.

Well, I would remove it as well.

lippserd · 2022-05-19T10:36:31Z

lib/icingadb/icingadb-objects.cpp

@@ -996,6 +996,8 @@ void IcingaDB::InsertObjectDependencies(const ConfigObject::Ptr& object, const S
 					// Stringify if set.
 					if (values->Get(attr, &value)) {
 						switch (value.GetType()) {
+							case ValueEmpty:
+								break;


Well, I would remove it as well.

…rguments Icinga 2 treats null (Empty) as if the corresponding attribute is not specified. However, without this commit, it would serialize the value as "null" (i.e. type string), so that it ends up in the database as this string instead of NULL. This commit adds handling for ValueEmpty so that is serialized as JSON null value and ends up in the database as NULL.

…s-null IcingaDB: handle null (Empty) for value/set_if/separator in command arguments

julianbrost added the area/icingadb New backend label May 16, 2022

julianbrost added this to the 2.14.0 milestone May 16, 2022

cla-bot bot added the cla/signed label May 16, 2022

Al2Klimov previously approved these changes May 19, 2022

View reviewed changes

Al2Klimov requested a review from lippserd May 19, 2022 10:34

lippserd requested changes May 19, 2022

View reviewed changes

julianbrost dismissed Al2Klimov’s stale review via f110e26 May 23, 2022 10:59

julianbrost force-pushed the bugfix/icingadb-command-arguments-null branch from a6565bc to f110e26 Compare May 23, 2022 10:59

julianbrost requested a review from lippserd May 23, 2022 10:59

lippserd approved these changes May 23, 2022

View reviewed changes

lippserd enabled auto-merge May 23, 2022 12:21

lippserd merged commit 18c8b4a into master May 23, 2022

icinga-probot bot deleted the bugfix/icingadb-command-arguments-null branch May 23, 2022 14:02

julianbrost added the consider backporting Should be considered for inclusion in a bugfix release label Jun 10, 2022

julianbrost mentioned this pull request Jun 14, 2022

IcingaDB: handle null (Empty) for value/set_if/separator in command arguments #9398

Merged

julianbrost added backported Fix was included in a bugfix release and removed consider backporting Should be considered for inclusion in a bugfix release labels Jun 14, 2022

yhabteab pushed a commit that referenced this pull request Sep 5, 2022

Merge pull request #9371 from Icinga/bugfix/icingadb-command-argument…

63ee762

…s-null IcingaDB: handle null (Empty) for value/set_if/separator in command arguments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IcingaDB: handle null (Empty) for value/set_if/separator in command arguments #9371

IcingaDB: handle null (Empty) for value/set_if/separator in command arguments #9371

julianbrost commented May 16, 2022 •

edited

Loading

Al2Klimov May 19, 2022

lippserd May 19, 2022

lippserd May 19, 2022

IcingaDB: handle null (Empty) for value/set_if/separator in command arguments #9371

IcingaDB: handle null (Empty) for value/set_if/separator in command arguments #9371

Conversation

julianbrost commented May 16, 2022 • edited Loading

Tests

Before

After

Al2Klimov May 19, 2022

Choose a reason for hiding this comment

lippserd May 19, 2022

Choose a reason for hiding this comment

lippserd May 19, 2022

Choose a reason for hiding this comment

julianbrost commented May 16, 2022 •

edited

Loading