Merge pull request #1231 from wking/log-release-metadata-parsing

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

Author: openshift-merge-bot[bot]

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/cvo/cvo.go

@@ -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
 				}

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
 }