Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions api/v1/clusterextension_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,29 @@ type BundleMetadata struct {
// +required
// +kubebuilder:validation:XValidation:rule="self.matches(\"^([0-9]+)(\\\\.[0-9]+)?(\\\\.[0-9]+)?(-([-0-9A-Za-z]+(\\\\.[-0-9A-Za-z]+)*))?(\\\\+([-0-9A-Za-z]+(-\\\\.[-0-9A-Za-z]+)*))?\")",message="version must be well-formed semver"
Version string `json:"version"`

// release is an optional field that identifies a specific release of this bundle's version.
// A release represents a re-publication of the same version, typically used to deliver
// packaging or metadata changes without changing the version number. When multiple
// releases exist for the same version, higher releases are preferred. An unset release
// is less preferred than all other release values.
//
// The value consists of dot-separated identifiers, where each identifier is either a
// numeric value (without leading zeros) or an alphanumeric string (e.g., "2", "1.el9",
// "3.alpha.1"). Releases are compared identifier by identifier: numeric identifiers are
// compared as integers, alphanumeric identifiers are compared lexically, and numeric
// identifiers always sort before alphanumeric identifiers.
//
// For bundles with explicit pkg.Release metadata, this field contains that release value.
// For registry+v1 bundles lacking an explicit release value, this field contains the release
// extracted from version's build metadata (e.g., '2' from '1.0.0+2').
// This field is omitted when the bundle's release value is unset.
//
// +optional
// <opcon:experimental>
// +kubebuilder:validation:MaxLength=20
// +kubebuilder:validation:XValidation:rule="self.matches(\"^$|^(0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*)(\\\\.(0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*))*$\")",message="release must be empty or consist of dot-separated identifiers (numeric without leading zeros, or alphanumeric)"
Release *string `json:"release,omitempty"`
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was planning to do this in a follow-up PR, because I think it ultimately should be based on op-reg Release type instead of op-con Release type.
This is neither.

Do we have any concern if a later PR makes this explicitly declcfg.Release type?

Please align the type+validation with api and my follow-on PR in operator-registry.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless there is meaning between nil and empty, we generally prefer to do

Suggested change
Release *string `json:"release,omitempty"`
Release string `json:"release,omitzero"`

I don't think there's any difference between these cases for this type. If the release is empty or omitted then it equates to no release value.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@grokspawn I was going to converge the type in the follow-up PR as well but based on this review comment , I added Release field to the BundleMetadata struct so we can persist it and roundtrip it properly.
No concern if a later PR makes this a declcfg.Release type though.

I'll align the type+validation with api and your follow-on PR in operator-registry as you suggested.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's any difference between these cases for this type. If the release is empty or omitted then it equates to no release value.

Regarding using omitzero, @joelanford has identified an upgrade edge case when OLMv1 upgrades from a version without the Release field to one with it:
#2543 (comment)

To fix this problem, I think we need to make Release a *string type to distinguish between:

  • Field not present (nil) - parse release from build metadata (legacy registry+v1 behavior for upgrade compatibility)
  • Field present but empty ("") - treat build metadata as actual build metadata, not release

Without this distinction, when OLMv1 reconciles an existing ClusterExtension after upgrade, we can't tell if Version: "1.2.0+1" with no Release field means "parse +1 as release" (old behavior) or "keep +1 as build metadata" (new behavior).

With omitzero on a value type, both cases serialize identically, breaking the upgrade path. The pointer lets us preserve the semantic difference between these two olm.package values:
{"version": "1.2.0+1"} --> {Version: "1.2.0", Release: "1"}
{"version": "1.2.0+1", "release": ""} --> {Version: "1.2.0+1", Release: ""}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validation is now aligned with MaxLength=20.

}

// RevisionStatus defines the observed state of a ClusterObjectSet.
Expand Down
9 changes: 7 additions & 2 deletions api/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

