[BUG]: version.Compare panics on an empty version string, crashing the reconcile loop

[ayuskauskas]

Summary

internal/version.Compare indexes version1[0] / version2[0] before its v-prefix check, so an empty string on either side panics. It is reachable from the reconcile loop via a Node annotation the apiserver does not schema-validate, which means an operator CrashLoop rather than a single degraded node.

Found during a coverage pass (#498); deliberately not fixed there because that PR is tests-only.

Reproduction

version.Compare("v1.0.0", "")
// runtime error: index out of range [0] with length 0

operator/internal/version/version.go:43:

func Compare(version1, version2 string) int {
	if version1[0] != 'v' {   // panics when version1 == ""
		version1 = "v" + version1
	}
	if version2[0] != 'v' {   // panics when version2 == ""
		version2 = "v" + version2
	}
	return semver.Compare(version1, version2)
}

Why it is reachable

The only caller is operator/internal/controller/skyhook_controller.go:1843:

} else if exists && _package.Version != packageStatus.Version {
	versionChangeDetected = true
	comparison := version.Compare(_package.Version, packageStatus.Version)

_package.Version comes from the CR spec and is webhook-validated. packageStatus.Version does not — it is read out of the nodewright.nvidia.com/nodeState_<name> annotation on the Node. That is an opaque JSON blob on a core object; the apiserver never applies the CRD schema to it, so the +kubebuilder:validation:Required marker on PackageStatus.Version buys nothing there.

kubectl nodewright update-state writes that annotation directly, so this is reachable through a supported user-facing path, not only through corruption.

The guard immediately above (_package.Version != packageStatus.Version) does not help: an empty stored version differs from any real spec version, so it falls straight through to Compare.

A second, related defect in the same call path

The call site treats -2 as "invalid version string":

if comparison == -2 {
	return nil, errors.New("error comparing package versions: invalid version string provided ...")
}

Compare returns semver.Compare, which only ever yields -1, 0 or +1. That branch is unreachable. Per the golang.org/x/mod/semver contract an invalid version sorts below a valid one, so today an unparsable stored version silently takes the downgrade path instead of erroring — which is the opposite of what the call site was written to do.

Suggested fix

1. Make Compare total: handle empty (and generally unparsable) input without panicking.
2. Decide the contract deliberately — either make Compare actually return the -2 the caller already checks for when either side is not valid semver, or drop the dead -2 branch and handle invalid input explicitly at the call site. internal/cli/utils.CompareVersions already solves the same problem a third way (returns 0 for invalid input), so picking one and making the two agree would be worthwhile.
3. Add table entries for both empty-argument positions, and for invalid-but-non-empty input.

Note that whichever contract is chosen changes reconcile behaviour for a node whose stored version is unparsable, so it wants a deliberate decision rather than just a nil-guard.

Environment

Affects main as of 68ffd071. Not version-specific; the code has been in this shape for some time.

[github-actions]

This issue uses a single-owner model. Comment /assign to claim it yourself, or /assign @user to assign one other person.

[Steve366]

/assign

[Steve366]

I would like to work on this. I plan to make version.Compare total, preserve the caller's -2 invalid-version contract, and add table-driven coverage for empty and invalid inputs. Please confirm this direction before I start.