Skip to content

[ACM-30429] Fix nil pointer dereference in Equal() method#1986

Merged
openshift-merge-bot[bot] merged 1 commit intostolostron:mainfrom
dislbenn:ACM-30429-fix-nil-pointer
Apr 29, 2026
Merged

[ACM-30429] Fix nil pointer dereference in Equal() method#1986
openshift-merge-bot[bot] merged 1 commit intostolostron:mainfrom
dislbenn:ACM-30429-fix-nil-pointer

Conversation

@dislbenn
Copy link
Copy Markdown
Contributor

@dislbenn dislbenn commented Apr 29, 2026

Description

Fixes a critical nil pointer dereference bug in the DiscoveredCluster.Equal() method that causes the operator to panic and crash during reconciliation when ActivityTimestamp or CreationTimestamp fields are nil.

Related Issue

Fixes ACM-30429

Changes Made

  1. Added timestampsEqual() helper function in both api/v1 and api/v1alpha1 packages

    • Safely compares two *metav1.Time pointers
    • Handles nil cases: both nil = equal, one nil = not equal
    • Truncates to second precision when both non-nil
  2. Updated Equal() methods to use the helper function instead of direct .Truncate() calls

    • Fixed in api/v1/discoveredcluster_types.go (lines 177, 180)
    • Fixed in api/v1alpha1/discoveredcluster_types.go (lines 93, 94)
  3. Updated component version from 2.17 to 5.0

    • Updated COMPONENT_VERSION file: 2.17.0 → 5.0.0
    • Updated build/Dockerfile.rhtap: CPE version 2.17 → 5.0

Screenshots (if applicable)

N/A - Bug fix, no visual changes

Checklist

  • I have tested the changes locally and they are functioning as expected.
  • I have updated the documentation (if necessary) to reflect the changes.
  • I have added/updated relevant unit tests (if applicable).
  • I have ensured that my code follows the project's coding standards.
  • I have checked for any potential security issues and addressed them.
  • I have added necessary comments to the code, especially in complex or unclear sections.
  • I have rebased my branch on top of the latest main/master branch.

Additional Notes

Before this fix:

// ❌ Panic if ActivityTimestamp is nil
a.Spec.ActivityTimestamp.Truncate(time.Second) != b.Spec.ActivityTimestamp.Truncate(time.Second)

After this fix:

// ✅ Safe nil handling
!timestampsEqual(a.Spec.ActivityTimestamp, b.Spec.ActivityTimestamp)

The panic would occur during reconciliation whenever a DiscoveredCluster object had a nil timestamp field, causing the operator to crash and requiring a restart.

Reviewers

/cc @ngraham20 @cameronmwall

Definition of Done

  • Code is reviewed.
  • Code is tested.
  • Documentation is updated.
  • All checks and tests pass.
  • Approved by at least one reviewer.
  • Merged into the main/master branch.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 29, 2026

Warning

Rate limit exceeded

@dislbenn has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 30 minutes and 6 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Enterprise

Run ID: 66743f93-0b14-4728-9463-46586ffcad47

📥 Commits

Reviewing files that changed from the base of the PR and between 3e07e53 and 4bbbe18.

📒 Files selected for processing (3)
  • COMPONENT_VERSION
  • api/v1/discoveredcluster_types.go
  • build/Dockerfile.rhtap
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 30 minutes and 6 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci openshift-ci Bot added the approved PR approval has been given label Apr 29, 2026
@dislbenn dislbenn force-pushed the ACM-30429-fix-nil-pointer branch from f45cd8d to 7e11026 Compare April 29, 2026 17:50
Fixed nil pointer panic in DiscoveredCluster.Equal() by adding nil checks
for ActivityTimestamp and CreationTimestamp pointers before calling Truncate().

Changes:
- Added timestampsEqual() helper function to safely compare *metav1.Time pointers
- Updated Equal() method in api/v1 to use the helper
- Updated component version from 2.17 to 5.0 in COMPONENT_VERSION and Dockerfile

Fixes: ACM-30429
Signed-off-by: dislbenn <dbennett@redhat.com>
@dislbenn dislbenn force-pushed the ACM-30429-fix-nil-pointer branch from 7e11026 to 4bbbe18 Compare April 29, 2026 19:24
@cameronmwall
Copy link
Copy Markdown
Contributor

/lgtm

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 29, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cameronmwall, dislbenn

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [cameronmwall,dislbenn]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot Bot merged commit 43aea18 into stolostron:main Apr 29, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved PR approval has been given dco-signoff: yes lgtm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants