modify to validate bundle versions differ by more than build metadata, or fail (#988)

Signed-off-by: Jordan Keister <[email protected]>

Author: grokspawn

alpha/veneer/semver/semver.go

@@ -13,6 +13,7 @@ import (
 	"github.com/operator-framework/operator-registry/alpha/declcfg"
 	"github.com/operator-framework/operator-registry/alpha/property"
 	"github.com/operator-framework/operator-registry/pkg/image"
+	"k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/yaml"
 )
@@ -140,18 +141,27 @@ func (sv *semverVeneer) getVersionsFromStandardChannels(cfg *declcfg.Declarative
 	if err != nil {
 		return nil, err
 	}
+	if err = validateVersions(&bdm); err != nil {
+		return nil, err
+	}
 	versions[candidateChannelName] = bdm
 
 	bdm, err = sv.getVersionsFromChannel(sv.Fast.Bundles, cfg)
 	if err != nil {
 		return nil, err
 	}
+	if err = validateVersions(&bdm); err != nil {
+		return nil, err
+	}
 	versions[fastChannelName] = bdm
 
 	bdm, err = sv.getVersionsFromChannel(sv.Stable.Bundles, cfg)
 	if err != nil {
 		return nil, err
 	}
+	if err = validateVersions(&bdm); err != nil {
+		return nil, err
+	}
 	versions[stableChannelName] = bdm
 
 	return &versions, nil
@@ -189,9 +199,6 @@ func (sv *semverVeneer) getVersionsFromChannel(semverBundles []semverVeneerBundl
 		if err != nil {
 			return nil, fmt.Errorf("bundle %q has invalid version %q: %v", b.Name, props.Packages[0].Version, err)
 		}
-		if len(v.Build) > 0 {
-			return nil, fmt.Errorf("bundle %q uses build metadata in its versioning, which is not sortable: %v", b.Name, v.String())
-		}
 
 		// package name detection
 		if sv.pkg != "" {
@@ -206,6 +213,10 @@ func (sv *semverVeneer) getVersionsFromChannel(semverBundles []semverVeneerBundl
 			sv.pkg = props.Packages[0].PackageName
 		}
 
+		if _, ok := entries[b.Name]; ok {
+			return nil, fmt.Errorf("duplicate bundle name %q", b.Name)
+		}
+
 		entries[b.Name] = v
 	}
 
@@ -255,7 +266,7 @@ func (sv *semverVeneer) generateChannels(semverChannels *semverRenderedChannelVe
 
 		// sort the bundle names according to their semver, so we can walk in ascending order
 		bundleNamesByVersion := []string{}
-		for b, _ := range bundles {
+		for b := range bundles {
 			bundleNamesByVersion = append(bundleNamesByVersion, b)
 		}
 		sort.Slice(bundleNamesByVersion, func(i, j int) bool {
@@ -434,3 +445,41 @@ func MermaidChannelWriter(cfg declcfg.DeclarativeConfig, out io.Writer) error {
 	}
 	return nil
 }
+
+func withoutBuildMetadataConflict(versions *map[string]semver.Version) error {
+	errs := []error{}
+
+	// using the stringified semver because the semver package generates deterministic representations,
+	// and because the semver.Version contains slice fields which make it unsuitable as a map key
+	//      stringified-semver.Version ==> incidence count
+	seen := make(map[string]int)
+	for b := range *versions {
+		stripped := stripBuildMetadata((*versions)[b])
+		if _, ok := seen[stripped]; !ok {
+			seen[stripped] = 1
+		} else {
+			seen[stripped] = seen[stripped] + 1
+			errs = append(errs, fmt.Errorf("bundle version %q cannot be compared to %q", (*versions)[b].String(), stripped))
+		}
+	}
+
+	if len(errs) != 0 {
+		return fmt.Errorf("encountered bundle versions which differ only by build metadata, which cannot be ordered: %v", errors.NewAggregate(errs))
+	}
+
+	return nil
+}
+
+func validateVersions(versions *map[string]semver.Version) error {
+	// short-circuit if empty, since that is not an error
+	if len(*versions) == 0 {
+		return nil
+	}
+	return withoutBuildMetadataConflict(versions)
+}
+
+// strips out the build metadata from a semver.Version and then stringifies it to make it suitable for collision detection
+func stripBuildMetadata(v semver.Version) string {
+	v.Build = nil
+	return v.String()
+}

alpha/veneer/semver/semver_test.go

@@ -581,6 +581,8 @@ func TestBailOnVersionBuildMetadata(t *testing.T) {
 				{Image: "repo/origin/a-v2.3.1"},
 				{Image: "repo/origin/a-v2.3.2"},
 				{Image: "repo/origin/a-v1.3.1-alpha"},
+				{Image: "repo/origin/a-v1.3.1-alpha+2001Jan21"},
+				{Image: "repo/origin/a-v1.3.1-alpha+2003May06"},
 			},
 		},
 	}
@@ -597,16 +599,18 @@ func TestBailOnVersionBuildMetadata(t *testing.T) {
 			{Schema: "olm.bundle", Image: "repo/origin/a-v0.1.1", Name: "a-v0.1.1", Properties: []property.Property{property.MustBuildPackage("a", "0.1.1")}},
 			{Schema: "olm.bundle", Image: "repo/origin/a-v1.1.0", Name: "a-v1.1.0", Properties: []property.Property{property.MustBuildPackage("a", "1.1.0")}},
 			{Schema: "olm.bundle", Image: "repo/origin/a-v1.2.1", Name: "a-v1.2.1", Properties: []property.Property{property.MustBuildPackage("a", "1.2.1")}},
+			{Schema: "olm.bundle", Image: "repo/origin/a-v1.3.1-alpha", Name: "a-v1.3.1-alpha", Properties: []property.Property{property.MustBuildPackage("a", "1.3.1-alpha")}},
 			{Schema: "olm.bundle", Image: "repo/origin/a-v1.3.1-alpha+2001Jan21", Name: "a-v1.3.1-alpha+2001Jan21", Properties: []property.Property{property.MustBuildPackage("a", "1.3.1-alpha+2001Jan21")}},
 			{Schema: "olm.bundle", Image: "repo/origin/a-v1.3.1", Name: "a-v1.3.1", Properties: []property.Property{property.MustBuildPackage("a", "1.3.1")}},
 			{Schema: "olm.bundle", Image: "repo/origin/a-v2.1.0", Name: "a-v2.1.0", Properties: []property.Property{property.MustBuildPackage("a", "2.1.0")}},
 			{Schema: "olm.bundle", Image: "repo/origin/a-v2.1.1", Name: "a-v2.1.1", Properties: []property.Property{property.MustBuildPackage("a", "2.1.1")}},
 			{Schema: "olm.bundle", Image: "repo/origin/a-v2.3.1", Name: "a-v2.3.1", Properties: []property.Property{property.MustBuildPackage("a", "2.3.1")}},
 			{Schema: "olm.bundle", Image: "repo/origin/a-v2.3.2", Name: "a-v2.3.2", Properties: []property.Property{property.MustBuildPackage("a", "2.3.2")}},
+			{Schema: "olm.bundle", Image: "repo/origin/a-v1.3.1-alpha+2003May06", Name: "a-v1.3.1-alpha+2003May06", Properties: []property.Property{property.MustBuildPackage("a", "1.3.1-alpha+2003May06")}},
 		},
 	}
 
-	t.Run("Abort on detected build metadata version data", func(t *testing.T) {
+	t.Run("Abort on unorderable build metadata version data", func(t *testing.T) {
 		_, err := sv.getVersionsFromStandardChannels(&dc)
 		require.Error(t, err)
 	})