27 changes: 27 additions & 0 deletions applyconfigurations/api/v1/bundlemetadata.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions docs/api-reference/olmv1-api-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ _Appears in:_
| --- | --- | --- | --- |
| `name` _string_ | name is required and follows the DNS subdomain standard as defined in [RFC 1123].<br />It must contain only lowercase alphanumeric characters, hyphens (-) or periods (.),<br />start and end with an alphanumeric character, and be no longer than 253 characters. | | Required: \{\} <br /> |
| `version` _string_ | version is required and references the version that this bundle represents.<br />It follows the semantic versioning standard as defined in https://semver.org/. | | Required: \{\} <br /> |
| `release` _string_ | release is an optional field that identifies a specific release of this bundle's version.<br />A release represents a re-publication of the same version, typically used to deliver<br />packaging or metadata changes without changing the version number. When multiple<br />releases exist for the same version, higher releases are preferred. An unset release<br />is less preferred than all other release values.<br />The value consists of dot-separated identifiers, where each identifier is either a<br />numeric value (without leading zeros) or an alphanumeric string (e.g., "2", "1.el9",<br />"3.alpha.1"). Releases are compared identifier by identifier: numeric identifiers are<br />compared as integers, alphanumeric identifiers are compared lexically, and numeric<br />identifiers always sort before alphanumeric identifiers.<br />For bundles with explicit pkg.Release metadata, this field contains that release value.<br />For registry+v1 bundles lacking an explicit release value, this field contains the release<br />extracted from version's build metadata (e.g., '2' from '1.0.0+2').<br />This field is omitted when the bundle's release value is unset.<br /><opcon:experimental> | | MaxLength: 20 <br />Optional: \{\} <br /> |


#### CRDUpgradeSafetyEnforcement
Expand Down
1 change: 1 addition & 0 deletions helm/experimental.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ options:
- HelmChartSupport
- BoxcutterRuntime
- DeploymentConfig
- BundleReleaseSupport
disabled:
- WebhookProviderOpenshiftServiceCA
# List of enabled experimental features for catalogd
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,30 @@ spec:
hyphens (-) or periods (.), start and end with an alphanumeric
character, and be no longer than 253 characters
rule: self.matches("^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$")
release:
description: |-
release is an optional field that identifies a specific release of this bundle's version.
A release represents a re-publication of the same version, typically used to deliver
packaging or metadata changes without changing the version number. When multiple
releases exist for the same version, higher releases are preferred. An unset release
is less preferred than all other release values.

The value consists of dot-separated identifiers, where each identifier is either a
numeric value (without leading zeros) or an alphanumeric string (e.g., "2", "1.el9",
"3.alpha.1"). Releases are compared identifier by identifier: numeric identifiers are
compared as integers, alphanumeric identifiers are compared lexically, and numeric
identifiers always sort before alphanumeric identifiers.

For bundles with explicit pkg.Release metadata, this field contains that release value.
For registry+v1 bundles lacking an explicit release value, this field contains the release
extracted from version's build metadata (e.g., '2' from '1.0.0+2').
This field is omitted when the bundle's release value is unset.
maxLength: 20
type: string
x-kubernetes-validations:
- message: release must be empty or consist of dot-separated
identifiers (numeric without leading zeros, or alphanumeric)
rule: self.matches("^$|^(0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*)(\\.(0|[1-9][0-9]*|[0-9]*[A-Za-z-][0-9A-Za-z-]*))*$")
version:
description: |-
version is required and references the version that this bundle represents.
Expand Down
13 changes: 13 additions & 0 deletions internal/operator-controller/bundle/versionrelease.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,19 @@ func (vr *VersionRelease) AsLegacyRegistryV1Version() bsemver.Version {

type Release []bsemver.PRVersion

// String returns the string representation of the release.
// Returns an empty string if the release is nil or empty.
func (r Release) String() string {
if len(r) == 0 {
return ""
}
parts := make([]string, len(r))
for i, pr := range r {
parts[i] = pr.String()
}
return strings.Join(parts, ".")
}

// Compare compares two Release values. It returns:
//
// -1 if r < other
Expand Down
99 changes: 82 additions & 17 deletions internal/operator-controller/bundleutil/bundle.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,34 +11,99 @@ 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)
}

// MetadataFor returns a BundleMetadata for the given bundle name and version.
func MetadataFor(bundleName string, bundleVersion bsemver.Version) ocv1.BundleMetadata {
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)
}

