[MOSIP-43841] Changes related to alerting setup via rapid deployment. #117
Conversation
Signed-off-by: Mohan E <[email protected]>
Signed-off-by: Mohan E <[email protected]>
Signed-off-by: Mohan E <[email protected]>
Signed-off-by: Mohan E <[email protected]>
WalkthroughReplace hard-coded cluster IDs, Slack webhook/channel, Kibana hosts, and domain values with template placeholders across Helmsman manifests and alerting templates; the alerting hook now accepts Slack channel, Slack API URL, and environment name as positional parameters. Changes
Sequence Diagram(s)sequenceDiagram
participant Helmsman as Helmsman (hook trigger)
participant Hook as alerting-setup.sh
participant Kube as Kubernetes (kubectl)
participant AM as Alertmanager / ConfigMaps
Helmsman->>Hook: invoke postInstall/postUpgrade with args (<slack-channel>, <slack-api-url>, <env-name>)
Hook->>Hook: sed replace placeholders in `alertmanager.yaml` and `patch-cluster-name.yaml`
Hook->>Kube: kubectl apply/patch updated manifests
Kube->>AM: deliver updated alertmanager & labels
AM-->>Kube: respond with apply status
Kube-->>Hook: return command output/status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Helmsman/hooks/alerting-setup.sh (1)
17-19: Non-idempotent in-place file modifications may cause issues on credential rotation or redeploy.The
sed -icommands modify alertmanager.yaml and patch-cluster-name.yaml in-place. This works correctly for the first invocation, but subsequent runs with different credentials will not find the original placeholders to replace, potentially leaving stale values in place.Recommended mitigation: Consider one of the following:
- Regenerate template files before each sed run — ensure alertmanager.yaml and patch-cluster-name.yaml are restored to their template state (with placeholders) before this hook executes.
- Use intermediate copies — apply sed to temporary copies and patch from those, or use kubectl create/replace with stdin instead of modifying source files.
- Document the expectation — clarify that the rapid deployment pattern assumes fresh file state per invocation.
For a true multi-environment or credential-rotation scenario, approach #1 is preferable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Helmsman/dsf/prereq-dsf.yaml(1 hunks)Helmsman/hooks/alerting-setup.sh(1 hunks)Helmsman/utils/alerting/alertmanager.yaml(2 hunks)Helmsman/utils/alerting/patch-cluster-name.yaml(1 hunks)
🔇 Additional comments (11)
Helmsman/utils/alerting/patch-cluster-name.yaml (1)
3-3: Placeholder introduction aligns with templating strategy.The hardcoded cluster name is properly replaced with a placeholder that will be substituted at runtime by the alerting-setup.sh hook (line 19 of that script).
Helmsman/utils/alerting/alertmanager.yaml (2)
4-4: Slack API URL placeholder introduced correctly.The hardcoded URL is replaced with
<YOUR-SLACK-API-URL>, which will be substituted by alerting-setup.sh line 18.
107-107: Slack channel placeholder introduced correctly.The hardcoded channel is replaced with
<YOUR-CHANNEL-HERE>, which will be substituted by alerting-setup.sh line 17.Helmsman/dsf/prereq-dsf.yaml (3)
45-46: Cluster ID placeholders align with templating approach.Both
grafana.global.cattle.clusterIdandglobal.cattle.clusterIdare correctly updated to use the<cluster-id>placeholder, which users should replace with their actual cluster identifier before deployment.
48-48: Trailing whitespace cleanup.Benign formatting improvement.
52-52: Verify: placeholders in postInstall hook require user substitution.The hook invocation passes
<slack-channel-name> <slack-api-url> <env-name>as placeholders that users must replace with actual values in their deployment configuration before running Helmsman. These three values directly map to positional parameters $1, $2, $3 expected by alerting-setup.sh (which uses them for sed substitutions in lines 17-19).Consider adding inline documentation to make this substitution requirement explicit, or verify whether these placeholders should be processed by a Helmsman template engine instead.
Helmsman/hooks/alerting-setup.sh (5)
9-11: Parameter-driven substitution enables flexible templating.The function now correctly receives Slack channel, API URL, and environment name as positional parameters ($1, $2, $3) instead of hardcoded defaults. This aligns with the Helmsman hook invocation pattern in prereq-dsf.yaml line 52.
17-19: Sed substitution patterns correctly match placeholder names.The sed patterns precisely target the placeholder strings in the template files:
- Line 17:
<YOUR-CHANNEL-HERE>→ alertmanager.yaml line 107 ✓- Line 18:
<YOUR-SLACK-API-URL>→ alertmanager.yaml line 4 ✓- Line 19:
<YOUR-CLUSTER-NAME-HERE>→ patch-cluster-name.yaml line 3 ✓All placeholders are properly accounted for.
24-24: kubectl patch operations use sed-modified files correctly.Both kubectl patch commands (lines 24 and 28) operate on files that have been substituted with actual values by the preceding sed operations (lines 17-19). The patch flow is correct.
Also applies to: 28-28
35-39: Comprehensive error handling configuration.The error handling is well-configured with
set -e,set -o nounset,set -o errtrace, andset -o pipefail. This ensures the script fails loudly on undefined variables or command failures rather than silently proceeding with invalid state.
9-11: Validate sed handling of special characters in credentials.If Slack URLs or channel names contain sed metacharacters (e.g.,
|,&,\,/), the substitution may fail or produce incorrect results. The current sed delimiter (|) is a reasonable choice to avoid conflicts with URLs (which contain/), but verify that:
- Slack webhook URLs do not contain literal
|characters- Channel names do not contain literal
|charactersAlternatively, consider escaping the variables or using a different delimiter if special characters are expected.
Also applies to: 17-19
ckm007
left a comment
ckm007
left a comment
There was a problem hiding this comment.
Need documentation aroind clusterid and slack url in README.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 783-790: The blockquote starting with "**Note:**" contains
inconsistent list indentation and a non-descriptive link text; update each
quoted list line to use consistent "> " prefix and uniform indentation so
markdownlint passes, and replace the link text "Link" with a descriptive label
such as "Slack incoming webhooks documentation"; keep the list items unchanged
otherwise and ensure the final sentence referencing slack_api_url, channel and
env-name in the rancher-monitoring section of prereq-dsf.yaml remains intact.
🧹 Nitpick comments (1)
README.md (1)
777-791: Standardize “Slack” capitalization and terminology.Use “Slack” consistently and consider “Slack webhook URL” for clarity.
✏️ Proposed doc tweak
- Alerting is part of cluster monitoring, where alert notifications are sent to the configured email or slack channel. - * `<slack-channel-name>` → slack channel name configured for alert notifications. - * `<slack-api-url>` → slack api url configured for alert notifications. + Alerting is part of cluster monitoring, where alert notifications are sent to the configured email or Slack channel. + * `<slack-channel-name>` → Slack channel name configured for alert notifications. + * `<slack-api-url>` → Slack webhook URL configured for alert notifications.
Signed-off-by: Mohanraj209 <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 777-781: The "Alerting Configuration" bullets are over-indented
and use lowercase "slack"; fix the Markdown list so each bullet is aligned as a
top-level list item under the "Alerting Configuration" heading (remove the extra
leading spaces before the "*" markers) and capitalize "Slack" in the bullet text
(e.g., change `<slack-channel-name>` and `<slack-api-url>` bullets to reference
"Slack" with proper capitalization) to satisfy MD007 and the naming consistency.
Signed-off-by: Mohanraj209 <[email protected]>
resolved |
Upated Documentation |
3 participants
Summary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.