MacroProcessor::ResolveMacro(): treat quasi-CV-object IcingaApplication as real CV-object

[Al2Klimov]

As MacroProcessor checked just for CustomVarObject base class, but IcingaApplication provided the vars attribute by itself, it had to also resolve CV macros by itself. That logic diverged from MacroProcessor so that macros inside IcingaApplication CVs weren't resolved. Until now.

ref/IP/46031

Test protocol

Config

object CheckerComponent "cc" { }

object IcingaApplication "app" {
	vars.myecho_1 = "42$$"
}

object CheckCommand "myecho" {
	command = [ "python3", "-c", "import sys; print(sys.argv); exit(128)" ]
	arguments = {
		myecho_1 = "$myecho_1$"
	}
}

object Host NodeName {
	check_command = "myecho"
}

Logs

[2023-05-31 11:57:08 +0200] warning/PluginCheckTask: Check command for object 'Alexanders-MacBook-Pro-2.local' (PID: 87725, arguments: 'python3' '-c' 'import sys; print(sys.argv); exit(128)' 'myecho_1' '42$') terminated with exit code 128, output: ['-c', 'myecho_1', '42$']

Conclusion

$$ now becomes $ as expected. (Before: $$ -> $$)

[julianbrost]

checked just for CustomVarObject base class, but IcingaApplication provided the vars attribute by itself

Could IcingaApplication just inherit from CustomVarObject? Otherwise, there could be more undesirable inconsistencies.

[Al2Klimov]

If you ask this way: no. IcingaApplication couldn’t "just" inherit from CustomVarObject. A such inheritance could bring far more undesirable surprises. You had to insert CustomVarObject -> ConfigObject into the existing IcingaApplication -> Application -> ConfigObject tree. Application -> CustomVarObject? Would require the latter being in libbase. Forget it. Virtual inheritance? This would open a construction area on its own. CustomVarObject as mixin like ConfigType? You already dislike such existing stuff. Inconsistencies or not – nobody complained. At best this is a quality issue.

[julianbrost]

I didn't consider that indirection in the inheritance of ConfigObject, so yes, that makes things ugly here.

So in summary, the ability to provide recursive macros is limited to within MacroProcessor, i.e. it does not expose *recursive_macro = true; to MacroResolver (which is implemented by IcingaApplication) so some of the code is moved.

[julianbrost]

The only other places that seem to take special care of CustomVarObject seem to be Icinga DB (which doesn't write the IcingaApplication object the database) and the IDO (it probably doesn't either, but I didn't even bother to check), so the change should be fine in that respect.

Apart from that, it changes which hard-coded macros from IcingaApplication can be overridden from that vars attribute. But given that the vars lookup was somewhere in the middle, that felt quite arbitrary anyways and I guess nobody relies on that (I'm not sure if many users are even aware of IcingaApplication.vars).

[Al2Klimov]

The only other places that seem to take special care of CustomVarObject seem to be Icinga DB (which doesn't write the IcingaApplication object the database) and the IDO (it probably doesn't either, but I didn't even bother to check), so the change should be fine in that respect.

Which change exactly? Also, does the latter imply that the remaining inconsistencies, if any, are at best pure code quality issues?

[julianbrost]

Which change exactly?

Doing it this way instead of trying to make IcingaApplication a CustomVarObject.

Also, does the latter imply that the remaining inconsistencies, if any, are at best pure code quality issues?

That means that I haven't looked at the IDO code, I just found that it references CustomVarObject. This PR is unlikely to introduce any new problems there, and if there was some inconsistency in the IDO nobody ever noticed, it doesn't matter (but I guess the IDO doesn't dump custom vars of IcingaApplication either).