OBSINTA-1002 consolidate COO 1.4.0 test case documentation#793
OBSINTA-1002 consolidate COO 1.4.0 test case documentation#793DavidRajnoha wants to merge 1 commit intoopenshift:mainfrom
Conversation
Add test cases for OU-632, OU-1039, OU-1123, OU-1205, OU-1213, and OU-1221. Include simulation scenario CSVs and RBAC resources referenced by the test documentation. Add CSV file references to prerequisites sections across all test docs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: DavidRajnoha The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
📝 WalkthroughWalkthroughAdds Kubernetes manifests for HTPasswd-based authentication and permission testing infrastructure, alongside expanded test documentation that includes new test scenarios for incident bar trimming, alert padding, and permission denied handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
docs/incident_detection/tests/3.api_calls_data_loading_flows.md (1)
110-112: Track this TODO with an owner or issue link.
TODO COO 1.4is easy to lose during release prep; adding an explicit tracking reference will keep this actionable.If useful, I can draft a ready-to-paste checklist item format with owner/date/issue fields.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/incident_detection/tests/3.api_calls_data_loading_flows.md` around lines 110 - 112, Replace the ambiguous "TODO COO 1.4" under the "### 3.5 Data Integrity" section with a tracked item that includes an explicit owner and issue reference: change the checklist line "- [ ] Incident grouping by `group_id` works correctly" to include fields like "Owner: <github-username>", "Issue: <link/number>", and "Due: <date>" (or a direct issue link) so the task is traceable; ensure the `TODO COO 1.4` token is removed or replaced with the issue/owner metadata to prevent it being lost during release prep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/incident_detection/resources/htpasswd-secret.yaml`:
- Around line 4-19: Remove the checked-in default credentials by deleting the
base64 value under the Secret named "htpass-secret" (metadata.name) and
replacing the static data key "htpasswd" with a templated placeholder or
removing the data section entirely so each environment must generate the secret;
update the manifest to either use stringData with a placeholder (e.g.,
<REPLACE_WITH_BASE64_OR_GENERATE>) or include comments directing operators to
create the secret per-environment (e.g., generate htpasswd and kubectl create
secret) rather than committing the hashed credential in the file, ensuring the
Secret still targets the same namespace "openshift-config".
In `@docs/incident_detection/resources/oauth-htpasswd.yaml`:
- Around line 10-17: Add a prominent warning at the top of the OAuth manifest
stating that applying this spec.identityProviders manifest will replace all
existing OAuth identity providers and can lock out clusters with existing
authentication; update the docs/incident_detection/resources/oauth-htpasswd.yaml
file to include the warning before the spec (e.g., reference the affected block
like the spec.identityProviders entry and the htpasswd provider named
my_htpasswd_provider with htpasswd.fileData.name set to htpass-secret) so
readers clearly know not to apply this to production clusters.
In `@docs/incident_detection/tests/2.ui_display_flows.md`:
- Around line 150-168: Update the two test checklist items that currently
reference AlertC to instead use the multi-severity Incident D: change the
reference under "End Times Should Be 5-Minute Rounded" and the reference under
"Start Times Match Alert Tooltip" so they instruct testers to use Incident D
(multi-severity) rather than AlertC, and add a brief note in those items that
Incident D is the multi-severity test case to ensure the boundary-time checks
are actionable.
---
Nitpick comments:
In `@docs/incident_detection/tests/3.api_calls_data_loading_flows.md`:
- Around line 110-112: Replace the ambiguous "TODO COO 1.4" under the "### 3.5
Data Integrity" section with a tracked item that includes an explicit owner and
issue reference: change the checklist line "- [ ] Incident grouping by
`group_id` works correctly" to include fields like "Owner: <github-username>",
"Issue: <link/number>", and "Due: <date>" (or a direct issue link) so the task
is traceable; ensure the `TODO COO 1.4` token is removed or replaced with the
issue/owner metadata to prevent it being lost during release prep.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (7)
docs/incident_detection/simulate_scenarios/100-alerts-14-days.csvis excluded by!**/*.csvdocs/incident_detection/simulate_scenarios/1000-alerts-15-min.csvis excluded by!**/*.csvdocs/incident_detection/simulate_scenarios/complete-test-data.csvis excluded by!**/*.csvdocs/incident_detection/simulate_scenarios/data-loading-silences.csvis excluded by!**/*.csvdocs/incident_detection/simulate_scenarios/filtering-test-data.csvis excluded by!**/*.csvdocs/incident_detection/simulate_scenarios/long-incident-15-days.csvis excluded by!**/*.csvdocs/incident_detection/simulate_scenarios/mixed-severity-escalation.csvis excluded by!**/*.csv
📒 Files selected for processing (7)
docs/incident_detection/resources/htpasswd-secret.yamldocs/incident_detection/resources/limited-permissions-user.yamldocs/incident_detection/resources/oauth-htpasswd.yamldocs/incident_detection/tests/0.overview.mddocs/incident_detection/tests/1.filtering_flows.mddocs/incident_detection/tests/2.ui_display_flows.mddocs/incident_detection/tests/3.api_calls_data_loading_flows.md
| # htpasswd -c -B -b users.htpasswd testuser password123 | ||
| # | ||
| # Then base64 encode it: | ||
| # base64 -w0 users.htpasswd | ||
| # | ||
| # Replace the data.htpasswd value below with your encoded content | ||
|
|
||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: htpass-secret | ||
| namespace: openshift-config | ||
| type: Opaque | ||
| data: | ||
| # base64 encoded: testuser:$2y$05$fBn5ChTgiV0A/6HEfoNKleU3CLVIWuV2816XVIsmmhwAz.fBpDObe | ||
| htpasswd: dGVzdHVzZXI6JDJ5JDA1JGZCbjVDaFRnaVYwQS82SEVmb05LbGVVM0NMVklXdVYyODE2WFZJc21taHdBei5mQnBET2JlCg== |
There was a problem hiding this comment.
Remove checked-in default credentials from the manifest.
Line 4 and Line 19 effectively publish reusable credentials. Please keep this file templated and require per-environment secret generation.
Proposed change
-# htpasswd -c -B -b users.htpasswd testuser password123
+# htpasswd -c -B -b users.htpasswd <username> <strong-random-password>
@@
- # base64 encoded: testuser:$2y$05$fBn5ChTgiV0A/6HEfoNKleU3CLVIWuV2816XVIsmmhwAz.fBpDObe
- htpasswd: dGVzdHVzZXI6JDJ5JDA1JGZCbjVDaFRnaVYwQS82SEVmb05LbGVVM0NMVklXdVYyODE2WFZJc21taHdBei5mQnBET2JlCg==
+ # base64 encoded content of users.htpasswd
+ htpasswd: <REPLACE_WITH_BASE64_HTPASSWD_CONTENT>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # htpasswd -c -B -b users.htpasswd testuser password123 | |
| # | |
| # Then base64 encode it: | |
| # base64 -w0 users.htpasswd | |
| # | |
| # Replace the data.htpasswd value below with your encoded content | |
| apiVersion: v1 | |
| kind: Secret | |
| metadata: | |
| name: htpass-secret | |
| namespace: openshift-config | |
| type: Opaque | |
| data: | |
| # base64 encoded: testuser:$2y$05$fBn5ChTgiV0A/6HEfoNKleU3CLVIWuV2816XVIsmmhwAz.fBpDObe | |
| htpasswd: dGVzdHVzZXI6JDJ5JDA1JGZCbjVDaFRnaVYwQS82SEVmb05LbGVVM0NMVklXdVYyODE2WFZJc21taHdBei5mQnBET2JlCg== | |
| # htpasswd -c -B -b users.htpasswd <username> <strong-random-password> | |
| # | |
| # Then base64 encode it: | |
| # base64 -w0 users.htpasswd | |
| # | |
| # Replace the data.htpasswd value below with your encoded content | |
| apiVersion: v1 | |
| kind: Secret | |
| metadata: | |
| name: htpass-secret | |
| namespace: openshift-config | |
| type: Opaque | |
| data: | |
| # base64 encoded content of users.htpasswd | |
| htpasswd: <REPLACE_WITH_BASE64_HTPASSWD_CONTENT> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/incident_detection/resources/htpasswd-secret.yaml` around lines 4 - 19,
Remove the checked-in default credentials by deleting the base64 value under the
Secret named "htpass-secret" (metadata.name) and replacing the static data key
"htpasswd" with a templated placeholder or removing the data section entirely so
each environment must generate the secret; update the manifest to either use
stringData with a placeholder (e.g., <REPLACE_WITH_BASE64_OR_GENERATE>) or
include comments directing operators to create the secret per-environment (e.g.,
generate htpasswd and kubectl create secret) rather than committing the hashed
credential in the file, ensuring the Secret still targets the same namespace
"openshift-config".
| spec: | ||
| identityProviders: | ||
| - name: my_htpasswd_provider | ||
| mappingMethod: claim | ||
| type: HTPasswd | ||
| htpasswd: | ||
| fileData: | ||
| name: htpass-secret |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual file content and surrounding context
cat -n docs/incident_detection/resources/oauth-htpasswd.yamlRepository: openshift/monitoring-plugin
Length of output: 610
🏁 Script executed:
# Check if there are other files in this directory and understand the context
ls -la docs/incident_detection/resources/Repository: openshift/monitoring-plugin
Length of output: 395
🏁 Script executed:
# Search for how this file is referenced or used in the codebase
rg "oauth-htpasswd" --type yaml --type mdRepository: openshift/monitoring-plugin
Length of output: 224
🏁 Script executed:
# Look for any documentation about OAuth configuration
find docs -type f \( -name "*.md" -o -name "*.yaml" \) | xargs grep -l "oauth\|identityProviders" 2>/dev/null | head -20Repository: openshift/monitoring-plugin
Length of output: 188
🏁 Script executed:
# Check the test documentation file that references this
cat -n docs/incident_detection/tests/3.api_calls_data_loading_flows.md | head -100Repository: openshift/monitoring-plugin
Length of output: 6680
🏁 Script executed:
# Search for any warnings or guidance about OAuth configuration
rg "OAuth\|identityProviders\|oauth" docs/incident_detection/tests/ --type md -A 3 -B 3Repository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Check if there's a README or setup guide in the resources directory
find docs/incident_detection -name "README*" -o -name "*setup*" -o -name "*guide*" | xargs cat 2>/dev/nullRepository: openshift/monitoring-plugin
Length of output: 53
🏁 Script executed:
# Check if there are any other identity provider examples or merge-based approaches
rg "identityProviders|mergeStrategy|patch" docs/ --type yaml --type mdRepository: openshift/monitoring-plugin
Length of output: 145
Add explicit warning that this OAuth manifest replaces all identity providers.
The existing comment implies fresh setup, but doesn't clearly warn that applying this manifest will completely replace any existing identity providers on the cluster. Even for documentation/test setup, add a prominent warning at the top of the file (e.g., "
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/incident_detection/resources/oauth-htpasswd.yaml` around lines 10 - 17,
Add a prominent warning at the top of the OAuth manifest stating that applying
this spec.identityProviders manifest will replace all existing OAuth identity
providers and can lock out clusters with existing authentication; update the
docs/incident_detection/resources/oauth-htpasswd.yaml file to include the
warning before the spec (e.g., reference the affected block like the
spec.identityProviders entry and the htpasswd provider named
my_htpasswd_provider with htpasswd.fileData.name set to htpass-secret) so
readers clearly know not to apply this to production clusters.
| - [ ] **End Times Should Be 5-Minute Rounded**: Use AlertC from test data (multi-severity incident) | ||
| - Hover over each severity segment within the incident bar | ||
| - Verify End time in tooltip shows rounded value (e.g., "23:30") not unrounded (e.g., "23:29:59") | ||
| - Verify all interval boundaries display at 5-minute precision in tooltips | ||
|
|
||
| #### Issue 2: Start Times 5 Minutes Off (OU-1221) | ||
| **BUG**: Multi-severity incident bar tooltip start times are 5 minutes off from the values shown in alert tooltips and alerts table. | ||
|
|
||
| **Background**: | ||
| - When hovering over a severity segment in an incident bar, the Start time shown differs from: | ||
| - The Start time shown in the corresponding alert tooltip | ||
| - The Start time shown in the alerts details table | ||
| - This creates user confusion as the same data point shows different times in different UI elements | ||
|
|
||
| - [ ] **Start Times Match Alert Tooltip**: Use AlertC from test data | ||
| - Click incident to open alerts chart | ||
| - Hover over an alert bar: Note the Start time in alert tooltip | ||
| - Hover over the corresponding severity segment in the incident bar | ||
| - Verify incident tooltip Start time matches alert tooltip Start time exactly |
There was a problem hiding this comment.
Use Incident D for multi-severity checks, not AlertC.
Line 150 and Line 164 currently reference AlertC as multi-severity, but AlertC is single-point/single-severity in this dataset. This makes the boundary-time checks non-actionable.
Proposed correction
-- [ ] **End Times Should Be 5-Minute Rounded**: Use AlertC from test data (multi-severity incident)
+- [ ] **End Times Should Be 5-Minute Rounded**: Use Incident D from test data (multi-severity incident)
@@
-- [ ] **Start Times Match Alert Tooltip**: Use AlertC from test data
+- [ ] **Start Times Match Alert Tooltip**: Use Incident D from test data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/incident_detection/tests/2.ui_display_flows.md` around lines 150 - 168,
Update the two test checklist items that currently reference AlertC to instead
use the multi-severity Incident D: change the reference under "End Times Should
Be 5-Minute Rounded" and the reference under "Start Times Match Alert Tooltip"
so they instruct testers to use Incident D (multi-severity) rather than AlertC,
and add a brief note in those items that Incident D is the multi-severity test
case to ensure the boundary-time checks are actionable.
|
@DavidRajnoha: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
Add test cases for OU-632, OU-1039, OU-1123, OU-1205, OU-1213, and OU-1221. Include simulation scenario CSVs and RBAC resources referenced by the test documentation. Add CSV file references to prerequisites sections across all test docs.
Summary by CodeRabbit