// Check if release field is explicitly present in JSON (even if empty).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since Package.Release is defined in operator-registry as string with omitzero, isn't the salient test whether the unmarshaled Package at 28 has a non-empty Release or not?

If we are worried about the scenario where the feature is disabled, but a bundle with a release version is in the catalog, then we can explicitly clear the field.

Is there another scenario that's important?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you agree that we need this distinction for the upgrade path based on the previous review comments, should we update property.Package.Release in operator-registry to *string instead of string with omitzero? That would preserve the field presence distinction at the source and eliminate the need for the helper struct workaround.

// property.Package has Release string, so we can't distinguish "field absent" from "field empty".
// We unmarshal again into a helper struct with Release *string to detect presence.
var releaseField struct {
Release *string `json:"release"`
}
if err := json.Unmarshal(pkgData, &releaseField); err != nil {
return nil, fmt.Errorf("error unmarshalling package release field: %w", err)
}

// When BundleReleaseSupport is enabled and bundle has explicit release field, use it.
if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) && releaseField.Release != nil {
return parseExplicitRelease(pkg.Version, *releaseField.Release)
}
Comment thread
rashmigottipati marked this conversation as resolved.

// Fall back to legacy registry+v1 behavior (release in build metadata)
Comment thread
rashmigottipati marked this conversation as resolved.
//
// TODO: For now, we assume that all bundles are registry+v1 bundles.
// In the future, for supporting other bundle formats, we should not
// use the legacy registry+v1 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
}

// parseExplicitRelease parses version and release from separate fields.
// Build metadata is preserved in the version 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.
func parseExplicitRelease(version, releaseStr string) (*bundle.VersionRelease, error) {
vers, err := bsemver.Parse(version)
if err != nil {
return nil, fmt.Errorf("error parsing version %q: %w", version, err)
}

var rel bundle.Release
if releaseStr == "" {
// Explicit empty release: use empty slice (not nil)
rel = bundle.Release([]bsemver.PRVersion{})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This goes against bundle.NewRelease handling, which returns nil if given an empty string.
I've also aligned with that approach here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my understanding, we need the empty slice (not nil) for roundtrip correctness.

If we use nil for explicit empty release, then in MetadataFor, the check if vr.Release != nil fails and we lose the Release field entirely. This breaks the distinction between "Release field not present" (legacy) vs "Release field explicitly empty".

The three-state representation is:

  • nil → field not present (parse from build metadata)
  • []bsemver.PRVersion{} → field explicitly empty
  • []bsemver.PRVersion{...} → field with value

Without this, we can't preserve the upgrade path semantics mentioned in this comment

Pls correct me if I am missing something here.

} else {
rel, err = bundle.NewRelease(releaseStr)
if err != nil {
return nil, fmt.Errorf("error parsing release %q: %w", releaseStr, err)
}
}

return &bundle.VersionRelease{
Version: vers,
Release: rel,
}, nil
}

// MetadataFor returns a BundleMetadata for the given bundle name and version/release.
func MetadataFor(bundleName string, vr bundle.VersionRelease) ocv1.BundleMetadata {
if features.OperatorControllerFeatureGate.Enabled(features.BundleReleaseSupport) {
// New behavior: separate Version and Release fields
bm := ocv1.BundleMetadata{
Name: bundleName,
Version: vr.Version.String(),
}
if vr.Release != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like we could skip it if we we move to omitempty instead of a pointer with omitzero, and furthermore could be omitted if we're using an explicit type instead of a string.
(I don't think we can use an explicit type yet, unless the bundle.Release supports marshaling/unmarshaling.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I am thinking that when opreg PR merges and we do a follow-up PR in opcon to do the type convergence, we can then use bundle.Release directly instead of *string, since opreg PR already uses the explicit type with JSON marshaling support. Let's align with that in the follow-up PR?

relStr := vr.Release.String()
bm.Release = &relStr
}
return bm
}
// Old behavior for backward compatibility: reconstitute build metadata in Version field
// This preserves release information (e.g., "1.0.0+2") for standard CRD users where
// the Release field is pruned by the API server.
return ocv1.BundleMetadata{
Name: bundleName,
Version: bundleVersion.String(),
Version: vr.AsLegacyRegistryV1Version().String(),
}
}
Loading
Loading