pkg/readiness: Return error from CompareVersions for invalid semver

Returning 0 on parse errors conflated invalid input with equal versions,
causing false-positive blockers in api_deprecations and network checks
when version strings were unparseable. Now callers can distinguish
parse failures from genuine equality.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: harche

pkg/readiness/api_deprecations.go

@@ -38,7 +38,8 @@ func (c *APIDeprecationsCheck) Run(ctx context.Context, dc dynamic.Interface, cu
 
 		// Check RemovedInRelease annotation
 		removedIn := NestedString(arc.Object, "status", "removedInRelease")
-		if removedIn != "" && CompareVersions(removedIn, target) <= 0 {
+		cmp, err := CompareVersions(removedIn, target)
+		if removedIn != "" && err == nil && cmp <= 0 {
 			requestCount := NestedInt64(arc.Object, "status", "requestCount")
 			if requestCount > 0 {
 				blockers = append(blockers, map[string]any{

pkg/readiness/client.go

@@ -2,6 +2,7 @@ package readiness
 
 import (
 	"context"
+	"fmt"
 	"strings"
 
 	semver "github.com/blang/semver/v4"
@@ -151,16 +152,16 @@ const (
 )
 
 // CompareVersions compares two semver strings. Returns -1, 0, or 1.
-func CompareVersions(a, b string) int {
+func CompareVersions(a, b string) (int, error) {
 	va, err := semver.ParseTolerant(a)
 	if err != nil {
-		return 0
+		return 0, fmt.Errorf("invalid version %q: %w", a, err)
 	}
 	vb, err := semver.ParseTolerant(b)
 	if err != nil {
-		return 0
+		return 0, fmt.Errorf("invalid version %q: %w", b, err)
 	}
-	return va.Compare(vb)
+	return va.Compare(vb), nil
 }
 
 // FormatLabelSelector converts a map of labels to a label selector string.

pkg/readiness/client_test.go

@@ -67,20 +67,27 @@ func TestGetConditions_NoConditions(t *testing.T) {
 
 func TestCompareVersions(t *testing.T) {
 	tests := []struct {
-		a, b     string
-		expected int
+		a, b      string
+		expected  int
+		expectErr bool
 	}{
-		{"4.21.5", "4.21.8", -1},
-		{"4.21.8", "4.21.5", 1},
-		{"4.21.5", "4.21.5", 0},
-		{"4.22.0", "4.21.5", 1},
-		{"bad", "4.21.5", 0},
-		{"4.21.5", "bad", 0},
+		{"4.21.5", "4.21.8", -1, false},
+		{"4.21.8", "4.21.5", 1, false},
+		{"4.21.5", "4.21.5", 0, false},
+		{"4.22.0", "4.21.5", 1, false},
+		{"bad", "4.21.5", 0, true},
+		{"4.21.5", "bad", 0, true},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.a+"_vs_"+tt.b, func(t *testing.T) {
-			got := CompareVersions(tt.a, tt.b)
+			got, err := CompareVersions(tt.a, tt.b)
+			if tt.expectErr && err == nil {
+				t.Errorf("CompareVersions(%q, %q) expected error, got nil", tt.a, tt.b)
+			}
+			if !tt.expectErr && err != nil {
+				t.Errorf("CompareVersions(%q, %q) unexpected error: %v", tt.a, tt.b, err)
+			}
 			if got != tt.expected {
 				t.Errorf("CompareVersions(%q, %q) = %d, want %d", tt.a, tt.b, got, tt.expected)
 			}

pkg/readiness/network.go

@@ -27,7 +27,8 @@ func (c *NetworkCheck) Run(ctx context.Context, dc dynamic.Interface, current, t
 
 	// SDN deprecation warning
 	if networkType == "OpenShiftSDN" {
-		if target != "" && CompareVersions(target, "4.17.0") >= 0 {
+		cmp, err := CompareVersions(target, "4.17.0")
+		if target != "" && err == nil && cmp >= 0 {
 			result["sdn_warning"] = "OpenShiftSDN blocks upgrades to 4.17+; migrate to OVN-Kubernetes first."
 		} else {
 			result["sdn_warning"] = "OpenShiftSDN detected. Migration to OVN-Kubernetes is required for future upgrades to 4.17+."