pkg/cincinnati: Centralize release metadata parsing

Instead of carring similar code in the payload and Cincinnati
packages, centralize in one place to make maintenance easier.  And
with the logic centralized, I've also put some time into hardening the
channel parsing, to grumble about some possible issues.

For payload loading, errors get a logged warning, but are not fatal.
Having the CVO come up with logged warnings still allows cluster
admins to update into a fix.  But a crash-looping CVO would not notice
ClusterVersion spec.desiredUpdate changes or be able to roll out a
requested update into a fix.  So this branch processes as much of the
metadata as it can (to not let, for example, an invalid URL block us
from loading architecture information), logs the warning about what it
failed to parse, and continues on.

For Cincinnati processing, errors are fatal.  Cluster admins can take
the ClusterVersion RetrievedUpdates=False message and complain to
their Update Service maintainers, who can fix the metadata.

Author: wking

pkg/cincinnati/cincinnati.go

@@ -3,6 +3,7 @@ package cincinnati
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -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"`
 }
 
 type edge struct {
@@ -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()
+	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 {
+				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...)
 }

pkg/cincinnati/cincinnati_test.go

@@ -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(`{
@@ -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",
 			},
 		},

pkg/payload/payload.go

@@ -10,8 +10,6 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
-	"sort"
-	"strings"
 	"time"
 
 	"k8s.io/klog/v2"
@@ -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
@@ -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.
@@ -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
 }