Skip to content

No longer allow overriding the frozen attribute of containers#10499

Merged
yhabteab merged 1 commit intomasterfrom
remove-override-frozen
Jul 8, 2025
Merged

No longer allow overriding the frozen attribute of containers#10499
yhabteab merged 1 commit intomasterfrom
remove-override-frozen

Conversation

@julianbrost
Copy link
Copy Markdown
Member

The Array, Dictionary, and Namespace types provide a Freeze() method that makes them read-only. So far, there was the possibility to call some methods with overrideFrozen=true which would then bypass the corresponding check and allow modification of the data structures nonetheless.

With #9627 (24b57f0), this possibility was already removed from the Namespace type. However, for interface compatibility, it kept the parameter and just ignores it, throwing an exception on any modification on a frozen instance.

The only place using overrideFrozen was processing of the -D/--define command line flag that allows setting additional variables in the DSL. At the time it is evaluated, there are no user-created data structures yet that could be frozen, so the only frozen objects that could be encountered are Namespaces (Icinga doesn't freeze other types by itself) and for these, overrideFrozen already has no effect.

Hence, there is no harm in removing overrideFrozen altogether. This simplifies the code and also means that frozen objects are now indeed read-only without exceptions, allowing further optimizations regarding locking in the future. In particular, #10414 currently assumes that frozen Arrays/Dictionaries can be iterated without holding (which is only true as long as there is no operation with overrideFrozen).

Tests

Because of the following, I don't expect any observable difference in behavior from this change:

The only place using overrideFrozen was processing of the -D/--define command line flag that allows setting additional variables in the DSL. At the time it is evaluated, there are no user-created data structures yet that could be frozen, so the only frozen objects that could be encountered are Namespaces (Icinga doesn't freeze other types by itself) and for these, overrideFrozen already has no effect.

I've tested that Icinga 2 still starts with this changes and works in general. Additionally, I've also successfully tested a config deployment from Director (as that's a case that makes use of --define).

The Array, Dictionary, and Namespace types provide a Freeze() method that makes
them read-only. So far, there was the possibility to call some methods with
`overrideFrozen=true` which would then bypass the corresponding check and allow
modification of the data structures nonetheless.

With 24b57f0, this possibility was already
removed from the Namespace type. However, for interface compatibility, it kept
the parameter and just ignores it, throwing an exception on any modification on
a frozen instance.

The only place using `overrideFrozen` was processing of the `-D`/`--define`
command line flag that allows setting additional variables in the DSL. At the
time it is evaluated, there are no user-created data structures yet that could
be frozen, so the only frozen objects that could be encountered are Namespaces
(Icinga doesn't freeze other types by itself) and for these, `overrideFrozen`
already has no effect.

Hence, there is no harm in removing `overrideFrozen` altogether. This
simplifies the code and also means that frozen objects are now indeed read-only
without exceptions, allowing further optimizations regarding locking in the
future.
@julianbrost julianbrost added the core/quality Improve code, libraries, algorithms, inline docs label Jul 8, 2025
@cla-bot cla-bot bot added the cla/signed label Jul 8, 2025
@julianbrost julianbrost requested a review from yhabteab July 8, 2025 13:25
Copy link
Copy Markdown
Member

@yhabteab yhabteab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ grep --exclude-dir=build --exclude-dir=prefix --exclude-dir=doc -niR 'overrideFrozen' .
<nothing>

Config package updates (director):

...
[2025-07-08 15:37:18 +0200] information/ApiListener: New client connection from [::1]:52531 (no client certificate)
[2025-07-08 15:37:18 +0200] information/ConfigPackageUtility: Updating configuration file: /Users/yhabteab/Workspace/icinga2/prefix/var/lib/icinga2/api/packages/example-cmdb/dc8d5503-71a4-4498-9c8b-637e99bd06c1/conf.d/test.conf
[2025-07-08 15:37:18 +0200] information/HttpServerConnection: Request POST /v1/config/stages/example-cmdb (from [::1]:52531, user: root, agent: curl/8.7.1, status: OK) took total 1ms.
[2025-07-08 15:37:18 +0200] information/HttpServerConnection: HTTP client disconnected (from [::1]:52531)
[2025-07-08 15:37:20 +0200] information/Application: Got reload command: Starting new instance.
...

@yhabteab yhabteab added this to the 2.16.0 milestone Jul 8, 2025
@yhabteab yhabteab enabled auto-merge July 8, 2025 13:45
@yhabteab yhabteab merged commit 241cf08 into master Jul 8, 2025
25 checks passed
@yhabteab yhabteab deleted the remove-override-frozen branch July 8, 2025 15:22
@yhabteab yhabteab mentioned this pull request Apr 1, 2026
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla/signed core/quality Improve code, libraries, algorithms, inline docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants