fix: reject non-semver versions for OCI charts

[immanuwell]

Description

Fixes #5156

OCI charts with an empty or non semver version could panic in Helm download code. This patch bails out early and returns a clean error instead. Small fix, but it kills a nasty footgun.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation update

How Has This Been Tested?

• Manual test
• Auto test added

Repro:

1. Use an OCI chart and leave spec.version empty, or set it to latest
2. Hit the Helm path that installs it
3. Before this patch, the path can panic with a nil pointer dereference in Helm
4. After this patch, it returns OCI charts require a fixed SemVer version

Checks I ran:

• go test ./pkg/helm -run TestCommands_locateChart_OCIRequiresFixedVersion -count=1
• go test ./pkg/helm/...

Checklist

• My code follows the style guidelines of this project
• My commit messages are signed-off
• I have performed a self-review of my code
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• Any dependent changes have been merged and published in downstream modules
• I have checked my code and corrected any misspellings

[jnummelin]

In what state would the Chart object be left at with the guard?

I'm also wondering if we'd be able to validate that on the API level via CEL or something? With webhook validation for sure, but that's a bit bigger effort to make happen. Not saying it might be worth it but CEL can do pretty powerful things too

[twz123]

In what state would the Chart object be left at with the guard?

Would probably be good if we could cover this in the Helm integration test.

I'm also wondering if we'd be able to validate that on the API level via CEL or something? With webhook validation for sure, but that's a bit bigger effort to make happen. Not saying it might be worth it but CEL can do pretty powerful things too

We don't need CEL or a webhook for this, I'd say. As long as the error reporting on the Chart resource is understandable, I'd keep it simple. Especially for CEL, we'd need to re-implement a Go library function (if that's even possible) and keep the CEL in sync with upstream changes. Moreover, the CEL is part of the CRDs, so changing this in patch releases needs special care. A webhook could of course just call the Go function, but we'd need to setup a validating webhook from scratch, just for this. Feels like overkill. If the webhook would already be in place, then things would be different, of course. Maybe some day...

[jnummelin]

Especially for CEL, we'd need to re-implement a Go library function

Why? Isn't the API-server validating things with CEL?

the CEL is part of the CRDs, so changing this in patch releases needs special care

Sure. I just think if we can use CEL to validate this properly, we should. In a minor release maybe...