Cluster config file sync broken in master

[julianbrost]

24b57f0 from #9627 broke the cluster config file sync: It uses --define System.ZonesStageVarDir=... which can no longer be modified as that namespace was already frozen at that point. Can be easily reproduced:

$ icinga2 daemon -C --define System.ZonesStageVarDir=/var/empty
[2023-01-25 10:58:30 +0000] critical/Application: Error: Namespace is read-only and must not be modified.


Additional information is available in '/var/log/icinga2/crash/report.1674644310.455242'
Aborted

May affect other uses of --define/-D. We have to check if there are other valid uses that were broken by this and how we can fix this. In general, forbidding something like -DTypes.Host=lol (which could be done before but will break basically every config) doesn't sound like a bad idea to me, but even then, it shouldn't write a crash dump.

[julianbrost]

Also affected: https://icinga.com/docs/icinga-2/latest/doc/17-language-reference/#debug-constants-and-variables

[julianbrost]

Idea 1: Use Internal.ZonesStageVarDir instead of System.ZonesStageVarDir and don't freeze the Internal namespace. The attribute isn't properly documented (it shows up in a log message once) and users really shouldn't set this. Also, Internal shouldn't be on any frequent code path, so there should be no performance penalty (alternatively, it could be frozen after evaluating --define).

Idea 2 (optional, in addition): Don't expose Internal to user configuration, i.e. remove it from the globals namespace after running internal DSL snippets and evaluating --define. It's internal, so users shouldn't interact with it, so why expose it in the first place.

[Al2Klimov]

We can’t do this. There are legit post-start users of globals.Internal, see grep -rnFe 'ScriptGlobal::Get("Internal", &Empty)'.

[julianbrost]

But then don't have to access this through globals. They should be just fine if they can access that namespace object using some other reference.

[Al2Klimov]

What’s our benefit?

[julianbrost]

User configuration should not interact with Internal, so why expose it at all?

[Al2Klimov]

This is our biggest "benefit"? So we're basically breaking the Director for... nothing? No, thanks. Once the Director doesn't rely on this: maybe.

[Al2Klimov]

TODO

• Do not use Internal.run_with_activation_context (Icinga DSL) icingaweb2-module-director#2699

[julianbrost]

Once the Director doesn't rely on this: maybe.

It is only used in a function that's labeled:

Note: this is for testing purposes only, NOT production-ready

And the only use of that function is guarded by an $this->hasExperimental('live-creation').

So I wouldn't exactly call this "breaking the Director".

Also, Internal.run_with_activation_context is a function intended for use in the config snippets that are run by Icinga 2 itself in the early initialization process. I don't know what happens or might go wrong if that's used in another way later. If there was a valid use for this, there should be a proper API for this.

[julianbrost]

Since this issue was mainly about another problem and is closed now, I've created #9651. We should continue any discussion over there.