Support Helm semver encoding in OCI repositories#1834
Conversation
76b4044 to
314c103
Compare
|
Must-have change — could someone review it? @stealthybox @matheuscscp @stefanprodan |
| // Helm translates `+` to `_` in OCI tags, because `+` is not a valid tag character. | ||
| versionStr := strings.Replace(t, "_", "+", 1) | ||
| // It would be even better to use `org.opencontainers.image.version` annotation | ||
| // if present, but that adds a fetch for each tag. | ||
| v, err := version.ParseVersion(versionStr) |
There was a problem hiding this comment.
OCIRepository is not specific to Helm in any way, we are going to break versions that contain _ as suffix. For this replacement to be safe, we should only do it if the layer selector is set to the Helm OCI type https://fluxcd.io/flux/components/helm/helmreleases/#ocirepository-reference-example
There was a problem hiding this comment.
I'm willing to do that; another option is to do this only if ParseVersion produces an error. It looks to me like both semver.StrictNewVersion and semver.NewVersion both reject (return err for) version numbers that contain _ at all, so our risk would be that we would accept versions that we currently reject.
There was a problem hiding this comment.
Interestingly, in the tests I didn't need to actually set the media type on the podinfo images for the test to pass.
6e3eb7c to
f772b6e
Compare
@stefanprodan Would it make sense to introduce either a controller-level flag or an OCIRepository spec flag to always replace _ with +? For example, we could build an artifact tag based on the application image version and the CI pipeline ID — e.g., v1.0.0+pid123. This would allow us to update manifests while keeping the underlying image version unchanged. |
It doesn't make sense, since Helm v4 charts require the layer selector, not having this in your OCIRepo will result in a pull error. |
I see your point regarding Helm v4 and the required layer selector. What I had in mind is a slightly broader use case — not limited to Helm artifacts. This would also apply to artifacts built and pushed via flux push artifact, where the OCIRepository is used more generically. |
|
I don't see any value in encoding build metadata in tags for |
My use case is a bit different — it’s more about how semver sorting behaves across environments. For example, I have two environments:
This works fine when only the app version changes. But in reality, we also change configs/secrets/deployments without bumping the app version itself. Especially when manifests are stored in a separate repository from the application. So from semver perspective I’d want something like:
so that ordering still makes sense and Flux picks the “latest” correctly, even if the app version didn’t change. Annotations help with traceability, but they don’t affect how versions are selected/sorted — that’s the gap I’m trying to solve. |
|
Build metadata is intended to store the Git SHA, commit SHAs are not numerical and can not be sorted. |
|
(Sorry, I'll get back to this shortly; I had a few other things come up.) My use case is that https://github.com/mindersec/minder produces helm charts with versions like |
matheuscscp
left a comment
There was a problem hiding this comment.
@evankanderson Please address all the three comments (one from Stefan, two from me), rebase, squash, force-push and then we can merge 👍
|
Please address the comments as soon as you can, I'd like to ship a patch release for this next week. |
Signed-off-by: Evan Anderson <evan.k.anderson@gmail.com>
f772b6e to
abe9571
Compare
evankanderson
left a comment
There was a problem hiding this comment.
Okay, I figured out how to run the tests for just this (make test-ctrl GO_TEST_PREFIX=TestOCIRepository_getArtifactRef) and they seem to pass. Let me know if you need other changes, though I have family visiting this weekend and I'll be on a trip next week.
|
(I also squashed all the commits and rebased) |
matheuscscp
left a comment
There was a problem hiding this comment.
@evankanderson You missed one thing, pls fix, amend, force-push 🙏
|
|
||
| var matchingVersions []*semver.Version | ||
| for _, t := range validTags { | ||
| // Helm translates `+` to `_` in OCI tags, because `+` is not permitted in OCI tags. |
There was a problem hiding this comment.
Reopening this as a separate comment.
Please note, I'm suggesting a few more comment lines here for a full explanation, this is quite a quirky behavior that is only for Helm, so it deserves full details in the comment.
| // Helm translates `+` to `_` in OCI tags, because `+` is not permitted in OCI tags. | |
| // Helm translates `+` to `_` in OCI tags, because `+` is not permitted in OCI tags. | |
| // On the other hand, `_` is not permitted in SemVer. | |
| // Each character not being permitted in one of the two sides establishes | |
| // a perfect bijection between them, which makes the mapping implemented | |
| // by Helm and honored here completely safe. |
(See also the related fluxcd/flux2#4674, which gave me the hint I needed.)
I'm attempting to use a Helm chart with versions like
0.20250616.4188+ref.2dec694(which gets encoded as the tag:0.20250616.4188_ref.2dec694) with the instructions here: https://fluxcd.io/flux/cheatsheets/oci-artifacts/#helm-ociUnfortunately, because
0.20250616.4188_ref.2dec694is not a valid semver, I get the error message:Based on the documentation at https://helm.sh/docs/topics/registries/#oci-feature-deprecation-and-behavior-changes-with-v380 and the discussion at opencontainers/distribution-spec#154, it seems plausible to assume that replacing
_with+(one time) when parsing semantic versions from tags is a reasonable thing to do. I'd be willing to discuss fetching the annotations for all the OCI manifests, but I suspect that's undesirable from an efficiency point of view. I'd also be willing to add a verification of theorg.opencontainers.image.versionannotation on the actually selected tag, but I was trying to minimize the amount of load Flux induces while supporting build metadata in semver.