Improve idempotence of the rule module - #1143
Draft
robin-checkmk wants to merge 5 commits into
Draft
Conversation
With state=present and a rule_id that does not exist, the module reported an unchanged success instead of the documented failure. run_module read rule_id from the wrong parameter level (always None, leaving the failure branch dead), and rule_id_found() treated the empty dict returned for HTTP 404 as a found rule.
The parens-to-brackets translation in _raw_value_eval also rewrote string contents, so a real change like 'echo (1)' to 'echo [1]' was reported as unchanged and never applied. Evaluate value_raw as-is and recursively convert tuples to lists on both sides before comparing: same tuple/list tolerance, but string contents stay untouched.
Checkmk 2.3 and newer migrate non-empty host_labels/service_labels conditions to host_label_groups/service_label_groups server-side, so a playbook using the old syntax never matched the stored rule and created a new rule on every run. Apply the same translation (verified against cmk.utils.labels.single_label_group_from_labels) to the desired state before comparing and sending.
Creating a rule already places it at the bottom of the target folder, but _moving_needed() treated the default position "any" (and any non-root folder) as requiring a move, causing an extra API call, an audit log entry and a misleading "Rule moved" message on every create.
Replace the memory_percentage_used examples, whose value format is rejected by Checkmk 2.5, with checkgroup_parameters:filesystem, which works on all supported versions. Document that value_raw must use the target version's canonical format, that secret-bearing rules cannot be compared reliably due to API-side masking, and that relative positions are order-dependent claims.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR was LLM-assisted. We will test it thoroughly and possibly adapt it in the process.
Human review is definitely the decisive factor here, we will only merge it, if it is well understood and valid.
Pull request type
Check the type of change your PR introduces:
What is the current behavior?
Issue Number: #186
What is the new behavior?
Implemented fixes
rule_idc7955117993c3e0aecho (1)→echo [1]detected + applied, then idempotent; tuple/list still toleratedcb5bd3adis_not)616ae9d7position: topstill moves, idempotent after967135e2checkgroup_parameters:filesystem(valid on 2.3/2.4/2.5); notes on canonical format, masking, positionsF1. Replace the
()→[]string hack with structural normalization_raw_value_evalrewrote parens to brackets inside string literals too:changing a command line from
echo (1)toecho [1]was reportedchanged=Falseand never applied (verified). The hack's original purpose isgone — 2.3/2.4/2.5 all reject list-for-tuple input (400) and return tuples
byte-stably. Implemented fix (
993c3e0a):literal_evalboth sides, then_tuples_to_lists()recursively converts tuples to lists (containers only,strings untouched) before
==. Keeps tolerance for any historically lenientpatch level, no version gate, no false equality.
F2. Translate label conditions before comparing
Non-empty
host_labels/service_labelsare accepted but returned migrated to*_label_groups— identically on 2.3/2.4/2.5. A playbook using the old syntaxnever matched → duplicate rule per run (verified). Implemented fix
(
cb5bd3ad):_migrate_label_conditions()translates the desired state on≥ 2.3 before comparing and sending —
{key, operator: is|is_not, value}→{operator: "and", label_group: [{operator: "and"|"not", label: "key:value"}]},all labels of one condition into a single group. The mapping was verified
against
cmk.utils.labels.single_label_group_from_labels(identical in the2.3, 2.4 and 2.5 trees) and live against the API.
F3. Nonexistent
rule_id+state: presentsilently "succeeds"Reported
changed=False, "Rule already exists with the desired parameters."instead of the documented failure. Two compounding defects:
run_modulereadmodule.params.get("rule_id")at the top level (alwaysNone— the paramlives under
rule), making the failure branch dead code; andrule_id_found()checked
is not Nonewhile_get_rule_by_idreturns{}on 404. Implementedfix (
c7955117): read the nested parameter and treat an emptycurrentas notfound.
F4. Pointless move after every create with default location
New rule + default
position: any→_moving_needed()was True ("any" != "bottom") → extra move POST, audit-log entry, and confusing "Rule created,Rule moved" message, although create already appends at the bottom. Implemented
fix (
616ae9d7): for new rules only positions other thanbottom/anytrigger a move (create already places the rule at the bottom of the target
folder, so the previous folder check was superfluous as well).
F5. Docs corrections
Implemented (
967135e2):EXAMPLESused{'levels': (80.0, 90.0)}formemory_percentage_used,which 2.5 rejects with 400 — switched to
checkgroup_parameters:filesystem,whose value format is valid on 2.3/2.4/2.5 (verified).
notes:document thatvalue_rawmust be the target version'scanonical form (GUI "Export rule for API" is the reliable source) and may
need manual migration after a Checkmk upgrade; that secret-bearing rules
cannot be compared reliably (see J1); and that only
position: anyis astable location claim (see J2).
Minor code smell noted, NOT addressed:
_normalize_ruleshallow-copies andpops from nested dicts, mutating
self.desired/self.currentin place —latent trap (F2's in-place translation follows the same existing pattern).
Other information
Judgement calls
J1. Secret masking (the hard one)
Every read masks secrets as
'******'on all versions → withrule_idthemodule edits every run; without it,
_get_rule_idnever matches → duplicaterule every run. Version specifics (all verified, stable within branches):
('store', 'id')(…'stored_password', ('id', ''))(…'stored_password', ('id', '******'))2.4 trap: legacy syntax is accepted-but-migrated (structure changes) → never
equal, even beyond masking. 2.3/2.5 reject the foreign syntax loudly instead.
Proposed fix — needs a decision:
(uuid because of 2.4), compare store-ids literally; apply the same rule in
_get_rule_id. Never send read-back values — PUTting a masked value isaccepted (200) and would store a literal
******as the password.update_secrets: always | on_create(Ansible'supdate_passwordpattern). Open decision: default
on_create= idempotent out of the boxbut a changed secret in the playbook is NOT rolled out unless something else
changes; default
always= today's behavior (changed every run) forsecret-bearing rules unless the user opts in.
version's byte-stable form from the table above. Patch-stable but per-major —
the code fix obsoletes it.
J2. Relative positioning cannot be made idempotent
position: bottom/top/before/afterare claims about global folder order; anylater rule (even from the same play) invalidates them. This is inherent
semantics, not a defect. Decision: document-only, or additionally warn at
runtime when a positional spec is used, or attempt convergence detection
(likely not worth it).
position: any+ folder is the stable pattern.