✨ Add support for parsing and persisting explicit pkg.Release field#2543
✨ Add support for parsing and persisting explicit pkg.Release field#2543rashmigottipati wants to merge 2 commits intooperator-framework:mainfrom
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR introduces an alpha feature gate to allow OLMv1 upgrade resolution to treat “re-released” bundles (same semver, higher release/build value like 2.0.0+1 -> 2.0.0+2) as valid successors when explicitly enabled.
Changes:
- Added
ReleaseVersionPriorityfeature gate (Alpha, default disabled). - Added
SameVersionHigherRelease()predicate and integrated it intoSuccessorsOf()behind the feature gate. - Added unit tests for
SameVersionHigherRelease()and forSuccessorsOf()with the gate disabled.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/operator-controller/features/features.go | Defines the new ReleaseVersionPriority feature gate and its spec. |
| internal/operator-controller/catalogmetadata/filter/successors.go | Conditionally expands successor matching to include same-version/higher-release bundles when gated on. |
| internal/operator-controller/catalogmetadata/filter/bundle_predicates.go | Adds the SameVersionHigherRelease() predicate. |
| internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go | Adds predicate unit tests including edge cases. |
| internal/operator-controller/catalogmetadata/filter/successors_test.go | Adds a regression test to ensure default (gate-off) behavior does not accept higher-release bundles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/successors_test.go
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2543 +/- ##
==========================================
+ Coverage 68.91% 68.96% +0.05%
==========================================
Files 139 140 +1
Lines 9863 9961 +98
==========================================
+ Hits 6797 6870 +73
- Misses 2559 2575 +16
- Partials 507 516 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/catalogmetadata/filter/successors_test.go
Outdated
Show resolved
Hide resolved
|
If I understand this correctly, it looks like this introduces the new behavior that an explicit upgrade edge doesn't actually have to exist in the catalog to upgrade to a bundle that has the same version and a higher release. Is that the intent? (If so let's make that clear in the PR description). Is it also the intent that we'll inherit the upgrade edges of the first release of the version? |
internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/bundle_predicates.go
Outdated
Show resolved
Hide resolved
|
#2273 makes the claim that future bundle types will drop this approach, not that registry+v1 bundles should. So that's my bad. |
Sadly, we have to rely on the presence of existing graph edges or we break assumptions users have with the registry+v1 bundle format, coming from v0. So even though we have the ability to prefer version+release over version, since replaces/skips use named bundles instead of bundle versions we have to support the older behavior. |
That would be a regression of a bug fix that #2273 made though, right?
Yeah, agreed. I reached the same conclusion after thinking about this more. So if I understand correctly now:
So the change we need now is "look for |
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
5681488 to
8dcab49
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/bundle_predicates_test.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
|
Adjusted my comments. I missed the part of the #2273 description when it said that we wouldn't divert from semver ordering for any new bundle formats. |
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
a2e5062 to
1e9cded
Compare
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
d7d824c to
aa0a0dc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/operator-controller/controllers/clusterextension_reconcile_steps.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/controllers/boxcutter_reconcile_steps.go
Outdated
Show resolved
Hide resolved
internal/operator-controller/catalogmetadata/filter/successors.go
Outdated
Show resolved
Hide resolved
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
aa0a0dc to
0ba7072
Compare
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
0ba7072 to
630c47f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
630c47f to
4519cc6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Version: vr.Version.String(), | ||
| } | ||
| relStr := vr.Release.String() | ||
| bm.Release = &relStr |
| releaseValue := "" | ||
| if state.resolvedRevisionMetadata.Release != nil { | ||
| releaseValue = *state.resolvedRevisionMetadata.Release | ||
| } | ||
| revisionAnnotations := map[string]string{ | ||
| labels.BundleNameKey: state.resolvedRevisionMetadata.Name, | ||
| labels.PackageNameKey: state.resolvedRevisionMetadata.Package, | ||
| labels.BundleVersionKey: state.resolvedRevisionMetadata.Version, | ||
| labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image, | ||
| labels.BundleReleaseKey: releaseValue, | ||
| } |
| releaseValue := "" | ||
| if state.resolvedRevisionMetadata.Release != nil { | ||
| releaseValue = *state.resolvedRevisionMetadata.Release | ||
| } | ||
| revisionAnnotations := map[string]string{ | ||
| labels.BundleNameKey: state.resolvedRevisionMetadata.Name, | ||
| labels.PackageNameKey: state.resolvedRevisionMetadata.Package, | ||
| labels.BundleVersionKey: state.resolvedRevisionMetadata.Version, | ||
| labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image, | ||
| labels.BundleReleaseKey: releaseValue, | ||
| } |
| require.NotNil(t, result.Release) | ||
| require.Empty(t, *result.Release) |
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
a833002 to
3b933d4
Compare
Signed-off-by: Rashmi Gottipati <rgottipa@redhat.com>
rashmigottipati
force-pushed
the
oprun-4277-implementation
branch
from
3b933d4 to
d13ce71
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| releaseValue := "" | ||
| if state.resolvedRevisionMetadata.Release != nil { | ||
| releaseValue = *state.resolvedRevisionMetadata.Release | ||
| } | ||
| revisionAnnotations := map[string]string{ | ||
| labels.BundleNameKey: state.resolvedRevisionMetadata.Name, | ||
| labels.PackageNameKey: state.resolvedRevisionMetadata.Package, | ||
| labels.BundleVersionKey: state.resolvedRevisionMetadata.Version, | ||
| labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image, | ||
| labels.BundleReleaseKey: releaseValue, | ||
| } |
| releaseValue := "" | ||
| if state.resolvedRevisionMetadata.Release != nil { | ||
| releaseValue = *state.resolvedRevisionMetadata.Release | ||
| } | ||
| revisionAnnotations := map[string]string{ | ||
| labels.BundleNameKey: state.resolvedRevisionMetadata.Name, | ||
| labels.PackageNameKey: state.resolvedRevisionMetadata.Package, | ||
| labels.BundleVersionKey: state.resolvedRevisionMetadata.Version, | ||
| labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image, | ||
| labels.BundleReleaseKey: releaseValue, | ||
| } |
| func parseVersionRelease(pkgData json.RawMessage) (*bundle.VersionRelease, error) { | ||
| var pkg property.Package | ||
| if err := json.Unmarshal(pkgData, &pkg); err != nil { | ||
| return nil, fmt.Errorf("error unmarshalling package property: %w", err) | ||
| } | ||
|
|
||
| // When BundleReleaseSupport is enabled and bundle has explicit release field, use it. | ||
| // Note: Build metadata is preserved here because with an explicit release field, | ||
| // build metadata serves its proper semver purpose (e.g., git commit, build number). | ||
| // In contrast, NewLegacyRegistryV1VersionRelease clears build metadata because it | ||
| // interprets build metadata AS the release value for registry+v1 bundles. | ||
| if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) && pkg.Release != "" { | ||
| vers, err := bsemver.Parse(pkg.Version) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing version %q: %w", pkg.Version, err) | ||
| } | ||
| rel, err := bundle.NewRelease(pkg.Release) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing release %q: %w", pkg.Release, err) | ||
| } | ||
| return &bundle.VersionRelease{ | ||
| Version: vers, | ||
| Release: rel, | ||
| }, nil | ||
| } |
| tests := []struct { | ||
| name string | ||
| pkgProperty *property.Property | ||
| wantVersionRelease *bundle.VersionRelease | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "explicit release field - takes precedence over build metadata", | ||
| pkgProperty: &property.Property{ | ||
| Type: property.TypePackage, | ||
| Value: json.RawMessage(`{"version": "1.0.0+ignored", "release": "2"}`), | ||
| }, | ||
| wantVersionRelease: &bundle.VersionRelease{ | ||
| Version: bsemver.MustParse("1.0.0+ignored"), // Build metadata preserved - serves its proper semver purpose | ||
| Release: bundle.Release([]bsemver.PRVersion{ | ||
| {VersionNum: 2, IsNum: true}, | ||
| }), | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "explicit release field - complex release", | ||
| pkgProperty: &property.Property{ | ||
| Type: property.TypePackage, | ||
| Value: json.RawMessage(`{"version": "2.1.0", "release": "1.alpha.3"}`), | ||
| }, | ||
| wantVersionRelease: &bundle.VersionRelease{ | ||
| Version: bsemver.MustParse("2.1.0"), | ||
| Release: bundle.Release([]bsemver.PRVersion{ | ||
| {VersionNum: 1, IsNum: true}, | ||
| {VersionStr: "alpha"}, | ||
| {VersionNum: 3, IsNum: true}, | ||
| }), | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "explicit release field - invalid release", | ||
| pkgProperty: &property.Property{ | ||
| Type: property.TypePackage, | ||
| Value: json.RawMessage(`{"version": "1.0.0", "release": "001"}`), | ||
| }, | ||
| wantErr: true, | ||
| }, | ||
| } |
pedjak
left a comment
pedjak
left a comment
There was a problem hiding this comment.
Deep Review: Roundtrip Correctness and Ungated API Change
The core design (separating release from version, feature-gating the new parsing path) is sound. The CRD field documentation, validation regex, and deep-copy generation all look correct.
However, I traced the full data lifecycle through all paths (resolve → MetadataFor → annotations → GetRevisionStates → SuccessorsOf → ExactVersionRelease) and found two issues that need attention before merge:
-
Ungated Version field change — The
MetadataForsignature change strips build metadata from theVersionfield for all users, not just those withBundleReleaseSupportenabled. Standard CRD users lose release information entirely (thereleasefield is pruned by the API server). -
Roundtrip correctness bug during controller upgrade — Writing
BundleReleaseKeyunconditionally creates a nil-vs-empty-string Release ambiguity that causesExactVersionReleaseto fail matching the installed bundle against itself in the catalog.
Both issues and suggested fixes are detailed in the inline comments below.
| // MetadataFor accepts VersionRelease and populates both Version and Release fields. | ||
| // - With BundleReleaseSupport enabled + explicit pkg.Release: Release from pkg.Release field | ||
| // - Registry+v1 bundles (e.g., "1.0.0+2"): Release extracted from build metadata (e.g., "2") | ||
| // - Both result in separate Version and Release fields in BundleMetadata for roundtripping | ||
| BundleMetadata: bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion), |
There was a problem hiding this comment.
Ungated behavioral change to the Version field.
This changes MetadataFor from using AsLegacyRegistryV1Version() (which reconstituted build metadata, e.g. "1.0.0+2") to using vr.Version.String() (which has build metadata already stripped by NewLegacyRegistryV1VersionRelease at versionrelease.go:44). This runs for all users regardless of the BundleReleaseSupport gate.
For standard CRD users (where the release field is pruned by the API server):
- Before:
status.install.bundle.version: "1.0.0+2" - After:
status.install.bundle.version: "1.0.0"(release information completely lost)
This also triggers a one-time re-apply for every installed bundle with build metadata on controller upgrade, because bundleUnchanged at line 377 compares "1.0.0" != "1.0.0+2".
Suggestion: When BundleReleaseSupport is disabled, preserve the old behavior:
if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) {
BundleMetadata: bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion),
} else {
BundleMetadata: bundleutil.MetadataFor(resolvedBundle.Name, resolvedBundleVersion.AsLegacyRegistryV1Version()),
}Or keep the old MetadataFor(string, bsemver.Version) signature for the ungated path.
| releaseValue := "" | ||
| if state.resolvedRevisionMetadata.Release != nil { | ||
| releaseValue = *state.resolvedRevisionMetadata.Release | ||
| } | ||
| revisionAnnotations := map[string]string{ | ||
| labels.BundleNameKey: state.resolvedRevisionMetadata.Name, | ||
| labels.PackageNameKey: state.resolvedRevisionMetadata.Package, | ||
| labels.BundleVersionKey: state.resolvedRevisionMetadata.Version, | ||
| labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image, | ||
| labels.BundleReleaseKey: releaseValue, |
There was a problem hiding this comment.
Unconditional BundleReleaseKey write causes a roundtrip correctness bug during controller upgrade.
When Release is nil (old install, no release annotation), releaseValue defaults to "" and BundleReleaseKey is written as an empty-string annotation. On next read at GetRevisionStates, the annotation key exists → Release = ptr("") (non-nil). This changes the code path taken in SuccessorsOf from legacy (correct) to explicit (broken for this case).
Full failure scenario:
- Old controller installed
"1.0.0+2"→ annotationBundleVersionKey="1.0.0+2", noBundleReleaseKey - New controller starts, catalogs temporarily unavailable → fallback at line 299 uses installed metadata
- Apply writes:
BundleReleaseKey=""(this line) - Next reconcile reads:
Version="1.0.0+2",Release=ptr("") SuccessorsOf:Release != nil→ explicit path →bsemver.Parse("1.0.0+2")=1.0.0+2,NewRelease("")=nil- →
installedVersionRelease = {Version: 1.0.0+2, Release: nil} - Catalog parses same bundle via
GetVersionAndRelease→{Version: 1.0.0, Release: [2]} ExactVersionReleasecompares:Version.Comparereturns 0 (build metadata ignored per semver), butRelease.Compare(nil, [2])returns -1- Result: installed bundle cannot match itself → resolution may fail if no explicit upgrade edge exists
Fix: Only write BundleReleaseKey when Release is non-nil:
if state.resolvedRevisionMetadata.Release != nil {
revisionAnnotations[labels.BundleReleaseKey] = *state.resolvedRevisionMetadata.Release
}| releaseValue := "" | ||
| if state.resolvedRevisionMetadata.Release != nil { | ||
| releaseValue = *state.resolvedRevisionMetadata.Release | ||
| } | ||
| revisionAnnotations := map[string]string{ | ||
| labels.BundleNameKey: state.resolvedRevisionMetadata.Name, | ||
| labels.PackageNameKey: state.resolvedRevisionMetadata.Package, | ||
| labels.BundleVersionKey: state.resolvedRevisionMetadata.Version, | ||
| labels.BundleReferenceKey: state.resolvedRevisionMetadata.Image, | ||
| labels.BundleReleaseKey: releaseValue, |
There was a problem hiding this comment.
Same unconditional BundleReleaseKey write issue as in ApplyBundle — see the detailed roundtrip failure scenario in my comment there. The fix is the same: only add BundleReleaseKey to the annotations map when Release is non-nil.
| if installedBundle.Release != nil { | ||
| // Bundle has explicit release field (or explicitly empty) - parse version and release from separate fields. | ||
| // Note: We can't use NewLegacyRegistryV1VersionRelease here because the version might | ||
| // already contain build metadata (e.g., "1.0.0+git.abc"), which serves its proper | ||
| // semver purpose when using explicit pkg.Release. Concatenating would create invalid | ||
| // semver like "1.0.0+git.abc+2". | ||
| version, err := bsemver.Parse(installedBundle.Version) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse installed bundle version %q: %w", installedBundle.Version, err) | ||
| } | ||
| release, err := bundle.NewRelease(*installedBundle.Release) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse installed bundle release %q: %w", *installedBundle.Release, err) | ||
| } | ||
| installedVersionRelease = &bundle.VersionRelease{ | ||
| Version: version, | ||
| Release: release, | ||
| } | ||
| } else { | ||
| // Legacy registry+v1: release embedded in version's build metadata | ||
| installedVersionRelease, err = bundle.NewLegacyRegistryV1VersionRelease(installedBundle.Version) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to get version and release of installed bundle: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
The branching on Release != nil is correct in steady state, but creates a roundtrip inconsistency during controller upgrade when BundleReleaseKey is written unconditionally (see my comment on ApplyBundle).
When old-format annotations (Version="1.0.0+2", no release annotation) go through a fallback apply and get BundleReleaseKey="" added, the next read produces Release=ptr(""). This branch then fires, parsing version "1.0.0+2" with NewRelease("") = nil, yielding {Version: 1.0.0+2, Release: nil}. Meanwhile, the catalog parses the same bundle as {Version: 1.0.0, Release: [2]}. These don't compare equal because Release nil != [2], even though Version.Compare returns 0 (blang ignores build metadata per semver spec).
This is fixed by not writing BundleReleaseKey unconditionally (see my other comments). But it may also be worth adding a defensive guard here: if the version contains build metadata and release is empty, fall through to the legacy path, since that's the scenario this explicit path can't handle correctly.
Description
Summary
Adds support for parsing the explicit
pkg.Releasefield from bundle metadata and persisting it through cluster storage (roundtripping). This is gated by the newBundleReleaseSupportalpha feature gate (disabled by default).Changes
BundleReleaseSupportalpha feature gate (disabled by default)Releasefield toBundleMetadataCRD typeBundleReleaseKeylabel constant for annotation storagebundleutil.parseVersionRelease()to: Parse explicitpkg.Releasewhen feature gate enabled and field present. Fall back to legacy registry+v1 behavior (release in build metadata) otherwisebundleutil.MetadataFor()to serialize release value fromVersionReleaseFeature Gate Behavior
When enabled:
pkg.Releasefield: release parsed separately, build metadata preserved for proper semver purposeWhen feature gate disabled:
pkg.Releasefield ignoredReviewer Checklist
Link to Github Issue: #2495
Epic: #2479