diff --git a/helm/experimental.yaml b/helm/experimental.yaml index b158389d48..05bb45f4d8 100644 --- a/helm/experimental.yaml +++ b/helm/experimental.yaml @@ -14,6 +14,7 @@ options: - HelmChartSupport - BoxcutterRuntime - DeploymentConfig + - BundleReleaseSupport disabled: - WebhookProviderOpenshiftServiceCA # List of enabled experimental features for catalogd diff --git a/internal/operator-controller/bundleutil/bundle.go b/internal/operator-controller/bundleutil/bundle.go index 2771c52593..cb8a1f78dd 100644 --- a/internal/operator-controller/bundleutil/bundle.go +++ b/internal/operator-controller/bundleutil/bundle.go @@ -11,30 +11,52 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" + "github.com/operator-framework/operator-controller/internal/operator-controller/features" ) func GetVersionAndRelease(b declcfg.Bundle) (*bundle.VersionRelease, error) { for _, p := range b.Properties { if p.Type == property.TypePackage { - var pkg property.Package - if err := json.Unmarshal(p.Value, &pkg); err != nil { - return nil, fmt.Errorf("error unmarshalling package property: %w", err) - } - - // TODO: For now, we assume that all bundles are registry+v1 bundles. - // In the future, when we support other bundle formats, we should stop - // using the legacy mechanism (i.e. using build metadata in the version) - // to determine the bundle's release. - vr, err := bundle.NewLegacyRegistryV1VersionRelease(pkg.Version) - if err != nil { - return nil, err - } - return vr, nil + return parseVersionRelease(p.Value) } } return nil, fmt.Errorf("no package property found in bundle %q", b.Name) } +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 + } + + // Fall back to legacy registry+v1 behavior (release in build metadata) + vr, err := bundle.NewLegacyRegistryV1VersionRelease(pkg.Version) + if err != nil { + return nil, err + } + return vr, nil +} + // MetadataFor returns a BundleMetadata for the given bundle name and version. func MetadataFor(bundleName string, bundleVersion bsemver.Version) ocv1.BundleMetadata { return ocv1.BundleMetadata{ diff --git a/internal/operator-controller/bundleutil/bundle_test.go b/internal/operator-controller/bundleutil/bundle_test.go index 1cdabad054..5c541e7d2d 100644 --- a/internal/operator-controller/bundleutil/bundle_test.go +++ b/internal/operator-controller/bundleutil/bundle_test.go @@ -2,6 +2,7 @@ package bundleutil_test import ( "encoding/json" + "fmt" "testing" bsemver "github.com/blang/semver/v4" @@ -12,6 +13,7 @@ import ( "github.com/operator-framework/operator-controller/internal/operator-controller/bundle" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" + "github.com/operator-framework/operator-controller/internal/operator-controller/features" ) func TestGetVersionAndRelease(t *testing.T) { @@ -83,12 +85,121 @@ func TestGetVersionAndRelease(t *testing.T) { Properties: properties, } - _, err := bundleutil.GetVersionAndRelease(bundle) + actual, err := bundleutil.GetVersionAndRelease(bundle) if tc.wantErr { require.Error(t, err) } else { require.NoError(t, err) + require.Equal(t, tc.wantVersionRelease, actual) } }) } } + +// TestGetVersionAndRelease_WithBundleReleaseSupport tests the feature-gated parsing behavior. +// Explicitly sets the gate to test both enabled and disabled paths. +func TestGetVersionAndRelease_WithBundleReleaseSupport(t *testing.T) { + t.Run("gate enabled - parses explicit release field", func(t *testing.T) { + // Enable the feature gate for this test + prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) + require.NoError(t, features.OperatorControllerFeatureGate.Set("BundleReleaseSupport=true")) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("BundleReleaseSupport=%t", prevEnabled))) + }) + + 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, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + bundle := declcfg.Bundle{ + Name: "test-bundle", + Properties: []property.Property{*tc.pkgProperty}, + } + + actual, err := bundleutil.GetVersionAndRelease(bundle) + if tc.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + require.Equal(t, tc.wantVersionRelease, actual) + } + }) + } + }) + + t.Run("gate disabled - ignores explicit release field, uses build metadata", func(t *testing.T) { + // Disable the feature gate for this test + prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) + require.NoError(t, features.OperatorControllerFeatureGate.Set("BundleReleaseSupport=false")) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("BundleReleaseSupport=%t", prevEnabled))) + }) + + // When gate disabled, explicit release field is ignored and parsing falls back to legacy behavior + bundleWithExplicitRelease := declcfg.Bundle{ + Name: "test-bundle", + Properties: []property.Property{ + { + Type: property.TypePackage, + Value: json.RawMessage(`{"version": "1.0.0+2", "release": "999"}`), + }, + }, + } + + actual, err := bundleutil.GetVersionAndRelease(bundleWithExplicitRelease) + require.NoError(t, err) + + // Should parse build metadata (+2), not explicit release field (999) + expected := &bundle.VersionRelease{ + Version: bsemver.MustParse("1.0.0"), + Release: bundle.Release([]bsemver.PRVersion{ + {VersionNum: 2, IsNum: true}, + }), + } + require.Equal(t, expected, actual) + }) +} diff --git a/internal/operator-controller/catalogmetadata/compare/compare.go b/internal/operator-controller/catalogmetadata/compare/compare.go index decab58953..c71abd45ce 100644 --- a/internal/operator-controller/catalogmetadata/compare/compare.go +++ b/internal/operator-controller/catalogmetadata/compare/compare.go @@ -10,6 +10,7 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" "github.com/operator-framework/operator-controller/internal/operator-controller/bundleutil" + "github.com/operator-framework/operator-controller/internal/operator-controller/features" slicesutil "github.com/operator-framework/operator-controller/internal/shared/util/slices" ) @@ -39,9 +40,27 @@ func newMastermindsRange(versionRange string) (bsemver.Range, error) { } // ByVersionAndRelease is a comparison function that compares bundles by -// version and release. Bundles with lower versions/releases are -// considered less than bundles with higher versions/releases. +// version and release. When the BundleReleaseSupport feature gate is +// enabled, it uses Bundle.Compare() (which reads pkg.Release from olm.package) +// and falls back to build metadata comparison if equal. When disabled, it uses +// version+release from build metadata (backward compatible). func ByVersionAndRelease(b1, b2 declcfg.Bundle) int { + if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) { + // Use CompositeVersion comparison (reads pkg.Release from olm.package). + // Note: Bundle.Compare() returns 0 in three cases: + // 1. Bundle names are identical + // 2. Versions and releases are equal + // 3. Parsing errors occur (silently swallowed) + // Catalog validation should catch malformed versions before they reach this code. + result := b2.Compare(&b1) + if result != 0 { + return result + } + // If CompositeVersion comparison is equal, fall back to version+release comparison + return compareByVersionRelease(b1, b2) + } + + // Default: use version+release from build metadata (backward compatible) vr1, err1 := bundleutil.GetVersionAndRelease(b1) vr2, err2 := bundleutil.GetVersionAndRelease(b2) @@ -54,6 +73,20 @@ func ByVersionAndRelease(b1, b2 declcfg.Bundle) int { return vr2.Compare(*vr1) } +// compareByVersionRelease compares bundles using GetVersionAndRelease, which +// returns version+release from either explicit pkg.Release field (when feature gate +// enabled and field present) or build metadata (legacy registry+v1 fallback). +// This is used as a fallback when CompositeVersion comparison returns equal. +func compareByVersionRelease(b1, b2 declcfg.Bundle) int { + vr1, err1 := bundleutil.GetVersionAndRelease(b1) + vr2, err2 := bundleutil.GetVersionAndRelease(b2) + + if err1 != nil || err2 != nil { + return compareErrors(err2, err1) + } + return vr2.Compare(*vr1) +} + func ByDeprecationFunc(deprecation declcfg.Deprecation) func(a, b declcfg.Bundle) int { deprecatedBundles := sets.New[string]() for _, entry := range deprecation.Entries { diff --git a/internal/operator-controller/catalogmetadata/compare/compare_test.go b/internal/operator-controller/catalogmetadata/compare/compare_test.go index 25557a9495..c9cc414ca2 100644 --- a/internal/operator-controller/catalogmetadata/compare/compare_test.go +++ b/internal/operator-controller/catalogmetadata/compare/compare_test.go @@ -2,6 +2,7 @@ package compare_test import ( "encoding/json" + "fmt" "slices" "testing" @@ -13,6 +14,7 @@ import ( "github.com/operator-framework/operator-registry/alpha/property" "github.com/operator-framework/operator-controller/internal/operator-controller/catalogmetadata/compare" + "github.com/operator-framework/operator-controller/internal/operator-controller/features" ) func TestNewVersionRange(t *testing.T) { @@ -168,3 +170,75 @@ func TestByDeprecationFunc(t *testing.T) { assert.Equal(t, 0, byDeprecation(c, d)) assert.Equal(t, 0, byDeprecation(d, c)) } + +// TestByVersionAndRelease_WithBundleReleaseSupport tests the feature-gated hybrid comparison. +// Explicitly sets the gate to test both enabled and disabled paths. +func TestByVersionAndRelease_WithBundleReleaseSupport(t *testing.T) { + t.Run("gate enabled - uses Bundle.Compare with build metadata fallback", func(t *testing.T) { + // Enable the feature gate for this test + prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) + require.NoError(t, features.OperatorControllerFeatureGate.Set("BundleReleaseSupport=true")) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("BundleReleaseSupport=%t", prevEnabled))) + }) + + // Registry+v1 bundles: same version, different build metadata + b1 := declcfg.Bundle{ + Name: "package1.v1.0.0+1", + Properties: []property.Property{ + {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0+1"}`)}, + }, + } + b2 := declcfg.Bundle{ + Name: "package1.v1.0.0+2", + Properties: []property.Property{ + {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0+2"}`)}, + }, + } + + result := compare.ByVersionAndRelease(b1, b2) + assert.Positive(t, result, "Bundle.Compare() returns 0 for registry+v1, fallback to build metadata: 1.0.0+2 > 1.0.0+1") + + // Test bundles with explicit pkg.Release field + explicitR1 := declcfg.Bundle{ + Name: "pkg.v2.0.0-r1", + Properties: []property.Property{ + {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "pkg", "version": "2.0.0", "release": "1"}`)}, + }, + } + explicitR2 := declcfg.Bundle{ + Name: "pkg.v2.0.0-r2", + Properties: []property.Property{ + {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "pkg", "version": "2.0.0", "release": "2"}`)}, + }, + } + result = compare.ByVersionAndRelease(explicitR1, explicitR2) + assert.Positive(t, result, "2.0.0+release.2 > 2.0.0+release.1 (explicit release field)") + }) + + t.Run("gate disabled - uses legacy build metadata comparison only", func(t *testing.T) { + // Disable the feature gate for this test + prevEnabled := features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) + require.NoError(t, features.OperatorControllerFeatureGate.Set("BundleReleaseSupport=false")) + t.Cleanup(func() { + require.NoError(t, features.OperatorControllerFeatureGate.Set(fmt.Sprintf("BundleReleaseSupport=%t", prevEnabled))) + }) + + // Registry+v1 bundles: same version, different build metadata + b1 := declcfg.Bundle{ + Name: "package1.v1.0.0+1", + Properties: []property.Property{ + {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0+1"}`)}, + }, + } + b2 := declcfg.Bundle{ + Name: "package1.v1.0.0+2", + Properties: []property.Property{ + {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "package1", "version": "1.0.0+2"}`)}, + }, + } + + result := compare.ByVersionAndRelease(b1, b2) + assert.Positive(t, result, "should sort by build metadata: 1.0.0+2 > 1.0.0+1") + }) +} diff --git a/internal/operator-controller/catalogmetadata/filter/successors.go b/internal/operator-controller/catalogmetadata/filter/successors.go index 975c8cb39f..8e1a1df370 100644 --- a/internal/operator-controller/catalogmetadata/filter/successors.go +++ b/internal/operator-controller/catalogmetadata/filter/successors.go @@ -20,7 +20,7 @@ func SuccessorsOf(installedBundle ocv1.BundleMetadata, channels ...declcfg.Chann // registry+v1 and embed release in build metadata. installedVersionRelease, err := bundle.NewLegacyRegistryV1VersionRelease(installedBundle.Version) if err != nil { - return nil, fmt.Errorf("failed to get version and release of installed bundle: %v", err) + return nil, fmt.Errorf("failed to get version and release of installed bundle: %w", err) } successorsPredicate, err := legacySuccessor(installedBundle, channels...) @@ -28,11 +28,14 @@ func SuccessorsOf(installedBundle ocv1.BundleMetadata, channels ...declcfg.Chann return nil, fmt.Errorf("getting successorsPredicate: %w", err) } - // We need either successors or current version (no upgrade) - return filter.Or( + // Bundle matches if it's a channel successor or current version (no upgrade). + // Re-releases must have explicit channel entries to be valid successors. + predicates := []filter.Predicate[declcfg.Bundle]{ successorsPredicate, ExactVersionRelease(*installedVersionRelease), - ), nil + } + + return filter.Or(predicates...), nil } func legacySuccessor(installedBundle ocv1.BundleMetadata, channels ...declcfg.Channel) (filter.Predicate[declcfg.Bundle], error) { diff --git a/internal/operator-controller/features/features.go b/internal/operator-controller/features/features.go index 0f99c1b28e..3af7079fb1 100644 --- a/internal/operator-controller/features/features.go +++ b/internal/operator-controller/features/features.go @@ -19,6 +19,7 @@ const ( HelmChartSupport featuregate.Feature = "HelmChartSupport" BoxcutterRuntime featuregate.Feature = "BoxcutterRuntime" DeploymentConfig featuregate.Feature = "DeploymentConfig" + BundleReleaseSupport featuregate.Feature = "BundleReleaseSupport" ) var operatorControllerFeatureGates = map[featuregate.Feature]featuregate.FeatureSpec{ @@ -89,6 +90,17 @@ var operatorControllerFeatureGates = map[featuregate.Feature]featuregate.Feature PreRelease: featuregate.Alpha, LockToDefault: false, }, + + // BundleReleaseSupport enables support for the explicit pkg.Release field + // in the olm.package property, allowing bundles to use semantic release + // versioning. When enabled, bundle comparison and successor filtering prefer + // the explicit release field over build metadata. This supports both new + // bundle formats (with explicit release) and registry+v1 bundles (with build metadata). + BundleReleaseSupport: { + Default: false, + PreRelease: featuregate.Alpha, + LockToDefault: false, + }, } var OperatorControllerFeatureGate featuregate.MutableFeatureGate = featuregate.NewFeatureGate() diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index 4d44532303..9e3303b28b 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -2759,6 +2759,7 @@ spec: - --feature-gates=HelmChartSupport=true - --feature-gates=BoxcutterRuntime=true - --feature-gates=DeploymentConfig=true + - --feature-gates=BundleReleaseSupport=true - --feature-gates=WebhookProviderOpenshiftServiceCA=false - --tls-cert=/var/certs/tls.crt - --tls-key=/var/certs/tls.key diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 7f5d7c53a8..6b750e34eb 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -2665,6 +2665,7 @@ spec: - --feature-gates=HelmChartSupport=true - --feature-gates=BoxcutterRuntime=true - --feature-gates=DeploymentConfig=true + - --feature-gates=BundleReleaseSupport=true - --feature-gates=WebhookProviderOpenshiftServiceCA=false - --tls-cert=/var/certs/tls.crt - --tls-key=/var/certs/tls.key