add Release version as an optional field in the CSV

[grokspawn]

As part of the epic, in order for bundle authors to be able to specify release information, it must be part of the CSV.

This PR proposes an optional additional release field which expresses this information, enabling its use in the catalog creation / consumption flows both on- and off-cluster.

Now with a bonus bundle manifest validator to enforce the release version bundle naming convention, which is used by the operator-verify tool.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign perdasilva for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[codecov]

Codecov Report

❌ Patch coverage is 47.61905% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 37.78%. Comparing base (d6a5128) to head (4a61e98).
⚠️ Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/lib/release/release.go	48.00%	11 Missing and 2 partials ⚠️
pkg/validation/internal/bundle.go	40.00%	4 Missing and 2 partials ⚠️
pkg/manifests/bundleloader.go	60.00%	1 Missing and 1 partial ⚠️
pkg/operators/v1alpha1/zz_generated.deepcopy.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #454      +/-   ##
==========================================
+ Coverage   37.71%   37.78%   +0.07%     
==========================================
  Files          56       57       +1     
  Lines        4516     4557      +41     
==========================================
+ Hits         1703     1722      +19     
- Misses       2661     2678      +17     
- Partials      152      157       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[grokspawn]

Respects the legacy ability to encode a release version implicitly in semver build metadata when adding a bundle-level olm.substitutesFor annotation by detecting the presence annotation in the bundle, and extracting the build metadata as a release version.

Before:

./bin/opm render registry.redhat.io/amq7/amq-broker-rhel8-operator-bundle@sha256:ce274a661f64f3b872b69f2c5b6f6c93f841f456037f69b813ede777c944489b -o yaml | yq 'select(.schema == "olm.bundle").properties[]| select(.type == "olm.package")'

type: olm.package
value:
  packageName: amq-broker-rhel8
  version: 7.10.2-opr-2+0.1676475747.p

After:

./bin/opm render registry.redhat.io/amq7/amq-broker-rhel8-operator-bundle@sha256:ce274a661f64f3b872b69f2c5b6f6c93f841f456037f69b813ede777c944489b -o yaml | yq 'select(.schema == "olm.bundle").properties[]| select(.type == "olm.package")'

type: olm.package
value:
  packageName: amq-broker-rhel8
  release: 0.1676475747.p
  version: 7.10.2-opr-2

[grokspawn]

I'm clearing the WIP because I'm hoping to get some reviews/eyes here, but I'm putting on a hold because we want to control the sequence of merges for this and related PRs.

/hold

[grokspawn]

Want to note here that all the off- and on-cluster tooling in o-f and known derivatives is set to unmarshal with tolerance for unknown fields, so the use of this feature with old tooling will result in the field getting dropped/ignored, not a functional impact.

[grokspawn]

/hold cancel

opening the floodgates to reviewers ;)

[rashmigottipati]

Not super clear behind the motivations for this PR, but changes look good overall.

One question I have is: currently I dont think this would reject invalid release strings. What do you think about adding validation to ensure semver correctness?

[jianzhangbjz]

cc
/assign @Xia-Zhao-rh

[joelanford]

We should add more validation here:

