FIX REST API: PagerDuty notification rule integration key format#917
Open
apepojken wants to merge 1 commit into
Open
FIX REST API: PagerDuty notification rule integration key format#917apepojken wants to merge 1 commit into
apepojken wants to merge 1 commit into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
The REST API was writing the PagerDuty integration key to
notification_parameter.mk as ("routing_key", key) — the legacy 2-tuple form.
The notification dispatcher's get_password_from_env_or_context() builds a list
of all PARAMETER_ROUTING_KEY* env vars and only recognises the 3-tuple form
("cmk_postprocessed", "explicit_password", (uuid, key)) by the literal string
"explicit_password" in that list. The 2-tuple form falls through to the
password-store branch which tries to look up "routing_key" as a store ID,
fails, and the plugin exits with "Unable to retrieve password from
passwordstore".
The GUI's FormSpec migrate (_migrate_to_password in
cmk/gui/wato/_notification_parameter/_pagerduty.py) already produces the
3-tuple form for UI-touched rules; the REST API path bypassed it.
Replace APIPagerDutyKeyOption with APICheckmkPassword_FromKey (which already
sits directly above APIPagerDutyKeyOption in the same file and is used by all
other key-bearing notification plugins). PagerDutyPluginModel.routing_key is
re-typed from RoutingKeyType to CheckmkPassword. A normalizer in
PagerDutyPlugin.from_mk_file_format() handles existing on-disk rules in the
legacy form so they keep working through the API and are rewritten in the new
format on next save.
Werk: 20007
Tested:
- ruff format and lint pass on all changed files
- pre-commit lightweight hooks pass (whitespace, license, secrets, namespace)
- Bazel-backed pre-commit hooks (format/lint/mypy/unittest) not run locally;
CI will validate
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
d7ee47b to
747d956
Compare
Author
|
I have read the CLA Document and I hereby sign the CLA or my organization already has a signed CLA. |
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.
General information
This PR fixes a bug in Checkmk's REST API for notification rules. It is not specific to any monitored device or appliance — the affected component is the PagerDuty notification plugin and the REST API endpoint that creates/updates notification rules (
/domain-types/notification_rule/collections/all). The bug breaks every PagerDuty notification rule created via the REST API on 2.4.x.Bug reports
/domain-types/notification_rule/collections/allwithplugin_params.plugin_name = "pagerduty"andintegration_key = {"option": "explicit", "key": "<your-key>"}.etc/check_mk/conf.d/wato/notification_parameter.mk— the new entry'srouting_keyfield is('routing_key', '<your-key>')(legacy 2-tuple) instead of('cmk_postprocessed', 'explicit_password', ('<uuid>', '<your-key>'))(current form produced by the GUI).tail var/log/notify.log— plugin exits withOutput: Unable to retrieve password from passwordstoreandPlug-in exited with code 2. The PagerDuty incident is never created.Proposed changes
Expected behavior: A PagerDuty rule created via the REST API delivers notifications, identical to one created through the GUI.
Observed behavior: The dispatcher exits with
Unable to retrieve password from passwordstoreevery time. No PagerDuty incident is created.How this patch changes the current behavior:
APIPagerDutyKeyOption.to_mk_file_format()was writing the integration key tonotification_parameter.mkas the legacy 2-tuple("routing_key", key). The notification dispatcher'sget_password_from_env_or_context()(inpackages/cmk-notification-plugins/cmk/notification_plugins/utils.py) collects everyPARAMETER_ROUTING_KEY*env var into a list and only accepts the modern 3-tuple form("cmk_postprocessed", "explicit_password", (uuid, key)), identified by the literal string"explicit_password"in the list. The 2-tuple form falls through to a password-store lookup usingparameter[-2]("routing_key") — not a valid store ID — and the plugin exits.The GUI's FormSpec migrate function
_migrate_to_passwordincmk/gui/wato/_notification_parameter/_pagerduty.py:69-91already produces the 3-tuple form for UI-touched rules. The REST API path bypassed it becauseAPIPagerDutyKeyOptionwas a bespoke option class that wrote the legacy form directly; every other key-bearing plugin (ilert, signl4, opsgenie, sms_api) was already migrated to theAPICheckmkPassword_From*wrapper classes that produce the correct form.This PR:
APIPagerDutyKeyOption.PagerDutyPlugin.integration_keytoAPICheckmkPassword_FromKey(already in the same file, identical input shape).PagerDutyPluginModel.routing_keyfromRoutingKeyTypetoCheckmkPassword._normalize_pagerduty_routing_key()so existing on-disk rules in the legacy form keep working through the REST API path until the next save normalises them.from_mk_file_format()setcls(option="store", key=data[1])for the store option instead ofstore_id=data[1], so reading a stored-key rule via the REST API returned an emptystore_id.Unit tests: Added four round-trip tests in
tests/unit/cmk/gui/watolib/test_notifications.py:stored_password3-tupleThe first three tests fail on master without this patch.
Is this a new problem? Introduced in 2.4.0 when
CheckmkPassword/cmk_postprocessedbecame the canonical form across notification plugins. PagerDuty was missed in the migration that converted every other plugin toAPICheckmkPassword_From*wrappers. No third-party device or firmware change is involved — this is a pure regression inside Checkmk.Werk
20007 (
.werks/20007.md) — classfix, componentrest-api, level 1, compatible yes, version 2.6.0b1.Backport request
This breaks production PagerDuty notifications on 2.4.x for any user creating rules via the REST API (Ansible, Terraform, custom scripts). A backport to 2.4 and 2.5 would be very helpful.
Local testing
ruff format --checkandruff checkpass on all changed files.cmk_postprocessedform, the dispatcher delivers the notification successfully. (The fix in this PR is the same rewrite, applied automatically.)