Skip to content

OTA-1627: pkg/cincinnati: Centralize release metadata parsing #1231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+85 −61
Open

OTA-1627: pkg/cincinnati: Centralize release metadata parsing #1231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a077100
pkg/cincinnati: Centralize release metadata parsing
wking Sep 17, 2025
9186228
pkg/cvo: Log failures to merge release metadata
wking Oct 3, 2025
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
82 changes: 60 additions & 22 deletions pkg/cincinnati/cincinnati.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package cincinnati
import (
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
Expand Down Expand Up @@ -289,9 +290,9 @@ type graph struct {
}

type node struct {
Version semver.Version `json:"version"`
Image string `json:"payload"`
Metadata map[string]string `json:"metadata,omitempty"`
Version semver.Version `json:"version"`
Image string `json:"payload"`
Metadata map[string]interface{} `json:"metadata,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/openshift/cincinnati/blob/master/docs/design/cincinnati.md#update-graph

The doc indeed says "arbitrary", and thus could be non-string.

}

type edge struct {
Expand Down Expand Up @@ -329,28 +330,65 @@ func (e *edge) UnmarshalJSON(data []byte) error {
}

func convertRetrievedUpdateToRelease(update node) (configv1.Release, error) {
cvoUpdate := configv1.Release{
Version: update.Version.String(),
Image: update.Image,
}
if urlString, ok := update.Metadata["url"]; ok {
_, err := url.Parse(urlString)
if err != nil {
return cvoUpdate, fmt.Errorf("invalid URL for %s: %s", cvoUpdate.Version, err)
release, err := ParseMetadata(update.Metadata)
release.Version = update.Version.String()
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Maybe this is my thing.
I feel odd to use a return without checking error.
I mean if err!=nil I would construct a new object to ensure other fields are good.

Copy link
Member Author

Choose a reason for hiding this comment

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

But in ParseMetadata, I'm trying to fill in as much as I can while aggregating errors. E.g. if I can't parse the URL, I'll still continue on to parse channels. And none of the metadata is critical to being able to use the release image, so I'm just passing along err in case a caller is aiming for perfection, while also passing along the as-good-as-we-could-make-it-even-if-it-isn't-perfect release in case they're willing to settle for the amount we were able to salvage.

release.Image = update.Image
return release, err
}

// ParseMetadata parses release metadata (URL, channels, etc.). It does not populate the version or image properties.
func ParseMetadata(metadata map[string]interface{}) (configv1.Release, error) {
release := configv1.Release{}
errs := []error{}
if urlInterface, hasURL := metadata["url"]; hasURL {
if urlString, isString := urlInterface.(string); isString {
if _, err := url.Parse(urlString); err == nil {
release.URL = configv1.URL(urlString)
} else {
errs = append(errs, fmt.Errorf("invalid release URL: %s", err))
}
} else {
errs = append(errs, fmt.Errorf("URL is not a string: %v", urlInterface))
}
cvoUpdate.URL = configv1.URL(urlString)
}
if arch, ok := update.Metadata["release.openshift.io/architecture"]; ok {
switch arch {
case "multi":
cvoUpdate.Architecture = configv1.ClusterVersionArchitectureMulti
default:
klog.Warningf("Unrecognized release.openshift.io/architecture value %q", arch)
if archInterface, hasArch := metadata["release.openshift.io/architecture"]; hasArch {
if arch, isString := archInterface.(string); isString {
if arch == "multi" {
release.Architecture = configv1.ClusterVersionArchitectureMulti
} else {
errs = append(errs, fmt.Errorf("unrecognized release.openshift.io/architecture value %q", arch))
}
} else {
errs = append(errs, fmt.Errorf("release.openshift.io/architecture is not a string: %v", archInterface))
}
}
if channels, ok := update.Metadata["io.openshift.upgrades.graph.release.channels"]; ok {
cvoUpdate.Channels = strings.Split(channels, ",")
sort.Strings(cvoUpdate.Channels)
if channelsInterface, hasChannels := metadata["io.openshift.upgrades.graph.release.channels"]; hasChannels {
if channelsString, isString := channelsInterface.(string); isString {
if len(channelsString) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to add unit tests to cover these fancy corner cases, to avoid regression?

errs = append(errs, fmt.Errorf("io.openshift.upgrades.graph.release.channels is an empty string"))
} else {
channels := strings.Split(channelsString, ",")
if len(channels) == 0 {
errs = append(errs, fmt.Errorf("no comma-delimited channels in io.openshift.upgrades.graph.release.channels %q", channelsString))
} else {
for i := len(channels) - 1; i >= 0; i-- {
if len(channels[i]) == 0 {
errs = append(errs, fmt.Errorf("io.openshift.upgrades.graph.release.channels entry %d is an empty string: %q", i, channelsString))
channels = append(channels[:i], channels[i+1:]...)
}
}
if len(channels) == 0 {
errs = append(errs, fmt.Errorf("no non-empty channels in io.openshift.upgrades.graph.release.channels %q", channels))
} else {
sort.Strings(channels)
release.Channels = channels
}
}
}
} else {
errs = append(errs, fmt.Errorf("io.openshift.upgrades.graph.release.channels is not a string: %v", channelsInterface))
}
}
return cvoUpdate, nil
klog.V(2).Infof("parsed metadata: URL %q, architecture %q, channels %v, errors %v", release.URL, release.Architecture, release.Channels, errs)
return release, errors.Join(errs...)
}
4 changes: 2 additions & 2 deletions pkg/cincinnati/cincinnati_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,7 +816,7 @@ func Test_nodeUnmarshalJSON(t *testing.T) {
exp: node{
Version: semver.MustParse("4.0.0-5"),
Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-5",
Metadata: map[string]string{},
Metadata: map[string]interface{}{},
},
}, {
raw: []byte(`{
Expand All @@ -829,7 +829,7 @@ func Test_nodeUnmarshalJSON(t *testing.T) {
exp: node{
Version: semver.MustParse("4.0.0-0.1"),
Image: "quay.io/openshift-release-dev/ocp-release:4.0.0-0.1",
Metadata: map[string]string{
Metadata: map[string]interface{}{
"description": "This is the beta1 image based on the 4.0.0-0.nightly-2019-01-15-010905 build",
},
},
Expand Down
19 changes: 18 additions & 1 deletion pkg/cvo/cvo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -918,7 +918,24 @@ func mergeReleaseMetadata(release configv1.Release, getAvailableUpdates func() *
}
}
}
if update != nil {
if update == nil {
var versionMatch *configv1.Release
if availableUpdates.Current.Version == merged.Version {
versionMatch = &availableUpdates.Current
} else {
for i, u := range availableUpdates.Updates {
if u.Version == merged.Version {
versionMatch = &availableUpdates.Updates[i]
break
}
}
}
if versionMatch == nil {
klog.V(2).Infof("No available update found matching the digest of %q or the version %q", merged.Image, merged.Version)
} else {
klog.V(2).Infof("No available update found matching the digest of %q, although there was a match for version %q with a different tag or digest %q", merged.Image, merged.Version, versionMatch.Image)
}
} else {
if merged.Version == "" {
merged.Version = update.Version
}
Expand Down
41 changes: 5 additions & 36 deletions pkg/payload/payload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@ import (
"os"
"path/filepath"
"runtime"
"sort"
"strings"
"time"

"k8s.io/klog/v2"
Expand All @@ -27,6 +25,7 @@ import (
"github.com/openshift/cluster-version-operator/lib/capability"
localmanifest "github.com/openshift/cluster-version-operator/lib/manifest"
"github.com/openshift/cluster-version-operator/lib/resourceread"
"github.com/openshift/cluster-version-operator/pkg/cincinnati"
)

// State describes the state of the payload and alters
Expand Down Expand Up @@ -68,9 +67,6 @@ const (
// provide better visibility during install and upgrade of
// error conditions.
PrecreatingPayload

// releaseMultiArchID identifies a multi architecture release.
releaseMultiArchID = "multi"
)

// Initializing is true if the state is InitializingPayload.
Expand Down Expand Up @@ -368,39 +364,12 @@ func loadReleaseMetadata(releaseDir string) (configv1.Release, error) {
return release, fmt.Errorf("Cincinnati metadata version %q is not a valid semantic version: %v", metadata.Version, err)
}

release.Version = metadata.Version

if archRaw, hasArch := metadata.Metadata["release.openshift.io/architecture"]; hasArch {
arch, isString := archRaw.(string)
if !isString {
return release, fmt.Errorf("Architecture from %s (%s) is not a string: %v",
cincinnatiJSONFile, release.Version, archRaw)
}

if arch != releaseMultiArchID {
return release, fmt.Errorf("Architecture from %s (%s) contains invalid value: %q. Valid value is %q.",
cincinnatiJSONFile, release.Version, arch, releaseMultiArchID)
}

release.Architecture = configv1.ClusterVersionArchitectureMulti
klog.V(2).Infof("Architecture from %s (%s) is multi: %q", cincinnatiJSONFile, release.Version, arch)
release, err = cincinnati.ParseMetadata(metadata.Metadata)
if err != nil {
klog.Warningf("errors while parsing metadata from %s (%s): %v", cincinnatiJSONFile, release.Version, err)
}

if urlInterface, ok := metadata.Metadata["url"]; ok {
if urlString, ok := urlInterface.(string); ok {
release.URL = configv1.URL(urlString)
} else {
klog.Warningf("URL from %s (%s) is not a string: %v", cincinnatiJSONFile, release.Version, urlInterface)
}
}
if channelsInterface, ok := metadata.Metadata["io.openshift.upgrades.graph.release.channels"]; ok {
if channelsString, ok := channelsInterface.(string); ok {
release.Channels = strings.Split(channelsString, ",")
sort.Strings(release.Channels)
} else {
klog.Warningf("channel list from %s (%s) is not a string: %v", cincinnatiJSONFile, release.Version, channelsInterface)
}
}
release.Version = metadata.Version

return release, nil
}
Expand Down