• CEL expression that does a regex match to make sure it is a valid prerelease string
• Max length (keeping in mind that we expect the release to be included in the CSV metadata.name, which has its own length restrictions.

Was there any API review on this front? They'd be better at pointing out all the things.

[grokspawn]

This PR was provided as part of the internal enhancement review, yes.
Links are in epic

[grokspawn]

Added some new CEL validations, including a max length of 20 chars. This was a nice, round number which was big enough for all known uses of analogous features (Freshmaker) as well as a minimal-precision date/time-stamp, and should leave ~40 chars to remain valid when composited to a metadata.name with the format <package-name>-v<version>-<release-version>.

Also added to the bundle validators to enforce the case where the author provides a noncompliant name for the release version case.

[joelanford]

Any chance you looked for bundles that have the olm.substitutesFor annotation and checked to see if the overall <package-name>-v<version>-<release-version> format that we would require if they released again using a release field that followed the format of their version build metadata would overflow the metadata.name length requirement of a CSV?

[grokspawn]

I evaluated all available FBCs I could get my hand on, and all existing entries would satisfy the naming requirements for bundles with release versions while continuing to follow their current release version schema use.
https://github.com/grokspawn/operator-registry/blob/validate-freshmaker/cmd/opm/validate-freshmaker/cmd.go is a single-use tool which performed this validation.

[joelanford]

Suggested change

	Release release.OperatorRelease `json:"release,omitzero"`
	Release *release.OperatorRelease `json:"release,omitempty"`

[joelanford]

Make it a pointer and use omitempty instead of omitzero?

[grokspawn]

To solve what problem?
If we make it a pointer, we get the same behavior but now also have to perform nil checking, which isn't necessary with this approach.

[rashmigottipati]

I guess it's a question of whether we want to differentiate between "field not set" which is the nil case vs "field explicitly set but empty"?

[everettraven]

Is the empty string a valid value? What does it mean for this to be set to the empty string vs being omitted entirely?

[grokspawn]

The important thing is the backwards-compatibility aspects of this change. If the value is an instance, protected by the omitzero directive, then it is omitted from the wire-format in the case where it matches a default value for the type (in this case, an empty string).
This eliminates some interactions if the sender and receiver of CSV info have not both adopted the new release feature.
An empty string is a valid value if one wished to express "no release version", as well as the default indication that no release version exists for the CSV.

[grokspawn]

Other than that, this reduces the number of required checks. With a pointer, it is:

1. nil := no release
2. non-nil and empty := no release
3. non-nil and non-empty := has release

with the instance, it's just

1. empty := no release
2. non-empty := has release

For more discussion on the feature, check out this article.

[joelanford]

Is the empty string a valid value? What does it mean for this to be set to the empty string vs being omitted entirely?

I would say both of those would mean "there is no release".

@grokspawn that's a convincing argument.

@everettraven I always forget the API-specific considerations here. Does the somewhat new omitzero change the calculus on this from the API review standpoint? I feel like in the past, we've always been told to use pointers for optional structs.

The type definition here is a bit atypical, which may be a complicating factor. Usually an object has fields that end up being serialized. But here, we are using custom (de)serialization to convert between a string (in the CRD schema to a []semver.PRVersion in the Go API), so that users and developers both get the UX that suits their needs.

We need the wrapper object in order to be able to implement MarshalJSON and UnmarshalJSON

[grokspawn]

omitzero is new in go1.24, so our conventions docs do not address it.

[joelanford]

Do you think we should leak that we are reusing the semver prerelease format (since that's something that might resonate with people), or do you think we should essentially just restate the specifics of what that format is so that we are essentially making it our own and warding off confusion around "is this where I'm supposed to put my semver prerelease" or other questions about whether this should only be used in prerelease scenarios, etc.?

[grokspawn]

I'm torn. I'd like to impress folks in general that we're leveraging semver, and it's not always clear if/when we are. In that context, I think there's value in saying "this additional versioning info also follows semver" (a sub-component of it).
On the other hand, semver is generally viewed as an implementation detail, and we don't generally want to leak those through our interface.

[grokspawn]

I came down on the side of not leaking implementation details in our interfaces. We can always add that detail if we want, but we can't very easily retract it later.
PTAL.

[joelanford]

I would argue that a base image change should require a bump of the version's patch number, not a bump of a release number. I know in the past the substitutesFor logic was specifically built to handle rebuild scenarios. But I think from a user trust standpoint, base image changes are happening generally in order to make security fixes, and to me, that falls squarely in patch territory, not re-release territory.

[grokspawn]

I don't disagree, but that was the way this was used in the past.
I definitely want to make sure we have good use-case capture, and I'm not sure I know them all, but I'll take another swing at it.

[grokspawn]

Took another swing, and it's not an exhaustive list, but I tried to capture likely cases. If I missed any good ones, we can add it here or in a follow-up PR.