ROSAENG-14171: add probe history and LB security group checks to console-ErrorBudgetBurn investigation#849
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@MateSaary: This pull request references ROSAENG-14171 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe PR extends the AWS ChangesSecurity Group Validation and Probe History Investigation
OCM Mock Import Alias Fix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MateSaary The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/investigations/consoleerrorbudgetburn/consoleerrorbudgetburn.go (1)
256-301: ⚡ Quick winConsider handling
net.ParseCIDRerror to avoid silent failures.Line 262 silently discards the error from
net.ParseCIDR. IfmachineCIDRis non-empty but malformed,machineNetwill be nil and CIDR-based matching will silently fail—only0.0.0.0/0rules will match, potentially producing misleading warnings.Additionally, the function only inspects
IpRanges(IPv4). Security group rules can also specifyIpv6Ranges,PrefixListIds, orUserIdGroupPairs, which would be missed. This is acceptable for an informational check but worth documenting.Proposed fix for CIDR error handling
func sgAllowsTCPInbound(securityGroups []ec2v2types.SecurityGroup, fromPort, toPort int32, machineCIDR string) bool { var machineIP net.IP var machineNet *net.IPNet if machineCIDR != "" { - machineIP, machineNet, _ = net.ParseCIDR(machineCIDR) + var err error + machineIP, machineNet, err = net.ParseCIDR(machineCIDR) + if err != nil { + // Log or handle; for now treat as empty to fall back to 0.0.0.0/0 matching only + machineNet = nil + } }Based on learnings from the coding guidelines: "Never ignore error returns". While this specific failure mode is unlikely (cluster config should provide valid CIDRs), handling the error explicitly makes the fallback behavior intentional rather than accidental.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/investigations/consoleerrorbudgetburn/consoleerrorbudgetburn.go` around lines 256 - 301, In the sgAllowsTCPInbound function, the error returned from net.ParseCIDR is being silently discarded with underscore on the line where machineCIDR is parsed. If machineCIDR is provided but malformed, machineNet will be nil and the CIDR-based matching logic will silently fail, producing incorrect results. Capture and explicitly handle the error from net.ParseCIDR—either by logging it, returning an error from the function, or making the fallback behavior intentional in a comment, so that callers understand when CIDR validation fails and the function falls back to only matching against 0.0.0.0/0.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/investigations/consoleerrorbudgetburn/consoleerrorbudgetburn.go`:
- Around line 256-301: In the sgAllowsTCPInbound function, the error returned
from net.ParseCIDR is being silently discarded with underscore on the line where
machineCIDR is parsed. If machineCIDR is provided but malformed, machineNet will
be nil and the CIDR-based matching logic will silently fail, producing incorrect
results. Capture and explicitly handle the error from net.ParseCIDR—either by
logging it, returning an error from the function, or making the fallback
behavior intentional in a comment, so that callers understand when CIDR
validation fails and the function falls back to only matching against 0.0.0.0/0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7fd85dbd-2075-481a-800c-59ea29d6e44c
📒 Files selected for processing (5)
pkg/aws/aws.gopkg/aws/mock/aws.gopkg/investigations/consoleerrorbudgetburn/consoleerrorbudgetburn.gopkg/investigations/consoleerrorbudgetburn/consoleerrorbudgetburn_test.gopkg/ocm/mock/ocmmock.go
|
@MateSaary: 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. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #849 +/- ##
==========================================
+ Coverage 43.09% 43.52% +0.43%
==========================================
Files 71 71
Lines 8254 8450 +196
==========================================
+ Hits 3557 3678 +121
- Misses 4484 4554 +70
- Partials 213 218 +5
🚀 New features to boost your workflow:
|
3 participants
What type of PR is this?
feature
What this PR does / Why we need it?
Adds two new informational checks to the consoleerrorbudgetburn investigation, automating manual SOP troubleshooting steps (sections 3.3 and 3.6-3.7 of console-ErrorBudgetBurn.md):
Probe History (classic only):
Load Balancer Security Group checks (AWS Classic clusters only):
Both checks are informational-only — no automated actions (no service logs, no silencing).
Special notes for your reviewer
Test Coverage
Guidelines for CAD investigations
Test coverage checks
Pre-checks (if applicable)
Summary by CodeRabbit
New Features
Tests