Skip to content

fix(version): prevent panic on empty or invalid version string in Compare - #645

Open
SatyamPandey-07 wants to merge 1 commit into
NVIDIA:mainfrom
SatyamPandey-07:bugfix/version-compare-empty-string-panic
Open

SatyamPandey-07 wants to merge 1 commit into
NVIDIA:mainfrom
SatyamPandey-07:bugfix/version-compare-empty-string-panic

Conversation

@SatyamPandey-07

Copy link
Copy Markdown

Related Issue

Fixes #644

Changes Proposed

  • operator/internal/version/version.go: Validated inputs in Compare(version1, version2 string) int using IsValid(version1) and IsValid(version2) prior to accessing slice indices version1[0] and version2[0]. If either version is empty or invalid semver, Compare now returns -2. This prevents runtime index-out-of-range panics when reconciling nodes with missing or empty version state annotations and fulfills the error contract expected by skyhook_controller.go (if comparison == -2).
  • operator/internal/version/version_test.go: Added table-driven test cases verifying that Compare returns -2 when given an empty string on the left, an empty string on the right, empty strings on both sides, or invalid semver strings (e.g., "dev").

How Has This Been Tested?

  • Unit tests passed: Added table entries to version_test.go verifying that Compare returns -2 for empty strings on either or both sides, as well as invalid semver inputs, alongside existing ordering test cases.
  • Reconcile error path verified: Verified that operator/internal/controller/skyhook_controller.go line 1968 receives -2 on invalid/empty version annotations and properly returns a reconciliation error instead of crashing.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

@github-actions

Copy link
Copy Markdown
Contributor

Welcome to NodeWright, @SatyamPandey-07! Thanks for your first pull request.

Before review, please ensure:

  • All commits are signed off and signed: git commit -s -S (see CONTRIBUTING.md)
  • Commits follow Conventional Commits
  • CI checks pass (tests, lint, security scan)
  • The PR description explains the why behind your changes

A maintainer will review this soon.

@github-actions github-actions Bot added doc Documentation change (PR path label; doc issues use the Documentation type) component/operator Skyhook operator (controller-manager) labels Sep 19, 2026
@github-actions

github-actions Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

✅ Every non-bot commit on this pull request is now signed off and signed. Thanks!

@SatyamPandey-07
SatyamPandey-07 force-pushed the bugfix/version-compare-empty-string-panic branch from 25c2e2c to f91a0fb Compare September 19, 2026 10:04
@coderabbitai

coderabbitai Bot commented Sep 19, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: NVIDIA/nodewright/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 78cc88a3-7bee-4aaa-9001-84426a764d32

📥 Commits

Reviewing files that changed from the base of the PR and between cdc1824 and f91a0fb.

📒 Files selected for processing (4)
  • issue.md
  • operator/internal/version/version.go
  • operator/internal/version/version_test.go
  • pr.md

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

version.Compare now validates both version strings before accessing or comparing them. It returns -2 when either string is empty or invalid. Table tests cover invalid operands, and issue and PR documentation describe the reconciliation error path.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the fix: preventing panics in version.Compare for empty or invalid version strings.
Description check ✅ Passed The description directly explains the validation change, expected -2 result, tests, and reconciliation impact.
Linked Issues check ✅ Passed Issue #644 requires version.Compare to validate both inputs and return -2 for empty or invalid semver values. operator/internal/version/version.go now calls IsValid for both inputs before inde…
Out of Scope Changes check ✅ Passed The changes stay within issue #644. The production change fixes version validation. The tests verify the required comparison behavior. The issue and PR documentation describe the same panic, reconcili…
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR

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

@SatyamPandey-07

Copy link
Copy Markdown
Author

@lockwobr Hi, just bumping this PR in case it slipped through your notifications. Let me know if you need any changes or further testing!

@lockwobr

Copy link
Copy Markdown
Collaborator

@SatyamPandey-07 Looks like maybe you commited two files you didnt mean too. Please remove them. They are not passing a test which is making your pr red.

@github-actions github-actions Bot removed the doc Documentation change (PR path label; doc issues use the Documentation type) label Sep 20, 2026
…pare

Signed-off-by: Satyam Pandey <satyam53@tsecol.onmicrosoft.com>
@SatyamPandey-07
SatyamPandey-07 force-pushed the bugfix/version-compare-empty-string-panic branch from 01ec89d to c81378f Compare September 20, 2026 02:24
@SatyamPandey-07

Copy link
Copy Markdown
Author

@lockwobr Thanks for the heads up! I have rebased the branch onto the latest main, squashed the commits into a single signed and DCO-compliant commit. The PR now only modifies operator/internal/version/version.go and operator/internal/version/version_test.go. All 29 unit tests pass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/operator Skyhook operator (controller-manager)

Projects

None yet

2 participants